Review fixes: corect arbos version, simplify redeem gas accounting

Author: MishkaRogachev

arbos/l2pricing/model.go

@@ -14,9 +14,6 @@ import (
 	"github.com/offchainlabs/nitro/util/arbmath"
 )
 
-const ArbosSingleGasConstraintsVersion = params.ArbosVersion_50
-const ArbosMultiGasConstraintsVersion = params.ArbosVersion_60
-
 const InitialSpeedLimitPerSecondV0 = 1000000
 const InitialPerBlockGasLimitV0 uint64 = 20 * 1000000
 const InitialSpeedLimitPerSecondV6 = 7000000
@@ -27,7 +24,7 @@ const InitialPricingInertia = 102
 const InitialBacklogTolerance = 10
 const InitialPerTxGasLimitV50 uint64 = 32 * 1000000
 
-const ArbOS60StaticBacklogUpdateCost = 15000
+const MultiConstraintStaticBacklogUpdateCost = 15000
 
 type GasModel int
 
@@ -41,7 +38,7 @@ const (
 // GasModelToUse returns the active gas-pricing model based on ArbOS version
 // and whether the corresponding constraints are currently configured.
 func (ps *L2PricingState) GasModelToUse() (GasModel, error) {
-	if ps.ArbosVersion >= ArbosMultiGasConstraintsVersion {
+	if ps.ArbosVersion >= params.ArbosVersion_MultiGasConstraintsVersion {
 		constraintsLength, err := ps.MultiGasConstraintsLength()
 		if err != nil {
 			return GasModelUnknown, err
@@ -50,7 +47,7 @@ func (ps *L2PricingState) GasModelToUse() (GasModel, error) {
 			return GasModelMultiGasConstraints, nil
 		}
 	}
-	if ps.ArbosVersion >= ArbosSingleGasConstraintsVersion {
+	if ps.ArbosVersion >= params.ArbosVersion_SingleGasConstraintsVersion {
 		constraintsLength, err := ps.GasConstraintsLength()
 		if err != nil {
 			return GasModelUnknown, err
@@ -152,19 +149,22 @@ func applyGasDelta(op BacklogOperation, backlog uint64, delta uint64) uint64 {
 }
 
 // BacklogUpdateCost returns the gas cost for updating the backlog in the active pricing model.
-// This function won't be called starting from ArbOS 60, where the cost is static (ArbOS60StaticBacklogUpdateCost)
 func (ps *L2PricingState) BacklogUpdateCost() uint64 {
-	result := uint64(0)
+	// Charge static price for any pricer starting from ArbosVersion_MultiGasConstraintsVersion
+	if ps.ArbosVersion >= params.ArbosVersion_MultiGasConstraintsVersion {
+		return MultiConstraintStaticBacklogUpdateCost
+	}
 
+	result := uint64(0)
 	// Single-dimensional constraint pricer costs
 	// This overhead applies even when no constraints are configured.
-	if ps.ArbosVersion >= ArbosSingleGasConstraintsVersion {
+	if ps.ArbosVersion >= params.ArbosVersion_SingleGasConstraintsVersion {
 		// Read gas constraints length for "GasModelToUse()"
 		result += storage.StorageReadCost
 	}
 
 	if ps.ArbosVersion >= params.ArbosVersion_MultiConstraintFix {
-		// updateSingleGasConstraintsBacklogs costs (ArbOS 51 and later)
+		// updateSingleGasConstraintsBacklogs costs (ArbosVersion_MultiConstraintFix and later)
 		constraintsLength, _ := ps.gasConstraints.Length()
 		if constraintsLength > 0 {
 			result += storage.StorageReadCost // read length to traverse

arbos/tx_processor.go

@@ -21,7 +21,6 @@ import (
 
 	"github.com/offchainlabs/nitro/arbos/arbosState"
 	"github.com/offchainlabs/nitro/arbos/l1pricing"
-	"github.com/offchainlabs/nitro/arbos/l2pricing"
 	"github.com/offchainlabs/nitro/arbos/retryables"
 	"github.com/offchainlabs/nitro/arbos/util"
 	"github.com/offchainlabs/nitro/util/arbmath"
@@ -537,7 +536,7 @@ func (p *TxProcessor) EndTxHook(gasLeft uint64, usedMultiGas multigas.MultiGas,
 
 	var multiDimensionalCost *big.Int
 	var err error
-	if p.state.L2PricingState().ArbosVersion >= l2pricing.ArbosMultiGasConstraintsVersion {
+	if p.state.L2PricingState().ArbosVersion >= params.ArbosVersion_MultiGasConstraintsVersion {
 		multiDimensionalCost, err = p.state.L2PricingState().MultiDimensionalPriceForRefund(usedMultiGas)
 		p.state.Restrict(err)
 	}

go-ethereum

@@ -12,7 +12,6 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/params"
 
-	"github.com/offchainlabs/nitro/arbos/l2pricing"
 	"github.com/offchainlabs/nitro/arbos/retryables"
 	"github.com/offchainlabs/nitro/arbos/util"
 	"github.com/offchainlabs/nitro/util/arbmath"
@@ -102,13 +101,7 @@ func (con ArbRetryableTx) Redeem(c ctx, evm mech, ticketId bytes32) (bytes32, er
 	// `redeem` must prepay the gas needed by the trailing call to
 	// L2PricingState().ShrinkBacklog(). BacklogUpdateCost() returns
 	// that amount based on the storage read/write mix used by ShrinkBacklog().
-	backlogUpdateCost := uint64(0)
-	if c.State.L2PricingState().ArbosVersion >= params.ArbosVersion_60 {
-		// Charge static price for any pricer starting from ArbOS 60
-		backlogUpdateCost = l2pricing.ArbOS60StaticBacklogUpdateCost
-	} else {
-		backlogUpdateCost = c.State.L2PricingState().BacklogUpdateCost()
-	}
+	backlogUpdateCost := c.State.L2PricingState().BacklogUpdateCost()
 
 	futureGasCosts := eventCost + gasCostToReturnResult + backlogUpdateCost
 	if c.GasLeft() < futureGasCosts {
@@ -139,16 +132,13 @@ func (con ArbRetryableTx) Redeem(c ctx, evm mech, ticketId bytes32) (bytes32, er
 
 	// Add the gasToDonate back to the gas pool: the retryable attempt will then consume it.
 	// This ensures that the gas pool has enough gas to run the retryable attempt.
-	// Starting from ArbOS 60, we don't charge gas for the ShrinkBacklog call here
-	if c.State.L2PricingState().ArbosVersion >= params.ArbosVersion_60 {
-		err = c.WithUnmeteredGasAccounting(func() error {
-			return c.State.L2PricingState().ShrinkBacklog(gasToDonate, multigas.ComputationGas(gasToDonate))
-		})
-	} else {
-		err = c.State.L2PricingState().ShrinkBacklog(gasToDonate, multigas.ComputationGas(gasToDonate))
+	// Starting from ArbosVersion_MultiGasConstraintsVersion, don't charge gas for the ShrinkBacklog call.
+	stopChargingGas := c.State.L2PricingState().ArbosVersion >= params.ArbosVersion_MultiGasConstraintsVersion
+	if stopChargingGas {
+		c.SetUnmeteredGasAccounting(true)
+		defer c.SetUnmeteredGasAccounting(false)
 	}
-
-	return retryTxHash, err
+	return retryTxHash, c.State.L2PricingState().ShrinkBacklog(gasToDonate, multigas.ComputationGas(gasToDonate))
 }
 
 // GetLifetime gets the default lifetime period a retryable has at creation

precompiles/ArbRetryableTx.go

@@ -127,7 +127,7 @@ func TestRetryableRedeemLegacy(t *testing.T) {
 
 func TestRetryableRedeemLegacyArbOS60(t *testing.T) {
 	evm := newMockEVMForTesting()
-	err := overrideStateArbOSVersion(evm, params.ArbosVersion_60)
+	err := overrideStateArbOSVersion(evm, params.ArbosVersion_MultiGasConstraintsVersion)
 	Require(t, err)
 
 	precompileCtx := testContext(common.Address{}, evm)
@@ -167,7 +167,7 @@ func TestRetryableRedeemWithGasConstraints(t *testing.T) {
 
 func TestRetryableRedeemWithGasConstraintsArbOS60(t *testing.T) {
 	evm := newMockEVMForTesting()
-	err := overrideStateArbOSVersion(evm, params.ArbosVersion_60)
+	err := overrideStateArbOSVersion(evm, params.ArbosVersion_MultiGasConstraintsVersion)
 	Require(t, err)
 
 	precompileCtx := testContext(common.Address{}, evm)
@@ -193,7 +193,7 @@ func TestRetryableRedeemWithGasConstraintsArbOS60(t *testing.T) {
 
 func TestRetryableRedeemWithMultiGasConstraints(t *testing.T) {
 	evm := newMockEVMForTesting()
-	err := overrideStateArbOSVersion(evm, l2pricing.ArbosMultiGasConstraintsVersion)
+	err := overrideStateArbOSVersion(evm, params.ArbosVersion_MultiGasConstraintsVersion)
 	Require(t, err)
 
 	precompileCtx := testContext(common.Address{}, evm)

precompiles/ArbRetryableTx_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/core/vm"
 	"github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethereum/go-ethereum/params"
 
 	"github.com/offchainlabs/nitro/arbos/arbosState"
 	"github.com/offchainlabs/nitro/arbos/burn"
@@ -38,7 +39,7 @@ func setupResourceConstraintHandles(
 	state, err := arbosState.OpenArbosState(evm.StateDB, burn.NewSystemBurner(tracer, false))
 	require.NoError(t, err)
 
-	state.L2PricingState().ArbosVersion = l2pricing.ArbosMultiGasConstraintsVersion
+	state.L2PricingState().ArbosVersion = params.ArbosVersion_MultiGasConstraintsVersion
 
 	arbGasInfo := &ArbGasInfo{}
 	arbOwner := &ArbOwner{}

precompiles/constraints_test.go

@@ -74,17 +74,12 @@ func (c *Context) ReadOnly() bool {
 	return c.readOnly
 }
 
-func (c *Context) Free() bool {
+func (c *Context) UnmeteredGasAccounting() bool {
 	return c.free
 }
 
-func (c *Context) WithUnmeteredGasAccounting(fn func() error) error {
-	prev := c.free
-	c.free = true
-	defer func() {
-		c.free = prev
-	}()
-	return fn()
+func (c *Context) SetUnmeteredGasAccounting(enabled bool) {
+	c.free = enabled
 }
 
 func (c *Context) TracingInfo() *util.TracingInfo {

precompiles/context.go

@@ -67,7 +67,7 @@ func TestSetAndGetMultiGasPricingConstraints(t *testing.T) {
 
 	builder := NewNodeBuilder(ctx).
 		DefaultConfig(t, false).
-		WithArbOSVersion(l2pricing.ArbosMultiGasConstraintsVersion)
+		WithArbOSVersion(params.ArbosVersion_MultiGasConstraintsVersion)
 
 	cleanup := builder.Build(t)
 	defer cleanup()
@@ -150,7 +150,7 @@ func TestMultiGasRefundForNormalTx(t *testing.T) {
 
 	builder := NewNodeBuilder(ctx).
 		DefaultConfig(t, false).
-		WithArbOSVersion(l2pricing.ArbosMultiGasConstraintsVersion)
+		WithArbOSVersion(params.ArbosVersion_MultiGasConstraintsVersion)
 
 	cleanup := builder.Build(t)
 	defer cleanup()
@@ -226,7 +226,7 @@ func TestMultiGasRefundForNormalTx(t *testing.T) {
 
 func TestMultiGasRefundForRetryableTx(t *testing.T) {
 	builder, delayedInbox, lookupL2Tx, ctx, teardown := retryableSetup(t, func(b *NodeBuilder) {
-		b.WithArbOSVersion(l2pricing.ArbosMultiGasConstraintsVersion)
+		b.WithArbOSVersion(params.ArbosVersion_MultiGasConstraintsVersion)
 	})
 	defer teardown()