diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 53bef9054..c23d5338f 100644 --- a/CHANGELOG_PENDING.md +++ b/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: diff --git a/evidence/errors.go b/evidence/errors.go new file mode 100644 index 000000000..7bad19c81 --- /dev/null +++ b/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" +} diff --git a/evidence/pool.go b/evidence/pool.go index 8c7d78694..acbd4bc23 100644 --- a/evidence/pool.go +++ b/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) diff --git a/evidence/pool_test.go b/evidence/pool_test.go index 844ca18af..ba39aaa61 100644 --- a/evidence/pool_test.go +++ b/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()) } diff --git a/evidence/reactor.go b/evidence/reactor.go index e4dbd51ad..7bedeeb43 100644 --- a/evidence/reactor.go +++ b/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: diff --git a/evidence/store.go b/evidence/store.go index 2bfe68523..f01e9de5f 100644 --- a/evidence/store.go +++ b/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. diff --git a/evidence/store_test.go b/evidence/store_test.go index 351abfee2..a0c7f5db1 100644 --- a/evidence/store_test.go +++ b/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) } diff --git a/rpc/core/evidence.go b/rpc/core/evidence.go index 4ae138e7e..7d7ac2ec7 100644 --- a/rpc/core/evidence.go +++ b/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 }