From 26816e2648a73bb60336c6d8cb88607ca18253f8 Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Wed, 2 Sep 2020 20:33:03 +0200 Subject: [PATCH] evm: debug non-determinism (#496) * evm: debug non-determinism * add tests * changelog --- CHANGELOG.md | 1 + x/evm/types/journal.go | 45 +++++++++++++++++++- x/evm/types/journal_test.go | 83 +++++++++++++++++++++++++++++++++++++ x/evm/types/statedb.go | 51 +++++++++++++++-------- 4 files changed, 161 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 986269800..ae9dd3d48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (`x/evm`) [\#496](https://github.com/ChainSafe/ethermint/pull/496) Fix bugs on `journal.revert` and `CommitStateDB.Copy`. * (types) [\#480](https://github.com/ChainSafe/ethermint/pull/480) Update [BIP44](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki) coin type to `60` to satisfy [EIP84](https://github.com/ethereum/EIPs/issues/84). ## [v0.1.0] - 2020-08-23 diff --git a/x/evm/types/journal.go b/x/evm/types/journal.go index 825c5d4b6..a09a7ca6c 100644 --- a/x/evm/types/journal.go +++ b/x/evm/types/journal.go @@ -195,8 +195,25 @@ func (ch createObjectChange) revert(s *CommitStateDB) { // perform no-op return } + // remove from the slice - s.stateObjects = append(s.stateObjects[:idx], s.stateObjects[idx+1:]...) + delete(s.addressToObjectIndex, *ch.account) + + // if the slice contains one element, delete it + if len(s.stateObjects) == 1 { + s.stateObjects = []stateEntry{} + return + } + + // move the elements one position left on the array + for i := idx + 1; i < len(s.stateObjects); i++ { + s.stateObjects[i-1] = s.stateObjects[i] + // the new index is i - 1 + s.addressToObjectIndex[s.stateObjects[i].address] = i - 1 + } + + // finally, delete the last element of the slice to account for the removed object + s.stateObjects = s.stateObjects[:len(s.stateObjects)-1] } func (ch createObjectChange) dirtied() *ethcmn.Address { @@ -293,7 +310,31 @@ func (ch addLogChange) dirtied() *ethcmn.Address { } func (ch addPreimageChange) revert(s *CommitStateDB) { - delete(s.preimages, ch.hash) + idx, exists := s.hashToPreimageIndex[ch.hash] + if !exists { + // perform no-op + return + } + + // remove from the slice + delete(s.hashToPreimageIndex, ch.hash) + + // if the slice contains one element, delete it + if len(s.preimages) == 1 { + s.preimages = []preimageEntry{} + return + } + + // move the elements one position left on the array + for i := idx + 1; i < len(s.preimages); i++ { + s.preimages[i-1] = s.preimages[i] + // the new index is i - 1 + s.hashToPreimageIndex[s.preimages[i].hash] = i - 1 + } + + // finally, delete the last element + + s.preimages = s.preimages[:len(s.preimages)-1] } func (ch addPreimageChange) dirtied() *ethcmn.Address { diff --git a/x/evm/types/journal_test.go b/x/evm/types/journal_test.go index 04ce0b5b0..5d2b107b2 100644 --- a/x/evm/types/journal_test.go +++ b/x/evm/types/journal_test.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "os" "testing" @@ -237,6 +238,88 @@ func (suite *JournalTestSuite) TestJournal_append_revert() { suite.Require().Zero(idx) } +func (suite *JournalTestSuite) TestJournal_preimage_revert() { + suite.stateDB.preimages = []preimageEntry{ + { + hash: ethcmn.BytesToHash([]byte("hash")), + preimage: []byte("preimage0"), + }, + { + hash: ethcmn.BytesToHash([]byte("hash1")), + preimage: []byte("preimage1"), + }, + { + hash: ethcmn.BytesToHash([]byte("hash2")), + preimage: []byte("preimage2"), + }, + } + + for i, preimage := range suite.stateDB.preimages { + suite.stateDB.hashToPreimageIndex[preimage.hash] = i + } + + change := addPreimageChange{ + hash: ethcmn.BytesToHash([]byte("hash")), + } + + // delete first entry + change.revert(suite.stateDB) + suite.Require().Len(suite.stateDB.preimages, 2) + suite.Require().Equal(len(suite.stateDB.preimages), len(suite.stateDB.hashToPreimageIndex)) + + for i, entry := range suite.stateDB.preimages { + suite.Require().Equal(fmt.Sprintf("preimage%d", i+1), string(entry.preimage), entry.hash.String()) + idx, found := suite.stateDB.hashToPreimageIndex[entry.hash] + suite.Require().True(found) + suite.Require().Equal(i, idx) + } +} + +func (suite *JournalTestSuite) TestJournal_createObjectChange_revert() { + addr := ethcmn.BytesToAddress([]byte("addr")) + + suite.stateDB.stateObjects = []stateEntry{ + { + address: addr, + stateObject: &stateObject{ + address: addr, + }, + }, + { + address: ethcmn.BytesToAddress([]byte("addr1")), + stateObject: &stateObject{ + address: ethcmn.BytesToAddress([]byte("addr1")), + }, + }, + { + address: ethcmn.BytesToAddress([]byte("addr2")), + stateObject: &stateObject{ + address: ethcmn.BytesToAddress([]byte("addr2")), + }, + }, + } + + for i, so := range suite.stateDB.stateObjects { + suite.stateDB.addressToObjectIndex[so.address] = i + } + + change := createObjectChange{ + account: &addr, + } + + // delete first entry + change.revert(suite.stateDB) + suite.Require().Len(suite.stateDB.stateObjects, 2) + suite.Require().Equal(len(suite.stateDB.stateObjects), len(suite.stateDB.addressToObjectIndex)) + + for i, entry := range suite.stateDB.stateObjects { + suite.Require().Equal(ethcmn.BytesToAddress([]byte(fmt.Sprintf("addr%d", i+1))).String(), entry.address.String()) + idx, found := suite.stateDB.addressToObjectIndex[entry.address] + suite.Require().True(found) + suite.Require().Equal(i, idx) + } +} + func (suite *JournalTestSuite) TestJournal_dirty() { // dirty entry hasn't been set idx, ok := suite.journal.addressToJournalIndex[suite.address] diff --git a/x/evm/types/statedb.go b/x/evm/types/statedb.go index 3fdc0b33d..153cd3887 100644 --- a/x/evm/types/statedb.go +++ b/x/evm/types/statedb.go @@ -59,9 +59,8 @@ type CommitStateDB struct { // TODO: Determine if we actually need this as we do not need preimages in // the SDK, but it seems to be used elsewhere in Geth. - // - // NOTE: it is safe to use map here because it's only used for Copy - preimages map[ethcmn.Hash][]byte + preimages []preimageEntry + hashToPreimageIndex map[ethcmn.Hash]int // map from hash to the index of the preimages slice // DB error. // State objects are used by the consensus core and VM which are @@ -95,7 +94,8 @@ func NewCommitStateDB( stateObjects: []stateEntry{}, addressToObjectIndex: make(map[ethcmn.Address]int), stateObjectsDirty: make(map[ethcmn.Address]struct{}), - preimages: make(map[ethcmn.Hash][]byte), + preimages: []preimageEntry{}, + hashToPreimageIndex: make(map[ethcmn.Hash]int), journal: newJournal(), } } @@ -207,12 +207,14 @@ func (csdb *CommitStateDB) AddLog(log *ethtypes.Log) { // AddPreimage records a SHA3 preimage seen by the VM. func (csdb *CommitStateDB) AddPreimage(hash ethcmn.Hash, preimage []byte) { - if _, ok := csdb.preimages[hash]; !ok { + if _, ok := csdb.hashToPreimageIndex[hash]; !ok { csdb.journal.append(addPreimageChange{hash: hash}) pi := make([]byte, len(preimage)) copy(pi, preimage) - csdb.preimages[hash] = pi + + csdb.preimages = append(csdb.preimages, preimageEntry{hash: hash, preimage: pi}) + csdb.hashToPreimageIndex[hash] = len(csdb.preimages) - 1 } } @@ -358,7 +360,12 @@ func (csdb *CommitStateDB) GetRefund() uint64 { // Preimages returns a list of SHA3 preimages that have been submitted. func (csdb *CommitStateDB) Preimages() map[ethcmn.Hash][]byte { - return csdb.preimages + preimages := map[ethcmn.Hash][]byte{} + + for _, pe := range csdb.preimages { + preimages[pe.hash] = pe.preimage + } + return preimages } // HasSuicided returns if the given account for the specified address has been @@ -608,7 +615,8 @@ func (csdb *CommitStateDB) Reset(_ ethcmn.Hash) error { csdb.bhash = ethcmn.Hash{} csdb.txIndex = 0 csdb.logSize = 0 - csdb.preimages = make(map[ethcmn.Hash][]byte) + csdb.preimages = []preimageEntry{} + csdb.hashToPreimageIndex = make(map[ethcmn.Hash]int) csdb.clearJournalAndRefund() return nil @@ -685,27 +693,29 @@ func (csdb *CommitStateDB) Copy() *CommitStateDB { ctx: csdb.ctx, storeKey: csdb.storeKey, accountKeeper: csdb.accountKeeper, - stateObjects: make([]stateEntry, len(csdb.journal.dirties)), - addressToObjectIndex: make(map[ethcmn.Address]int, len(csdb.journal.dirties)), - stateObjectsDirty: make(map[ethcmn.Address]struct{}, len(csdb.journal.dirties)), + stateObjects: []stateEntry{}, + addressToObjectIndex: make(map[ethcmn.Address]int), + stateObjectsDirty: make(map[ethcmn.Address]struct{}), refund: csdb.refund, logSize: csdb.logSize, - preimages: make(map[ethcmn.Hash][]byte), + preimages: make([]preimageEntry, len(csdb.preimages)), + hashToPreimageIndex: make(map[ethcmn.Hash]int, len(csdb.hashToPreimageIndex)), journal: newJournal(), } // copy the dirty states, logs, and preimages - for i, dirty := range csdb.journal.dirties { + for _, dirty := range csdb.journal.dirties { // There is a case where an object is in the journal but not in the // stateObjects: OOG after touch on ripeMD prior to Byzantium. Thus, we // need to check for nil. // // Ref: https://github.com/ethereum/go-ethereum/pull/16485#issuecomment-380438527 if idx, exist := csdb.addressToObjectIndex[dirty.address]; exist { - state.stateObjects[i] = stateEntry{ + state.stateObjects = append(state.stateObjects, stateEntry{ address: dirty.address, stateObject: csdb.stateObjects[idx].stateObject.deepCopy(state), - } + }) + state.addressToObjectIndex[dirty.address] = len(state.stateObjects) - 1 state.stateObjectsDirty[dirty.address] = struct{}{} } } @@ -721,8 +731,9 @@ func (csdb *CommitStateDB) Copy() *CommitStateDB { } // copy pre-images - for hash, preimage := range csdb.preimages { - state.preimages[hash] = preimage + for i, preimageEntry := range csdb.preimages { + state.preimages[i] = preimageEntry + state.hashToPreimageIndex[preimageEntry.hash] = i } return state @@ -854,3 +865,9 @@ func (csdb *CommitStateDB) setStateObject(so *stateObject) { func (csdb *CommitStateDB) RawDump() ethstate.Dump { return ethstate.Dump{} } + +type preimageEntry struct { + // hash key of the preimage entry + hash ethcmn.Hash + preimage []byte +}