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 <connect@thanethomson.com>

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,

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) {