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.

Sign up for GitHub

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

Jump to bottom

Span batch atomicity #7867

Merged
merged 12 commits into from
Nov 3, 2023
Merged

Span batch atomicity #7867

merged 12 commits into from
Nov 3, 2023

Conversation

ImTei
Copy link
Collaborator

@ImTei ImTei commented Oct 26, 2023

This branch is based on testinprod-io:tip/span-batch-e2e. And the new commit is 44eaa48. Please create a mirror branch and change the base branch to the mirror branch for the better code review

Fix the derivation pipeline to guarantee the atomicity of span batches, as discussed in code reviews.

There are changes in BatchQueue and EngineQueue

  • BatchQueue returns a boolean indicator representing whether the next batch is the last in the span.
  • BatchQueue resets its cached batches derived from span batches, when detecting the batch applying error.
  • EngineQueue manages a new field eq.pendingSafeHead, which means the block that derived from batches, but not consolidated to safe because the span batch is not fully processed yet.
  • In EngineQueue step, if the safe attribute from the attributes queue is not the last batch in the span, EngineQueue does not advance the safe head. only tries to build a block as an unsafe block.
  • In EngineQueue step, if the safe attributes is the last batch of the span, it consolidates the eq.pendingSafeHead into the new safe head.

There are e2e test cases to cover these changes.

  • Check block consolidation without block building occurs atomically. (TestSpanBatchAtomicity_Consolidation)
  • Check block consolidation with block building occurs atomically. (TestSpanBatchAtomicity_ForceAdvance)
  • Check BatchQueue resets when the engine queue failed to process the attrs from the span batch (TestInvalidPayloadInSpanBatch)

@ImTei ImTei requested a review from tynes October 26, 2023 03:50
@ImTei ImTei mentioned this pull request Oct 26, 2023
Closed
2 tasks
@ImTei @protolambda
Add span batch e2e tests
cfcb147
@ImTei @protolambda
Fix flaky e2e test
aec1466
@ImTei @protolambda
Implement span batch atomicity during chain derivation
210492c 
Add pendingSafeHead to engine queue
Engine queue advances node's safe head once the span batch is fully processed
Batch queue resets cached batches derived from span batch when detecting processing error
@ImTei @protolambda
Add comments in batch queue
79c1c5a
@ImTei @protolambda
Rename safeHead to parent
d72dbe3
@protolambda
Copy link
Contributor

Rebased on develop, fixed merge conflicts 👍

ajsutton
ajsutton reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

I think this is looking good generally. I haven't dug into the details of the new tests - figured it was worth getting your thoughts on the approach to e2e testing before digging too much into the details of what new tests we've added.

.circleci/config.yml Show resolved Hide resolved
op-node/rollup/derive/batch_queue.go Show resolved Hide resolved
@ImTei
Copy link
Collaborator Author

ImTei commented Oct 31, 2023

Some existing e2e tests failed due to span batch atomicity changes.

  • TestStopStartBatcher
    • It must wait for the verifier to process the full batch and advance its safe head.
    • Add PendingSafeL2 field to eth.SyncStatus struct and fix the test using this field
  • TestVerifyL2OutputRoot & TestVerifyL2OutputRootDetached
    • Fault proof tests failed because derivation pipeline change is not applied to op-program.
    • Currently, op-program fetches the output root from the head of its chain, but it must specify the block number for now because the chain head can be advanced multiple blocks at once.

@protolambda protolambda mentioned this pull request Nov 2, 2023
Open
ajsutton
ajsutton approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Yep, I'm happy to do those couple of things as a follow up as well.

@protolambda protolambda added this pull request to the merge queue Nov 3, 2023
Merged via the queue into ethereum-optimism:develop with commit 3e890bb Nov 3, 2023
58 checks passed
@protolambda protolambda deleted the tip/span-batch-atomicity branch November 3, 2023 00:29
@ImTei ImTei mentioned this pull request Nov 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@protolambda protolambda protolambda approved these changes

@ajsutton ajsutton ajsutton approved these changes

@trianglesphere trianglesphere Awaiting requested review from trianglesphere

@tynes tynes Awaiting requested review from tynes tynes was automatically assigned from ethereum-optimism/contract-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ImTei @protolambda @ajsutton