chore: complete optimism mainnet forkid tests #8114

estensen · 2024-05-05T20:12:54Z

Closes #8012 when completed

Rjected

left some comments, let me know if you're still stuck on getting the expected forkhash for these tests

Rjected · 2024-05-06T15:16:56Z

crates/primitives/src/chain/spec.rs

@@ -287,6 +287,7 @@ pub static OP_MAINNET: Lazy<Arc<ChainSpec>> = Lazy::new(|| {
            (Hardfork::Bedrock, ForkCondition::Block(105235063)),
            (Hardfork::Regolith, ForkCondition::Timestamp(0)),
            (Hardfork::Canyon, ForkCondition::Timestamp(1704992401)),
+            (Hardfork::Delta, ForkCondition::Timestamp(1708560000)),


where did you find this? Is there an op mainnet genesis link somewhere?

Added comment with link
2ea4bcc

ah, reading this it looks like that's an op-node (CL) change, so it does not need to exist here

Where do you see that?

see https://docs.optimism.io/builders/node-operators/network-upgrades

The Delta upgrade uses a L2 block-timestamp activation-rule, and is specified only in the rollup-node (delta_time).

Rjected · 2024-05-06T15:21:36Z

crates/primitives/src/chain/spec.rs

+                // Canyon
+                (
+                    Head { timestamp: 1704992401, ..Default::default() },
+                    ForkId { hash: ForkHash([0x0, 0x0, 0x0, 0x0]), next: 1708560000 },
+                ),
+                // Delta
+                (
+                    Head { timestamp: 1708560000, ..Default::default() },
+                    ForkId { hash: ForkHash([0x0, 0x0, 0x0, 0x0]), next: 1710374401 },
+                ),


re: #8012 (comment)

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

It's not supposed to be all zeros, typically values for this would be obtained from writing a similar test in geth, but since our final (ecotone) forkhash is compliant with geth, what we compute in test failures is probably also correct. However, starting at Canyon we can't use ..Default::default() because the block needs to be higher than the london / bedrock block. Let's change it back to the previous block number

[0x0, 0x0, 0x0, 0x0] is just temporary since test failure output is not too useful, to be able to find back to the test case that failed

thread 'chain::spec::tests::op_mainnet_forkids' panicked at crates/primitives/src/chain/spec.rs:1643:13: 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 } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Rjected · 2024-05-06T15:22:16Z

crates/primitives/src/chain/spec.rs

                    Head { number: 0, ..Default::default() },
                    ForkId { hash: ForkHash([0xca, 0xf5, 0x17, 0xed]), next: 3950000 },
                ),
-                // TODO: complete these, see https://github.com/paradigmxyz/reth/issues/8012
+                // Berlin
                (
-                    Head { number: 105235063, timestamp: 1710374401, ..Default::default() },
+                    Head { number: 3950000, ..Default::default() },
+                    ForkId { hash: ForkHash([0x52, 0x6a, 0x21, 0x71]), next: 105235063 },
+                ),
+                // London
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Arrow Glacier
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Grey Glacier
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Paris
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Bedrock
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Canyon
+                (
+                    Head { timestamp: 1704992401, ..Default::default() },
+                    ForkId { hash: ForkHash([0x0, 0x0, 0x0, 0x0]), next: 1708560000 },
+                ),
+                // Delta
+                (
+                    Head { timestamp: 1708560000, ..Default::default() },
+                    ForkId { hash: ForkHash([0x0, 0x0, 0x0, 0x0]), next: 1710374401 },


let's make sure we specify number and timestamp explicitly for each of these

emhane

using constants here, would be super helpful. would make it way easier to review if tests are correct + import values to other parts of codebase.

grouping constants in a type impl is nice.

pub struct OpMainnetForkHash;

impl OpMainnetForkHash {
    pub const BEDROCK: ForkHash = <fork-hash>;
    ..
}

pub struct OpMainnetForkId;

impl OpMainnetForkId {
    pub const BEDROCK: ForkId = ForkId::new(OpMainnetForkHash::BEDROCK, <next>);
    ..
}

pub struct OpMainnetForkTimestamp;

impl OpMainnetForkTimestamp {
    pub const REGOLITH: u64 = 0;
    ..
}

Rjected · 2024-05-21T21:11:40Z

using constants here, would be super helpful. would make it way easier to review if tests are correct + import values to other parts of codebase.

grouping constants in a type impl is nice.

pub struct OpMainnetForkHash;

impl OpMainnetForkHash {
    pub const BEDROCK: ForkHash = <fork-hash>;
    ..
}

pub struct OpMainnetForkId;

impl OpMainnetForkId {
    pub const BEDROCK: ForkId = ForkId::new(OpMainnetForkHash::BEDROCK, <next>);
    ..
}

pub struct OpMainnetForkTimestamp;

impl OpMainnetForkTimestamp {
    pub const REGOLITH: u64 = 0;
    ..
}

IMO out of scope for this PR, since it should just be completing a few tests instead of introducing a new pattern for known forkhashes

Rjected

needs lint and a few other small things

Rjected · 2024-05-21T21:12:48Z

crates/primitives/src/chain/spec.rs

+                // Berlin
+                (
+                    Head { number: 3950000, ..Default::default() },
+                    ForkId { hash: ForkHash([0x52, 0x6a, 0x21, 0x71]), next: 105235063 },
+                ),
+                // London
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Arrow Glacier
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Grey Glacier
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Paris
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Bedrock
+                (
+                    Head { number: 105235063, ..Default::default() },
+                    ForkId { hash: ForkHash([0xe3, 0x39, 0x8d, 0x7c]), next: 1704992401 },
+                ),
+                // Shanghai
+                (
+                    Head { number: 105235063, timestamp: 1704992401, ..Default::default() },
+                    ForkId { hash: ForkHash([0xbd, 0xd4, 0xfd, 0xb2]), next: 1710374401 },
+                ),


let's just use op-specific hardforks, I am realizing these are not super useful

Rjected · 2024-05-21T21:12:58Z

crates/ethereum-forks/src/hardfork.rs

+    /// Delta:
+    /// https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/superchain-upgrades.md#delta
+    #[cfg(feature = "optimism")]
+    Delta,


this still needs to be removed

estensen mentioned this pull request May 5, 2024

Complete optimism mainnet forkid tests #8012

Open

Rjected requested changes May 6, 2024

View reviewed changes

estensen force-pushed the op-forkid-tests branch from e491782 to aa14542 Compare May 9, 2024 11:21

estensen marked this pull request as ready for review May 14, 2024 17:42

estensen requested a review from gakonst as a code owner May 14, 2024 17:42

estensen requested a review from Rjected May 14, 2024 18:09

mattsse added C-test A change that impacts how or what we test A-op-reth Related to Optimism and op-reth labels May 14, 2024

estensen added 5 commits May 15, 2024 12:28

chore: complete optimism mainnet forkid tests

25a2fbc

chore: add link to op activation timestamps

9ce2756

chore: add number for op forks

1f93c67

chore: complete optimism mainnet forkid tests

00ef9f7

chore: remove op delta hardfork from forkid tests

b7a36ec

estensen force-pushed the op-forkid-tests branch from 388ccd2 to b7a36ec Compare May 15, 2024 10:30

emhane reviewed May 17, 2024

View reviewed changes

Rjected requested changes May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: complete optimism mainnet forkid tests #8114

chore: complete optimism mainnet forkid tests #8114

estensen commented May 5, 2024 •

edited

Rjected left a comment

Rjected May 6, 2024

estensen May 9, 2024

Rjected May 9, 2024

estensen May 14, 2024

Rjected May 14, 2024

Rjected May 6, 2024

estensen May 8, 2024 •

edited

Rjected May 6, 2024

emhane left a comment

Rjected commented May 21, 2024

Rjected left a comment

Rjected May 21, 2024 •

edited

Rjected May 21, 2024

chore: complete optimism mainnet forkid tests #8114

Are you sure you want to change the base?

chore: complete optimism mainnet forkid tests #8114

Conversation

estensen commented May 5, 2024 • edited

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estensen May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane left a comment

Choose a reason for hiding this comment

Rjected commented May 21, 2024

Rjected left a comment

Choose a reason for hiding this comment

Rjected May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estensen commented May 5, 2024 •

edited

estensen May 8, 2024 •

edited

Rjected May 21, 2024 •

edited