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.

Sign up for GitHub

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

Jump to bottom

Block, VM: Unify hardforkBy Options #2800

Merged
merged 6 commits into from Jun 20, 2023
Merged

Block, VM: Unify hardforkBy Options #2800

merged 6 commits into from Jun 20, 2023

Conversation

holgerd77
Copy link
Member

Second part of #2774 following #2798

This PR does the following:

  • Block: unify hardforkByBlockNumber, hardforkByTTD options -> setHardfork
  • VM (constructor/runBlock()): unify hardforkByBlockNumber, hardforkByTTD options -> setHardfork

@codecov
Copy link

codecov bot commented Jun 19, 2023 •
edited

Codecov Report

Merging #2800 (ae42919) into master (3bf4692) will decrease coverage by 0.02%.
The diff coverage is 65.45%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.56% <90.47%> (-0.04%) ⬇️
blockchain 92.85% <100.00%> (ø)
client 87.10% <66.66%> (+0.01%) ⬆️
common 97.09% <ø> (ø)
devp2p 86.58% <ø> (ø)
ethash ∅ <ø> (∅)
evm 66.60% <ø> (ø)
rlp ∅ <ø> (?)
statemanager 86.35% <ø> (ø)
trie 90.02% <ø> (ø)
tx 95.87% <ø> (ø)
util 85.31% <ø> (ø)
vm 77.07% <30.00%> (-0.48%) ⬇️

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

jochem-brouwer
jochem-brouwer previously approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

LGTM

acolytec3
acolytec3 previously requested changes Jun 20, 2023
View reviewed changes
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.

I could be missing the significance but requested one change in how we evaluate the setHardfork opt so we don't check it twice (if it's indeed true). Small efficiency gain if I'm not missing something. Otherwise, looks great!

@holgerd77 @acolytec3
Apply suggestions from code review
a7e9428 
Co-authored-by: acolytec3 <konjou@gmail.com>
@holgerd77
Copy link
Member Author

@jochem-brouwer @acolytec3 thanks for reviewing 🙂, have applied the suggested changes, needs re-approval now! 🙂

jochem-brouwer
jochem-brouwer previously approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Ah right, good catch @acolytec3. LGTM still! 😄 (assuming CI passes)

@jochem-brouwer
Copy link
Member

Nvm, CI fails. I should not approve so fast 😅 (how do I dismiss my review?)

@jochem-brouwer
Copy link
Member

Will re-approve if CI passes

g11tech
g11tech approved these changes Jun 20, 2023
View reviewed changes
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.

looks amazing! 🚀 ❤️

@holgerd77 holgerd77 dismissed acolytec3’s stale review June 20, 2023 09:22

Changes are adressed, will dismiss.

@holgerd77 holgerd77 merged commit df8d8d7 into master Jun 20, 2023
38 of 40 checks passed
@holgerd77 holgerd77 deleted the block-vm-unify-hardfork-by-options branch June 20, 2023 09:23
@holgerd77 holgerd77 mentioned this pull request Jul 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g11tech g11tech g11tech approved these changes

@acolytec3 acolytec3 acolytec3 left review comments

@jochem-brouwer jochem-brouwer jochem-brouwer left review comments

Assignees
No one assigned
Labels
breaking package: block package: blockchain package: client package: vm PR state: needs review type: enhancement type: refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@holgerd77 @jochem-brouwer @acolytec3 @g11tech