From 5516682619264ca5890fce647af1e079c2b1abc7 Mon Sep 17 00:00:00 2001 From: Luis Ayuso Date: Fri, 18 Oct 2024 11:28:56 +0200 Subject: [PATCH] Revert "Fix selfdestruct empty vs. exists bug (#878)" This reverts commit f898b6b0dd572888197df4d9b8bcb3b348e73aa3. --- .../interpreter/dynamic_gas.go | 7 - .../interpreter/verify_gas_test.go | 5 +- go/interpreter/lfvm/instructions.go | 22 +- go/interpreter/lfvm/instructions_test.go | 203 +++++++----------- go/interpreter/lfvm/interpreter_test.go | 1 - 5 files changed, 90 insertions(+), 148 deletions(-) diff --git a/go/integration_test/interpreter/dynamic_gas.go b/go/integration_test/interpreter/dynamic_gas.go index 68fad0aab..270ce6bd2 100644 --- a/go/integration_test/interpreter/dynamic_gas.go +++ b/go/integration_test/interpreter/dynamic_gas.go @@ -840,13 +840,6 @@ func gasDynamicSelfDestruct(revision Revision) []*DynGasTest { }) mock.EXPECT().AccountExists(targetAddress).AnyTimes().Return(!empty) - mock.EXPECT().GetBalance(targetAddress).AnyTimes().Return(tosca.Value{}) - mock.EXPECT().GetCode(targetAddress).AnyTimes().Return(nil) - if empty { - mock.EXPECT().GetNonce(targetAddress).AnyTimes().Return(uint64(0)) - } else { - mock.EXPECT().GetNonce(targetAddress).AnyTimes().Return(uint64(1)) - } mock.EXPECT().IsAddressInAccessList(targetAddress).AnyTimes().Return(inAcl) mock.EXPECT().AccessAccount(targetAddress).AnyTimes().Return(tosca.AccessStatus(inAcl)) } diff --git a/go/integration_test/interpreter/verify_gas_test.go b/go/integration_test/interpreter/verify_gas_test.go index 3c6dff148..27aa069cb 100644 --- a/go/integration_test/interpreter/verify_gas_test.go +++ b/go/integration_test/interpreter/verify_gas_test.go @@ -106,13 +106,12 @@ func TestDynamicGas(t *testing.T) { // World state interactions triggered by the EVM. mockStateDB.EXPECT().SetBalance(gomock.Any(), gomock.Any()).AnyTimes() + mockStateDB.EXPECT().GetNonce(gomock.Any()).AnyTimes() mockStateDB.EXPECT().SetNonce(gomock.Any(), gomock.Any()).AnyTimes() mockStateDB.EXPECT().SetCode(gomock.Any(), gomock.Any()).AnyTimes() - // SELFDESTRUCT gas computation is dependent on an account balance and - // existence (handled by nonce in this test), it sets its own expectations + // SELFDESTRUCT gas computation is dependent on an account balance and sets its own expectations if op != vm.SELFDESTRUCT { - mockStateDB.EXPECT().GetNonce(gomock.Any()).AnyTimes() mockStateDB.EXPECT().GetBalance(gomock.Any()).AnyTimes().Return(accountBalance) } diff --git a/go/interpreter/lfvm/instructions.go b/go/interpreter/lfvm/instructions.go index 15d6df456..4afbc7252 100644 --- a/go/interpreter/lfvm/instructions.go +++ b/go/interpreter/lfvm/instructions.go @@ -656,16 +656,8 @@ func opSelfdestruct(c *context) (status, error) { cost += getAccessCost(accessStatus) } } - - empty := c.context.GetBalance(beneficiary) == (tosca.Value{}) && - c.context.GetCode(beneficiary) == nil && - c.context.GetNonce(beneficiary) == 0 - balance := c.context.GetBalance(c.params.Recipient) - if empty && balance != (tosca.Value{}) { - // cost of creating an account defined in eip-150 (see https://eips.ethereum.org/EIPS/eip-150) - cost += 25_000 - } - + cost += selfDestructNewAccountCost(c.context.AccountExists(beneficiary), + c.context.GetBalance(c.params.Recipient)) // even death is not for free if err := c.useGas(cost); err != nil { return statusStopped, err @@ -676,6 +668,16 @@ func opSelfdestruct(c *context) (status, error) { return statusSelfDestructed, nil } +func selfDestructNewAccountCost(accountExists bool, balance tosca.Value) tosca.Gas { + if !accountExists && balance != (tosca.Value{}) { + // cost of creating an account defined in eip-150 (see https://eips.ethereum.org/EIPS/eip-150) + // CreateBySelfdestructGas is used when the refunded account is one that does + // not exist. This logic is similar to call. + return 25_000 + } + return 0 +} + func selfDestructRefund(destructed bool, revision tosca.Revision) tosca.Gas { // Since London and after there is no more refund (see https://eips.ethereum.org/EIPS/eip-3529) if destructed && revision < tosca.R10_London { diff --git a/go/interpreter/lfvm/instructions_test.go b/go/interpreter/lfvm/instructions_test.go index 319979a2f..7ccf24315 100644 --- a/go/interpreter/lfvm/instructions_test.go +++ b/go/interpreter/lfvm/instructions_test.go @@ -851,6 +851,45 @@ func TestSelfDestruct_Refund(t *testing.T) { } } +func TestSelfDestruct_NewAccountCost(t *testing.T) { + + tests := map[string]struct { + beneficiaryExists bool + balance tosca.Value + cost tosca.Gas + }{ + "account exists no balance": { + beneficiaryExists: true, + balance: tosca.Value{}, + cost: 0, + }, + "account exists with balance": { + beneficiaryExists: true, + balance: tosca.Value{1}, + cost: 0, + }, + "new account without balance": { + beneficiaryExists: false, + balance: tosca.Value{}, + cost: 0, + }, + "new account with balance": { + beneficiaryExists: false, + balance: tosca.Value{1}, + cost: 25_000, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + cost := selfDestructNewAccountCost(test.beneficiaryExists, test.balance) + if cost != test.cost { + t.Errorf("unexpected gas, wanted %d, got %d", test.cost, cost) + } + }) + } +} + func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) { // This tests produces the combination of context calls/results for the maximum dynamic gas cost possible. @@ -862,9 +901,7 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) { ctrl := gomock.NewController(t) runContext := tosca.NewMockRunContext(ctrl) runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(tosca.ColdAccess) - runContext.EXPECT().GetBalance(beneficiaryAddress).Return(tosca.Value{}) - runContext.EXPECT().GetCode(beneficiaryAddress).Return(nil) - runContext.EXPECT().GetNonce(beneficiaryAddress).Return(uint64(0)) + runContext.EXPECT().AccountExists(beneficiaryAddress).Return(false) runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1}) runContext.EXPECT().SelfDestruct(selfAddress, beneficiaryAddress).Return(true) @@ -895,135 +932,47 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) { } } -func TestSelfDestruct_ReturnsSelfDestructedAndConsumesGas(t *testing.T) { - tests := map[string]struct { - balance tosca.Value - beneficiaryNonce uint64 - beneficiaryBalance tosca.Value - beneficiaryCode []byte - remainingGas tosca.Gas - }{ - "balance and empty beneficiary account": { - tosca.Value{1}, - 0, - tosca.Value{}, - nil, - tosca.Gas(0), - }, - "balance and non empty beneficiary account with nonce": { - tosca.Value{1}, - 1, - tosca.Value{}, - nil, - tosca.Gas(25_000), - }, - "balance and non empty beneficiary account with Balance": { - tosca.Value{1}, - 0, - tosca.Value{1}, - nil, - tosca.Gas(25_000), - }, - "balance and non empty beneficiary account with Code": { - tosca.Value{1}, - 0, - tosca.Value{}, - []byte{0x01}, - tosca.Gas(25_000), - }, - "no balance and empty beneficiary": { - tosca.Value{}, - 0, - tosca.Value{}, - nil, - tosca.Gas(25_000), - }, - "no balance and non empty beneficiary": { - tosca.Value{}, - 1, - tosca.Value{1}, - nil, - tosca.Gas(25_000), - }, - } - - for name, test := range tests { - t.Run(name, func(t *testing.T) { - address := tosca.Address{0x01} - beneficiary := tosca.Address{0x02} - ctrl := gomock.NewController(t) - runContext := tosca.NewMockRunContext(ctrl) - - runContext.EXPECT().AccessAccount(beneficiary).Return(tosca.ColdAccess) - runContext.EXPECT().GetBalance(beneficiary).Return(test.beneficiaryBalance) - runContext.EXPECT().GetCode(beneficiary).Return(test.beneficiaryCode).AnyTimes() - runContext.EXPECT().GetNonce(beneficiary).Return(test.beneficiaryNonce).AnyTimes() - runContext.EXPECT().GetBalance(address).Return(test.balance) - runContext.EXPECT().SelfDestruct(address, beneficiary).Return(true) - - ctxt := context{ - params: tosca.Parameters{ - BlockParameters: tosca.BlockParameters{ - Revision: tosca.R13_Cancun, - }, - Recipient: address, - }, - stack: NewStack(), - memory: NewMemory(), - context: runContext, - gas: 27_600, - } - ctxt.stack.push(new(uint256.Int).SetBytes(beneficiary[:])) - status, err := opSelfdestruct(&ctxt) - if err != nil { - t.Fatalf("unexpected error, got %v", err) - } - if want, got := statusSelfDestructed, status; want != got { - t.Fatalf("unexpected status, wanted %v, got %v", want, got) - } - if want, got := test.remainingGas, ctxt.gas; want != got { - t.Errorf("unexpected gas, wanted %d, got %d", want, got) - } - }) - } -} - -func TestSelfDestruct_ReturnsOutOfGasError(t *testing.T) { +func TestSelfDestruct_ProperlyReportsNotEnoughGas(t *testing.T) { for _, beneficiaryAccess := range []tosca.AccessStatus{tosca.WarmAccess, tosca.ColdAccess} { - t.Run(fmt.Sprintf("beneficiaryAccess:%v", beneficiaryAccess), func(t *testing.T) { - beneficiaryAddress := tosca.Address{1} - selfAddress := tosca.Address{2} - - ctrl := gomock.NewController(t) - runContext := tosca.NewMockRunContext(ctrl) - runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(beneficiaryAccess) - // Existing beneficiary - runContext.EXPECT().GetBalance(beneficiaryAddress).Return(tosca.Value{1}) - runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1}) + for _, accountExists := range []bool{true, false} { + t.Run(fmt.Sprintf("beneficiaryAccess:%v_accountExists:%v", beneficiaryAccess, accountExists), func(t *testing.T) { + beneficiaryAddress := tosca.Address{1} + selfAddress := tosca.Address{2} - ctxt := context{ - params: tosca.Parameters{ - BlockParameters: tosca.BlockParameters{ - Revision: tosca.R13_Cancun, + ctrl := gomock.NewController(t) + runContext := tosca.NewMockRunContext(ctrl) + runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(beneficiaryAccess) + runContext.EXPECT().AccountExists(beneficiaryAddress).Return(accountExists) + runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1}) + + ctxt := context{ + params: tosca.Parameters{ + BlockParameters: tosca.BlockParameters{ + Revision: tosca.R13_Cancun, + }, + Recipient: selfAddress, }, - Recipient: selfAddress, - }, - stack: NewStack(), - memory: NewMemory(), - context: runContext, - } - if beneficiaryAccess == tosca.ColdAccess { - ctxt.gas += 2600 - } - ctxt.gas -= 1 + stack: NewStack(), + memory: NewMemory(), + context: runContext, + } + if beneficiaryAccess == tosca.ColdAccess { + ctxt.gas += 2600 + } + if !accountExists { + ctxt.gas += 25000 + } + ctxt.gas -= 1 - ctxt.stack.push(new(uint256.Int).SetBytes(beneficiaryAddress[:])) + ctxt.stack.push(new(uint256.Int).SetBytes(beneficiaryAddress[:])) - _, err := opSelfdestruct(&ctxt) - if err != errOutOfGas { - t.Fatalf("expected out of gas but got %v", err) - } - }) + _, err := opSelfdestruct(&ctxt) + if err != errOutOfGas { + t.Fatalf("expected out of gas but got %v", err) + } + + }) + } } } diff --git a/go/interpreter/lfvm/interpreter_test.go b/go/interpreter/lfvm/interpreter_test.go index d59f7e26b..287e91709 100644 --- a/go/interpreter/lfvm/interpreter_test.go +++ b/go/interpreter/lfvm/interpreter_test.go @@ -188,7 +188,6 @@ func TestInterpreter_CanDispatchExecutableInstructions(t *testing.T) { mock.EXPECT().GetBalance(gomock.Any()).AnyTimes() mock.EXPECT().GetCodeSize(gomock.Any()).AnyTimes() mock.EXPECT().GetCode(gomock.Any()).AnyTimes() - mock.EXPECT().GetNonce(gomock.Any()).AnyTimes() mock.EXPECT().AccountExists(gomock.Any()).AnyTimes() mock.EXPECT().AccessStorage(gomock.Any(), gomock.Any()).AnyTimes() mock.EXPECT().GetStorage(gomock.Any(), gomock.Any()).AnyTimes()