Merge pull request #348 from OffchainLabs/gligneul/nit-5020

Backport(consensus): Do not combine multi-gas refunds with EVM refunds

Author: joshuacolvin0

arbos/l2pricing/model.go

@@ -8,7 +8,6 @@ import (
 	"math/big"
 
 	"github.com/ethereum/go-ethereum/arbitrum/multigas"
-	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/params"
 
 	"github.com/offchainlabs/nitro/arbos/storage"
@@ -366,6 +365,9 @@ func (ps *L2PricingState) GetMultiGasBaseFeePerResource(blockBaseFee *big.Int) (
 	return fees, nil
 }
 
+// MultiDimensionalPriceForRefund returns the multi-gas fee considering each dimension's base-fee.
+// It doesn't take in consideration EVM SSTORE refunds. So, the transaction will be charged the
+// minimum between `single-gas fee minus EVM refunds` AND `multi-gas fee without EVM refunds`.
 func (ps *L2PricingState) MultiDimensionalPriceForRefund(usedMultiGas multigas.MultiGas, blockBaseFee *big.Int) (*big.Int, error) {
 	fees, err := ps.GetMultiGasBaseFeePerResource(blockBaseFee)
 	if err != nil {
@@ -388,19 +390,6 @@ func (ps *L2PricingState) MultiDimensionalPriceForRefund(usedMultiGas multigas.M
 		)
 		total.Add(total, part)
 	}
-
-	// Take in consideration evm refunds using the block base fee.
-	if ps.ArbosVersion >= params.ArbosVersion_MultiGasRefundFix {
-		evmRefunds := new(big.Int).Mul(new(big.Int).SetUint64(usedMultiGas.GetRefund()), blockBaseFee)
-		if total.Cmp(evmRefunds) > 0 {
-			// This should always be true, but check anyway to ensure we don't return a negative value.
-			total.Sub(total, evmRefunds)
-		} else {
-			log.Warn("EVM refund greater than multi-gas fee",
-				"multi-gas-fee", total, "evm-refunds", evmRefunds)
-		}
-	}
-
 	return total, nil
 }
 

arbos/tx_processor_multigas_test.go

@@ -273,12 +273,9 @@ func TestEndTxHookMultiGasRefundRetryableTx(t *testing.T) {
 	)
 }
 
-// TestEndTxHookMultiGasRefundWithEVMRefundCredit verifies that the multi-gas
-// refund stays invariant under EVM SSTORE refunds (EIP-3529). usedMultiGas's
-// per-kind sum is pre-refund (peak); on v61+ MultiDimensionalPriceForRefund
-// subtracts usedMultiGas.GetRefund() * baseFee from its result so it can be
-// paired with the net totalCost = baseFee * (gasLimit - gasLeft) computed in
-// EndTxHook.
+// TestEndTxHookMultiGasRefundWithEVMRefundCredit verifies that the EndTxHook doesn't give extra refunds when the EVM
+// refunds are greater than the multi-dimensional one.
+// The EVM refunds are given back to the user before EndTxHook is called, in stateTransition.returnGas().
 func TestEndTxHookMultiGasRefundWithEVMRefundCredit(t *testing.T) {
 	const gasLimit uint64 = 1_000_000
 	const evmRefundCredit uint64 = 200_000 // EIP-3529 cap
@@ -302,40 +299,37 @@ func TestEndTxHookMultiGasRefundWithEVMRefundCredit(t *testing.T) {
 	pricing := txProcessor.state.L2PricingState()
 	pricing.ArbosVersion = params.ArbosVersion_MultiGasRefundFix
 
+	// Set a 99/100 ratio to make the multi-gas refund smaller than the EVM refunds.
 	Require(t, pricing.AddMultiGasConstraint(
 		100_000,
 		10,
 		200_000_000_000,
 		map[uint8]uint64{
-			uint8(multigas.ResourceKindComputation):   1,
-			uint8(multigas.ResourceKindStorageGrowth): 10,
+			uint8(multigas.ResourceKindComputation):   99,
+			uint8(multigas.ResourceKindStorageGrowth): 100,
 		},
 	))
-	pricing.UpdatePricingModel(100)
+	pricing.UpdatePricingModel(1)
 	require.NoError(t, pricing.CommitMultiGasFees())
 
 	baseFee, err := pricing.BaseFeeWei()
 	require.NoError(t, err)
-	evm.Context.BaseFee = new(big.Int).Set(baseFee)
+	evm.Context.BaseFee = baseFee
 
 	gasUsed := gasLimit - gasLeft
 	peakGasUsed := gasUsed + evmRefundCredit
 	usedMultiGas := multigas.MultiGasFromPairs(
 		multigas.Pair{Kind: multigas.ResourceKindComputation, Amount: peakGasUsed / 2},
-		multigas.Pair{Kind: multigas.ResourceKindStorageAccessRead, Amount: peakGasUsed / 2},
+		multigas.Pair{Kind: multigas.ResourceKindStorageGrowth, Amount: peakGasUsed / 2},
 	).WithRefund(evmRefundCredit)
 
-	multiDimensionalCost, err := pricing.MultiDimensionalPriceForRefund(usedMultiGas, baseFee)
+	multiGasFee, err := pricing.MultiDimensionalPriceForRefund(usedMultiGas, baseFee)
 	require.NoError(t, err)
-	totalCost := new(big.Int).Mul(baseFee, new(big.Int).SetUint64(gasUsed))
-	expectedRefund := new(big.Int).Sub(totalCost, multiDimensionalCost)
-	require.True(t, expectedRefund.Sign() > 0, "expected positive multi-gas refund, got %v", expectedRefund)
+	singleGasFee := new(big.Int).Mul(baseFee, new(big.Int).SetUint64(gasUsed))
+	require.Negative(t, singleGasFee.Cmp(multiGasFee), "expected singleGasFee < multiGasFee")
 
+	// Expect zero balance because the EVM refunds are given before EndTxHook is called.
 	txProcessor.EndTxHook(gasLeft, usedMultiGas, true)
-
-	require.True(t,
-		arbmath.BigEquals(expectedRefund, evm.StateDB.GetBalance(from).ToBig()),
-		"refund mismatch under EVM refund credit: got %v, want %v",
-		evm.StateDB.GetBalance(from).ToBig(), expectedRefund,
-	)
+	balance := evm.StateDB.GetBalance(from).ToBig()
+	require.Zero(t, balance.Sign(), "unexpected refund in EndTxHook")
 }

changelog/gligneul-nit-5019.md

@@ -0,0 +1,2 @@
+### Fixed
+- Do not combine multi-gas refunds with EVM refunds.