From 90ba63948ad48856fbe23c725b9ffd85c956c174 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 8 Feb 2019 18:25:48 -0500 Subject: [PATCH 1/2] 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) From 87bdc42bf827c659aaa4b3c9c796bc3f8e698a87 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Sat, 9 Feb 2019 00:30:45 +0100 Subject: [PATCH 2/2] Reject blocks with committed evidence (#37) * evidence: NewEvidencePool takes evidenceDB * evidence: failing TestStoreCommitDuplicate tendermint/security#35 * GetEvidence -> GetEvidenceInfo * fix TestStoreCommitDuplicate * comment in VerifyEvidence * add check if evidence was already seen - modify EventPool interface (EventStore is not known in ApplyBlock): - add IsCommitted method to iface - add test * update changelog * fix TestStoreMark: - priority in evidence info gets reset to zero after evidence gets committed * review comments: simplify EvidencePool.IsCommitted - delete obsolete EvidenceStore.IsCommitted * add simple test for IsCommitted * update changelog: this is actually breaking (PR number still missing) * fix TestStoreMark: - priority in evidence info gets reset to zero after evidence gets committed * review suggestion: simplify return --- CHANGELOG_PENDING.md | 15 +++++++++++++++ consensus/reactor_test.go | 1 + evidence/pool.go | 6 ++++++ evidence/pool_test.go | 21 +++++++++++++++++++++ evidence/store.go | 2 +- state/execution.go | 2 +- state/services.go | 3 +++ state/validation.go | 5 ++++- state/validation_test.go | 25 +++++++++++++++++++++++++ 9 files changed, 77 insertions(+), 3 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 4548eb1eb..91eed1882 100644 --- a/CHANGELOG_PENDING.md +++ b/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 + diff --git a/consensus/reactor_test.go b/consensus/reactor_test.go index 28e245aec..d35eaf3c0 100644 --- a/consensus/reactor_test.go +++ b/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 } //------------------------------------ diff --git a/evidence/pool.go b/evidence/pool.go index 3df0ec70a..18ccb3344 100644 --- a/evidence/pool.go +++ b/evidence/pool.go @@ -133,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) diff --git a/evidence/pool_test.go b/evidence/pool_test.go index 677ce78bb..30b20011e 100644 --- a/evidence/pool_test.go +++ b/evidence/pool_test.go @@ -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)) +} diff --git a/evidence/store.go b/evidence/store.go index 998a763d4..464d6138e 100644 --- a/evidence/store.go +++ b/evidence/store.go @@ -167,7 +167,7 @@ func (store *EvidenceStore) AddNewEvidence(evidence types.Evidence, priority int func (store *EvidenceStore) MarkEvidenceAsBroadcasted(evidence types.Evidence) { ei := store.getEvidenceInfo(evidence) if ei.Evidence == nil { - // nothin to do + // nothing to do; we did not store the evidence yet (AddNewEvidence): return } // remove from the outqueue diff --git a/state/execution.go b/state/execution.go index e3668c77a..8ab95839e 100644 --- a/state/execution.go +++ b/state/execution.go @@ -101,7 +101,7 @@ func (blockExec *BlockExecutor) CreateProposalBlock( // 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, diff --git a/state/services.go b/state/services.go index 90f0cd019..02c3aa7d1 100644 --- a/state/services.go +++ b/state/services.go @@ -85,6 +85,8 @@ 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. @@ -93,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 } diff --git a/state/validation.go b/state/validation.go index 82c479ac6..3cb0ee8fb 100644 --- a/state/validation.go +++ b/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 diff --git a/state/validation_test.go b/state/validation_test.go index 12aaf6361..a873855a9 100644 --- a/state/validation_test.go +++ b/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