[CON-76] fix: sei-tendermint: include all fields in CommitHash#2600
[CON-76] fix: sei-tendermint: include all fields in CommitHash#2600
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2600 +/- ##
==========================================
+ Coverage 42.01% 43.22% +1.20%
==========================================
Files 1518 1857 +339
Lines 119497 153094 +33597
==========================================
+ Hits 50207 66169 +15962
- Misses 64906 81008 +16102
- Partials 4384 5917 +1533
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
4a28386 to
f0e8d54
Compare
udpatil
left a comment
There was a problem hiding this comment.
Overall the change looks good. However, when we actually roll it out via upgrade, how do we expect that it will behave changing the hashing algo across the blocks (eg. if upgrade is on height 100, will the last commit hash include these fields or not?
|
|
||
| // Hash returns the hash of the commit | ||
| // Hash returns the hash of the commit. | ||
| // It computes a Merkle tree from all commit fields: Height, Round, BlockID, and Signatures. |
There was a problem hiding this comment.
nit: we're not computing a merkle tree here? just concating the fields to the hash
There was a problem hiding this comment.
The hash is the root of the merkle tree, as far as I understand the implementation of HashFromByteSlices.
|
@stevenlanders and I will discuss and perform upgrade testing for this, after which we can land this change. |
f0e8d54 to
7d9bf23
Compare
7d9bf23 to
9c1de29
Compare
Good question! The expectation is that if height 100 is finalized with the old rules, then the block at height 101's LastCommitHash (which points to block at height 100), will be the first one created by the new rules. This works because the block at height 101 will be created with the new version. |
|
Testing update: @stevenlanders and I confirmed that:
|
…grade The Commit.Hash() algorithm changed in CON-76 (#2600) to include Height, Round, and BlockID in the Merkle tree alongside signatures. Blocks stored by v6.3 nodes have LastCommitHash computed with the old signatures-only algorithm. When v6.4 loads these blocks, ValidateBasic recomputes the hash with the new algorithm, causing a mismatch panic. Add Commit.LegacyHash() that uses the old algorithm, and fall back to it in ValidateBasic when the new hash doesn't match. Co-authored-by: Masih H. Derkani <m@derkani.org>
The Commit.Hash() algorithm was changed in CON-76 (#2600) to include Height, Round, and BlockID in the Merkle tree alongside signatures. Blocks stored by v6.3 nodes have LastCommitHash computed with the old signatures-only algorithm. When v6.4 loads these blocks during ABCI handshake replay, ValidateBasic recomputes the hash with the new algorithm, producing a different value and causing a panic first observed on `arctic-1`. Adds Commit.LegacyHash() that uses the old (signatures-only) algorithm, and updates Block.ValidateBasic() to fall back to it when the new hash doesn't match. Note that release 6.5 would make this fallback path entirely redundant, and it should be removed in the next release.
The Commit.Hash() algorithm was changed in CON-76 (#2600) to include Height, Round, and BlockID in the Merkle tree alongside signatures. Blocks stored by v6.3 nodes have LastCommitHash computed with the old signatures-only algorithm. When v6.4 loads these blocks during ABCI handshake replay, ValidateBasic recomputes the hash with the new algorithm, producing a different value and causing a panic first observed on `arctic-1`. Adds Commit.LegacyHash() that uses the old (signatures-only) algorithm, and updates Block.ValidateBasic() to fall back to it when the new hash doesn't match. Note that release 6.5 would make this fallback path entirely redundant, and it should be removed in the next release. Separated out from #3095 to capture a clear commit history of changes for the future archeologists.
The Commit.Hash() algorithm was changed in CON-76 (#2600) to include Height, Round, and BlockID in the Merkle tree alongside signatures. Blocks stored by v6.3 nodes have LastCommitHash computed with the old signatures-only algorithm. When v6.4 loads these blocks during ABCI handshake replay, ValidateBasic recomputes the hash with the new algorithm, producing a different value and causing a panic first observed on `arctic-1`. Adds Commit.LegacyHash() that uses the old (signatures-only) algorithm, and updates Block.ValidateBasic() to fall back to it when the new hash doesn't match. Note that release 6.5 would make this fallback path entirely redundant, and it should be removed in the next release. Separated out from #3095 to capture a clear commit history of changes for the future archeologists. (cherry picked from commit 512976d)
Describe your changes and provide context
Pretty much as in the title -- the CommitHash now covers all fields.
Testing performed to validate your change
Added a few new tests,
make testpasses.