privval: Switch to amino encoding in SignBytes by liamsi · Pull Request #2459 · tendermint/tendermint

liamsi · 2018-09-21T12:06:36Z

will resolve #2455

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

- currently only Vote is done

codecov-io · 2018-09-21T12:11:37Z

Codecov Report

Merging #2459 into develop will increase coverage by 0.02%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #2459      +/-   ##
===========================================
+ Coverage    61.72%   61.74%   +0.02%     
===========================================
  Files          197      198       +1     
  Lines        16478    16380      -98     
===========================================
- Hits         10171    10114      -57     
+ Misses        5447     5440       -7     
+ Partials       860      826      -34

Impacted Files	Coverage Δ
privval/socket.go	`75.8% <100%> (+4.31%)`	⬆️
privval/priv_validator.go	`71.02% <66.66%> (+1.57%)`	⬆️
lite/dbprovider.go	`59.18% <0%> (-18.37%)`	⬇️
libs/bech32/bech32.go	`42.85% <0%> (-10.99%)`	⬇️
lite/dynamic_verifier.go	`56.25% <0%> (-5.94%)`	⬇️
libs/autofile/autofile.go	`74.64% <0%> (-4.3%)`	⬇️
p2p/peer.go	`58.64% <0%> (-4.09%)`	⬇️
cmd/tendermint/commands/lite.go	`13.15% <0%> (-2.23%)`	⬇️
consensus/wal.go	`61.24% <0%> (-1.56%)`	⬇️
consensus/reactor.go	`72.1% <0%> (-1.03%)`	⬇️
... and 20 more

- add error description on error

liamsi · 2018-09-21T14:31:01Z

@xla: I've added Errors to the reply messages such that the remote validator can add those; but would it be OK to add proper error handling (and a bunch of known error codes) in a followup PR?

xla · 2018-09-21T14:37:38Z

@liamsi Of course!

liamsi · 2018-09-21T15:46:00Z

privval/socket.go

+}
+
+// Error allows (remote) validators to include meaningful error descriptions in their reply.
+type Error struct {


s/Error/RemoteError or RemoteSignerError

liamsi · 2018-09-21T15:48:39Z

types/vote.go

 		cmn.Fingerprint(vote.Signature),
-		CanonicalTime(vote.Timestamp))
+		vote.Timestamp)
 }


make this consistently formatted (each new line, or pairs were it makes sense)

liamsi · 2018-09-21T15:54:33Z

privval/socket.go

-// SignProposalMsg is a PrivValidatorSocket message containing a Proposal.
-type SignProposalMsg struct {
+// SignVoteReply is a PrivValidatorSocket message containing a signed vote along with a potenial error message.
+type SignedVoteReply struct {


change to Request/Response instead.

liamsi · 2018-09-21T15:56:04Z

privval/socket.go

-			res = &SignProposalMsg{r.Proposal}
-		case *SignHeartbeatMsg:
+			if err != nil {
+				res = &SignedProposalReply{nil, &Error{0, err.Error()}}


this decreased the test-coverage ... add tests for these new branches too

- contains all changes besides the test-coverage / error'ing branches

- add tests for each newly introduced error'ing code path

xla

👏 Would be nice to add a changelog entry and review relevant docs.

melekes · 2018-09-22T06:41:16Z

privval/socket.go

 	}

-	*vote = *res.(*SignVoteMsg).Vote
+	resp := *res.(*SignedVoteResponse)


what if it's not SignedVoteResponse?

This wasn't considered previously either, but yeah good catch! Will return an error in that case (and add a test-case). Thx.

melekes · 2018-09-22T06:41:33Z

privval/socket.go

 	}
-
-	*proposal = *res.(*SignProposalMsg).Proposal
+	resp := *res.(*SignedProposalResponse)


melekes · 2018-09-22T06:44:03Z

privval/socket.go

 	}
-
-	*heartbeat = *res.(*SignHeartbeatMsg).Heartbeat
+	resp := *res.(*SignedHeartbeatResponse)


liamsi · 2018-09-22T14:38:54Z

addressed @melekes comments and updated changelog / docs (@xla)

liamsi · 2018-09-22T14:59:03Z

TestWaitChan seems to fail non-deterministically (unrelated to the changes in that commit, which are only in *.md files). This seems to be known: #2227

jleni · 2018-09-25T11:11:13Z

types/canonical.go

+	Round     int              `json:"round"`
+	Timestamp time.Time        `json:"timestamp"`
+	VoteType  byte             `json:"type"`
+}


I understand that this might be a first step towards solving #1622

This is very different from the agreed schema (#1622 (comment))

// vanilla protobuf / amino encoded message Vote { Version fixed32 Height sfixed64 Round sfixed32 VoteType fixed32 Timestamp Timestamp // << using protobuf definition BlockID BlockID // << as already defined ChainID string // at the end because length could vary a lot }

The order and fixed size types are important so some fields can be serialized without having to deal with amino. This has been extensively discussed in #1622

ebuchman · 2018-09-28T15:13:02Z

privval/socket.go


-// SignProposalMsg is a PrivValidatorSocket message containing a Proposal.
-type SignProposalMsg struct {
+// SignVoteReply is a PrivValidatorSocket message containing a signed vote along with a potenial error message.


SignedVoteResponse

ebuchman · 2018-09-28T15:18:38Z

types/heartbeat.go

 // It panics if the Heartbeat is nil.
 func (heartbeat *Heartbeat) SignBytes(chainID string) []byte {
-	bz, err := cdc.MarshalJSON(CanonicalHeartbeat(chainID, heartbeat))
+	bz, err := cdc.MarshalJSON(CanonicalizeHeartbeat(chainID, heartbeat))


MarshalBinary?

ebuchman · 2018-09-28T15:43:52Z

types/vote_test.go

-		// NOTE: when this fails, you probably want to fix up consensus/replay_test too
-		t.Errorf("Got unexpected sign string for Vote. Expected:\n%v\nGot:\n%v", expected, signStr)
-	}
+	expected, err := cdc.MarshalBinary(CanonicalizeVote("test_chain_id", vote))


It might be worth hard-coding a binary representation here as an extra assurance. Helps make sure we know exactly what this looks like for other implementers and in-case we ever break it!

Opened #2508 for this

Addressed!

…ermint#2459) (tendermint#2464) This is an automatic backport of pull request tendermint#2459 done by [Mergify](https://mergify.com). --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…ermint#2459) (tendermint#2612) <hr>This is an automatic backport of pull request tendermint#2459 done by [Mergify](https://mergify.com). --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

switch to amino for SignBytes and add Reply with error message

ceb860c

- currently only Vote is done

liamsi requested review from ebuchman, melekes, xla and zramsay as code owners September 21, 2018 12:06

liamsi changed the base branch from master to develop September 21, 2018 12:06

liamsi force-pushed the mvp_amino_signbytes branch from 7c8375c to f7a38b3 Compare September 21, 2018 12:11

switch Reply type in socket for other messages

91e88bb

- add error description on error

xla changed the title ~~[WIP]: Switch to amino encoding in SignBytes~~ [WIP] privval: Switch to amino encoding in SignBytes Sep 21, 2018

xla added the T:validator Type: Validator related label Sep 21, 2018

xla assigned liamsi Sep 21, 2018

add TODOs regarding error handling

e4bd120

liamsi changed the title ~~[WIP] privval: Switch to amino encoding in SignBytes~~ privval: Switch to amino encoding in SignBytes Sep 21, 2018

liamsi mentioned this pull request Sep 21, 2018

Sign integration test & catch up with latest tm-core develop tendermint/tmkms#55

Merged

7 tasks

liamsi commented Sep 21, 2018

View reviewed changes

address comments from peer review session (thx @xla)

dfcb818

- contains all changes besides the test-coverage / error'ing branches

liamsi force-pushed the mvp_amino_signbytes branch from f7a38b3 to dfcb818 Compare September 21, 2018 17:05

increase test coverage:

3aa2914

- add tests for each newly introduced error'ing code path

liamsi force-pushed the mvp_amino_signbytes branch from 0258834 to 3aa2914 Compare September 21, 2018 21:07

xla previously requested changes Sep 21, 2018

View reviewed changes

melekes reviewed Sep 22, 2018

View reviewed changes

liamsi added 3 commits September 22, 2018 15:43

return error if received wrong response

942f2ea

add test for wrong response branches (ErrUnexpectedResponse)

0c4f564

update CHANGELOG_PENDING and related documentation (spec)

270f5f2

melekes approved these changes Sep 22, 2018

View reviewed changes

liamsi added 2 commits September 24, 2018 15:04

fix typo: s/CanonicallockID/CanonicalBlockID

e6eb400

Merge branch 'develop' into mvp_amino_signbytes

5829f33

jleni mentioned this pull request Sep 25, 2018

Why Vote.SignBytes is a JSON string? #1622

Closed

jleni reviewed Sep 25, 2018

View reviewed changes

ebuchman approved these changes Sep 28, 2018

View reviewed changes

ebuchman force-pushed the mvp_amino_signbytes branch from 414f396 to 8d99cb0 Compare September 28, 2018 23:50

ebuchman mentioned this pull request Sep 28, 2018

SignBytes Test: hard-code at least one encoding example for the SignBytes #2508

Closed

fixes from review

8d99cb0

ebuchman merged commit fc07374 into develop Sep 28, 2018

ebuchman deleted the mvp_amino_signbytes branch September 28, 2018 23:57

This was referenced Oct 5, 2018

Switch to amino encoding in SignBytes #2455

Closed

Use fixed lengths in and finalize sign bytes #2545

Closed

Conversation

liamsi commented Sep 21, 2018 • edited by xla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liamsi commented Sep 21, 2018

Uh oh!

xla commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi commented Sep 22, 2018

Uh oh!

liamsi commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liamsi commented Sep 21, 2018 •

edited by xla

Loading

codecov-io commented Sep 21, 2018 •

edited

Loading

liamsi commented Sep 22, 2018 •

edited

Loading