From a749afe8fb63302efeb3260aad2136cc55892dee Mon Sep 17 00:00:00 2001 From: Sergio Mena Date: Fri, 16 Dec 2022 12:16:53 +0100 Subject: [PATCH] Divergences in comparison with #9620. Part 1: easy/obvious (#9888) * Obvious changes (including bugs) * Update privval/file.go Co-authored-by: Thane Thomson Co-authored-by: Thane Thomson --- blocksync/pool.go | 4 +--- blocksync/reactor_test.go | 4 +--- internal/test/params.go | 1 + privval/file.go | 2 ++ proto/tendermint/abci/types.proto | 10 +++++++++- proto/tendermint/types/canonical.proto | 1 - proto/tendermint/types/types.proto | 19 ++++--------------- proxy/app_conn.go | 6 +++--- state/execution.go | 2 +- store/store.go | 2 +- types/block_test.go | 24 ++++++++++++------------ types/vote_set.go | 2 +- 12 files changed, 36 insertions(+), 41 deletions(-) diff --git a/blocksync/pool.go b/blocksync/pool.go index 147ce0603..9a5e7cea8 100644 --- a/blocksync/pool.go +++ b/blocksync/pool.go @@ -566,9 +566,7 @@ func (bpr *bpRequester) setBlock(block *types.Block, extCommit *types.ExtendedCo return false } bpr.block = block - if extCommit != nil { - bpr.extCommit = extCommit - } + bpr.extCommit = extCommit bpr.mtx.Unlock() select { diff --git a/blocksync/reactor_test.go b/blocksync/reactor_test.go index 96feb8b9a..c091e02b5 100644 --- a/blocksync/reactor_test.go +++ b/blocksync/reactor_test.go @@ -109,14 +109,12 @@ func newReactor( panic(err) } - var lastExtCommit *types.ExtendedCommit - // The commit we are building for the current height. seenExtCommit := &types.ExtendedCommit{} // let's add some blocks in for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ { - lastExtCommit = seenExtCommit.Clone() + lastExtCommit := seenExtCommit.Clone() thisBlock := state.MakeBlock(blockHeight, nil, lastExtCommit.ToCommit(), nil, state.Validators.Proposer.Address) diff --git a/internal/test/params.go b/internal/test/params.go index a45a6fb58..167ba460c 100644 --- a/internal/test/params.go +++ b/internal/test/params.go @@ -8,6 +8,7 @@ import ( // for use in testing func ConsensusParams() *types.ConsensusParams { c := types.DefaultConsensusParams() + // enable vote extensions c.ABCI.VoteExtensionsEnableHeight = 1 return c } diff --git a/privval/file.go b/privval/file.go index 200295b39..f0ac56b8d 100644 --- a/privval/file.go +++ b/privval/file.go @@ -304,6 +304,7 @@ func (pv *FilePV) String() string { // signVote checks if the vote is good to sign and sets the vote signature. // It may need to set the timestamp as well if the vote is otherwise the same as // a previously signed vote (ie. we crashed after signing but before the vote hit the WAL). +// Extension signatures are always signed for non-nil precommits (even if the data is empty). func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { height, round, step := vote.Height, vote.Round, voteToStep(vote) @@ -320,6 +321,7 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { // application may have created a different extension. We therefore always // re-sign the vote extensions of precommits. For prevotes and nil // precommits, the extension signature will always be empty. + // Even if the signed over data is empty, we still add the signature var extSig []byte if vote.Type == tmproto.PrecommitType && !types.ProtoBlockIDIsNil(&vote.BlockID) { extSignBytes := types.VoteExtensionSignBytes(chainID, vote) diff --git a/proto/tendermint/abci/types.proto b/proto/tendermint/abci/types.proto index ea00a8654..20e59333e 100644 --- a/proto/tendermint/abci/types.proto +++ b/proto/tendermint/abci/types.proto @@ -153,15 +153,19 @@ message RequestProcessProposal { bytes proposer_address = 8; } -// Extends a vote with application-side injection +// Extends a vote with application-injected data message RequestExtendVote { + // the hash of the block that this vote may be referring to bytes hash = 1; + // the height of the extended vote int64 height = 2; } // Verify the vote extension message RequestVerifyVoteExtension { + // the hash of the block that this received vote corresponds to bytes hash = 1; + // the validator that signed the vote extension bytes validator_address = 2; int64 height = 3; bytes vote_extension = 4; @@ -329,6 +333,10 @@ message ResponseVerifyVoteExtension { enum VerifyStatus { UNKNOWN = 0; ACCEPT = 1; + // Rejecting the vote extension will reject the entire precommit by the sender. + // Incorrectly implementing this thus has liveness implications as it may affect + // tendermint's ability to receive 2/3+ valid votes to finalize the block. + // Honest nodes should never be rejected. REJECT = 2; } } diff --git a/proto/tendermint/types/canonical.proto b/proto/tendermint/types/canonical.proto index 705561490..e9ed1e55d 100644 --- a/proto/tendermint/types/canonical.proto +++ b/proto/tendermint/types/canonical.proto @@ -34,7 +34,6 @@ message CanonicalVote { CanonicalBlockID block_id = 4 [(gogoproto.customname) = "BlockID"]; google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; string chain_id = 6 [(gogoproto.customname) = "ChainID"]; - VoteExtensionToSign vote_extension = 7; } // CanonicalVoteExtension provides us a way to serialize a vote extension from diff --git a/proto/tendermint/types/types.proto b/proto/tendermint/types/types.proto index d5870b94f..71521576b 100644 --- a/proto/tendermint/types/types.proto +++ b/proto/tendermint/types/types.proto @@ -89,7 +89,7 @@ message Data { repeated bytes txs = 1; } -// Vote represents a prevote, precommit, or commit vote from validators for +// Vote represents a prevote or precommit vote from validators for // consensus. message Vote { SignedMsgType type = 1; @@ -109,21 +109,10 @@ message Vote { bytes extension = 9; // Vote extension signature by the validator if they participated in // consensus for the associated block. + // Only valid for precommit messages. bytes extension_signature = 10; } -// VoteExtension is app-defined additional information to the validator votes. -message VoteExtension { - bytes app_data_to_sign = 1; - bytes app_data_self_authenticating = 2; -} - -// VoteExtensionToSign is a subset of VoteExtension that is signed by the validators private key. -// VoteExtensionToSign is extracted from an existing VoteExtension. -message VoteExtensionToSign { - bytes app_data_to_sign = 1; -} - // Commit contains the evidence that a block was committed by a set of validators. message Commit { int64 height = 1; @@ -139,7 +128,6 @@ message CommitSig { google.protobuf.Timestamp timestamp = 3 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; bytes signature = 4; - VoteExtensionToSign vote_extension = 5; } message ExtendedCommit { @@ -151,7 +139,8 @@ message ExtendedCommit { } // ExtendedCommitSig retains all the same fields as CommitSig but adds vote -// extension-related fields. +// extension-related fields. We use two signatures to ensure backwards compatibility. +// That is the digest of the original signature is still the same in prior versions message ExtendedCommitSig { BlockIDFlag block_id_flag = 1; bytes validator_address = 2; diff --git a/proxy/app_conn.go b/proxy/app_conn.go index a9f84e7e6..3eadbe83e 100644 --- a/proxy/app_conn.go +++ b/proxy/app_conn.go @@ -90,17 +90,17 @@ func (app *appConnConsensus) ProcessProposal(ctx context.Context, req *types.Req } func (app *appConnConsensus) ExtendVote(ctx context.Context, req *types.RequestExtendVote) (*types.ResponseExtendVote, error) { - defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))() + defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "extend_vote"))() return app.appConn.ExtendVote(ctx, req) } func (app *appConnConsensus) VerifyVoteExtension(ctx context.Context, req *types.RequestVerifyVoteExtension) (*types.ResponseVerifyVoteExtension, error) { - defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))() + defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "verify_vote_extension"))() return app.appConn.VerifyVoteExtension(ctx, req) } func (app *appConnConsensus) FinalizeBlock(ctx context.Context, req *types.RequestFinalizeBlock) (*types.ResponseFinalizeBlock, error) { - defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))() + defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "finalize_block"))() return app.appConn.FinalizeBlock(ctx, req) } diff --git a/state/execution.go b/state/execution.go index 4d3310724..2bbed6edd 100644 --- a/state/execution.go +++ b/state/execution.go @@ -549,7 +549,7 @@ func updateState( nextParams = state.ConsensusParams.Update(abciResponse.ConsensusParamUpdates) err := nextParams.ValidateBasic() if err != nil { - return state, fmt.Errorf("updating consensus params: %w", err) + return state, fmt.Errorf("validating new consensus params: %w", err) } err = state.ConsensusParams.ValidateUpdate(abciResponse.ConsensusParamUpdates, header.Height) diff --git a/store/store.go b/store/store.go index 660c1d415..ac46ac0db 100644 --- a/store/store.go +++ b/store/store.go @@ -247,7 +247,7 @@ func (bs *BlockStore) LoadBlockCommit(height int64) *types.Commit { // The extended commit is not guaranteed to contain the same +2/3 precommits data // as the commit in the block. func (bs *BlockStore) LoadBlockExtendedCommit(height int64) *types.ExtendedCommit { - var pbec = new(tmproto.ExtendedCommit) + pbec := new(tmproto.ExtendedCommit) bz, err := bs.db.Get(calcExtCommitKey(height)) if err != nil { panic(fmt.Errorf("fetching extended commit: %w", err)) diff --git a/types/block_test.go b/types/block_test.go index b72ea083d..c4cb72d87 100644 --- a/types/block_test.go +++ b/types/block_test.go @@ -232,21 +232,21 @@ func TestCommit(t *testing.T) { lastID := makeBlockIDRandom() h := int64(3) voteSet, _, vals := randVoteSet(h-1, 1, tmproto.PrecommitType, 10, 1) - commit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, time.Now()) + extCommit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, time.Now()) require.NoError(t, err) - assert.Equal(t, h-1, commit.Height) - assert.EqualValues(t, 1, commit.Round) - assert.Equal(t, tmproto.PrecommitType, tmproto.SignedMsgType(commit.Type())) - if commit.Size() <= 0 { - t.Fatalf("commit %v has a zero or negative size: %d", commit, commit.Size()) + assert.Equal(t, h-1, extCommit.Height) + assert.EqualValues(t, 1, extCommit.Round) + assert.Equal(t, tmproto.PrecommitType, tmproto.SignedMsgType(extCommit.Type())) + if extCommit.Size() <= 0 { + t.Fatalf("commit %v has a zero or negative size: %d", extCommit, extCommit.Size()) } - require.NotNil(t, commit.BitArray()) - assert.Equal(t, bits.NewBitArray(10).Size(), commit.BitArray().Size()) + require.NotNil(t, extCommit.BitArray()) + assert.Equal(t, bits.NewBitArray(10).Size(), extCommit.BitArray().Size()) - assert.Equal(t, voteSet.GetByIndex(0), commit.GetByIndex(0)) - assert.True(t, commit.IsCommit()) + assert.Equal(t, voteSet.GetByIndex(0), extCommit.GetByIndex(0)) + assert.True(t, extCommit.IsCommit()) } func TestCommitValidateBasic(t *testing.T) { @@ -440,11 +440,11 @@ func randCommit(now time.Time) *Commit { lastID := makeBlockIDRandom() h := int64(3) voteSet, _, vals := randVoteSet(h-1, 1, tmproto.PrecommitType, 10, 1) - commit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, now) + extCommit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, now) if err != nil { panic(err) } - return commit.ToCommit() + return extCommit.ToCommit() } func hexBytesFromString(s string) bytes.HexBytes { diff --git a/types/vote_set.go b/types/vote_set.go index 9ed6df9e2..a543e1ffe 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -217,7 +217,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { // Check signature. if voteSet.extensionsEnabled { if err := vote.VerifyVoteAndExtension(voteSet.chainID, val.PubKey); err != nil { - return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) + return false, fmt.Errorf("failed to verify extended vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) } } else { if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil {