From 90ba63948ad48856fbe23c725b9ffd85c956c174 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 8 Feb 2019 18:25:48 -0500 Subject: [PATCH] Sec/bucky/35 commit duplicate evidence (#36) Don't add committed evidence to evpool --- evidence/pool.go | 3 ++- evidence/pool_test.go | 4 ++-- evidence/reactor_test.go | 4 ++-- evidence/store.go | 38 +++++++++++++++++++++----------------- evidence/store_test.go | 22 +++++++++++++++++++--- node/node.go | 3 +-- node/node_test.go | 6 ++---- state/execution.go | 1 - state/services.go | 1 + state/validation.go | 2 ++ 10 files changed, 52 insertions(+), 32 deletions(-) diff --git a/evidence/pool.go b/evidence/pool.go index b5fdbdf1d..3df0ec70a 100644 --- a/evidence/pool.go +++ b/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), diff --git a/evidence/pool_test.go b/evidence/pool_test.go index 1f4f1a06f..677ce78bb 100644 --- a/evidence/pool_test.go +++ b/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} diff --git a/evidence/reactor_test.go b/evidence/reactor_test.go index 1c4e731ab..635e9553f 100644 --- a/evidence/reactor_test.go +++ b/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)) } diff --git a/evidence/store.go b/evidence/store.go index 17b37aaba..998a763d4 100644 --- a/evidence/store.go +++ b/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()) } diff --git a/evidence/store_test.go b/evidence/store_test.go index 35eb28d01..5a7a8bd36 100644 --- a/evidence/store_test.go +++ b/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) } diff --git a/node/node.go b/node/node.go index 1b7319811..969452c40 100644 --- a/node/node.go +++ b/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) diff --git a/node/node_test.go b/node/node_test.go index 3218c8327..4b4610e10 100644 --- a/node/node_test.go +++ b/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) { diff --git a/state/execution.go b/state/execution.go index 470e22bc0..e3668c77a 100644 --- a/state/execution.go +++ b/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. diff --git a/state/services.go b/state/services.go index b8f1febe1..90f0cd019 100644 --- a/state/services.go +++ b/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 diff --git a/state/validation.go b/state/validation.go index cd571e34f..82c479ac6 100644 --- a/state/validation.go +++ b/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)