Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider different intrinsic-gas for fee-currencies in gas-estimation #246

Open
ezdac opened this issue Oct 2, 2024 · 0 comments
Open
Labels
type:bug Something isn't working

Comments

@ezdac
Copy link

ezdac commented Oct 2, 2024

Problem

We don't consider the different intrinsic-gas costs for fee-currencies (FeeCurrencyDirectory determined) compared to a native transaction (21000) within the gas-estimation that is accessible via the eth_estimateGas RPC method.

Details

Issue 1

If a user would supply a gas parameter to the estimated transaction that is higher than 21000 but lower than the intrinsic gas for that fee-currency, the gas estimation would fail:

if call.GasLimit >= params.TxGas {
hi = call.GasLimit
}

This is because the EVM-call with a gas of the hi value is set as the upper bound of the binary search.

(Note that this is unlikely that clients do that, since the point of gas-estimation is to determine the gas value of a transaction.
However this parameter can be used to set the upper bound of the binary search)

Issue 2

Additionally, not considering the different intrinsic gas for fee-currency transactions will not work with the value-only transaction short-circuit:

// If the transaction is a plain value transfer, short circuit estimation and
// directly try 21000. Returning 21000 without any execution is dangerous as
// some tx field combos might bump the price up even for plain transfers (e.g.
// unused access list items). Ever so slightly wasteful, but safer overall.
if len(call.Data) == 0 {
if call.To != nil && opts.State.GetCodeSize(*call.To) == 0 {
failed, _, err := execute(ctx, call, opts, params.TxGas)
if !failed && err == nil {
return params.TxGas, nil, nil
}
}
}

The gas estimation will still work as expected, but the node does more computation than necessary.

Proposed solution

In both mentioned cases, for fee-currency transactions the params.TxGas value should be substituted with the value of the fee-currencies intrinsic gas. This value should be queried from the FeeCurrencyDirectory chain-state prior. This is somewhat unfortunate because at that point there is no EVM-context initialized yet.

@ezdac ezdac added the type:bug Something isn't working label Oct 2, 2024
@lvpeschke lvpeschke added this to the 5 - Celo MVP Mainnet milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants