Fix gas estimations

[emlautarom1]

Fixes #5706

Changes

• Change Gas estimation algorithm for more consistent results

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Some tests were updated to reflect the behavior changes.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

Remarks

The code for Gas estimation is still very complex compared to the competition (see go-ethereum implementation: https://github.com/ethereum/go-ethereum/blob/2274a03e339f213361453590b54917bbfd0a0c31/internal/ethapi/api.go#L1137). A refactor would be highly suggested.

[emlautarom1]

This PR supersedes #5968. It seems that I introduced some error that I could not figure out how to fix, so I created a new branch with the same changes. Now, all test pass successfully without need for change.

I'll migrate the comments from the original PR to this one to facilitate work for future maintainers.

[emlautarom1]

About gas estimation:

When a user triggers a eth_estimateGas, the payload can include an optional gas field to limit the amount of gas to be used in the tx, that is, the estimation will be at most gas. If no gas value is provided, an arbitrary default of 100_000_000 is used at the moment by Nethermind.

Here is (or was) the buggy code: If the gas value is higher than what block gas limit where the tx will be executed, an alternative gas estimation is used. This alternative estimation is not clear to me, nor are the reasons for it to exist. Since the default value is 100_000_000, on mainnet which has a block gas limit of 30_000_000, this alternative estimation is used. In practice this method is the most used given that users rarely include a gas field when asking for estimations.

This PR limits the tx gas value to be at most the block gas limit, and then uses the "default" gas estimation method, which involves a binary search to find the required amount of gas until no OutOfGas exception is triggered.

[emlautarom1]

A different problem regarding gas estimations comes when a contract performs arbitrary checks on the remaining gas, like in #5637. Here, the contract used makes the following check:

require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction");

If the remaining gas doesn't satisfy the condition, execution halts and all changes get reverted, but importantly, this does not fail with an OutOfGas exception. Remember that Nethermind tries to find the minimum amount of gas required for the transaction to not fail with an OutOfGas exception, not the minimum amount of gas for the transaction to be completed successfully.

Here, there are as far as I can tell two options:

• Change the estimation method to find the minimum amount of gas required for the tx to go through.
• Change the contract to fail with an OutOfGas exception, somehow.

[emlautarom1]

Note: Geth uses the gas value provided by the user as long as it's at least the minimum amount of gas required by any Tx (21_000), or the block gas limit by default (so in this case, 30_000_000).

[rmeissner]

It seems it was opted to go for Change the contract to fail with an OutOfGas exception, somehow. which is not optimal in my opinion. With this nethermind behaves differently than other clients (the Safe team now has to add special cases just for chains that use nethermind as they return different estimation results ... Currently this will make us prefer chains with geth or erigon support).

The suggestion to change the contract is also weird, as it would mean changing the require to an assert. This is a change with a high impact as the gas costs for wrongly estimated txs increases.

It would be nice to have more insights why it was chosen to not "fix" the nethermind client to behave the same way as other clients.

Edit: the 4337 entrypoint has similar logic. Not sure if it was tested if this causes estimation issues.

[emlautarom1]

It would be nice to have more insights why it was chosen to not "fix" the nethermind client to behave the same way as other clients.

The explanation on why we decided to not spend more time with "safe transactions" is that the error when executing a contract like this is not an "OutOfGas" error, but a require error. Requires can perform arbitrary checks on the gas left, making it impossible to ensure that the gas estimations are always correct in the general case.

For a technical explanation, Nethermind (like Geth) uses a binary search to determine the optimal amount of gas to use in a transaction, where the lower bound is the minimum amount of gas per transaction, and the upper bound a value provided by the user, or the next block gas limit. Let's assume that the lower bound is 10, and upper bound is 90 to make the math simpler. On a first try, Nethermind will try to run the Tx with 50 gas (90+10 / 2), and if it fails, it will try with 50 + 90 / 2 = 70, and so forth. If a contract had a require that stated that the gas should be 40, Nethermind will never find a gas value that makes this transaction go through (it will try higher and higher until reaching 90, which still fails).

Is important to note that require checks are not ou
For a technical explanation, Nethermind (like Geth) uses a binary search to determine the optimal amount of gas to use in a transaction, where the lower bound is the minimum amount of gas per transaction, and the upper bound a value provided by the user, or the next block gas limit. Let's assume that the lower bound is 10, and upper bound is 90 to make the math simpler. On a first try, Nethermind will try to run the Tx with 50 gas (90+10 / 2), and if it fails, it will try with 50 + 90 / 2 = 70, and so forth. If a contract had a require that stated that the gas should be 40, Nethermind will never find a gas value that makes this transaction go through (it will try higher and higher until reaching 90, which still fails).

Another small example would be a contract that does the following:

require(gasleft() >= y, "Too little gas");
require(gasleft() < y, "Too much gas");

This contract will never be executed, so what should the gas estimation be here?

Is important to note that require checks are not out-of-gas errors, but contract specific, user defined errors. The fact that safe transactions uses that for validating remaining gas is just a particular case that cannot be generalized.

Our options are either:

• Try all possible gas estimations, in hope that the contract goes through for some particular value. This is not reasonable since it would make estimations painfully slow.
• Change the semantics of eth_estimateGas to to find the value that makes the transaction "go through": should eth_estimateGas provide the minimum gas value for a Tx to not fail with an out-of-gas error, or should it provide the minimum gas value for a Tx to be successfully executed?. The later results in more questions: what if the contract will never be fully executed (like in the previous example), what should be the estimation then?

We decided to not change the semantics of the call, neither we want to make gas estimations take an unacceptable amount of time. I hope I could provide enough details on why we made this decision.

[rmeissner]

I get your reasoning, but we see this error exclusively with Nethermind. Geth, Erigon and the old OpenEthereum clients provide correct estimates. This means we have to add special handling for Nethermind nodes.

So my question is why does it work for the other nodes, but not for Nethermind. We would love to understand this as we evaluated Nethermind to be one of the nodes we request from network providers for tracing purposes, but if the estimation doesn't work then this would be a blocker for us.

This contract will never be executed, so what should the gas estimation be here?

An error should be returned that this transaction cannot be estimated (as it is done by other clients too).

executing a contract like this is not an "OutOfGas" error

This is exactly the difference. Geth looks for a value where it doesn't get an error (not matter if a revert error or any else). Looking at the estimate function of Geth we can see a executable function. This function uses doCall which is the method Geth uses to handle eth_call. eth_call returns true for Failed if there is any error (so a require will trigger this the same way as an out of gas).

Edit:
Also as specified this is not exclusive to Safe transaction but can pose issues with any smart contract based account and 4337 related wallet, which makes it harder to use Nethermind with those for AA.