Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to the latest version of lazyledger-core #19

Closed
wants to merge 2 commits into from

Conversation

evan-forbes
Copy link
Member

Description

lazyledger-core has just had some minor dependency version bumps, so this PR updates the go.mod to the latest commit of the master branch. One of the updated dependencies is grpc v1.35.0, which causes a weird marshaling bug, so to avoid the bug this PR also adds a replace directive to force the sdk to use v1.33.2.

closes: #XXXX

Copy link
Member

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically you're updating grpc and downgrading it back (with replace) at the same commit?

@liamsi
Copy link
Member

liamsi commented Feb 25, 2021

Yes, see : celestiaorg/celestia-core#159 (comment) or Evan's opening comment. Reason is that we want a protobuf security fix while not caring about the grpc update (as it currently breaks stuff in the app). Also, the latest stable tendermint uses these versions, too: https://github.com/tendermint/tendermint/blob/28ce35565675fbb7cef202b283d65826bf514a71/go.mod#L31-L34

liamsi
liamsi previously approved these changes Feb 25, 2021
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 But we need to investigate the failing tests and appease the linter gods before merging.

@evan-forbes
Copy link
Member Author

Weirdly enough, the app works fine when v1.35.0 is used, however it introduces non-deterministic test failures in some of the modules' integration tests in the sdk. Even weirder, the tests that fail seem to fail consistently per machine, but which tests fail change per machine. 🤷‍♂️

The test that was failing appeared to be this. Re-running the CI fixes this.

The linter gods kindly never let us forget about the writing file permission issues that were discussed here. That writing function in the tmos library, mentioned in the linked comment, is not recognized by the linter, so it doesn't complain. That function was deleted in one of the tendermint version updates to lazyledger-core, so it was replaced and now we live under constant scrutiny from the linter gods.

I can make a new wrapper of ioutil to silence them, use different permissions, ignore the linter in this case, or we can remove the linter check somehow. Which sounds better?

@liamsi
Copy link
Member

liamsi commented Feb 25, 2021

Hmm, the plan is to use the latest stable tendermint version in lazyledger-core (0.34.x) and the latest stable sdk release: https://github.com/cosmos/cosmos-sdk/tree/v0.41.x

I think in both those version os package exists and is still used. TBH, I'm OK with ignoring those here (using a nolint directive). We should track this in an issue so we won't forget about it and we should also open an issue on the SDK side as the perms look dodgy!

@liamsi
Copy link
Member

liamsi commented Feb 25, 2021

Regarding the grpc story I trust Marko's call to replace it and also this seems to be the taken approach in the SDK.

@evan-forbes
Copy link
Member Author

evan-forbes commented Feb 25, 2021

The linter has been disabled, but I can also add the write function back to ll-core here

It's been removed in the master branch of tendermint, but is still there in v0.34.8

@evan-forbes evan-forbes reopened this Feb 25, 2021
@liamsi
Copy link
Member

liamsi commented Feb 25, 2021

Let's figure out the whole write function story together with bringing lazyledger-core more on par with the 0.34.x branch. We need an issue for the latter too. I thought I've already opened one but apparently not.

@evan-forbes
Copy link
Member Author

I'm opening one now with all the context

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Apr 12, 2021
@github-actions github-actions bot closed this Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants