Fix selfdestruct empty vs. exists bug (#878)

* Fix selfdestruct empty vs exists bug

* Restructure selfdestruct tests

go/integration_test/interpreter/dynamic_gas.go

@@ -840,6 +840,13 @@ 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))
 		}

go/integration_test/interpreter/verify_gas_test.go

@@ -106,12 +106,13 @@ 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 sets its own expectations
+						// SELFDESTRUCT gas computation is dependent on an account balance and
+						// existence (handled by nonce in this test), it sets its own expectations
 						if op != vm.SELFDESTRUCT {
+							mockStateDB.EXPECT().GetNonce(gomock.Any()).AnyTimes()
 							mockStateDB.EXPECT().GetBalance(gomock.Any()).AnyTimes().Return(accountBalance)
 						}
 

go/interpreter/lfvm/instructions.go

@@ -656,8 +656,16 @@ func opSelfdestruct(c *context) (status, error) {
 			cost += getAccessCost(accessStatus)
 		}
 	}
-	cost += selfDestructNewAccountCost(c.context.AccountExists(beneficiary),
-		c.context.GetBalance(c.params.Recipient))
+
+	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
+	}
+
 	// even death is not for free
 	if err := c.useGas(cost); err != nil {
 		return statusStopped, err
@@ -668,16 +676,6 @@ 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 {

go/interpreter/lfvm/instructions_test.go

@@ -851,45 +851,6 @@ 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.
 
@@ -901,7 +862,9 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	runContext := tosca.NewMockRunContext(ctrl)
 	runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(tosca.ColdAccess)
-	runContext.EXPECT().AccountExists(beneficiaryAddress).Return(false)
+	runContext.EXPECT().GetBalance(beneficiaryAddress).Return(tosca.Value{})
+	runContext.EXPECT().GetCode(beneficiaryAddress).Return(nil)
+	runContext.EXPECT().GetNonce(beneficiaryAddress).Return(uint64(0))
 	runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1})
 	runContext.EXPECT().SelfDestruct(selfAddress, beneficiaryAddress).Return(true)
 
@@ -932,47 +895,135 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) {
 	}
 }
 
-func TestSelfDestruct_ProperlyReportsNotEnoughGas(t *testing.T) {
-	for _, beneficiaryAccess := range []tosca.AccessStatus{tosca.WarmAccess, tosca.ColdAccess} {
-		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}
+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),
+		},
+	}
 
-				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,
+	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,
 					},
-					stack:   NewStack(),
-					memory:  NewMemory(),
-					context: runContext,
-				}
-				if beneficiaryAccess == tosca.ColdAccess {
-					ctxt.gas += 2600
-				}
-				if !accountExists {
-					ctxt.gas += 25000
-				}
-				ctxt.gas -= 1
+					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)
+			}
+		})
+	}
+}
 
-				ctxt.stack.push(new(uint256.Int).SetBytes(beneficiaryAddress[:]))
+func TestSelfDestruct_ReturnsOutOfGasError(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}
 
-				_, err := opSelfdestruct(&ctxt)
-				if err != errOutOfGas {
-					t.Fatalf("expected out of gas but got %v", err)
-				}
+			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})
 
-			})
-		}
+			ctxt := context{
+				params: tosca.Parameters{
+					BlockParameters: tosca.BlockParameters{
+						Revision: tosca.R13_Cancun,
+					},
+					Recipient: selfAddress,
+				},
+				stack:   NewStack(),
+				memory:  NewMemory(),
+				context: runContext,
+			}
+			if beneficiaryAccess == tosca.ColdAccess {
+				ctxt.gas += 2600
+			}
+			ctxt.gas -= 1
+
+			ctxt.stack.push(new(uint256.Int).SetBytes(beneficiaryAddress[:]))
+
+			_, err := opSelfdestruct(&ctxt)
+			if err != errOutOfGas {
+				t.Fatalf("expected out of gas but got %v", err)
+			}
+		})
 	}
 }
 

go/interpreter/lfvm/interpreter_test.go

@@ -188,6 +188,7 @@ 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()