feat: high precision sysvar#1384
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change propagates millisecond-precision timestamps through replicated blocks, Assessment against linked issues
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 |
![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
|
Also as a reminder, we need to carefully release this as it would make previously produced block invalidly parse the clock (previously stored in seconds), until the ledger fully truncate the olds blocks |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-ledger/src/store/api.rs (1)
287-302: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVersion or migrate the
blocktime_cfunit change.Existing rows contain seconds, but Lines 287-293 now divide every value as milliseconds. Until old rows are truncated, RPC block times and replayed clocks become roughly 1000× too small. Use a schema/version marker or migration, rather than reinterpreting the existing column in place.
Also applies to: 430-431
🤖 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-ledger/src/store/api.rs` around lines 287 - 302, The blocktime_cf unit change must preserve compatibility with existing rows stored in seconds. Add a schema/version marker or migration before changing the interpretation, and update the block-time accessors around get_block_time and get_block_time_millis so legacy values remain seconds while migrated values are converted from milliseconds; apply the same handling to the related code near the second call site.
🤖 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 749-755: Update update_sysvar_account to return and propagate
errors from both serialize_data and insert_account instead of discarding their
results. Thread the resulting error through the transition/replay update callers
so the operation aborts when Clock or HighPrecisionClock persistence fails.
---
Outside diff comments:
In `@magicblock-ledger/src/store/api.rs`:
- Around line 287-302: The blocktime_cf unit change must preserve compatibility
with existing rows stored in seconds. Add a schema/version marker or migration
before changing the interpretation, and update the block-time accessors around
get_block_time and get_block_time_millis so legacy values remain seconds while
migrated values are converted from milliseconds; apply the same handling to the
related code near the second call site.
🪄 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: 00e3736c-b0ef-4a04-a86a-79973f5da2cb
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlmagicblock-core/src/link/blocks.rsmagicblock-core/src/link/replication.rsmagicblock-ledger/src/blockstore_processor/mod.rsmagicblock-ledger/src/store/api.rsmagicblock-magic-program-api/src/lib.rsmagicblock-processor/src/executor/mod.rsmagicblock-processor/src/scheduler/mod.rstest-integration/Cargo.tomltest-integration/test-ledger-restore/tests/17_high_precision_clock.rs
💤 Files with no reviewable changes (1)
- magicblock-core/src/link/replication.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
magicblock-processor/src/scheduler/mod.rs (2)
571-584:⚠️ Potential issue | 🟠 MajorPreviously reported production
unwrap()remains.
duration_since(UNIX_EPOCH).unwrap()can panic on a pre-epoch system clock and terminate the scheduler’s block-production loop. Handle the error explicitly or document and enforce the invariant.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.”🤖 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 571 - 584, Replace the production `duration_since(UNIX_EPOCH).unwrap()` in the block timestamp construction with explicit handling for pre-epoch system times, preventing scheduler termination; propagate or handle the error according to the surrounding block-production flow while preserving millisecond timestamp behavior.Source: Path instructions
747-751:⚠️ Potential issue | 🟠 MajorPreviously reported persistence errors remain discarded.
Failures from
serialize_dataandinsert_accountare ignored, allowingClockorHighPrecisionClockto remain stale while the scheduler advances. Propagate the error or enforce an explicit infallibility invariant.🤖 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 747 - 751, Update update_sysvar_account to handle failures from account.serialize_data and accountsdb.insert_account instead of discarding their results. Propagate the error through the caller chain, or explicitly enforce and document an infallibility invariant if the API cannot return errors, ensuring Clock and HighPrecisionClock updates cannot silently remain stale.
🤖 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.
Duplicate comments:
In `@magicblock-processor/src/scheduler/mod.rs`:
- Around line 571-584: Replace the production
`duration_since(UNIX_EPOCH).unwrap()` in the block timestamp construction with
explicit handling for pre-epoch system times, preventing scheduler termination;
propagate or handle the error according to the surrounding block-production flow
while preserving millisecond timestamp behavior.
- Around line 747-751: Update update_sysvar_account to handle failures from
account.serialize_data and accountsdb.insert_account instead of discarding their
results. Propagate the error through the caller chain, or explicitly enforce and
document an infallibility invariant if the API cannot return errors, ensuring
Clock and HighPrecisionClock updates cannot silently remain stale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77107b5a-93f8-4329-8df1-d778b761807a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlmagicblock-processor/src/scheduler/mod.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-ledger/src/store/api.rs (1)
170-175: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd a migration or legacy-ledger guard before rebuilding
latest_blockfromblocktime_cf(magicblock-ledger/src/store/api.rs:170-175). New writes store milliseconds, but reopening an older ledger will treat second-based values as millis and reconstruct a tip clock that’s ~1000x too small. Add a versioned normalization/open-path fallback and a regression test for a pre-millis ledger.🤖 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-ledger/src/store/api.rs` around lines 170 - 175, Guard the latest_block reconstruction around LatestBlockInner::new_with_millis and ledger.get_block_time_millis so legacy blocktime_cf values stored in seconds are detected and normalized before use. Add a versioned migration or open-path fallback that preserves millisecond values for current ledgers, and add a regression test covering reopening a pre-millis ledger.
🤖 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.
Outside diff comments:
In `@magicblock-ledger/src/store/api.rs`:
- Around line 170-175: Guard the latest_block reconstruction around
LatestBlockInner::new_with_millis and ledger.get_block_time_millis so legacy
blocktime_cf values stored in seconds are detected and normalized before use.
Add a versioned migration or open-path fallback that preserves millisecond
values for current ledgers, and add a regression test covering reopening a
pre-millis ledger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a63a080b-1fd4-4a0e-a2b2-43971b820209
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlmagicblock-ledger/src/store/api.rsmagicblock-processor/src/scheduler/mod.rsmagicblock-processor/src/scheduler/state.rstest-integration/Cargo.toml
2 participants
Summary
Introduces a high precision sysvar updated at each slot and stored in the ledger. Closes #1434
Breaking Changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
HighPrecisionClocksysvar that exposes Unix time in milliseconds.timestamp_millisfor higher-precision tracking.Bug Fixes
Tests
Chores