-
Notifications
You must be signed in to change notification settings - Fork 897
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
Support timestamp forks and implement shanghaiTime #4743
Conversation
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/TimestampSchedule.java
Fixed
Show fixed
Hide fixed
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/TimestampScheduleBuilder.java
Fixed
Show fixed
Hide fixed
9e1b343
to
59f7d06
Compare
33f2f7a
to
d7eedd6
Compare
f797f51
to
8bdfafd
Compare
c8843f9
to
56edfa7
Compare
Is there some EIP specification which is being implemented here? I am having an issue finding a paper-trail (and prior discussions and justifications) for switching to date-based activation. |
@bobsummerwill I've not seen anything written down other than on calls/discord. It was mentioned in the last 15 mins of the recent ACD#151: Marius from geth team is going to implement an EIP for this. I think most/all client teams have implemented this in some form already though for withdrawals devnet interop. There's also some hive tests PRs for Withdrawals that use timestamp activation: ethereum/hive#659 |
Yep, ultimately this will just be specified by the Shanghai EIP/meta-EIP though I'm not entirely sure if we're doing meta-EIPs for upgrades anymore (we might, I just lost track of the process). Since the format of chain config files aren't standardised it basically just boils down to the fact that Shanghai will state "for any block where timestamp > X" rather than "for any block where number is > X". Marius Van Der Wijden is putting together an EIP around this though (as per ACD notes) - I think that's predominantly for how the fork ID is calculated now but may cover more than that. And as Simon notes it is a requirement for making withdrawals work that the CL and EL trigger the upgrade at the same time and that can't be done with block number because the CL needs to perform the transition at an epoch boundary and block number and slot number aren't tied together (can have empty slots that don't increase the block number). |
Thanks for the clarification, @ajsutton. Meta EIPs are no more, so it would make sense to capture this in an EIP, yes. Best wishes! |
besu/src/main/java/org/hyperledger/besu/controller/TransitionBesuControllerBuilder.java
Show resolved
Hide resolved
...sus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionProtocolSchedule.java
Show resolved
Hide resolved
consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionUtils.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/DefaultTimestampSchedule.java
Outdated
Show resolved
Hide resolved
import java.util.TreeSet; | ||
import java.util.stream.Collectors; | ||
|
||
public class DefaultTimestampSchedule implements TimestampSchedule { |
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.
Is the timestamp forks implementation only for non-privacy uses? It's missing the setPublicWorldStateArchiveForPrivacyBlockProcessor
and setTransactionFilter
which are needed for privacy features.
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.
I don't think we should exclude private use cases (I renamed MainnetTimestampSchedule -> DefaultTimestampSchedule to reflect this).
I didn't spot that anything was missed compared to the others, so I'll look into adding these, thanks!
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.
committed something for this including a new PrivacySupportingProtocolSchedule interface
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.
disagree about the privacy stuff. I think we should be excising it in favor of mainnet compatibility. $0.02
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.
I think we should discuss with hyperledger community whether timestamp support is desirable (in lieu of having an easy way to split out these use cases)
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolSpecBuilder.java
Show resolved
Hide resolved
.../core/src/main/java/org/hyperledger/besu/ethereum/mainnet/TimestampProtocolSpecAdapters.java
Outdated
Show resolved
Hide resolved
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class TimestampScheduleBuilder { |
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.
There is a lot of duplication between this class and the ProtocolScheduleBuilder. Is it possible to reuse some of the code from ProtocolScheduleBuilder
? Or just reuse the ProtocolScheduleBuilder
passing the timestamp specific pieces?
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.
I'll have a think about this. Some of the duplication goes away when we remove the modifiers as part of #4788.
There's enough slight differences that I'm worried it could make it unnecessarily complicated to understand the code.
I don't expect the logic of this code to change much either so wouldn't greatly benefit from the DRYness.
Also, eventually we'll probably only use the TimestampSchedule for public networks at least and ProtocolSchedule for private use cases so they may naturally live separately in the future.
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.
OK, I've implemented it using a template pattern and introducing AbstractProtocolScheduleBuilder here: gezero#1
It was easy enough...question is though, is it worth the extra complexity for the future code reader?
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.
After speaking to Jason, decide to add the refactor into this PR.
4221768
to
9e8243c
Compare
9e8243c
to
e1522b1
Compare
…lder Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…valuation lazy Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
… in the timestamp schedule Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…wrong schedule This error is likely to happen and a followup PR should update getByBlockNumber to use getByBlockHeader Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…called in reality though Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…ntially wrong schedule" This reverts commit 2756a9d. Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…ring Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…ultTimestampSchedule Support includes setPublicWorldStateArchiveForPrivacyBlockProcessor and setTransactionFilter methods Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Use ProtocolSpecAdapters with a renamed field instead Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…template pattern Add test coverage for ProtocolScheduleBuilder Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Also cancunTimestamp -> cancunTime Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
0661818
to
81a45e0
Compare
Implement shanghaiTime including TimestampSchedule and associated infrastructure code. TimestampSchedule sits alongside the pre and post ProtocolSchedules in TransitionProtocolSchedule. Introduces getByTimestamp, wrapped inside getByBlockHeader (to also support getByBlockNumber). General call pattern followed is that if a given timestamp precedes the first timestamp in the schedule, i.e. a pre-shanghai block, then delegate to the appropriate pre or post merge ProtocolSchedule to get by block instead. cancunTime and a placeholder cancunDefinition has also been implemented in order to effectively test fork order logic. Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jiri Peinlich <jiri.peinlich@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: Justin Florentine <justin+github@florentine.us>
Implement shanghaiTime including TimestampSchedule and associated infrastructure code. TimestampSchedule sits alongside the pre and post ProtocolSchedules in TransitionProtocolSchedule. Introduces getByTimestamp, wrapped inside getByBlockHeader (to also support getByBlockNumber). General call pattern followed is that if a given timestamp precedes the first timestamp in the schedule, i.e. a pre-shanghai block, then delegate to the appropriate pre or post merge ProtocolSchedule to get by block instead. cancunTime and a placeholder cancunDefinition has also been implemented in order to effectively test fork order logic. Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jiri Peinlich <jiri.peinlich@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Implement shanghaiTime including TimestampSchedule and associated infrastructure code. TimestampSchedule sits alongside the pre and post ProtocolSchedules in TransitionProtocolSchedule. Introduces getByTimestamp, wrapped inside getByBlockHeader (to also support getByBlockNumber). General call pattern followed is that if a given timestamp precedes the first timestamp in the schedule, i.e. a pre-shanghai block, then delegate to the appropriate pre or post merge ProtocolSchedule to get by block instead. cancunTime and a placeholder cancunDefinition has also been implemented in order to effectively test fork order logic. Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jiri Peinlich <jiri.peinlich@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Jason Frame <jason.frame@consensys.net>
Implement shanghaiTime including TimestampSchedule and associated infrastructure code.
TimestampSchedule sits alongside the pre and post MergeProtocolSchedules in TransitionProtocolSchedule.
General call pattern followed is that if a given timestamp precedes the first timestamp in the schedule, i.e. a pre-shanghai block, then delegate to the appropriate MergeProtocolSchedule.
cancunTime has also been implemented in order to effectively test fork order logic.
Note, some issues will follow on from this:
#4793
#4788
#4789