Skip debit/credit during gas estimation

[palango]

Description

This PR fixes an issue with gas estimation for fee currencies which revert when sending tokens to the zero address.

The blockchain client doesn't have control over the fee currency token contracts and thus has to try to handle possible restrictions as well as possible.

Before this PR we set the gas price to zero during gas estimation (for history see https://github.com/celo-org/celo-blockchain/pull/825 and https://github.com/celo-org/celo-blockchain/pull/1251). This works, but omits the gas where EIP1559 style fields are set. Currently they overwrite the gas price to be non-zero again. This led to problems when the token has implemented an early return on debit/credit where the amount is zero. This is fixed by the first commit and included for completeness.

The second commit fixes the issue at hand more generally. As credit/debit costs are (currently) handled by an increased intrinsic gas, there's no need to run them during gas estimation. In the second commit new EVM flag SkipDebitCredit is added. If set it skips calls to debit and credit, which further relaxes the reliance on the token implementation.

[github-actions[bot]]

Test failures:
TestUpdatedKeyfileContents: keystore account_cache_test.go:393: Emptying account file failed account_cache_test.go:394: wasn't notified of new accounts
This test report was produced by the test-summary action.  Made with ❤️ in Cambridge.

[github-actions[bot]]

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 6f0c8d605d77e4de43d45456b6fbb3d0af8d52f8

coverage: 50.6% of statements across all listed packages

coverage:  63.2% of statements in consensus/istanbul
coverage:  42.7% of statements in consensus/istanbul/announce
coverage:  55.7% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  64.4% of statements in consensus/istanbul/core
coverage:  50.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

[piersy]

Hey @palango, could you expand upon the use cases for this, is there an associated ticket? It just seems like a bit of a random change, and I'm wondering why do it, who is it helping?

[palango]

Hey @palango, could you expand upon the use cases for this, is there an associated ticket? It just seems like a bit of a random change, and I'm wondering why do it, who is it helping?

I expanded the PR's description, let me know if that makes sense now.

[piersy]

Hi @palango thanks for expanding the description, makes a lot more sense now. However I still don't see why we need the flag, it sounds like the first commit solves the problem at hand so why add a flag to skip credit/debit?

[palango]

Hi @palango thanks for expanding the description, makes a lot more sense now. However I still don't see why we need the flag, it sounds like the first commit solves the problem at hand so why add a flag to skip credit/debit?

It fixes the problem for the tokens we're aware of, with the second it fixes the problem for all possible implementations.

Imagine a token that reverts on credit when a receiver is the zero address, which might happen for example when the coinbase is not set for the node doing the gas estimation. If the token implementation early returns (like here), then the fix commit is sufficient. But if that's not the case the revert would still happen. So in general, without further knowledge of the token, we just skip calling debit/credit.