feat: follower node sync from DA #1098
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThe pull request introduces several enhancements to the Ethereum command-line client and related components. New command-line flags for data availability (DA) syncing have been added, including options for enabling syncing and specifying API endpoints. A new exponential backoff implementation is introduced, along with a generic min-heap data structure. Additionally, a comprehensive set of functionalities for managing DA syncing, including batch processing and blob client interactions, has been implemented. The changes also involve modifications to existing methods and the removal of outdated functions, streamlining the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Blockchain
participant Database
User->>CLI: Start geth with new flags
CLI->>Blockchain: Initialize with flags
Blockchain->>Database: Write synced L1 block number
Database-->>Blockchain: Confirm write
Blockchain->>CLI: Confirm initialization
CLI-->>User: Ready for commands
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip You can customize the tone of the review comments and chat replies.Set the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Semgrep found 6
Risk: Affected versions of golang.org/x/net, golang.org/x/net/http2, and net/http are vulnerable to Uncontrolled Resource Consumption. An attacker may cause an HTTP/2 endpoint to read arbitrary amounts of header data by sending an excessive number of CONTINUATION frames. Fix: Upgrade this library to at least version 0.23.0 at go-ethereum/go.mod:106. Reference(s): GHSA-4v7x-pqxf-cx7m, CVE-2023-45288 Ignore this finding from ssc-46663897-ab0c-04dc-126b-07fe2ce42fb2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 40
🧹 Outside diff range and nitpick comments (46)
common/heap.go (1)
8-10: Consider renaming the fieldheapto avoid confusionThe
Heapstruct contains a field namedheap, which might cause confusion with the importedcontainer/heappackage. Consider renaming the field toelementsordatafor better clarity.rollup/rollup_sync_service/rollup_sync_service.go (4)
598-601: Check for empty blocks more gracefullyThe current implementation returns an error if any DA chunk has no blocks. Consider handling this case to either skip empty chunks or provide more context in the error message.
You might adjust the error handling as follows:
-if len(daChunkRawTx.Blocks) == 0 { - return nil, fmt.Errorf("no blocks found in DA chunk, version: %v", codecVersion) +if len(daChunkRawTx.Blocks) == 0 { + log.Warn("Empty DA chunk encountered, skipping", "version", codecVersion) + continue }
309-310: Clarify error message for empty chunk block rangesThe error message
"chunkBlockRanges is empty"may benefit from additional context to aid in debugging.Consider providing more details in the error message:
-return nil, fmt.Errorf("chunkBlockRanges is empty") +return nil, fmt.Errorf("chunkBlockRanges is empty for batch index: %v", batchIndex)
523-525: Consolidate error handling for codec retrievalIn multiple places, you're retrieving the codec and handling errors similarly. Consider extracting this logic into a helper function to reduce code duplication.
Create a helper function like
getCodec(codecVersion):func getCodec(codecVersion encoding.CodecVersion) (encoding.Codec, error) { codec, err := encoding.CodecFromVersion(codecVersion) if err != nil { return nil, fmt.Errorf("unsupported codec version: %v, err: %w", codecVersion, err) } return codec, nil }Then replace the codec retrievals:
-codec, err := encoding.CodecFromVersion(codecVersion) +codec, err := getCodec(codecVersion)
Line range hint
358-410: Simplify and modularizegetCommittedBatchMetafunctionThe
getCommittedBatchMetafunction is quite lengthy and handles multiple concerns. Consider refactoring it to improve readability and maintainability.You could extract the transaction retrieval logic into a separate function:
func (s *RollupSyncService) retrieveTransaction(txHash common.Hash, blockNumber uint64, blockHash common.Hash) (*types.Transaction, error) { tx, _, err := s.client.client.TransactionByHash(s.ctx, txHash) if err != nil { // Existing logic for fetching the block and searching for the transaction } // Rest of the logic }Then update
getCommittedBatchMeta:-tx, _, err := s.client.client.TransactionByHash(s.ctx, vLog.TxHash) +tx, err := s.retrieveTransaction(vLog.TxHash, vLog.BlockNumber, vLog.BlockHash)rollup/da_syncer/da/commitV1.go (1)
68-70: Check for empty slices in addition to nilCurrently, the code checks if
decodedChunksisnil, but it might also be empty. If an empty slice is invalid in this context, consider checking for length:Modify the check to:
- if decodedChunks == nil { + if len(decodedChunks) == 0 {rollup/da_syncer/blob_client/blob_scan_client.go (1)
62-62: Validate the hex string before decodingWhen decoding the blob data, ensure that the string is a valid hex representation and starts with
0xto prevent potential errors.Add a validation check:
+ if !strings.HasPrefix(result.Data, "0x") { + return nil, fmt.Errorf("blob data does not start with '0x'") + } blobBytes, err := hex.DecodeString(result.Data[2:])rollup/da_syncer/batch_queue.go (2)
57-58: Improve error message to includedaEntry.Type()Consider enhancing the error message to include the actual
daEntry.Type()value for better debugging context.Suggested change:
- return nil, fmt.Errorf("unexpected type of daEntry: %T", daEntry) + return nil, fmt.Errorf("unexpected type of daEntry: %T (%v)", daEntry, daEntry.Type())
77-80: Check for duplicate batch indices before adding tobatchesMapEnsure that a batch with the same
BatchIndexdoes not already exist inbatchesMapbefore adding it, to prevent potential duplicates and inconsistencies.Suggested change:
if _, exists := bq.batchesMap.Get(batch.BatchIndex()); exists { // Handle duplicate batch index, e.g., log a warning or skip adding } else { heapElement := bq.batches.Push(batch) bq.batchesMap.Set(batch.BatchIndex(), heapElement) }rollup/da_syncer/blob_client/beacon_node_client.go (2)
75-80: Avoid reusing variablerespfor different HTTP responsesReusing the
respvariable for multiple HTTP responses can lead to unintended side effects if the previous response body is not properly closed. Consider using separate variables or ensure the response body is fully consumed and closed before reassigning.Suggested change:
... respSpec, err := http.Get(specPath) if err != nil { ... } defer respSpec.Body.Close() if respSpec.StatusCode != http.StatusOK { ... } var specResp SpecResp err = json.NewDecoder(respSpec.Body).Decode(&specResp) ...
116-121: Handle potential errors fromdefer resp.Body.Close()When deferring the closure of the response body, consider handling any potential errors that might occur during
resp.Body.Close(), especially in case of network issues.Suggested change:
defer func() { if err := resp.Body.Close(); err != nil { // Handle the error, e.g., log a warning log.Warn("Failed to close response body", "err", err) } }()rollup/da_syncer/syncing_pipeline.go (2)
150-152: Properly handle context cancellation in the main loopIn the main loop's select statement, consider directly returning upon context cancellation without the need for the default case to ensure the pipeline stops promptly.
Suggested change:
select { case <-s.ctx.Done(): return case <-delayedStepCh: // ... }
189-210: Refactor error handling for clarityThe error handling in the main loop's step execution can be refactored to improve readability and maintainability. Consider using a switch statement on the error types.
Suggested change:
switch { case errors.Is(err, serrors.EOFError): // Handle EOFError // ... case errors.Is(err, serrors.TemporaryError): // Handle TemporaryError // ... case errors.Is(err, ErrBlockTooLow): // Handle ErrBlockTooLow // ... case errors.Is(err, ErrBlockTooHigh): // Handle ErrBlockTooHigh // ... case errors.Is(err, context.Canceled): // Handle context cancellation // ... default: // Handle unrecoverable errors // ... }rollup/da_syncer/da/calldata_blob_source.go (3)
46-47: Initializel1Finalizedin the constructorThe
l1Finalizedfield in theCalldataBlobSourcestruct is declared but not initialized in theNewCalldataBlobSourceconstructor. Althoughl1Finalizedis handled when it is zero in theNextData()method, initializing it explicitly in the constructor would enhance code clarity and ensure it's set to a known starting value.Apply this diff to initialize
l1Finalized:func NewCalldataBlobSource(ctx context.Context, l1height uint64, l1Client *rollup_sync_service.L1Client, blobClient blob_client.BlobClient, db ethdb.Database) (*CalldataBlobSource, error) { scrollChainABI, err := rollup_sync_service.ScrollChainMetaData.GetAbi() if err != nil { return nil, fmt.Errorf("failed to get scroll chain abi: %w", err) } return &CalldataBlobSource{ ctx: ctx, l1Client: l1Client, blobClient: blobClient, l1height: l1height, scrollChainABI: scrollChainABI, l1CommitBatchEventSignature: scrollChainABI.Events[commitBatchEventName].ID, l1RevertBatchEventSignature: scrollChainABI.Events[revertBatchEventName].ID, l1FinalizeBatchEventSignature: scrollChainABI.Events[finalizeBatchEventName].ID, db: db, + l1Finalized: 0, // Initialize to appropriate value }, nil }
203-204: Use%xformat specifier for byte slices in error messagesWhen formatting byte slices in error messages, such as
txData[:methodIDLength], using%xwill display the data in hexadecimal format, improving readability.Apply this diff:
return nil, fmt.Errorf("failed to get method by ID, ID: %v, err: %w", txData[:methodIDLength], err) + return nil, fmt.Errorf("failed to get method by ID, ID: %x, err: %w", txData[:methodIDLength], err)
207-208: Use%xto formattxDatain error messagesIn the error message when unpacking transaction data using ABI, formatting
txData(a byte slice) with%xwill provide a hexadecimal representation, enhancing clarity.Apply this diff:
return nil, fmt.Errorf("failed to unpack transaction data using ABI, tx data: %v, err: %w", txData, err) + return nil, fmt.Errorf("failed to unpack transaction data using ABI, tx data: %x, err: %w", txData, err)eth/backend.go (2)
Line range hint
687-687: Handle nilhandlerinSynced()method to prevent panicWhen
EnableDASyncingis true,s.handleris not initialized and remainsnil. TheSynced()method accessess.handler.acceptTxs, which will cause a nil pointer dereference panic ifs.handlerisnil. Modify theSynced()method to check ifs.handlerisnilbefore accessing its fields.Apply this diff:
func (s *Ethereum) Synced() bool { + if s.handler == nil { + return false + } return atomic.LoadUint32(&s.handler.acceptTxs) == 1 }Ensure that any callers of
Synced()can handle afalsereturn value when DA syncing is enabled.
Line range hint
689-689: Handle nilhandlerinDownloader()method to prevent panicSimilarly, when
EnableDASyncingis true ands.handlerisnil, callingDownloader()will cause a nil pointer dereference panic since it accessess.handler.downloader. Update theDownloader()method to handle the case whens.handlerisnil.Apply this diff:
func (s *Ethereum) Downloader() *downloader.Downloader { + if s.handler == nil { + return nil + } return s.handler.downloader }Ensure that callers of
Downloader()handle a potentialnilreturn value.rollup/da_syncer/serrors/errors.go (2)
12-15: Consider documenting global error instancesThe global error variables
TemporaryErrorandEOFErrorare initialized with nil underlying errors. This should be documented to explain the intended usage pattern.Add documentation comments explaining when to use these global instances vs creating new errors:
var ( + // TemporaryError is a sentinel value for temporary errors without additional context TemporaryError = NewTemporaryError(nil) + // EOFError is a sentinel value for EOF conditions without additional context EOFError = NewEOFError(nil) )
51-62: Consider adding error type assertion helperThe
Ismethod implements type checking, but consider adding a helper method to directly check error types without type assertion.Add a helper method like:
+// IsTemporary returns true if err is a temporary error +func IsTemporary(err error) bool { + var syncErr *syncError + if errors.As(err, &syncErr) { + return syncErr.t == temporary + } + return false +}common/backoff/exponential.go (1)
27-43: Consider adding context supportThe backoff implementation could benefit from context support for cancellation.
Consider adding a context-aware method:
+// NextDurationCtx returns the next backoff duration or error if context is cancelled +func (e *Exponential) NextDurationCtx(ctx context.Context) (time.Duration, error) { + select { + case <-ctx.Done(): + return 0, ctx.Err() + default: + return e.NextDuration(), nil + } +}common/backoff/exponential_test.go (2)
28-33: Enhance jitter test coverageThe jitter test could be more comprehensive by:
- Testing multiple attempts to ensure consistent behavior
- Verifying that jitter doesn't cause the duration to exceed the maximum bound
t.Run("Jitter added", func(t *testing.T) { e := NewExponential(1*time.Second, 10*time.Second, 1*time.Second) - duration := e.NextDuration() - require.GreaterOrEqual(t, duration, 1*time.Second) - require.Less(t, duration, 2*time.Second) + // Test multiple attempts to ensure consistent behavior + for i := 0; i < 5; i++ { + duration := e.NextDuration() + require.GreaterOrEqual(t, duration, 1*time.Second, "attempt %d", i) + require.Less(t, duration, 2*time.Second, "attempt %d", i) + // Verify exponential growth with jitter doesn't exceed max + require.LessOrEqual(t, duration, 10*time.Second, "attempt %d", i) + } })
35-38: Add more edge casesConsider adding tests for additional edge cases:
- Zero minimum duration
- Equal min and max durations
- Zero jitter
t.Run("Edge case: min > max", func(t *testing.T) { e := NewExponential(10*time.Second, 5*time.Second, 0) require.Equal(t, 5*time.Second, e.NextDuration()) }) + +t.Run("Edge case: zero min duration", func(t *testing.T) { + e := NewExponential(0, 5*time.Second, 0) + require.Equal(t, 0*time.Second, e.NextDuration()) +}) + +t.Run("Edge case: equal min and max", func(t *testing.T) { + e := NewExponential(5*time.Second, 5*time.Second, 0) + require.Equal(t, 5*time.Second, e.NextDuration()) +})rollup/da_syncer/modes.go (1)
8-13: Define string constants for mode valuesConsider defining string constants for mode values to ensure consistency across String(), MarshalText(), and UnmarshalText() methods.
const ( // L1RPC mode fetches DA from L1RPC L1RPC FetcherMode = iota // Snapshot mode loads DA from snapshot file Snapshot + + // String representations + l1RPCString = "l1rpc" + snapshotString = "snapshot" + unknownString = "unknown" )rollup/da_syncer/da/da.go (3)
9-20: Consider adding String() method for Type enumThe Type enum is well-structured, but adding a String() method would improve logging and debugging capabilities.
+func (t Type) String() string { + switch t { + case CommitBatchV0Type: + return "CommitBatchV0" + case CommitBatchWithBlobType: + return "CommitBatchWithBlob" + case RevertBatchType: + return "RevertBatch" + case FinalizeBatchType: + return "FinalizeBatch" + default: + return "Unknown" + } +}
22-28: Document CompareTo method contractThe CompareTo method lacks documentation about its comparison rules and return values. Consider adding detailed documentation to ensure consistent implementation across different Entry types.
// Entry represents a single DA event (commit, revert, finalize). type Entry interface { Type() Type BatchIndex() uint64 L1BlockNumber() uint64 + // CompareTo compares this entry with another entry. + // Returns: + // -1 if this entry should be ordered before the other + // 0 if entries are equivalent + // 1 if this entry should be ordered after the other CompareTo(Entry) int }
58-69: Add validation and helper methods to PartialBlockThe PartialBlock implementation could be enhanced with:
- Input validation in NewPartialBlock
- Helper methods for common operations (e.g., Hash(), Size())
func NewPartialBlock(partialHeader *PartialHeader, txs types.Transactions) *PartialBlock { + if partialHeader == nil { + panic("partial header cannot be nil") + } return &PartialBlock{ PartialHeader: partialHeader, Transactions: txs, } } + +// Hash returns the block hash +func (b *PartialBlock) Hash() common.Hash { + return b.PartialHeader.ToHeader().Hash() +}rollup/da_syncer/data_source.go (2)
19-25: Document DataSourceFactory fieldsAdd documentation for each field to explain their purpose and requirements.
type DataSourceFactory struct { + // config contains the configuration for data source creation config Config + // genesisConfig contains the chain configuration from genesis genesisConfig *params.ChainConfig + // l1Client is used to interact with L1 chain l1Client *rollup_sync_service.L1Client + // blobClient handles blob data retrieval blobClient blob_client.BlobClient + // db is the database instance for persistence db ethdb.Database }
37-39: Improve error handling in OpenDataSourceConsider adding:
- Validation for l1height
- Error wrapping for better error context
func (ds *DataSourceFactory) OpenDataSource(ctx context.Context, l1height uint64) (DataSource, error) { + if l1height == 0 { + return nil, fmt.Errorf("invalid l1height: %d", l1height) + } - return da.NewCalldataBlobSource(ctx, l1height, ds.l1Client, ds.blobClient, ds.db) + source, err := da.NewCalldataBlobSource(ctx, l1height, ds.l1Client, ds.blobClient, ds.db) + if err != nil { + return nil, fmt.Errorf("failed to create calldata blob source: %w", err) + } + return source, nil }rollup/da_syncer/block_queue.go (2)
10-22: Add parameter validation and optimize slice initializationConsider these improvements:
- Validate batchQueue parameter
- Add initial capacity to blocks slice based on expected batch size
+const defaultBlocksCapacity = 16 // adjust based on typical batch size + func NewBlockQueue(batchQueue *BatchQueue) *BlockQueue { + if batchQueue == nil { + panic("batchQueue cannot be nil") + } return &BlockQueue{ batchQueue: batchQueue, - blocks: make([]*da.PartialBlock, 0), + blocks: make([]*da.PartialBlock, 0, defaultBlocksCapacity), } }
53-56: Optimize memory management in ResetThe current implementation could lead to memory leaks. Consider clearing references and reusing slice capacity.
func (bq *BlockQueue) Reset(height uint64) { - bq.blocks = make([]*da.PartialBlock, 0) + // Clear references to help GC + for i := range bq.blocks { + bq.blocks[i] = nil + } + bq.blocks = bq.blocks[:0] bq.batchQueue.Reset(height) }rollup/da_syncer/da_queue.go (1)
66-70: Consider adding Close method for cleanupThe
DAQueueshould implement aClosemethod to properly clean up resources, especially the data source.+func (dq *DAQueue) Close() error { + if dq.dataSource != nil { + // Assuming DataSource has a Close method + if err := dq.dataSource.Close(); err != nil { + return fmt.Errorf("failed to close data source: %w", err) + } + dq.dataSource = nil + } + dq.da = nil + return nil +}rollup/rollup_sync_service/l1client_test.go (2)
21-26: Consider adding error cases for initializationThe test only covers the happy path for client initialization. Consider adding test cases for invalid chain ID, invalid ABI, or connection failures.
func TestL1Client(t *testing.T) { + t.Run("successful initialization", func(t *testing.T) { ctx := context.Background() mockClient := &mockEthClient{} scrollChainABI, err := ScrollChainMetaData.GetAbi() require.NoError(t, err) l1Client, err := NewL1Client(ctx, mockClient, 11155111, scrollChainAddress, scrollChainABI) require.NoError(t, err) + }) + + t.Run("invalid chain ID", func(t *testing.T) { + ctx := context.Background() + mockClient := &mockEthClient{chainIDErr: errors.New("chain ID error")} + scrollChainABI, err := ScrollChainMetaData.GetAbi() + require.NoError(t, err) + _, err = NewL1Client(ctx, mockClient, 11155111, scrollChainAddress, scrollChainABI) + require.Error(t, err) + })
Line range hint
38-71: Enhance mock client implementationThe mock client implementation could be improved to:
- Support error scenarios
- Allow configurable responses
- Track method calls for verification
type mockEthClient struct { txRLP []byte + logs []types.Log + chainIDErr error + calls struct { + blockNumber int + filterLogs int + } } func (m *mockEthClient) FilterLogs(ctx context.Context, q ethereum.FilterQuery) ([]types.Log, error) { + m.calls.filterLogs++ - return []types.Log{}, nil + return m.logs, nil } func (m *mockEthClient) ChainID(ctx context.Context) (*big.Int, error) { + if m.chainIDErr != nil { + return nil, m.chainIDErr + } return big.NewInt(11155111), nil }rollup/rollup_sync_service/abi_test.go (2)
Line range hint
16-23: Add test cases for additional eventsThe test verifies basic event signatures but could be expanded to cover:
- Error cases with invalid event names
- All events defined in the ABI
- Event parameter validation
func TestEventSignatures(t *testing.T) { scrollChainABI, err := ScrollChainMetaData.GetAbi() if err != nil { t.Fatal("failed to get scroll chain abi", "err", err) } + t.Run("valid events", func(t *testing.T) { assert.Equal(t, crypto.Keccak256Hash([]byte("CommitBatch(uint256,bytes32)")), scrollChainABI.Events["CommitBatch"].ID) assert.Equal(t, crypto.Keccak256Hash([]byte("RevertBatch(uint256,bytes32)")), scrollChainABI.Events["RevertBatch"].ID) assert.Equal(t, crypto.Keccak256Hash([]byte("FinalizeBatch(uint256,bytes32,bytes32,bytes32)")), scrollChainABI.Events["FinalizeBatch"].ID) + }) + + t.Run("invalid event", func(t *testing.T) { + _, exists := scrollChainABI.Events["NonExistentEvent"] + assert.False(t, exists) + }) }
Line range hint
27-77: Add error cases for log unpackingThe test thoroughly covers successful unpacking but should also verify:
- Malformed log data
- Incorrect event name
- Missing required fields
+ { + "MalformedFinalizeBatch", + types.Log{ + Data: []byte{1, 2, 3}, // Invalid data + Topics: []common.Hash{scrollChainABI.Events["FinalizeBatch"].ID, common.BigToHash(mockBatchIndex), mockBatchHash}, + }, + nil, + &L1FinalizeBatchEvent{}, + true, // expectError + }, } for _, tt := range tests { t.Run(tt.eventName, func(t *testing.T) { err := UnpackLog(scrollChainABI, tt.out, tt.eventName, tt.mockLog) + if tt.expectError { + assert.Error(t, err) + return + } assert.NoError(t, err) assert.Equal(t, tt.expected, tt.out) })common/shrinkingmap_test.go (2)
75-100: Consider automating memory leak detectionThe memory profiling tests are currently manual. Consider:
- Automating memory leak detection
- Setting memory thresholds
- Adding benchmarks
+func BenchmarkShrinkingMap(b *testing.B) { + m := NewShrinkingMap[int, int](1000) + + b.Run("write", func(b *testing.B) { + for i := 0; i < b.N; i++ { + m.Set(i, i) + } + }) + + b.Run("read", func(b *testing.B) { + for i := 0; i < b.N; i++ { + m.Get(i) + } + }) +}
129-135: Enhance memory statistics reportingThe
gcAndPrintAllocfunction could provide more detailed memory statistics:
- Peak memory usage
- GC pause times
- Memory growth rate
func gcAndPrintAlloc(prefix string) { runtime.GC() var stats runtime.MemStats runtime.ReadMemStats(&stats) - fmt.Printf(prefix+", Allocated memory %d KiB\n", stats.Alloc/1024) + fmt.Printf("%s:\n"+ + " Allocated: %d KiB\n"+ + " System: %d KiB\n"+ + " GC Cycles: %d\n"+ + " Next GC: %d KiB\n", + prefix, + stats.Alloc/1024, + stats.Sys/1024, + stats.NumGC, + stats.NextGC/1024) }rollup/rollup_sync_service/l1client.go (1)
94-119: Consider adding retry mechanism for transaction fetchingThe
FetchTxDatamethod falls back to fetching the entire block whenTransactionByHashfails. While this is a good fallback strategy, consider implementing a retry mechanism with exponential backoff for the initialTransactionByHashcall to handle temporary network issues.func (c *L1Client) FetchTxData(vLog *types.Log) ([]byte, error) { + // Add retry mechanism with exponential backoff + retries := 3 + for i := 0; i < retries; i++ { tx, _, err := c.client.TransactionByHash(c.ctx, vLog.TxHash) if err != nil { + if i < retries-1 { + time.Sleep(time.Duration(1<<uint(i)) * time.Second) + continue + } log.Debug("failed to get transaction by hash...")} + } else { + return tx.Data(), nil + } // Fallback to fetching the whole block block, err := c.client.BlockByHash(c.ctx...miner/miner.go (1)
79-92: Document the implications of disabling the update loopWhen
daSyncingEnabledis true, the update loop is not started. This change could affect the miner's ability to respond to downloader events. Please add documentation explaining:
- Why the update loop is skipped when DA syncing is enabled
- How this affects the miner's behavior during chain syncing
func New(eth Backend, config *Config, chainConfig *params.ChainConfig, mux *event.TypeMux, engine consensus.Engine, isLocalBlock func(block *types.Block) bool, daSyncingEnabled bool) *Miner { + // When DA syncing is enabled, the miner's update loop is not started because + // <add explanation here about why this is necessary and its implications> miner := &Miner{miner/miner_test.go (1)
279-279: Consider adding test cases for DA syncing enabled scenarioThe test currently only covers the case where
daSyncingEnabledis false. For comprehensive test coverage, consider adding test cases that verify the miner's behavior when DA syncing is enabled.eth/ethconfig/config.go (1)
222-227: Consider adding detailed documentation for DA syncing optionsWhile the fields are self-explanatory, consider adding more detailed documentation comments that explain:
- The purpose and impact of enabling DA syncing
- The relationship between EnableDASyncing and DA config
- Any default values or recommended settings
Example documentation:
// List of peer ids that take part in the shadow-fork ShadowForkPeerIDs []string -// Enable syncing node from DA +// EnableDASyncing when true enables the node to sync from Data Availability layer. +// This must be set to true when using DA syncer features. EnableDASyncing bool -// DA syncer options +// DA contains configuration options for the Data Availability syncer. +// These options are only relevant when EnableDASyncing is true. DA da_syncer.Configcore/rawdb/schema.go (1)
127-129: LGTM! Consider adding a comment for the new key.The new key follows the established naming convention and is appropriately placed with other L1-related keys.
Consider adding a comment describing the key's purpose, similar to other keys in the file:
+ // daSyncedL1BlockNumberKey tracks the latest L1 block number synced by the DA syncer daSyncedL1BlockNumberKey = []byte("LastDASyncedL1BlockNumber")node/node.go (1)
265-272: LGTM! Consider enhancing the log message.The conditional logic for P2P node startup based on DA syncing is well implemented. The error handling is preserved, and the behavior is clearly logged.
Consider making the log message more specific:
- n.log.Info("Peer-to-peer node will not start, because DA syncing is enabled") + n.log.Info("Skipping P2P node startup: DA syncing enabled")rollup/da_syncer/da/revert.go (2)
3-7: Initializel1BlockNumberin the constructor or clarify its usageThe
l1BlockNumberfield inRevertBatchis not initialized in theNewRevertBatchconstructor. If this field is expected to be set later, consider documenting this behavior. Otherwise, initialize it with a default or passed-in value to avoid unintended zero values.Apply this diff to initialize
l1BlockNumberin the constructor:- func NewRevertBatch(batchIndex uint64) *RevertBatch { + func NewRevertBatch(batchIndex uint64, l1BlockNumber uint64) *RevertBatch { return &RevertBatch{ batchIndex: batchIndex, + l1BlockNumber: l1BlockNumber, } }Note: Adjust the constructor calls accordingly in the codebase.
26-33: Add a nil check forotherin theCompareTomethodTo prevent potential panics, consider adding a check to handle the case where
otherisnilbefore accessing itsBatchIndex()method.Apply this diff to add a nil check:
func (r *RevertBatch) CompareTo(other Entry) int { + if other == nil { + return 1 // Decide on appropriate return value when 'other' is nil + } if r.BatchIndex() < other.BatchIndex() { return -1 } else if r.BatchIndex() > other.BatchIndex() { return 1 } return 0 }Note: Decide on the appropriate return value based on the intended logic when
otherisnil.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
cmd/geth/main.go(1 hunks)cmd/utils/flags.go(4 hunks)common/backoff/exponential.go(1 hunks)common/backoff/exponential_test.go(1 hunks)common/heap.go(1 hunks)common/heap_test.go(1 hunks)common/shrinkingmap.go(1 hunks)common/shrinkingmap_test.go(1 hunks)core/blockchain.go(1 hunks)core/rawdb/accessors_da_syncer.go(1 hunks)core/rawdb/accessors_rollup_event.go(0 hunks)core/rawdb/accessors_rollup_event_test.go(0 hunks)core/rawdb/schema.go(1 hunks)eth/backend.go(9 hunks)eth/ethconfig/config.go(2 hunks)go.mod(4 hunks)miner/miner.go(1 hunks)miner/miner_test.go(1 hunks)miner/scroll_worker.go(2 hunks)miner/scroll_worker_test.go(1 hunks)node/config.go(1 hunks)node/node.go(1 hunks)rollup/da_syncer/batch_queue.go(1 hunks)rollup/da_syncer/blob_client/beacon_node_client.go(1 hunks)rollup/da_syncer/blob_client/blob_client.go(1 hunks)rollup/da_syncer/blob_client/blob_scan_client.go(1 hunks)rollup/da_syncer/blob_client/block_native_client.go(1 hunks)rollup/da_syncer/block_queue.go(1 hunks)rollup/da_syncer/da/calldata_blob_source.go(1 hunks)rollup/da_syncer/da/commitV0.go(1 hunks)rollup/da_syncer/da/commitV1.go(1 hunks)rollup/da_syncer/da/da.go(1 hunks)rollup/da_syncer/da/finalize.go(1 hunks)rollup/da_syncer/da/revert.go(1 hunks)rollup/da_syncer/da_queue.go(1 hunks)rollup/da_syncer/da_syncer.go(1 hunks)rollup/da_syncer/data_source.go(1 hunks)rollup/da_syncer/modes.go(1 hunks)rollup/da_syncer/serrors/errors.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(1 hunks)rollup/rollup_sync_service/abi.go(1 hunks)rollup/rollup_sync_service/abi_test.go(2 hunks)rollup/rollup_sync_service/l1client.go(4 hunks)rollup/rollup_sync_service/l1client_test.go(1 hunks)rollup/rollup_sync_service/rollup_sync_service.go(14 hunks)rollup/rollup_sync_service/rollup_sync_service_test.go(17 hunks)
💤 Files with no reviewable changes (2)
- core/rawdb/accessors_rollup_event_test.go
- core/rawdb/accessors_rollup_event.go
🔇 Additional comments (20)
common/shrinkingmap.go (1)
1-71:
Ensure thread safety for concurrent access
The ShrinkingMap does not include any synchronization mechanisms. If multiple goroutines access this map concurrently, data races may occur. Consider adding synchronization (e.g., a sync.RWMutex) or clearly documenting that ShrinkingMap is not safe for concurrent use.
Verify if ShrinkingMap instances are accessed concurrently:
✅ Verification successful
Based on the analysis of the codebase, I can now provide the final response:
Add documentation about thread safety guarantees
The ShrinkingMap is currently only used in BatchQueue, which shows a single-threaded access pattern through its methods. The BatchQueue processes events sequentially through NextBatch, and there's no evidence of concurrent access in the codebase. However, since this is a utility type in the common package that could be used elsewhere, it should be explicitly documented whether it's thread-safe or not.
- Add a comment to
ShrinkingMapstruct documentation clarifying that it's not safe for concurrent use - Example:
// ShrinkingMap is not safe for concurrent use. If concurrent access is needed, the caller must implement proper synchronization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of ShrinkingMap to check for concurrent access.
# Find all instantiations of ShrinkingMap in Go files
rg 'ShrinkingMap\[.*\]' -g '*.go'
# Look for go routines that might access the map concurrently
rg -A5 'ShrinkingMap\[.*\]' -g '*.go' | rg 'go '
Length of output: 1415
Script:
#!/bin/bash
# Let's check the usage context in batch_queue.go and any potential concurrent access patterns
# Check the full content of batch_queue.go to understand the usage context
cat rollup/da_syncer/batch_queue.go
# Look for goroutine spawning patterns in the same file
rg "go func" rollup/da_syncer/batch_queue.go -A 5
# Look for methods that might be called concurrently
rg "func.*BatchQueue" rollup/da_syncer/batch_queue.go -A 2
Length of output: 3940
Script:
#!/bin/bash
# Let's check how BatchQueue is used in the codebase to determine concurrent access patterns
# Find files that import and use BatchQueue
rg -l "da_syncer.BatchQueue" --type go
# Look for concurrent patterns in files using BatchQueue
rg "da_syncer.BatchQueue" -B 5 -A 10 --type go
# Check if there are any tests that might show concurrent usage
rg -l "BatchQueue.*Test" --type go
Length of output: 122
eth/backend.go (1)
575-579: Verify omission of protocols when DA syncing is enabled
In the Protocols() method, the function returns nil when EnableDASyncing is true, resulting in no network protocols being started. Confirm whether this behavior is intentional because it may prevent the node from participating in essential network activities unrelated to DA syncing.
rollup/da_syncer/da/finalize.go (1)
27-34: Verify comparison logic completeness
The CompareTo method only compares BatchIndex. Consider whether l1BlockNumber should also be part of the comparison logic for complete ordering.
✅ Verification successful
Let me gather more information about the Entry interface and its implementations to understand if L1BlockNumber is a required part of the comparison logic.
Let me check how these entries are used in practice to understand if the ordering by BatchIndex alone is sufficient.
Let me check how these entries are used in the syncer to understand the significance of ordering.
Based on the gathered information, I can now provide a final response:
Comparison by BatchIndex alone is correct
The current implementation of CompareTo comparing only BatchIndex is sufficient because:
- All Entry implementations (CommitBatchDAV0, RevertBatch, FinalizeBatch) follow the same pattern
- While L1BlockNumber is part of the Entry interface, it serves as metadata rather than a sorting key
- BatchIndex provides a unique, sequential ordering which is crucial for maintaining the correct order of operations in the rollup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of CompareTo to understand if the current comparison logic is sufficient
rg -A 5 "CompareTo.*Entry.*int"
Length of output: 1363
Script:
#!/bin/bash
# Search for Entry interface definition and its methods
rg -A 10 "type Entry interface"
# Search for L1BlockNumber usage in the da package
rg "L1BlockNumber" rollup/da_syncer/da/
Length of output: 1188
Script:
#!/bin/bash
# Search for places where entries are stored/sorted
rg -A 5 "sort|Order|Compare" rollup/da_syncer/da/
# Look for any tests that verify the ordering
rg -A 10 "Test.*Compare|Test.*Order|Test.*Sort" rollup/da_syncer/da/
Length of output: 1452
Script:
#!/bin/bash
# Search for usage of these entries in the syncer
rg -A 10 "type.*Syncer" rollup/da_syncer/
# Look for methods that process these entries
rg -A 10 "func.*process.*Entry|func.*handle.*Entry" rollup/da_syncer/
Length of output: 741
go.mod (2)
7-7: Verify compatibility with the updated da-codec version
The update to github.com/scroll-tech/da-codec from v0.1.1 to v0.1.3 is significant for the DA syncing feature. Please ensure all DA-related functionality has been tested with this new version.
Also applies to: 53-53
✅ Verification successful
Update to da-codec appears safe for existing functionality
The codebase uses da-codec's encoding package consistently across DA-related components, primarily for chunk and block data structures. The update is to a specific pre-release version (v0.1.3-0.20241210035500-70810faccc35) and the usage patterns in the code suggest this is a coordinated update that maintains compatibility with existing implementations in:
- rollup/da_syncer/da/: Chunk processing and commit handling
- rollup/rollup_sync_service/: Block validation and batch processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes or migration guides in da-codec
gh api repos/scroll-tech/da-codec/releases | jq -r '.[] | select(.tag_name=="v0.1.3") | .body'
Length of output: 96
Script:
#!/bin/bash
# Let's check the usage of da-codec in the codebase to understand the impact
rg -l "da-codec" .
# Search for imports of da-codec
rg "github.com/scroll-tech/da-codec" .
# Check git history for da-codec version changes
git log -p -- go.mod | grep -A 5 -B 5 "scroll-tech/da-codec"
Length of output: 1252
Script:
#!/bin/bash
# Let's examine the specific usage of da-codec encoding package to understand potential breaking changes
ast-grep --pattern 'encoding.$_' ./rollup/da_syncer/da/
ast-grep --pattern 'encoding.$_' ./rollup/rollup_sync_service/
# Check the specific version being used (as it appears to be a pre-release)
rg "v0.1.3-0.20241210035500-70810faccc35" . -A 2 -B 2
Length of output: 9360
88-88: Verify security implications of the compress library
The addition of github.com/klauspost/compress v1.17.9 as an indirect dependency warrants a security review.
✅ Verification successful
Let me gather more information about how this dependency is being used in the codebase.
Let me try one more verification to check the dependency chain.
No security concerns found with klauspost/compress dependency
The klauspost/compress library is an indirect dependency pulled in through other dependencies. Based on the verification:
- No known security vulnerabilities were found in the GitHub Security Advisory Database
- The library is used indirectly and not imported directly in the codebase
- Version v1.17.9 is a recent stable release
The dependency appears to be safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known vulnerabilities in the compress library
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/klauspost/compress") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 361
Script:
#!/bin/bash
# Check direct imports of the compress library
rg --type go "github.com/klauspost/compress" -l
# Check go.mod to see if this is a transitive dependency
rg "require.*github.com/klauspost/compress" go.mod
# Find which direct dependencies might be pulling this in
go mod graph | grep "github.com/klauspost/compress"
Length of output: 245
Script:
#!/bin/bash
# Check the content of go.mod to understand the dependency chain
cat go.mod
# Look for any references to compression in the Go files
rg -i "compress|decompress" --type go
Length of output: 21617
rollup/rollup_sync_service/l1client.go (2)
30-32: LGTM! Method renames follow Go conventions
The renaming of methods to start with uppercase letters (e.g., NewL1Client, FetchRollupEventsInRange) correctly follows Go's exported method naming conventions.
Also applies to: 58-60
121-149: Add validation for blob hash index
The FetchTxBlobHash method always returns the first blob hash (blobHashes[0]). Consider adding validation or documentation to explain why we always use the first blob hash.
✅ Verification successful
Let me gather more information about the blob transaction handling to understand if there's a specific reason for using the first blob hash.
Let me search for more specific information about the blob transaction structure and commit batch metadata.
Based on the gathered information, I can now generate the final response:
The first blob hash is used because each commit batch contains exactly one blob
Looking at the codebase, we can see that each CommittedBatchMeta consistently contains a single blob hash in its BlobVersionedHashes field, as evidenced by the test cases. The FetchTxBlobHash method is used in the context of processing commit batch events, where each transaction is expected to contain exactly one blob containing the batch data. The validation for non-empty blob hashes is sufficient, as it ensures the transaction contains at least one blob.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of blob hashes to understand if we always expect a single blob
rg -A 5 "BlobHashes\(\)"
Length of output: 1934
Script:
#!/bin/bash
# Search for commit batch related code to understand blob hash usage
ast-grep --pattern 'type CommitBatch struct {
$$$
}'
# Search for blob-related event handling
rg -B 2 -A 5 "CommitBatch.*Event"
Length of output: 8490
Script:
#!/bin/bash
# Search for CommitBatchMeta struct and its usage
ast-grep --pattern 'type CommitBatchMeta struct {
$$$
}'
# Search for blob-related constants or documentation
rg -B 2 -A 5 "BlobVersionedHashes"
Length of output: 21622
miner/miner.go (1)
89-91: Verify impact on existing mining operations
The conditional start of the update goroutine could affect existing mining operations. Please ensure this change doesn't break mining functionality when DA syncing is disabled.
✅ Verification successful
Let me check the test file content to understand the test coverage for mining operations.
Let me check the miner implementation to understand how the update goroutine affects mining operations.
Based on the code analysis and test coverage, I can now provide the final response:
The conditional start of update goroutine is safe and well-tested
The update goroutine is specifically designed to handle downloader events and mining state transitions. The tests in miner_test.go comprehensively verify mining behavior under various scenarios:
TestMinerverifies mining state transitions during sync eventsTestMinerDownloaderFirstFailsensures proper mining behavior when syncing failsTestStartStopMinerconfirms basic mining operations work correctlyTestStartWhileDownloadvalidates mining behavior during downloads
The conditional start based on daSyncingEnabled is safe because:
- When DA syncing is enabled, the miner doesn't need to track downloader events
- When DA syncing is disabled, the update goroutine provides the necessary downloader event handling, which is proven to work through extensive test coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify mining behavior
rg -l "TestMiner" --type go
Length of output: 47
Script:
#!/bin/bash
# Check the content of miner test file
cat miner/miner_test.go
Length of output: 8854
Script:
#!/bin/bash
# Check the miner.go implementation focusing on the update method
cat miner/miner.go
Length of output: 8171
rollup/rollup_sync_service/abi.go (1)
13-14: Verify the impact of exporting ScrollChainMetaData
The change from scrollChainMetaData to ScrollChainMetaData makes this variable public. While this change is necessary for DA syncing integration, ensure that this breaking change is properly documented and that all dependent packages are updated accordingly.
✅ Verification successful
The variable is already being used with its public name
The search results show that ScrollChainMetaData is already being used with its public name across multiple files in the codebase:
rollup/da_syncer/syncing_pipeline.gorollup/da_syncer/da/calldata_blob_source.gorollup/rollup_sync_service/abi_test.gorollup/rollup_sync_service/l1client_test.gorollup/rollup_sync_service/rollup_sync_service.gorollup/rollup_sync_service/rollup_sync_service_test.go
There are no instances of the private scrollChainMetaData name being used, indicating that the variable was already public or the dependent code was updated along with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing usage of the private scrollChainMetaData variable
# that needs to be updated to use the new public name.
rg -l "scrollChainMetaData" --type go
Length of output: 37
Script:
#!/bin/bash
# Let's check for both the old private name and the new public name, and include context
rg -i "scrollchainmetadata" --type go -C 2
# Also check for any imports or usage of this package to understand the impact
rg -l "rollup_sync_service" --type go
Length of output: 12697
eth/ethconfig/config.go (1)
40-40: LGTM!
The import of the da_syncer package is correctly placed and follows the package import conventions.
cmd/geth/main.go (1)
174-178: Verify completeness of DA syncing configuration flags.
The new DA-related flags are properly grouped and placed. Let's verify if all necessary configuration options are covered:
DASyncEnabledFlag: Controls DA syncingDASnapshotFileFlag: Specifies snapshot fileDABlockNativeAPIEndpoint: Native API endpointDABlobScanAPIEndpoint: Blob scan API endpointDABeaconNodeAPIEndpoint: Beacon node API endpoint
Let's verify if these flags cover all DA syncing requirements:
✅ Verification successful
Let me analyze the search results to verify if all necessary DA syncing flags are present. Let me run one more search to check for any DA-related configuration patterns in the codebase.
Based on the search results, I can now generate the final response:
DA syncing configuration flags are complete and properly structured
The DA syncing configuration flags are complete and properly integrated into the codebase:
- The flags are defined in
cmd/utils/flags.gowith proper names and usage descriptions - They map to the corresponding config struct in
rollup/da_syncer/syncing_pipeline.go - The configuration flow is properly handled through the node and eth configs
- All required endpoints are covered:
- Enable/disable flag
- BlobScan API endpoint
- BlockNative API endpoint
- Beacon node API endpoint
- Snapshot file path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any additional DA-related configuration in the codebase
# Search for DA-related configuration patterns
echo "Searching for DA configuration patterns..."
rg -i "da.*config|da.*setting|da.*option" --type go
# Search for DA-related environment variables
echo "Searching for DA environment variables..."
rg -i "da_|da\." --type go
Length of output: 105169
Script:
#!/bin/bash
# Search for DA configuration patterns in the codebase
ast-grep --pattern 'type Config struct {
$$$
}'
# Search for DA-related flags
rg -A 5 "DASyncEnabled|DASnapshotFile|DABlockNativeAPI|DABlobScanAPI|DABeaconNodeAPI" --type go
Length of output: 41257
miner/scroll_worker.go (1)
Line range hint 180-200: LGTM! Clean implementation of DA syncing check.
The addition of the daSyncingEnabled parameter and the early return logic is well implemented. The worker is properly initialized but inactive when DA syncing is enabled, with clear logging.
rollup/rollup_sync_service/rollup_sync_service_test.go (5)
54-54: LGTM! Comprehensive test coverage for all codec versions.
The test updates properly handle the ScrollChainMetaData changes and maintain thorough test coverage across all codec versions (v0-v3).
Also applies to: 113-113, 166-166, 219-219
568-571: LGTM! Proper initialization for codec v0.
The CommittedBatchMeta structure is correctly initialized for codec v0 with appropriate version and nil blob hashes.
628-631: LGTM! Proper initialization for codec v1.
The CommittedBatchMeta structure is correctly initialized for codec v1 with appropriate version and blob hash.
687-690: LGTM! Proper initialization for codec v2.
The CommittedBatchMeta structure is correctly initialized for codec v2 with appropriate version and blob hash.
747-750: LGTM! Proper initialization for codec v3.
The CommittedBatchMeta structure is correctly initialized for codec v3 with appropriate version and blob hash.
cmd/utils/flags.go (2)
874-895: LGTM! The new DA syncing flags are well-defined.
The new flags follow consistent naming conventions and provide clear usage descriptions for configuring the DA syncing feature.
1626-1639: LGTM! The setDA function is well-implemented.
The function follows the established pattern of other configuration functions and properly checks if DA syncing is enabled before setting other configurations.
miner/scroll_worker_test.go (1)
211-211: LGTM! Test updated to match new worker signature.
The test properly updates the worker initialization to include the new DA syncing parameter.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
rollup/da_syncer/da/calldata_blob_source.go (4)
20-21: Add documentation for the block range constantConsider adding a comment explaining the purpose and implications of the
callDataBlobSourceFetchBlockRangeconstant. This would help other developers understand why this specific value was chosen and its impact on performance and resource usage.
50-66: Add input parameter validation in constructorConsider adding validation for input parameters to ensure they are not nil/invalid:
func NewCalldataBlobSource(ctx context.Context, l1height uint64, l1Client *rollup_sync_service.L1Client, blobClient blob_client.BlobClient, db ethdb.Database) (*CalldataBlobSource, error) { + if ctx == nil { + return nil, errors.New("context cannot be nil") + } + if l1Client == nil { + return nil, errors.New("l1Client cannot be nil") + } + if blobClient == nil { + return nil, errors.New("blobClient cannot be nil") + } + if db == nil { + return nil, errors.New("db cannot be nil") + } scrollChainABI, err := rollup_sync_service.ScrollChainMetaData.GetAbi()
119-119: Standardize log messages across event typesThe log messages for different events are inconsistent in their format and content. Consider standardizing them for better log analysis.
- log.Trace("found new CommitBatch event", "batch index", batchIndex) + log.Trace("Processing event", "type", "CommitBatch", "batch_index", batchIndex) - log.Trace("found new RevertBatchType event", "batch index", batchIndex) + log.Trace("Processing event", "type", "RevertBatch", "batch_index", batchIndex) - log.Trace("found new FinalizeBatchType event", "batch index", event.BatchIndex.Uint64()) + log.Trace("Processing event", "type", "FinalizeBatch", "batch_index", batchIndex)Also applies to: 132-132, 142-142
189-247: Add unit tests for version handlingThe
getCommitBatchDAmethod contains complex logic for handling different versions and methods. Consider adding comprehensive unit tests to ensure all paths are covered.Would you like me to help generate unit tests for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/da_syncer/da/calldata_blob_source.go(1 hunks)
🔇 Additional comments (1)
rollup/da_syncer/da/calldata_blob_source.go (1)
110-111:
Prevent potential panic by checking vLog.Topics length
This is a critical issue that was previously identified and still needs to be addressed.
for _, vLog := range logs {
+ if len(vLog.Topics) == 0 {
+ continue // or handle the error appropriately
+ }
switch vLog.Topics[0] {
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
rollup/da_syncer/da_syncer.go (2)
11-14: Consider enhancing error messages with more contextWhile the error messages are clear, they could be more helpful by including format specifiers for actual values.
- ErrBlockTooLow = fmt.Errorf("block number is too low") - ErrBlockTooHigh = fmt.Errorf("block number is too high") + ErrBlockTooLow = fmt.Errorf("block number %d is too low (current: %d)") + ErrBlockTooHigh = fmt.Errorf("block number %d is too high (current: %d)")
26-49: Consider adding metrics for monitoringAdding metrics would help monitor the syncing progress and performance.
+import "github.com/scroll-tech/go-ethereum/metrics" + func (s *DASyncer) SyncOneBlock(block *da.PartialBlock) error { + syncTimer := metrics.GetOrRegisterTimer("da_syncer/block_sync_time", nil) + defer syncTimer.UpdateSince(time.Now()) + // ... existing code ... + + metrics.GetOrRegisterCounter("da_syncer/blocks_synced", nil).Inc(1) + if err != nil { + metrics.GetOrRegisterCounter("da_syncer/sync_errors", nil).Inc(1) + } return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/da_syncer/da_syncer.go(1 hunks)
🔇 Additional comments (3)
rollup/da_syncer/da_syncer.go (3)
1-9: LGTM: Clean package structure and imports
The package name and imports are well-organized and follow Go conventions.
26-28: Add nil validation for input parameters
The method should validate input parameters to prevent nil pointer dereferences.
func (s *DASyncer) SyncOneBlock(block *da.PartialBlock) error {
+ if block == nil {
+ return fmt.Errorf("nil block parameter")
+ }
+ if block.PartialHeader == nil {
+ return fmt.Errorf("nil partial header")
+ }
currentBlock := s.blockchain.CurrentBlock()39-42: Add parent block validation
The code should validate that the parent block exists before proceeding.
parentBlock := s.blockchain.GetBlockByNumber(currentBlock.Number().Uint64())
+ if parentBlock == nil {
+ return fmt.Errorf("parent block not found at height %d", currentBlock.Number().Uint64())
+ }
if _, err := s.blockchain.BuildAndWriteBlock(parentBlock, block.PartialHeader.ToHeader(), block.Transactions); err != nil {
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
rollup/da_syncer/da/commitV1.go (3)
23-33: Consider refactoring to reduce the number of function parametersThe
NewCommitBatchDAWithBlobfunction has a lengthy list of parameters, which can make the code harder to read and maintain. Consider encapsulating the parameters within a struct or utilizing the builder pattern to improve readability and maintainability.
36-36: Use appropriate format specifier forbatchIndexin error messageIn the error message, use
%dinstead of%vto formatbatchIndex, which is an integer. This enhances clarity and consistency in the error output.Apply this diff:
- return nil, fmt.Errorf("failed to unpack chunks: %v, err: %w", batchIndex, err) + return nil, fmt.Errorf("failed to unpack chunks: %d, err: %w", batchIndex, err)
74-75: Wrap the returned error with additional contextReturning the error directly may make it harder to trace the source of the error. Consider wrapping the error with additional context to improve debuggability.
Apply this diff:
- return nil, err + return nil, fmt.Errorf("failed to create CommitBatchDAV0: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
rollup/da_syncer/batch_queue.go(1 hunks)rollup/da_syncer/blob_client/block_native_client.go(1 hunks)rollup/da_syncer/da/commitV1.go(1 hunks)rollup/da_syncer/da_queue.go(1 hunks)rollup/da_syncer/da_syncer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rollup/da_syncer/da_queue.go
- rollup/da_syncer/blob_client/block_native_client.go
🔇 Additional comments (4)
rollup/da_syncer/da_syncer.go (3)
20-23:
Add validation for nil blockchain parameter in the constructor
Consider adding a check to ensure that the blockchain argument is not nil to prevent potential nil pointer dereferences.
Apply this diff to add the validation:
func NewDASyncer(blockchain *core.BlockChain) *DASyncer {
+ if blockchain == nil {
+ panic("blockchain is required")
+ }
return &DASyncer{
blockchain: blockchain,
}
}27-29:
Add validation for nil block parameter in SyncOneBlock
To prevent potential nil pointer dereferences, validate the block parameter before using it.
Apply this diff to add the validation:
func (s *DASyncer) SyncOneBlock(block *da.PartialBlock) error {
+ if block == nil {
+ return fmt.Errorf("nil block parameter")
+ }
+ if block.PartialHeader == nil {
+ return fmt.Errorf("nil partial header")
+ }
currentBlock := s.blockchain.CurrentBlock()48-50:
Fix typo and potential race condition in progress logging
There's a typo in "blockhain," and multiple calls to CurrentBlock() may lead to inconsistent logging. Store the result in a variable to ensure consistency.
Apply this diff to fix the typo and use the stored currentBlock:
+ currentBlock = s.blockchain.CurrentBlock() // Get the latest state
if s.blockchain.CurrentBlock().Number().Uint64()%1000 == 0 {
- log.Info("L1 sync progress", "blockhain height", s.blockchain.CurrentBlock().Number().Uint64(), "block hash", s.blockchain.CurrentBlock().Hash(), "root", s.blockchain.CurrentBlock().Root())
+ log.Info("L1 sync progress", "blockchain height", currentBlock.Number().Uint64(), "block hash", currentBlock.Hash(), "root", currentBlock.Root())
}rollup/da_syncer/batch_queue.go (1)
93-96: Defer updating DASyncedL1BlockNumber until after batch processing
As indicated by the TODO comment, updating DASyncedL1BlockNumber within deleteBatch may lead to inconsistencies. Consider storing this information only after the batch has been fully processed to ensure data consistency.
| // if DA syncing enabled then we don't create handler | ||
| if s.config.EnableDASyncing { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Could consider enabling some protocols so that e.g. we can send txs through L1 follower nodes in the future.
|
|
||
| // we store here min height of currently loaded batches to be able to start syncing from the same place in case of restart | ||
| // TODO: we should store this information when the batch is done being processed to avoid inconsistencies | ||
| rawdb.WriteDASyncedL1BlockNumber(bq.db, batch.L1BlockNumber()-1) |
There was a problem hiding this comment.
Any plan to adjust this TODO? I'm asking because if we ever run into such inconsistency it's hard to debug/recover. We have a similar issue with l1 message sync that we haven't been able to debug
There was a problem hiding this comment.
There is an inconsistency in the sense that we write the last processed batch here, but we haven't actually processed all blocks from this batch as they are still in the pipeline. If the node crashes or we shutdown in the meantime the DB is in an inconsistent state. This is not optimal and in a perfect world we'd already write the correct state.
However, since the pipeline might also be started from (slightly incorrect) user input the pipeline itself needs to be able to recover to create a sequence of consecutive blocks. So whether there is an inconsistency due to the aforementioned TODO or misconfigured user input doesn't really matter here.
Specifically, we're checking for the blocks being consecutive here before processing:
go-ethereum/rollup/da_syncer/da_syncer.go
Lines 30 to 37 in ac8164f
Are handling those cases with a skip in case we already processed the block:
Or reset the pipeline by moving a little bit backwards and read a previous batch from L1:
With this mechanism, in all of my testing the pipeline was able to recover from any inconsistent state I started it with.
| return ErrBlockTooHigh | ||
| } | ||
|
|
||
| parentBlock := s.blockchain.GetBlockByNumber(currentBlock.Number().Uint64()) |
There was a problem hiding this comment.
Can't we directly use currentBlock here?
There was a problem hiding this comment.
Yeah you're right. This is probably a remnant from backporting as in the upstream branch returns *types.Header. I'll address this once I add the new da-codec.
|
Merging since this has been extensively reviewed in this and the other PR. |
1. Purpose or design rationale of this PR
This PR ports back changes to the
developbranch from #1013 and #1068.Depends on scroll-tech/da-codec#32.
This PR implements a
follower node from DA/L1mode which reproduces the L2 state solely from L1 events and loading data from DA (calldata is retrieved from L1 RPC directly, historical blobs are loaded via beacon node or blob APIs and verified viaversionedHash).On a high level, it works as follows: the L2 functionality of the node is disabled and instead it connects only to the configured L1 RPC, beacon node or blob APIs, retrieves all rollup events (commit batch, revert, finalize), L1 messages and batch data (i.e. calldata or blobs since Bernoulli). Once an event is finalized on L1 the resulting state (meaning L2 state and blocks) are derived and verified from this data.
The derivation process works by implementing a pipeline with following steps:
-
DAQueue: usesDataSourceto retrieve events and corresponding data (calldata or blob).-
BatchQueue: sorts differentDATypesand returns committed, finalized batches in order.-
BlockQueue: converts batches toPartialBlocksthat can be used to create the L2 state-
DASyncer: executesPartialBlockand inserts into chainHow to run?
Run
l2gethwith the--da.syncflag. Provide blob APIs and beacon node with--da.blob.beaconnode "<L1 beacon node>"(recommended, if beacon node supports historical blobs)--da.blob.blobscan "https://api.blobscan.com/blobs/"--da.blob.blocknative "https://api.ethernow.xyz/v1/blob/"for mainnet--da.blob.blobscan "https://api.sepolia.blobscan.com/blobs/"for Sepolia.Strictly speaking only one of the blob providers is necessary, but during testing blobscan and blocknative were not fully reliable. That's why using a beacon node with historical blob data is recommended (can be additionally to blobscan and blobnative). The pipeline rotates the blob providers and retries if one of them fails.
mainnet
A full sync will take about 2 weeks depending on the speed of the RPC node, beacon node and the local machine. Progess is reported as follows for every 1000 blocks applied:
INFO [08-01|16:44:42.173] L1 sync progress blockhain height=87000 block hash=608eec..880ebd root=218215..9a58a2Sepolia
A full sync will take about 2-3 days depending on the speed of the RPC node, beacon node and the local machine. Progess is reported as follows for every 1000 blocks applied:
INFO [08-01|16:44:42.173] L1 sync progress blockhain height=87000 block hash=608eec..880ebd root=218215..9a58a2Troubleshooting
You should see something like this shortly after starting:
L1 sync progress [...].L1 sync progress [...]to appear as the L1 blocks are more sparse at the beginningTemporary errors
Especially at the beginning some errors like below might appear in the console. This is expected, as the pipeline relies on the L1 messages but in case they're not synced fast enough such an error might pop up. It will continue once the L1 messages are available.
Limitations
The
state rootof a block can be reproduced when using this mode of syncing but currently not theblock hash. This is due to the fact that currently the header fieldsdifficultyandextraDataare not stored on DA but these fields are utilized by Clique consensus which is used by the Scroll protocol. This will be fixed in a future upgrade where the main implementation on l2geth is already done: #903 #913.To verify the locally created
state rootagainst mainnet (https://sepolia-rpc.scroll.io/for Sepolia), we can do the following:By comparing the headers we can most importantly see that
state root,receiptsRootand everything that has to do with the state matches. However, the following fields will be different:difficultyand thereforetotalDifficultyextraDatasizedue to differences in header sizehashand thereforeparentHashExample local output for block 11000:
{ "jsonrpc": "2.0", "id": 0, "result": { "difficulty": "0xa", "extraData": "0x0102030405060708", "gasLimit": "0x989680", "gasUsed": "0xa410", "hash": "0xf3cdafbe35d5e7c18d8274bddad9dd12c94b83a81cefeb82ebb73fa799ff9fcc", "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "miner": "0x0000000000000000000000000000000000000000", "mixHash": "0x0000000000000000000000000000000000000000000000000000000000000000", "nonce": "0x0000000000000000", "number": "0x2af8", "parentHash": "0xde244f7e8bc54c8809e6c2ce65c439b58e90baf11f6cf9aaf8df33a827bd01ab", "receiptsRoot": "0xd95b673818fa493deec414e01e610d97ee287c9421c8eff4102b1647c1a184e4", "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347", "size": "0x252", "stateRoot": "0x0f387e78e4a7457a318c7bce7cde0b05c3609347190144a7e105ef05194ae218", "timestamp": "0x6526db8e", "totalDifficulty": "0x1adb1", "transactionsRoot": "0x6a81c9342456693d57963883983bba024916f4d277392c9c1dc497e3518a78e3" } }Example remote output:
{ "id": 0, "jsonrpc": "2.0", "result": { "difficulty": "0x2", "extraData": "0xd883050000846765746888676f312e31392e31856c696e7578000000000000009920319c246ec8ae4d4f73f07d79f68b2890e9c2033966efe5a81aedddae12875c3170f0552f48b7e5d8e92ac828a6008b2ba7c5b9c4a0af1692337bbdc792be01", "gasLimit": "0x989680", "gasUsed": "0xa410", "hash": "0xb7848d5b300247d7c33aeba0f1b33375e1cb3113b950dffc140945e9d3d88d58", "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "miner": "0x0000000000000000000000000000000000000000", "mixHash": "0x0000000000000000000000000000000000000000000000000000000000000000", "nonce": "0x0000000000000000", "number": "0x2af8", "parentHash": "0xa93e6143ab213a044eb834cdd391a6ef2c818de25b04a3839ee44a75bd28a2c7", "receiptsRoot": "0xd95b673818fa493deec414e01e610d97ee287c9421c8eff4102b1647c1a184e4", "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347", "size": "0x2ab", "stateRoot": "0x0f387e78e4a7457a318c7bce7cde0b05c3609347190144a7e105ef05194ae218", "timestamp": "0x6526db8e", "totalDifficulty": "0x55f1", "transactionsRoot": "0x6a81c9342456693d57963883983bba024916f4d277392c9c1dc497e3518a78e3" } }2. 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
New Features
SyncingPipelinefor managing data availability synchronization.BatchQueueandBlockQueuefor processing batch events.BlobClientand related structures for interacting with blob APIs.DASyncerfor synchronizing blocks with error handling.Bug Fixes
Documentation
Tests