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

Complete optimism mainnet forkid tests #8012

Open
Rjected opened this issue Apr 30, 2024 · 9 comments · May be fixed by #8114
Open

Complete optimism mainnet forkid tests #8012

Rjected opened this issue Apr 30, 2024 · 9 comments · May be fixed by #8114
Assignees
@estensen
Labels
A-devp2p Related to the Ethereum P2P protocol A-op-reth Related to Optimism and op-reth C-enhancement New feature or request C-test A change that impacts how or what we test D-good-first-issue Nice and easy! A great choice to get started

Comments

@Rjected
Copy link
Member

Rjected commented Apr 30, 2024

We have a comprehensive set of tests for the mainnet and testnet forkids, including some optimism network tests:

reth/crates/primitives/src/chain/spec.rs

Lines 2443 to 2472 in 3d28a69

#[cfg(feature = "optimism")]
#[test]
fn op_sepolia_forkids() {
test_fork_ids(
&OP_SEPOLIA,
&[
(
Head { number: 0, ..Default::default() },
ForkId { hash: ForkHash([0x67, 0xa4, 0x03, 0x28]), next: 1699981200 },
),
(
Head { number: 0, timestamp: 1699981199, ..Default::default() },
ForkId { hash: ForkHash([0x67, 0xa4, 0x03, 0x28]), next: 1699981200 },
),
(
Head { number: 0, timestamp: 1699981200, ..Default::default() },
ForkId { hash: ForkHash([0xa4, 0x8d, 0x6a, 0x00]), next: 1708534800 },
),
(
Head { number: 0, timestamp: 1708534799, ..Default::default() },
ForkId { hash: ForkHash([0xa4, 0x8d, 0x6a, 0x00]), next: 1708534800 },
),
(
Head { number: 0, timestamp: 1708534800, ..Default::default() },
ForkId { hash: ForkHash([0xcc, 0x17, 0xc7, 0xeb]), next: 0 },
),
],
);
}

We should also add these for optimism mainnet, so we have forkid tests for all of the optimism networks covered.
@Rjected Rjected added C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started A-devp2p Related to the Ethereum P2P protocol C-test A change that impacts how or what we test A-op-reth Related to Optimism and op-reth labels Apr 30, 2024
@Rjected Rjected mentioned this issue May 1, 2024
Merged
@Rjected Rjected changed the title Add optimism mainnet forkid tests Complete optimism mainnet forkid tests May 1, 2024
@Rjected
Copy link
Member Author

Rjected commented May 1, 2024

Note: I just added a very basic, non-comprehensive test for this. The test now just needs to be completed with the other forks!

@estensen
Copy link
Contributor

estensen commented May 1, 2024

I'd like to work on this

@estensen
Copy link
Contributor

estensen commented May 1, 2024

Where can I find the block numbers for the forks? Found timestamps here:
https://specs.optimism.io/protocol/superchain-upgrades.html#activation-timestamps

By knowing the block number I can find the Bedrock upgrade
https://optimistic.etherscan.io/block/105235063

@Rjected
Copy link
Member Author

Rjected commented May 1, 2024

Where can I find the block numbers for the forks? Found timestamps here: https://specs.optimism.io/protocol/superchain-upgrades.html#activation-timestamps

By knowing the block number I can find the Bedrock upgrade https://optimistic.etherscan.io/block/105235063

yes, those should be correct, also take a look at the OP_MAINNET chainspec for the proper block and timestamp values. Just assigned!

@estensen
Copy link
Contributor

estensen commented May 1, 2024

I can only see the timestamps and not the blocks in the chainspec

reth/crates/primitives/src/chain/spec.rs

Lines 297 to 300 in 3d28a69

(Hardfork::Bedrock, ForkCondition::Block(105235063)),
(Hardfork::Regolith, ForkCondition::Timestamp(0)),
(Hardfork::Canyon, ForkCondition::Timestamp(1704992401)),
(Hardfork::Ecotone, ForkCondition::Timestamp(1710374401)),

Also, do you want forkid tests for all hardforks or just Canyon, Delta and Ecotone?

@onbjerg
Copy link
Member

onbjerg commented May 1, 2024

some hardforks are only conditioned on the timestamp and not the block number which is why some of them do not have a block number:)

@estensen estensen linked a pull request May 5, 2024 that will close this issue
Open
@estensen
Copy link
Contributor

estensen commented May 5, 2024

I've added the forks as test cases, but I'm stuck on computing ForkHash and next for Canyon and Delta
#8114

Computed fork ID is not computed correctly for Canyon and Delta:

assertion `left == right` failed: Expected fork ID ForkId { hash: ForkHash("00000000"), next: 1708560000 }, computed fork ID ForkId { hash: ForkHash("caf517ed"), next: 3950000 } at block 0
  left: ForkId { hash: ForkHash("00000000"), next: 1708560000 }
 right: ForkId { hash: ForkHash("caf517ed"), next: 3950000 }

@onbjerg
Copy link
Member

onbjerg commented May 6, 2024 •
edited

Is the forkhash supposed to be 00000000, do you have a source on that? That sounds incorrect

@Rjected
Copy link
Member Author

Rjected commented May 6, 2024

left some comments in reviews, let's move there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@estensen estensen

Labels
A-devp2p Related to the Ethereum P2P protocol A-op-reth Related to Optimism and op-reth C-enhancement New feature or request C-test A change that impacts how or what we test D-good-first-issue Nice and easy! A great choice to get started
Projects
Reth Tracker
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: complete optimism mainnet forkid tests estensen/reth
3 participants
@Rjected @onbjerg @estensen