types: CommitVotes struct as last step towards #1648 by ebuchman · Pull Request #3298 · tendermint/tendermint

ebuchman · 2019-02-12T00:11:46Z

I believe this is the last commit necessary for #1648 before we make the actual breaking change.

That change will add Height and Round fields directly in the Commit and change the CommitSig to be a struct with just Timestamp and Signature.

Wahoo!

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

- new CommitVotes struct holds both the Commit and the ValidatorSet and implements VoteSetReader - ToVote takes a ValidatorSet

melekes

🏎

melekes · 2019-02-12T05:51:52Z

types/block_test.go:211:48: invalid operation: commit (variable of type *Commit) has no field or method GetByIndex (typecheck)
	assert.Equal(t, voteSet.GetByIndex(0), commit.GetByIndex(0))

ebuchman

Still need to fix the tests :)

consensus/reactor.go

types/block.go

ebuchman · 2019-02-13T10:46:00Z

Wow, integration test caught a bug. We should write a unit test to catch it too - basically need to be able to validate commits where some of the precommits aren't for the committed BlockID; up to 1/3 of them could be for nil or even for a different blockID!

This means we need the CommitSig to indicate which BlockID it signed!

Alternatively, we could have the Commit contain two lists - one for those that signed the commit.BlockID, and one for those that did not

Commits may include votes for a different BlockID, could be nil, or different altogether. This means we can't use `commit.BlockID` for reconstructing the sign bytes, since up to -1/3 of the commits might be for independent BlockIDs. This means CommitSig will need to include an indicator for what BlockID it signed - if it's not the committed one or nil, it will need to include it fully in order to be verified. This is unfortunate but unavoidable so long as we include votes for non-committed BlockIDs (which we do to track validator liveness)

codecov-io · 2019-02-13T10:54:16Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@7b162f5). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             develop    #3298   +/-   ##
==========================================
  Coverage           ?   64.06%           
==========================================
  Files              ?      213           
  Lines              ?    17495           
  Branches           ?        0           
==========================================
  Hits               ?    11208           
  Misses             ?     5353           
  Partials           ?      934

Impacted Files	Coverage Δ
consensus/state.go	`79.38% <0%> (ø)`

consensus/reactor.go

melekes · 2019-03-26T16:37:24Z

What's left here? Adding a sleep #3298 (comment)?

ebuchman · 2019-03-26T23:04:17Z

What's left here?

Write tests for the current code (not the code in this PR) for the case where the block includes commits for nil or for the wrong block ID (this is supposed to be allowed, as long as +2/3 of the votes are for the right block ID) - types: test block commits with votes for the wrong blockID #3484
Answer consensus: Do we need to allow votes in the commit for the wrong block ID? #3485 and change the block protocol accordingly. This should cause tests from (1) to fail, which should thus be updated.
Update this PR according to the outcome of (2)
Update the ADR-25 as necessary.

Of course updating the ADR should probably come first, and we probably actually need a new/better process altogether for making breaking changes to the block protocol (as opposed to architectural changes, which is what ADRs are for), but that's a separate issue.

If #3485 decides we do need to include nil votes (presumably we won't need votes for the explicitly wrong blockID), then we need to determine how to track whether a vote is for the blockID or for nil. I see roughly three approaches:

Don't track it, and when we verify the signature, if it fails for the right blockID then try verifying it against a nil vote. This requires no changes but is kind of hacky.
Include a bool in the CommitSig, true if the vote is for the blockID, false if it's for nil
Add an array to the Commit that is a list of validator indices that signed nil. If all votes are for the blockID, this would be empty

ebuchman · 2019-03-26T23:06:42Z

Oh whoops, we don't actually need to solve all that before merging this. We can move forward with this PR and deal with the final changes in a new one :D

ebuchman · 2019-03-26T23:07:12Z

What's left here? Adding a sleep #3298 (comment)?

So yes, that's probably the only thing left here!

Thanks @melekes

ebuchman · 2019-04-26T04:48:43Z

Ok based on #3596 I think we want the CommitSig to have the ValidatorAddress, which simplifies this PR quite a bit.

ebuchman

More feedback. Will fix

types/block.go

liamsi

The changes look good to me! Happy to help out with follow-up work in a separate PR

liamsi · 2019-04-30T09:57:18Z

types/block.go

 	return &v
 }

+//-------------------------------------


Potentially unpopular opinion: can we structure the code via files (and packages) instead of these ascii line separators? (Not blocking this PR)

liamsi · 2019-04-30T10:13:18Z

types/block.go


+// Construct a VoteSet from the Commit and validator set. Panics
+// if precommits from the commit can't be added to the voteset.
+func CommitToVoteSet(chainID string, commit *Commit, vals *ValidatorSet) *VoteSet {


These methods probably deserve tests to not decrease our test-coverage (?)

Yes you're right.

liamsi · 2019-04-30T10:14:14Z

docs/architecture/adr-025-commit.md

+const (
+	BlockIDFlagAbsent BlockIDFlag = iota // vote is not included in the Commit.Precommits
+	BlockIDFlagCommit                    // voted for the Commit.BlockID
+	BlockIDFlagNil                       // voted for nil


liamsi · 2019-04-30T10:16:38Z

docs/architecture/adr-025-commit.md

-to:
+Note the need for an extra byte to indicate whether the signature is for the BlockID or for nil.
+This byte can also be used to indicate an absent vote, rather than using a nil object like we currently do,
+which has been [problematic for Amino](https://github.com/tendermint/go-amino/issues/260).


It rather is problematic for protobuf compatibility (but I think the linked issue is clear about this).

ebuchman · 2019-05-05T15:50:31Z

Ok added a test and fixed the note about amino

liamsi

Thanks 👍

…rmint#3298) * VoteSignBytes builds CanonicalVote * CommitVotes implements VoteSetReader - new CommitVotes struct holds both the Commit and the ValidatorSet and implements VoteSetReader - ToVote takes a ValidatorSet * fix TestCommit * use CommitSig.BlockID Commits may include votes for a different BlockID, could be nil, or different altogether. This means we can't use `commit.BlockID` for reconstructing the sign bytes, since up to -1/3 of the commits might be for independent BlockIDs. This means CommitSig will need to include an indicator for what BlockID it signed - if it's not the committed one or nil, it will need to include it fully in order to be verified. This is unfortunate but unavoidable so long as we include votes for non-committed BlockIDs (which we do to track validator liveness) * fixes from review * remove CommitVotes. CommitSig contains address * remove commit.canonicalVote method * toVote -> getVote, takes valIdx * update adr-025 * commit.ToVoteSet -> CommitToVoteSet * add test * fix from review

VoteSignBytes builds CanonicalVote

99dc83c

ebuchman requested a review from melekes as a code owner February 12, 2019 00:11

CommitVotes implements VoteSetReader

86106e7

- new CommitVotes struct holds both the Commit and the ValidatorSet and implements VoteSetReader - ToVote takes a ValidatorSet

melekes approved these changes Feb 12, 2019

View reviewed changes

fix TestCommit

e8ebddc

ebuchman commented Feb 12, 2019

View reviewed changes

consensus/reactor.go Outdated Show resolved Hide resolved

types/block.go Outdated Show resolved Hide resolved

fixes from review

c7f9e2e

melekes reviewed Feb 18, 2019

View reviewed changes

consensus/reactor.go Outdated Show resolved Hide resolved

ebuchman mentioned this pull request Mar 17, 2019

Change Commit to consist of just signatures #1648

Closed

ebuchman mentioned this pull request Mar 26, 2019

types: test block commits with votes for the wrong blockID #3484

Closed

ebuchman mentioned this pull request Mar 26, 2019

consensus: Do we need to allow votes in the commit for the wrong block ID? #3485

Closed

xla changed the title ~~CommitVotes struct as last step towards #1648~~ types: CommitVotes struct as last step towards #1648 Mar 27, 2019

melekes assigned ebuchman Apr 23, 2019

ebuchman mentioned this pull request Apr 26, 2019

consensus reactor: making votes from commit #3596

Closed

ebuchman requested a review from xla as a code owner April 26, 2019 04:35

remove CommitVotes. CommitSig contains address

cc40f2f

remove commit.canonicalVote method

325d463

ebuchman commented Apr 26, 2019

View reviewed changes

types/block.go Outdated Show resolved Hide resolved

types/block.go Outdated Show resolved Hide resolved

ebuchman added 3 commits April 26, 2019 18:49

toVote -> getVote, takes valIdx

6a589fd

Merge branch 'develop' into bucky/1648-commit-votes

715fa1d

update adr-025

c1b9847

commit.ToVoteSet -> CommitToVoteSet

6635c13

liamsi approved these changes Apr 30, 2019

View reviewed changes

ebuchman added 2 commits May 5, 2019 11:49

add test

981c574

fix from review

4656020

liamsi approved these changes May 5, 2019

View reviewed changes

liamsi merged commit 4905640 into develop May 5, 2019

ebuchman deleted the bucky/1648-commit-votes branch May 5, 2019 16:20

melekes mentioned this pull request May 7, 2019

v0.31.6 release preview #3637

Closed

36 tasks

melekes mentioned this pull request May 30, 2019

temporary #3684

Closed

44 tasks

ebuchman mentioned this pull request Nov 21, 2019

types: change Commit to consist of just signatures #4146

Merged

5 tasks

Conversation

ebuchman commented Feb 12, 2019

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Feb 12, 2019

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ebuchman commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

melekes commented Mar 26, 2019

Uh oh!

ebuchman commented Mar 26, 2019

Uh oh!

ebuchman commented Mar 26, 2019

Uh oh!

ebuchman commented Mar 26, 2019

Uh oh!

ebuchman commented Apr 26, 2019

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

liamsi Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ebuchman May 3, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ebuchman May 3, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ebuchman commented May 5, 2019

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ebuchman commented Feb 13, 2019 •

edited

Loading

codecov-io commented Feb 13, 2019 •

edited

Loading