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
Simplify proposal msg #2735
Simplify proposal msg #2735
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2735 +/- ##
===========================================
- Coverage 62.32% 62.29% -0.03%
===========================================
Files 211 211
Lines 17053 17029 -24
===========================================
- Hits 10628 10608 -20
+ Misses 5555 5552 -3
+ Partials 870 869 -1
|
a989354
to
fe04e5b
Compare
fe04e5b
to
3ff909b
Compare
types/canonical.go
Outdated
Height int64 `binary:"fixed64"` | ||
Round int64 `binary:"fixed64"` | ||
BlockID CanonicalBlockID | ||
POLRound int64 `binary:"fixed64"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the BlockID
is not of fixed size (?). For HSMs to be able to parse the proposals, it is important that we keep the fixed sized fields in front (Type, Height, Round, POLRound, and then, BlockID, Timestamp, ChainID).
3ff909b
to
e72d9e5
Compare
Height int64 `json:"height"` | ||
Round int `json:"round"` | ||
BlockID BlockID `json:"block_id"` | ||
POLRound int `json:"pol_round"` // -1 if null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the order here aligned with the Canonical order, so POLRound should come before BlockID
Looks good, also need to update the spec: https://github.com/tendermint/tendermint/blob/develop/docs/spec/reactors/consensus/consensus.md#proposal |
Fix #2646.
Note that we could easily add nice Proposal related tests after #2652 is merged.