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

client/vm: fix block builder london transition #3039

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

jochem-brouwer
Copy link
Member

This fixes the construction of the initial London block. We take the gasLimit from the previous block (this is incorrect in London and should be (approx.) twice the gasLimit of the previous pre-London block at the initial London block) and also do not set the initial base fee correctly.

Found in: ethereum/execution-apis#398

To run:

  1. Checkout the branch of the PR above (it is not merged yet)
  2. Run: ./hive --client ethereumjs --sim ethereum/engine --sim.limit "engine-withdrawals/Pre-Merge Fork Number" --docker.output

@@ -85,7 +85,11 @@ export class BlockBuilder {
this.vm.common.isActivatedEIP(1559) === true &&
typeof this.headerData.baseFeePerGas === 'undefined'
) {
this.headerData.baseFeePerGas = opts.parentBlock.header.calcNextBaseFee()
if (this.headerData.number === vm.common.hardforkBlock(Hardfork.London)) {
this.headerData.baseFeePerGas = vm.common.param('gasConfig', 'initialBaseFee')
Copy link
Member Author

Choose a reason for hiding this comment

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

This could also be moved back into client, but I think the place for this change is better here?

Note: it doesn't seem that we have access to the parent block in this scope, so cannot check parent gas limit here... (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

opts.parentBlock could be available here

Copy link
Contributor

Choose a reason for hiding this comment

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

added a commit to address this

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #3039 (ea3e900) into master (9389082) will increase coverage by 0.00%.
The diff coverage is 88.23%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.73% <ø> (ø)
blockchain 92.58% <ø> (ø)
client 87.67% <100.00%> (+0.04%) ⬆️
common 98.19% <ø> (ø)
ethash ∅ <ø> (∅)
evm 71.75% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 90.15% <ø> (ø)
trie 90.21% <ø> (-0.11%) ⬇️
tx 96.35% <ø> (ø)
util 86.78% <ø> (ø)
vm 78.28% <71.42%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@acolytec3
Copy link
Contributor

I've confirmed the specified test above passes so this LGTM. That said, why does that test not appear in the official hive test results? I spent a while looking for it in the results but I don't see it listed in the most recent engine-withdrawals results.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm penging @acolytec3 retest because of commit addition

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@g11tech g11tech merged commit 9d62bb4 into master Sep 21, 2023
42 checks passed
@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Sep 21, 2023

EDIT: oh wait, nvm, this is not about cancun

@holgerd77 holgerd77 deleted the fix-miner-london-transition branch July 11, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants