From f618fc2a1c0499fec1b27bc754209163ccedd326 Mon Sep 17 00:00:00 2001 From: Thane Thomson Date: Tue, 29 Mar 2022 08:35:13 -0400 Subject: [PATCH] Make extendedCommitInfo function more robust At first extendedCommitInfo expected votes to be in the same order as their corresponding validators in the supplied CommitInfo struct, but this proved to be rather difficult since when a validator set's loaded from state it's first sorted by voting power and then by address. Instead of sorting the votes in the same way, this approach simply maps votes to their corresponding validator's address prior to constructing the extended commit info. This way it's easy to look up the corresponding vote and we don't need to care about vote order. Signed-off-by: Thane Thomson --- internal/state/execution.go | 31 +++++++++++++++++++------------ node/node_test.go | 27 ++++++++++++++++----------- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/internal/state/execution.go b/internal/state/execution.go index 4f1c5cc47..5547f522d 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -1,13 +1,13 @@ package state import ( - "bytes" "context" "fmt" "time" abciclient "github.com/tendermint/tendermint/abci/client" abci "github.com/tendermint/tendermint/abci/types" + "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/encoding" "github.com/tendermint/tendermint/crypto/merkle" "github.com/tendermint/tendermint/internal/eventbus" @@ -437,27 +437,34 @@ func buildLastCommitInfo(block *types.Block, store Store, initialHeight int64) a } } -// extendedCommitInfo assumes that votes from the given commit correspond -// precisely to those provided in the list of votes, such that vote extensions -// can be correlated with the votes in the commit. +// extendedCommitInfo expects a CommitInfo struct along with all of the +// original votes relating to that commit, including their vote extensions. The +// order of votes does not matter. func extendedCommitInfo(c abci.CommitInfo, votes []*types.Vote) abci.ExtendedCommitInfo { if len(c.Votes) != len(votes) { panic(fmt.Sprintf("extendedCommitInfo: number of votes from commit differ from the number of votes supplied (%d != %d)", len(c.Votes), len(votes))) } + votesByVal := make(map[string]*types.Vote) + for _, vote := range votes { + if vote != nil { + valAddr := vote.ValidatorAddress.String() + if _, ok := votesByVal[valAddr]; ok { + panic(fmt.Sprintf("extendedCommitInfo: found duplicate vote for validator with address %s", valAddr)) + } + votesByVal[valAddr] = vote + } + } vs := make([]abci.ExtendedVoteInfo, len(c.Votes)) for i := range vs { var ext []byte // votes[i] will be nil if c.Votes[i].SignedLastBlock is false if c.Votes[i].SignedLastBlock { - // We would panic anyways, but at least this will provide more information. - if votes[i] == nil { - panic(fmt.Sprintf("extendedCommitInfo: expected vote in position %d (%v), but got nil", i, c.Votes[i])) + valAddr := crypto.Address(c.Votes[i].Validator.Address).String() + vote, ok := votesByVal[valAddr] + if !ok || vote == nil { + panic(fmt.Sprintf("extendedCommitInfo: validator with address %s signed last block, but could not find vote for it", valAddr)) } - // Strong guarantee that the vote corresponds to the relevant validator - if !bytes.Equal(c.Votes[i].Validator.Address, votes[i].ValidatorAddress) { - panic(fmt.Sprintf("extendedCommitInfo: expected votes in position %d to have the same validator address (%v != %v)", i, c.Votes[i].Validator.Address, votes[i].ValidatorAddress)) - } - ext = votes[i].Extension + ext = vote.Extension } vs[i] = abci.ExtendedVoteInfo{ Validator: c.Votes[i].Validator, diff --git a/node/node_test.go b/node/node_test.go index 2280e7a6e..007d8d6f4 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -279,7 +279,7 @@ func TestCreateProposalBlock(t *testing.T) { require.NoError(t, err) const height int64 = 1 - state, stateDB, privVals := state(t, 1, height) + state, stateDB, privVals, _ := state(t, 1, height) stateStore := sm.NewStore(stateDB) maxBytes := 16384 const partSize uint32 = 256 @@ -379,7 +379,7 @@ func TestMaxTxsProposalBlockSize(t *testing.T) { require.NoError(t, err) const height int64 = 1 - state, stateDB, _ := state(t, 1, height) + state, stateDB, _, _ := state(t, 1, height) stateStore := sm.NewStore(stateDB) blockStore := store.NewBlockStore(dbm.NewMemDB()) const maxBytes int64 = 16384 @@ -449,7 +449,7 @@ func TestMaxProposalBlockSize(t *testing.T) { err = proxyApp.Start(ctx) require.NoError(t, err) - state, stateDB, _ := state(t, types.MaxVotesCount, int64(1)) + state, stateDB, _, valAddrs := state(t, types.MaxVotesCount, int64(1)) stateStore := sm.NewStore(stateDB) blockStore := store.NewBlockStore(dbm.NewMemDB()) const maxBytes int64 = 1024 * 1024 * 2 @@ -519,10 +519,9 @@ func TestMaxProposalBlockSize(t *testing.T) { state.ChainID = maxChainID cs := types.CommitSig{ - BlockIDFlag: types.BlockIDFlagNil, - ValidatorAddress: crypto.AddressHash([]byte("validator_address")), - Timestamp: timestamp, - Signature: crypto.CRandBytes(types.MaxSignatureSize), + BlockIDFlag: types.BlockIDFlagCommit, + Timestamp: timestamp, + Signature: crypto.CRandBytes(types.MaxSignatureSize), } commit := &types.Commit{ @@ -535,14 +534,18 @@ func TestMaxProposalBlockSize(t *testing.T) { // add maximum amount of signatures to a single commit for i := 0; i < types.MaxVotesCount; i++ { - votes[i] = &types.Vote{} + votes[i] = &types.Vote{ + ValidatorAddress: valAddrs[i], + } + cs.ValidatorAddress = valAddrs[i] commit.Signatures = append(commit.Signatures, cs) } block, err := blockExec.CreateProposalBlock( ctx, math.MaxInt64, - state, commit, + state, + commit, proposerAddr, votes, ) @@ -699,10 +702,11 @@ func TestNodeSetEventSink(t *testing.T) { t.Cleanup(cleanup(ns)) } -func state(t *testing.T, nVals int, height int64) (sm.State, dbm.DB, []types.PrivValidator) { +func state(t *testing.T, nVals int, height int64) (sm.State, dbm.DB, []types.PrivValidator, []crypto.Address) { t.Helper() privVals := make([]types.PrivValidator, nVals) vals := make([]types.GenesisValidator, nVals) + addresses := make([]crypto.Address, nVals) for i := 0; i < nVals; i++ { privVal := types.NewMockPV() privVals[i] = privVal @@ -712,6 +716,7 @@ func state(t *testing.T, nVals int, height int64) (sm.State, dbm.DB, []types.Pri Power: 1000, Name: fmt.Sprintf("test%d", i), } + addresses[i] = vals[i].Address } s, _ := sm.MakeGenesisState(&types.GenesisDoc{ ChainID: "test-chain", @@ -731,7 +736,7 @@ func state(t *testing.T, nVals int, height int64) (sm.State, dbm.DB, []types.Pri s.LastValidators = s.Validators.Copy() require.NoError(t, stateStore.Save(s)) } - return s, stateDB, privVals + return s, stateDB, privVals, addresses } func TestLoadStateFromGenesis(t *testing.T) {