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 Type, Encoding, and Decoding #7288

Conversation

ImTei
Copy link
Collaborator

@ImTei ImTei commented Sep 18, 2023

Contexts

This PR contains the type, encoding, and decoding code for Span Batch as defined specs.

Please refer to the Encoding Optimization section in Design Docs for the rationale behind the current encoding specs.

This PR has a dependency on the Hardfork Configuration PR.

Changes

  • Added the following types:
    • Batch interface
    • RawSpanBatch
    • SpanBatchElement
    • SpanBatch
    • SpanBatchBuilder
  • Changed the following types:
    • Renamed BatchV1 to SingularBatch.
    • BatchData
  • ^ Please refer to the Type section of Implementation Design Docs for details.
  • Added unit tests.
    • ^ Please refer to the Data Structure category in the Test List Sheet for each test’s details.

@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-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
@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-hard-fork-config September 26, 2023 21:42
@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 before auto-merge by mergify kicks in.

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! There are some stylistic but breaking things I think we can do in a follow-up PR, after the PR stack is wrapped up.

op-node/rollup/derive/batch.go Show resolved Hide resolved
op-node/rollup/derive/batch_tob_test.go Outdated Show resolved Hide resolved
op-node/rollup/derive/span_batch.go Show resolved Hide resolved
op-node/rollup/derive/span_batch.go Show resolved Hide resolved
op-node/rollup/derive/span_batch_test.go Show resolved Hide resolved
op-node/rollup/derive/span_batch_tx.go Show resolved Hide resolved
op-node/rollup/derive/span_batch_txs.go Show resolved Hide resolved
op-node/rollup/derive/batch.go Outdated Show resolved Hide resolved
op-node/rollup/derive/span_batch.go Outdated Show resolved Hide resolved
@trianglesphere trianglesphere mentioned this pull request Sep 28, 2023
2 tasks
@ImTei ImTei changed the base branch from mirror-tip/span-batch-hard-fork-config to develop October 10, 2023 02:28
pcw109550 and others added 2 commits October 10, 2023 11:41
Define SpanBatch and related types
Rename BatchV1 to SingularBatch
Add unit test cases
Fix comments
Check if the span batch is empty in LogContext()
Check chain ID of the protected tx in newSpanBatchTxs()
Write SpanBatchType explicitly when encoding span batch
@ImTei
Copy link
Collaborator Author

ImTei commented Oct 10, 2023

applied simple changes from code reviews and rebased to the latest develop branch.

Other suggestions will be applied in the follow-up PR!

@protolambda
Copy link
Contributor

Follow-ups to implement in later PR:

  • spanBatchTxs has unused EncodeRLP and DecodeRLP. The marshal/unmarshal-binary functions are enough.
  • Some encoding/decoding tests in span_batch_tx_test.go randomize the test data and assert
    if the tx types all occurred at the end. Instead, we can make sub-tests with t.Run() for each tx-type,
    and randomize the same amount of txs per type.
  • The BatchData wrapper around the typed batch shouldn't have an explicit byte field for the type:
    it can already be inferred from the contained batch data. See types.Transaction in geth as example.
  • decodeTyped can be simplified, and the switch-statement should have be more consistent (not mix constants and data[0])
  • SingularBatchType = iota is neat, but being explict about protocol constants is better. iota is only really meant for internal enumerations that you can flexibly add/remove to.
  • Review usage of require vs assert in batch tests
  • parentCheck []byte and l1OriginCheck []byte should be [20]byte types

And opened #7615 to fix a blocking issue: we must not allow span-batches to be decoded successfully before the HF, or we diverge from the actual mainnet specs.

@github-merge-queue github-merge-queue bot merged commit 454c525 into ethereum-optimism:develop Oct 10, 2023
60 checks passed
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-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.

4 participants