EIP1559 strict balance validation #857
Conversation
frame/evm/src/runner/stack.rs
Outdated
| // TODO: after simplifying the logic here, we no longer care about the specified priority | ||
| // fee except for calculating the total fee paid. | ||
| // When paying out the priority fee, we let correct_and_deposit_fee() come up with an | ||
| // arbitrary value that is used as the tip to pay. | ||
| // Assuming this is ok, actual_priority_fee_per_gas does not need to be returned above. |
There was a problem hiding this comment.
I see three possibilities:
- do nothing (remove
actual_priority_fee_per_gasand this comment) - don't let
correct_and_deposit_fee()return (or even know about...?) priority fee - leave it as is but use
actual_priority_fee_per_gasfor sanity checks
There was a problem hiding this comment.
1 or 3 would be appropriate for this PR, 2 would make sense as a follow-up.
There was a problem hiding this comment.
I'd slightly prefer option 3. This is only executed once per transaction so the performance impact is minimum.
There was a problem hiding this comment.
After another review, this was really always the case. Previously, we only calculated priority fee as an incremental step to get to total fee, and that's no longer necessary in this PR.
It's correct_and_deposit_fee() which is responsible for coming up with the tip to be paid.
We could add a sanity check that correct_and_deposit_fee() returns something equal to (...or less than?) the correct priority_fee, but that would require additional trait bounds for LiquidityInfo to at least compare it.
There was a problem hiding this comment.
I went with 1 for now. If 3 is worth the extra trait bounds in your opinion, I'm happy to do it.
There was a problem hiding this comment.
Yeah that's okay. Please resolve the conflicts.
|
@sorpaas Conflicts resolved 👍 |
* Change test assumptions * Validate account balance against max_fee_per_gas * Deduct base_fee + tip rather than max_fee_per_gas * fmt * Leave actual_priority_fee_per_gas unused * Remove TODO comment
* Change test assumptions * Validate account balance against max_fee_per_gas * Deduct base_fee + tip rather than max_fee_per_gas * fmt * Leave actual_priority_fee_per_gas unused * Remove TODO comment
This PR clears up the accounting and validation related to
max_fee_per_gasandbase_feein order to match the EIP1559 spec.Essentially this PR swaps the checks done by validation and during execution.
Before:
base_fee(plus applicable tip)max_fee_per_gasAfter this PR:
max_fee_per_gasbase_fee(plus applicable tip)TODO:
max_fee_per_gas < base_feecan't be included -- shouldexecute()check this or should it assume that this is properly checked during validation?