[R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes #2598

liamsi · 2018-10-10T16:35:25Z

resolves #2545

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

liamsi · 2018-10-10T16:37:46Z

types/vote_test.go

+				0x11,                                   // (field_number << 3) | wire_type
+				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
+				// remaining fields:
+				0x22, 0x9, 0x9, 0x0, 0x9, 0x6e, 0x88, 0xf1, 0xff, 0xff, 0xff},


@jleni do this kind of tests tests suit your need for test-cases (as mentioned in #2545 (comment))?
I'll add a few more (also for the other types). I can remove the explanations/comments if you'd prefer sth you can easily copy&paste.

That is great. What about version number?
It would be good that it is there too. So breaking changes can be signaled and the HSM can reject signing.

btw, what checks would you recommend in HSM code? I am planning to check at least:

total length == blob size

version == whatever we agree on

field_number == 0x09 (or the corresponding value)

block number >= 0

field_number == 0x11 (or the corresponding value)

round number >= 0

Thanks! Let me sync up with @ebuchman or someone from the tm-core team about the version field.
The checks looks reasonable! I'll think about it and let you know if there should be more checks.

So, the story around the version field is that the version isn't fully integrated yet. I'll add the field to CanocialVote and could add it manually for a few test cases so it would unblock your work on the HSM parsing side.
@ebuchman does it make sense to change the Signable interface to take a version before we integrating the "Block Protocol Version"?

That is fine, what we have now is enough for me to continue. I can now use these offsets and layout, complete things and later adjust.
We will eventually need this field anyway.
Maybe it makes sense that you add it and set to zero for now, until the other code starts using it but I don't actually need this anyway.

Did you notice that fields are skipped whenever they are the default value? Does that pose a problem for you? I think you checks would look slightly different, e.g. if the first field is missing field_number == 0x09 would be false but field_number == 0x11 would be true (in case the second field isn't zero too etc).

Does that behaviour (skipping defaults) have an analog in proto3? Or is it amino specific?

@liamsi Yes, I am aware of that. No problem, I was planning to consider this case
@ebuchman AFAIK, it is the same in protobuf3

Does that behaviour (skipping defaults) have an analog in proto3? Or is it amino specific?

@jleni is right. It is the default behaviour in proto3, too. In amino this ought to be configurable via the write_empty tag, though: https://github.com/tendermint/go-amino/blob/faa6e731944e2b7b6a46ad202902851e8ce85bee/binary-encode.go#L404-L408
Surprisingly, this doesn't work here. If this isn't a problem while parsing, let's keep it that way. I'll investigate and open an issue in amino if necessary.

codecov-io · 2018-10-10T16:40:58Z

Codecov Report

Merging #2598 into develop will decrease coverage by <.01%.
The diff coverage is 75.67%.

@@             Coverage Diff             @@
##           develop    #2598      +/-   ##
===========================================
- Coverage    61.32%   61.31%   -0.01%     
===========================================
  Files          203      203              
  Lines        16779    16781       +2     
===========================================
  Hits         10289    10289              
- Misses        5619     5620       +1     
- Partials       871      872       +1

Impacted Files	Coverage Δ
lite/helpers.go	`80.76% <100%> (ø)`	⬆️
privval/priv_validator.go	`71.02% <50%> (ø)`	⬆️
consensus/reactor.go	`72.51% <68.42%> (+0.25%)`	⬆️
consensus/state.go	`75.64% <86.66%> (ø)`	⬆️
p2p/pex/pex_reactor.go	`74% <0%> (-0.67%)`	⬇️
p2p/pex/addrbook.go	`69.58% <0%> (-0.49%)`	⬇️
crypto/hash.go	`100% <0%> (ø)`	⬆️

liamsi · 2018-10-12T09:49:34Z

@jleni @ebuchman I think this is ready for review. Let me know if we need to add, or reorder anything. The Version field currently isn't set anywhere. It is merely a placeholder. As soon as we fully integrated the "Block Version", we can add it as a param to SignBytes.

The ADR needs to be updated / improved, too. I propose to do this in a separate PR / issue.

ebuchman · 2018-10-12T16:02:34Z

types/canonical.go

+	Height           int64                  `json:"height" binary:"fixed64"`
+	Round            int64                  `json:"round" binary:"fixed64"`
+	POLRound         int64                  `json:"pol_round" binary:"fixed64"`
+	Timestamp        time.Time              `json:"timestamp"`
 	ChainID          string                 `json:"@chain_id"`


Should we be consistent with putting the ChainID at the end?

Also, do we even JSON marshal this ever? If not, should we drop the JSON tags too?

Good point. Moved to the end.

Also, do we even JSON marshal this ever? If not, should we drop the JSON tags too?

No, I kept them in case someone wants to marshal them to JSON though. But yeah, it makes sense to delete those JSON tags and re-add them if we see a need.

ebuchman · 2018-10-12T16:18:44Z

types/canonical.go

 	ChainID          string                 `json:"@chain_id"`
-	Type             string                 `json:"@type"`


I guess one problem with removing type is that we have to be careful that eg. the encoding of a CanonicalProposal could not be mistaken for that of a CanonicalVote.

Are we certain that marhsalBinary(x CanonicalVote) != marshalBinary(y CanonicalProposal) ?

You are right. They would actually be the same if for instance the first 3 fields are equal (because of the same order and nothing else to differentiate between the two). I'll add a test-case and add back the type.

Maybe we can just use the existing VoteType byte ?

Instead of VoteType, could be SignedMsgType, and then we could add SignedMsgType Proposal = byte(0x10) for example.

So eg. Proposal/Vote would both start like

Version uint64 `binary:"fixed64"` Height int64 `binary:"fixed64"` Round int64 `binary:"fixed64"` Type byte

That's much better. Thanks for the thorough review and the great suggestions ❤️
This change will touch quite a lot of files though. I'll rebase (and resolve conflicts) and this should be ready for another review.

melekes

🥗 🔥 🌯

melekes · 2018-10-12T17:39:28Z

types/canonical.go

 		Height:           proposal.Height,
+		Round:            int64(proposal.Round), // cast int->int64 to make amino encode it fixed64 (does not work for int)


Should we standardize Round's type? make it uint16 or int32?

Can you open a new issue with the case for it?

melekes · 2018-10-12T17:40:34Z

types/vote_test.go

+		{
+			CanonicalizeVote("", &Vote{}),
+			// NOTE: Height and Round are skipped here. This case needs to be considered while parsing.
+			[]byte{0xb, 0x2a, 0x9, 0x9, 0x0, 0x9, 0x6e, 0x88, 0xf1, 0xff, 0xff, 0xff},


how do I get this something changes?

It's just the output of MarshalBinary. The test serves two purposes: 1) mainly documentation for people wanting to parse this without having to fully implement amino/proto3 (like @jleni) 2) get notified if the encoding changes for whatever reason.

ebuchman · 2018-10-12T20:27:57Z

types/canonical.go

@@ -27,6 +27,7 @@ type CanonicalProposal struct {
 	Height           int64  `binary:"fixed64"`
 	Round            int64  `binary:"fixed64"`
 	POLRound         int64  `binary:"fixed64"`
+	Type             string


#2598 (comment)

- it doesn't affect if default fixed size fields ((u)int64) are written or not - add comment about int->int64 casting

- SignedMsgType replaces VoteTypePrevote, VoteTypePrecommit and adds new ProposalType to separate votes from proposal when signed - update test-vectors

ebuchman

Thanks Ismail. I'll make the final adjustments.

ebuchman · 2018-10-12T22:28:16Z

types/canonical.go

+	Version          uint64 `binary:"fixed64"`
+	Height           int64  `binary:"fixed64"`
+	Round            int64  `binary:"fixed64"`
+	POLRound         int64  `binary:"fixed64"`


Shouldn't it be

Round Type POLRound

ebuchman · 2018-10-12T22:42:41Z

types/canonical.go

-	ValidatorIndex   int     `json:"validator_index"`
+	Version          uint64 `binary:"fixed64"`
+	Height           int64  `binary:"fixed64"`
+	Round            int    `binary:"fixed64"`


ebuchman · 2018-10-12T22:46:10Z

types/signed_msg_type.go

+	PrevoteType   SignedMsgType = 0x01
+	PrecommitType SignedMsgType = 0x02
+	// To separate (canonicalized) Vote and Proposal messages:
+	ProposalType SignedMsgType = 0x03


Let's distinguish categories of msg type like "vote-associated", "proposal-associated", "heartbeat-associated". So eg.

// Vote types PrevoteType SignedMsgType = 0x01 PrecommitType SignedMsgType = 0x02 // Proposal Types ProposalType SignedMsgType = 0x10 // Heartbeat Types HeartbeatType SignedMsgType = 0x20 ExampleNewHeartbeatType SignedMsgType = 0x21

ebuchman · 2018-10-12T22:56:57Z

FYI: #2626

Sorry I didn't open that sooner

liamsi · 2018-10-12T23:13:36Z

Thanks Ismail. I'll make the final adjustments.

Just saw this. I've added a commit that uses SignedMsgType in Vote and CanonicalVote to avoid casting to byte everywhere. I'm going to bed now. So no further commits from my side until tomorrow.

ebuchman · 2018-10-12T23:22:59Z

Oh, I changed the order of some fields in that commit and the tests didn't break. Could that be an issue?

Also, is byte fixed length?

liamsi · 2018-10-12T23:43:18Z

Oh, I changed the order of some fields in that commit and the tests didn't break. Could that be an issue?

That's probably because those where not included in the tests (because they are skipped if they are empty). The tests include only those fields which are relevant for the HSM (and currently only for Votes as requested per @jleni; Vote didn't change besides renaming VoteType -> Type).

Also, is byte fixed length?

Yes, amino treats byte as uint8 (calling EncodeUint8).

liamsi requested review from ebuchman, melekes and xla as code owners October 10, 2018 16:35

liamsi commented Oct 10, 2018

View reviewed changes

liamsi force-pushed the fixed_offset branch from a4b35cb to 806d815 Compare October 12, 2018 09:22

liamsi requested a review from zramsay as a code owner October 12, 2018 09:31

liamsi changed the title ~~[WIP] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes~~ [R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes Oct 12, 2018

liamsi force-pushed the fixed_offset branch from 4853ba7 to ec25edd Compare October 12, 2018 09:51

ebuchman approved these changes Oct 12, 2018

View reviewed changes

ebuchman reviewed Oct 12, 2018

View reviewed changes

melekes reviewed Oct 12, 2018

View reviewed changes

jackzampolin mentioned this pull request Oct 12, 2018

0.26.0 Release #2624

Closed

7 tasks

ebuchman reviewed Oct 12, 2018

View reviewed changes

liamsi added 12 commits October 12, 2018 23:56

WIP: switching to fixed offsets for SignBytes

ebf5f6c

add version field to sign bytes and update order

2b5b782

more comments on test-cases and add a tc with a chainID

a737cc4

remove amino:"write_empty" tag

97b517c

- it doesn't affect if default fixed size fields ((u)int64) are written or not - add comment about int->int64 casting

update CHANGELOG_PENDING

4bb0856

update documentation

52dc9ea

add back link to issue #1622 in documentation

6760c86

remove JSON tags and add (failing test-case)

2efbdec

fix failing test

8a4b8fa

update test-vectors due to added Type field

ffa4977

change Type field from string to byte and add new type alias

e00847c

- SignedMsgType replaces VoteTypePrevote, VoteTypePrecommit and adds new ProposalType to separate votes from proposal when signed - update test-vectors

fix remains from rebasing

f8fdbc3

liamsi force-pushed the fixed_offset branch from 4298d9b to f8fdbc3 Compare October 12, 2018 22:16

ebuchman approved these changes Oct 12, 2018

View reviewed changes

ebuchman closed this Oct 12, 2018

ebuchman reopened this Oct 12, 2018

use SignMessageType instead of byte everywhere

32b8a85

fixes from review

283a357

ebuchman merged commit 8888595 into develop Oct 12, 2018

ebuchman deleted the fixed_offset branch October 12, 2018 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes #2598

[R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes #2598

liamsi commented Oct 10, 2018 •

edited

liamsi Oct 10, 2018 •

edited

jleni Oct 10, 2018

jleni Oct 10, 2018

liamsi Oct 10, 2018

liamsi Oct 11, 2018

jleni Oct 11, 2018

liamsi Oct 11, 2018

ebuchman Oct 11, 2018

jleni Oct 12, 2018

liamsi Oct 12, 2018

codecov-io commented Oct 10, 2018 •

edited

liamsi commented Oct 12, 2018 •

edited

ebuchman Oct 12, 2018

liamsi Oct 12, 2018

ebuchman Oct 12, 2018 •

edited

liamsi Oct 12, 2018

ebuchman Oct 12, 2018

ebuchman Oct 12, 2018

liamsi Oct 12, 2018 •

edited

melekes left a comment

melekes Oct 12, 2018

ebuchman Oct 12, 2018

melekes Oct 12, 2018

liamsi Oct 12, 2018

ebuchman Oct 12, 2018

ebuchman left a comment

ebuchman Oct 12, 2018

ebuchman Oct 12, 2018

ebuchman Oct 12, 2018

ebuchman commented Oct 12, 2018

liamsi commented Oct 12, 2018

ebuchman commented Oct 12, 2018

liamsi commented Oct 12, 2018 •

edited

		Height: proposal.Height,
		Round: int64(proposal.Round), // cast int->int64 to make amino encode it fixed64 (does not work for int)

[R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes #2598

[R4R] Fixed sized and reordered fields for Vote/Proposal/Heartbeat SignBytes #2598

Conversation

liamsi commented Oct 10, 2018 • edited

liamsi Oct 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 10, 2018 • edited

Codecov Report

liamsi commented Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 12, 2018 • edited

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Oct 12, 2018

liamsi commented Oct 12, 2018

ebuchman commented Oct 12, 2018

liamsi commented Oct 12, 2018 • edited

liamsi commented Oct 10, 2018 •

edited

liamsi Oct 10, 2018 •

edited

codecov-io commented Oct 10, 2018 •

edited

liamsi commented Oct 12, 2018 •

edited

ebuchman Oct 12, 2018 •

edited

liamsi Oct 12, 2018 •

edited

liamsi commented Oct 12, 2018 •

edited