abci++: Vote extension cleanup by thanethomson · Pull Request #8402 · tendermint/tendermint

thanethomson · 2022-04-23T13:22:27Z

Follows from #8141. Relates to #8272, but does not close it.

I realized that we're signing prevotes' vote extensions too, which is incorrect (we should only be signing and verifying/validating precommits' vote extensions).

After trying to work with #8375 and discussion with @tychoish, we realized that we should rather expose different validation methods than introduce a boolean flag into the ValidateBasic method for Vote.

I'll be submitting several separate PRs that build on this one and each other (eventually including some commits from #8375) in order to implement RFC 017 (#8317).

creachadair

A couple nits and a couple questions, but this seems on the right track to me.

privval/file.go

creachadair · 2022-04-23T17:27:34Z

types/vote.go

 }

+func voteFromProto(pv *tmproto.Vote) (*Vote, error) {
+	if pv == nil {


As a general rule, please don't inject nil checks for these. The pointer is the actionable type for a protobuf message, so if the caller passes nil here it's not a recoverable error and should probably be allowed to panic.

(A program that calls this function with a nil Vote message is already statically incorrect. If there needs to e a check, it should be on the caller's side. But the way we use it, that isn't necessary).

I just kept the existing code as-is as far as possible, but I agree - I'll change this.

creachadair · 2022-04-23T17:31:22Z

types/vote_test.go

-			tc.malleateVote(vote)
-			assert.Equal(t, tc.expectErr, vote.ValidateBasic() != nil, "Validate Basic had an unexpected result")
+			votes := []*Vote{examplePrevote(t), examplePrecommit(t)}
+			for i, vote := range votes {


Some of this is pre-existing, but I'd like to put in a plug for getting rid of this loop (instead test each vote separately). I'd also suggest separating these test cases into batches: "ok", "basic validation errors", "prevote errors", and "precommit errors" that do not require flags in the test case.

The fiddly logic to check "is this the specially designated good case" and the various legerdemain with computing Booleans makes it really hard to decipher what is going on when a test fails. The check per vote is short, it's ok to copy paste. It'll all be on the same screenful. 🙂

It would make a little more code, but IMO tests are a good place to strongly favor legibility and obviousness over compactness IMO.

Schematically:

type testCase struct { name string malleateVote func(*Vote) } okTests := []testCase{ // … } for _, test := range okTests { t.Run(test.name, func(t *testing.T) { require.NoError(t, examplePrevote(t).ValidateBasic(), "valid prevote") require.NoError(t, examplePrecommit(t).ValidateBasic(), "valid precommit") }) } basicErrTests := []testCase{ // … } for _, test := range basicErrTests { t.Run(test.name, func(t *testing.T) { require.Error(t, examplePrevote(t).ValidateBasic(), "invalid prevote") require.Error(t, examplePrecommit(t).ValidateBasic(), "invalid precommit") }) } // etc.

Sounds good 👍

types/vote_test.go

creachadair · 2022-04-23T17:32:25Z

types/vote_test.go

+				err := privVal.SignVote(ctx, "test_chain_id", v)
+				vote.Signature = v.Signature
+				require.NoError(t, err)
+				tc.malleateVote(vote)


This name tickles me. 😀

creachadair · 2022-04-23T17:32:59Z

types/vote_test.go

+				require.NoError(t, err)
+				tc.malleateVote(vote)
+				// ValidateBasic errors should be consistent across prevotes and precommits
+				assert.Equal(t, tc.expectBasicErr, vote.ValidateBasic() != nil, "ValidateBasic had an unexpected result")


Is there some reason these are assert instead of require?

It seems as though different authors had different conventions throughout the history of the codebase. Do you have a preference for one of them?

The answer is complicated.

With t.Error (analogous to assert) and t.Fatal (analogous to require), I prefer Error where practical so that the test can keep providing useful feedback while it's running anyway. When I write tests with the standard handlers I usually only Fatal for cases where it doesn't make sense to continue.

But with testify, I default to "require" (the fatal form) because it obviates the need to check the result and explicitly continue (which defeats the point of the assertion). Since the standard case already has the condition it's not a big addition.

If it were up to me we wouldn't use either, but on the above rationale I usually default "require". YMMV.

internal/consensus/msgs.go

creachadair · 2022-04-23T17:47:48Z

types/vote.go

+// our Vote domain type, performing basic validation with ValidateBasic. Note
+// that this performs no vote extension validation - to additionally validate
+// vote extensions, use VoteWithExtensionFromProto instead.
+func VoteFromProto(pv *tmproto.Vote) (*Vote, error) {


I find the names of these two slightly misleading: "VoteFromProto" suggests I'm getting the entire vote, implying all its parts (including the extension). Reading "VoteWithExtensionFromProto" feels like "this one HAS the extension, the other doesn't".

But both return the extension—it's just this one doesn't verify it while the other does.

If this were Java, we'd just say ExtractAndVerifyVoteWithoutExtensionFromProto and ExtractAndVerifyVoteWithExtensionFromProto. But here, I wonder if maybe we should consider separating "extract" and "verify", i.e., just let the caller call ValidateBasic or ValidateWithExtension as they prefer. I don't think we're adding much value by hiding that call, when they already have to know which they want.

Yes, it does make sense to rather separate the logic for extracting and validating votes. I think it's usually better to have each function just do one thing. I'll separate them 👍

thanethomson · 2022-04-27T00:59:09Z

I think I've addressed all of the outstanding comments here. Let me know if there's anything else that anyone feels needs to be improved here.

creachadair

Looks good to me! I don't know the answer to your TODO, though.

thanethomson · 2022-04-27T11:35:44Z

I don't know the answer to your TODO, though.

No problem! The idea was to address it in a follow-up PR. The approach implemented in this PR doesn't change the existing behavior.

sergio-mena

Thanks for this patch. It improves upon my eleventh-hour introduction of a boolean before my vacation break.
However, I still don't clearly see how we can guarantee we check the extensions at all callsites where it is needed.

sergio-mena · 2022-04-27T11:12:55Z

internal/consensus/msgs.go

 func (m *VoteMessage) ValidateBasic() error {
-	return m.Vote.ValidateBasic()
+	// Here we validate votes with vote extensions, since we require vote
+	// extensions to be sent in vote messages during consensus.


Is it also the case with prevotes?
I mean... may they also come in a VoteMessage

I'll update the comment. The ValidateWithExtension method ensures (through its internal call to ValidateBasic) that prevotes don't have extensions, but precommits do.

privval/file.go

types/priv_validator.go

types/vote.go

sergio-mena · 2022-04-27T11:33:37Z

types/vote_set.go

 	}

 	// Check signature.
+	// TODO(thane): Should we always require vote extensions in vote sets?


I think we should, as long as they are precommits
The only case we don't want precommits to have extensions is when embedded into an evidence, which is what complicates this problem.
Would it make sense to also requires extensions in evidences (as long as they refer to precommits) ?
If yes, then all this logic would be greatly simplified

It doesn't look like VoteSet is used in relation to evidence. I've now updated the code to use ValidateWithExtension instead and it seems to work, but we'll see when we enforce the vote extension requirement in a subsequent PR.

sergio-mena

Thanks. I still don't 100% the validation of votes w.r.t vote extensions, but I understand from your answers this will only be possible after the whole #8375 is completed

internal/consensus/msgs.go

williambanfield · 2022-04-27T15:30:32Z

types/vote_test.go

+	require.NoError(t, pv.SignVote(ctx, chainID, v))
+	vote.Signature = v.Signature
+	vote.ExtensionSignature = v.ExtensionSignature
+	malleateVote(vote)


It seems like this could be just a separate function, is there a strong reason to inject this? I think it's a bit more clear if this is separately called directly after the sign vote method.

Sure, I've separated it out now. I originally did it like this because it seemed slightly neater to have this functionality handled on a single line, but I see your point.

types/vote_test.go

Some parts of the code need vote extensions to be verified and validated (mostly in consensus), and other parts of the code don't because its possible that, in some cases (as per RFC 017), we won't have vote extensions. This explicitly facilitates that split. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Split vote verification/validation based on vote extensions Some parts of the code need vote extensions to be verified and validated (mostly in consensus), and other parts of the code don't because its possible that, in some cases (as per RFC 017), we won't have vote extensions. This explicitly facilitates that split. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Only sign extensions in precommits, not prevotes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update privval/file.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Apply suggestions from code review Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Temporarily disable extension requirement again for E2E testing Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorganize comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Leave vote validation and pre-call nil check up to caller of VoteToProto Signed-off-by: Thane Thomson <connect@thanethomson.com> * Split complex vote validation test into multiple tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * Universally enforce no vote extensions on any vote type but precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Make error messages more generic Signed-off-by: Thane Thomson <connect@thanethomson.com> * Verify with vote extensions when constructing a VoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension check for prevotes prior to signing votes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix supporting test code to only inject extensions into precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Separate vote malleation from signing in vote tests for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension signature length check and corresponding test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Perform basic vote validation in CommitToVoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

* [cherry-picked] abci++: Vote extension cleanup (#8402) * Split vote verification/validation based on vote extensions Some parts of the code need vote extensions to be verified and validated (mostly in consensus), and other parts of the code don't because its possible that, in some cases (as per RFC 017), we won't have vote extensions. This explicitly facilitates that split. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Only sign extensions in precommits, not prevotes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update privval/file.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Apply suggestions from code review Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Temporarily disable extension requirement again for E2E testing Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorganize comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Leave vote validation and pre-call nil check up to caller of VoteToProto Signed-off-by: Thane Thomson <connect@thanethomson.com> * Split complex vote validation test into multiple tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * Universally enforce no vote extensions on any vote type but precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Make error messages more generic Signed-off-by: Thane Thomson <connect@thanethomson.com> * Verify with vote extensions when constructing a VoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension check for prevotes prior to signing votes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix supporting test code to only inject extensions into precommits Signed-off-by: Thane Thomson <connect@thanethomson.com> * Separate vote malleation from signing in vote tests for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add extension signature length check and corresponding test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Perform basic vote validation in CommitToVoteSet Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Appease TestMempoolProgressAfterCreateEmptyBlocksInterval Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

thanethomson requested review from ancazamfir, cmwaters, creachadair, ebuchman, jmalicevic, sergio-mena, tychoish and williambanfield as code owners April 23, 2022 13:22

creachadair reviewed Apr 23, 2022

View reviewed changes

thanethomson marked this pull request as draft April 23, 2022 18:08

thanethomson force-pushed the thane/8272-vote-extension-cleanup branch from 0860e2c to ae26068 Compare April 23, 2022 21:40

thanethomson marked this pull request as ready for review April 23, 2022 21:52

thanethomson force-pushed the thane/8272-vote-extension-cleanup branch 2 times, most recently from beda38e to 6cddb4c Compare April 25, 2022 16:48

creachadair approved these changes Apr 27, 2022

View reviewed changes

sergio-mena reviewed Apr 27, 2022

View reviewed changes

thanethomson force-pushed the thane/8272-vote-extension-cleanup branch from 6cddb4c to 1249fb6 Compare April 27, 2022 12:16

sergio-mena approved these changes Apr 27, 2022

View reviewed changes

williambanfield reviewed Apr 27, 2022

View reviewed changes

thanethomson force-pushed the thane/8272-vote-extension-cleanup branch 2 times, most recently from 7b9f7f5 to e111425 Compare April 27, 2022 20:11

thanethomson and others added 7 commits April 27, 2022 17:58

Only sign extensions in precommits, not prevotes

99c8070

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update privval/file.go

d342e16

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Apply suggestions from code review

1b3977b

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Temporarily disable extension requirement again for E2E testing

d939fb6

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Reorganize comment for clarity

a1b0ad2

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Leave vote validation and pre-call nil check up to caller of VoteToProto

7b2a2cb

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added 10 commits April 27, 2022 17:58

Split complex vote validation test into multiple tests

0c15d8a

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Universally enforce no vote extensions on any vote type but precommits

bea483a

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Make error messages more generic

a23c1af

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Verify with vote extensions when constructing a VoteSet

18408f8

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Expand comment for clarity

9c70971

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add extension check for prevotes prior to signing votes

e2bc1c3

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix supporting test code to only inject extensions into precommits

644d50c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Separate vote malleation from signing in vote tests for clarity

98f03ea

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add extension signature length check and corresponding test

bb05d40

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Perform basic vote validation in CommitToVoteSet

1e6fdce

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson force-pushed the thane/8272-vote-extension-cleanup branch from e111425 to 1e6fdce Compare April 27, 2022 21:58

Merge branch 'master' into thane/8272-vote-extension-cleanup

3d49eb4

thanethomson merged commit d69bd64 into master Apr 28, 2022

thanethomson deleted the thane/8272-vote-extension-cleanup branch April 28, 2022 17:53

thanethomson mentioned this pull request Apr 28, 2022

abci++: Propagate vote extensions (RFC 017) #8433

Merged

sergio-mena mentioned this pull request Sep 8, 2022

ABCI++ vote extensions & FinalizeBlock #9396

Closed

17 tasks

cmwaters mentioned this pull request Oct 24, 2022

abci: implement vote extensions #9620

Closed

3 tasks

sergio-mena mentioned this pull request Dec 6, 2022

Vote extension cleanup #9846

Closed

sergio-mena mentioned this pull request Dec 6, 2022

abci++: Vote extension cleanup #9848

Merged

3 tasks

sergio-mena mentioned this pull request Dec 7, 2022

Vote extension propagation #9852

Closed

sergio-mena mentioned this pull request Dec 23, 2022

ABCI++ vote extensions & FinalizeBlock cometbft/cometbft#12

Closed

17 tasks

Conversation

thanethomson commented Apr 23, 2022

Uh oh!

creachadair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

creachadair Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thanethomson commented Apr 27, 2022

Uh oh!

creachadair left a comment

Choose a reason for hiding this comment

Uh oh!

thanethomson commented Apr 27, 2022

Uh oh!

sergio-mena 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

creachadair Apr 23, 2022 •

edited

Loading

sergio-mena left a comment •

edited

Loading