feat: high precision sysvar#1384
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThis PR introduces millisecond-precision timestamps throughout the block-production and ledger pipeline. The replication Sequence Diagram(s)Sequence diagrams are included within the hidden review stack artifact above. Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
This requires merging this SVM PR first |
| @@ -79,6 +79,10 @@ pub struct Block { | |||
| pub hash: Hash, | |||
| /// Unix timestamp (seconds). | |||
| pub timestamp: i64, | |||
There was a problem hiding this comment.
timestamp is now redundant. We can remove it after a migration time (once all ledgers truncated all blocks with the legacy timestamp)
There was a problem hiding this comment.
Would be great to document this and leave a TODO in the codebase to remove the timestamp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-processor/src/scheduler/mod.rs`:
- Around line 585-598: The scheduler hot path in the block/tick timestamp path
uses SystemTime::now().duration_since(UNIX_EPOCH).unwrap(), which can panic and
stop block production. Update the timestamp creation in the scheduler module
(around LatestBlockInner::new_with_millis and the Message::Block construction)
to handle the error explicitly, either by propagating/logging a recoverable
failure or by falling back to a safe timestamp, and keep the loop running
instead of unwrapping.
- Around line 670-684: The sysvar update logic is duplicated between
update_high_precision_clock_sysvar and update_clock_sysvar, both doing the same
get_account/serialize_data/insert_account flow. Extract that repeated sequence
into a shared helper in scheduler::mod, and have both update_clock_sysvar and
update_high_precision_clock_sysvar call it with their specific sysvar value and
account ID to keep future sysvar additions consistent.
In `@test-integration/test-ledger-restore/tests/17_high_precision_clock.rs`:
- Around line 56-63: The sanity check around observed.count in the
HighPrecisionClock restore tests uses an incorrect upper bound and misleading
label: count is derived from milliseconds modulo 1000, so the assertion should
validate a value in the documented 0–999 range instead of a nanosecond-sized
bound, and the failure message should say millis/sub-second value rather than
nanos. Update both occurrences in the test to use the same correct bound and
message so the check actually catches regressions in the HighPrecisionClock read
path.
- Around line 123-129: The restart check in high precision clock test is flaky
because it compares the `observed.updates` and `observed.count` components
independently in the `assert!`, which can fail even when real time advanced;
update the logic around the `high precision clock should advance after restart`
assertion to compare a single combined millisecond timestamp derived from the
same fields instead of separate component-wise greater-than checks, and keep the
`cleanup(&mut validator)` flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90eae371-dadb-4a26-916f-f393ace9c697
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlmagicblock-core/src/link/replication.rsmagicblock-ledger/src/blockstore_processor/mod.rsmagicblock-ledger/src/database/columns.rsmagicblock-ledger/src/lib.rsmagicblock-ledger/src/store/api.rsmagicblock-magic-program-api/Cargo.tomlmagicblock-magic-program-api/src/lib.rsmagicblock-magic-program-api/src/sysvar.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/state.rsmagicblock-processor/tests/high_precision_clock.rsmagicblock-processor/tests/replica_ordering.rstest-integration/Cargo.tomltest-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-ledger-restore/tests/17_high_precision_clock.rs
| let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); | ||
| // Sampled at millisecond accuracy: this is the value persisted in the | ||
| // ledger and exposed via the HighPrecisionClock sysvar, so live | ||
| // execution and a restart/replay observe exactly the same timestamp. | ||
| let timestamp_millis = now.as_millis() as i64; | ||
| let block = LatestBlockInner::new_with_millis( | ||
| self.slot, | ||
| blockhash, | ||
| timestamp_millis, | ||
| ); | ||
| let msg = Message::Block(replication::Block { | ||
| slot: block.slot, | ||
| hash: block.blockhash, | ||
| timestamp: block.clock.unix_timestamp, | ||
| timestamp_millis: block.timestamp_millis, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.unwrap() on system clock in production hot path.
SystemTime::now().duration_since(UNIX_EPOCH).unwrap() runs every slot tick on the scheduler's dedicated thread. A pre-epoch/misconfigured system clock would panic and kill the entire scheduling loop (halts block production). Please handle this gracefully instead of panicking.
🔧 Proposed fix
- let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap();
+ let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_else(|error| {
+ error!(%error, "system clock is before UNIX_EPOCH; using zero timestamp");
+ Duration::ZERO
+ });As per path instructions, "Treat any usage of .unwrap() or .expect() in production Rust code as a MAJOR issue... Request proper error handling or explicit justification with invariants."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); | |
| // Sampled at millisecond accuracy: this is the value persisted in the | |
| // ledger and exposed via the HighPrecisionClock sysvar, so live | |
| // execution and a restart/replay observe exactly the same timestamp. | |
| let timestamp_millis = now.as_millis() as i64; | |
| let block = LatestBlockInner::new_with_millis( | |
| self.slot, | |
| blockhash, | |
| timestamp_millis, | |
| ); | |
| let msg = Message::Block(replication::Block { | |
| slot: block.slot, | |
| hash: block.blockhash, | |
| timestamp: block.clock.unix_timestamp, | |
| timestamp_millis: block.timestamp_millis, | |
| let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_else(|error| { | |
| error!(%error, "system clock is before UNIX_EPOCH; using zero timestamp"); | |
| Duration::ZERO | |
| }); | |
| // Sampled at millisecond accuracy: this is the value persisted in the | |
| // ledger and exposed via the HighPrecisionClock sysvar, so live | |
| // execution and a restart/replay observe exactly the same timestamp. | |
| let timestamp_millis = now.as_millis() as i64; | |
| let block = LatestBlockInner::new_with_millis( | |
| self.slot, | |
| blockhash, | |
| timestamp_millis, | |
| ); | |
| let msg = Message::Block(replication::Block { | |
| slot: block.slot, | |
| hash: block.blockhash, | |
| timestamp_millis: block.timestamp_millis, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magicblock-processor/src/scheduler/mod.rs` around lines 585 - 598, The
scheduler hot path in the block/tick timestamp path uses
SystemTime::now().duration_since(UNIX_EPOCH).unwrap(), which can panic and stop
block production. Update the timestamp creation in the scheduler module (around
LatestBlockInner::new_with_millis and the Message::Block construction) to handle
the error explicitly, either by propagating/logging a recoverable failure or by
falling back to a safe timestamp, and keep the loop running instead of
unwrapping.
Source: Path instructions
| /// Updates the HighPrecisionClock sysvar account. | ||
| fn update_high_precision_clock_sysvar(&self, block: &LatestBlockInner) { | ||
| if let Some(mut account) = | ||
| self.accountsdb.get_account(&HIGH_PRECISION_CLOCK_ID) | ||
| { | ||
| let high_precision_clock = HighPrecisionClock { | ||
| unix_timestamp_millis: block.timestamp_millis, | ||
| }; | ||
| let _ = account.serialize_data(&high_precision_clock); | ||
| let _ = self | ||
| .accountsdb | ||
| .insert_account(&HIGH_PRECISION_CLOCK_ID, &account); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate sysvar-update pattern; extract a shared helper.
update_high_precision_clock_sysvar (670-684) repeats the exact "get account → serialize_data → insert_account" pattern of update_clock_sysvar (662-668). Consider a generic helper to avoid duplication as more sysvars are added.
♻️ Proposed refactor
+ /// Serializes `data` into `id`'s account if it already exists.
+ fn update_sysvar_account<T: Serialize>(&self, id: &Pubkey, data: &T) {
+ if let Some(mut account) = self.accountsdb.get_account(id) {
+ let _ = account.serialize_data(data);
+ let _ = self.accountsdb.insert_account(id, &account);
+ }
+ }
+
fn update_clock_sysvar(&self, clock: &Clock) {
- if let Some(mut account) = self.accountsdb.get_account(&clock::ID) {
- let _ = account.serialize_data(clock);
- let _ = self.accountsdb.insert_account(&clock::ID, &account);
- }
+ self.update_sysvar_account(&clock::ID, clock);
}
fn update_high_precision_clock_sysvar(&self, block: &LatestBlockInner) {
- if let Some(mut account) =
- self.accountsdb.get_account(&HIGH_PRECISION_CLOCK_ID)
- {
- let high_precision_clock = HighPrecisionClock {
- unix_timestamp_millis: block.timestamp_millis,
- };
- let _ = account.serialize_data(&high_precision_clock);
- let _ = self
- .accountsdb
- .insert_account(&HIGH_PRECISION_CLOCK_ID, &account);
- }
+ let high_precision_clock = HighPrecisionClock {
+ unix_timestamp_millis: block.timestamp_millis,
+ };
+ self.update_sysvar_account(&HIGH_PRECISION_CLOCK_ID, &high_precision_clock);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@magicblock-processor/src/scheduler/mod.rs` around lines 670 - 684, The sysvar
update logic is duplicated between update_high_precision_clock_sysvar and
update_clock_sysvar, both doing the same
get_account/serialize_data/insert_account flow. Extract that repeated sequence
into a shared helper in scheduler::mod, and have both update_clock_sysvar and
update_high_precision_clock_sysvar call it with their specific sysvar value and
account ID to keep future sysvar additions consistent.
| // Sanity: the sub-second component (stored in `count`) must be a valid | ||
| // fraction of a second, confirming a real HighPrecisionClock was read. | ||
| assert!( | ||
| observed.count < 1_000_000_000, | ||
| cleanup(&mut validator), | ||
| "nanos out of range: {}", | ||
| observed.count | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sanity bound doesn't match documented value range; log label says "nanos" but value is millis.
observed.count is derived from millis.rem_euclid(1000) (per instruction.rs docs), so it's always < 1000. Checking < 1_000_000_000 never fails, making this "sanity check" a no-op — it wouldn't catch a regression where, e.g., a raw nanosecond or unclamped value leaked into count. The log/assert message also mislabels the value as "nanos".
🐛 Proposed fix (apply to both occurrences)
assert!(
- observed.count < 1_000_000_000,
+ observed.count < 1_000,
cleanup(&mut validator),
- "nanos out of range: {}",
+ "millis out of range: {}",
observed.count
);Also applies to: 113-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-integration/test-ledger-restore/tests/17_high_precision_clock.rs` around
lines 56 - 63, The sanity check around observed.count in the HighPrecisionClock
restore tests uses an incorrect upper bound and misleading label: count is
derived from milliseconds modulo 1000, so the assertion should validate a value
in the documented 0–999 range instead of a nanosecond-sized bound, and the
failure message should say millis/sub-second value rather than nanos. Update
both occurrences in the test to use the same correct bound and message so the
check actually catches regressions in the HighPrecisionClock read path.
| assert!( | ||
| observed.updates > expected.updates || observed.count > expected.count, | ||
| cleanup(&mut validator), | ||
| "high precision clock should advance after restart: before={}, after={}", | ||
| expected.updates, | ||
| observed.updates | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Flaky advance check: comparing components independently instead of combined timestamp.
If observed and expected land in the same whole second and the newly sampled sub-second component is numerically lower than the previous one, both observed.updates > expected.updates and observed.count > expected.count are false even though real time has genuinely advanced — causing a false test failure.
🐛 Proposed fix: compare combined millisecond timestamp
+ let expected_millis =
+ expected.updates as i64 * 1000 + expected.count as i64;
+ let observed_millis =
+ observed.updates as i64 * 1000 + observed.count as i64;
assert!(
- observed.updates > expected.updates || observed.count > expected.count,
+ observed_millis > expected_millis,
cleanup(&mut validator),
"high precision clock should advance after restart: before={}, after={}",
- expected.updates,
- observed.updates
+ expected_millis,
+ observed_millis
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert!( | |
| observed.updates > expected.updates || observed.count > expected.count, | |
| cleanup(&mut validator), | |
| "high precision clock should advance after restart: before={}, after={}", | |
| expected.updates, | |
| observed.updates | |
| ); | |
| let expected_millis = | |
| expected.updates as i64 * 1000 + expected.count as i64; | |
| let observed_millis = | |
| observed.updates as i64 * 1000 + observed.count as i64; | |
| assert!( | |
| observed_millis > expected_millis, | |
| cleanup(&mut validator), | |
| "high precision clock should advance after restart: before={}, after={}", | |
| expected_millis, | |
| observed_millis | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-integration/test-ledger-restore/tests/17_high_precision_clock.rs` around
lines 123 - 129, The restart check in high precision clock test is flaky because
it compares the `observed.updates` and `observed.count` components independently
in the `assert!`, which can fail even when real time advanced; update the logic
around the `high precision clock should advance after restart` assertion to
compare a single combined millisecond timestamp derived from the same fields
instead of separate component-wise greater-than checks, and keep the
`cleanup(&mut validator)` flow intact.
2 participants
Summary
Introduces a high precision sysvar updated at each slot and stored in the ledger
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes