feat(L1 follower): adjust to recent CodecV7 and contract changes
#1120
Conversation
…retrieve data from L1
…s submitted in one transaction
…single transactions
…-use-code-from-l1-follower
…1-follower-mode-update-da-codec
…atch within a set of batches
…erifier-use-code-from-l1-follower
…1-follower-mode-update-da-codec
…iven via calldata from commit transaction
jonastheis
changed the title
CodecV7 changesCodecV7 and contract changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
rollup/rollup_sync_service/rollup_sync_service.go (1)
326-351: Consider optimizing batch deletions for large ranges.
If the range is large, look into batching the deletions or using an atomic approach to improve performance and consistency.core/rawdb/accessors_da_syncer.go (2)
12-16: Introduce a dedicated version identifier or docstring for the newly added struct.Currently, this struct is pivotal in storing batch-related metadata, but it lacks a version marker or any comments clarifying the data layout. If future expansions are expected, consider adding a version field or, at minimum, a docstring indicating how it evolves over time to ensure clarity for maintainers.
18-27: Avoid crashing the application on RLP or DB errors.Calling
log.Crithalts the entire process when RLP encoding or DB writes fail. If partial or invalid data is encountered in production, it may be preferable to return an error to the caller or handle it gracefully. Consider refactoring this method to surface errors rather than abruptly terminating.rollup/rollup_sync_service/rollup_sync_service_test.go (4)
164-166: Implement or remove the placeholder.This method panics if called. If tests currently do not rely on it, consider returning a sensible default or removing it until implemented, to avoid accidental panics.
188-190: Implement or remove the placeholder.The
Blocks()method is essential for retrieving partial blocks in multiple test scenarios. Panicking here may cause confusion if the method is ever invoked in future tests. Provide a mock implementation or remove it.
200-202: Implement or remove the placeholder.The setter for
SetParentTotalL1MessagePoppedpanics immediately, which could impede testing of parent-synchronization logic in the future. If no plans exist to use this method, remove it or provide a mock that logs usage.
204-206: Implement or remove the placeholder.Returning a panic in
TotalL1MessagesPopped()suggests the method is incomplete. If test logic eventually calls it, the test will panic. Consider returning zero or a configurable test value.rollup/da_syncer/syncing_pipeline.go (1)
109-119: Ensure fallback for missing metadata.If
ReadDAProcessedBatchMetareturns nil, you initialize a brand new metadata struct usingl1DeploymentBlock - 1. Confirm that starting froml1DeploymentBlock - 1is desired. If prior partial data existed, this approach might skip certain checks or replay steps.rollup/l1/reader.go (4)
19-20: Consider renaming event constants for clarity.
Currently,revertBatchV0EventNameis"RevertBatch"andrevertBatchV7EventNameis"RevertBatch0". This might be confusing or unintentional, especially since the latter is labeled "V7" but references"RevertBatch0".
180-183: Clarify topic ordering.
The ordering of event signatures (CommitBatch,RevertBatchV0,RevertBatchV7,FinalizeBatch) is strict and positional. Consider documenting the order or using a more dynamic approach if further event variations are expected.
253-253: Additional log statement.
Logging"found new CommitBatch event"within this switch block is consistent. Ensure similar log formats for revert and finalize events for consistent debugging.
410-441: Added FetchFinalizeTxDataPostEuclidV2 method.
This mirrors existing fetch logic for finalize events while handling post-Euclid V2 specifics. It would benefit from targeted unit tests to confirm the new method’s correctness under edge cases.rollup/da_syncer/da/commitV0.go (2)
24-24: Use unsigned type if messages popped cannot be negative.
l1MessagesPoppedis declared asint. If message counts can never be negative, consider usinguint64to be consistent with other message counters in the codebase.
112-152: Blocks() uses getL1Messages and merges L1/L2 transactions.
- The code merges retrieved L1 transactions (
l1Txs) and L2 transactions from chunks.- The logic increments
curL1TxIndexbut also checksl1TxPointer < len(l1Txs); watch out for out-of-range if the chunk data is malformed.- Overall structure is good, returning error early if messages are unavailable.
rollup/da_syncer/da/commitV7.go (1)
124-136:❓ Verification inconclusive
Consider adding validation for L1 message order.
The TODO comment on line 132 indicates a need for sanity checking L1 messages with previous and post hashes. This is important for ensuring message integrity.
I can help implement the sanity check for L1 messages using previous and post hashes. Would you like me to provide a suggested implementation?
🏁 Script executed:
#!/bin/bash # Check if there are existing implementations of L1 message hash validation rg -A 5 "L1MessageQueueHash|messageQueueHash|validateL1Messages" --type goLength of output: 14472
L1 Message Hash Integrity Validation Suggestion
In the current implementation in
rollup/da_syncer/da/commitV7.go(lines 124–136), we’re appending L1 messages without verifying their order or checking the consistency between previous and post hashes—as noted by the TODO comment. Given that similar validations (e.g., the computation ofPostL1MessageQueueHashand derivation of the previous hash inrollup_sync_service.goand related tests) are already implemented elsewhere in the codebase, it would be beneficial to add a sanity check here. This check should ensure that the order of L1 messages is preserved and that the hash values before and after processing align as expected, thereby maintaining message integrity.Would you like assistance with a proposed implementation of this validation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
core/rawdb/accessors_da_syncer.go(2 hunks)core/rawdb/accessors_rollup_event.go(4 hunks)core/rawdb/accessors_rollup_event_test.go(3 hunks)go.mod(1 hunks)params/version.go(1 hunks)rollup/da_syncer/batch_queue.go(5 hunks)rollup/da_syncer/block_queue.go(2 hunks)rollup/da_syncer/da/calldata_blob_source.go(3 hunks)rollup/da_syncer/da/commitV0.go(5 hunks)rollup/da_syncer/da/commitV7.go(6 hunks)rollup/da_syncer/da/da.go(1 hunks)rollup/da_syncer/da/revert.go(2 hunks)rollup/da_syncer/da_queue.go(2 hunks)rollup/da_syncer/da_syncer.go(2 hunks)rollup/da_syncer/l1_message_queue_height_finder.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(2 hunks)rollup/l1/abi.go(5 hunks)rollup/l1/abi_test.go(2 hunks)rollup/l1/reader.go(9 hunks)rollup/rollup_sync_service/rollup_sync_service.go(8 hunks)rollup/rollup_sync_service/rollup_sync_service_test.go(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (70)
go.mod (1)
54-54: Verify compatibility with the updated da-codec dependencyThe dependency on
github.com/scroll-tech/da-codechas been updated to a newer version. This update is likely related to supporting the changes inCodecV7mentioned in the PR objectives.Please ensure all tests pass with this updated dependency to confirm compatibility with the renamed variables and new features.
core/rawdb/accessors_rollup_event_test.go (1)
185-185: Field rename correctly appliedThe field has been consistently renamed from
LastL1MessageQueueHashtoPostL1MessageQueueHashthroughout the tests, aligning with the variable renaming mentioned in the PR objectives.Also applies to: 193-193, 240-240, 285-285
rollup/da_syncer/da_syncer.go (1)
36-37: Improved log visibilityChanging these log levels from Debug to Info enhances visibility of important synchronization errors, which is a good improvement.
Also applies to: 39-40
rollup/da_syncer/da/revert.go (1)
8-8: Interface update supports the new RevertEvent featureChanging from a specific event type (
*l1.RevertBatchEvent) to the more generall1.RollupEventinterface enables support for the newRevertEvent(startBatchIndex,endBatchIndex)feature mentioned in the PR objectives.This change allows the RevertBatch struct to work with different types of revert events, improving flexibility of the event handling system.
Also applies to: 11-11
rollup/da_syncer/block_queue.go (3)
7-7: No concerns about the new import.
38-38: Streamlined retrieval of blocks from the next batch.
This refactoring clarifies the type usage by directly assigning the result toentryWithBlocks.
43-45: Improved error handling for block retrieval.
The revised error message provides clearer context. Good job.rollup/rollup_sync_service/rollup_sync_service.go (7)
241-243: Appropriate error wrapping in revert event handling.
Including the batch index in the error provides valuable debugging information.
409-409: Empty hash usage for PostL1MessageQueueHash is standard.
This makes sense as the default for an initial batch where no prior queue state exists.
418-419: Clear and concise CodecV7 documentation.
Nicely explains howPrevL1MessageQueueHashis carried over from the previous batch.
424-424: Variable naming for final queue hash is unambiguous.
lastL1MessageQueueHashdistinctly captures the final post-state.
431-437: Validate parentCommittedBatchMeta before using.
If there’s any scenario with a missing parent, ensure a nil check to avoid panics.
451-451: Accumulating L1 messages aligns with CodecV7 changes.
Properly applies message queue logic from the blocks.
460-460: Storing the computed PostL1MessageQueueHash.
Aligns correctly with the new continuous hashing approach.rollup/l1/abi_test.go (4)
15-18: Verified the updated event signatures.
The expected hashes for CommitBatch, RevertBatch (v0, v7), and FinalizeBatch match correctly.
23-23: New finishMockBatchIndex variable improves test coverage.
Helps test multi-batch revert scenarios thoroughly.
47-57: Refined RevertBatch V0 event checks.
Renaming torevertBatchV0EventNameaccurately reflects the new ABI convention.
58-81: Expanded tests for RevertBatch V7 event.
Tests both identical and different start/finish indices for robust coverage.rollup/da_syncer/da/calldata_blob_source.go (4)
147-147: Good improvement to explicit event type handlingYour change to handle specific event types (
l1.RevertEventV0Typeandl1.RevertEventV7Type) rather than using a generic type improves code clarity and makes the event handling more precise.
155-155: Simplified revert batch creationThe code now directly passes the rollup event to
NewRevertBatchwithout type assertion, making the implementation cleaner and safer.
232-240: Enhanced readability with named constantsUsing explicit constants (
encoding.CodecV0,encoding.CodecV1, etc.) instead of numeric literals improves code readability and maintainability.
262-269: Strong data consistency check for newer codec versionsAdding this sanity check ensures data consistency for CodecV7 and above by verifying that all batches in the transaction have been successfully processed.
rollup/da_syncer/da/da.go (2)
37-37: Improved error handling in Blocks() methodAdding error return value to the
Blocks()method signature improves error handling by allowing errors to be properly propagated to the caller rather than handling them internally or through panic.
41-43: Enhanced L1 message tracking capabilitiesThe addition of these methods provides better control over L1 message tracking, which aligns with the PR objectives of adjusting to recent
CodecV7changes and supporting the recursive carryover of the parent's total popped L1 messages.rollup/da_syncer/da_queue.go (2)
14-14: Simplified DAQueue implementationRemoving the
initialBatchfield and parameter simplifies the DAQueue implementation, which aligns with the PR objectives of removing theInitialMessageQueueIndexand syncing from genesis.Also applies to: 21-21
82-84: Enhanced Reset method with comprehensive batch metadataThe updated
Resetmethod now uses the more comprehensiveDAProcessedBatchMetastruct instead of just a height value, providing better batch metadata handling and supporting the L1 follower mode's ability to sync from genesis.core/rawdb/accessors_rollup_event.go (3)
28-28: Improved field naming clarityRenaming from
LastL1MessageQueueHashtoPostL1MessageQueueHashimproves clarity, as it better reflects that this is the state of the message queue hash after the batch is processed.
173-173: Consistent field renaming in serialization/deserializationThe field name changes in these functions ensure consistent handling of the renamed field throughout the serialization and deserialization process.
Also applies to: 205-205, 217-217
41-42: Verify field naming consistencyWhile the field in
CommittedBatchMetawas renamed toPostL1MessageQueueHash, the field incommittedBatchMetaV7remains asLastL1MessageQueueHash. This might be intentional for backward compatibility with stored data, but consider documenting this decision or updating for consistency.core/rawdb/accessors_da_syncer.go (1)
29-63: Confirm default fallback logic for legacy data.When the code fails to decode the new format, it falls back to reading only a single
uint64. It then setsBatchIndexto zero, which might force repeated replays if older data persists. Verify that this approach meets expectations for older node upgrades and that overwritten or older data won't cause unintended reprocessing.rollup/da_syncer/syncing_pipeline.go (2)
82-107: Validate recovery mode logic.In recovery mode, a new
DAProcessedBatchMetais created based on initial L1 block and batch. Ensure these fields align with the actual state in L1 logs and that you are not unintentionally overwriting valid existing metadata if this function is repeatedly invoked.
281-298: Check for negative or zero-based L1 block resets.The reset calculation subtracts
100 * resetCounterfromL1BlockNumberif it’s greater thanamount. Consider verifying that repeated resets do not push the L1 block below zero or other unexpected boundaries. Additionally, review whether theBatchIndexshould be decremented in sync with the L1 block rollback to avoid potential mismatch.rollup/l1/reader.go (6)
36-37: New fields for separating event signatures look good.
Having distinct fields for V0 and V7 revert events is clear and follows the separation of concerns for each event version.
65-66: Verify existence of ABI events.
Ensure that"RevertBatch"and"RevertBatch0"events actually exist in the updated ABI. If the contract’s event name for V7 differs, these references must be updated.
178-178: Topic slice capacity expanded to 4.
Expanding the array to four separate event signatures is correct for handling multiple revert events.
262-275: RevertBatchEventV0 unpack logic.
Unpacking intoRevertBatchEventV0Unpackedand then intoRevertBatchEventV0properly follows the new approach. No issues spotted.
276-288: Handle range-based reverts for V7.
StoringstartBatchIndexandfinishBatchIndexcleanly supports reverting multiple batches. Double-check downstream usage to ensure partial reverts aren’t incorrectly processed as a single index.
396-400: Support for commitBatchesV7 method.
The new condition forcommitBatchesV7MethodNameand associated argument parsing looks correctly integrated.rollup/da_syncer/da/commitV0.go (3)
19-20: Database reference in struct.
Addingdb ethdb.Databaseensures direct retrieval of needed data. It can simplify logic but be mindful of establishing clear ownership and usage patterns to avoid concurrency issues if other goroutines access the same DB.
57-61: Constructing CommitBatchDAV0 with DB and L1 messages popped.
This cleanly initializes the new fields. Verify thatgetTotalMessagesPoppedFromChunks(decodedChunks)correctly handles large sums without overflow (though unlikely in typical scenarios).
154-164: Additional message-popped methods.
The setter is a no-op, which is documented by a comment—makes sense for V0. The new getters for total and per-batch L1 messages popped align well with the design.rollup/l1/abi.go (10)
27-27: Updated ScrollChain ABI definition.
Contains constructor, function, and event updates, including multiple RevertBatch definitions. Ensure the changes match the on-chain contract’s actual ABI.
38-41: New RevertEvent constants.
DefiningRevertEventV0TypeandRevertEventV7Typeclarifies event handling across versions.
47-49: New commit methods.
commitBatchesV7MethodNameandfinalizeBundlePostEuclidV2MethodNameexpand functionality. Confirm these match the contracts to avoid mismatch.
110-115: Separating RevertBatchEventV0Unpacked from RevertBatchEventV0.
This separation helps keep the unpacking logic separate from the final event struct, aiding clarity.
148-152: New RevertBatchEventV7Unpacked fields.
StartBatchIndexandFinishBatchIndexeffectively track batch ranges. No concerns here.
153-164: RevertBatchEventV7 for multi-batch revert.
Provides typed accessors for start/finish batch indices.BatchHash()returns an empty hash, indicating it’s inapplicable. This is consistent.
293-293: Added BlobHashes field to CommitBatchArgs.
ExtendingCommitBatchArgswithBlobHashesis consistent with the system’s approach of passing relevant data in one struct.
297-298: ParentBatchHash and LastBatchHash fields for commit operations.
Including these multi-batch references is logical for the new commit path in V7.
321-333: newCommitBatchArgsFromCommitBatchesV7 function.
Populates the new fields (Version,ParentBatchHash,LastBatchHash) from the V7 commit method. Ensure these fields receive valid data from upstream invocation.
349-365: FinalizeBatchArgs structured for post-Euclid V2 finalize.
The fields align with expected finalize data: batch header, post state root, withdraw root, etc. Confirm consistent usage inFetchFinalizeTxDataPostEuclidV2.rollup/da_syncer/l1_message_queue_height_finder.go (6)
13-18: Good implementation of the L1MessageQueueHeightFinder struct.The struct is well-designed with clear responsibilities and appropriate fields. It properly encapsulates the logic for finding the L1 message queue height.
19-30: Constructor implementation looks good.The constructor properly initializes the L1MessageQueueHeightFinder and handles error cases when creating the calldata blob source.
32-37: Comprehensive documentation for the method.The comments explaining the process for finding the L1 message queue height are clear and informative, which will aid future developers in understanding this complex operation.
103-127: Robust revert event handling implementation.The handleRevertEvent method correctly handles both RevertEventV0Type (single batch revert) and RevertEventV7Type (range of batches revert) with proper type assertions and error handling. The implementation is thorough and includes detailed error messages.
88-98:✅ Verification successful
Verify that L1 message queue height calculation is correct.
This calculation works backwards from the finalized bundle's last processed message queue index by subtracting the L1 messages popped in each batch. This approach depends on accurate L1MessagesPoppedInBatch values. Ensure this logic aligns with how these values are tracked throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for usages of L1MessagesPoppedInBatch to ensure consistency rg -A 3 "L1MessagesPoppedInBatch" --type go # Check how TotalL1MessagesPopped is used elsewhere rg -A 3 "TotalL1MessagesPopped" --type goLength of output: 6366
L1 Message Queue Height Calculation Verified
The reverse subtraction logic in
rollup/da_syncer/l1_message_queue_height_finder.go(lines 88–98) correctly subtracts each batch's popped messages from the overall L1 message queue height. Our verification confirms that the implementation ofL1MessagesPoppedInBatch()is consistent across its usage in commit batches (e.g., incommitV0.goandcommitV7.go), and its behavior aligns with howTotalL1MessagesPoppedis computed elsewhere in the codebase. No inconsistencies were found in how these values are tracked.No further changes are required.
38-101:✅ Verification successful
Ensure error handling is comprehensive for edge cases.
The FindL1MessageQueueHeight method has a well-structured approach to calculating the message queue height. However, there could be additional edge cases to consider:
- What happens if
finalizedBundleis null but we've processed all available data entries?- What if the bundle contains target batch but not all required batches for calculation?
Consider adding additional error handling or logging to make troubleshooting easier in these scenarios.
🏁 Script executed:
#!/bin/bash # Check if there's any existing error handling for these edge cases elsewhere in the codebase # Look for error handling related to finding finalized bundles rg -A 5 -B 5 "failed to find the bundle" --type go # Check if there are retry mechanisms elsewhere for handling missing batches rg -A 10 "batch.*not found" --type goLength of output: 2047
Action Required: Clarification on Error Handling and Optional Logging
The error handling for both the missing finalized bundle and absent batches is already implemented with clear messages:
- A null
finalizedBundlereturns an error:
"failed to find the bundle in which the target batch was finalized"- A missing batch condition is handled by checking for the batch in the map (with an error like
"batch %d not found"), and an additional check ensures the L1 messages popped are within expectation.These error messages provide sufficient feedback for troubleshooting the identified edge cases. If further operational diagnostics are desired, consider integrating structured logging to capture more context (e.g., targeted batch indices, input values) alongside the errors. This is optional and based on how you plan to monitor production issues.
rollup/da_syncer/batch_queue.go (8)
23-23: Good addition of previousBatch field.Adding the previousBatch field to track the last processed batch's metadata is a good design choice that facilitates batch processing continuity, especially after restarts.
26-35: Constructor properly updated for the new field.The constructor correctly initializes the previousBatch field and uses it to set the lastFinalizedBatchIndex.
58-60: Improved error handling for revert events.The introduction of a dedicated handleRevertEvent method with proper error propagation improves the robustness of the code when dealing with revert events.
96-120: Well-structured revert event handling.The handleRevertEvent method effectively handles different types of revert events with proper type assertions and error handling. The V7 revert event handling correctly implements the range-based batch deletion.
122-132: Improved batch deletion with return status.Updating the deleteBatch method to return a boolean indicating success/failure is a good improvement that allows calling methods to make decisions based on the result.
136-169: Well-implemented batch processing and metadata management.The processAndDeleteBatch method effectively:
- Carries forward the total L1 messages from previous batch
- Persists the completed batch metadata to the database
- Updates the previousBatch reference for the next cycle
- Includes helpful logging
This comprehensive approach ensures batch processing continuity and recoverability.
171-177: Reset method properly updated.The Reset method is correctly updated to handle the previousBatch field and propagate the reset to the DAQueue.
148-151:❓ Verification inconclusive
Consider the implications of skipping non-sequential batches.
The code skips a batch if it's not sequential to the previous batch (previousBatch.BatchIndex + 1 != entryWithBlocks.BatchIndex()). While this is a valid sanity check, there could be legitimate reasons for gaps in batch indices, especially after reverts. Consider whether this could lead to unintended consequences in certain scenarios.
🏁 Script executed:
#!/bin/bash # Check for other places where batch sequence is validated rg -A 5 -B 5 "batch.*sequence|sequential batch" --type go # Look for any logic that might handle batch gaps rg -A 5 -B 5 "gap.*batch|batch.*gap" --type goLength of output: 2280
Attention: Validate Batch Sequence Handling in
rollup/da_syncer/batch_queue.goThe current check ensures that a new batch’s index is exactly one greater than that of the previous batch:
if bq.previousBatch.BatchIndex > 0 && bq.previousBatch.BatchIndex+1 != entryWithBlocks.BatchIndex() { log.Info("BatchQueue: skipping batch ", "currentBatch", entryWithBlocks.BatchIndex(), "previousBatch", bq.previousBatch.BatchIndex) return nil }While this helps catch unexpected differences, it assumes that any non-sequential batch is an error. However, there may be valid scenarios (such as after reverts) where gaps in batch indices are intentional. Our search through the codebase did not uncover complementary logic that accommodates legitimate batch index gaps.
Please verify that:
- Non-sequential batch indices due to reverts or other valid operations are either handled elsewhere or should be accounted for here.
- Skipping batches in these scenarios will not inadvertently discard necessary state information.
rollup/da_syncer/da/commitV7.go (5)
22-34: Improved CommitBatchDAV7 struct with better state management.The struct has been refactored to remove unnecessary fields and add focused fields that better represent the state. Using blobPayload to encapsulate block and transaction data is a good design choice.
76-84: Constructor updated to align with new state management.The constructor now properly initializes the new fields and calculates l1MessagesPopped from the blob payload, which is consistent with the overall changes to batch management.
116-151: Enhanced Blocks method with improved error handling.The Blocks method now properly handles errors when fetching L1 messages and uses the parentTotalL1MessagePopped as the initial index, which aligns with the overall message queue height tracking approach.
153-163: Well-designed methods for L1 message management.The new methods SetParentTotalL1MessagePopped, TotalL1MessagesPopped, and L1MessagesPoppedInBatch provide a clear and consistent interface for managing L1 message counts. This is essential for the batch processing pipeline.
209-217: Effective utility function for counting L1 messages.The getL1MessagesPoppedFromBlocks function is simple but necessary for calculating the total number of L1 messages popped from blocks. Its implementation is straightforward and efficient.
go.mod
Outdated
| @@ -51,7 +51,7 @@ require ( | |||
| github.com/prometheus/tsdb v0.7.1 | |||
| github.com/rjeczalik/notify v0.9.1 | |||
| github.com/rs/cors v1.7.0 | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250210041951-d028c537b995 | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250219115141-f9608ed9904d | |||
There was a problem hiding this comment.
Should we change it to support set code transactions? i.e. latest commit in da-codec's main branch.
| } | ||
|
|
||
| bq.deleteBatch(revertBatch.BatchIndex().Uint64()) | ||
| case l1.RevertEventV7Type: |
There was a problem hiding this comment.
not related to this PR, but would this event affect the handling logic of bridge-history and chain-monitor? worth taking a look.
|
|
||
| // We can simply set only the L1BlockNumber because carrying forward the totalL1MessagesPopped is not required before EuclidV2 (CodecV7) | ||
| // (the parentTotalL1MessagePopped is given via the parentBatchHeader). | ||
| // Nodes need to update to the new version to be able to continue syncing after EuclidV2 (CodecV7). Therefore, |
There was a problem hiding this comment.
What's the behavior if a node is not updated? Fail at the EuclidV2 transition, but then recover after the node is upgraded?
rollup/da_syncer/batch_queue.go
Outdated
| @@ -66,15 +73,16 @@ func (bq *BatchQueue) NextBatch(ctx context.Context) (da.Entry, error) { | |||
| } | |||
|
|
|||
| // getFinalizedBatch returns next finalized batch if there is available | |||
| func (bq *BatchQueue) getFinalizedBatch() da.Entry { | |||
| func (bq *BatchQueue) getFinalizedBatch() da.EntryWithBlocks { | |||
There was a problem hiding this comment.
A bit counter-intuitive that a get* function has side effects
rollup/da_syncer/batch_queue.go
Outdated
| // processAndDeleteBatch deletes data committed in the batch from map, because this batch is reverted or finalized | ||
| // updates DASyncedL1BlockNumber | ||
| func (bq *BatchQueue) processAndDeleteBatch(batch da.Entry) da.EntryWithBlocks { |
There was a problem hiding this comment.
Seems like it's only called for finalized batches, not for reverts, is that intentional? Seems like for reverts we don't call WriteDAProcessedBatchMeta.
There was a problem hiding this comment.
yeah the comment was outdated, it was used before but not anymore.
|
|
||
| // sanity check that the next batch is the one we expect | ||
| if bq.previousBatch.BatchIndex > 0 && bq.previousBatch.BatchIndex+1 != entryWithBlocks.BatchIndex() { | ||
| log.Info("BatchQueue: skipping batch ", "currentBatch", entryWithBlocks.BatchIndex(), "previousBatch", bq.previousBatch.BatchIndex) |
There was a problem hiding this comment.
Would this happen? If not, why is this Info?
There was a problem hiding this comment.
this might happen if we specify a wrong batch in recovery mode for example
|
|
||
| dataSourceFactory *DataSourceFactory | ||
| dataSource DataSource | ||
| da da.Entries | ||
| } | ||
|
|
||
| func NewDAQueue(l1height uint64, initialBatch uint64, dataSourceFactory *DataSourceFactory) *DAQueue { |
There was a problem hiding this comment.
Recovery doesn't use initialBatch anymore?
There was a problem hiding this comment.
this is now handled via
go-ethereum/rollup/da_syncer/batch_queue.go
Line 147 in ee53754
| batches := make(map[uint64]da.EntryWithBlocks) | ||
|
|
||
| // 1. find bundle in which target batch was finalized | ||
| daEntries, err := f.calldataBlobSource.NextData() |
There was a problem hiding this comment.
Why do we expect to find the correct finalize event after one invocation of NextData here? Is it because we make sure to pass the correct l1height1?
There was a problem hiding this comment.
you are right, I totally forgot the loop around it :D
|
|
||
| // 3. with this information we can calculate the L1 message queue height for target batch: for each batch from last finalized batch to the target batch subtract L1 messages popped in the batch from L1 message queue height | ||
| lastBatchInBundle := finalizedBundle.BatchIndex() | ||
| l1MessageQueueHeight := args.LastProcessedMessageQueueIndex.Uint64() |
There was a problem hiding this comment.
Need to update to totalMessagesPoppedAfter as per yesterday's change.
| // 1. find bundle in which target batch was finalized | ||
| // 2. fetch the tx of the bundle to get the height of the L1 message queue after the bundle | ||
| // 3. with this information we can calculate the L1 message count for each batch from last finalized bundle to the target batch. | ||
| func (f *L1MessageQueueHeightFinder) FindL1MessageQueueHeight(targetBatch uint64) (uint64, error) { |
There was a problem hiding this comment.
I find "message queue height" a bit vague here, easy to misunderstand. Maybe better to use "message queue height before batch" or change to "total l1 messages popped before"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/da_syncer/syncing_pipeline.go (1)
283-302: Enhanced reset functionality with metadata persistenceThe reset method has been properly updated to work with the new metadata structure. It reads the current state from the database, adjusts the L1 block number based on the reset counter, and persists the changes back to the database.
Consider extracting the magic number 100 in
amount := 100 * uint64(resetCounter)to a named constant for better readability and maintainability.+const resetBlockNumberStep = 100 func (s *SyncingPipeline) reset(resetCounter int) { - amount := 100 * uint64(resetCounter) + amount := resetBlockNumberStep * uint64(resetCounter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/da_syncer/da/calldata_blob_source.go(3 hunks)rollup/da_syncer/syncing_pipeline.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/da_syncer/da/calldata_blob_source.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
rollup/da_syncer/syncing_pipeline.go (4)
82-82: Improved state tracking with more comprehensive metadataThe change from a simple
initialL1Blockto a more structuredlastProcessedBatchMetaprovides better context and tracking of the syncing state, including batch index, L1 block number, and total L1 messages popped.
91-105: Well-structured recovery mode with L1MessageQueueHeightFinderThe implementation correctly handles the recovery mode by using the L1MessageQueueHeightFinder to determine the L1 message queue height before the initial batch. This aligns with the PR objective of operating without InitialMessageQueueIndex by recursively carrying over the parent's total popped L1 messages.
109-122: Robust initialization in non-recovery modeThe code properly handles the initialization in non-recovery mode by reading the existing metadata from the database or creating a default entry with appropriate values. The approach ensures continuity of operation and persistence of state across restarts.
125-126: Updated queue initialization with metadataThe DAQueue and BatchQueue are now correctly initialized with the fields from lastProcessedBatchMeta, providing them with the necessary context about the batch processing state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
rollup/da_syncer/batch_queue.go (6)
10-10: Consider documenting import usage
The newly added imports forlogandl1are clear in context. However, adding a brief comment or docstring where these are used may help future maintainers locate their usage more quickly, especially given the multiple logging approaches and event definitions in the codebase.Also applies to: 12-12
38-39: Clarify the return ofda.EntryWithBlocks
The method signature ofNextBatchnow returnsda.EntryWithBlocksinstead ofda.Entry. Consider updating the method doc comment (line 37) to reflect the new return type and its implications.
66-67: Add final batch check logs
After processing aFinalizeBatchType, you check for a newly finalized batch. Logging a short message here could help operators see that finalization triggered an immediate return of a new batch.
75-76: Loop over potentially multiple finalized batches
nextFinalizedBatchcurrently returns on the first batch in the heap if it meets the finalized index condition. If multiple batches in the heap fall under that condition, you may need a loop to process them all. Confirm whether partial processing is intended.Also applies to: 82-83, 85-85
96-120: Enhance logging inhandleRevertEvent
The revert event logic is sound. However, adding a log statement for each revert action would help diagnose unexpected revert ranges or repeated reverts. You may also want to confirm thatStartBatchIndex <= FinishBatchIndexbefore looping to avoid potential no-op or invalid range errors.
134-167: Clarify batch metadata write order
InprocessAndDeleteBatch, the code writes the oldpreviousBatchto disk before assigning the new batch topreviousBatch. This design might be intentional, but it can be confusing since the new batch is the one being processed. Documenting why the old metadata is persisted just before overwriting would help avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)rollup/da_syncer/batch_queue.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
rollup/da_syncer/batch_queue.go (4)
22-24: Validate or documentpreviousBatchinitialization
DeclaringpreviousBatchin the struct is a good approach for maintaining state. However, ensure that callers never passnilforlastProcessedBatchinNewBatchQueue, or handle that case gracefully. Otherwise, accessingBatchIndexon anilstruct could cause a runtime panic.
58-60: Approach revert events with caution
When a revert event is encountered, the logic delegates handling tohandleRevertEvent. Make sure there's no concurrency gap that could cause new finalizations or commits to slip in during revert processing. If concurrency is possible, consider adding synchronization or queue pausing around this call.
122-132: Check references when deleting the current batch
ThedeleteBatchmethod cleanly removes a batch from bothbatchesandbatchesMap. However, consider verifying that if the current peek or last accessed batch is removed, no stale references remain.
170-175: Confirm expected workflow afterReset
TheResetmethod resets the queue and updatespreviousBatchto the provided metadata. If there's any need to re-queue items based on the new state, ensure that's handled elsewhere. Documenting theResetworkflow can prevent misunderstandings about system state post-reset.
…which would always be decoded to non-existing instead of 0
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/da_syncer/l1_message_queue_height_finder.go (1)
70-105: Consider adding a timeout or maximum iteration limit to prevent potential infinite loops.The
findFinalizedBundlemethod iteratively processes data entries until it finds the bundle containing the target batch. While the implementation is correct, there is no timeout or limit on the number of iterations, which could potentially lead to an infinite loop if the target batch is never found (e.g., due to data corruption or if the target batch was reverted).Consider adding a timeout or maximum iteration limit:
func (f *L1MessageQueueHeightFinder) findFinalizedBundle(targetBatch uint64, batches map[uint64]da.EntryWithBlocks) (*da.FinalizeBatch, error) { + maxIterations := 10000 // Adjust as needed + iterCount := 0 for { + if iterCount >= maxIterations { + return nil, fmt.Errorf("exceeded maximum iterations (%d) while searching for batch %d", maxIterations, targetBatch) + } + iterCount++ // Rest of the method remains unchanged } }Alternatively, you could use a context with timeout:
func (f *L1MessageQueueHeightFinder) findFinalizedBundle(targetBatch uint64, batches map[uint64]da.EntryWithBlocks) (*da.FinalizeBatch, error) { + ctx, cancel := context.WithTimeout(f.ctx, 5*time.Minute) // Adjust timeout as needed + defer cancel() for { + select { + case <-ctx.Done(): + return nil, fmt.Errorf("timeout while searching for batch %d: %w", targetBatch, ctx.Err()) + default: + } // Rest of the method remains unchanged } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
core/rawdb/accessors_l1_message.go(2 hunks)params/version.go(1 hunks)rollup/da_syncer/l1_message_queue_height_finder.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- params/version.go
🔇 Additional comments (9)
core/rawdb/accessors_l1_message.go (2)
386-389: Improved efficiency by using direct binary encodingGood optimization by switching from big.Int conversion to direct binary encoding for storing uint64 values. This approach:
- Reduces memory allocations
- Ensures consistent 8-byte representation
- Makes the code more straightforward
406-409: Improved uint64 retrieval with direct binary decodingGood simplification of the data validation and conversion process. The new approach:
- Simply checks for the expected 8-byte length
- Uses direct binary decoding instead of big.Int conversion
- Maintains backward compatibility by returning nil for invalid lengths
rollup/da_syncer/syncing_pipeline.go (4)
82-107: Well-structured recovery mode implementation with good error handling.The implementation has been updated to use a comprehensive
DAProcessedBatchMetastructure instead of simple block numbers, which provides better tracking of the syncing state. The code properly validates inputs (checking for zero values in lines 84-89) and uses the newL1MessageQueueHeightFinderto determine the L1 message queue height before the initial batch.The error handling is thorough, with descriptive error messages that will help with debugging if issues arise.
109-122: Good implementation of non-recovery mode initialization.The code appropriately reads the last processed batch metadata from the database or initializes it with default values if not found. The initialization logic for the L1 block number is sensible, using the deployment block minus one when available.
The persistence handling with
rawdb.WriteDAProcessedBatchMetaensures data consistency between runs.
125-127: Clean initialization of queue components with metadata.The initialization of
daQueueandbatchQueuenow properly uses the comprehensivelastProcessedBatchMetainstead of individual parameters, which maintains consistency in the syncing state.
283-302: Reset logic properly handles state regression.The reset method now correctly reads the batch metadata from the database, updates the L1 block number based on the reset counter, and persists the changes. The incremental backing up strategy (using
100 * uint64(resetCounter)) provides a good balance between responsiveness and recovery capacity.One thing to consider: if
InitialL1Blockis less thanamount, the subtraction on line 290 could underflow. However, this should be rare in practice since the initial L1 block is typically large and reset counts are small.rollup/da_syncer/l1_message_queue_height_finder.go (3)
13-30: Well-structured implementation of L1MessageQueueHeightFinder.The struct and its constructor are well-designed with appropriate error handling. The constructor properly initializes the calldata blob source and returns meaningful errors if initialization fails.
32-68: Solid implementation of L1 message height calculation logic.The
TotalL1MessagesPoppedBeforemethod implements a clear algorithm for finding the L1 message queue height before a target batch. The code:
- Finds the finalized bundle containing the target batch
- Fetches transaction data to get the message queue height after the bundle
- Calculates backwards to determine the height before the target batch
Error handling is thorough, with checks for missing batches and potential underflows.
107-131: Good implementation of revert event handling with support for batch ranges.The
handleRevertEventmethod properly handles different types of revert events, including the new V7 type that supports reverting a range of batches. The error handling is thorough with descriptive error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile.mockccc (2)
14-15: Recommendation: Use COPY Instead of ADD
Static analysis (Hadolint DL3020) suggests usingCOPYin place ofADDwhen merely copying files or directories. Since there is no need for archive extraction or remote URL retrieval in this context, replacingADD . ./withCOPY . ./would improve clarity and adhere to best practices. Consider applying the following diff:-ADD . ./ +COPY . ./🧰 Tools
🪛 Hadolint (2.12.0)
[error] 14-14: Use COPY instead of ADD for files and folders
(DL3020)
20-21: Clarify Commented Package Installation Command
The previously activeapt-getcommand is now fully commented out. If this command is no longer needed, consider removing it altogether to clean up the Dockerfile. Otherwise, add a clarifying comment explaining why it remains commented (e.g., documentation on dependency requirements for future reference).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dockerignore(1 hunks)Dockerfile.mockccc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
🧰 Additional context used
🪛 Hadolint (2.12.0)
Dockerfile.mockccc
[error] 14-14: Use COPY instead of ADD for files and folders
(DL3020)
🔇 Additional comments (1)
Dockerfile.mockccc (1)
10-12: Approval of Dependency Caching Setup
The modifications setting the working directory (WORKDIR /go-ethereum) and copying thego.modandgo.sumfiles followed by runninggo mod download -xare well implemented. This change improves dependency caching and build efficiency.
1. Purpose or design rationale of this PR
This PR is a collection of changes to adjust to recent changes in
CodecV7and contracts.initialL1MessageQueueHash->prevL1MessageQueueHashandlastL1MessageQueueHash->postL1MessageQueueHashInitialMessageQueueIndexfrom blob payload: in normal operation of the L1 follower mode we assume a sync from genesis. Therefore, we can carry over the parent's total popped L1 messages recursively.RevertEvent(startBatchIndex,endBatchIndex)to revert a range of batchesInitialMessageQueueIndex: implementL1MessageQueueHeightFinderto determine the L1 message queue height for a given target batch from its finalized bundleScrollChainABIcommitBatchesfunction inl1.Readerto be able to read calldata from new commit functions2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit
L1MessageQueueHeightFinderstruct for determining L1 message queue height.handleRevertEventmethod for structured revert event processing.