feat: implement superblock based ledger crate - #15
Conversation
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.
|
|
Warning Review limit reached
Next review available in: 59 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 (12)
📝 WalkthroughWalkthroughAdded the ChangesLedger storage and access
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)
✨ 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 |
![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.