check evidence hasn't already been stored (#4632)

Add Has function, create better handling of errors when adding evidence, usage of error types.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

CHANGELOG_PENDING.md

@@ -40,6 +40,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 - [lite2] [\#4575](https://github.com/tendermint/tendermint/pull/4575) Use bisection for within-range verification (@cmwaters)
 - [tools] \#4615 Allow developers to use Docker to generate proto stubs, via `make proto-gen-docker`.
 - [p2p] \#4621(https://github.com/tendermint/tendermint/pull/4621) ban peers when messages are unsolicited or too frequent (@cmwaters)
+- [evidence] [\#4632](https://github.com/tendermint/tendermint/pull/4632) Inbound evidence checked if already existing (@cmwaters)
 
 ### BUG FIXES:
 

evidence/errors.go

@@ -0,0 +1,21 @@
+package evidence
+
+import (
+	"fmt"
+)
+
+// ErrInvalidEvidence returns when evidence failed to validate
+type ErrInvalidEvidence struct {
+	Reason error
+}
+
+func (e ErrInvalidEvidence) Error() string {
+	return fmt.Sprintf("evidence is not valid: %v ", e.Reason)
+}
+
+// ErrEvidenceAlreadyStored indicates that the evidence has already been stored in the evidence db
+type ErrEvidenceAlreadyStored struct{}
+
+func (e ErrEvidenceAlreadyStored) Error() string {
+	return "evidence is already stored"
+}

evidence/pool.go

@@ -97,11 +97,13 @@ func (evpool *Pool) Update(block *types.Block, state sm.State) {
 // AddEvidence checks the evidence is valid and adds it to the pool.
 func (evpool *Pool) AddEvidence(evidence types.Evidence) error {
 
-	// TODO: check if we already have evidence for this
-	// validator at this height so we dont get spammed
+	// check if evidence is already stored
+	if evpool.store.Has(evidence) {
+		return ErrEvidenceAlreadyStored{}
+	}
 
 	if err := sm.VerifyEvidence(evpool.stateDB, evpool.State(), evidence); err != nil {
-		return err
+		return ErrInvalidEvidence{err}
 	}
 
 	// fetch the validator and return its voting power as its priority
@@ -113,10 +115,9 @@ func (evpool *Pool) AddEvidence(evidence types.Evidence) error {
 	_, val := valset.GetByAddress(evidence.Address())
 	priority := val.VotingPower
 
-	added := evpool.store.AddNewEvidence(evidence, priority)
-	if !added {
-		// evidence already known, just ignore
-		return nil
+	_, err = evpool.store.AddNewEvidence(evidence, priority)
+	if err != nil {
+		return err
 	}
 
 	evpool.logger.Info("Verified new evidence of byzantine behaviour", "evidence", evidence)

evidence/pool_test.go

@@ -70,7 +70,7 @@ func TestEvidencePool(t *testing.T) {
 
 	// bad evidence
 	err := pool.AddEvidence(badEvidence)
-	assert.NotNil(t, err)
+	assert.Error(t, err)
 	// err: evidence created at 2019-01-01 00:00:00 +0000 UTC has expired. Evidence can not be older than: ...
 
 	var wg sync.WaitGroup
@@ -81,14 +81,14 @@ func TestEvidencePool(t *testing.T) {
 	}()
 
 	err = pool.AddEvidence(goodEvidence)
-	assert.Nil(t, err)
+	assert.NoError(t, err)
 	wg.Wait()
 
 	assert.Equal(t, 1, pool.evidenceList.Len())
 
-	// if we send it again, it shouldnt change the size
+	// if we send it again, it shouldnt add and return an error
 	err = pool.AddEvidence(goodEvidence)
-	assert.Nil(t, err)
+	assert.Error(t, err)
 	assert.Equal(t, 1, pool.evidenceList.Len())
 }
 

evidence/reactor.go

@@ -82,10 +82,18 @@ func (evR *Reactor) Receive(chID byte, src p2p.Peer, msgBytes []byte) {
 	case *ListMessage:
 		for _, ev := range msg.Evidence {
 			err := evR.evpool.AddEvidence(ev)
-			if err != nil {
-				evR.Logger.Info("Evidence is not valid", "evidence", msg.Evidence, "err", err)
+			switch err.(type) {
+			case ErrInvalidEvidence:
+				evR.Logger.Error("Evidence is not valid", "evidence", msg.Evidence, "err", err)
 				// punish peer
 				evR.Switch.StopPeerForError(src, err)
+				return
+			case ErrEvidenceAlreadyStored:
+				evR.Logger.Debug("Evidence already exists", "evidence", msg.Evidence)
+			case nil:
+			default:
+				evR.Logger.Error("Evidence has not been added", "evidence", msg.Evidence, "err", err)
+				return
 			}
 		}
 	default:

evidence/store.go

@@ -140,16 +140,22 @@ func (store *Store) GetInfo(height int64, hash []byte) Info {
 	return ei
 }
 
+// Has checks if the evidence is already stored
+func (store *Store) Has(evidence types.Evidence) bool {
+	key := keyLookup(evidence)
+	ok, _ := store.db.Has(key)
+	return ok
+}
+
 // AddNewEvidence adds the given evidence to the database.
 // It returns false if the evidence is already stored.
-func (store *Store) AddNewEvidence(evidence types.Evidence, priority int64) bool {
+func (store *Store) AddNewEvidence(evidence types.Evidence, priority int64) (bool, error) {
 	// check if we already have seen it
-	ei := store.getInfo(evidence)
-	if ei.Evidence != nil {
-		return false
+	if store.Has(evidence) {
+		return false, nil
 	}
 
-	ei = Info{
+	ei := Info{
 		Committed: false,
 		Priority:  priority,
 		Evidence:  evidence,
@@ -157,16 +163,23 @@ func (store *Store) AddNewEvidence(evidence types.Evidence, priority int64) bool
 	eiBytes := cdc.MustMarshalBinaryBare(ei)
 
 	// add it to the store
+	var err error
 	key := keyOutqueue(evidence, priority)
-	store.db.Set(key, eiBytes)
+	if err = store.db.Set(key, eiBytes); err != nil {
+		return false, err
+	}
 
 	key = keyPending(evidence)
-	store.db.Set(key, eiBytes)
+	if err = store.db.Set(key, eiBytes); err != nil {
+		return false, err
+	}
 
 	key = keyLookup(evidence)
-	store.db.SetSync(key, eiBytes)
+	if err = store.db.SetSync(key, eiBytes); err != nil {
+		return false, err
+	}
 
-	return true
+	return true, nil
 }
 
 // MarkEvidenceAsBroadcasted removes evidence from Outqueue.

evidence/store_test.go

@@ -1,6 +1,7 @@
 package evidence
 
 import (
+	"github.com/stretchr/testify/require"
 	"testing"
 	"time"
 
@@ -21,11 +22,13 @@ func TestStoreAddDuplicate(t *testing.T) {
 	priority := int64(10)
 	ev := types.NewMockEvidence(2, time.Now().UTC(), 1, []byte("val1"))
 
-	added := store.AddNewEvidence(ev, priority)
+	added, err := store.AddNewEvidence(ev, priority)
+	require.NoError(t, err)
 	assert.True(added)
 
 	// cant add twice
-	added = store.AddNewEvidence(ev, priority)
+	added, err = store.AddNewEvidence(ev, priority)
+	require.NoError(t, err)
 	assert.False(added)
 }
 
@@ -40,7 +43,8 @@ func TestStoreCommitDuplicate(t *testing.T) {
 
 	store.MarkEvidenceAsCommitted(ev)
 
-	added := store.AddNewEvidence(ev, priority)
+	added, err := store.AddNewEvidence(ev, priority)
+	require.NoError(t, err)
 	assert.False(added)
 }
 
@@ -59,7 +63,8 @@ func TestStoreMark(t *testing.T) {
 	priority := int64(10)
 	ev := types.NewMockEvidence(2, time.Now().UTC(), 1, []byte("val1"))
 
-	added := store.AddNewEvidence(ev, priority)
+	added, err := store.AddNewEvidence(ev, priority)
+	require.NoError(t, err)
 	assert.True(added)
 
 	// get the evidence. verify. should be uncommitted
@@ -116,7 +121,8 @@ func TestStorePriority(t *testing.T) {
 	}
 
 	for _, c := range cases {
-		added := store.AddNewEvidence(c.ev, c.priority)
+		added, err := store.AddNewEvidence(c.ev, c.priority)
+		require.NoError(t, err)
 		assert.True(added)
 	}
 

rpc/core/evidence.go

@@ -1,6 +1,7 @@
 package core
 
 import (
+	"github.com/tendermint/tendermint/evidence"
 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
 	rpctypes "github.com/tendermint/tendermint/rpc/lib/types"
 	"github.com/tendermint/tendermint/types"
@@ -10,8 +11,8 @@ import (
 // More: https://docs.tendermint.com/master/rpc/#/Info/broadcast_evidence
 func BroadcastEvidence(ctx *rpctypes.Context, ev types.Evidence) (*ctypes.ResultBroadcastEvidence, error) {
 	err := evidencePool.AddEvidence(ev)
-	if err != nil {
-		return nil, err
+	if _, ok := err.(evidence.ErrEvidenceAlreadyStored); err == nil || ok {
+		return &ctypes.ResultBroadcastEvidence{Hash: ev.Hash()}, nil
 	}
-	return &ctypes.ResultBroadcastEvidence{Hash: ev.Hash()}, nil
+	return nil, err
 }