celo-org / op-geth

GNU Lesser General Public License v3.0
0 stars 0 forks source link

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

Open ezdac opened 1 month ago

ezdac commented 1 month ago

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: https://github.com/celo-org/op-geth/blob/4eb43bcaa65c927b6be39efcaebb0e323852e624/eth/gasestimator/gasestimator.go#L61-L63 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: https://github.com/celo-org/op-geth/blob/4eb43bcaa65c927b6be39efcaebb0e323852e624/eth/gasestimator/gasestimator.go#L129-L140

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.