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

Changes 1559 to have a single shared pool for all transactions. #2924

Merged
merged 9 commits into from
Oct 6, 2020

Conversation

MicahZoltu
Copy link
Contributor

Rather than having two pools and a migration, we instead have a single transaction pool and legacy transactions are "upgraded" into 1559 transactions using a very naive strategy of just setting fee_cap == gas_bribe == gas_price.

While in here, I also made a few other minor changes since this is a consensus change and will require all clients update:

  1. Renamed premium to bribe. I think this makes it more clear what this is, it is a bribe for the miner to convince them to include your transaction.
  2. Changed the gas target calculation to be a bit simpler (per prior conversation with @i-norden)
  3. Added special handling for when the gas_used == gas_target (no change in target for next block).
  4. Adds missing assertion that gas_used wasn't more than 2x the gas target.
  5. Added clarity to some names like fee_cap to fee_cap_per_gas and miner_bribe to miner_bribe_per_gas.

@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 Show resolved Hide resolved
@i-norden
Copy link
Contributor

i-norden commented Aug 31, 2020

I think instead of fee_cap == gas_bribe == gas_price

It is more accurate to say fee_cap == gas_price; gas_bribe = fee_cap - base_fee

In practice we should set gas_bribe = gas_price in the tx struct, but the amount of gas_price actually available for the bribe after burning the base_fee will be gas_price - base_fee i.e. fee_cap - base_fee.

Thanks for opening this! I haven't had a chance to look into the rest of this PR in more depth, will do so soon.

Base automatically changed from MicahZoltu-patch-4 to master August 31, 2020 17:50
@MicahZoltu
Copy link
Contributor Author

@i-norden I think what you are getting at is that miner_bribe_per_gas is a bit of a misnomer and really it is maximum_miner_bribe_per_gas. If so, I agree with you and have updated the EIP to reflect that.

Now that we have merged the other PR, I have rebased this and it is ready to merge once the authors approve it. Per discussion in Discord, we should make note that 711474afe36a9d778f7e1858b2c9a88b0bf146d0 is the commit hash for EIP-1559 that is currently on the integration testnet, and whatever the merge commit hash for this is will be the "target" EIP we are working toward and probably what new clients should implement.

@i-norden, @mslipper, @AFDudley, @vbuterin, @econoar

EIPS/eip-1559.md Outdated Show resolved Hide resolved
EIPS/eip-1559.md Outdated Show resolved Hide resolved
@MicahZoltu
Copy link
Contributor Author

I have removed the prose text that referred to the transition as well (missed it in original PR).

Copy link
Contributor

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

MicahZoltu and others added 9 commits October 6, 2020 21:48
Rather than having two pools and a migration, we instead have a single transaction pool and legacy transactions are "upgraded" into 1559 transactions using a *very* naive strategy of just setting `fee_cap == gas_bribe == gas_price`.

While in here, I also made a few other minor changes since this is a consensus change and will require all clients update:
1. Renamed premium to bribe.  I think this makes it more clear what this is, it is a bribe for the miner to convince them to include your transaction.
2. Changed the gas target calculation to be a bit simpler (per prior conversation with @i-norden)
3. Added special handling for when the gas_used == gas_target (no change in target for next block).
4. Adds missing assertion that gas_used wasn't more than 2x the gas target.
5. Added clarity to some names like `fee_cap` to `fee_cap_per_gas` and `miner_bribe` to `miner_bribe_per_gas`.
Hopefully adds a bit more clarity to people reading the specification.
Co-authored-by: vbuterin <v@buterin.com>
Co-authored-by: vbuterin <v@buterin.com>
@eip-automerger eip-automerger merged commit 1d6ff84 into master Oct 6, 2020
@MicahZoltu MicahZoltu deleted the MicahZoltu-patch-1 branch October 6, 2020 14:08
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.

8 participants