Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4f5ee62 to
e964aa5
Compare
5507317 to
cc10928
Compare
b2f67ae to
c472d84
Compare
02e0c7b to
6460a11
Compare
81e789d to
8d9b940
Compare
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdded the ChangesKeeper engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant Keeper
participant AccountsDB
participant Ledger
participant Subscribers
Runtime->>Keeper: append and execute transaction
Keeper->>AccountsDB: load and update account state
Keeper->>Ledger: persist transaction and execution event
Ledger-->>Keeper: return commit result
Keeper->>Subscribers: publish status, logs, and account updates
Subscribers-->>Runtime: deliver notifications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
keeper/src/lib.rs (1)
213-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA failed thread spawn leaves the temporary archive file behind.
dstis created at Line 215 before the thread spawn. Ifthread::Builder::spawnreturns an error at Line 217, the.tmpfile stays in the superblock directory. The nextfinalize_superblocktruncates it, so this is not a correctness failure, but the stale file remains after a terminal spawn failure.Remove
tmpon the spawn error path.🤖 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 `@keeper/src/lib.rs` around lines 213 - 217, Update the thread creation flow around thread::Builder::new().name("snapshot-archiver") so a failed spawn removes the temporary archive at tmp before propagating the spawn error. Preserve the existing successful spawn behavior and error propagation.
🤖 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 `@keeper/src/accessor.rs`:
- Around line 214-215: Make the ledger appender transition at
accessor.rs:214-215 recoverable with the accountsdb mutation by changing the
Event::Execution flow around commit_state_transitions to use a durable
pending-transition or rollback/replay protocol, and publish the event only after
both stores commit. In accessor.rs:309-319, defer block-cache publication and
block subscriptions until update_sysvars and set_slot succeed, using the same
recovery protocol; both sites are in the accessor transition flow and must
preserve consistent ledger and account state after failures.
In `@keeper/src/builder.rs`:
- Around line 270-275: Update the restored-store failure branch in the
validation match to return a corruption-specific error such as
AccountsDBError::Corruption instead of SnapshotError::Missing, while preserving
the existing backup and logging behavior.
- Around line 256-286: In the accountsdb method, explicitly drop the AccountsDB
instance after saving the backup and before calling self.unarchive(ledger),
ensuring the LMDB environment and mapped storage are closed before unpacking the
restored snapshot. Preserve the existing restore and error-handling flow.
- Around line 196-207: Update the SlotHashes construction in the surrounding
builder flow to initialize it from the retained blocks returned by
handle.recv_timeout().await??, rather than seeding it with [Default::default();
SLOTHASH_ENTRIES]. Add each retained block’s slot and hash to the resulting
SlotHashes and preserve the existing account creation and last_block updates.
- Around line 161-170: The program-account construction loop in the builder must
account for the loader-v4 state layout: prepend the required LoaderV4State
header before each ELF (or switch to a loader matching the raw ELF layout), and
compute rent from the complete account data size. Preserve the loader_v4 owner
and executable configuration while ensuring the seeded data begins with valid
loader-v4 state.
In `@keeper/src/cache.rs`:
- Around line 40-48: Enforce the documented 256-slot minimum for the cache
capacity used by Cache::new and the lru_capacity configuration path: reject
values below 256 with a startup error before constructing HashCache, or
consistently update the configuration documentation and default to a lower
minimum. Preserve valid capacities and ensure the behavior matches the chosen
documented contract.
In `@keeper/src/lib.rs`:
- Around line 180-188: Update the final-shutdown logic in sync so every ledger
reader receives its own shutdown signal; do not rely on sending multiple
ReadRequest::Shutdown messages through the shared MPMC channel, since one reader
may consume more than one. Use the existing per-reader or broadcast mechanism if
available, and preserve the subsequent superblocks and accounts database
synchronization.
In `@keeper/src/metrics.rs`:
- Around line 69-72: Update the rustdoc for the metrics `init` function to
describe only one-time metrics registration via
`METRICS.get_or_init(Default::default)`. Remove the claim that gauges are seeded
from current caches, leaving the implementation unchanged.
- Around line 79-82: Update account_cache_eviction() to decrement
m.account_cache_entries when an account cache entry is evicted, while preserving
its existing eviction-counter increment. Keep account_cache_insert() increasing
the same gauge so ACCOUNT_CACHE_ENTRIES reflects current occupancy.
In `@keeper/src/subscriptions.rs`:
- Around line 43-50: Correct the rustdoc comments on the subscription fields:
end the `programs` description with a period instead of a semicolon, and update
the `blocks` description to state that it broadcasts newly committed `Block`
values rather than slots. Leave the field types and other comments unchanged.
- Around line 82-88: Update Subscriptions::send so sending and conditional
removal occur under the same bucket lock, preventing subscribe from inserting
between the send and removal. Prefer remove_if_sync with a predicate that sends
the cloned value and removes the channel when sending fails or oneshot is true;
otherwise use the existing send_sync/read_sync path while preserving atomic
removal semantics.
In `@keeper/src/testkit.rs`:
- Around line 3-5: Update the module-level rustdoc describing the Keeper test
configuration to state the values actually used by keeper_builder: 100 ms
blocktime and superblock 4, preserving the existing description of the other
parameters.
---
Nitpick comments:
In `@keeper/src/lib.rs`:
- Around line 213-217: Update the thread creation flow around
thread::Builder::new().name("snapshot-archiver") so a failed spawn removes the
temporary archive at tmp before propagating the spawn error. Preserve the
existing successful spawn behavior and error propagation.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 9274987e-68ef-495d-9594-3228059455ce
📒 Files selected for processing (23)
Cargo.tomlkeeper/Cargo.tomlkeeper/README.mdkeeper/build.rskeeper/src/accessor.rskeeper/src/builder.rskeeper/src/cache.rskeeper/src/error.rskeeper/src/lib.rskeeper/src/metrics.rskeeper/src/subscriptions.rskeeper/src/testkit.rskeeper/src/tests/caches.rskeeper/src/tests/mod.rskeeper/src/tests/recovery.rskeeper/src/tests/subscriptions.rskeeper/src/util.rsprograms/v42-calculator-program/Cargo.tomlprograms/v42-calculator-program/README.mdprograms/v42-calculator-program/src/calculator.rsprograms/v42-calculator-program/src/error.rsprograms/v42-calculator-program/src/lib.rsprograms/v42-calculator-program/src/transfer.rs
| /// Registers keeper metrics once and seeds gauges from current caches. | ||
| pub(crate) fn init() { | ||
| METRICS.get_or_init(Default::default); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The init doc comment does not match the code.
The comment states that init "seeds gauges from current caches". The body only calls METRICS.get_or_init(Default::default), which registers collectors with zero values. No cache is read.
Update the comment to describe registration only.
📝 Proposed fix
-/// Registers keeper metrics once and seeds gauges from current caches.
+/// Registers keeper metrics once in the default Prometheus registry.
pub(crate) fn init() {
METRICS.get_or_init(Default::default);
}As per path instructions: "Check docs and rustdoc for factual consistency with the code. Flag only real mismatches, broken examples, stale comments, or important omissions."
📝 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.
| /// Registers keeper metrics once and seeds gauges from current caches. | |
| pub(crate) fn init() { | |
| METRICS.get_or_init(Default::default); | |
| } | |
| /// Registers keeper metrics once in the default Prometheus registry. | |
| pub(crate) fn init() { | |
| METRICS.get_or_init(Default::default); | |
| } |
🤖 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 `@keeper/src/metrics.rs` around lines 69 - 72, Update the rustdoc for the
metrics `init` function to describe only one-time metrics registration via
`METRICS.get_or_init(Default::default)`. Remove the claim that gauges are seeded
from current caches, leaving the implementation unchanged.
Source: Path instructions
| /// Program account updates keyed by owner pubkey; | ||
| pub(crate) programs: Subscribers<Pubkey, AccountEntry>, | ||
| /// Signature status updates keyed by transaction signature. | ||
| pub(crate) signatures: Subscribers<Signature, TransactionStatus>, | ||
| /// Log broadcasts keyed by mentioned program or account pubkey. | ||
| pub(crate) logs: Subscribers<Pubkey, Arc<TransactionLogs>>, | ||
| /// Broadcast channel for newly committed slots. | ||
| pub(crate) blocks: Sender<Block>, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix two doc defects on the subscription fields.
Line 43 ends with a semicolon instead of a period. Line 49 describes the channel as carrying slots, but the item type is Block.
📝 Proposed doc fix
- /// Program account updates keyed by owner pubkey;
+ /// Program account updates keyed by owner pubkey.
pub(crate) programs: Subscribers<Pubkey, AccountEntry>,
@@
- /// Broadcast channel for newly committed slots.
+ /// Broadcast channel for newly committed blocks.
pub(crate) blocks: Sender<Block>,As per path instructions: "Check docs and rustdoc for factual consistency with the code" and flag "Typos in identifiers, comments, or user-facing strings."
📝 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.
| /// Program account updates keyed by owner pubkey; | |
| pub(crate) programs: Subscribers<Pubkey, AccountEntry>, | |
| /// Signature status updates keyed by transaction signature. | |
| pub(crate) signatures: Subscribers<Signature, TransactionStatus>, | |
| /// Log broadcasts keyed by mentioned program or account pubkey. | |
| pub(crate) logs: Subscribers<Pubkey, Arc<TransactionLogs>>, | |
| /// Broadcast channel for newly committed slots. | |
| pub(crate) blocks: Sender<Block>, | |
| /// Program account updates keyed by owner pubkey. | |
| pub(crate) programs: Subscribers<Pubkey, AccountEntry>, | |
| /// Signature status updates keyed by transaction signature. | |
| pub(crate) signatures: Subscribers<Signature, TransactionStatus>, | |
| /// Log broadcasts keyed by mentioned program or account pubkey. | |
| pub(crate) logs: Subscribers<Pubkey, Arc<TransactionLogs>>, | |
| /// Broadcast channel for newly committed blocks. | |
| pub(crate) blocks: Sender<Block>, |
🤖 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 `@keeper/src/subscriptions.rs` around lines 43 - 50, Correct the rustdoc
comments on the subscription fields: end the `programs` description with a
period instead of a semicolon, and update the `blocks` description to state that
it broadcasts newly committed `Block` values rather than slots. Leave the field
types and other comments unchanged.
Source: Path instructions
1 participant
What changed
Added the
keepercrate as the orchestration layer overaccountsdb,ledger, caches, subscriptions, sysvar seeding, and account snapshot archival.Why
Runtime code needs one consistency boundary that keeps account state, ledger records, cache state, and subscription updates aligned around execution and slot progress.
Closes #12 #41.
Impact
Keeper,KeeperBuilder, storage directory parameters, and keeper-level errors.Reviewer notes
finalizeseals the next superblock and archives the matching account snapshot. Restore keeps a backup before unpacking archived state so failed recovery can roll back.Follow-up
Transaction processor and runtime integration can build on the keeper API.