SystemContract: support millisecond block generation #1174
Conversation
WalkthroughThis change introduces new helper functions and logic for calculating block timestamps and deadlines based on configurable blocks-per-second rates in the consensus system contract. It updates the configuration structure, modifies block production timing, adds a new deadline calculation helper, and introduces a dedicated test to validate block interval computations. Go module dependencies are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant SystemContract
participant Config
Worker->>Config: Get Period, BlocksPerSecond
alt Period == 1
Worker->>SystemContract: CalcBlocksPerSecond()
Worker->>SystemContract: CalcPeriodMs()
Worker->>SystemContract: CalcTimestamp(parent)
Worker->>Worker: CalculateBlockDeadline(config, header)
else Period != 1
Worker->>SystemContract: CalcTimestamp(parent)
Worker->>Worker: Default deadline logic
end
Worker->>Worker: Log deadline and transaction status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 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 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: 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
| // CalcBlocksPerSecond converts period to milliseconds and calculate blocks per second | ||
| // For example: if Period = 250ms = 0.25s, then periodMs = 250 | ||
| func CalcBlocksPerSecond(periodMs uint64) uint64 { | ||
| blocksPerSecond := 1000 / periodMs // integer division, e.g. 1000/250 = 4 |
There was a problem hiding this comment.
periodMs can be 0. It is for devnet mode that we don't use, but still safer to handle that case here.
| timestamp := parent.Time + s.config.Period | ||
| // Get the base timestamp (in seconds) | ||
| timestamp := parent.Time |
There was a problem hiding this comment.
This change essentially means that we won't be able to support >1s block times, is that intentional?
There was a problem hiding this comment.
yeah I was thinking the same. maybe we should introduce an additional (optional) parameter that allows us to specify the amount of blocks if the period is 1. This way we can still support block times bigger than one and there's less risk when deploying the new version
There was a problem hiding this comment.
Sure. But I guess we also do not need to support longer block times, for now.
miner/scroll_worker.go
Outdated
| // Add one period to determine the deadline | ||
| nextBlockNano := baseTimeNano + fractionNano + int64(periodMs)*int64(time.Millisecond) |
There was a problem hiding this comment.
What would happen if we didn't add one more period here?
There was a problem hiding this comment.
I guess the deadline would be at the current time and thus the block would timeout immediately
georgehao
dismissed stale reviews from Thegaram, colinlyguo, and jonastheis
via
7c997fb
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.
| blocksPerSecond := CalcBlocksPerSecond(s.config.BlocksPerSecond) | ||
|
|
||
| // Calculate blocks per period | ||
| blocksPerPeriod := blocksPerSecond * period |
There was a problem hiding this comment.
I think for simplicity we should do something like this in this function:
if period == 1 {
blocksPerSecond := CalcBlocksPerSecond(s.config.BlocksPerSecond)
...
} else {
timestamp := parent.Time + s.config.Period
}
miner/scroll_worker.go
Outdated
| } | ||
|
|
||
| blocksPerSecond := system_contract.CalcBlocksPerSecond(config.BlocksPerSecond) | ||
| periodMs := system_contract.CalcPeriodMs(config.BlocksPerSecond) |
There was a problem hiding this comment.
| periodMs := system_contract.CalcPeriodMs(config.BlocksPerSecond) | |
| periodMs := system_contract.CalcPeriodMs(blocksPerSecond) |
miner/scroll_worker.go
Outdated
| periodMs := system_contract.CalcPeriodMs(config.BlocksPerSecond) | ||
|
|
||
| // Calculate blocks per period | ||
| blocksPerPeriod := blocksPerSecond * period |
There was a problem hiding this comment.
also here I would simplify this by wrapping it into a big if:
if period == 1 {
// do blockIndex stuff
} else {
deadline = time.Unix(int64(header.Time+w.chainConfig.SystemContract.Period), 0)
}
miner/scroll_worker.go
Outdated
| @@ -558,10 +558,13 @@ func (w *worker) newWork(now time.Time, parent *types.Block, reorging bool, reor | |||
| // clique with relaxed period uses time.Now() as the header.Time, calculate the deadline | |||
| deadline = time.Unix(int64(header.Time+w.chainConfig.Clique.Period), 0) | |||
| } | |||
| if w.chainConfig.SystemContract != nil && w.chainConfig.SystemContract.RelaxedPeriod { | |||
| if w.chainConfig.SystemContract != nil && w.chainConfig.SystemContract.RelaxedPeriod && w.chainConfig.SystemContract.Period != 1 { | |||
There was a problem hiding this comment.
Why do we need to add && w.chainConfig.SystemContract.Period != 1 here?
| baseTimeNano := int64(header.Time) * int64(time.Second) | ||
| fractionNano := int64(blockIndex) * int64(periodMs) * int64(time.Millisecond) | ||
|
|
||
| // Add one period to determine the deadline | ||
| nextBlockNano := baseTimeNano + fractionNano + int64(periodMs)*int64(time.Millisecond) | ||
|
|
||
| return time.Unix(0, nextBlockNano) |
There was a problem hiding this comment.
| baseTimeNano := int64(header.Time) * int64(time.Second) | |
| fractionNano := int64(blockIndex) * int64(periodMs) * int64(time.Millisecond) | |
| // Add one period to determine the deadline | |
| nextBlockNano := baseTimeNano + fractionNano + int64(periodMs)*int64(time.Millisecond) | |
| return time.Unix(0, nextBlockNano) | |
| fractionNano := int64(blockIndex) * int64(periodMs) * int64(time.Millisecond) | |
| // Add one period to determine the deadline | |
| nextBlockNano := fractionNano + int64(periodMs)*int64(time.Millisecond) | |
| return time.Unix(int64(header.Time), nextBlockNano) |
There was a problem hiding this comment.
I don't see anything optimization of the new format. How about keeping the original one (because this format have been tested) @jonastheis
…t/support_ms_block_generation
…tech/go-ethereum into feat/support_ms_block_generation
1. Purpose or design rationale of this PR
...
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