changes requested in review. add and use getAccessCost from #730

go/interpreter/lfvm/gas.go

@@ -58,8 +58,7 @@ func init() {
 }
 
 func initBerlinGasPrice() {
-	// Changed static gas prices with EIP2929.
-	// these prices account for warm access cost
+	// Changed static gas prices with EIP2929
 	static_gas_prices_berlin[SLOAD] = 0
 	static_gas_prices_berlin[EXTCODECOPY] = 100
 	static_gas_prices_berlin[EXTCODESIZE] = 100

go/interpreter/lfvm/instructions.go

@@ -727,7 +727,11 @@ func opSelfdestruct(c *context) {
 	// Selfdestruct gas cost defined in EIP-105 (see https://eips.ethereum.org/EIPS/eip-150)
 	cost := tosca.Gas(0)
 	if c.isAtLeast(tosca.R09_Berlin) {
-		cost += selfDestructedBeneficiaryAccessCost(c.context.AccessAccount(beneficiary))
+		// as https://eips.ethereum.org/EIPS/eip-2929#selfdestruct-changes says,
+		// selfdestruct does not charge for warm access
+		if accessStatus := c.context.AccessAccount(beneficiary); accessStatus != tosca.WarmAccess {
+			cost += getAccessCost(accessStatus)
+		}
 	}
 	cost += selfDestructNewAccountCost(c.context.AccountExists(beneficiary),
 		c.context.GetBalance(c.params.Recipient))
@@ -741,14 +745,6 @@ func opSelfdestruct(c *context) {
 	c.status = statusSelfDestructed
 }
 
-func selfDestructedBeneficiaryAccessCost(accessStatus tosca.AccessStatus) tosca.Gas {
-	if accessStatus == tosca.ColdAccess {
-		// Cost of cold account access after EIP 2929 (see https://eips.ethereum.org/EIPS/eip-2929)
-		return 2_600
-	}
-	return 0
-}
-
 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)
@@ -760,8 +756,8 @@ func selfDestructNewAccountCost(accountExists bool, balance tosca.Value) tosca.G
 }
 
 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.R09_Berlin {
-		// Refunded following a selfdestruct operation (see https://eips.ethereum.org/EIPS/eip-6780)
 		return 24_000
 	}
 	return 0
@@ -1113,6 +1109,16 @@ func neededMemorySize(c *context, offset, size *uint256.Int) (uint64, error) {
 	return offset.Uint64() + size.Uint64(), nil
 }
 
+func getAccessCost(accessStatus tosca.AccessStatus) tosca.Gas {
+	// EIP-2929 says that cold access cost is 2600 and warm is 100.
+	// however, the warm access cost is charged as part of the base gas cost.
+	// (https://eips.ethereum.org/EIPS/eip-2929)
+	if accessStatus == tosca.ColdAccess {
+		return tosca.Gas(2600)
+	}
+	return tosca.Gas(100)
+}
+
 func genericCall(c *context, kind tosca.CallKind) {
 	warmAccess, coldCost, err := addressInAccessList(c)
 	if err != nil {

go/interpreter/lfvm/instructions_test.go

@@ -922,65 +922,40 @@ func TestSelfDestruct_Refund(t *testing.T) {
 	}
 }
 
-func TestSelfDestruct_BeneficiaryAccessCost(t *testing.T) {
-	tests := map[string]struct {
-		accessStatus tosca.AccessStatus
-		gas          tosca.Gas
-	}{
-		"cold account": {
-			accessStatus: tosca.ColdAccess,
-			gas:          2_600,
-		},
-		"warm account": {
-			accessStatus: tosca.WarmAccess,
-			gas:          0,
-		},
-	}
-
-	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
-			gas := selfDestructedBeneficiaryAccessCost(test.accessStatus)
-			if gas != test.gas {
-				t.Errorf("unexpected gas, wanted %d, got %d", test.gas, gas)
-			}
-		})
-	}
-}
-
 func TestSelfDestruct_NewAccountCost(t *testing.T) {
 
 	tests := map[string]struct {
 		beneficiaryExists bool
 		balance           tosca.Value
-		gas               tosca.Gas
+		cost              tosca.Gas
 	}{
 		"account exists no balance": {
 			beneficiaryExists: true,
 			balance:           tosca.Value{},
-			gas:               0,
+			cost:              0,
 		},
 		"account exists with balance": {
 			beneficiaryExists: true,
 			balance:           tosca.Value{1},
-			gas:               0,
+			cost:              0,
 		},
 		"new account without balance": {
 			beneficiaryExists: false,
 			balance:           tosca.Value{},
-			gas:               0,
+			cost:              0,
 		},
 		"new account with balance": {
 			beneficiaryExists: false,
 			balance:           tosca.Value{1},
-			gas:               25_000,
+			cost:              25_000,
 		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			cost := selfDestructNewAccountCost(test.beneficiaryExists, test.balance)
-			if cost != test.gas {
-				t.Errorf("unexpected gas, wanted %d, got %d", test.gas, cost)
+			if cost != test.cost {
+				t.Errorf("unexpected gas, wanted %d, got %d", test.cost, cost)
 			}
 		})
 	}
@@ -991,6 +966,8 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) {
 
 	beneficiaryAddress := tosca.Address{1}
 	selfAddress := tosca.Address{2}
+	// added to gas to ensure operation is not simply setting gas to zero.
+	gasDelta := tosca.Gas(1)
 
 	ctrl := gomock.NewController(t)
 	runContext := tosca.NewMockRunContext(ctrl)
@@ -1011,7 +988,7 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) {
 		memory:  NewMemory(),
 		context: runContext,
 		// 25_000 for new account, 2_600 for beneficiary access
-		gas: 27_600,
+		gas: 27_600 + gasDelta,
 	}
 	ctxt.stack.push(new(uint256.Int).SetBytes(beneficiaryAddress[:]))
 
@@ -1020,7 +997,7 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) {
 	if ctxt.status != statusSelfDestructed {
 		t.Errorf("unexpected status, wanted %v, got %v", statusRunning, ctxt.status)
 	}
-	if ctxt.gas != 0 {
-		t.Errorf("all gas should have been used, got %d spare", ctxt.gas)
+	if ctxt.gas != gasDelta {
+		t.Errorf("unexpected remaining gas, wated %v, got %d", gasDelta, ctxt.gas)
 	}
 }

go/tosca/world_state.go

@@ -34,7 +34,7 @@ type WorldState interface {
 	GetStorage(Address, Key) Word
 	SetStorage(Address, Key, Word) StorageStatus
 
-	// Destroys addr and transfers its balance to beneficiary.
+	// SelfDestruct destroys addr and transfers its balance to beneficiary.
 	// Returns true if the given account is destructed the first time in the ongoing transaction, false otherwise.
 	SelfDestruct(addr Address, beneficiary Address) bool
 }