Skip to content

op-node: fix sequencer time drift edge case, strictly enforce sequencer conf depth#4758

Closed
protolambda wants to merge 1 commit intodevelopfrom
origin-selection-fix
Closed

op-node: fix sequencer time drift edge case, strictly enforce sequencer conf depth#4758
protolambda wants to merge 1 commit intodevelopfrom
origin-selection-fix

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Jan 21, 2023

Description

L1 Origin selection had related problems in both sequencer and verifier areas. This PR fixes those.

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" (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.
And aside from halting as such, it risks adopting the next L1 origin differently than the sequencer, causing a reorg of the unsafe chain.

Meanwhile we still want deposits to be processed as soon as the sequencer time drift, so we should not give the sequencer too much leeway either,
and force them to adopt the next L1 origin when they theoretically can.

This also means that we need to accept batches with an old origin if the batches hit sequencer time drift edge cases
where it is just not possible to adopt the next L1 origin without breaking the L2 time >= L1 origin time invariant.

And while we are at it, we might as well solve the "what if L1 block time is smaller than L2 block time" edge case:
we can ignore the sequencer time drift check if the other checks pass & the batch advances the L1 origin forward.
Advancing the L1 origin is exactly what the sequencer time drift is specified for, so we should not disallow advancing
to a later L1 origin that we still exceed the L2 sequencer time drift of.

In the sequencer we should be strict, and avoid sequencing if we ever doubt about the validity of staying on the current L1 origin:
i.e. if we are over the sequencer time drift, and can't find the next L1 origin, and then wait until we see the L1 origin,
instead of creating a L2 block that could be dropped/reorged out.

With such a sequencer, we can also clean up the conf-depth approach:
the L1 head should not have to leak into the l1 origin selection logic if the logic already has accesses L1 through a limited by-number view.
Like the verifier conf depth, we can re-use the exact same tested code to enforce the conf depth for L1 origin selection.

Changes:

  • Fix L1OriginSelector to implement the stricter sequencer origin selection as described above
  • Update Driver to use L1OriginSelector with conf-depth util that we also use for verifier conf depth, and simplify the origin selection.
  • Fix sequencer time drift section in CheckBatch to handle edge cases
  • Fix derivation batch filtering spec to define the edge case filtering conditions in the sequencer time drift check case.
  • testing, see below

Tests

  • Updated and extended driver/origin_selector_test.go to cover the new strict sequencer behavior.
  • Updated and extended derive/batches_test.go with cases that cover the (new) different branches you can take.
    • 96% filtering coverage at first. We were missing uint64(batch.Batch.EpochNum) < epoch.Number,
      so I fixed the epoch too old case to get 100% coverage.

Invariants

  • Ensure L2 time >= L1 origin time at all times
  • Ensure max sequencer time drift is respected, but make exception to always preserve L1 Origin advancement and above invariant.

Additional context

Note that the sequencer origin selection got stricter (shouldn't produce invalid cases anymore) and the verifier filtering got more relaxed (shouldn't drop the exceptions to the sequencer time drift rule anymore).

Goerli users who don't update are likely risk of halting their view of the chain upon highly irregular L1 chain advancement or very delayed L1 origin inclusion.
This, together with these events already being ultra-rare (especially with the quite high sequencer time drift of 10 minutes),
means that I think we can safely roll this out to Goerli & encourage updating.

Fix ENG-3255

Stacked on #4757 and draft until that is merged.

Full PR stack:

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 21, 2023

⚠️ No Changeset found

Latest commit: 9489a0e

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

…er conf depth and reuse verifier conf depth util
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 22, 2023

Codecov Report

Merging #4758 (9489a0e) into sequencer-engine-control (465a10b) will increase coverage by 0.18%.
The diff coverage is 86.95%.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                      @@
##           sequencer-engine-control    #4758      +/-   ##
============================================================
+ Coverage                     36.46%   36.65%   +0.18%     
============================================================
  Files                           180      180              
  Lines                         14399    14410      +11     
============================================================
+ Hits                           5251     5282      +31     
+ Misses                         8618     8604      -14     
+ Partials                        530      524       -6     
Flag Coverage Δ
bedrock-go-tests 36.65% <86.95%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/rollup/driver/driver.go 0.00% <0.00%> (ø)
op-node/rollup/driver/state.go 0.00% <0.00%> (ø)
op-node/rollup/driver/origin_selector.go 83.33% <82.35%> (+10.00%) ⬆️
op-node/rollup/derive/batches.go 100.00% <100.00%> (+6.49%) ⬆️
op-node/rollup/driver/sequencer.go 94.59% <100.00%> (ø)
op-node/p2p/discovery.go 67.91% <0.00%> (+2.38%) ⬆️
op-node/sources/batching.go 85.71% <0.00%> (+3.06%) ⬆️

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 31, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jan 31, 2023
@protolambda
Copy link
Copy Markdown
Contributor Author

Closing in favor of #4809 and #4808

@mergify mergify bot removed the conflict label Jan 31, 2023
@protolambda protolambda deleted the origin-selection-fix branch August 6, 2023 17:18
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.

1 participant