feat: implement superblock based ledger crate - #15
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. |
28ab64f to
91b7b9c
Compare
bdbb6cc to
82c74d2
Compare
aea5687 to
5f18904
Compare
0c9530e to
ef7e5e8
Compare
c8e3a23 to
d17dabf
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
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 |
|---|---|
Ledger contracts and package wiring Cargo.toml, ledger/Cargo.toml, ledger/src/error.rs, ledger/src/schema.rs, ledger/src/request.rs, ledger/README.md |
Added the workspace package, ledger wire formats, read request and response types, cancellation and timeout handling, shared errors, and storage lifecycle documentation. |
Storage primitives and LMDB indexes ledger/src/storage.rs, ledger/src/index.rs, ledger/src/tests/index.rs |
Added append-file and memory-mapped metadata storage. Added LMDB indexes for blocks, transactions, and account execution spans. Added codec and ordering tests. |
Ledger lifecycle and append pipeline ledger/src/lib.rs, ledger/src/appender.rs, ledger/src/metrics.rs |
Added ledger initialization, superblock management, background event processing, synchronization, sealing, rotation, retention, and Prometheus metrics. |
Read, replay, and response processing ledger/src/reader.rs, ledger/src/tests/integration.rs, ledger/src/tests/mod.rs |
Added indexed transaction, block, range, account-history, and replay reads. Added decoding, pagination, cancellation, corruption handling, restart, retention, and cross-superblock integration coverage. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Sequence Diagram(s)
sequenceDiagram
participant LedgerHandle
participant LedgerAppender
participant Superblock
participant Index
participant LedgerReader
LedgerHandle->>LedgerAppender: submit transaction and execution events
LedgerAppender->>Superblock: append blockstore and execution records
LedgerAppender->>Index: commit transaction, block, and account spans
LedgerAppender->>Superblock: sync cursors and seal or rotate
LedgerHandle->>LedgerReader: submit read or replay request
LedgerReader->>Index: resolve indexed spans
LedgerReader->>Superblock: read and decode stored records
LedgerReader-->>LedgerHandle: return response or replay entries
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the primary change: implementing a superblock-based ledger crate. |
| Description check | ✅ Passed | The description accurately summarizes the ledger implementation and its purpose for transaction and execution tracking. |
| Linked Issues check | ✅ Passed | The changes implement the ledger crate and align transaction and execution data with block and slot context as required by issue #7. |
| Out of Scope Changes check | ✅ Passed | The workspace updates, documentation, metrics, storage, indexes, workers, and tests directly support the ledger objectives in issue #7. |
| 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
ledger
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
ledger/src/reader.rs (2)
268-281: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStop the replay loop when the receiver disconnects.
The
breakat line 276 exits only the innerwhileloop. The outerforloop then opens the next superblock, decodes an entry, and callsblocking_sendagain, which fails immediately. Return instead, so a disconnected receiver ends the request at once.replayalso never checksrequest.cancelled(), unlike the other handlers in this file.♻️ Proposed fix for replay cancellation
for superblock in self.ledger.iter_after(*superblock) { let limit = superblock.meta.cursors.blockstore.load(Acquire); let mut reader = BufReader::new((&superblock.blockstore).take(limit)); while !reader.fill_buf()?.is_empty() { + if request.cancelled() { + return Ok(()); + } let entry = blockstore::decode(&mut reader).map_err(Into::<Error>::into)?; if tx.blocking_send(entry).is_err() { - break; + return Ok(()); } } }🤖 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 `@ledger/src/reader.rs` around lines 268 - 281, Update replay to return immediately when tx.blocking_send(entry) fails, rather than only breaking the inner while loop. Also check request.cancelled() while processing superblocks and entries, returning early when cancellation is requested, consistent with the other handlers.
350-356: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the full copy of the block byte range.
Line 352 clones
self.buffers.blockstore. The range covers every transaction between the two block boundaries, so the copy scales with the block size. The test atledger/src/tests/integration.rslines 240-274 already stores a payload above 10 MiB.Take the buffer out with
mem::take, decode from the owned value, and put it back at the end. That keeps the mutable borrow ofselfavailable inside the loop without a copy.🤖 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 `@ledger/src/reader.rs` around lines 350 - 356, Update the block-reading flow around the cursor loop to replace the clone of self.buffers.blockstore with mem::take, decoding from the owned buffer while retaining mutable access to self during iteration. Restore the buffer to self.buffers.blockstore after decoding completes, including the cancellation path.ledger/src/lib.rs (1)
130-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd SAFETY comments to the
MetaMap::newcall sites.
SuperblockWriter::newinledger/src/appender.rs(lines 268-271) documents the sameunsafecontract. Line 134 here and line 256 inSuperblock::opencallMetaMap::newwithout any SAFETY note. Add the equivalent justification so everyunsafecall site states why the contract holds.♻️ Proposed documentation for the unsafe call sites
let meta = directory.join(LEDGER_META); + // SAFETY: `LedgerMeta` is a fixed-layout header whose shared fields are + // atomics, satisfying `MetaMap::new`'s contract. `directory` is created + // above by `create_dir_all`. let meta = unsafe { MetaMap::<LedgerMeta>::new(&meta) }?;🤖 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 `@ledger/src/lib.rs` around lines 130 - 148, Add SAFETY comments immediately before the unsafe MetaMap::new calls in Ledger::new and Superblock::open, matching the justification already documented in SuperblockWriter::new and explaining why the mapped metadata remains valid for the call.Source: Path instructions
ledger/src/appender.rs (1)
346-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the SAFETY justification.
The comment states that the LMDB write transaction is a part of
LedgerAppender. It is not.runholds that transaction in a local variable (line 92), and no field of the struct stores it. Name the field that actually blocks the automaticSendimplementation, for example thezstdCompressorinsideSuperblockWriter, and state why moving it to one thread is sound. The same applies toLedgerReaderinledger/src/reader.rs(lines 469-471), whose comment is accurate about decoder state.🤖 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 `@ledger/src/appender.rs` around lines 346 - 348, Update the SAFETY comment above LedgerAppender’s unsafe impl Send to identify the non-Send zstd Compressor held by SuperblockWriter as the field preventing automatic Send, and state that moving the appender to its single background thread is sound. Leave LedgerReader’s existing decoder-state justification unchanged.Source: Path instructions
ledger/src/tests/integration.rs (1)
57-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
SyncandResetevents.
appendcloses the sender to end the appender, so no test sendsEvent::Sync { response, is_final }. The README describes that path at lines 35-39: a final sync flushes preceding events, reports its durability result, and then closes the appender. No test sendsEvent::Reseteither, althoughwrite_resetwrites a marker and the replay shape check at lines 415-421 has aReset(_)arm.Add one test that drives a
Syncwithis_finalset to both values, and one that appends aResetand asserts the marker appears in replay.🤖 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 `@ledger/src/tests/integration.rs` around lines 57 - 72, Add integration tests covering both branches of Event::Sync by sending a sync event with is_final set to false and true, asserting the response durability result and final-sync shutdown behavior. Add a separate test that uses append to write Event::Reset, then reopens or replays the ledger and verifies the reset marker is present, reusing the existing replay shape checks and test helpers.Source: Path instructions
ledger/src/metrics.rs (1)
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the source of the
ledger_superblocksgauge.The gauge reads
ledger.meta.head(), which is the active superblock id.LedgerMetaalso has a distinctsuperblocksfield that holds the retained count (ledger/src/lib.rslines 186 andledger/src/appender.rsline 119). The help text, "Current total ledger superblocks allocated from genesis", does not say which of the two values the gauge reports. Either reportmeta.superblocksfor the retained count, or state in the help text that the value is the newest superblock id.Also applies to: 127-127
🤖 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 `@ledger/src/metrics.rs` around lines 34 - 37, Clarify the ledger_superblocks metric by either changing its source to LedgerMeta.superblocks for the retained count or updating the SUPERBLOCKS help text to explicitly identify the value as the newest active superblock id from meta.head(). Keep the metric name and surrounding registration unchanged.Source: Path instructions
🤖 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 `@ledger/README.md`:
- Around line 31-33: The README description of the optional testkit feature is
incomplete. Update the testkit sentence to mention both effects: reducing the
index LMDB map_size and setting the ledger reader worker count to 1, while
preserving the statement that the on-disk format is unchanged.
In `@ledger/src/appender.rs`:
- Around line 157-169: Bound the lifetime of entries inserted by
write_transaction in self.pending by defining an explicit retention rule. If
transactions may only pair within the current block, clear unpaired entries in
write_block; otherwise enforce age or maximum-count eviction and log each
eviction, while preserving write_execution’s missing-counterpart behavior.
- Around line 142-145: Update the Event::Superblock handling in run to commit
the current open index write transaction before calling write_superblock and
rotate. Ensure the transaction is finalized before the blockstore is sealed,
while preserving the existing superblock-writing and rotation flow.
In `@ledger/src/index.rs`:
- Around line 27-28: Update the INDEX_DBS constant or its documentation so they
agree: Index::new creates three named databases—transactions, slots, and
accounts—so set INDEX_DBS to 3 unless an extra reserved slot is intentional, in
which case document that reservation explicitly.
- Around line 78-83: Enforce the 39-bit offset limit in SuperblockWriter before
deriving spans from AppendFile::cursor. Update write_blockstore and
write_execution to detect a cursor that would exceed the limit, rotate/seal the
current file, and continue with a fresh cursor before calling Span::new; retain
Span::new’s assertions as a defensive check.
In `@ledger/src/lib.rs`:
- Around line 169-191: Guard the entire retention pass in Ledger::truncate with
a dedicated mutex acquired before reading the oldest superblock and held through
purge, removal, metadata updates, and flush. Ensure all callers, including
write_block and public handle access, use this same serialization path so
concurrent truncations cannot process the same superblock or decrement the
counter twice.
In `@ledger/src/reader.rs`:
- Around line 344-350: Guard the previous-boundary lookup in the block-reading
flow by replacing the unguarded slot - 1 calculation with checked subtraction.
Treat a None result from checked_sub as no previous boundary and use start = 0,
while preserving the existing index lookup and offset-plus-size behavior when a
preceding slot exists.
- Around line 240-266: Update blocks to clamp the requested Range<Slot> against
the retained range in self.ledger.meta.range before counting, allocating, or
iterating. Enforce a fixed maximum range length by rejecting or truncating
oversized requests, so range.clone().count(), Vec::with_capacity, and the slot
lookup loop cannot process unbounded caller input.
- Around line 298-309: Update LedgerReader initialization and the execution
method to ensure buffers.details has capacity matching the writer’s
MAX_ENTRY_SIZE/Span::MAX_SIZE contract rather than the current 4 * MB
reservation. Use the frame or upper_bound to derive the required decompressed
capacity, or reserve the established maximum bound before decompress_to_buffer
in execution, while preserving existing decoding behavior.
In `@ledger/src/schema.rs`:
- Around line 138-150: Correct the documentation comments on Cpis and
Instruction: describe Cpis as inner instructions executed from an outer
instruction, and update stack_height to state that it contains the invocation
stack height without calling it optional. Keep the struct fields and types
unchanged.
- Line 25: Rename the public type alias OwnedBlockestoreEntry to
OwnedBlockstoreEntry in schema.rs, and update every reference in request.rs and
the blockstore module to use the corrected name consistently.
In `@ledger/src/storage.rs`:
- Around line 282-289: Add #[repr(C)] to the BlockRange struct so its AtomicU64
fields have a stable C-compatible order when embedded in LedgerMeta and
SuperblockMeta and persisted through MetaMap.
- Around line 174-193: Update the existing-file branch in unsafe fn new to
validate file.metadata().len() before MmapOptions::len(size). If the file is
shorter than size, extend it with set_len(size as u64) before mapping; preserve
the existing mapping and return flow for files already large enough.
- Around line 251-257: Update superblocks() so the calculation of start uses
saturating subtraction for the outer head-minus-count operation, preventing
underflow when separate atomic reads observe a torn state. Preserve the existing
retained-range semantics, including the active head.
---
Nitpick comments:
In `@ledger/src/appender.rs`:
- Around line 346-348: Update the SAFETY comment above LedgerAppender’s unsafe
impl Send to identify the non-Send zstd Compressor held by SuperblockWriter as
the field preventing automatic Send, and state that moving the appender to its
single background thread is sound. Leave LedgerReader’s existing decoder-state
justification unchanged.
In `@ledger/src/lib.rs`:
- Around line 130-148: Add SAFETY comments immediately before the unsafe
MetaMap::new calls in Ledger::new and Superblock::open, matching the
justification already documented in SuperblockWriter::new and explaining why the
mapped metadata remains valid for the call.
In `@ledger/src/metrics.rs`:
- Around line 34-37: Clarify the ledger_superblocks metric by either changing
its source to LedgerMeta.superblocks for the retained count or updating the
SUPERBLOCKS help text to explicitly identify the value as the newest active
superblock id from meta.head(). Keep the metric name and surrounding
registration unchanged.
In `@ledger/src/reader.rs`:
- Around line 268-281: Update replay to return immediately when
tx.blocking_send(entry) fails, rather than only breaking the inner while loop.
Also check request.cancelled() while processing superblocks and entries,
returning early when cancellation is requested, consistent with the other
handlers.
- Around line 350-356: Update the block-reading flow around the cursor loop to
replace the clone of self.buffers.blockstore with mem::take, decoding from the
owned buffer while retaining mutable access to self during iteration. Restore
the buffer to self.buffers.blockstore after decoding completes, including the
cancellation path.
In `@ledger/src/tests/integration.rs`:
- Around line 57-72: Add integration tests covering both branches of Event::Sync
by sending a sync event with is_final set to false and true, asserting the
response durability result and final-sync shutdown behavior. Add a separate test
that uses append to write Event::Reset, then reopens or replays the ledger and
verifies the reset marker is present, reusing the existing replay shape checks
and test helpers.
🪄 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: 55f4d57b-33f7-49f5-9ada-42e48a367f8c
📒 Files selected for processing (15)
Cargo.tomlledger/Cargo.tomlledger/README.mdledger/src/appender.rsledger/src/error.rsledger/src/index.rsledger/src/lib.rsledger/src/metrics.rsledger/src/reader.rsledger/src/request.rsledger/src/schema.rsledger/src/storage.rsledger/src/tests/index.rsledger/src/tests/integration.rsledger/src/tests/mod.rs
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
What changed
Implemented the
ledgercrate with superblock storage, append and reader workers, block/execution schemas, LMDB indexes, request handling, and retention hooks.Why
The engine needs a durable record of transaction execution, block metadata, and superblock boundaries that can be read independently from account storage.
Closes #7.
Impact
Ledger,LedgerHandle, retainedSuperblockstorage, and reader/appender service wiring.Reviewer notes
Superblocks own their files and indexes so retention can remove whole directories.
headis the active superblock, while sealed superblocks remain readable until truncation.Follow-up
keepercoordinates this ledger withaccountsdbupstack.