Skip to content

Commit

Permalink
Merge pull request #690 from onflow/mpeter/fix-gas-estimation-logic
Browse files Browse the repository at this point in the history 
Fix logical error in `eth_estimateGas` endpoint
  • Loading branch information
@m-Peter
m-Peter authored Dec 3, 2024
2 parents 73ca87e + 0e107bf commit 4b32ea8
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 55 deletions.
23 changes: 15 additions & 8 deletions api/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.LegacyTx, error) {
func encodeTxFromArgs(args ethTypes.TransactionArgs) (*types.DynamicFeeTx, error) {
var data []byte
if args.Data != nil {
data = *args.Data
Expand All @@ -156,12 +156,19 @@ func encodeTxFromArgs(args ethTypes.TransactionArgs) (*types.LegacyTx, error) {
value = args.Value.ToInt()
}

return &types.LegacyTx{
Nonce: 0,
To: args.To,
Value: value,
Gas: gasLimit,
GasPrice: big.NewInt(0),
Data: data,
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,
}, nil
}
131 changes: 98 additions & 33 deletions services/requester/requester.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ 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.
Expand All @@ -58,7 +62,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.LegacyTx,
tx *types.DynamicFeeTx,
from common.Address,
height uint64,
stateOverrides *ethTypes.StateOverride,
Expand All @@ -68,7 +72,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.LegacyTx,
tx *types.DynamicFeeTx,
from common.Address,
height uint64,
stateOverrides *ethTypes.StateOverride,
Expand Down Expand Up @@ -324,7 +328,7 @@ func (e *EVM) GetStorageAt(
}

func (e *EVM) Call(
tx *types.LegacyTx,
tx *types.DynamicFeeTx,
from common.Address,
height uint64,
stateOverrides *ethTypes.StateOverride,
Expand All @@ -334,42 +338,111 @@ func (e *EVM) Call(
return nil, err
}

return result.ReturnedData, 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
}

func (e *EVM) EstimateGas(
tx *types.LegacyTx,
tx *types.DynamicFeeTx,
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 the 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
}
if result.Failed() {
failingGasLimit = optimisticGasLimit
} else {
passingGasLimit = optimisticGasLimit
}
}

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
// 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
}
if result.Failed() {
failingGasLimit = mid
} else {
passingGasLimit = mid
}
}

// Take into account any gas refunds, which are calculated only after
// transaction execution.
result.GasConsumed += result.GasRefund
if tx.AccessList != nil {
passingGasLimit += uint64(len(tx.AccessList)) * gethParams.TxAccessListAddressGas
passingGasLimit += uint64(tx.AccessList.StorageKeys()) * gethParams.TxAccessListStorageKeyGas
}

return result.GasConsumed, err
return passingGasLimit, nil
}

func (e *EVM) GetCode(
Expand Down Expand Up @@ -461,7 +534,7 @@ func (e *EVM) evmToCadenceHeight(height uint64) (uint64, error) {
}

func (e *EVM) dryRunTx(
tx *types.LegacyTx,
tx *types.DynamicFeeTx,
from common.Address,
height uint64,
stateOverrides *ethTypes.StateOverride,
Expand Down Expand Up @@ -521,14 +594,6 @@ 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
}

Expand Down
4 changes: 2 additions & 2 deletions tests/web3js/build_evm_state_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ it('should handle a large number of EVM interactions', async () => {
gas: 55_000,
gasPrice: conf.minGasPrice
}, 82n)
assert.equal(estimatedGas, 23823n)
assert.equal(estimatedGas, 21358n)

estimatedGas = await web3.eth.estimateGas({
from: conf.eoa.address,
Expand All @@ -165,7 +165,7 @@ it('should handle a large number of EVM interactions', async () => {
gas: 55_000,
gasPrice: conf.minGasPrice
}, latest)
assert.equal(estimatedGas, 29292n)
assert.equal(estimatedGas, 26811n)

// Add calls to verify correctness of eth_getCode on historical heights
let code = await web3.eth.getCode(contractAddress, 82n)
Expand Down
16 changes: 8 additions & 8 deletions tests/web3js/debug_traces_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, '0x118e0c')
assert.equal(txTrace.gas, '0x1167ac')
assert.equal(txTrace.gasUsed, '0x114010')
assert.equal(txTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab')
assert.lengthOf(txTrace.input, 9856n)
Expand Down Expand Up @@ -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, '0x72c3')
assert.equal(txTrace.gas, '0x697f')
assert.equal(txTrace.gasUsed, '0x6827')
assert.equal(txTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab')
assert.equal(
Expand Down Expand Up @@ -161,10 +161,10 @@ it('should retrieve transaction traces', async () => {
txTraces,
[
{
txHash: '0x87449feedc004c75c0e8b12d01656f2e28366c7d73b1b5336beae20aaa5033dd',
txHash: '0xc34f49f9c6b56ebd88095054e2ad42d6854ba818a9657caf3f8500161a5e4ef7',
result: {
from: '0xfacf71692421039876a5bb4f10ef7a439d8ef61e',
gas: '0x72c3',
gas: '0x697f',
gasUsed: '0x6827',
to: '0x99a64c993965f8d69f985b5171bc20065cc32fab',
input: '0x6057361d0000000000000000000000000000000000000000000000000000000000000064',
Expand Down Expand Up @@ -200,10 +200,10 @@ it('should retrieve transaction traces', async () => {
txTraces,
[
{
txHash: '0x87449feedc004c75c0e8b12d01656f2e28366c7d73b1b5336beae20aaa5033dd',
txHash: '0xc34f49f9c6b56ebd88095054e2ad42d6854ba818a9657caf3f8500161a5e4ef7',
result: {
from: '0xfacf71692421039876a5bb4f10ef7a439d8ef61e',
gas: '0x72c3',
gas: '0x697f',
gasUsed: '0x6827',
to: '0x99a64c993965f8d69f985b5171bc20065cc32fab',
input: '0x6057361d0000000000000000000000000000000000000000000000000000000000000064',
Expand Down Expand Up @@ -257,15 +257,15 @@ it('should retrieve transaction traces', async () => {
txTrace,
{
from: conf.eoa.address.toLowerCase(),
gas: '0xc9c7',
gas: '0xbf57',
gasUsed: '0x6147',
to: contractAddress.toLowerCase(),
input: '0xc550f90f',
output: '0x0000000000000000000000000000000000000000000000000000000000000006',
calls: [
{
from: contractAddress.toLowerCase(),
gas: '0x6948',
gas: '0x5f01',
gasUsed: '0x2',
to: '0x0000000000000000000000010000000000000001',
input: '0x53e87d66',
Expand Down
8 changes: 4 additions & 4 deletions tests/web3js/eth_deploy_contract_and_interact_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ it('deploy contract and interact', async () => {
},
'0x1'
)
assert.equal(gasEstimate, 23977n)
assert.equal(gasEstimate, 21510n)

gasEstimate = await web3.eth.estimateGas(
{
Expand All @@ -233,7 +233,7 @@ it('deploy contract and interact', async () => {
},
'latest'
)
assert.equal(gasEstimate, 27398n)
assert.equal(gasEstimate, 25052n)

// check that `eth_call` can handle state overrides
let stateOverrides = {
Expand Down Expand Up @@ -274,7 +274,7 @@ it('deploy contract and interact', async () => {
assert.isDefined(response.body)

result = response.body.result
assert.equal(result, '0x72c3')
assert.equal(result, '0x697f')

stateOverrides = {
[contractAddress]: {
Expand All @@ -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, '0xb69a')
assert.equal(result, '0xac6d')
})

0 comments on commit 4b32ea8

Please sign in to comment.