feat(l1 follower): add periodic logs for L1 follower mode sync progress #1110
Conversation
WalkthroughThe changes add new accessor methods to expose internal fields and extend interfaces across several modules. A new Changes
Sequence Diagram(s)sequenceDiagram
participant SP as SyncingPipeline
participant T as Ticker (1 min)
participant DQ as DAQueue
participant DS as DataSource
SP->>T: Start progress ticker
T-->>SP: Tick event received
SP->>DQ: Call DataSource() to retrieve data source
DQ->>DS: Return data source info (includes L1Finalized)
DS-->>DQ: Provide L1 finalized block number
DQ-->>SP: Return data source with L1 info
SP->>SP: Log synchronization progress
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/da_syncer/syncing_pipeline.go (1)
164-164: Consider using float64 conversion to prevent potential overflow.The progress calculation
float64(dataSource.L1Height())/float64(dataSource.L1Finalized())*100could overflow for very large block numbers when converting to float64 sequentially.Apply this diff to prevent potential overflow:
- log.Info("L1 sync progress", "L1 processed", dataSource.L1Height(), "L1 finalized", dataSource.L1Finalized(), "progress(%)", float64(dataSource.L1Height())/float64(dataSource.L1Finalized())*100, "L2 height", currentBlock.Number().Uint64(), "L2 hash", currentBlock.Hash()) + progress := 100.0 * (float64(dataSource.L1Height()) / float64(dataSource.L1Finalized())) + log.Info("L1 sync progress", "L1 processed", dataSource.L1Height(), "L1 finalized", dataSource.L1Finalized(), "progress(%)", progress, "L2 height", currentBlock.Number().Uint64(), "L2 hash", currentBlock.Hash())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rollup/da_syncer/da/calldata_blob_source.go(1 hunks)rollup/da_syncer/da_queue.go(1 hunks)rollup/da_syncer/data_source.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
rollup/da_syncer/data_source.go (1)
17-17: LGTM! Interface addition is well-designed.The new
L1Finalized()method is a clean addition to expose finalized block information through the interface.rollup/da_syncer/da_queue.go (1)
72-74: LGTM! Clean accessor implementation.The
DataSource()accessor method provides controlled access to the internal data source, which is necessary for the logging functionality.rollup/da_syncer/da/calldata_blob_source.go (1)
88-90: LGTM! Implementation satisfies interface requirement.The
L1Finalized()method correctly implements the interface by exposing the internal finalized block number.rollup/da_syncer/syncing_pipeline.go (1)
42-42: LGTM! Field addition and initialization are well-structured.The
daQueuefield is appropriately added and initialized in the constructor.Also applies to: 96-96
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/da_syncer/syncing_pipeline.go (1)
120-121: Consider using time.NewTicker in defer pattern.For better resource management, consider creating and stopping the ticker in a single defer statement.
-progressTicker := time.NewTicker(1 * time.Minute) -defer progressTicker.Stop() +progressTicker := time.NewTicker(1 * time.Minute) +defer func() { + if progressTicker != nil { + progressTicker.Stop() + } +}()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
params/version.go(1 hunks)rollup/da_syncer/da_syncer.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
rollup/da_syncer/da_syncer.go (1)
49-49: LGTM! Enhanced log readability.The change improves log readability by converting block hash and root to hex format, making it easier to track and debug L1 sync progress.
params/version.go (1)
27-27: LGTM! Appropriate version bump.The patch version increment is appropriate for adding new logging functionality, as it represents a backward-compatible enhancement.
rollup/da_syncer/syncing_pipeline.go (2)
42-42: LGTM! Clean integration of daQueue field.The daQueue field is properly initialized and integrated into the SyncingPipeline struct.
Also applies to: 96-96
164-164:⚠️ Potential issuePrevent potential overflow in progress calculation.
The progress calculation
float64(dataSource.L1Height())/float64(dataSource.L1Finalized())*100could overflow for large block numbers. Consider casting to float64 after division.-log.Info("L1 sync progress", "L1 processed", dataSource.L1Height(), "L1 finalized", dataSource.L1Finalized(), "progress(%)", float64(dataSource.L1Height())/float64(dataSource.L1Finalized())*100, "L2 height", currentBlock.Number().Uint64(), "L2 hash", currentBlock.Hash().Hex()) +log.Info("L1 sync progress", "L1 processed", dataSource.L1Height(), "L1 finalized", dataSource.L1Finalized(), "progress(%)", float64(dataSource.L1Height()*100)/float64(dataSource.L1Finalized()), "L2 height", currentBlock.Number().Uint64(), "L2 hash", currentBlock.Hash().Hex())Likely invalid or redundant comment.
1. Purpose or design rationale of this PR
This PR adds a periodic log message to the L1 follower mode syncing pipeline to make it easier to see syncing progress.
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