Sec/bucky/35 commit duplicate evidence (#36)

Don't add committed evidence to evpool

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),

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}

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 {
+		// nothin to do
+		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,7 +94,6 @@ 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.

state/services.go

@@ -80,6 +80,7 @@ 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

state/validation.go

@@ -185,6 +185,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)