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

Fix to increment/decrement gas-limit in block production #6425

Merged
merged 12 commits into from
Jan 18, 2024

Conversation

non-fungible-nelson
Copy link
Contributor

PR description

Fixes discord bug found by protoplanetary

Fixed Issue(s)

Copy link

github-actions bot commented Jan 18, 2024

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@non-fungible-nelson non-fungible-nelson changed the title Nelson gas limit fix Fix to increment/decrement gas-limit in block production Jan 18, 2024
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
@siladu
Copy link
Contributor

siladu commented Jan 18, 2024

Think this is the geth code and it doesn't include a constant 1024 increment so besu does appear to be wrong according to both the spec and geth...https://github.com/ethereum/go-ethereum/blob/e5d5e09faae48dac3723634e2b1813e4f2e89535/core/block_validator.go#L152

Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
@non-fungible-nelson non-fungible-nelson marked this pull request as ready for review January 18, 2024 02:52
CHANGELOG.md Outdated Show resolved Hide resolved
non-fungible-nelson and others added 2 commits January 17, 2024 20:17
Co-authored-by: Stefan Pingel <16143240+pinges@users.noreply.github.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
…imit of 1024

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

wow. just wow.

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

As long as we verified the arithmetic and the resulting diff in the test artifact, 🚢

@siladu
Copy link
Contributor

siladu commented Jan 18, 2024

@garyschulte

As long as we verified the arithmetic and the resulting diff in the test artifact, 🚢

currentGasLimit = 3141592 (0x2fefd8)

currentGasLimit = 3141592
targetGasLimit = 30000000 (not set so uses default)
3141592 // 1024 = 3067
3067 - 1 = 3066 (`maxProportionalAdjustmentLimit`)
3141592 + 3066 = 3144658

3144658 = 0x2FFBD2

CHANGELOG.md Outdated Show resolved Hide resolved
non-fungible-nelson and others added 2 commits January 18, 2024 14:37
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@siladu
Copy link
Contributor

siladu commented Jan 18, 2024

Added same unit tests as geth for extra robustness https://github.com/ethereum/go-ethereum/blob/830f3c764c21f0d314ae0f7e60d6dd581dc540ce/core/block_validator_test.go#L242

@siladu siladu enabled auto-merge (squash) January 18, 2024 22:33
@siladu siladu merged commit f81d544 into hyperledger:main Jan 18, 2024
18 checks passed
@non-fungible-nelson non-fungible-nelson deleted the nelson-gas-limit-fix branch January 18, 2024 23:03
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.

6 participants