Skip to content
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

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Eip1559 updates #2505

merged 5 commits into from
Mar 12, 2020

Conversation

i-norden
Copy link
Contributor

@i-norden i-norden commented Feb 3, 2020

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!

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))`
Copy link
Contributor

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?

Copy link

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

Copy link
Contributor Author

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.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

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.``
Copy link
Contributor

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?

Copy link
Contributor Author

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)`
Copy link
Contributor

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?

Copy link
Contributor Author

@i-norden i-norden Feb 4, 2020

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.

Copy link
Contributor

@vbuterin vbuterin Mar 24, 2020

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))

Copy link
Contributor

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)
  • 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))

Copy link
Contributor Author

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.

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".

Copy link
Contributor Author

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.

@Souptacular
Copy link
Contributor

Just wanted to pop in and say great work!

@eip-automerger eip-automerger merged commit 01e6360 into ethereum:master Mar 12, 2020
pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
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
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
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
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants