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

Create eip to cap gas limit #3756

Merged
merged 5 commits into from
Aug 23, 2021
Merged

Conversation

lightclient
Copy link
Member

^

@eth-bot eth-bot enabled auto-merge (squash) August 21, 2021 15:00
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 21, 2021

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

 - File with name EIPS/eip-3756.md is new and new files must be reviewed
 - This PR requires review from one of [@micahzoltu, @arachnid, @cdetrio, @souptacular, @vbuterin, @nicksavers, @wanderer, @gcolvin, @axic]

@MicahZoltu @Arachnid @cdetrio @Souptacular @vbuterin @nicksavers @wanderer @gcolvin @axic

@lightclient lightclient requested a review from MicahZoltu August 21, 2021 22:11
@lightclient
Copy link
Member Author

lightclient commented Aug 21, 2021

@MicahZoltu updated, PTAL.

Comment on lines +26 to +27
As of the fork block `N`, consider blocks with a `gas_limit` greater than
`30,000,000` invalid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
As of the fork block `N`, consider blocks with a `gas_limit` greater than
`30,000,000` invalid.
As of FORK_BLOCK_NUMBER, consider blocks with a `gas_limit` greater than `30,000,000` invalid.

This is the style we usually use for referring to the fork block number inline. Unless there is an argument for deviating from that I would prefer to stick to the normal style here (note: I also personally like the previous style, so I'm a bit biased).


## Specification

As of the fork block `N`, consider blocks with a `gas_limit` greater than
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be valuable to be more explicit about what exactly is invalid here. In this case it is the 10th RLP encoded item in the block header that must be a QUANTITY that is less than or equal to 30,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.

Counting RLP fields is too pedantic for readability. I think referencing EIP-1559 in the header and then "a block with gas_limit greater than 30,000,000" reads much better.

@lightclient
Copy link
Member Author

Can we go ahead and merge this as a draft? I'll update these things before review.

EIPS/eip-3756.md Outdated
A valuable property of proposers choosing the gas limit is they can scale it
down quickly if the network becomes unstable or is undergoing certain types of
attacks. For this reason, we maintain their ability to lower the gas limit
_below_ 15,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.

Do you mean Gas target here?

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Only blocker is the 15M to 30M change (since currently it is incorrect). The rest are just recommendations.

EIPS/eip-3756.md Outdated
A valuable property of proposers choosing the gas limit is they can scale it
down quickly if the network becomes unstable or is undergoing certain types of
attacks. For this reason, we maintain their ability to lower the gas limit
_below_ 15,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.

Suggested change
_below_ 15,000,000.
_below_ 30,000,000.

Copy link

Choose a reason for hiding this comment

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

If we'd like to restrict something, shall we have a good benchmark first for all clients? And then discuss real gas limits of the clients.

Comment on lines +48 to +50
## Test Cases
TBD

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Test Cases
TBD

Recommend removing sections that aren't done yet, rather than leaving them stubbed out (only applies to sections that aren't required).

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like this TBD especially for the Test Cases as a reminder that we should add at some point.

@lightclient
Copy link
Member Author

@MicahZoltu updated again.

@MaxHastings
Copy link

I would highly suggest to add another section that addresses the concerns some people may have that the gas limit will never be raised again. We have seen dev limits in the past cause lots of controversy. Most notably the 1 MB block limit on Bitcoin. How will the Ethereum team reassure the community that this EIP will not lead to the same problem? Thank you :)

@lightclient lightclient changed the title Create eip to cap gas target Create eip to cap gas limit Aug 22, 2021
@0xalizk
Copy link
Contributor

0xalizk commented Aug 22, 2021

Is the implementation simply this?

@MicahZoltu
Copy link
Contributor

Discussion about the general content of the EIP should be posted in the discussions-to link (in the EIP header). PR comments are considered transient and meant just for editorial remarks and technical comments on the structure of the EIP.

@MicahZoltu MicahZoltu disabled auto-merge August 23, 2021 07:58
@MicahZoltu MicahZoltu merged commit 223ddc2 into ethereum:master Aug 23, 2021
@ghost
Copy link

ghost commented Sep 9, 2021

Most notably the 1 MB block limit on Bitcoin

@MaxHastings It's not 1 MB right now: https://bitcoin.stackexchange.com/questions/98810/whats-the-blocksize-limit-after-segwit-and-how-do-legacy-nodes-deal-with-segwit

PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
* create eip to cap gas taret

* s/target/limit

* rename file

* clean up and add discussion link

* typo
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.

9 participants