feat: parameterize sync service fetch block range #1247
Conversation
WalkthroughAdds a configurable L1 fetch block range: new CLI flag, config field, and SyncService support. Node startup parses the flag into config; SyncService reads the value (falls back to default) and uses it in fetchMessages batching. Also bumps VersionPatch to 6. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User/Operator
participant Geth as Geth (cmd/geth)
participant Utils as utils.Flags
participant Node as Node Config
participant Sync as SyncService
participant L1 as L1 RPC
User->>Geth: start geth --l1.sync.fetchblockrange=N
Geth->>Utils: Register & parse L1FetchBlockRangeFlag
Utils-->>Node: set cfg.L1FetchBlockRange = N
Geth->>Sync: New SyncService(cfg)
alt cfg.L1FetchBlockRange == 0
Sync->>Sync: fetchBlockRange = DefaultFetchBlockRange
else
Sync->>Sync: fetchBlockRange = cfg.L1FetchBlockRange
end
loop Fetch L1 messages (batched)
Sync->>L1: eth_getLogs(from..to) using fetchBlockRange
L1-->>Sync: logs
Sync->>Sync: process & advance by fetchBlockRange
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/geth/main.go(1 hunks)cmd/utils/flags.go(2 hunks)node/config.go(1 hunks)params/version.go(1 hunks)rollup/sync_service/sync_service.go(4 hunks)
🔇 Additional comments (6)
node/config.go (1)
203-204: LGTM! Well-structured configuration field.The new L1FetchBlockRange field is properly defined with a clear comment and appropriate type. The placement among other L1-related configuration fields is logical.
rollup/sync_service/sync_service.go (2)
109-112: Good defensive default handling.The fallback to DefaultFetchBlockRange when the configured value is 0 ensures backward compatibility and prevents invalid configurations.
54-54: Verify fetch block range limits for production use.The implementation correctly parameterizes the fetch block range, but consider validating bounds:
- Many Ethereum RPC providers limit eth_getLogs to ~10,000 blocks
- Very small values (e.g., 1-10 blocks) may cause excessive RPC calls
- The DogeOS team requested this for smaller ranges, but ensure the chosen value is tested
Run this script to check if there are any existing limits or validations in the codebase:
Also applies to: 120-120, 246-246, 260-260
cmd/utils/flags.go (2)
860-863: LGTM! CLI flag properly defined.The flag definition follows the established naming convention for L1 sync-related flags and includes clear usage documentation.
1484-1486: LGTM! Flag wiring is correct.The flag is properly wired into the L1 configuration flow, consistent with other L1 flags in the same function.
cmd/geth/main.go (1)
174-174: LGTM! Flag correctly exposed in CLI.The flag is properly added to the nodeFlags slice and placed logically with other L1-related configuration flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/sync_service/sync_service.go (1)
109-112: Consider adding validation for the fetch block range.The initialization logic correctly implements the fallback to
DefaultFetchBlockRangewhennodeConfig.L1FetchBlockRangeis zero. However, consider adding validation to ensure the value is within reasonable bounds.While the current implementation is functional, you may want to add validation to prevent potential issues:
fetchBlockRange := nodeConfig.L1FetchBlockRange if fetchBlockRange == 0 { fetchBlockRange = DefaultFetchBlockRange } + // Validate fetch range is reasonable to avoid eth_getLogs failures + if fetchBlockRange > 10000 { + log.Warn("L1 fetch block range is very large, may cause eth_getLogs to fail", "range", fetchBlockRange) + }Additionally, adding a comment explaining the fallback logic would improve code clarity:
+ // Use configured fetch block range, or fall back to default if not set fetchBlockRange := nodeConfig.L1FetchBlockRange if fetchBlockRange == 0 { fetchBlockRange = DefaultFetchBlockRange }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/geth/main.go(1 hunks)cmd/utils/flags.go(2 hunks)node/config.go(1 hunks)params/version.go(1 hunks)rollup/sync_service/sync_service.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/geth/main.go (1)
cmd/utils/flags.go (1)
L1FetchBlockRangeFlag(860-863)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
params/version.go (1)
27-27: LGTM! Version bump aligns with the new feature.The patch version increment is appropriate for adding a configurable parameter. The change follows semantic versioning practices.
cmd/geth/main.go (1)
174-174: LGTM! Flag correctly positioned with other L1 configuration flags.The addition of
L1FetchBlockRangeFlagto thenodeFlagslist is properly placed alongside other L1 sync configuration flags (L1SyncIntervalFlag, L1DisableMessageQueueV2Flag, etc.), maintaining logical grouping.node/config.go (1)
203-204: LGTM! Config field properly defined and documented.The
L1FetchBlockRangefield is correctly added with:
- Clear naming that matches the CLI flag
- Descriptive comment explaining its purpose
- Appropriate
toml:",omitempty"tag for optional configuration- Logical placement with other L1 sync configuration fields
cmd/utils/flags.go (2)
860-863: LGTM! CLI flag properly defined.The
L1FetchBlockRangeFlagis correctly configured with:
- Appropriate type (
cli.Uint64Flag) for block range values- Consistent naming convention (
l1.sync.fetchblockrange)- Clear usage description
1484-1486: LGTM! Flag correctly wired to configuration.The flag value is properly assigned to
cfg.L1FetchBlockRangein thesetL1function, with appropriate checks for whether the flag is set. The zero value is intentionally used as a signal for the fallback toDefaultFetchBlockRangein the SyncService.rollup/sync_service/sync_service.go (4)
54-54: LGTM! Field appropriately added to track configurable fetch range.The
fetchBlockRangefield is correctly added to theSyncServicestruct to store the configured block range value. The field is unexported (internal use only) and has an appropriate type.
120-120: LGTM! Fetch block range correctly stored in service instance.The configured
fetchBlockRangevalue is properly assigned to theSyncServicestruct, making it available for use in the fetch loop.
246-246: LGTM! Loop increment correctly uses configurable fetch range.The loop increment now uses
s.fetchBlockRangeinstead of the hardcodedDefaultFetchBlockRange, properly implementing the configurable behavior while maintaining the existing loop logic.
260-260: LGTM! End block calculation correctly uses configurable fetch range.The end block calculation now uses
s.fetchBlockRange, ensuring consistency with the loop increment and properly implementing the configurable batch size.
1. Purpose or design rationale of this PR
This PR parameterize sync service fetch block range, which required by DogeOS team to address the issue that the default 100 block range is too large in their case.
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
Chores