feat: add optimized accountsdb crate - #14
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Workspace and crate contract Cargo.toml, accountsdb/Cargo.toml, accountsdb/src/lib.rs, README.md, accountsdb/README.md |
The workspace now includes accountsdb and its storage dependencies. The crate defines public database APIs, errors, account aliases, and storage documentation. |
Persisted storage engine accountsdb/src/store/kv.rs, accountsdb/src/store/mmap.rs, accountsdb/src/store/index.rs, accountsdb/src/store/mod.rs |
The persisted backend adds byte codecs, memory-mapped account storage, LMDB indexes, freelist allocation, transactional updates, checksums, validation, and program iteration. |
Backend routing and account access accountsdb/src/lib.rs, accountsdb/src/volatile.rs, accountsdb/src/metrics.rs, accountsdb/src/tests.rs |
AccountsDB routes accounts between persisted and volatile stores. Loaders and iterators read both stores. Volatile state supports restoration, owner remapping, reset, and metrics. |
Snapshots, backups, and compaction accountsdb/src/snapshot.rs, accountsdb/src/store/defrag.rs, accountsdb/src/tests.rs |
Snapshots, volatile dumps, backups, defragmentation, corruption checks, freelist reuse, checksum stability, and large-account storage growth are implemented and tested. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Sequence Diagram(s)
sequenceDiagram
participant AccountClient
participant AccountsDB
participant PersistedStore
participant VolatileStore
AccountClient->>AccountsDB: upsert account
AccountsDB->>PersistedStore: store authoritative account
AccountsDB->>VolatileStore: store non-authoritative account
AccountClient->>AccountsDB: load account
AccountsDB->>PersistedStore: check persisted storage
AccountsDB->>VolatileStore: check volatile storage
AccountsDB-->>AccountClient: return account entry
Possibly related PRs
- magicblock-labs/magicblock-engine#31: Adds related Solana account and runtime dependencies used by
AccountsDB.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | Deleting solana/account/src/cow/tests.rs is unrelated to the accountsdb objectives and lacks a stated migration rationale. | Restore the deleted cow tests or explain why their removal is required for the accountsdb implementation. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the main change: adding an optimized accountsdb crate. |
| Description check | ✅ Passed | The description explains the accountsdb crate, its storage backends, routing logic, snapshots, and purpose. |
| Linked Issues check | ✅ Passed | The changes implement the accountsdb crate with persisted and volatile backends, routing logic, and storage layout required by issue #6. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
accountsdb
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 @coderabbitai help to get the list of available commands.
|
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. |
c9feec4 to
bb19e9a
Compare
013aa2e to
cd1401e
Compare
0e5d509 to
9d2303c
Compare
|
@coderabbitai 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: 7
🧹 Nitpick comments (2)
accountsdb/src/snapshot.rs (1)
85-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
dumpoverwritesvolatile.dbin place and can leave an undeserializable file.Line 91 opens the target with
truncate(true), so the previous payload is destroyed before the new one is written. If the process stops between the truncate andsync_data, the active tree holds a partialvolatile.db.
VolatileStore::newcallsbincode::deserialize_fromon that file and propagates the error, so the next open fails and the operator must remove the file by hand before the database starts.Write to a temporary file in the same directory, then rename it over the target.
renameis atomic within one filesystem, so the reader observes either the old payload or the new one.🤖 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 `@accountsdb/src/snapshot.rs` around lines 85 - 96, Update Snapshot::dump to serialize the volatile accounts into a temporary file in the destination directory instead of truncating the target VOLATILE_DB_FILE in place. Flush and sync the temporary file, then atomically rename it over the target path, preserving the existing error propagation and ensuring cleanup or safe handling if writing fails.accountsdb/src/metrics.rs (1)
175-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive load counters from
StoreKindinstead of duplicating ordering.
loadusesstore as usizewhileMetrics::newbuildsloadswith an explicitPersisted,Volatile,Absentordering. A futureStoreKindreorder or variant change can mismatch the counter and cause an index panic without a compile error. Generate the counters from the enum at one location.🤖 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 `@accountsdb/src/metrics.rs` at line 175, Update the loads counter initialization in Metrics::new and the related load indexing to derive their ordering and count from StoreKind rather than hard-coded [IntCounter; 3] and explicit Persisted/Volatile/Absent ordering. Centralize the StoreKind-to-index mapping or iterate the enum variants so adding or reordering variants cannot mismatch counters or panic.
🤖 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 `@accountsdb/src/snapshot.rs`:
- Around line 55-63: Update the safety documentation for snapshot to include
self.persisted.defragment() as the first step, before flushing the persisted
backend, cloning the active tree, and rewriting the volatile store. Keep the
exclusive-write-access requirement and remaining ordering description accurate.
- Around line 99-111: Update AccountsDB::backup so it returns the destination
path `to` for both BackupOp::Save and BackupOp::Restore, rather than always
returning `backup`. Document the Restore contract on backup: callers must stop
using or drop the current AccountsDB instance before restoring because the
active backend may be removed while open. Preserve the existing Save behavior
and rename flow.
In `@accountsdb/src/store/defrag.rs`:
- Around line 82-91: Update the doc comment for defragment to explicitly state
that it is not crash-safe: reindex commits relocated offsets before the
corresponding bytes are moved, so a process crash in that window can leave the
persisted tree inconsistent and require rebuilding from a backup. Mention that
this also applies when snapshot defragmentation runs on the active tree, and
document whether rebuild-from-backup is the intended recovery.
In `@accountsdb/src/store/mmap.rs`:
- Around line 236-240: Correct the rustdoc for the Mmap::meta method to state
that it returns a shared or immutable reference to the metadata header, matching
its &DatabaseMeta return type.
In `@accountsdb/src/store/mod.rs`:
- Around line 280-290: Update the Iterator implementation’s next method so LMDB
errors from self.iter.inner.next() are logged before terminating iteration,
rather than being silently converted to None by the ?.ok()? chain. Preserve
normal end-of-iteration behavior for an actual exhausted iterator, and use the
existing logging mechanism and error context available in the module.
In `@accountsdb/src/volatile.rs`:
- Around line 73-87: Update the owner cleanup in the account upsert flow to
compare the new owner key with prev.owner() before calling
programs.remove_if_sync. Only remove pubkey from the previous owner’s set when
the owners differ, preserving the insertion for legal no-op reassignments to the
same owner.
In `@README.md`:
- Around line 201-203: Update the README account-routing description to use
AccountMode::authoritative() rather than mutable() as the criterion for
selecting the live backend. Clarify that Transient accounts may be
runtime-immutable while remaining authoritative and persisted, and ensure the
statement about moving accounts and removing stale copies matches this
authoritative classification.
---
Nitpick comments:
In `@accountsdb/src/metrics.rs`:
- Line 175: Update the loads counter initialization in Metrics::new and the
related load indexing to derive their ordering and count from StoreKind rather
than hard-coded [IntCounter; 3] and explicit Persisted/Volatile/Absent ordering.
Centralize the StoreKind-to-index mapping or iterate the enum variants so adding
or reordering variants cannot mismatch counters or panic.
In `@accountsdb/src/snapshot.rs`:
- Around line 85-96: Update Snapshot::dump to serialize the volatile accounts
into a temporary file in the destination directory instead of truncating the
target VOLATILE_DB_FILE in place. Flush and sync the temporary file, then
atomically rename it over the target path, preserving the existing error
propagation and ensuring cleanup or safe handling if writing fails.
🪄 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: 5c791543-d3af-4279-ab95-72316709061a
📒 Files selected for processing (15)
Cargo.tomlREADME.mdaccountsdb/Cargo.tomlaccountsdb/README.mdaccountsdb/src/lib.rsaccountsdb/src/metrics.rsaccountsdb/src/snapshot.rsaccountsdb/src/store/defrag.rsaccountsdb/src/store/index.rsaccountsdb/src/store/kv.rsaccountsdb/src/store/mmap.rsaccountsdb/src/store/mod.rsaccountsdb/src/tests.rsaccountsdb/src/volatile.rssolana/account/src/cow/tests.rs
💤 Files with no reviewable changes (1)
- solana/account/src/cow/tests.rs
1 participant
What changed
Added the
accountsdbcrate with persisted and volatile stores, routing logic, snapshot support, and mmap/LMDB-backed storage.Why
The engine needs one account database that routes mutable and immutable accounts to the correct backend and removes stale copies when an account changes mode.
Closes #6.
Impact
AccountsDB, loaders, iterators, snapshots, backups, and error handling.StorageUnitand converts to bytes only at mmap/file boundaries.Reviewer notes
Snapshot creation requires exclusive write access. The persisted layout and storage-unit arithmetic are the sharp edges to review closely.
Follow-up
ledgerandkeeperconsume this API upstack.