privval: Switch to amino encoding in SignBytes #2459

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

@@ -108,7 +108,7 @@ func (vote *Vote) String() string {
 		vote.Height, vote.Round, vote.Type, typeString,
 		cmn.Fingerprint(vote.BlockID.Hash),
 		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

@@ -160,7 +160,13 @@ func (sc *SocketPV) SignVote(chainID string, vote *types.Vote) error {
 		return err
 	}

-	*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

@@ -179,8 +185,13 @@ func (sc *SocketPV) SignProposal(
 	if err != nil {
 		return err
 	}
-
-	*proposal = *res.(*SignProposalMsg).Proposal
+	resp := *res.(*SignedProposalResponse)


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

privval/socket.go

@@ -199,8 +210,14 @@ func (sc *SocketPV) SignHeartbeat(
 	if err != nil {
 		return err
 	}
-
-	*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

 	Vote *types.Vote
 }

-// 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

@@ -23,7 +23,7 @@ type Heartbeat struct {
 // SignBytes returns the Heartbeat bytes for signing.
 // 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!

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

privval: Switch to amino encoding in SignBytes #2459

privval: Switch to amino encoding in SignBytes #2459

liamsi commented Sep 21, 2018 •

edited by xla

codecov-io commented Sep 21, 2018 •

edited

liamsi commented Sep 21, 2018

xla commented Sep 21, 2018

liamsi Sep 21, 2018

liamsi Sep 21, 2018

liamsi Sep 21, 2018

liamsi Sep 21, 2018

xla left a comment

melekes Sep 22, 2018

liamsi Sep 22, 2018

melekes Sep 22, 2018

melekes Sep 22, 2018

liamsi commented Sep 22, 2018

liamsi commented Sep 22, 2018 •

edited

jleni Sep 25, 2018

ebuchman Sep 28, 2018

ebuchman Sep 28, 2018

ebuchman Sep 28, 2018

ebuchman Sep 28, 2018

privval: Switch to amino encoding in SignBytes #2459

privval: Switch to amino encoding in SignBytes #2459

Conversation

liamsi commented Sep 21, 2018 • edited by xla

codecov-io commented Sep 21, 2018 • edited

Codecov Report

liamsi commented Sep 21, 2018

xla commented Sep 21, 2018

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

xla 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

liamsi commented Sep 22, 2018

liamsi commented Sep 22, 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

liamsi commented Sep 21, 2018 •

edited by xla

codecov-io commented Sep 21, 2018 •

edited

liamsi commented Sep 22, 2018 •

edited