feat(l1 sync service): add flag for sync interval #1241
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI User
participant Geth as geth (cmd/geth)
participant Utils as utils (flags)
participant Node as node.Config
participant Sync as rollup.SyncService
User->>Geth: Start with --l1.sync.interval=Dur?
Geth->>Utils: Parse flags
Utils->>Node: set cfg.L1SyncInterval = Dur (if provided)
Geth->>Sync: Initialize with nodeConfig
alt Interval provided
Sync->>Sync: pollInterval = cfg.L1SyncInterval
else Zero value
Sync->>Sync: pollInterval = DefaultPollInterval
end
Note over Sync: Ticker uses pollInterval for L1 sync loop
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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/sync_service/sync_service.go (1)
103-113: LGTM!The implementation correctly reads the configured
L1SyncIntervaland falls back toDefaultPollIntervalwhen the config value is zero, maintaining backward compatibility.Consider adding validation to ensure the configured interval is reasonable (e.g., not too short), though the current implementation is safe:
pollInterval := nodeConfig.L1SyncInterval if pollInterval == 0 { pollInterval = DefaultPollInterval +} else if pollInterval < time.Second { + log.Warn("L1 sync interval is very short, using minimum of 1 second", "configured", pollInterval) + pollInterval = time.Second }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(2 hunks)node/config.go(2 hunks)rollup/sync_service/sync_service.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/geth/usage.go (1)
cmd/utils/flags.go (1)
L1SyncIntervalFlag(856-859)
cmd/geth/main.go (1)
cmd/utils/flags.go (1)
L1SyncIntervalFlag(856-859)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: test
🔇 Additional comments (6)
node/config.go (2)
28-28: LGTM!The
timeimport is correctly added to support the newL1SyncIntervalfield.
201-202: LGTM!The
L1SyncIntervalfield is correctly defined with appropriate type, documentation, and TOML tagging consistent with other L1 configuration fields.cmd/geth/usage.go (1)
236-236: LGTM!The
L1SyncIntervalFlagis correctly added to the ROLLUP flag group, ensuring it appears in the help documentation alongside other L1 configuration options.cmd/utils/flags.go (2)
856-859: LGTM!The
L1SyncIntervalFlagis correctly defined with a clear name and helpful usage examples demonstrating duration formats.
1477-1479: LGTM!The flag parsing correctly uses
ctx.GlobalDurationand assigns the value tocfg.L1SyncInterval. The zero-value fallback is appropriately handled downstream in the sync service.cmd/geth/main.go (1)
173-173: LGTM!The
L1SyncIntervalFlagis correctly added tonodeFlags, ensuring the flag is recognized and processed by the CLI.
3 participants
1. Purpose or design rationale of this PR
This PR adds a flag to make the L1 sync interval configurable. This is necessary for testing with shorter L1 block and finality times: scroll-tech/rollup-node#343
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
Documentation