Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Facet batch wire format to improve parsing efficiency and simplify the protocol. The changes move from an RLP-nested structure to a streamlined binary header format.
- Simplifies batch structure by using a binary header instead of nested RLP encoding
- Updates role terminology from "FORCED" to "PERMISSIONLESS" for clarity
- Removes target L1 block requirement from batches
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/blob_test_helper.rb | Updates test helper to create batches in new wire format |
| spec/services/facet_block_builder_spec.rb | Updates test batch creation to use new role and remove target block |
| spec/services/facet_batch_parser_spec.rb | Comprehensive test updates for new wire format parsing |
| spec/services/facet_batch_collector_spec.rb | Updates batch payload creation for new format |
| spec/services/blob_aggregation_spec.rb | Updates blob tests to use new wire format |
| spec/services/batch_signature_verifier_spec.rb | New test file for signature verification in wire format |
| spec/models/standard_l2_transaction_signature_recovery_spec.rb | Updates error handling for signature recovery |
| spec/mixed_transaction_types_spec.rb | Updates batch creation and debugging for new format |
| spec/integration/forced_tx_filtering_spec.rb | Updates forced batch test to use permissionless role |
| spec/integration/blob_end_to_end_spec.rb | Updates end-to-end blob tests for new format |
| sequencer/src/l1/monitor.ts | Changes dropped transaction detection from block-based to time-based |
| sequencer/src/db/schema.ts | Removes target_l1_block field from batch schema |
| sequencer/src/batch/maker.ts | Updates batch creation to use new wire format |
| app/services/facet_batch_parser.rb | Major refactor to parse new binary header format |
| app/services/facet_batch_collector.rb | Removes l1_block_number parameter from parser calls |
| app/services/batch_signature_verifier.rb | Simplifies signature verification for new format |
| app/models/standard_l2_transaction.rb | Adds error handling for signature deserialization |
| app/models/parsed_batch.rb | Updates model to remove target_l1_block and extra_data |
| app/models/facet_batch_constants.rb | Adds wire format constants and updates role names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 12345, # l1_block_number | ||
| 0, # l1_tx_index | ||
| FacetBatchConstants::Source::BLOB, | ||
| { versioned_hash: "0x" + "a" * 64 } |
There was a problem hiding this comment.
The parser.parse_payload call is missing the source_details parameter in the method signature but provides it as the fourth argument. According to the updated signature in facet_batch_parser.rb, source_details is now the fourth parameter with a default value.
| { versioned_hash: "0x" + "a" * 64 } | |
| source_details: { versioned_hash: "0x" + "a" * 64 } |
| # Create valid wire format batch | ||
| magic = FacetBatchConstants::MAGIC_PREFIX.to_bin | ||
| chain_id_bytes = [chain_id].pack('Q>') # uint64 big-endian | ||
| version_byte = [FacetBatchConstants::VERSION].pack('C') # uint8 | ||
| role_byte = [role].pack('C') # uint8 | ||
| length_bytes = [rlp_tx_list.length].pack('N') # uint32 big-endian | ||
|
|
||
| batch = magic + chain_id_bytes + version_byte + role_byte + length_bytes + rlp_tx_list | ||
|
|
||
| # Add signature for priority batches | ||
| if role == FacetBatchConstants::Role::PRIORITY && signature | ||
| batch += signature | ||
| end | ||
|
|
||
| batch |
There was a problem hiding this comment.
The signature parameter is optional but the logic only adds it if role is PRIORITY AND signature is provided. This could lead to incomplete priority batches if signature is not provided. Consider either requiring signature for priority batches or handling the case explicitly.
| const role = 0; // 0 = permissionless | ||
| const signature: Hex | undefined = undefined; |
There was a problem hiding this comment.
Hard-coded role value should use the constant from FacetBatchConstants rather than a magic number. Consider defining these role constants in TypeScript or importing them.
app/services/facet_batch_parser.rb
Outdated
| offset = index + FacetBatchConstants::HEADER_SIZE + length | ||
| # Add signature size if priority batch | ||
| role_offset = index + FacetBatchConstants::ROLE_OFFSET | ||
| role = data[role_offset, FacetBatchConstants::ROLE_SIZE].unpack1('C') |
There was a problem hiding this comment.
The code reads the role after already reading the length to skip the batch. Since we're skipping due to wrong chain_id, we don't need to check the role first - we can read it once and use it for both validation and offset calculation.
| offset = index + FacetBatchConstants::HEADER_SIZE + length | |
| # Add signature size if priority batch | |
| role_offset = index + FacetBatchConstants::ROLE_OFFSET | |
| role = data[role_offset, FacetBatchConstants::ROLE_SIZE].unpack1('C') | |
| role_offset = index + FacetBatchConstants::ROLE_OFFSET | |
| role = data[role_offset, FacetBatchConstants::ROLE_SIZE].unpack1('C') | |
| offset = index + FacetBatchConstants::HEADER_SIZE + length |
| unless [0, 1].include?(v_normalised) | ||
| error_class = defined?(Eth::Signature::SignatureError) ? Eth::Signature::SignatureError : StandardError | ||
| raise error_class, "Invalid recovery id #{raw_v}" | ||
| end |
There was a problem hiding this comment.
The error class selection logic is repeated in the signature_error? method. Consider extracting this into a helper method or using a consistent approach for signature error handling.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
spec/services/facet_batch_parser_spec.rb:1
- [nitpick] The wire format construction logic is duplicated across multiple test helper methods. Consider extracting this into a shared test utility method to reduce code duplication and ensure consistency.
require 'rails_helper'
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Build new wire format: [MAGIC:22][CHAIN_ID:8][VERSION:1][ROLE:1][LENGTH:4][RLP_TX_LIST] | ||
| const rlpSize = size(rlpTxList); | ||
| const parts: Hex[] = [ | ||
| this.FACET_MAGIC_PREFIX, // MAGIC: 12 bytes |
There was a problem hiding this comment.
The comment says '12 bytes' on line 153 but the magic prefix is actually 22 bytes based on the ASCII string 'unstoppable sequencing'.
| this.FACET_MAGIC_PREFIX, // MAGIC: 12 bytes | |
| this.FACET_MAGIC_PREFIX, // MAGIC: 22 bytes |
| def signature_error?(error) | ||
| return true if defined?(Eth::Signature::SignatureError) && error.is_a?(Eth::Signature::SignatureError) | ||
|
|
||
| if defined?(Secp256k1) && Secp256k1.const_defined?(:Error) | ||
| return true if error.is_a?(Secp256k1.const_get(:Error)) | ||
| end | ||
|
|
||
| false | ||
| end |
There was a problem hiding this comment.
[nitpick] The signature error detection logic is complex and fragile due to dynamic constant checking. Consider defining specific error classes or using a more explicit error handling approach.
|
bugbot run |
1 participant
Note
Switches batches to a new V2 wire format (magic + chain_id/version/role/length [+signature] + RLP tx list), renames FORCED to PERMISSIONLESS, and updates parser, verifier, sequencer, DB, and tests accordingly.
MAGIC_PREFIXto ASCII "unstoppable sequencing" hex and add header sizing/offsets (MAGIC_SIZE,CHAIN_ID_SIZE,VERSION_SIZE,ROLE_SIZE,LENGTH_SIZE,HEADER_SIZE,SIGNATURE_SIZE).FORCED->PERMISSIONLESS; keepPRIORITY.FacetBatchParser)[MAGIC][CHAIN_ID:8][VERSION:1][ROLE:1][LENGTH:4][RLP_TX_LIST][SIGNATURE?].l1_block_numberparam fromparse_payload.content_hashfromCHAIN_ID+VERSION+ROLE+RLP_TX_LIST(+SIGNATURE); decode RLP tx list; support priority signature presence.ParsedBatch: fields reflect new semantics (notarget_l1_block, updated role method,content_hashmeaning,chain_idfrom header, source excludes events).BatchSignatureVerifier)v(accept 0/1 or 27/28); improved error handling helper.BatchMaker: emit new wire format, include role and optional signature; update content hash; removetarget_l1_blockusage in schema and inserts; update magic prefix.schema.ts): droptarget_l1_blockcolumn usages in inserts and logic.StandardL2Transaction: raiseDecodeErroronSecp256k1::DeserializationErrorduring EIP-1559 recovery.parse_payload(no block number); parse from calldata/blobs only.PERMISSIONLESS; remove reliance ontarget_l1_block; update helpers and integration tests.Written by Cursor Bugbot for commit 9a88f84. This will update automatically on new commits. Configure here.