ValidatorSet change delayed by 1 block, and lite refactor (#2)#1815
ValidatorSet change delayed by 1 block, and lite refactor (#2)#1815
Conversation
|
LGTM Not sure why the test_p2p is failing here but not develop - looks like it has to do with ABCI getting merged in but it's trying to install it from an external repo! @xla @melekes Need to update the changelog - this is breaking for:
|
wal tests are failing. error messages can definitely be improved |
1e1bfb2 to
f5d0b88
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1815 +/- ##
==========================================
Coverage ? 60.89%
==========================================
Files ? 193
Lines ? 15802
Branches ? 0
==========================================
Hits ? 9623
Misses ? 5370
Partials ? 809
|
f5d0b88 to
b51ed13
Compare
|
Merged in develop post v0.22.0 launch. Will merge this back to develop today and prepare for v0.23.0. Whoo! |
| err := ic.useClosestTrust(fc.Height()) | ||
| if err != nil { | ||
| // Get the next validator set. | ||
| nextValset, err := ic.source.ValidatorSet(ic.chainID, shdr.Height+1) |
There was a problem hiding this comment.
Don't understand the purpose of this line.
There was a problem hiding this comment.
Once you've certified a header, you want to also fetch the next validator set. the next validator set (hash) is in the block header, which we can do because we delay validator set changes by 1 block (this PR).
lite/inquiring_certifier.go
Outdated
| // best match trusted full commit, and if good, persist to ic.trusted. | ||
| // Returns ErrTooMuchChange when >2/3 of tfc did not sign sfc. | ||
| // Panics if tfc.Height() >= sfc.Height(). | ||
| func (ic *InquiringCertifier) verifyAndSave(tfc, sfc FullCommit) error { |
There was a problem hiding this comment.
Maybe use longer names for parameters.
lite/inquiring_certifier.go
Outdated
| FOR_LOOP: | ||
| for { | ||
| // Fetch latest full commit from trusted. | ||
| tfc, err := ic.trusted.LatestFullCommit(ic.chainID, 1, h) |
There was a problem hiding this comment.
Latest trusted full commit is known at this point as it has already been fetched.
Why we don't pass it as a parameter to this function?
There was a problem hiding this comment.
TODO make issue and assign to Jae
lite/inquiring_certifier.go
Outdated
| } | ||
|
|
||
| // Try to update to full commit with checks. | ||
| err = ic.verifyAndSave(tfc, sfc) |
There was a problem hiding this comment.
If I am not wrong this method does not return ErrTooMuchChange.
There was a problem hiding this comment.
Hmm wonder why the tests didn't catch it. I'll fix and test.
There was a problem hiding this comment.
TODO make issue and assign to Jae
| // we can handle IsTooMuchChangeErr specially | ||
| if !liteErr.IsTooMuchChangeErr(err) { | ||
| return err | ||
| FOR_LOOP: |
There was a problem hiding this comment.
This loop can probably be rewritten to reduce number of LastCommit fetches.
There was a problem hiding this comment.
Saving optimizations for later. This one isn't too bad, still has the O() property we want, though the whole scheme could improve w/ Tendermint streaming memoized valset changes to the lite client (rather than any binary search).
| ) | ||
|
|
||
| func ValidateBlockMeta(meta *types.BlockMeta, check lite.Commit) error { | ||
| func ValidateBlockMeta(meta *types.BlockMeta, sh types.SignedHeader) error { |
There was a problem hiding this comment.
Proxy package does not seem to be in finished state.
milosevic
left a comment
There was a problem hiding this comment.
I have finished review of lite package. There are lot of good code, concepts are cleaner than before, but there are still few open questions and possibility for code improvement. I recommend to extract parts of clean code (in smaller) chunks from this code and to go with normal development process with it.
|
Linter failed because amino is pointing to a branch rather than a release ... |
…LightTrusting` (tendermint#1806) (tendermint#1815) * Introduce `countAllSignatures` in `VerifyCommitLight` & `VerifyCommitLightTrusting` * Revert unneded change * Addressed @insumity's comments --------- Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
Rebase of #1756. Addresses #1313