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.

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

op-node: Span Batch Derivation #7289

Closed

Conversation

ImTei
Copy link
Contributor

@ImTei ImTei commented Sep 18, 2023

Contexts

This PR contains the derivation code for Span Batch.

Please refer to the Implementations - Derivation and Hard Fork Activation sections of the Design Docs and op-node section of Implementation Design Docs for details & rationales.

This PR has a dependency on the Span Batch Type, Encoding, and Decoding PR.

Changes

  • Modifications:
    • Modify ChannelReader to decode both SingularBatch and SpanBatch.
    • Verify if the processing batch’s timestamp matches the expected batch type by spanBatchTime at BatchQueue.
    • BatchQueue splits a SpanBatch to multiple SingularBatch-es.
    • Leave the rest of the derivation stages as-is by passing SingularBatches.
    • ^ Please refer to the op-node section of Implementation Design Docs for details.
  • Added unit tests.
    • ^ Please refer to the Derivation category in the Test List Sheet for each test’s details.

@ImTei ImTei requested a review from refcell September 18, 2023 12:46
@mergify mergify bot added A-op-batcher Area: op-batcher A-op-chain-ops Area: op-chain-ops A-op-e2e Area: op-e2e A-op-node Area: op-node A-op-program Area: op-program A-pkg-contracts-bedrock Area: packages/contracts-bedrock labels Sep 18, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2023

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

@mergify mergify bot added the S-conflict Status: A conflict is present label Sep 21, 2023
ImTei and others added 3 commits September 22, 2023 10:59
Add L2GenesisSpanBatchTImeOffset into deploy config
Activate SpanBatch hardfork for e2e tests by OP_E2E_USE_SPAN_BATCH env
Define SpanBatch and related types
Rename BatchV1 to SingularBatch
Add unit test cases
@mergify mergify bot removed the S-conflict Status: A conflict is present label Sep 22, 2023
@protolambda protolambda changed the base branch from develop to mirror-tip/span-batch-types September 26, 2023 21:44
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Looks good, left a few comments about testing and edge-cases to harden against. I will do a second review pass later, since the derivation code is quite critical.

op-node/rollup/derive/batch_queue.go Show resolved Hide resolved
op-node/rollup/derive/batch_queue.go Show resolved Hide resolved
op-node/rollup/derive/batch_queue.go Outdated Show resolved Hide resolved
op-node/rollup/derive/batch_queue_test.go Outdated Show resolved Hide resolved
op-node/rollup/derive/batch_queue_test.go Outdated Show resolved Hide resolved
op-node/rollup/derive/batches.go Show resolved Hide resolved
op-node/rollup/derive/batches.go Show resolved Hide resolved
@protolambda protolambda added the M-do-not-merge Meta: Do not merge label Sep 26, 2023
@protolambda
Copy link
Contributor

Adding a do-not-merge label to give others the chance to review as well.

@trianglesphere trianglesphere mentioned this pull request Sep 28, 2023
2 tasks
Rename advanceEpoch() to advanceEpochMaybe()
Set number of calls for L2 client mock and assert them in batch queue tests
@protolambda protolambda changed the base branch from mirror-tip/span-batch-types to develop October 10, 2023 20:57
@protolambda
Copy link
Contributor

Opened #7621 to resolve conflicts with develop

Follow-ups to implement after this PR:

  • [in response to span-batch origin-bits checking]:
    I think this is safe-ish: it's limited by the origin_bits length, and can thus not be infinite.
    But we might end up waiting for a very large span-batch (in terms of covered L1 origins) if one is crafted maliciously like that.
    We may need to harden it with additional checks/limits here. We can do so in a follow-up PR however.
  • The Batch typing improved a little, but we should look if we can clean this up more:
    ideally there are less different types of intermediate batch forms.
  • Proto moved the fix from last PR that filters span-batches upon reading from the batch-reader to the channel-in-reader,
    since the batch-reader no longer has L1 inclusion data (due to the way span-batches work).
    We still want to avoid span-batch code-paths before the hardfork activates however,
    so it's worth checking the L1 data before converting a raw span-batch into a full one.
  • In the batch-checking function we need to review the hardfork activation conditions,
    and update the specs as well, so the span-batches activate at the right moment (w.r.t. L1 inclusion and L2 block time)
    This should be added to the span-batch derivation specs, and then referenced as a feature of Canyon,
    in the superchain-upgrades spec document.

@protolambda
Copy link
Contributor

Closing in favor of #7621

github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2023
Span batch derivation [by Test In Prod, rebased on develop, see #7289]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-batcher Area: op-batcher A-op-chain-ops Area: op-chain-ops A-op-e2e Area: op-e2e A-op-node Area: op-node A-op-program Area: op-program A-pkg-contracts-bedrock Area: packages/contracts-bedrock M-do-not-merge Meta: Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants