-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: support time-based forking #985
Conversation
Let's discuss this before merging |
@gakonst yeah I made some impacting changes... And yet I held back, I was this close to replace: reth/crates/primitives/src/lib.rs Line 65 in 78ffd0a
by pub struct BlockNumber(u64); |
what's the motivation for this? this would be super inconvenient imo. edit: to distinguish between timestamp and blocknumber? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving this to ChainSpec, seems like the correct change to me.
@Rjected can review these changes best in detail
there are several unwraps and the Default inits that look a bit weird to atm.
Exactly, to be able to impl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok overall, not sure what I would have done much differently. We need to figure out what we are going to do about the places where we hard depend on Paris having a set block number; this is not the case for all networks, so we should prefer TTD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good start! most notes are about MergeNetsplit
- this has unfortunate naming because it's basically only used in sepolia, and ttd
is still used in mainnet
@Rjected I think I've taken all change requests into account, the PR can be reviewed again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few nits, otherwise lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
pending @Rjected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 lgtm! thank you!
I think this introduced a regression.
This happens all the time. @onbjerg probably correctly identified that this is incorrect: reth/crates/primitives/src/chain/spec.rs Lines 147 to 148 in e8d7c05
because of how it's called here: reth/crates/consensus/src/verification.rs Lines 53 to 55 in e8d7c05
Should we revert this PR? And investigate a proper solution, in preparation for Shanghai? cc @onbjerg @mattsse @Rjected Also see this WIP spec ethereum/EIPs#6122 |
Closes #937