Skip to content

op-node: ensure sequencer selects L1 origin within sequencer time drift and use conf depth util#4808

Merged
mergify[bot] merged 1 commit intodevelopfrom
fix-sequencer-origin-selection
Jan 31, 2023
Merged

op-node: ensure sequencer selects L1 origin within sequencer time drift and use conf depth util#4808
mergify[bot] merged 1 commit intodevelopfrom
fix-sequencer-origin-selection

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Jan 31, 2023

Description

Subset of original #4758

As sequencer it didn't follow the safety-over-liveness priority, and ignored the sequencer conf depth to try and handle sequencer time drift.

This however is dangerous because of the recent verifier "fix" in (good fix, but incomplete):

  • When the max time drift is exceeded and the next origin cannot be found, then old origin would be used, causing the batch to be dropped.
  • But the sequencer still produces blocks after this, which can have later origins & include transactions again, building on top of the empty block with old origin.

This means that as a verifier you end up dropping the empty batch with old origin, causing a halt (of safe blocks) until the sequencer window forces an empty block to be created. This already went wrong once due to a L1 RPC outage on Goerli

And aside from halting as such, it risks adopting the next L1 origin differently than the sequencer, causing a reorg of the unsafe chain.

Tests

  • Modified TestOriginSelectorRespectsMaxSeqDrift into TestOriginSelectorStrictConfDepth to enforce conf depth.
  • Added TestOriginSelectorHandlesLateL1Blocks to cover the case when the L1 block time gap is larger than the sequencer time drift. It preserves the time invariant in favor of the timedrift invariant. Note: the safe-head progress would be bricked after this (assuming L1 does not reorg out the gap), as one of the two invariants breaks. We need some sort of derivation change like in the original PR.

Invariants

  • Ensure that the sequencer conf depth is maintained
  • Ensure that the sequencer never produces a block that exceeds sequencer time drift w.r.t. selected origin.
    • The exception here is if the block has to exceed the sequencer time drift to maintain the L2 timestamp >= L1 origin timestamp invariant. This can happen if L1 blocks have a huge time gap, larger than the sequencer time drift. This was fixed in op-node: fix sequencer time drift edge case, strictly enforce sequencer conf depth #4758 but will now be split into a different PR because it affects derivation. In such case I believe it's best to allow the sequencer to continue, but only until the L1 origin can be introduced without breaking the (L2 timestamp >= L1 origin timestamp` invariant, and disable tx inclusion by sequencer to avoid L2 activity that has to be submitted in such risky L1 situation (large L1 gaps = likely trouble including batch data).

Additional context

See #4758 and #3861 (comments too) for additional context.

Metadata

Fix ENG-3285

@protolambda protolambda requested review from a team and trianglesphere as code owners January 31, 2023 14:45
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 31, 2023

⚠️ No Changeset found

Latest commit: 72f0f9e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice works. Thanks for the fix.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 31, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit 679522b into develop Jan 31, 2023
@mergify mergify bot deleted the fix-sequencer-origin-selection branch January 31, 2023 17:00
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 31, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants