fix: use --syncmode=execution-layer from op-node for optimistic pipeline sync
#7552
Conversation
joshieDo
changed the title
op-reth now uses --syncmode=execution-layer from op-node on starting up
joshieDo
added
C-bug
|
smol meta nit: can we make the PR title imperative 😄 |
joshieDo
changed the title
op-reth now uses --syncmode=execution-layer from op-node on starting up--syncmode=execution-layer from op-node for pipeline sync
crates/rpc/rpc/src/eth/error.rs
Outdated
| /// op-node uses case sensitive string comparison to parse this error: | ||
| /// https://github.com/ethereum-optimism/optimism/blob/0913776869f6cb2c1218497463d7377cf4de16de/op-service/sources/l2_client.go#L105 | ||
| /// | ||
| /// Temporary, until a version of https://github.com/ethereum-optimism/optimism/pull/10071 is pushed through that doesn't require this to figure out the EL sync status. |
| } else if error.optimistic_revert_block_number().is_some() { | ||
| // engine already set the pipeline unwind target on | ||
| // `try_make_sync_target_canonical` | ||
| PayloadStatus::from_status(PayloadStatusEnum::Syncing) |
There was a problem hiding this comment.
unsure here if returning Syncing is the best
| @@ -668,6 +669,21 @@ where | |||
| // threshold | |||
| return Some(state.finalized_block_hash) | |||
| } | |||
|
|
|||
| // OPTIMISTIC SYNCING | |||
There was a problem hiding this comment.
or should we do this only for OP (chainspec.is_optimism) for now?
There was a problem hiding this comment.
i mean, its unlikely that it can happen on mainnet, but it can happen on testnets/devnets l1 for sure.
i can add the feature gate or check... but what would be the reason?
|
1st run of hive tests seemed to fail 2 tests, tried locally and they were all passing so issued a second action and they all passed same as main: https://github.com/paradigmxyz/reth/actions/runs/8930154135 |
|
eth-holesky: triggered optimistic sync on lighthouse + reth (although low block, instead of near the tip) |
| head_block_hash: head.hash(), | ||
| safe_block_hash: head.hash(), | ||
| finalized_block_hash: head.hash(), |
There was a problem hiding this comment.
I'm pretty sure this was problematic from the start
| } else if error.optimistic_revert_block_number().is_some() { | ||
| // engine already set the pipeline unwind target on | ||
| // `try_make_sync_target_canonical` | ||
| PayloadStatus::from_status(PayloadStatusEnum::Syncing) |
| @@ -668,6 +669,21 @@ where | |||
| // threshold | |||
| return Some(state.finalized_block_hash) | |||
| } | |||
|
|
|||
| // OPTIMISTIC SYNCING | |||
There was a problem hiding this comment.
or should we do this only for OP (chainspec.is_optimism) for now?
…ipeline sync (paradigmxyz#7552) Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Adds optimistic sync (unsafe head hash & finalized zero hash). Although a requirement for optimism, it's also an expectation in non OP chains.
PipelineTarget::Unwind(block)crates/optimism/node/tests/e2e/p2p.rshas an example scenarioPending: syncing base main from scratch.--
OP specific changes:
https://github.com/paradigmxyz/reth/pull/7552/files#diff-c821f432ea2a3ba2fdad959da77aae18c2b20e4988e032aa856576cfafb3a58a:
op-nodewill ignore--syncmode=execution-layer, if the EL has any finalized block. This makes sure that we don't start with one (as is usual behaviour). Otherwise,op-nodewill move on to a CL sync style, feeding us one block at a time from our lowest.The error from
UnknownSafeOrFinalizedBlockhas to start with an upper case since it's what op-node requires: https://github.com/ethereum-optimism/optimism/blob/0913776869f6cb2c1218497463d7377cf4de16de/op-service/sources/l2_client.go#L105. This is temporary until a version of the following PR is pushed through, where the tracking of the EL status is properly tracked.