-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Eip1559 updates #2505
Eip1559 updates #2505
Conversation
EIPS/eip-1559.md
Outdated
* `TARGET_GASUSED`: 8,000,000 | ||
* `TARGET_GAS_USED`: 10,000,000 | ||
* `MAX_GAS_EIP1559`: 16,000,000 | ||
* `FINAL_FORK_BLKNUM`: `INITIAL_FORK_BLKNUM + (MAX_GAS_EIP1559 / (10 * SLACK_COEFFICIENT))` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slack coefficient was removed, but is still used here.
Also, why a slack coefficient of only 1.6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gogo I want to see ethereum at #1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this comment when it was made.
I've removed any mention of the slack coefficient from the doc, as it had already been removed from the implementation.
The slack coefficient was never being used for anything other than as the scalar that described the ratio between max gas used and target gas used. It was 2 when target gas used was 8m and max gas used was 16m, but after switching to a target gas used of 10m it became 1.6.
EIPS/eip-1559.md
Outdated
* At `block.number == INITIAL_FORK_BLKNUM`, let `GASLIMIT = (MAX_GAS_EIP1559 / 2)` so that the gas maximum is split evenly between the legacy and EIP1559 gas pools | ||
* As `block.number` increases towards `FINAL_FORK_BLKNUM`, at every block we shift `EIP1559_GAS_INCREMENT_AMOUNT` from the legacy pool into the EIP1559 gas pool | ||
* At `block.number >= FINAL_FORK_BLKNUM` the entire `MAX_GAS_EIP1559` is assigned to the EIP1559 gas pool and the legacy pool is empty | ||
* We enforce a per-transaction gas limit of 8,000,000.`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8,000,000 should be a parameter, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks! It is a parameter in the implementation, but I didn't reflect that here. Will update. Although it looks like there is some more discussion about what the per-transaction gas limit should in the magicians thread: https://ethereum-magicians.org/t/eip-1559-go-ethereum-implementation/3918/15?u=i-norden
* As a default strategy, miners set `BASEFEE` as follows. | ||
* Let `delta = block.gas_used - TARGET_GASUSED` (possibly negative). | ||
* Set `BASEFEE = PARENT_BASEFEE + PARENT_BASEFEE * delta // TARGET_GASUSED // BASEFEE_MAX_CHANGE_DENOMINATOR` | ||
* Clamp the resulting `BASEFEE` inside of the allowable bounds if needed, where a valid `BASEFEE` is one such that `abs(BASEFEE - PARENT_BASEFEE) <= max(1, PARENT_BASEFEE // BASEFEE_MAX_CHANGE_DENOMINATOR)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we letting miners adjust the basefee? I thought we are forcing the specific change in the protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had the time to thoroughly model the BASEFEE function, but in my limited experimentation there were scenarios where the specific change enforced by the protocol would result in a BASEFEE that falls out of the bounds described above. In that case the result is clamped down to those bounds.
I will update all the references to "miners adjusting/setting BASEFEE" to reflect it being the protocol. I think this is a bit of semantic misunderstanding on my part, in my eyes the miners are still "setting" the BASEFEE in the blocks they mine, even if they have no control over what that set value is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to avoid simplicity reword this as:
- Let
gas_delta = block.gas_used - TARGET_GASUSED
(possibly negative). - Let
basefee_delta = PARENT_BASEFEE * gas_delta // TARGET_GASUSED // BASEFEE_MAX_CHANGE_DENOMINATOR
- Let
max_basefee_delta = max(1, PARENT_BASEFEE // BASEFEE_MAX_CHANGE_DENOMINATOR)
- Set
BASEFEE = PARENT_BASEFEE + max(-max_basefee_delta, min(max_basefee_delta, basefee_delta))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's also important to specify cleanly what we mean by // in the negative case. Is -7 // 5
equal to -1
or -2
? In python it's the latter, in some languages it's the former. Perhaps avoid negative numbers entirely?
- Let
max_basefee_delta = max(1, PARENT_BASEFEE // BASEFEE_MAX_CHANGE_DENOMINATOR)
- If
block.gas_used >= TARGET_GASUSED
:- Let
gas_delta = block.gas_used - TARGET_GASUSED
- Let
basefee_delta = PARENT_BASEFEE * gas_delta // TARGET_GASUSED // BASEFEE_MAX_CHANGE_DENOMINATOR
- Set
BASEFEE = PARENT_BASEFEE + min(max_basefee_delta, basefee_delta)
- Let
block.gas_used < TARGET_GASUSED
:- Let
gas_delta = TARGET_GASUSED - block.gas_used
- Let
basefee_delta = PARENT_BASEFEE * gas_delta // TARGET_GASUSED // BASEFEE_MAX_CHANGE_DENOMINATOR
- Set
BASEFEE = max(0, PARENT_BASEFEE - min(max_basefee_delta, basefee_delta))
- Let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point, we are using big.Int
division which is true Euclidean division where the remainder is bound above 0 so we round to negative infinity and would get -2
. We can make that the specification, or avoid negative numbers entirely. What do you think is best?
EIPS/eip-1559.md
Outdated
* A "cap" which represents the maximum total that the transaction sender would be willing to pay to get included. | ||
* A fee cap which represents the maximum total (base fee + gas premium) that the transaction sender would be willing to pay to get their transaction included. | ||
|
||
The current miner-voting based gas limit is changed to a hard-coded gas limit of 16 million. Instead of miners directly adjusting the gas limit in response to changes in network demand they adjust the base fee to apply economic pressure towards a target gas usage of 10 million. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads like miners still have control of the parameter. If I'm right in thinking they don't it would be clearer to say "the protocol adjusts the gas limit" or "the gas limit is adjusted".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, thanks for pointing this out! I've updated it to hopefully be more clear.
Just wanted to pop in and say great work! |
Hi, I'm a bot! This change was automatically merged because: - It only modifies existing Draft or Last Call EIP(s) - The PR was approved or written by at least one author of each modified EIP - The build is passing
Hi, I'm a bot! This change was automatically merged because: - It only modifies existing Draft or Last Call EIP(s) - The PR was approved or written by at least one author of each modified EIP - The build is passing
Hi, I'm a bot! This change was automatically merged because: - It only modifies existing Draft or Last Call EIP(s) - The PR was approved or written by at least one author of each modified EIP - The build is passing
I've updated the EIP1559 doc to reflect the implementation found here or here if you want to see the tests passing. The PR for the implementation is now open here.
The magicians thread for this can be found here. Please let me know if there is any additional feedback or updates to incorporate!
Thanks!