Rebase Merge spec with London #2533
Conversation
specs/merge/beacon-chain.md
Outdated
| gas_used_delta = parent_gas_target - parent_gas_used | ||
| base_fee_per_gas_delta = \ | ||
| parent_base_fee_per_gas * gas_used_delta // parent_gas_target // BASE_FEE_MAX_CHANGE_DENOMINATOR | ||
| return max(parent_base_fee_per_gas - base_fee_per_gas_delta, 0) |
There was a problem hiding this comment.
underflow protection
I don't see the EIP-1559 code have such underflow protection so I checked the math. I think it is not required:
Since gas_used_delta = parent_gas_target - parent_gas_used
→ parent_gas_target >= gas_used_delta ---- (1)
Now look at expected_base_fee_per_gas:
expected_base_fee_per_gas
= parent_base_fee_per_gas - base_fee_per_gas_delta
= parent_base_fee_per_gas - (parent_base_fee_per_gas * gas_used_delta // parent_gas_target // BASE_FEE_MAX_CHANGE_DENOMINATOR) ---- (2)
Apply (1) to (2), we can get parent_base_fee_per_gas - base_fee_per_gas_delta >= 0.
Or more precisely, expected_base_fee_per_gas is greater than or equal to ~parent_base_fee_per_gas * 7/8.
But both Geth and Nethermind do have such underflow protection.
- Geth: https://github.com/ethereum/go-ethereum/blob/523866c2ccbfd75ee88b309167cdba7311ba9695/consensus/misc/eip1559.go#L88-L91
- Nethermind: https://github.com/NethermindEth/nethermind/blob/6512c685a21ef1c08b34ddefb2e9075575d0656c/src/Nethermind/Nethermind.Core/BaseFeeCalculator.cs#L55
For eth2 spec, I'm not sure...
- To make the spec clear -> maybe remove the unnecessary protection
- To align with Eth1 and minimize risks -> maybe keep it
Any thoughts?
There was a problem hiding this comment.
Good question. I'd remove max with a corresponding comment that it never underflows. It would make sense to leave this sanity check would this arithmetics be a subject to change to avoid potential underflow that could appear in the future. Even if BASE_FEE_MAX_CHANGE_DENOMINATOR = 1 (which should never be the case) we got 0 in the "worst" case and never a < 0 value.
There was a problem hiding this comment.
The check is removed and the comment is added
There was a problem hiding this comment.
I just realized that the unnecessary max was removed 2 days ago: ethereum/EIPs#3640 😂
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
There was a problem hiding this comment.
nice!
We need to revive the conversation about if/how empty slots should affect base fee
options:
- you only consider the capacity and usage of the chain created (similar to the sequential block-by-block alg used on pow mainnet)
- consider a skip slot as an empty payload and thus adjusting basefee downward to account for what was "unused" capacity
We can revive this convo on discord and/or the next merge call
Good point! If we decide that an empty slot is treated as an empty execution payload which it certainly is up to some extent, then how do you think |
Option 3 would be to do nothing (take option 1) at the merge and consider basefee computation re-adjustment in the fork after the merge. |
|
I made an issue to track the base_fee / empty-slot convo |
The following changes are introduced by this PR:
base_fee_per_gastoExecutionPayloadandExecutionPayloadHeaderbase_fee_per_gasverification toprocess_execution_payloadfunctiongas_limitverification toprocess_execution_payloadfunctionThe source of above changes is https://eips.ethereum.org/EIPS/eip-1559 and its code snippets.