evidence: fix bug with hashes (backport #6375) (#6381)

evidence/pool.go

@@ -4,7 +4,6 @@ import (
  "bytes"
  "errors"
  "fmt"
- "sort"
  "sync"
  "sync/atomic"
  "time"
@@ -194,9 +193,11 @@ func (evpool *Pool) CheckEvidence(evList types.EvidenceList) error {
  hashes := make([][]byte, len(evList))
  for idx, ev := range evList {
 
- ok := evpool.fastCheck(ev)
+ _, isLightEv := ev.(*types.LightClientAttackEvidence)
 
- if !ok {
+ // We must verify light client attack evidence regardless because there could be a
+ // different conflicting block with the same hash.
+ if isLightEv || !evpool.isPending(ev) {
  // check that the evidence isn't already committed
  if evpool.isCommitted(ev) {
  return &types.ErrInvalidEvidence{Evidence: ev, Reason: errors.New("evidence was already committed")}
@@ -213,7 +214,7 @@ func (evpool *Pool) CheckEvidence(evList types.EvidenceList) error {
  evpool.logger.Error("Can't add evidence to pending list", "err", err, "ev", ev)
  }
 
- evpool.logger.Info("Verified new evidence of byzantine behavior", "evidence", ev)
+ evpool.logger.Info("Check evidence: verified evidence of byzantine behavior", "evidence", ev)
  }
 
  // check for duplicate evidence. We cache hashes so we don't have to work them out again.
@@ -255,68 +256,6 @@ func (evpool *Pool) State() sm.State {
  return evpool.state
 }
 
-//--------------------------------------------------------------------------
-
-// fastCheck leverages the fact that the evidence pool may have already verified the evidence to see if it can
-// quickly conclude that the evidence is already valid.
-func (evpool *Pool) fastCheck(ev types.Evidence) bool {
- if lcae, ok := ev.(*types.LightClientAttackEvidence); ok {
- key := keyPending(ev)
- evBytes, err := evpool.evidenceStore.Get(key)
- if evBytes == nil { // the evidence is not in the nodes pending list
- return false
- }
- if err != nil {
- evpool.logger.Error("Failed to load light client attack evidence", "err", err, "key(height/hash)", key)
- return false
- }
- var trustedPb tmproto.LightClientAttackEvidence
- err = trustedPb.Unmarshal(evBytes)
- if err != nil {
- evpool.logger.Error("Failed to convert light client attack evidence from bytes",
- "err", err, "key(height/hash)", key)
- return false
- }
- trustedEv, err := types.LightClientAttackEvidenceFromProto(&trustedPb)
- if err != nil {
- evpool.logger.Error("Failed to convert light client attack evidence from protobuf",
- "err", err, "key(height/hash)", key)
- return false
- }
- // ensure that all the byzantine validators that the evidence pool has match the byzantine validators
- // in this evidence
- if trustedEv.ByzantineValidators == nil && lcae.ByzantineValidators != nil {
- return false
- }
-
- if len(trustedEv.ByzantineValidators) != len(lcae.ByzantineValidators) {
- return false
- }
-
- byzValsCopy := make([]*types.Validator, len(lcae.ByzantineValidators))
- for i, v := range lcae.ByzantineValidators {
- byzValsCopy[i] = v.Copy()
- }
-
- // ensure that both validator arrays are in the same order
- sort.Sort(types.ValidatorsByVotingPower(byzValsCopy))
-
- for idx, val := range trustedEv.ByzantineValidators {
- if !bytes.Equal(byzValsCopy[idx].Address, val.Address) {
- return false
- }
- if byzValsCopy[idx].StakingPower != val.StakingPower {
- return false
- }
- }
-
- return true
- }
-
- // for all other evidence the evidence pool just checks if it is already in the pending db
- return evpool.isPending(ev)
-}
-
 // IsExpired checks whether evidence or a polc is expired by checking whether a height and time is older
 // than set by the evidence consensus parameters
 func (evpool *Pool) isExpired(height int64, time time.Time) bool {

evidence/pool_test.go

@@ -198,6 +198,9 @@ func TestReportConflictingVotes(t *testing.T) {
  // should be able to retrieve evidence from pool
  evList, _ = pool.PendingEvidence(defaultEvidenceMaxBytes)
  require.Equal(t, []types.Evidence{ev}, evList)
+
+ next = pool.EvidenceFront()
+ require.NotNil(t, next)
 }
 
 func TestEvidencePoolUpdate(t *testing.T) {
@@ -258,64 +261,32 @@ func TestVerifyDuplicatedEvidenceFails(t *testing.T) {
 
 // check that valid light client evidence is correctly validated and stored in
 // evidence pool
-func TestCheckEvidenceWithLightClientAttack(t *testing.T) {
+func TestLightClientAttackEvidenceLifecycle(t *testing.T) {
  var (
- nValidators = 5
- validatorPower int64 = 10
- height int64 = 10
+ height int64 = 100
+ commonHeight int64 = 90
  )
- conflictingVals, conflictingVoters, conflictingPrivVals := types.RandVoterSet(nValidators, validatorPower)
- trustedHeader := makeHeaderRandom(height)
- trustedHeader.Time = defaultEvidenceTime
-
- conflictingHeader := makeHeaderRandom(height)
- conflictingHeader.VotersHash = conflictingVoters.Hash()
-
- trustedHeader.VotersHash = conflictingHeader.VotersHash
- trustedHeader.NextValidatorsHash = conflictingHeader.NextValidatorsHash
- trustedHeader.ConsensusHash = conflictingHeader.ConsensusHash
- trustedHeader.AppHash = conflictingHeader.AppHash
- trustedHeader.LastResultsHash = conflictingHeader.LastResultsHash
-
- // for simplicity we are simulating a duplicate vote attack where all the validators in the
- // conflictingVals set voted twice
- blockID := makeBlockID(conflictingHeader.Hash(), 1000, []byte("partshash"))
- voteSet := types.NewVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVoters)
- commit, err := types.MakeCommit(blockID, height, 1, voteSet, conflictingPrivVals, defaultEvidenceTime)
- require.NoError(t, err)
- ev := &types.LightClientAttackEvidence{
- ConflictingBlock: &types.LightBlock{
- SignedHeader: &types.SignedHeader{
- Header: conflictingHeader,
- Commit: commit,
- },
- ValidatorSet: conflictingVals,
- VoterSet: conflictingVoters,
- },
- CommonHeight: height,
- TotalVotingPower: int64(nValidators) * validatorPower,
- ByzantineValidators: conflictingVals.Validators,
- Timestamp: defaultEvidenceTime,
- }
-
- trustedBlockID := makeBlockID(trustedHeader.Hash(), 1000, []byte("partshash"))
- trustedVoteSet := types.NewVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVoters)
- trustedCommit, err := types.MakeCommit(trustedBlockID, height, 1, trustedVoteSet, conflictingPrivVals,
- defaultEvidenceTime)
- require.NoError(t, err)
+ ev, trusted, common := makeLunaticEvidence(t, height, commonHeight,
+ 10, 5, 5, defaultEvidenceTime, defaultEvidenceTime.Add(1*time.Hour))
 
  state := sm.State{
- LastBlockTime: defaultEvidenceTime.Add(1 * time.Minute),
- LastBlockHeight: 11,
+ LastBlockTime: defaultEvidenceTime.Add(2 * time.Hour),
+ LastBlockHeight: 110,
  ConsensusParams: *types.DefaultConsensusParams(),
  }
  stateStore := &smmocks.Store{}
- stateStore.On("LoadValidators", height).Return(conflictingVals, nil)
- stateStore.On("LoadVoters", height, mock.AnythingOfType("*types.VoterParams")).Return(conflictingVoters, nil)
+ stateStore.On("LoadValidators", height).Return(trusted.ValidatorSet, nil)
+ stateStore.On("LoadValidators", commonHeight).Return(common.ValidatorSet, nil)
+ stateStore.On("LoadVoters", height, mock.AnythingOfType("*types.VoterParams")).Return(
+ ev.ConflictingBlock.VoterSet, nil) // Should use correct VoterSet for bls.VerifyAggregatedSignature
+ stateStore.On("LoadVoters", commonHeight, mock.AnythingOfType("*types.VoterParams")).Return(
+ ev.ConflictingBlock.VoterSet, nil) // Should use correct VoterSet for bls.VerifyAggregatedSignature
  stateStore.On("Load").Return(state, nil)
  blockStore := &mocks.BlockStore{}
- blockStore.On("LoadBlockMeta", height).Return(&types.BlockMeta{Header: *trustedHeader})
- blockStore.On("LoadBlockCommit", height).Return(trustedCommit)
+ blockStore.On("LoadBlockMeta", height).Return(&types.BlockMeta{Header: *trusted.Header})
+ blockStore.On("LoadBlockMeta", commonHeight).Return(&types.BlockMeta{Header: *common.Header})
+ blockStore.On("LoadBlockCommit", height).Return(trusted.Commit)
+ blockStore.On("LoadBlockCommit", commonHeight).Return(common.Commit)
 
  pool, err := evidence.NewPool(memdb.NewDB(), stateStore, blockStore)
  require.NoError(t, err)
@@ -324,8 +295,32 @@ func TestCheckEvidenceWithLightClientAttack(t *testing.T) {
  err = pool.AddEvidence(ev)
  assert.NoError(t, err)
 
- err = pool.CheckEvidence(types.EvidenceList{ev})
- assert.NoError(t, err)
+ hash := ev.Hash()
+
+ require.NoError(t, pool.AddEvidence(ev))
+ require.NoError(t, pool.AddEvidence(ev))
+
+ pendingEv, _ := pool.PendingEvidence(state.ConsensusParams.Evidence.MaxBytes)
+ require.Equal(t, 1, len(pendingEv))
+ require.Equal(t, ev, pendingEv[0])
+
+ require.NoError(t, pool.CheckEvidence(pendingEv))
+ require.Equal(t, ev, pendingEv[0])
+
+ state.LastBlockHeight++
+ state.LastBlockTime = state.LastBlockTime.Add(1 * time.Minute)
+ pool.Update(state, pendingEv)
+ require.Equal(t, hash, pendingEv[0].Hash())
+
+ remaindingEv, _ := pool.PendingEvidence(state.ConsensusParams.Evidence.MaxBytes)
+ require.Empty(t, remaindingEv)
+
+ // evidence is already committed so it shouldn't pass
+ require.Error(t, pool.CheckEvidence(types.EvidenceList{ev}))
+ require.NoError(t, pool.AddEvidence(ev))
+
+ remaindingEv, _ = pool.PendingEvidence(state.ConsensusParams.Evidence.MaxBytes)
+ require.Empty(t, remaindingEv)
 }
 
 // Tests that restarting the evidence pool after a potential failure will recover the
@@ -365,7 +360,7 @@ func TestRecoverPendingEvidence(t *testing.T) {
  Evidence: tmproto.EvidenceParams{
  MaxAgeNumBlocks: 20,
  MaxAgeDuration: 20 * time.Minute,
- MaxBytes: 1000,
+ MaxBytes: defaultEvidenceMaxBytes,
  },
  },
  }, nil)

evidence/verify.go

@@ -4,7 +4,6 @@ import (
  "bytes"
  "errors"
  "fmt"
- "sort"
  "time"
 
  "github.com/line/ostracon/light"
@@ -98,34 +97,6 @@ func (evpool *Pool) verify(evidence types.Evidence) error {
  if err != nil {
  return err
  }
- // find out what type of attack this was and thus extract the malicious validators. Note in the case of an
- // Amnesia attack we don't have any malicious validators.
- validators := ev.GetByzantineValidators(commonVoters, trustedHeader)
- // ensure this matches the validators that are listed in the evidence. They should be ordered based on power.
- if validators == nil && ev.ByzantineValidators != nil {
- return fmt.Errorf("expected nil validators from an amnesia light client attack but got %d",
- len(ev.ByzantineValidators))
- }
-
- if exp, got := len(validators), len(ev.ByzantineValidators); exp != got {
- return fmt.Errorf("expected %d byzantine validators from evidence but got %d",
- exp, got)
- }
-
- // ensure that both validator arrays are in the same order
- sort.Sort(types.ValidatorsByVotingPower(ev.ByzantineValidators))
-
- for idx, val := range validators {
- if !bytes.Equal(ev.ByzantineValidators[idx].Address, val.Address) {
- return fmt.Errorf("evidence contained a different byzantine validator address to the one we were expecting."+
- "Expected %v, got %v", val.Address, ev.ByzantineValidators[idx].Address)
- }
- if ev.ByzantineValidators[idx].StakingPower != val.StakingPower {
- return fmt.Errorf("evidence contained a byzantine validator with a different power to the one we were expecting."+
- "Expected %d, got %d", val.StakingPower, ev.ByzantineValidators[idx].StakingPower)
- }
- }
-
  return nil
  default:
  return fmt.Errorf("unrecognized evidence type: %T", evidence)
@@ -161,7 +132,7 @@ func VerifyLightClientAttack(
  }
 
  // In the case of equivocation and amnesia we expect all header hashes to be correctly derived
- } else if isInvalidHeader(trustedHeader.Header, e.ConflictingBlock.Header) {
+ } else if e.ConflictingHeaderIsInvalid(trustedHeader.Header) {
  return errors.New("common height is the same as conflicting block height so expected the conflicting" +
  " block to be correctly derived yet it wasn't")
  }
@@ -191,7 +162,7 @@ func VerifyLightClientAttack(
  trustedHeader.Hash())
  }
 
- return nil
+ return validateABCIEvidence(e, commonVoters, trustedHeader)
 }
 
 // VerifyDuplicateVote verifies DuplicateVoteEvidence against the state of full node. This involves the
@@ -262,6 +233,62 @@ func VerifyDuplicateVote(e *types.DuplicateVoteEvidence, chainID string, voterSe
  return nil
 }
 
+// validateABCIEvidence validates the ABCI component of the light client attack
+// evidence i.e voting power and byzantine validators
+func validateABCIEvidence(
+ ev *types.LightClientAttackEvidence,
+ commonVoter *types.VoterSet,
+ trustedHeader *types.SignedHeader,
+) error {
+ if evTotal, valsTotal := ev.TotalVotingPower, commonVoter.TotalVotingPower(); evTotal != valsTotal {
+ return fmt.Errorf("total voting power from the evidence and our validator set does not match (%d != %d)",
+ evTotal, valsTotal)
+ }
+
+ // Find out what type of attack this was and thus extract the malicious
+ // validators. Note, in the case of an Amnesia attack we don't have any
+ // malicious validators.
+ validators := ev.GetByzantineValidators(commonVoter, trustedHeader)
+
+ // Ensure this matches the validators that are listed in the evidence. They
+ // should be ordered based on power.
+ if validators == nil && ev.ByzantineValidators != nil {
+ return fmt.Errorf(
+ "expected nil validators from an amnesia light client attack but got %d",
+ len(ev.ByzantineValidators),
+ )
+ }
+
+ if exp, got := len(validators), len(ev.ByzantineValidators); exp != got {
+ return fmt.Errorf("expected %d byzantine validators from evidence but got %d", exp, got)
+ }
+
+ for idx, val := range validators {
+ if !bytes.Equal(ev.ByzantineValidators[idx].Address, val.Address) {
+ return fmt.Errorf(
+ "evidence contained an unexpected byzantine validator address; expected: %v, got: %v",
+ val.Address, ev.ByzantineValidators[idx].Address,
+ )
+ }
+
+ if ev.ByzantineValidators[idx].StakingPower != val.StakingPower {
+ return fmt.Errorf(
+ "evidence contained unexpected byzantine validator staking power; expected %d, got %d",
+ val.StakingPower, ev.ByzantineValidators[idx].StakingPower,
+ )
+ }
+
+ if ev.ByzantineValidators[idx].VotingPower != val.VotingPower {
+ return fmt.Errorf(
+ "evidence contained unexpected byzantine validator voting power; expected %d, got %d",
+ val.VotingPower, ev.ByzantineValidators[idx].VotingPower,
+ )
+ }
+ }
+
+ return nil
+}
+
 func getSignedHeader(blockStore BlockStore, height int64) (*types.SignedHeader, error) {
  blockMeta := blockStore.LoadBlockMeta(height)
  if blockMeta == nil {
@@ -276,15 +303,3 @@ func getSignedHeader(blockStore BlockStore, height int64) (*types.SignedHeader,
  Commit: commit,
  }, nil
 }
-
-// isInvalidHeader takes a trusted header and matches it againt a conflicting header
-// to determine whether the conflicting header was the product of a valid state transition
-// or not. If it is then all the deterministic fields of the header should be the same.
-// If not, it is an invalid header and constitutes a lunatic attack.
-func isInvalidHeader(trusted, conflicting *types.Header) bool {
- return !bytes.Equal(trusted.VotersHash, conflicting.VotersHash) ||
- !bytes.Equal(trusted.NextValidatorsHash, conflicting.NextValidatorsHash) ||
- !bytes.Equal(trusted.ConsensusHash, conflicting.ConsensusHash) ||
- !bytes.Equal(trusted.AppHash, conflicting.AppHash) ||
- !bytes.Equal(trusted.LastResultsHash, conflicting.LastResultsHash)
-}