Fixes for duplicate evidence

CHANGELOG_PENDING.md

@@ -6,8 +6,23 @@ Special thanks to external contributors on this release:
 
 ### BREAKING CHANGES:
 
+* CLI/RPC/Config
+
+* Apps
+
+* Go API
+
+* Blockchain Protocol
+
+ - [types] Reject blocks which contain already committed evidence
+
+* P2P Protocol
+
 ### FEATURES:
 
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
+
+ - [evidence] Do not store evidence which was already marked as committed
+

consensus/reactor_test.go

@@ -211,6 +211,7 @@ func (m *mockEvidencePool) Update(block *types.Block, state sm.State) {
 	}
 	m.height++
 }
+func (m *mockEvidencePool) IsCommitted(types.Evidence) bool { return false }
 
 //------------------------------------
 

evidence/pool.go

@@ -28,7 +28,8 @@ type EvidencePool struct {
 	state sm.State
 }
 
-func NewEvidencePool(stateDB dbm.DB, evidenceStore *EvidenceStore) *EvidencePool {
+func NewEvidencePool(stateDB, evidenceDB dbm.DB) *EvidencePool {
+	evidenceStore := NewEvidenceStore(evidenceDB)
 	evpool := &EvidencePool{
 		stateDB:       stateDB,
 		state:         sm.LoadState(stateDB),
@@ -132,6 +133,12 @@ func (evpool *EvidencePool) MarkEvidenceAsCommitted(height int64, evidence []typ
 
 }
 
+// IsCommitted returns true if we have already seen this exact evidence and it is already marked as committed.
+func (evpool *EvidencePool) IsCommitted(evidence types.Evidence) bool {
+	ei := evpool.evidenceStore.getEvidenceInfo(evidence)
+	return ei.Evidence != nil && ei.Committed
+}
+
 func (evpool *EvidencePool) removeEvidence(height, maxAge int64, blockEvidenceMap map[string]struct{}) {
 	for e := evpool.evidenceList.Front(); e != nil; e = e.Next() {
 		ev := e.Value.(types.Evidence)

evidence/pool_test.go

@@ -56,8 +56,8 @@ func TestEvidencePool(t *testing.T) {
 	valAddr := []byte("val1")
 	height := int64(5)
 	stateDB := initializeValidatorState(valAddr, height)
-	store := NewEvidenceStore(dbm.NewMemDB())
-	pool := NewEvidencePool(stateDB, store)
+	evidenceDB := dbm.NewMemDB()
+	pool := NewEvidencePool(stateDB, evidenceDB)
 
 	goodEvidence := types.NewMockGoodEvidence(height, 0, valAddr)
 	badEvidence := types.MockBadEvidence{goodEvidence}
@@ -84,3 +84,24 @@ func TestEvidencePool(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, 1, pool.evidenceList.Len())
 }
+
+func TestEvidencePoolIsCommitted(t *testing.T) {
+	// Initialization:
+	valAddr := []byte("validator_address")
+	height := int64(42)
+	stateDB := initializeValidatorState(valAddr, height)
+	evidenceDB := dbm.NewMemDB()
+	pool := NewEvidencePool(stateDB, evidenceDB)
+
+	// evidence not seen yet:
+	evidence := types.NewMockGoodEvidence(height, 0, valAddr)
+	assert.False(t, pool.IsCommitted(evidence))
+
+	// evidence seen but not yet committed:
+	assert.NoError(t, pool.AddEvidence(evidence))
+	assert.False(t, pool.IsCommitted(evidence))
+
+	// evidence seen and committed:
+	pool.MarkEvidenceAsCommitted(height, []types.Evidence{evidence})
+	assert.True(t, pool.IsCommitted(evidence))
+}

evidence/reactor_test.go

@@ -37,8 +37,8 @@ func makeAndConnectEvidenceReactors(config *cfg.Config, stateDBs []dbm.DB) []*Ev
 	logger := evidenceLogger()
 	for i := 0; i < N; i++ {
 
-		store := NewEvidenceStore(dbm.NewMemDB())
-		pool := NewEvidencePool(stateDBs[i], store)
+		evidenceDB := dbm.NewMemDB()
+		pool := NewEvidencePool(stateDBs[i], evidenceDB)
 		reactors[i] = NewEvidenceReactor(pool)
 		reactors[i].SetLogger(logger.With("validator", i))
 	}

evidence/store.go

@@ -117,32 +117,33 @@ func (store *EvidenceStore) listEvidence(prefixKey string, maxNum int64) (eviden
 	return evidence
 }
 
-// GetEvidence fetches the evidence with the given height and hash.
-func (store *EvidenceStore) GetEvidence(height int64, hash []byte) *EvidenceInfo {
+// GetEvidenceInfo fetches the EvidenceInfo with the given height and hash.
+// If not found, ei.Evidence is nil.
+func (store *EvidenceStore) GetEvidenceInfo(height int64, hash []byte) EvidenceInfo {
 	key := keyLookupFromHeightAndHash(height, hash)
 	val := store.db.Get(key)
 
 	if len(val) == 0 {
-		return nil
+		return EvidenceInfo{}
 	}
 	var ei EvidenceInfo
 	err := cdc.UnmarshalBinaryBare(val, &ei)
 	if err != nil {
 		panic(err)
 	}
-	return &ei
+	return ei
 }
 
 // AddNewEvidence adds the given evidence to the database.
 // It returns false if the evidence is already stored.
 func (store *EvidenceStore) AddNewEvidence(evidence types.Evidence, priority int64) bool {
 	// check if we already have seen it
-	ei_ := store.GetEvidence(evidence.Height(), evidence.Hash())
-	if ei_ != nil && ei_.Evidence != nil {
+	ei := store.getEvidenceInfo(evidence)
+	if ei.Evidence != nil {
 		return false
 	}
 
-	ei := EvidenceInfo{
+	ei = EvidenceInfo{
 		Committed: false,
 		Priority:  priority,
 		Evidence:  evidence,
@@ -165,6 +166,11 @@ func (store *EvidenceStore) AddNewEvidence(evidence types.Evidence, priority int
 // MarkEvidenceAsBroadcasted removes evidence from Outqueue.
 func (store *EvidenceStore) MarkEvidenceAsBroadcasted(evidence types.Evidence) {
 	ei := store.getEvidenceInfo(evidence)
+	if ei.Evidence == nil {
+		// nothing to do; we did not store the evidence yet (AddNewEvidence):
+		return
+	}
+	// remove from the outqueue
 	key := keyOutqueue(evidence, ei.Priority)
 	store.db.Delete(key)
 }
@@ -177,8 +183,12 @@ func (store *EvidenceStore) MarkEvidenceAsCommitted(evidence types.Evidence) {
 	pendingKey := keyPending(evidence)
 	store.db.Delete(pendingKey)
 
-	ei := store.getEvidenceInfo(evidence)
-	ei.Committed = true
+	// committed EvidenceInfo doens't need priority
+	ei := EvidenceInfo{
+		Committed: true,
+		Evidence:  evidence,
+		Priority:  0,
+	}
 
 	lookupKey := keyLookup(evidence)
 	store.db.SetSync(lookupKey, cdc.MustMarshalBinaryBare(ei))
@@ -187,13 +197,7 @@ func (store *EvidenceStore) MarkEvidenceAsCommitted(evidence types.Evidence) {
 //---------------------------------------------------
 // utils
 
+// getEvidenceInfo is convenience for calling GetEvidenceInfo if we have the full evidence.
 func (store *EvidenceStore) getEvidenceInfo(evidence types.Evidence) EvidenceInfo {
-	key := keyLookup(evidence)
-	var ei EvidenceInfo
-	b := store.db.Get(key)
-	err := cdc.UnmarshalBinaryBare(b, &ei)
-	if err != nil {
-		panic(err)
-	}
-	return ei
+	return store.GetEvidenceInfo(evidence.Height(), evidence.Hash())
 }

evidence/store_test.go

@@ -27,6 +27,21 @@ func TestStoreAddDuplicate(t *testing.T) {
 	assert.False(added)
 }
 
+func TestStoreCommitDuplicate(t *testing.T) {
+	assert := assert.New(t)
+
+	db := dbm.NewMemDB()
+	store := NewEvidenceStore(db)
+
+	priority := int64(10)
+	ev := types.NewMockGoodEvidence(2, 1, []byte("val1"))
+
+	store.MarkEvidenceAsCommitted(ev)
+
+	added := store.AddNewEvidence(ev, priority)
+	assert.False(added)
+}
+
 func TestStoreMark(t *testing.T) {
 	assert := assert.New(t)
 
@@ -46,7 +61,7 @@ func TestStoreMark(t *testing.T) {
 	assert.True(added)
 
 	// get the evidence. verify. should be uncommitted
-	ei := store.GetEvidence(ev.Height(), ev.Hash())
+	ei := store.GetEvidenceInfo(ev.Height(), ev.Hash())
 	assert.Equal(ev, ei.Evidence)
 	assert.Equal(priority, ei.Priority)
 	assert.False(ei.Committed)
@@ -72,9 +87,10 @@ func TestStoreMark(t *testing.T) {
 	assert.Equal(0, len(pendingEv))
 
 	// evidence should show committed
-	ei = store.GetEvidence(ev.Height(), ev.Hash())
+	newPriority := int64(0)
+	ei = store.GetEvidenceInfo(ev.Height(), ev.Hash())
 	assert.Equal(ev, ei.Evidence)
-	assert.Equal(priority, ei.Priority)
+	assert.Equal(newPriority, ei.Priority)
 	assert.True(ei.Committed)
 }
 

node/node.go

@@ -345,8 +345,7 @@ func NewNode(config *cfg.Config,
 		return nil, err
 	}
 	evidenceLogger := logger.With("module", "evidence")
-	evidenceStore := evidence.NewEvidenceStore(evidenceDB)
-	evidencePool := evidence.NewEvidencePool(stateDB, evidenceStore)
+	evidencePool := evidence.NewEvidencePool(stateDB, evidenceDB)
 	evidencePool.SetLogger(evidenceLogger)
 	evidenceReactor := evidence.NewEvidenceReactor(evidencePool)
 	evidenceReactor.SetLogger(evidenceLogger)

node/node_test.go

@@ -227,11 +227,10 @@ func TestCreateProposalBlock(t *testing.T) {
 	mempool.SetLogger(logger)
 
 	// Make EvidencePool
-	types.RegisterMockEvidencesGlobal()
+	types.RegisterMockEvidencesGlobal() // XXX!
 	evidence.RegisterMockEvidences()
 	evidenceDB := dbm.NewMemDB()
-	evidenceStore := evidence.NewEvidenceStore(evidenceDB)
-	evidencePool := evidence.NewEvidencePool(stateDB, evidenceStore)
+	evidencePool := evidence.NewEvidencePool(stateDB, evidenceDB)
 	evidencePool.SetLogger(logger)
 
 	// fill the evidence pool with more evidence
@@ -270,7 +269,6 @@ func TestCreateProposalBlock(t *testing.T) {
 
 	err = blockExec.ValidateBlock(state, block)
 	assert.NoError(t, err)
-
 }
 
 func state(nVals int, height int64) (sm.State, dbm.DB) {

state/execution.go

@@ -94,15 +94,14 @@ func (blockExec *BlockExecutor) CreateProposalBlock(
 	txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas)
 
 	return state.MakeBlock(height, txs, commit, evidence, proposerAddr)
-
 }
 
 // ValidateBlock validates the given block against the given state.
 // If the block is invalid, it returns an error.
 // Validation does not mutate state, but does require historical information from the stateDB,
 // ie. to verify evidence from a validator at an old height.
 func (blockExec *BlockExecutor) ValidateBlock(state State, block *types.Block) error {
-	return validateBlock(blockExec.db, state, block)
+	return validateBlock(blockExec.evpool, blockExec.db, state, block)
 }
 
 // ApplyBlock validates the block against the state, executes it against the app,

state/services.go

@@ -80,10 +80,13 @@ type BlockStore interface {
 // evidence pool
 
 // EvidencePool defines the EvidencePool interface used by the ConsensusState.
+// Get/Set/Commit
 type EvidencePool interface {
 	PendingEvidence(int64) []types.Evidence
 	AddEvidence(types.Evidence) error
 	Update(*types.Block, State)
+	// IsCommitted indicates if this evidence was already marked committed in another block.
+	IsCommitted(types.Evidence) bool
 }
 
 // MockMempool is an empty implementation of a Mempool, useful for testing.
@@ -92,3 +95,4 @@ type MockEvidencePool struct{}
 func (m MockEvidencePool) PendingEvidence(int64) []types.Evidence { return nil }
 func (m MockEvidencePool) AddEvidence(types.Evidence) error       { return nil }
 func (m MockEvidencePool) Update(*types.Block, State)             {}
+func (m MockEvidencePool) IsCommitted(types.Evidence) bool        { return false }

state/validation.go

@@ -13,7 +13,7 @@ import (
 //-----------------------------------------------------
 // Validate block
 
-func validateBlock(stateDB dbm.DB, state State, block *types.Block) error {
+func validateBlock(evidencePool EvidencePool, stateDB dbm.DB, state State, block *types.Block) error {
 	// Validate internal consistency.
 	if err := block.ValidateBasic(); err != nil {
 		return err
@@ -145,6 +145,9 @@ func validateBlock(stateDB dbm.DB, state State, block *types.Block) error {
 		if err := VerifyEvidence(stateDB, state, ev); err != nil {
 			return types.NewErrEvidenceInvalid(ev, err)
 		}
+		if evidencePool != nil && evidencePool.IsCommitted(ev) {
+			return types.NewErrEvidenceInvalid(ev, errors.New("evidence was already committed"))
+		}
 	}
 
 	// NOTE: We can't actually verify it's the right proposer because we dont
@@ -185,6 +188,8 @@ func VerifyEvidence(stateDB dbm.DB, state State, evidence types.Evidence) error
 	// The address must have been an active validator at the height.
 	// NOTE: we will ignore evidence from H if the key was not a validator
 	// at H, even if it is a validator at some nearby H'
+	// XXX: this makes lite-client bisection as is unsafe
+	// See https://github.com/tendermint/tendermint/issues/3244
 	ev := evidence
 	height, addr := ev.Height(), ev.Address()
 	_, val := valset.GetByAddress(addr)

state/validation_test.go

@@ -121,6 +121,31 @@ func TestValidateBlockEvidence(t *testing.T) {
 	require.True(t, ok)
 }
 
+// always returns true if asked if any evidence was already committed.
+type mockEvPoolAlwaysCommitted struct{}
+
+func (m mockEvPoolAlwaysCommitted) PendingEvidence(int64) []types.Evidence { return nil }
+func (m mockEvPoolAlwaysCommitted) AddEvidence(types.Evidence) error       { return nil }
+func (m mockEvPoolAlwaysCommitted) Update(*types.Block, State)             {}
+func (m mockEvPoolAlwaysCommitted) IsCommitted(types.Evidence) bool        { return true }
+
+func TestValidateFailBlockOnCommittedEvidence(t *testing.T) {
+	var height int64 = 1
+	state, stateDB := state(1, int(height))
+
+	blockExec := NewBlockExecutor(stateDB, log.TestingLogger(), nil, nil, mockEvPoolAlwaysCommitted{})
+	// A block with a couple pieces of evidence passes.
+	block := makeBlock(state, height)
+	addr, _ := state.Validators.GetByIndex(0)
+	alreadyCommittedEvidence := types.NewMockGoodEvidence(height, 0, addr)
+	block.Evidence.Evidence = []types.Evidence{alreadyCommittedEvidence}
+	block.EvidenceHash = block.Evidence.Hash()
+	err := blockExec.ValidateBlock(state, block)
+
+	require.Error(t, err)
+	require.IsType(t, err, &types.ErrEvidenceInvalid{})
+}
+
 /*
 	TODO(#2589):
 	- test unmarshalling BlockParts that are too big into a Block that