Merge pull request #687 from onflow/revert-671-mpeter/improve-gas-estimation-logic

Revert "Improve gas estimation logic for `eth_estimateGas`"

Author: peterargue

api/utils.go

@@ -136,7 +136,7 @@ func handleError[T any](err error, log zerolog.Logger, collector metrics.Collect
 // `EVM.dryRun` inside Cadence scripts, meaning that no state change
 // will occur.
 // This is only useful for `eth_estimateGas` and `eth_call` endpoints.
-func encodeTxFromArgs(args ethTypes.TransactionArgs) (*types.DynamicFeeTx, error) {
+func encodeTxFromArgs(args ethTypes.TransactionArgs) (*types.LegacyTx, error) {
 	var data []byte
 	if args.Data != nil {
 		data = *args.Data
@@ -156,19 +156,12 @@ func encodeTxFromArgs(args ethTypes.TransactionArgs) (*types.DynamicFeeTx, error
 		value = args.Value.ToInt()
 	}
 
-	accessList := types.AccessList{}
-	if args.AccessList != nil {
-		accessList = *args.AccessList
-	}
-
-	return &types.DynamicFeeTx{
-		Nonce:      0,
-		To:         args.To,
-		Value:      value,
-		Gas:        gasLimit,
-		Data:       data,
-		GasTipCap:  (*big.Int)(args.MaxPriorityFeePerGas),
-		GasFeeCap:  (*big.Int)(args.MaxFeePerGas),
-		AccessList: accessList,
+	return &types.LegacyTx{
+		Nonce:    0,
+		To:       args.To,
+		Value:    value,
+		Gas:      gasLimit,
+		GasPrice: big.NewInt(0),
+		Data:     data,
 	}, nil
 }

services/requester/requester.go

@@ -45,10 +45,6 @@ var (
 const minFlowBalance = 2
 const blockGasLimit = 120_000_000
 
-// estimateGasErrorRatio is the amount of overestimation eth_estimateGas
-// is allowed to produce in order to speed up calculations.
-const estimateGasErrorRatio = 0.015
-
 type Requester interface {
 	// SendRawTransaction will submit signed transaction data to the network.
 	// The submitted EVM transaction hash is returned.
@@ -62,7 +58,7 @@ type Requester interface {
 	// Note, this function doesn't make and changes in the state/blockchain and is
 	// useful to execute and retrieve values.
 	Call(
-		tx *types.DynamicFeeTx,
+		tx *types.LegacyTx,
 		from common.Address,
 		height uint64,
 		stateOverrides *ethTypes.StateOverride,
@@ -72,7 +68,7 @@ type Requester interface {
 	// Note, this function doesn't make any changes in the state/blockchain and is
 	// useful to executed and retrieve the gas consumption and possible failures.
 	EstimateGas(
-		tx *types.DynamicFeeTx,
+		tx *types.LegacyTx,
 		from common.Address,
 		height uint64,
 		stateOverrides *ethTypes.StateOverride,
@@ -328,7 +324,7 @@ func (e *EVM) GetStorageAt(
 }
 
 func (e *EVM) Call(
-	tx *types.DynamicFeeTx,
+	tx *types.LegacyTx,
 	from common.Address,
 	height uint64,
 	stateOverrides *ethTypes.StateOverride,
@@ -338,113 +334,42 @@ func (e *EVM) Call(
 		return nil, err
 	}
 
-	resultSummary := result.ResultSummary()
-	if resultSummary.ErrorCode != 0 {
-		if resultSummary.ErrorCode == evmTypes.ExecutionErrCodeExecutionReverted {
-			return nil, errs.NewRevertError(resultSummary.ReturnedData)
-		}
-		return nil, errs.NewFailedTransactionError(resultSummary.ErrorMessage)
-	}
-
-	return result.ReturnedData, nil
+	return result.ReturnedData, err
 }
 
 func (e *EVM) EstimateGas(
-	tx *types.DynamicFeeTx,
+	tx *types.LegacyTx,
 	from common.Address,
 	height uint64,
 	stateOverrides *ethTypes.StateOverride,
 ) (uint64, error) {
-	// Note: The following algorithm, is largely inspired from
-	// https://github.com/onflow/go-ethereum/blob/master/eth/gasestimator/gasestimator.go#L49-L192,
-	// and adapted to fit our use-case.
-	// Binary search the gas limit, as it may need to be higher than the amount used
-	var (
-		failingGasLimit uint64 // lowest-known gas limit where tx execution fails
-		passingGasLimit uint64 // lowest-known gas limit where tx execution succeeds
-	)
-	// Determine the highest gas limit that can be used during the estimation.
-	passingGasLimit = blockGasLimit
-	if tx.Gas >= gethParams.TxGas {
-		passingGasLimit = tx.Gas
-	}
-	tx.Gas = passingGasLimit
-	// We first execute the transaction at the highest allowable gas limit,
-	// since if this fails we can return error immediately.
 	result, err := e.dryRunTx(tx, from, height, stateOverrides)
 	if err != nil {
 		return 0, err
 	}
-	resultSummary := result.ResultSummary()
-	if resultSummary.ErrorCode != 0 {
-		if resultSummary.ErrorCode == evmTypes.ExecutionErrCodeExecutionReverted {
-			return 0, errs.NewRevertError(resultSummary.ReturnedData)
-		}
-		return 0, errs.NewFailedTransactionError(resultSummary.ErrorMessage)
-	}
-
-	// For almost any transaction, the gas consumed by the unconstrained execution
-	// above lower-bounds the gas limit required for it to succeed. One exception
-	// is those that explicitly check gas remaining in order to execute within a
-	// given limit, but we probably don't want to return the lowest possible gas
-	// limit for these cases anyway.
-	failingGasLimit = result.GasConsumed - 1
-
-	// There's a fairly high chance for the transaction to execute successfully
-	// with gasLimit set to the first execution's GasConsumed + GasRefund.
-	// Explicitly check that gas amount and use as a limit for the binary search.
-	optimisticGasLimit := (result.GasConsumed + result.GasRefund + gethParams.CallStipend) * 64 / 63
-	if optimisticGasLimit < passingGasLimit {
-		tx.Gas = optimisticGasLimit
-		result, err = e.dryRunTx(tx, from, height, stateOverrides)
-		if err != nil {
-			// This should not happen under normal conditions since if we make it this far the
-			// transaction had run without error at least once before.
-			return 0, err
-		}
-		resultSummary := result.ResultSummary()
-		if resultSummary.ErrorCode == evmTypes.ExecutionErrCodeOutOfGas {
-			failingGasLimit = optimisticGasLimit
-		} else {
-			passingGasLimit = optimisticGasLimit
-		}
-	}
 
-	// Binary search for the smallest gas limit that allows the tx to execute successfully.
-	for failingGasLimit+1 < passingGasLimit {
-		// It is a bit pointless to return a perfect estimation, as changing
-		// network conditions require the caller to bump it up anyway. Since
-		// wallets tend to use 20-25% bump, allowing a small approximation
-		// error is fine (as long as it's upwards).
-		if float64(passingGasLimit-failingGasLimit)/float64(passingGasLimit) < estimateGasErrorRatio {
-			break
-		}
-		mid := (passingGasLimit + failingGasLimit) / 2
-		if mid > failingGasLimit*2 {
-			// Most txs don't need much higher gas limit than their gas used, and most txs don't
-			// require near the full block limit of gas, so the selection of where to bisect the
-			// range here is skewed to favor the low side.
-			mid = failingGasLimit * 2
-		}
-		tx.Gas = mid
-		result, err = e.dryRunTx(tx, from, height, stateOverrides)
-		if err != nil {
-			return 0, err
-		}
-		resultSummary := result.ResultSummary()
-		if resultSummary.ErrorCode == evmTypes.ExecutionErrCodeOutOfGas {
-			failingGasLimit = mid
-		} else {
-			passingGasLimit = mid
-		}
-	}
+	if result.Successful() {
+		// As mentioned in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md#specification
+		// Define "all but one 64th" of N as N - floor(N / 64).
+		// If a call asks for more gas than the maximum allowed amount
+		// (i.e. the total amount of gas remaining in the parent after subtracting
+		// the gas cost of the call and memory expansion), do not return an OOG error;
+		// instead, if a call asks for more gas than all but one 64th of the maximum
+		// allowed amount, call with all but one 64th of the maximum allowed amount of
+		// gas (this is equivalent to a version of EIP-901 plus EIP-1142).
+		// CREATE only provides all but one 64th of the parent gas to the child call.
+		result.GasConsumed = AddOne64th(result.GasConsumed)
+
+		// Adding `gethParams.SstoreSentryGasEIP2200` is needed for this condition:
+		// https://github.com/onflow/go-ethereum/blob/master/core/vm/operations_acl.go#L29-L32
+		result.GasConsumed += gethParams.SstoreSentryGasEIP2200
 
-	if tx.AccessList != nil {
-		passingGasLimit += uint64(len(tx.AccessList)) * gethParams.TxAccessListAddressGas
-		passingGasLimit += uint64(tx.AccessList.StorageKeys()) * gethParams.TxAccessListStorageKeyGas
+		// Take into account any gas refunds, which are calculated only after
+		// transaction execution.
+		result.GasConsumed += result.GasRefund
 	}
 
-	return passingGasLimit, nil
+	return result.GasConsumed, err
 }
 
 func (e *EVM) GetCode(
@@ -536,7 +461,7 @@ func (e *EVM) evmToCadenceHeight(height uint64) (uint64, error) {
 }
 
 func (e *EVM) dryRunTx(
-	tx *types.DynamicFeeTx,
+	tx *types.LegacyTx,
 	from common.Address,
 	height uint64,
 	stateOverrides *ethTypes.StateOverride,
@@ -596,6 +521,14 @@ func (e *EVM) dryRunTx(
 		return nil, err
 	}
 
+	resultSummary := result.ResultSummary()
+	if resultSummary.ErrorCode != 0 {
+		if resultSummary.ErrorCode == evmTypes.ExecutionErrCodeExecutionReverted {
+			return nil, errs.NewRevertError(resultSummary.ReturnedData)
+		}
+		return nil, errs.NewFailedTransactionError(resultSummary.ErrorMessage)
+	}
+
 	return result, nil
 }
 

tests/web3js/build_evm_state_test.js

@@ -156,7 +156,7 @@ it('should handle a large number of EVM interactions', async () => {
         gas: 55_000,
         gasPrice: conf.minGasPrice
     }, 82n)
-    assert.equal(estimatedGas, 21358n)
+    assert.equal(estimatedGas, 23823n)
 
     estimatedGas = await web3.eth.estimateGas({
         from: conf.eoa.address,
@@ -165,7 +165,7 @@ it('should handle a large number of EVM interactions', async () => {
         gas: 55_000,
         gasPrice: conf.minGasPrice
     }, latest)
-    assert.equal(estimatedGas, 26811n)
+    assert.equal(estimatedGas, 29292n)
 
     // Add calls to verify correctness of eth_getCode on historical heights
     let code = await web3.eth.getCode(contractAddress, 82n)

tests/web3js/debug_traces_test.js

@@ -35,7 +35,7 @@ it('should retrieve transaction traces', async () => {
     // Assert proper response for `callTracer`
     let txTrace = response.body.result
     assert.equal(txTrace.from, '0xfacf71692421039876a5bb4f10ef7a439d8ef61e')
-    assert.equal(txTrace.gas, '0x1167ac')
+    assert.equal(txTrace.gas, '0x118e0c')
     assert.equal(txTrace.gasUsed, '0x114010')
     assert.equal(txTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab')
     assert.lengthOf(txTrace.input, 9856n)
@@ -92,7 +92,7 @@ it('should retrieve transaction traces', async () => {
     // Assert proper response for `callTracer`
     txTrace = response.body.result
     assert.equal(txTrace.from, '0xfacf71692421039876a5bb4f10ef7a439d8ef61e')
-    assert.equal(txTrace.gas, '0x697f')
+    assert.equal(txTrace.gas, '0x72c3')
     assert.equal(txTrace.gasUsed, '0x6827')
     assert.equal(txTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab')
     assert.equal(
@@ -161,10 +161,10 @@ it('should retrieve transaction traces', async () => {
         txTraces,
         [
             {
-                txHash: '0xc34f49f9c6b56ebd88095054e2ad42d6854ba818a9657caf3f8500161a5e4ef7',
+                txHash: '0x87449feedc004c75c0e8b12d01656f2e28366c7d73b1b5336beae20aaa5033dd',
                 result: {
                     from: '0xfacf71692421039876a5bb4f10ef7a439d8ef61e',
-                    gas: '0x697f',
+                    gas: '0x72c3',
                     gasUsed: '0x6827',
                     to: '0x99a64c993965f8d69f985b5171bc20065cc32fab',
                     input: '0x6057361d0000000000000000000000000000000000000000000000000000000000000064',
@@ -200,10 +200,10 @@ it('should retrieve transaction traces', async () => {
         txTraces,
         [
             {
-                txHash: '0xc34f49f9c6b56ebd88095054e2ad42d6854ba818a9657caf3f8500161a5e4ef7',
+                txHash: '0x87449feedc004c75c0e8b12d01656f2e28366c7d73b1b5336beae20aaa5033dd',
                 result: {
                     from: '0xfacf71692421039876a5bb4f10ef7a439d8ef61e',
-                    gas: '0x697f',
+                    gas: '0x72c3',
                     gasUsed: '0x6827',
                     to: '0x99a64c993965f8d69f985b5171bc20065cc32fab',
                     input: '0x6057361d0000000000000000000000000000000000000000000000000000000000000064',
@@ -257,15 +257,15 @@ it('should retrieve transaction traces', async () => {
         txTrace,
         {
             from: conf.eoa.address.toLowerCase(),
-            gas: '0xbf57',
+            gas: '0xc9c7',
             gasUsed: '0x6147',
             to: contractAddress.toLowerCase(),
             input: '0xc550f90f',
             output: '0x0000000000000000000000000000000000000000000000000000000000000006',
             calls: [
                 {
                     from: contractAddress.toLowerCase(),
-                    gas: '0x5f01',
+                    gas: '0x6948',
                     gasUsed: '0x2',
                     to: '0x0000000000000000000000010000000000000001',
                     input: '0x53e87d66',

tests/web3js/eth_deploy_contract_and_interact_test.js

@@ -221,7 +221,7 @@ it('deploy contract and interact', async () => {
         },
         '0x1'
     )
-    assert.equal(gasEstimate, 21510n)
+    assert.equal(gasEstimate, 23977n)
 
     gasEstimate = await web3.eth.estimateGas(
         {
@@ -233,7 +233,7 @@ it('deploy contract and interact', async () => {
         },
         'latest'
     )
-    assert.equal(gasEstimate, 25052n)
+    assert.equal(gasEstimate, 27398n)
 
     // check that `eth_call` can handle state overrides
     let stateOverrides = {
@@ -274,7 +274,7 @@ it('deploy contract and interact', async () => {
     assert.isDefined(response.body)
 
     result = response.body.result
-    assert.equal(result, '0x697f')
+    assert.equal(result, '0x72c3')
 
     stateOverrides = {
         [contractAddress]: {
@@ -295,5 +295,5 @@ it('deploy contract and interact', async () => {
     // setting a storage slot from a zero-value, to a non-zero value has an
     // increase of about 20,000 gas. Which is quite different to `0x72c3`.
     result = response.body.result
-    assert.equal(result, '0xac6d')
+    assert.equal(result, '0xb69a')
 })