evm: debug non-determinism (#496)

* evm: debug non-determinism

* add tests

* changelog

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

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 {

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]

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.
-	//
+	preimages           []preimageEntry
-	// NOTE: it is safe to use map here because it's only used for Copy
+	hashToPreimageIndex map[ethcmn.Hash]int // map from hash to the index of the preimages slice
-	preimages map[ethcmn.Hash][]byte
 
 	// 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)),
+		stateObjects:         []stateEntry{},
-		addressToObjectIndex: make(map[ethcmn.Address]int, len(csdb.journal.dirties)),
+		addressToObjectIndex: make(map[ethcmn.Address]int),
-		stateObjectsDirty:    make(map[ethcmn.Address]struct{}, len(csdb.journal.dirties)),
+		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 {
+	for i, preimageEntry := range csdb.preimages {
-		state.preimages[hash] = preimage
+		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
+}