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 Unprotected Tx Handling + Logic Error Fix #8458

Conversation

pcw109550
Copy link
Contributor

@pcw109550 pcw109550 commented Dec 5, 2023

Description
This PR fixes two critical errors.

  1. Span batch Unprotected Tx

Span batch did not encode/decode correctly for L2 transactions which is not protected(signed with homestead signer), causing L2 unsafe block reorg.
Example input:

0xf86280830f42418252089400000000000000000000000030bd3402deadbeef01801ca048543f9148389fc09f9c37092a255a9550f6748d6fee3d6e066c92f56199f0e1a02003e0128eece7c6ee59d13a9856964c7252034558fe5551b309f033d4d3efb5

Example output(after upper tx encoded and is written to L1, then decoded by derivation):

0xf86480830f42418252089400000000000000000000000030bd3402deadbeef018082072ea048543f9148389fc09f9c37092a255a9550f6748d6fee3d6e066c92f56199f0e1a02003e0128eece7c6ee59d13a9856964c7252034558fe5551b309f033d4d3efb5

This discrepancy was caused because span batch encoding always assumed that legacy transaction will be always protected. THIS IS WRONG. v value was originally 28 but after enc/dec, it becomes 1838 = 901 * 2 + 35 + 1. A disaster!

Therefore, we do have to store bits whether legacy tx is protected or not. All other tx types(access list, eip1559) is always protected. Added protected_bits field. Please refer to the updated specs for the details. This may affect span batch performance, but not much because it adds single bit per L2 legacy transactions.

  1. Wrong Inequality

Found out while self reviewing. If window is larger than span, must truncate.

Tests

Ensured than non-protected tx are included at span batch.
Added unit tests for new methods.
Updated unit tests to handle newly added field in span batch txs.

@pcw109550 pcw109550 marked this pull request as ready for review December 5, 2023 23:03
@pcw109550 pcw109550 requested a review from clabby December 5, 2023 23:03
Copy link
Contributor

coderabbitai bot commented Dec 5, 2023

Walkthrough

Walkthrough

The changes across various Go source files and markdown specifications involve the introduction of a new protectedBits field and associated logic for handling protected transactions. The signer variable has been renamed to londonSigner to reflect a specific signing algorithm, and transaction generation logic has been updated to include unprotected transactions. Additionally, tests have been updated to accommodate these changes, and new functions and types have been introduced to support the new transaction protection feature.

Changes

File Path Change Summary
op-node/rollup/derive/batch_test.go
op-node/rollup/derive/span_batch.go
op-node/rollup/derive/span_batch_txs.go
op-node/rollup/derive/span_batch_txs_test.go
Introduced protectedBits field, updated logic for handling protected transactions, and added new methods and test functions.
op-service/testutils/random.go Added RandomLegacyTxNotProtected function for generating legacy transactions without explicit signer.
specs/span-batches.md Reorganized txs field structure to include protected_bits, replaced storage of v with y_parity and protected_bit.
op-node/rollup/derive/span_batch_test.go
op-node/rollup/derive/span_batch_tx_test.go
Updated test functions and structs to handle conditional signer assignment.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@tynes
Copy link
Contributor

tynes commented Dec 6, 2023

Good catch!

specs/span-batches.md Outdated Show resolved Hide resolved
@ImTei ImTei force-pushed the tip/span-batch-protected-tx-patch branch from fa45a08 to 6f10382 Compare December 6, 2023 11:49
@ImTei
Copy link
Contributor

ImTei commented Dec 6, 2023

Pushed a commit: improve test cases to cover all tx types including unprotected txs

protolambda and others added 2 commits December 6, 2023 15:03
span-batches: encode bitlists as big-endian integers, use standard Go…
Copy link
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.

LGTM. I like the new tests & the shared bit list code.

@trianglesphere trianglesphere added this pull request to the merge queue Dec 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 6, 2023
@trianglesphere trianglesphere added this pull request to the merge queue Dec 6, 2023
Merged via the queue into ethereum-optimism:develop with commit afbe2e8 Dec 6, 2023
57 checks passed
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.

6 participants