SystemContract: support millisecond block generation #1174
Conversation
WalkthroughAdds BlocksPerSecond to SystemContract config, adds CalcBlocksPerSecond and CalcPeriodMs helpers, refactors SystemContract.CalcTimestamp to handle Period==1 using per-block indexing, introduces nanosecond-precision CalculateBlockDeadline and a test, bumps VersionPatch to 3, and updates two go.mod dependency revisions. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Config
participant SystemContract
Worker->>Config: read Period, BlocksPerSecond, RelaxedPeriod
alt Period == 1
Worker->>SystemContract: CalcBlocksPerSecond(BlocksPerSecond)
Worker->>SystemContract: CalcPeriodMs(BlocksPerSecond)
Worker->>Worker: CalculateBlockDeadline(config, header) -> deadline (ns precision)
else Period != 1
Worker->>SystemContract: CalcTimestamp(parent) -> timestamp (parent + period)
Worker->>Worker: default deadline based on timestamp (+ relaxed override)
end
Worker->>Worker: use deadline for newWork/mining
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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). (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
consensus/system_contract/consensus.go(1 hunks)consensus/system_contract/system_contract.go(1 hunks)miner/scroll_worker.go(1 hunks)params/config.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
consensus/system_contract/system_contract.go (1)
common/types.go (1)
HexToAddress(218-218)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
params/config.go (1)
800-800: Documentation update aligns with implementation changesThe comment has been updated to clarify that the Period is measured in milliseconds rather than seconds, which aligns with the implementation changes in other files that now treat this value as milliseconds.
miner/scroll_worker.go (1)
561-578: Improved deadline calculation with millisecond precisionThe mining deadline calculation has been refined to use millisecond-level granularity, correctly interpreting the system contract period as milliseconds. This provides more precise scheduling of blocks within each second.
The implementation includes proper handling of edge cases like zero
blocksPerSecondand calculates timestamps with nanosecond precision, aligning with the changes in the consensus timestamp calculation.consensus/system_contract/consensus.go (1)
229-256: Enhanced timestamp calculation with millisecond precisionThe timestamp calculation has been improved to support millisecond-level block periods, calculating how many blocks fit within each second and appropriately incrementing the timestamp for the last block in each second interval.
This change allows for more flexible and precise block timing, while still maintaining compatibility with the Unix timestamp format (which uses seconds). The implementation also properly handles edge cases like when
blocksPerSecondwould be zero.
georgehao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/system_contract/consensus.go (1)
240-245: Consider edge cases with non-divisible blocks per second values.The implementation looks correct for typical values. However, be aware that for blocks per second values that don't divide evenly into 1000 (e.g., 3, 6, 7), integer division will truncate the result, potentially causing slight timing drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
consensus/system_contract/consensus.go(1 hunks)consensus/system_contract/consensus_test.go(1 hunks)miner/scroll_worker.go(2 hunks)miner/scroll_worker_test.go(1 hunks)params/config.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- params/config.go
- miner/scroll_worker_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- miner/scroll_worker.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: the `override` parameter in `dasyncer.synconeblock()` is intentionally designed to allow rewriting e...
Learnt from: jonastheis
PR: scroll-tech/go-ethereum#1115
File: rollup/da_syncer/da_syncer.go:35-41
Timestamp: 2025-02-14T04:10:06.754Z
Learning: The `override` parameter in `DASyncer.SyncOneBlock()` is intentionally designed to allow rewriting existing chain data during recovery mode, enabling reconstruction of a valid canonical L2 chain from permissionless committed batches.
Applied to files:
consensus/system_contract/consensus.go
⏰ 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). (4)
- GitHub Check: build-and-push
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
consensus/system_contract/consensus.go (2)
230-238: LGTM - Good default handling for blocks per second.The function correctly handles the zero case by defaulting to 1 block per second, which addresses the devnet mode concern mentioned in previous reviews.
247-280: Complex but correct timestamp calculation logic.The refactored logic correctly implements period-based timestamp increments using modular arithmetic. The approach properly handles:
- Sub-second block intervals through blocks per second configuration
- Period boundaries for timestamp increments
- Genesis block edge case (nextBlockNumber > 0 check)
- Existing RelaxedPeriod override behavior
While significantly more complex than the previous implementation, this enables the millisecond block generation feature as intended.
consensus/system_contract/consensus_test.go (5)
12-105: Excellent comprehensive test coverage for timestamp calculation.The table-driven tests thoroughly validate the new timestamp calculation logic across various scenarios:
- Different blocks per second configurations (1, 2)
- Different period lengths (1s, 2s)
- Period boundary detection and timestamp increment behavior
- Clear test descriptions and detailed error messages
The use of future timestamps to avoid timing conflicts with the current time is a good practice.
107-165: Good validation of millisecond interval calculations.The tests properly validate the
CalcPeriodMshelper function with various blocks per second configurations. The mathematical verification thatblocksPerSecond * interval = 1 secondadds confidence in the timing calculations.
167-205: Excellent simulation of progressive timestamp behavior.This test effectively validates the cumulative timestamp behavior over multiple blocks, ensuring that the modular arithmetic approach works correctly across period boundaries. The simulation of blockchain progression by updating baseTime is realistic and thorough.
207-238: Good validation of default configuration handling.The test properly validates that zero/unset configuration values default to sensible values (period=1s, blocksPerSecond=1), ensuring backward compatibility and robust behavior when configuration is incomplete.
240-282: Precise validation of timestamp increment logic.This test effectively validates the core increment logic by simulating blockchain progression and verifying that timestamps increment only at period boundaries (even-numbered blocks for 2 blocks per second). The modulo arithmetic in the test matches the implementation logic.
…t/support_ms_block_generation
…tech/go-ethereum into feat/support_ms_block_generation
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 (1)
params/version.go(1 hunks)
⏰ 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). (3)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
params/version.go (1)
27-27: Patch bump to 5.9.3 acknowledgedThe version bump is minimal, scoped, and correctly updates Version, VersionWithMeta, and ArchiveVersion via the existing closures.
…tech/go-ethereum into feat/support_ms_block_generation
1. Purpose or design rationale of this PR
This PR enables sub-second block generation on l2geth sequencer nodes.
If
genesis.config.systemContract.periodis set to1, andgenesis.config.systemContract.blocks_per_secondis also set, it will then be used to determine the block time. E.g. withblocks_per_second = 4, the sequencer will produce blocks every 250ms.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