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

Support timestamp forks and implement shanghaiTime #4743

Merged
merged 35 commits into from
Dec 16, 2022

Conversation

gezero
Copy link
Contributor

@gezero gezero commented Nov 28, 2022

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

@gezero gezero force-pushed the spike-transition-schedule branch 3 times, most recently from 9e1b343 to 59f7d06 Compare November 30, 2022 13:17
@siladu siladu added the TeamGroot GH issues worked on by Groot Team label Dec 2, 2022
@siladu siladu added EIP Ethereum Improvement Proposal mainnet labels Dec 5, 2022
@siladu siladu mentioned this pull request Dec 5, 2022
27 tasks
@siladu siladu changed the title Spike transition schedule Support and implement shanghaiTimestamp Dec 8, 2022
@siladu siladu changed the title Support and implement shanghaiTimestamp Support timestamp forks and implement shanghaiTimestamp Dec 8, 2022
@siladu siladu marked this pull request as ready for review December 8, 2022 08:09
@bobsummerwill
Copy link

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.

@siladu
Copy link
Contributor

siladu commented Dec 8, 2022

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.
I think the justification is the ability to couple CL and EL forks, i.e. Capella and Shanghai should fork at the same time. CL uses forks based on Epochs, which are a proxy for timestamps.

There's also some hive tests PRs for Withdrawals that use timestamp activation: ethereum/hive#659
https://notes.ethereum.org/@marioevz/Withdrawals-hive-testing

@ajsutton
Copy link
Contributor

ajsutton commented Dec 9, 2022

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).

@bobsummerwill
Copy link

Thanks for the clarification, @ajsutton.

Meta EIPs are no more, so it would make sense to capture this in an EIP, yes.

Best wishes!

import java.util.TreeSet;
import java.util.stream.Collectors;

public class DefaultTimestampSchedule implements TimestampSchedule {
Copy link
Contributor

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.

Copy link
Contributor

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!

Copy link
Contributor

@siladu siladu Dec 12, 2022

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

Copy link
Contributor

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

Copy link
Contributor

@siladu siladu Dec 14, 2022

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)

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class TimestampScheduleBuilder {
Copy link
Contributor

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?

Copy link
Contributor

@siladu siladu Dec 12, 2022

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.

Copy link
Contributor

@siladu siladu Dec 13, 2022

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?

Copy link
Contributor

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.

…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>
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>
@siladu siladu enabled auto-merge (squash) December 16, 2022 02:20
@siladu siladu merged commit 3fd0681 into hyperledger:main Dec 16, 2022
jflo pushed a commit to jflo/besu that referenced this pull request Dec 22, 2022
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>
macfarla pushed a commit to jflo/besu that referenced this pull request Jan 10, 2023
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>
@siladu siladu mentioned this pull request Jan 14, 2023
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP Ethereum Improvement Proposal mainnet TeamGroot GH issues worked on by Groot Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants