From 90f39390bca1f958b022154cf938ec4582111c86 Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Mon, 13 Jul 2020 22:01:45 +0200 Subject: [PATCH] evm: fix non-determinism (#352) * evm: fix non-determinism * fixes * typo * fix tests * use Storage slice * more fixes * lint * merge development * additional tests --- tests/rpc_test.go | 4 +- types/code.go | 34 +----- x/evm/genesis.go | 4 +- x/evm/genesis_test.go | 4 +- x/evm/keeper/querier.go | 4 +- x/evm/types/errors.go | 12 ++ x/evm/types/genesis.go | 36 +----- x/evm/types/genesis_test.go | 43 ++------ x/evm/types/journal.go | 83 ++++++++++++-- x/evm/types/journal_test.go | 13 ++- x/evm/types/state_object.go | 182 +++++++++++++++++++------------ x/evm/types/state_object_test.go | 8 ++ x/evm/types/statedb.go | 168 ++++++++++++++++------------ x/evm/types/statedb_test.go | 7 ++ x/evm/types/storage.go | 72 ++++++++++++ x/evm/types/storage_test.go | 84 ++++++++++++++ 16 files changed, 503 insertions(+), 255 deletions(-) create mode 100644 x/evm/types/errors.go create mode 100644 x/evm/types/storage.go create mode 100644 x/evm/types/storage_test.go diff --git a/tests/rpc_test.go b/tests/rpc_test.go index e3cf506d..4d8d66ed 100644 --- a/tests/rpc_test.go +++ b/tests/rpc_test.go @@ -768,7 +768,7 @@ func TestEth_ExportAccount(t *testing.T) { require.Equal(t, "0x1122334455667788990011223344556677889900", account.Address.Hex()) require.Equal(t, big.NewInt(0), account.Balance) require.Equal(t, hexutil.Bytes(nil), account.Code) - require.Equal(t, []types.GenesisStorage(nil), account.Storage) + require.Equal(t, types.Storage(nil), account.Storage) } func TestEth_ExportAccount_WithStorage(t *testing.T) { @@ -812,7 +812,7 @@ func TestEth_ExportAccount_WithStorage(t *testing.T) { require.Equal(t, addr, strings.ToLower(account.Address.Hex())) require.Equal(t, big.NewInt(0), account.Balance) require.Equal(t, hexutil.Bytes(bytecode), account.Code) - require.NotEqual(t, []types.GenesisStorage(nil), account.Storage) + require.NotEqual(t, types.Storage(nil), account.Storage) } func TestEth_GetBlockByNumber(t *testing.T) { diff --git a/types/code.go b/types/code.go index 17049de4..fa1da75d 100644 --- a/types/code.go +++ b/types/code.go @@ -1,40 +1,12 @@ package types -import ( - "fmt" - - ethcmn "github.com/ethereum/go-ethereum/common" -) - // ---------------------------------------------------------------------------- -// Code & Storage +// Code // ---------------------------------------------------------------------------- -type ( - // Code is account Code type alias - Code []byte - // Storage is account storage type alias - Storage map[ethcmn.Hash]ethcmn.Hash -) +// Code is account Code type alias +type Code []byte func (c Code) String() string { return string(c) } - -func (c Storage) String() (str string) { - for key, value := range c { - str += fmt.Sprintf("%X : %X\n", key, value) - } - - return -} - -// Copy returns a copy of storage. -func (c Storage) Copy() Storage { - cpy := make(Storage) - for key, value := range c { - cpy[key] = value - } - - return cpy -} diff --git a/x/evm/genesis.go b/x/evm/genesis.go index a19f2e7d..58e41d2e 100644 --- a/x/evm/genesis.go +++ b/x/evm/genesis.go @@ -59,9 +59,9 @@ func ExportGenesis(ctx sdk.Context, k Keeper, ak types.AccountKeeper) GenesisSta addr := common.BytesToAddress(ethAccount.GetAddress().Bytes()) - var storage []types.GenesisStorage + var storage types.Storage err = k.CommitStateDB.ForEachStorage(addr, func(key, value common.Hash) bool { - storage = append(storage, types.NewGenesisStorage(key, value)) + storage = append(storage, types.NewState(key, value)) return false }) if err != nil { diff --git a/x/evm/genesis_test.go b/x/evm/genesis_test.go index 9992bb34..0bc5b094 100644 --- a/x/evm/genesis_test.go +++ b/x/evm/genesis_test.go @@ -44,10 +44,10 @@ func (suite *EvmTestSuite) TestContractExportImport() { suite.T().Logf("contract addr 0x%x", address) // clear keeper code and re-initialize - suite.app.EvmKeeper.CommitStateDB.WithContext(suite.ctx).SetCode(address, nil) + suite.app.EvmKeeper.SetCode(suite.ctx, address, nil) _ = evm.InitGenesis(suite.ctx, suite.app.EvmKeeper, genState) - resCode := suite.app.EvmKeeper.CommitStateDB.WithContext(suite.ctx).GetCode(address) + resCode := suite.app.EvmKeeper.GetCode(suite.ctx, address) suite.Require().Equal(deployedEnsFactoryCode, resCode) } diff --git a/x/evm/keeper/querier.go b/x/evm/keeper/querier.go index 066a0089..94a1aafa 100644 --- a/x/evm/keeper/querier.go +++ b/x/evm/keeper/querier.go @@ -201,9 +201,9 @@ func queryAccount(ctx sdk.Context, path []string, keeper Keeper) ([]byte, error) func queryExportAccount(ctx sdk.Context, path []string, keeper Keeper) ([]byte, error) { addr := ethcmn.HexToAddress(path[1]) - var storage []types.GenesisStorage + var storage types.Storage err := keeper.CommitStateDB.ForEachStorage(addr, func(key, value ethcmn.Hash) bool { - storage = append(storage, types.NewGenesisStorage(key, value)) + storage = append(storage, types.NewState(key, value)) return false }) if err != nil { diff --git a/x/evm/types/errors.go b/x/evm/types/errors.go new file mode 100644 index 00000000..aff740dd --- /dev/null +++ b/x/evm/types/errors.go @@ -0,0 +1,12 @@ +package types + +import ( + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// NOTE: We can't use 1 since that error code is reserved for internal errors. + +var ( + // ErrInvalidState returns an error resulting from an invalid Storage State. + ErrInvalidState = sdkerrors.Register(ModuleName, 2, "invalid storage state") +) diff --git a/x/evm/types/genesis.go b/x/evm/types/genesis.go index 8b24ec89..9b808a34 100644 --- a/x/evm/types/genesis.go +++ b/x/evm/types/genesis.go @@ -17,21 +17,14 @@ type ( TxsLogs []TransactionLogs `json:"txs_logs"` } - // GenesisStorage represents the GenesisAccount Storage map as single key value - // pairs. This is to prevent non determinism at genesis initialization or export. - GenesisStorage struct { - Key ethcmn.Hash `json:"key"` - Value ethcmn.Hash `json:"value"` - } - // GenesisAccount defines an account to be initialized in the genesis state. // Its main difference between with Geth's GenesisAccount is that it uses a custom // storage type and that it doesn't contain the private key field. GenesisAccount struct { - Address ethcmn.Address `json:"address"` - Balance *big.Int `json:"balance"` - Code hexutil.Bytes `json:"code,omitempty"` - Storage []GenesisStorage `json:"storage,omitempty"` + Address ethcmn.Address `json:"address"` + Balance *big.Int `json:"balance"` + Code hexutil.Bytes `json:"code,omitempty"` + Storage Storage `json:"storage,omitempty"` } ) @@ -50,26 +43,7 @@ func (ga GenesisAccount) Validate() error { return errors.New("code bytes cannot be empty") } - seenStorage := make(map[string]bool) - for i, state := range ga.Storage { - if seenStorage[state.Key.String()] { - return fmt.Errorf("duplicate state key %d", i) - } - if bytes.Equal(state.Key.Bytes(), ethcmn.Hash{}.Bytes()) { - return fmt.Errorf("state %d key hash cannot be empty", i) - } - // NOTE: state value can be empty - seenStorage[state.Key.String()] = true - } - return nil -} - -// NewGenesisStorage creates a new GenesisStorage instance -func NewGenesisStorage(key, value ethcmn.Hash) GenesisStorage { - return GenesisStorage{ - Key: key, - Value: value, - } + return ga.Storage.Validate() } // DefaultGenesisState sets default evm genesis state with empty accounts. diff --git a/x/evm/types/genesis_test.go b/x/evm/types/genesis_test.go index e9fd62d8..72c7b010 100644 --- a/x/evm/types/genesis_test.go +++ b/x/evm/types/genesis_test.go @@ -24,8 +24,8 @@ func TestValidateGenesisAccount(t *testing.T) { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ - NewGenesisStorage(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), + Storage: Storage{ + NewState(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), }, }, true, @@ -63,31 +63,6 @@ func TestValidateGenesisAccount(t *testing.T) { }, false, }, - { - "empty storage key bytes", - GenesisAccount{ - Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), - Balance: big.NewInt(1), - Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ - {Key: ethcmn.Hash{}}, - }, - }, - false, - }, - { - "duplicated storage key", - GenesisAccount{ - Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), - Balance: big.NewInt(1), - Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ - {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, - {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, - }, - }, - false, - }, } for _, tc := range testCases { @@ -124,7 +99,7 @@ func TestValidateGenesis(t *testing.T) { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ + Storage: Storage{ {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, }, }, @@ -169,16 +144,16 @@ func TestValidateGenesis(t *testing.T) { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ - NewGenesisStorage(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), + Storage: Storage{ + NewState(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), }, }, { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ - NewGenesisStorage(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), + Storage: Storage{ + NewState(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), }, }, }, @@ -193,7 +168,7 @@ func TestValidateGenesis(t *testing.T) { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ + Storage: Storage{ {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, }, }, @@ -243,7 +218,7 @@ func TestValidateGenesis(t *testing.T) { Address: ethcmn.BytesToAddress([]byte{1, 2, 3, 4, 5}), Balance: big.NewInt(1), Code: []byte{1, 2, 3}, - Storage: []GenesisStorage{ + Storage: Storage{ {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, }, }, diff --git a/x/evm/types/journal.go b/x/evm/types/journal.go index 366574d8..825c5d4b 100644 --- a/x/evm/types/journal.go +++ b/x/evm/types/journal.go @@ -22,14 +22,24 @@ type journalEntry interface { // commit. These are tracked to be able to be reverted in case of an execution // exception or revertal request. type journal struct { - entries []journalEntry // Current changes tracked by the journal - dirties map[ethcmn.Address]int // Dirty accounts and the number of changes + entries []journalEntry // Current changes tracked by the journal + dirties []dirty // Dirty accounts and the number of changes + addressToJournalIndex map[ethcmn.Address]int // map from address to the index of the dirties slice +} + +// dirty represents a single key value pair of the journal dirties, where the +// key correspons to the account address and the value to the number of +// changes for that account. +type dirty struct { + address ethcmn.Address + changes int } // newJournal create a new initialized journal. func newJournal() *journal { return &journal{ - dirties: make(map[ethcmn.Address]int), + dirties: []dirty{}, + addressToJournalIndex: make(map[ethcmn.Address]int), } } @@ -37,7 +47,7 @@ func newJournal() *journal { func (j *journal) append(entry journalEntry) { j.entries = append(j.entries, entry) if addr := entry.dirtied(); addr != nil { - j.dirties[*addr]++ + j.addDirty(*addr) } } @@ -50,8 +60,9 @@ func (j *journal) revert(statedb *CommitStateDB, snapshot int) { // Drop any dirty tracking induced by the change if addr := j.entries[i].dirtied(); addr != nil { - if j.dirties[*addr]--; j.dirties[*addr] == 0 { - delete(j.dirties, *addr) + j.substractDirty(*addr) + if j.getDirty(*addr) == 0 { + j.deleteDirty(*addr) } } } @@ -62,7 +73,7 @@ func (j *journal) revert(statedb *CommitStateDB, snapshot int) { // otherwise suggest it as clean. This method is an ugly hack to handle the RIPEMD // precompile consensus exception. func (j *journal) dirty(addr ethcmn.Address) { - j.dirties[addr]++ + j.addDirty(addr) } // length returns the current number of entries in the journal. @@ -70,6 +81,56 @@ func (j *journal) length() int { return len(j.entries) } +// getDirty returns the dirty count for a given address. If the address is not +// found it returns 0. +func (j *journal) getDirty(addr ethcmn.Address) int { + idx, found := j.addressToJournalIndex[addr] + if !found { + return 0 + } + + return j.dirties[idx].changes +} + +// addDirty adds 1 to the dirty count of an address. If the dirty entry is not +// found it creates it. +func (j *journal) addDirty(addr ethcmn.Address) { + idx, found := j.addressToJournalIndex[addr] + if !found { + j.dirties = append(j.dirties, dirty{address: addr, changes: 0}) + idx = len(j.dirties) - 1 + j.addressToJournalIndex[addr] = idx + } + + j.dirties[idx].changes++ +} + +// substractDirty subtracts 1 to the dirty count of an address. It performs a +// no-op if the address is not found. +func (j *journal) substractDirty(addr ethcmn.Address) { + idx, found := j.addressToJournalIndex[addr] + if !found { + return + } + + if j.dirties[idx].changes == 0 { + return + } + j.dirties[idx].changes-- +} + +// deleteDirty deletes a dirty entry from the jounal's dirties slice. If the +// entry is not found it performs a no-op. +func (j *journal) deleteDirty(addr ethcmn.Address) { + idx, found := j.addressToJournalIndex[addr] + if !found { + return + } + + j.dirties = append(j.dirties[:idx], j.dirties[idx+1:]...) + delete(j.addressToJournalIndex, addr) +} + type ( // Changes to the account trie. createObjectChange struct { @@ -128,8 +189,14 @@ type ( ) func (ch createObjectChange) revert(s *CommitStateDB) { - delete(s.stateObjects, *ch.account) delete(s.stateObjectsDirty, *ch.account) + idx, exists := s.addressToObjectIndex[*ch.account] + if !exists { + // perform no-op + return + } + // remove from the slice + s.stateObjects = append(s.stateObjects[:idx], s.stateObjects[idx+1:]...) } func (ch createObjectChange) dirtied() *ethcmn.Address { diff --git a/x/evm/types/journal_test.go b/x/evm/types/journal_test.go index 8b71ed4a..6c1673b9 100644 --- a/x/evm/types/journal_test.go +++ b/x/evm/types/journal_test.go @@ -228,7 +228,8 @@ func (suite *JournalTestSuite) TestJournal_append_revert() { suite.Require().Equal(suite.journal.length(), i+1, tc.name) if tc.entry.dirtied() != nil { dirtyCount++ - suite.Require().Equal(dirtyCount, suite.journal.dirties[suite.address], tc.name) + + suite.Require().Equal(dirtyCount, suite.journal.getDirty(suite.address), tc.name) } } @@ -236,18 +237,18 @@ func (suite *JournalTestSuite) TestJournal_append_revert() { suite.journal.revert(suite.stateDB, 0) // verify the dirty entry has been deleted - count, ok := suite.journal.dirties[suite.address] + idx, ok := suite.journal.addressToJournalIndex[suite.address] suite.Require().False(ok) - suite.Require().Zero(count) + suite.Require().Zero(idx) } func (suite *JournalTestSuite) TestJournal_dirty() { // dirty entry hasn't been set - count, ok := suite.journal.dirties[suite.address] + idx, ok := suite.journal.addressToJournalIndex[suite.address] suite.Require().False(ok) - suite.Require().Zero(count) + suite.Require().Zero(idx) // update dirty count suite.journal.dirty(suite.address) - suite.Require().Equal(1, suite.journal.dirties[suite.address]) + suite.Require().Equal(1, suite.journal.getDirty(suite.address)) } diff --git a/x/evm/types/state_object.go b/x/evm/types/state_object.go index dbe15579..9bf50e9f 100644 --- a/x/evm/types/state_object.go +++ b/x/evm/types/state_object.go @@ -24,57 +24,63 @@ var ( emptyCodeHash = ethcrypto.Keccak256(nil) ) -type ( - // StateObject interface for interacting with state object - StateObject interface { - GetCommittedState(db ethstate.Database, key ethcmn.Hash) ethcmn.Hash - GetState(db ethstate.Database, key ethcmn.Hash) ethcmn.Hash - SetState(db ethstate.Database, key, value ethcmn.Hash) +// StateObject interface for interacting with state object +type StateObject interface { + GetCommittedState(db ethstate.Database, key ethcmn.Hash) ethcmn.Hash + GetState(db ethstate.Database, key ethcmn.Hash) ethcmn.Hash + SetState(db ethstate.Database, key, value ethcmn.Hash) - Code(db ethstate.Database) []byte - SetCode(codeHash ethcmn.Hash, code []byte) - CodeHash() []byte + Code(db ethstate.Database) []byte + SetCode(codeHash ethcmn.Hash, code []byte) + CodeHash() []byte - AddBalance(amount *big.Int) - SubBalance(amount *big.Int) - SetBalance(amount *big.Int) + AddBalance(amount *big.Int) + SubBalance(amount *big.Int) + SetBalance(amount *big.Int) - Balance() *big.Int - ReturnGas(gas *big.Int) - Address() ethcmn.Address + Balance() *big.Int + ReturnGas(gas *big.Int) + Address() ethcmn.Address - SetNonce(nonce uint64) - Nonce() uint64 - } - // stateObject represents an Ethereum account which is being modified. + SetNonce(nonce uint64) + Nonce() uint64 +} + +// stateObject represents an Ethereum account which is being modified. +// +// The usage pattern is as follows: +// First you need to obtain a state object. +// Account values can be accessed and modified through the object. +// Finally, call CommitTrie to write the modified storage trie into a database. +type stateObject struct { + code types.Code // contract bytecode, which gets set when code is loaded + + // State objects are used by the consensus core and VM which are + // unable to deal with database-level errors. Any error that occurs + // during a database read is memoized here and will eventually be returned + // by StateDB.Commit. + originStorage Storage // Storage cache of original entries to dedup rewrites + dirtyStorage Storage // Storage entries that need to be flushed to disk + + // DB error + dbErr error + stateDB *CommitStateDB + account *types.EthAccount + balance sdk.Int + + keyToOriginStorageIndex map[ethcmn.Hash]int + keyToDirtyStorageIndex map[ethcmn.Hash]int + + address ethcmn.Address + + // cache flags // - // The usage pattern is as follows: - // First you need to obtain a state object. - // Account values can be accessed and modified through the object. - // Finally, call CommitTrie to write the modified storage trie into a database. - stateObject struct { - code types.Code // contract bytecode, which gets set when code is loaded - // DB error. - // State objects are used by the consensus core and VM which are - // unable to deal with database-level errors. Any error that occurs - // during a database read is memoized here and will eventually be returned - // by StateDB.Commit. - dbErr error - stateDB *CommitStateDB - account *types.EthAccount - balance sdk.Int - originStorage types.Storage // Storage cache of original entries to dedup rewrites - dirtyStorage types.Storage // Storage entries that need to be flushed to disk - address ethcmn.Address - // cache flags - // - // When an object is marked suicided it will be delete from the trie during - // the "update" phase of the state transition. - dirtyCode bool // true if the code was updated - suicided bool - deleted bool - } -) + // When an object is marked suicided it will be delete from the trie during + // the "update" phase of the state transition. + dirtyCode bool // true if the code was updated + suicided bool + deleted bool +} func newStateObject(db *CommitStateDB, accProto authexported.Account, balance sdk.Int) *stateObject { ethermintAccount, ok := accProto.(*types.EthAccount) @@ -88,12 +94,14 @@ func newStateObject(db *CommitStateDB, accProto authexported.Account, balance sd } return &stateObject{ - stateDB: db, - account: ethermintAccount, - balance: balance, - address: ethcmn.BytesToAddress(ethermintAccount.GetAddress().Bytes()), - originStorage: make(types.Storage), - dirtyStorage: make(types.Storage), + stateDB: db, + account: ethermintAccount, + balance: balance, + address: ethcmn.BytesToAddress(ethermintAccount.GetAddress().Bytes()), + originStorage: Storage{}, + dirtyStorage: Storage{}, + keyToOriginStorageIndex: make(map[ethcmn.Hash]int), + keyToDirtyStorageIndex: make(map[ethcmn.Hash]int), } } @@ -122,8 +130,18 @@ func (so *stateObject) SetState(db ethstate.Database, key, value ethcmn.Hash) { so.setState(prefixKey, value) } +// setState sets a state with a prefixed key and value to the dirty storage. func (so *stateObject) setState(key, value ethcmn.Hash) { - so.dirtyStorage[key] = value + idx, ok := so.keyToDirtyStorageIndex[key] + if ok { + so.dirtyStorage[idx].Value = value + return + } + + // create new entry + so.dirtyStorage = append(so.dirtyStorage, NewState(key, value)) + idx = len(so.dirtyStorage) - 1 + so.keyToDirtyStorageIndex[key] = idx } // SetCode sets the state object's code. @@ -224,23 +242,31 @@ func (so *stateObject) commitState() { ctx := so.stateDB.ctx store := prefix.NewStore(ctx.KVStore(so.stateDB.storeKey), AddressStoragePrefix(so.Address())) - for key, value := range so.dirtyStorage { - delete(so.dirtyStorage, key) + for i, state := range so.dirtyStorage { + delete(so.keyToDirtyStorageIndex, state.Key) + so.dirtyStorage = append(so.dirtyStorage[:i], so.dirtyStorage[i+1:]...) // skip no-op changes, persist actual changes - if value == so.originStorage[key] { + idx, ok := so.keyToOriginStorageIndex[state.Key] + if !ok { continue } - so.originStorage[key] = value - - // delete empty values - if (value == ethcmn.Hash{}) { - store.Delete(key.Bytes()) + if state.Value == so.originStorage[idx].Value { continue } - store.Set(key.Bytes(), value.Bytes()) + so.originStorage[idx].Value = state.Value + + // NOTE: key is already prefixed from GetStorageByAddressKey + + // delete empty values from the store + if (state.Value == ethcmn.Hash{}) { + store.Delete(state.Key.Bytes()) + continue + } + + store.Set(state.Key.Bytes(), state.Value.Bytes()) } } @@ -312,9 +338,9 @@ func (so *stateObject) GetState(db ethstate.Database, key ethcmn.Hash) ethcmn.Ha prefixKey := so.GetStorageByAddressKey(key.Bytes()) // if we have a dirty value for this state entry, return it - value, dirty := so.dirtyStorage[prefixKey] + idx, dirty := so.keyToDirtyStorageIndex[prefixKey] if dirty { - return value + return so.dirtyStorage[idx].Value } // otherwise return the entry's original value @@ -322,27 +348,35 @@ func (so *stateObject) GetState(db ethstate.Database, key ethcmn.Hash) ethcmn.Ha } // GetCommittedState retrieves a value from the committed account storage trie. -// Note, the key will be prefixed with the address of the state object. +// +// NOTE: the key will be prefixed with the address of the state object. func (so *stateObject) GetCommittedState(_ ethstate.Database, key ethcmn.Hash) ethcmn.Hash { prefixKey := so.GetStorageByAddressKey(key.Bytes()) // if we have the original value cached, return that - value, cached := so.originStorage[prefixKey] + idx, cached := so.keyToOriginStorageIndex[prefixKey] if cached { - return value + return so.originStorage[idx].Value } + if len(so.originStorage) == 0 { + so.originStorage = append(so.originStorage, NewState(prefixKey, ethcmn.Hash{})) + so.keyToOriginStorageIndex[prefixKey] = len(so.originStorage) - 1 + } + + state := so.originStorage[idx] + // otherwise load the value from the KVStore ctx := so.stateDB.ctx store := prefix.NewStore(ctx.KVStore(so.stateDB.storeKey), AddressStoragePrefix(so.Address())) rawValue := store.Get(prefixKey.Bytes()) if len(rawValue) > 0 { - value.SetBytes(rawValue) + state.Value.SetBytes(rawValue) } - so.originStorage[prefixKey] = value - return value + so.originStorage[idx] = state + return state.Value } // ---------------------------------------------------------------------------- @@ -403,3 +437,11 @@ func (so stateObject) GetStorageByAddressKey(key []byte) ethcmn.Hash { return ethcrypto.Keccak256Hash(compositeKey) } + +// stateEntry represents a single key value pair from the StateDB's stateObject mappindg. +// This is to prevent non determinism at genesis initialization or export. +type stateEntry struct { + // address key of the state object + address ethcmn.Address + stateObject *stateObject +} diff --git a/x/evm/types/state_object_test.go b/x/evm/types/state_object_test.go index da2ec013..f07a1e24 100644 --- a/x/evm/types/state_object_test.go +++ b/x/evm/types/state_object_test.go @@ -35,6 +35,14 @@ func (suite *StateDBTestSuite) TestStateObject_State() { suite.stateObject.SetState(nil, ethcmn.BytesToHash([]byte("key1")), ethcmn.BytesToHash([]byte("value1"))) }, }, + { + "update value", + ethcmn.BytesToHash([]byte("key1")), + ethcmn.BytesToHash([]byte("value2")), + func() { + suite.stateObject.SetState(nil, ethcmn.BytesToHash([]byte("key1")), ethcmn.BytesToHash([]byte("value2"))) + }, + }, } for _, tc := range testCase { diff --git a/x/evm/types/statedb.go b/x/evm/types/statedb.go index 4f3f745e..446fbb2d 100644 --- a/x/evm/types/statedb.go +++ b/x/evm/types/statedb.go @@ -16,6 +16,7 @@ import ( ethtypes "github.com/ethereum/go-ethereum/core/types" ethvm "github.com/ethereum/go-ethereum/core/vm" ethcrypto "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/rlp" ) var ( @@ -45,10 +46,11 @@ type CommitStateDB struct { accountKeeper AccountKeeper bankKeeper BankKeeper - // maps that hold 'live' objects, which will get modified while processing a + // array that hold 'live' objects, which will get modified while processing a // state transition - stateObjects map[ethcmn.Address]*stateObject - stateObjectsDirty map[ethcmn.Address]struct{} + stateObjects []stateEntry + addressToObjectIndex map[ethcmn.Address]int // map from address to the index of the state objects slice + stateObjectsDirty map[ethcmn.Address]struct{} // The refund counter, also used by state transitioning. refund uint64 @@ -59,6 +61,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 // DB error. @@ -87,14 +91,15 @@ func NewCommitStateDB( ctx sdk.Context, storeKey sdk.StoreKey, ak AccountKeeper, bk BankKeeper, ) *CommitStateDB { return &CommitStateDB{ - ctx: ctx, - storeKey: storeKey, - accountKeeper: ak, - bankKeeper: bk, - stateObjects: make(map[ethcmn.Address]*stateObject), - stateObjectsDirty: make(map[ethcmn.Address]struct{}), - preimages: make(map[ethcmn.Hash][]byte), - journal: newJournal(), + ctx: ctx, + storeKey: storeKey, + accountKeeper: ak, + bankKeeper: bk, + stateObjects: []stateEntry{}, + addressToObjectIndex: make(map[ethcmn.Address]int), + stateObjectsDirty: make(map[ethcmn.Address]struct{}), + preimages: make(map[ethcmn.Hash][]byte), + journal: newJournal(), } } @@ -389,34 +394,34 @@ func (csdb *CommitStateDB) Commit(deleteEmptyObjects bool) (ethcmn.Hash, error) defer csdb.clearJournalAndRefund() // remove dirty state object entries based on the journal - for addr := range csdb.journal.dirties { - csdb.stateObjectsDirty[addr] = struct{}{} + for _, dirty := range csdb.journal.dirties { + csdb.stateObjectsDirty[dirty.address] = struct{}{} } // set the state objects - for addr, so := range csdb.stateObjects { - _, isDirty := csdb.stateObjectsDirty[addr] + for _, stateEntry := range csdb.stateObjects { + _, isDirty := csdb.stateObjectsDirty[stateEntry.address] switch { - case so.suicided || (isDirty && deleteEmptyObjects && so.empty()): + case stateEntry.stateObject.suicided || (isDirty && deleteEmptyObjects && stateEntry.stateObject.empty()): // If the state object has been removed, don't bother syncing it and just // remove it from the store. - csdb.deleteStateObject(so) + csdb.deleteStateObject(stateEntry.stateObject) case isDirty: // write any contract code associated with the state object - if so.code != nil && so.dirtyCode { - so.commitCode() - so.dirtyCode = false + if stateEntry.stateObject.code != nil && stateEntry.stateObject.dirtyCode { + stateEntry.stateObject.commitCode() + stateEntry.stateObject.dirtyCode = false } // update the object in the KVStore - if err := csdb.updateStateObject(so); err != nil { + if err := csdb.updateStateObject(stateEntry.stateObject); err != nil { return ethcmn.Hash{}, err } } - delete(csdb.stateObjectsDirty, addr) + delete(csdb.stateObjectsDirty, stateEntry.address) } // NOTE: Ethereum returns the trie merkle root here, but as commitment @@ -429,8 +434,8 @@ func (csdb *CommitStateDB) Commit(deleteEmptyObjects bool) (ethcmn.Hash, error) // removing the csdb destructed objects and clearing the journal as well as the // refunds. func (csdb *CommitStateDB) Finalise(deleteEmptyObjects bool) error { - for addr := range csdb.journal.dirties { - so, exist := csdb.stateObjects[addr] + for _, dirty := range csdb.journal.dirties { + idx, exist := csdb.addressToObjectIndex[dirty.address] if !exist { // ripeMD is 'touched' at block 1714175, in tx: // 0x1237f737031e40bcde4a8b7e717b2d15e3ecadfe49bb1bbc71ee9deb09c6fcf2 @@ -444,18 +449,19 @@ func (csdb *CommitStateDB) Finalise(deleteEmptyObjects bool) error { continue } - if so.suicided || (deleteEmptyObjects && so.empty()) { - csdb.deleteStateObject(so) + stateEntry := csdb.stateObjects[idx] + if stateEntry.stateObject.suicided || (deleteEmptyObjects && stateEntry.stateObject.empty()) { + csdb.deleteStateObject(stateEntry.stateObject) } else { // Set all the dirty state storage items for the state object in the // KVStore and finally set the account in the account mapper. - so.commitState() - if err := csdb.updateStateObject(so); err != nil { + stateEntry.stateObject.commitState() + if err := csdb.updateStateObject(stateEntry.stateObject); err != nil { return err } } - csdb.stateObjectsDirty[addr] = struct{}{} + csdb.stateObjectsDirty[dirty.address] = struct{}{} } // invalidate journal because reverting across transactions is not allowed @@ -587,7 +593,8 @@ func (csdb *CommitStateDB) Suicide(addr ethcmn.Address) bool { // the underlying account mapper and store keys to avoid reloading data for the // next operations. func (csdb *CommitStateDB) Reset(_ ethcmn.Hash) error { - csdb.stateObjects = make(map[ethcmn.Address]*stateObject) + csdb.stateObjects = []stateEntry{} + csdb.addressToObjectIndex = make(map[ethcmn.Address]int) csdb.stateObjectsDirty = make(map[ethcmn.Address]struct{}) csdb.thash = ethcmn.Hash{} csdb.bhash = ethcmn.Hash{} @@ -601,27 +608,28 @@ func (csdb *CommitStateDB) Reset(_ ethcmn.Hash) error { // UpdateAccounts updates the nonce and coin balances of accounts func (csdb *CommitStateDB) UpdateAccounts() { - for addr, so := range csdb.stateObjects { - currAcc := csdb.accountKeeper.GetAccount(csdb.ctx, sdk.AccAddress(addr.Bytes())) + for _, stateEntry := range csdb.stateObjects { + currAcc := csdb.accountKeeper.GetAccount(csdb.ctx, sdk.AccAddress(stateEntry.address.Bytes())) emintAcc, ok := currAcc.(*emint.EthAccount) if !ok { continue } balance := csdb.bankKeeper.GetBalance(csdb.ctx, emintAcc.GetAddress(), emint.DenomDefault) - if so.Balance() != balance.Amount.BigInt() && balance.IsValid() { - so.balance = balance.Amount + if stateEntry.stateObject.Balance() != balance.Amount.BigInt() && balance.IsValid() { + stateEntry.stateObject.balance = balance.Amount } - if so.Nonce() != emintAcc.GetSequence() { - so.account = emintAcc + if stateEntry.stateObject.Nonce() != emintAcc.GetSequence() { + stateEntry.stateObject.account = emintAcc } } } // ClearStateObjects clears cache of state objects to handle account changes outside of the EVM func (csdb *CommitStateDB) ClearStateObjects() { - csdb.stateObjects = make(map[ethcmn.Address]*stateObject) + csdb.stateObjects = []stateEntry{} + csdb.addressToObjectIndex = make(map[ethcmn.Address]int) csdb.stateObjectsDirty = make(map[ethcmn.Address]struct{}) } @@ -665,28 +673,32 @@ func (csdb *CommitStateDB) Copy() *CommitStateDB { // copy all the basic fields, initialize the memory ones state := &CommitStateDB{ - ctx: csdb.ctx, - storeKey: csdb.storeKey, - accountKeeper: csdb.accountKeeper, - bankKeeper: csdb.bankKeeper, - stateObjects: make(map[ethcmn.Address]*stateObject, len(csdb.journal.dirties)), - stateObjectsDirty: make(map[ethcmn.Address]struct{}, len(csdb.journal.dirties)), - refund: csdb.refund, - logSize: csdb.logSize, - preimages: make(map[ethcmn.Hash][]byte), - journal: newJournal(), + ctx: csdb.ctx, + storeKey: csdb.storeKey, + accountKeeper: csdb.accountKeeper, + bankKeeper: csdb.bankKeeper, + 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)), + refund: csdb.refund, + logSize: csdb.logSize, + preimages: make(map[ethcmn.Hash][]byte), + journal: newJournal(), } // copy the dirty states, logs, and preimages - for addr := 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 object, exist := csdb.stateObjects[addr]; exist { - state.stateObjects[addr] = object.deepCopy(state) - state.stateObjectsDirty[addr] = struct{}{} + if idx, exist := csdb.addressToObjectIndex[dirty.address]; exist { + state.stateObjects[idx] = stateEntry{ + address: dirty.address, + stateObject: csdb.stateObjects[idx].stateObject.deepCopy(state), + } + state.stateObjectsDirty[dirty.address] = struct{}{} } } @@ -694,8 +706,8 @@ func (csdb *CommitStateDB) Copy() *CommitStateDB { // copied, the loop above will be a no-op, since the copy's journal is empty. // Thus, here we iterate over stateObjects, to enable copies of copies. for addr := range csdb.stateObjectsDirty { - if _, exist := state.stateObjects[addr]; !exist { - state.stateObjects[addr] = csdb.stateObjects[addr].deepCopy(state) + if idx, exist := state.addressToObjectIndex[addr]; !exist { + state.setStateObject(csdb.stateObjects[idx].stateObject.deepCopy(state)) state.stateObjectsDirty[addr] = struct{}{} } } @@ -708,9 +720,9 @@ func (csdb *CommitStateDB) Copy() *CommitStateDB { return state } -// ForEachStorage iterates over each storage items, all invokes the provided -// callback on each key, value pair . -func (csdb *CommitStateDB) ForEachStorage(addr ethcmn.Address, cb func(key, value ethcmn.Hash) bool) error { +// ForEachStorage iterates over each storage items, all invoke the provided +// callback on each key, value pair. +func (csdb *CommitStateDB) ForEachStorage(addr ethcmn.Address, cb func(key, value ethcmn.Hash) (stop bool)) error { so := csdb.getStateObject(addr) if so == nil { return nil @@ -725,18 +737,23 @@ func (csdb *CommitStateDB) ForEachStorage(addr ethcmn.Address, cb func(key, valu key := ethcmn.BytesToHash(iterator.Key()) value := ethcmn.BytesToHash(iterator.Value()) - if value, dirty := so.dirtyStorage[key]; dirty { + if idx, dirty := so.keyToDirtyStorageIndex[key]; dirty { // check if iteration stops - if cb(key, value) { + if cb(key, so.dirtyStorage[idx].Value) { break } continue } + _, content, _, err := rlp.Split(value.Bytes()) + if err != nil { + return err + } + // check if iteration stops - if cb(key, value) { - break + if cb(key, ethcmn.BytesToHash(content)) { + return nil } } @@ -784,13 +801,16 @@ func (csdb *CommitStateDB) setError(err error) { // getStateObject attempts to retrieve a state object given by the address. // Returns nil and sets an error if not found. func (csdb *CommitStateDB) getStateObject(addr ethcmn.Address) (stateObject *stateObject) { - // prefer 'live' (cached) objects - if so := csdb.stateObjects[addr]; so != nil { - if so.deleted { - return nil - } + idx, found := csdb.addressToObjectIndex[addr] + if found { + // prefer 'live' (cached) objects + if so := csdb.stateObjects[idx].stateObject; so != nil { + if so.deleted { + return nil + } - return so + return so + } } // otherwise, attempt to fetch the account from the account mapper @@ -810,7 +830,21 @@ func (csdb *CommitStateDB) getStateObject(addr ethcmn.Address) (stateObject *sta } func (csdb *CommitStateDB) setStateObject(so *stateObject) { - csdb.stateObjects[so.Address()] = so + idx, found := csdb.addressToObjectIndex[so.Address()] + if found { + // update the existing object + csdb.stateObjects[idx].stateObject = so + return + } + + // append the new state object to the stateObjects slice + se := stateEntry{ + address: so.Address(), + stateObject: so, + } + + csdb.stateObjects = append(csdb.stateObjects, se) + csdb.addressToObjectIndex[se.address] = len(csdb.stateObjects) - 1 } // RawDump returns a raw state dump. diff --git a/x/evm/types/statedb_test.go b/x/evm/types/statedb_test.go index 2957b560..f8a8c437 100644 --- a/x/evm/types/statedb_test.go +++ b/x/evm/types/statedb_test.go @@ -404,6 +404,13 @@ func (suite *StateDBTestSuite) TestCommitStateDB_Finalize() { }, false, true, }, + { + "finalize, dirty storage", + func() { + suite.stateDB.SetState(suite.address, ethcmn.BytesToHash([]byte("key")), ethcmn.BytesToHash([]byte("value"))) + }, + false, true, + }, { "faled to update state object", func() { diff --git a/x/evm/types/storage.go b/x/evm/types/storage.go new file mode 100644 index 00000000..cf7afaf2 --- /dev/null +++ b/x/evm/types/storage.go @@ -0,0 +1,72 @@ +package types + +import ( + "bytes" + "fmt" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + + ethcmn "github.com/ethereum/go-ethereum/common" +) + +// Storage represents the account Storage map as a slice of single key value +// State pairs. This is to prevent non determinism at genesis initialization or export. +type Storage []State + +// Validate performs a basic validation of the Storage fields. +func (s Storage) Validate() error { + seenStorage := make(map[string]bool) + for i, state := range s { + if seenStorage[state.Key.String()] { + return sdkerrors.Wrapf(ErrInvalidState, "duplicate state key %d", i) + } + + if err := state.Validate(); err != nil { + return err + } + + seenStorage[state.Key.String()] = true + } + return nil +} + +// String implements the stringer interface +func (s Storage) String() string { + var str string + for _, state := range s { + str += fmt.Sprintf("%s: %s\n", state.Key.String(), state.Value.String()) + } + + return str +} + +// Copy returns a copy of storage. +func (s Storage) Copy() Storage { + cpy := make(Storage, len(s)) + copy(cpy, s) + + return cpy +} + +// State represents a single Storage key value pair item. +type State struct { + Key ethcmn.Hash `json:"key"` + Value ethcmn.Hash `json:"value"` +} + +// Validate performs a basic validation of the State fields. +func (s State) Validate() error { + if bytes.Equal(s.Key.Bytes(), ethcmn.Hash{}.Bytes()) { + return sdkerrors.Wrap(ErrInvalidState, "state key hash cannot be empty") + } + // NOTE: state value can be empty + return nil +} + +// NewState creates a new State instance +func NewState(key, value ethcmn.Hash) State { + return State{ + Key: key, + Value: value, + } +} diff --git a/x/evm/types/storage_test.go b/x/evm/types/storage_test.go new file mode 100644 index 00000000..b2300267 --- /dev/null +++ b/x/evm/types/storage_test.go @@ -0,0 +1,84 @@ +package types + +import ( + "testing" + + ethcmn "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" +) + +func TestStorageValidate(t *testing.T) { + testCases := []struct { + name string + storage Storage + expPass bool + }{ + { + "valid storage", + Storage{ + NewState(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), + }, + true, + }, + { + "empty storage key bytes", + Storage{ + {Key: ethcmn.Hash{}}, + }, + false, + }, + { + "duplicated storage key", + Storage{ + {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, + {Key: ethcmn.BytesToHash([]byte{1, 2, 3})}, + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + err := tc.storage.Validate() + if tc.expPass { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + } +} + +func TestStorageCopy(t *testing.T) { + testCases := []struct { + name string + storage Storage + }{ + { + "single storage", + Storage{ + NewState(ethcmn.BytesToHash([]byte{1, 2, 3}), ethcmn.BytesToHash([]byte{1, 2, 3})), + }, + }, + { + "empty storage key value bytes", + Storage{ + {Key: ethcmn.Hash{}, Value: ethcmn.Hash{}}, + }, + }, + { + "empty storage", + Storage{}, + }, + } + + for _, tc := range testCases { + tc := tc + require.Equal(t, tc.storage, tc.storage.Copy(), tc.name) + } +} + +func TestStorageString(t *testing.T) { + storage := Storage{NewState(ethcmn.BytesToHash([]byte("key")), ethcmn.BytesToHash([]byte("value")))} + str := "0x00000000000000000000000000000000000000000000000000000000006b6579: 0x00000000000000000000000000000000000000000000000000000076616c7565\n" + require.Equal(t, str, storage.String()) +}