Stop Signing over the PartSetHeader for Votes and remove it from the Header
#457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evan-forbes
commented
Jul 14, 2021
| @@ -63,23 +63,22 @@ message Header { | |||
|
|
|||
| // prev block info | |||
| BlockID last_block_id = 5 [(gogoproto.nullable) = false]; | |||
| PartSetHeader last_part_set_header = 6; | |||
There was a problem hiding this comment.
removal of the LastPartSetHeader field in the proto header
| @@ -35,5 +35,4 @@ message CanonicalVote { | |||
| CanonicalBlockID block_id = 4 [(gogoproto.customname) = "BlockID"]; | |||
| google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; | |||
| string chain_id = 6 [(gogoproto.customname) = "ChainID"]; | |||
| CanonicalPartSetHeader part_set_header = 7; | |||
There was a problem hiding this comment.
removal of the PartSetHeader from the CanonicalVote
There was a problem hiding this comment.
So the CanonicalPartSetHeader is still needed bc/ we have to sign over it in the proposal at least, right?
There was a problem hiding this comment.
@liamsi, yes. I double-checked and CanonicalPartSetHeader is only used in the canonical proposal.
Comment on lines
58
to
+65
| func CanonicalizeVote(chainID string, vote *tmproto.Vote) tmproto.CanonicalVote { | ||
| cppsh := CanonicalizePartSetHeader(*vote.PartSetHeader) | ||
| return tmproto.CanonicalVote{ | ||
| Type: vote.Type, | ||
| Height: vote.Height, // encoded as sfixed64 | ||
| Round: int64(vote.Round), // encoded as sfixed64 | ||
| BlockID: CanonicalizeBlockID(vote.BlockID), | ||
| Timestamp: vote.Timestamp, | ||
| ChainID: chainID, | ||
| PartSetHeader: &cppsh, | ||
| Type: vote.Type, | ||
| Height: vote.Height, // encoded as sfixed64 | ||
| Round: int64(vote.Round), // encoded as sfixed64 | ||
| BlockID: CanonicalizeBlockID(vote.BlockID), | ||
| Timestamp: vote.Timestamp, | ||
| ChainID: chainID, |
There was a problem hiding this comment.
the removal of the PartSetHeader in the CanonicalVote allows for the preservation of the PartSetHeader in tmproto.Vote
liamsi
reviewed
Jul 15, 2021
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, voteA.Signature, voteB.Signature) | ||
|
|
liamsi
reviewed
Jul 15, 2021
liamsi
approved these changes
Jul 19, 2021
Wondertan
approved these changes
Jul 21, 2021
evan-forbes
changed the base branch from
evan/decouple-blockID
to
evan/ismail/explore_blockid_partsetheader
Base automatically changed from
evan/ismail/explore_blockid_partsetheader
to
master
July 25, 2021 16:43
evan-forbes
force-pushed
the
evan/remove-lastpartsetheader-from-header
branch
from
da3d85d to
cbfed8c
Compare
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
==========================================
+ Coverage 62.77% 62.82% +0.04%
==========================================
Files 263 263
Lines 32526 32499 -27
==========================================
- Hits 20419 20417 -2
+ Misses 10555 10532 -23
+ Partials 1552 1550 -2
|
3 tasks
This was referenced Aug 23, 2021
evan-forbes
added a commit
that referenced
this pull request
Aug 25, 2021
* Revert "Remove the PartSetHeader from VoteSetMaj23, VoteSetBits, and state.State (#479)" This reverts commit 3ba47c2. * Revert "Stop Signing over the `PartSetHeader` for Votes and remove it from the `Header` (#457)" This reverts commit 818be04. * Revert "Decouple PartSetHeader from BlockID (#441)" This reverts commit 9d4265d.
evan-forbes
added a commit
that referenced
this pull request
Aug 25, 2021
* Revert "Remove the PartSetHeader from VoteSetMaj23, VoteSetBits, and state.State (#479)" This reverts commit 3ba47c2. * Revert "Stop Signing over the `PartSetHeader` for Votes and remove it from the `Header` (#457)" This reverts commit 818be04. * Revert "Decouple PartSetHeader from BlockID (#441)" This reverts commit 9d4265d. * Revert "Save and load block data using IPFS (#374)" This reverts commit 8da1644. * fix merge error * Revert "Add the ipfs dag api object in Blockstore (#356)" and get rid of embedded ipfs objects This reverts commit 40acb17. * remove ipfs node provider from new node * stop init ipfs repos * remove IPFS node config * clean up remaining ipfs stuff
evan-forbes
pushed a commit
that referenced
this pull request
Jun 9, 2023
Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.14.0 to 1.15.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.14.0...v1.15.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR builds off of #441 and continues the work of #418 by not including the
PartSetHeaderin theCanonicalVote. It also removes thePartSetHeaderfrom theHeader's hash by removing it entirely from bothtypes.Headerandtmproto.HeaderThis PR can be merged into #441 or master after #441 is merged, whichever is most convenient for the reviewers
Closes: #456