evidence: fix bug with hashes (#6375)

types/evidence.go

@@ -296,7 +296,10 @@ func (l *LightClientAttackEvidence) ConflictingHeaderIsInvalid(trustedHeader *He
 // with evidence that have the same conflicting header and common height but different permutations
 // of validator commit signatures. The reason for this is that we don't want to allow several
 // permutations of the same evidence to be committed on chain. Ideally we commit the header with the
-// most commit signatures (captures the most byzantine validators) but anything greater than 1/3 is sufficient.
+// most commit signatures (captures the most byzantine validators) but anything greater than 1/3 is
+// sufficient.
+// TODO: We should change the hash to include the commit, header, total voting power, byzantine
+// validators and timestamp
 func (l *LightClientAttackEvidence) Hash() []byte {
 	buf := make([]byte, binary.MaxVarintLen64)
 	n := binary.PutVarint(buf, l.CommonHeight)
@@ -315,8 +318,14 @@ func (l *LightClientAttackEvidence) Height() int64 {
 
 // String returns a string representation of LightClientAttackEvidence
 func (l *LightClientAttackEvidence) String() string {
-	return fmt.Sprintf("LightClientAttackEvidence{ConflictingBlock: %v, CommonHeight: %d}",
-		l.ConflictingBlock.String(), l.CommonHeight)
+	return fmt.Sprintf(`LightClientAttackEvidence{
+		ConflictingBlock: %v, 
+		CommonHeight: %d, 
+		ByzatineValidators: %v, 
+		TotalVotingPower: %d, 
+		Timestamp: %v}#%X`,
+		l.ConflictingBlock.String(), l.CommonHeight, l.ByzantineValidators,
+		l.TotalVotingPower, l.Timestamp, l.Hash())
 }
 
 // Time returns the time of the common block where the infraction leveraged off.
@@ -335,8 +344,8 @@ func (l *LightClientAttackEvidence) ValidateBasic() error {
 		return errors.New("conflicting block missing header")
 	}
 
-	if err := l.ConflictingBlock.ValidateBasic(l.ConflictingBlock.ChainID); err != nil {
-		return fmt.Errorf("invalid conflicting light block: %w", err)
+	if l.TotalVotingPower <= 0 {
+		return errors.New("negative or zero total voting power")
 	}
 
 	if l.CommonHeight <= 0 {
@@ -351,6 +360,10 @@ func (l *LightClientAttackEvidence) ValidateBasic() error {
 			l.CommonHeight, l.ConflictingBlock.Height)
 	}
 
+	if err := l.ConflictingBlock.ValidateBasic(l.ConflictingBlock.ChainID); err != nil {
+		return fmt.Errorf("invalid conflicting light block: %w", err)
+	}
+
 	return nil
 }
 
@@ -422,6 +435,8 @@ func (evl EvidenceList) Hash() []byte {
 	// the Evidence size is capped.
 	evidenceBzs := make([][]byte, len(evl))
 	for i := 0; i < len(evl); i++ {
+		// TODO: We should change this to the hash. Using bytes contains some unexported data that
+		// may cause different hashes
 		evidenceBzs[i] = evl[i].Bytes()
 	}
 	return merkle.HashFromByteSlices(evidenceBzs)

types/evidence_test.go

@@ -89,9 +89,11 @@ func TestDuplicateVoteEvidenceValidation(t *testing.T) {
 	}
 }
 
-func TestLightClientAttackEvidence(t *testing.T) {
+func TestLightClientAttackEvidenceBasic(t *testing.T) {
 	height := int64(5)
-	voteSet, valSet, privVals := randVoteSet(height, 1, tmproto.PrecommitType, 10, 1)
+	commonHeight := height - 1
+	nValidators := 10
+	voteSet, valSet, privVals := randVoteSet(height, 1, tmproto.PrecommitType, nValidators, 1)
 	header := makeHeaderRandom()
 	header.Height = height
 	blockID := makeBlockID(tmhash.Sum([]byte("blockhash")), math.MaxInt32, tmhash.Sum([]byte("partshash")))
@@ -105,56 +107,52 @@ func TestLightClientAttackEvidence(t *testing.T) {
 			},
 			ValidatorSet: valSet,
 		},
-		CommonHeight: height - 1,
+		CommonHeight:        commonHeight,
+		TotalVotingPower:    valSet.TotalVotingPower(),
+		Timestamp:           header.Time,
+		ByzantineValidators: valSet.Validators[:nValidators/2],
 	}
 	assert.NotNil(t, lcae.String())
 	assert.NotNil(t, lcae.Hash())
-	// only 7 validators sign
-	differentCommit, err := MakeCommit(blockID, height, 1, voteSet, privVals[:7], defaultVoteTime)
-	require.NoError(t, err)
-	differentEv := &LightClientAttackEvidence{
-		ConflictingBlock: &LightBlock{
-			SignedHeader: &SignedHeader{
-				Header: header,
-				Commit: differentCommit,
-			},
-			ValidatorSet: valSet,
-		},
-		CommonHeight: height - 1,
-	}
-	assert.Equal(t, lcae.Hash(), differentEv.Hash())
-	// different header hash
-	differentHeader := makeHeaderRandom()
-	differentEv = &LightClientAttackEvidence{
-		ConflictingBlock: &LightBlock{
-			SignedHeader: &SignedHeader{
-				Header: differentHeader,
-				Commit: differentCommit,
-			},
-			ValidatorSet: valSet,
-		},
-		CommonHeight: height - 1,
-	}
-	assert.NotEqual(t, lcae.Hash(), differentEv.Hash())
-	// different common height should produce a different header
-	differentEv = &LightClientAttackEvidence{
-		ConflictingBlock: &LightBlock{
-			SignedHeader: &SignedHeader{
-				Header: header,
-				Commit: differentCommit,
-			},
-			ValidatorSet: valSet,
-		},
-		CommonHeight: height - 2,
-	}
-	assert.NotEqual(t, lcae.Hash(), differentEv.Hash())
-	assert.Equal(t, lcae.Height(), int64(4)) // Height should be the common Height
+	assert.Equal(t, lcae.Height(), commonHeight) // Height should be the common Height
 	assert.NotNil(t, lcae.Bytes())
+
+	// maleate evidence to test hash uniqueness
+	testCases := []struct {
+		testName         string
+		malleateEvidence func(*LightClientAttackEvidence)
+	}{
+		{"Different header", func(ev *LightClientAttackEvidence) { ev.ConflictingBlock.Header = makeHeaderRandom() }},
+		{"Different common height", func(ev *LightClientAttackEvidence) {
+			ev.CommonHeight = height + 1
+		}},
+	}
+
+	for _, tc := range testCases {
+		lcae := &LightClientAttackEvidence{
+			ConflictingBlock: &LightBlock{
+				SignedHeader: &SignedHeader{
+					Header: header,
+					Commit: commit,
+				},
+				ValidatorSet: valSet,
+			},
+			CommonHeight:        commonHeight,
+			TotalVotingPower:    valSet.TotalVotingPower(),
+			Timestamp:           header.Time,
+			ByzantineValidators: valSet.Validators[:nValidators/2],
+		}
+		hash := lcae.Hash()
+		tc.malleateEvidence(lcae)
+		assert.NotEqual(t, hash, lcae.Hash(), tc.testName)
+	}
 }
 
 func TestLightClientAttackEvidenceValidation(t *testing.T) {
 	height := int64(5)
-	voteSet, valSet, privVals := randVoteSet(height, 1, tmproto.PrecommitType, 10, 1)
+	commonHeight := height - 1
+	nValidators := 10
+	voteSet, valSet, privVals := randVoteSet(height, 1, tmproto.PrecommitType, nValidators, 1)
 	header := makeHeaderRandom()
 	header.Height = height
 	header.ValidatorsHash = valSet.Hash()
@@ -169,7 +167,10 @@ func TestLightClientAttackEvidenceValidation(t *testing.T) {
 			},
 			ValidatorSet: valSet,
 		},
-		CommonHeight: height - 1,
+		CommonHeight:        commonHeight,
+		TotalVotingPower:    valSet.TotalVotingPower(),
+		Timestamp:           header.Time,
+		ByzantineValidators: valSet.Validators[:nValidators/2],
 	}
 	assert.NoError(t, lcae.ValidateBasic())
 
@@ -178,16 +179,22 @@ func TestLightClientAttackEvidenceValidation(t *testing.T) {
 		malleateEvidence func(*LightClientAttackEvidence)
 		expectErr        bool
 	}{
-		{"Good DuplicateVoteEvidence", func(ev *LightClientAttackEvidence) {}, false},
+		{"Good LightClientAttackEvidence", func(ev *LightClientAttackEvidence) {}, false},
 		{"Negative height", func(ev *LightClientAttackEvidence) { ev.CommonHeight = -10 }, true},
 		{"Height is greater than divergent block", func(ev *LightClientAttackEvidence) {
 			ev.CommonHeight = height + 1
 		}, true},
+		{"Height is equal to the divergent block", func(ev *LightClientAttackEvidence) {
+			ev.CommonHeight = height
+		}, false},
 		{"Nil conflicting header", func(ev *LightClientAttackEvidence) { ev.ConflictingBlock.Header = nil }, true},
 		{"Nil conflicting blocl", func(ev *LightClientAttackEvidence) { ev.ConflictingBlock = nil }, true},
 		{"Nil validator set", func(ev *LightClientAttackEvidence) {
 			ev.ConflictingBlock.ValidatorSet = &ValidatorSet{}
 		}, true},
+		{"Negative total voting power", func(ev *LightClientAttackEvidence) {
+			ev.TotalVotingPower = -1
+		}, true},
 	}
 	for _, tc := range testCases {
 		tc := tc
@@ -200,7 +207,10 @@ func TestLightClientAttackEvidenceValidation(t *testing.T) {
 					},
 					ValidatorSet: valSet,
 				},
-				CommonHeight: height - 1,
+				CommonHeight:        commonHeight,
+				TotalVotingPower:    valSet.TotalVotingPower(),
+				Timestamp:           header.Time,
+				ByzantineValidators: valSet.Validators[:nValidators/2],
 			}
 			tc.malleateEvidence(lcae)
 			if tc.expectErr {

types/light.go

@@ -149,7 +149,7 @@ func (sh SignedHeader) ValidateBasic(chainID string) error {
 	if sh.Commit.Height != sh.Height {
 		return fmt.Errorf("header and commit height mismatch: %d vs %d", sh.Height, sh.Commit.Height)
 	}
-	if hhash, chash := sh.Hash(), sh.Commit.BlockID.Hash; !bytes.Equal(hhash, chash) {
+	if hhash, chash := sh.Header.Hash(), sh.Commit.BlockID.Hash; !bytes.Equal(hhash, chash) {
 		return fmt.Errorf("commit signs block %X, header is block %X", chash, hhash)
 	}
 

types/light_test.go

@@ -152,11 +152,13 @@ func TestSignedHeaderValidateBasic(t *testing.T) {
 				Header: tc.shHeader,
 				Commit: tc.shCommit,
 			}
-			assert.Equal(
+			err := sh.ValidateBasic(validSignedHeader.Header.ChainID)
+			assert.Equalf(
 				t,
 				tc.expectErr,
-				sh.ValidateBasic(validSignedHeader.Header.ChainID) != nil,
+				err != nil,
 				"Validate Basic had an unexpected result",
+				err,
 			)
 		})
 	}

types/validator_set.go

@@ -1028,7 +1028,9 @@ func (vals *ValidatorSet) ToProto() (*tmproto.ValidatorSet, error) {
 	}
 	vp.Proposer = valProposer
 
-	vp.TotalVotingPower = vals.totalVotingPower
+	// NOTE: Sometimes we use the bytes of the proto form as a hash. This means that we need to
+	// be consistent with cached data
+	vp.TotalVotingPower = 0
 
 	return vp, nil
 }
@@ -1059,7 +1061,12 @@ func ValidatorSetFromProto(vp *tmproto.ValidatorSet) (*ValidatorSet, error) {
 
 	vals.Proposer = p
 
-	vals.totalVotingPower = vp.GetTotalVotingPower()
+	// NOTE: We can't trust the total voting power given to us by other peers. If someone were to
+	// inject a non-zeo value that wasn't the correct voting power we could assume a wrong total
+	// power hence we need to recompute it.
+	// FIXME: We should look to remove TotalVotingPower from proto or add it in the validators hash
+	// so we don't have to do this
+	vals.TotalVotingPower()
 
 	return vals, vals.ValidateBasic()
 }