-
Notifications
You must be signed in to change notification settings - Fork 198
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
Skip debit/credit during gas estimation #2258
Conversation
6f5afb1
to
f5d5b5f
Compare
|
Coverage from tests in coverage: 50.6% of statements across all listed packagescoverage: 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 |
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? |
f5d5b5f
to
bf2c8d0
Compare
bf2c8d0
to
0c9b5d8
Compare
0c9b5d8
to
294599c
Compare
294599c
to
4798934
Compare
I expanded the PR's description, let me know if that makes sense now. |
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. |
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 #825 and #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 todebit
andcredit
, which further relaxes the reliance on the token implementation.