From 4edd05ec409da76791545a39f2989dc447bff254 Mon Sep 17 00:00:00 2001 From: drklee3 Date: Wed, 28 Feb 2024 11:36:02 -0800 Subject: [PATCH] feat: EIP-161 clear empty touched accounts --- x/evm/keeper/integration_test.go | 168 +++++++++++++++++++++++++++++++ x/evm/statedb/ctx.go | 2 +- x/evm/statedb/statedb.go | 27 +++++ x/evm/statedb/store.go | 77 +++++++++++--- 4 files changed, 259 insertions(+), 15 deletions(-) diff --git a/x/evm/keeper/integration_test.go b/x/evm/keeper/integration_test.go index c11c8b8b70..9422bbd70d 100644 --- a/x/evm/keeper/integration_test.go +++ b/x/evm/keeper/integration_test.go @@ -10,6 +10,7 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/evmos/ethermint/types" + "github.com/evmos/ethermint/x/evm/statedb" "github.com/evmos/ethermint/x/evm/testutil" "github.com/stretchr/testify/suite" ) @@ -102,6 +103,173 @@ func (suite *IntegrationTestSuite) TestEIP161_CreateNonce() { ) } +func (suite *IntegrationTestSuite) TestEIP161_TouchEmptyDeletes() { + // From EIP-161 point D: + // At the end of the transaction, any account touched by the execution of + // that transaction which is now empty SHALL instead become non-existent + // (i.e. deleted). + + // Where: + // An account is considered to be touched when it is involved in any + // potentially state-changing operation. This includes, but is not limited + // to, being the recipient of a transfer of zero value. + + // An account changes state when: + // 1) it is the target or refund of a SUICIDE operation for zero or more + // value; + // 2) it is the source or destination of a CALL operation or message-call + // transaction transferring zero or more value; + // 3) it is the source or creation of a CREATE operation or + // contract-creation transaction endowing zero or more value; + + // Not relevant: + // 4) as the block author (“miner”) it is the recipient of block-rewards or + // transaction-fees of zero or more value. + + // Some cases aren't relevant when clearing state. e.g. as an EOA + // sending a tx, I will never be empty as my nonce increments. + // Account state changes with a >0 value will also *not* clear the account + // as it will be non-empty with a >0 balance. + + var contractAddr common.Address + targetAddr := common.Address{10} + + tests := []struct { + name string + malleate func() + wantContractDeleted bool + wantAccountDeleted bool + }{ + { + "not touched", + func() { + _, rsp, err := suite.CallContract( + testutil.EIP161TestContract, + contractAddr, + common.Big0, + "selfDestructTo", + common.Address{20}, // unrelated account + ) + suite.Require().NoError(err) + suite.Require().Empty(rsp.VmError) + }, + true, + false, + }, + { + "self destruct target - 0 value", + func() { + // beneficiary account with 0 value should be touched + _, rsp, err := suite.CallContract( + testutil.EIP161TestContract, + contractAddr, + common.Big0, + "selfDestructTo", + targetAddr, + ) + suite.Require().NoError(err) + suite.Require().Empty(rsp.VmError) + }, + true, + true, + }, + { + "call target - 0 value", + func() { + // target account with 0 value should be touched + _, rsp, err := suite.CallContract( + testutil.EIP161TestContract, + contractAddr, + common.Big0, + "callAccount", + targetAddr, + ) + suite.Require().NoError(err) + suite.Require().Empty(rsp.VmError) + }, + false, + true, + }, + } + + for _, tt := range tests { + suite.Run(tt.name, func() { + // Setup test + // Deploy contract + contractAddr = suite.DeployContract(testutil.EIP161TestContract) + + // Create empty account + acc := statedb.NewEmptyAccount() + err := suite.App.EvmKeeper.SetAccount(suite.Ctx, targetAddr, *acc) + suite.Require().NoError(err, "empty target should be created") + + targetAcc := suite.App.EvmKeeper.GetAccount(suite.Ctx, targetAddr) + suite.Require().NotNil(targetAcc, "empty account should exist") + suite.Require().True(targetAcc.IsEmpty()) + + // Run test specific setup + tt.malleate() + + // Check result + targetAcc = suite.App.EvmKeeper.GetAccount(suite.Ctx, targetAddr) + if tt.wantAccountDeleted { + suite.Require().Nil( + targetAcc, + "EIP-161: empty account should be deleted after being touched", + ) + } else { + suite.Require().NotNil( + targetAcc, + "EIP-161: empty account should not be deleted if not touched", + ) + } + + contractAcc := suite.App.EvmKeeper.GetAccount(suite.Ctx, contractAddr) + if tt.wantContractDeleted { + suite.Require().Nil( + contractAcc, + "EIP-161: contract should be deleted after touching empty account", + ) + } else { + suite.Require().NotNil( + contractAcc, + "EIP-161: contract should not be deleted if not touching empty account", + ) + } + }) + } +} + +func (suite *IntegrationTestSuite) TestEIP161_CallDeletesEmpty() { + addr := suite.DeployContract(testutil.EIP161TestContract) + + // Create an empty account in state + targetAddr := common.Address{10} + acc := statedb.NewEmptyAccount() + + err := suite.App.EvmKeeper.SetAccount(suite.Ctx, targetAddr, *acc) + suite.Require().NoError(err, "target should be created") + + targetAcc := suite.App.EvmKeeper.GetAccount(suite.Ctx, targetAddr) + suite.Require().True(targetAcc.IsEmpty()) + + _, rsp, err := suite.CallContract( + testutil.EIP161TestContract, + addr, + common.Big0, // no transfer + "callAccount", + targetAddr, + ) + suite.Require().NoError(err) + suite.Require().Empty(rsp.VmError) + + targetAcc = suite.App.EvmKeeper.GetAccount(suite.Ctx, targetAddr) + suite.Require().Nil( + targetAcc, + "EIP-161: empty account should be deleted after being touched", + ) +} + func (suite *IntegrationTestSuite) TestEIP161_CallGas() { addr := suite.DeployContract(testutil.EIP161TestContract) diff --git a/x/evm/statedb/ctx.go b/x/evm/statedb/ctx.go index 3439cf34b9..ea6a2d2514 100644 --- a/x/evm/statedb/ctx.go +++ b/x/evm/statedb/ctx.go @@ -29,7 +29,7 @@ func NewSnapshotCtx(initialCtx sdk.Context) *SnapshotCommitCtx { // Create an initial snapshot of the initialCtx so no state is written until // Commit() is called. The ID is -1 but disregarded along with the // StoreRevertKey indices as this is only to branch the ctx. - _ = sCtx.Snapshot(0, StoreRevertKey{0, 0, 0}) + _ = sCtx.Snapshot(0, initialStoreRevertKey) return sCtx } diff --git a/x/evm/statedb/statedb.go b/x/evm/statedb/statedb.go index e32fc95ec8..6a09236119 100644 --- a/x/evm/statedb/statedb.go +++ b/x/evm/statedb/statedb.go @@ -70,6 +70,11 @@ func (s *StateDB) Keeper() Keeper { return s.keeper } +// TouchAccount marks the account as touched. +func (s *StateDB) TouchAccount(addr common.Address) { + s.ephemeralStore.SetTouched(addr) +} + // AddLog adds a log, called by evm. func (s *StateDB) AddLog(log *ethtypes.Log) { log.TxHash = s.txConfig.TxHash @@ -255,6 +260,8 @@ func (s *StateDB) ForEachStorage(addr common.Address, cb func(key, value common. // AddBalance adds amount to the account associated with addr. func (s *StateDB) AddBalance(addr common.Address, amount *big.Int) { + s.TouchAccount(addr) + // Only allow positive amounts. // TODO: Geth apparently allows negative amounts, but can cause negative // balance which is not allowed in bank keeper @@ -272,6 +279,8 @@ func (s *StateDB) AddBalance(addr common.Address, amount *big.Int) { // SubBalance subtracts amount from the account associated with addr. func (s *StateDB) SubBalance(addr common.Address, amount *big.Int) { + s.TouchAccount(addr) + if amount.Sign() == 0 { return } @@ -286,6 +295,8 @@ func (s *StateDB) SubBalance(addr common.Address, amount *big.Int) { // SetNonce sets the nonce of account. func (s *StateDB) SetNonce(addr common.Address, nonce uint64) { + s.TouchAccount(addr) + account := s.getOrNewAccount(addr) account.Nonce = nonce @@ -309,6 +320,8 @@ func (s *StateDB) SetCode(addr common.Address, code []byte) { // SetState sets the contract state. func (s *StateDB) SetState(addr common.Address, key, value common.Hash) { + s.TouchAccount(addr) + // We cannot attempt to skip noop changes by just checking committed state // Example: // 1. With committed state to 0x0 @@ -444,6 +457,20 @@ func (s *StateDB) Commit() error { } } + // EIP-161: Clear touched empty accounts, after execution of suicide list + touchedAddrs := s.ephemeralStore.GetAllTouched() + for _, addr := range touchedAddrs { + account := s.keeper.GetAccount(s.ctx.CurrentCtx(), addr) + // Ignore non-existent or non-empty accounts + if account == nil || !account.IsEmpty() { + continue + } + + if err := s.keeper.DeleteAccount(s.ctx.CurrentCtx(), addr); err != nil { + return fmt.Errorf("failed to delete empty account: %w", err) + } + } + // Commit after account deletions s.ctx.Commit() diff --git a/x/evm/statedb/store.go b/x/evm/statedb/store.go index 6ec6c08822..4c390b0792 100644 --- a/x/evm/statedb/store.go +++ b/x/evm/statedb/store.go @@ -7,11 +7,20 @@ import ( ethtypes "github.com/ethereum/go-ethereum/core/types" ) +// initialStoreRevertKey is the initial state of the store. +var initialStoreRevertKey = StoreRevertKey{ + RefundIndex: 0, + SuicidedAccountsIndex: 0, + LogsIndex: 0, + TouchedAccountsIndex: 0, +} + // StoreRevertKey defines the required information to revert to a previous state. type StoreRevertKey struct { RefundIndex int SuicidedAccountsIndex int LogsIndex int + TouchedAccountsIndex int } // EphemeralStore provides in-memory state of the refund and suicided accounts @@ -20,6 +29,7 @@ type EphemeralStore struct { RefundStates []uint64 SuicidedAccountStates []common.Address Logs []*ethtypes.Log + TouchedAccounts []common.Address } // NewEphemeralStore creates a new EphemeralStore. @@ -28,6 +38,7 @@ func NewEphemeralStore() *EphemeralStore { RefundStates: nil, SuicidedAccountStates: nil, Logs: nil, + TouchedAccounts: nil, } } @@ -37,6 +48,7 @@ func (es *EphemeralStore) GetRevertKey() StoreRevertKey { RefundIndex: len(es.RefundStates), SuicidedAccountsIndex: len(es.SuicidedAccountStates), LogsIndex: len(es.Logs), + TouchedAccountsIndex: len(es.TouchedAccounts), } } @@ -49,28 +61,36 @@ func (es *EphemeralStore) Revert(key StoreRevertKey) { es.RefundStates = es.RefundStates[:key.RefundIndex] es.SuicidedAccountStates = es.SuicidedAccountStates[:key.SuicidedAccountsIndex] es.Logs = es.Logs[:key.LogsIndex] + es.TouchedAccounts = es.TouchedAccounts[:key.TouchedAccountsIndex] } -func (es *EphemeralStore) ValidateRevertKey(key StoreRevertKey) error { - if key.RefundIndex > len(es.RefundStates) { +func validateIndex(idx int, targetLen int, name string) error { + if idx > targetLen { return fmt.Errorf( - "invalid RefundIndex, %d is greater than the length of the refund states (%d)", - key.RefundIndex, len(es.RefundStates), + "invalid %s index, %d is greater than the length of the %s (%d)", + name, idx, name, targetLen, ) } - if key.SuicidedAccountsIndex > len(es.SuicidedAccountStates) { - return fmt.Errorf( - "invalid SuicidedAccountsIndex, %d is greater than the length of the suicided accounts (%d)", - key.SuicidedAccountsIndex, len(es.SuicidedAccountStates), - ) + return nil +} + +func (es *EphemeralStore) ValidateRevertKey(key StoreRevertKey) error { + validations := []struct { + idx int + length int + name string + }{ + {key.RefundIndex, len(es.RefundStates), "Refund"}, + {key.SuicidedAccountsIndex, len(es.SuicidedAccountStates), "SuicidedAccounts"}, + {key.LogsIndex, len(es.Logs), "Logs"}, + {key.TouchedAccountsIndex, len(es.TouchedAccounts), "TouchedAccounts"}, } - if key.LogsIndex > len(es.Logs) { - return fmt.Errorf( - "invalid LogsIndex, %d is greater than the length of the logs (%d)", - key.LogsIndex, len(es.Logs), - ) + for _, v := range validations { + if err := validateIndex(v.idx, v.length, v.name); err != nil { + return err + } } return nil @@ -147,3 +167,32 @@ func (es *EphemeralStore) GetAccountSuicided(addr common.Address) bool { func (es *EphemeralStore) GetAllSuicided() []common.Address { return es.SuicidedAccountStates } + +// ----------------------------------------------------------------------------- +// Touched accounts + +// SetTouched sets the given account as touched. +func (es *EphemeralStore) SetTouched(addr common.Address) { + // If already in the list, ignore + if es.IsTouched(addr) { + return + } + + es.TouchedAccounts = append(es.TouchedAccounts, addr) +} + +// IsTouched returns true if the given account is touched. +func (es *EphemeralStore) IsTouched(addr common.Address) bool { + for _, a := range es.TouchedAccounts { + if a == addr { + return true + } + } + + return false +} + +// GetAllTouched returns all touched accounts. +func (es *EphemeralStore) GetAllTouched() []common.Address { + return es.TouchedAccounts +}