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:
📝 WalkthroughWalkthroughAdds a trait-based mempool with in-memory (EphemeralMempool) and Redb-backed implementations, wires mempool config and store into adapters and startup, updates callers to the new backend API, and exposes TRP RPC endpoints to inspect and page mempool state and finalized logs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRP as TRP RPC
participant Domain
participant Mempool as MempoolStore
participant Storage as Backend
Client->>TRP: trp.submit(tx)
TRP->>Domain: submit(tx)
Domain->>Mempool: receive(tx)
Mempool->>Storage: persist Pending
Mempool-->>Domain: emit MempoolEvent
Domain-->>Client: ack
Client->>TRP: trp.peekPending()
TRP->>Mempool: peek_pending(limit)
Mempool->>Storage: read Pending
Storage-->>Mempool: pending list
Mempool-->>TRP: PeekPendingResponse
Note over Mempool,Storage: lifecycle: Pending → Propagated → Acknowledged → Confirmed → Finalized
Domain->>Mempool: mark_inflight(hashes)
Mempool->>Storage: move Pending→Inflight (Propagated)
Mempool-->>Domain: MempoolEvent
Domain->>Mempool: confirm(point, seen, unseen)
Mempool->>Storage: update confirmations / rollback unseen
Mempool-->>Domain: MempoolEvent
Client->>TRP: trp.dumpLogs(cursor,limit)
TRP->>Mempool: dump_finalized(cursor,limit)
Mempool->>Storage: paginate Finalized
Storage-->>Mempool: entries
Mempool-->>TRP: DumpLogsResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/sync/submit.rs (1)
46-67:⚠️ Potential issue | 🟠 MajorTransactions marked inflight before peer confirmation — risk of orphaned txs on send failure.
mark_inflight(line 54) moves transactions out of the pending queue beforereply_tx_ids(line 59–64) succeeds. If the peer send fails and the worker restarts (viaor_restart), the newWorkerbegins with an emptypropagated_hashes(line 144), while the txs remain in the inflight/Propagated state in the mempool. Since they're no longer pending, they won't be re-propagated. They can only recover if chain-sync observes them on-chain or rolls them back; otherwise they remain stuck indefinitely.Consider either:
- Moving
mark_inflightto after the successful peer reply, or- Adding a recovery mechanism that re-queues orphaned inflight txs back to pending on worker restart.
crates/core/src/mempool.rs (1)
257-301:⚠️ Potential issue | 🟠 Major
exclude_inflight_stxismisleadingly scans only pending transactions, not inflight — inflight UTxOs are invisible during validation.All three helper functions (
scan_mempool_utxos,exclude_inflight_stxis,select_mempool_utxos) callmempool.peek_pending(usize::MAX)only. When transactions move to inflight viamark_inflight, they are removed from the pending queue and added to the inflight table. This creates two issues:
- Double-spend risk: An incoming transaction can spend inputs already locked by inflight txs, since
exclude_inflight_stxisdoesn't check inflight state.- Broken chaining: An incoming transaction cannot chain from outputs produced by inflight txs.
The function name
exclude_inflight_stxisis particularly misleading — it actually scans and excludes pending inputs, not inflight ones. The debug message at line 284 ("checking inflight tx") contradicts the implementation.These functions should also scan
peek_inflightwhen gathering UTxOs for validation, or the function name should be clarified to reflect that only pending transactions are considered.
🤖 Fix all issues with AI agents
In `@crates/core/src/builtin/mempool.rs`:
- Around line 91-103: The receive method on EphemeralMempool adds incoming
MempoolTxs unconditionally, causing duplicates; update EphemeralMempool::receive
to first read or write-lock self.state and check whether tx.hash already exists
in any of state.pending, state.proposed, or state.committed (or whichever state
collections track existing txs) and return Ok(()) early if found; only push to
state.pending, call self.notify(tx) and self.log_state(&state) when the tx is
new. Use the existing state variable, the pending field, and the receive method
to locate where to add this guard.
- Around line 20-26: MempoolState.finalized_log currently grows unbounded;
change the data structure or add pruning so old entries are dropped—e.g.,
replace finalized_log: Vec<MempoolTx> or keep it but enforce a max size constant
(MAX_FINALIZED_LOG_LEN) and trim older entries when appending in finalize()
and/or during housekeeping(): after pushing new MempoolTx, if
finalized_log.len() > MAX_FINALIZED_LOG_LEN remove oldest entries (or switch to
VecDeque and pop_front) to ensure the finalized_log size remains bounded and
memory usage is capped.
- Around line 247-275: check_status currently only looks in acknowledged,
inflight, and pending so finalized transactions moved by finalize() into
finalized_log return Unknown; update check_status to additionally check
state.finalized_log (e.g., state.finalized_log.get(tx_hash) or iter().find(...)
depending on its structure) before returning Unknown and, if found, return a
TxStatus with stage: MempoolTxStage::Finalized and the appropriate confirmations
and confirmed_at taken from the finalized_log entry; keep the existing
inflight/pending/acknowledged checks and use finalized_log as the final
fallback.
In `@crates/redb3/src/mempool.rs`:
- Around line 120-133: The current into_mempool_tx implementation uses
copy_from_slice and try_into().unwrap(), which can panic on malformed lengths;
change these to safe, non-panicking conversions: for the hash field, replace
hash_bytes.copy_from_slice(&self.hash) with a length check / try_into pattern
(e.g., if let Ok(arr) = self.hash.as_slice().try_into() { TxHash::from(arr) }
else { /* fallback: default hash or return Err */ }) and for confirmed_at (and
the other sites that call ChainPoint::from_bytes) replace
ChainPoint::from_bytes(b[..].try_into().unwrap()) with a safe match on
b[..].try_into() and only call ChainPoint::from_bytes when Ok(arr), otherwise
map to None or propagate an error; update the function signature to return
Result<MempoolTx, Error> if you prefer failing fast instead of using defaults,
and apply the same safe pattern to the other occurrences of
ChainPoint::from_bytes in the codebase.
- Around line 650-692: The implementation of check_status currently only queries
INFLIGHT_TABLE and PENDING_TABLE and returns MempoolTxStage::Unknown for
finalized transactions; update the check_status method to also open and query
FINALIZED_LOG_TABLE using the existing read transaction rx, lookup the tx_hash
key, deserialize the finalized entry (e.g., via the same pattern used for
InflightRecord or the appropriate FinalizedRecord type) and return its TxStatus
(mapping to MempoolTxStage::Finalized) when found; use the same error-tolerant
open_table/get logic as for INFLIGHT_TABLE and PENDING_TABLE so check_status
returns the finalized status instead of Unknown.
In `@crates/trp/src/methods.rs`:
- Around line 264-270: The code uses limit + 1 when calling mempool.peek_pending
(and similarly in trp_peek_inflight), which can overflow when params.limit is
usize::MAX; change the arithmetic to safe operations (e.g., compute let
peek_count = limit.saturating_add(1) or cap limit to a reasonable max before
adding) and pass peek_count to mempool.peek_pending; apply the same fix in
trp_peek_inflight to avoid panic/wraparound and preserve the has_more check
using peek_count instead of limit + 1.
🧹 Nitpick comments (10)
src/sync/emulator.rs (1)
52-77: Back-to-backmark_inflight→mark_acknowledgedwith no error handling.In the emulator path,
peek_pending→mark_inflight→mark_acknowledgedis called sequentially without checking return values or propagating errors. This works for the emulator since there's no real network propagation, but the silent ignoring of potential failures could mask issues during development/debugging.Consider at minimum logging if the mark operations affect fewer transactions than expected.
crates/trp/src/lib.rs (1)
83-145: New RPC registrations follow the established pattern consistently.The four new endpoints (
trp.checkStatus,trp.dumpLogs,trp.peekPending,trp.peekInflight) all mirror the existingtrp.resolve/trp.submitregistration pattern including metrics tracking and error handling.The repetitive boilerplate across all six registrations could be extracted into a helper macro or closure, but this is optional given the existing codebase style.
crates/core/src/lib.rs (1)
617-619: Hardcoded finalization threshold — consider making it configurable.
MEMPOOL_FINALIZATION_THRESHOLDis hardcoded to 10 confirmations. The other housekeeping parameters (max_historyfor WAL and archive) are config-driven. Consider adding this toMempoolStoreConfigfor operational flexibility.src/sync/submit.rs (1)
80-106: Potential hot-loop with 10-second sleep as fallback.The
schedule_unfulfilledmethod sleeps 10 seconds when no pending txs are available (line 98). The TODO on line 96-97 already notes the need to watch the mempool for changes. With the newsubscribe()method onMempoolStore, this could be improved by awaiting the mempool event stream instead of polling.Would you like me to draft an implementation that uses the mempool's
subscribe()stream to wake up when new txs arrive?crates/core/src/builtin/mempool.rs (2)
219-245: Redundant cloning infinalize— bothfinalizedandevent_txare identical.After
acknowledged.remove(&hash), the ownedtxis cloned twice to producefinalizedandevent_tx, both withstageset toFinalized. One clone can be eliminated:Proposed simplification
for hash in to_finalize { if let Some(tx) = state.acknowledged.remove(&hash) { - let mut finalized = tx.clone(); + let mut finalized = tx; finalized.stage = MempoolTxStage::Finalized; - state.finalized_log.push(finalized); - let mut event_tx = tx.clone(); - event_tx.stage = MempoolTxStage::Finalized; - info!(tx.hash = %tx.hash, "tx finalized"); - self.notify(event_tx); + info!(tx.hash = %finalized.hash, "tx finalized"); + self.notify(finalized.clone()); + state.finalized_log.push(finalized); } }
42-47: Broadcast channel capacity of 16 may be tight under load.The broadcast channel is created with a capacity of 16 (line 44). During bursts (e.g., a block confirmation touching many acknowledged txs), multiple events are emitted in a single
confirm()orfinalize()call. Slow subscribers will receiveLaggederrors, mapped toMempoolError::Internal. Consider making this configurable or using a larger default.crates/trp/src/methods.rs (2)
139-176: No upper bound on the number of hashes intrp_check_status.A client can submit an arbitrarily large
hashesarray, each triggering acheck_statuscall (which in the Redb backend performs a read transaction with table scans). Consider adding a reasonable cap (e.g., 100) to prevent abuse.Proposed cap on input size
let params: CheckStatusParams = params.parse()?; + if params.hashes.len() > 100 { + return Err(Error::InvalidParams("too many hashes (max 100)".into())); + } + let mempool = context.domain.mempool();
202-235:trp_dump_logshas no upper bound onlimit.A client could request
limit: 999999999, causing a large allocation and heavy I/O indump_finalized. Consider capping similarly to the peek endpoints.Proposed cap
let cursor = params.cursor.unwrap_or(0); - let limit = params.limit.unwrap_or(50); + let limit = params.limit.unwrap_or(50).min(1000); let include_payload = params.include_payload.unwrap_or(false);crates/redb3/src/mempool.rs (2)
112-151:unwrap()on CBOR encode/decode may panic on corrupted persistent data.
serialize()anddeserialize()in bothFinalizedLogEntryandInflightRecorduseunwrap(). For an in-memory-only store this is fine, but sinceRedbMempoolis a persistent store, corrupted data on disk would cause a panic instead of a graceful error.The same pattern appears in
peek_pending(line 461) andmark_inflight(line 499).Consider returning
Resultfromdeserializeand propagating errors, or at minimum logging and skipping corrupted entries.
368-399:with_write_txsilently swallows write errors.When the closure or commit fails, the error is logged at
warnlevel and the function returns without notifying the caller. Since the lifecycle methods (mark_inflight,mark_acknowledged,confirm,finalize) return(), there's no way to propagate the failure. This means state transitions can silently fail — e.g., amark_inflightcould fail to persist, leaving txs stuck in pending without any external indication.This is a deliberate trade-off given the
()return type, but worth documenting or revisiting if reliability is a concern.
| fn into_mempool_tx(self) -> MempoolTx { | ||
| let mut hash_bytes = [0u8; 32]; | ||
| hash_bytes.copy_from_slice(&self.hash); | ||
| MempoolTx { | ||
| hash: TxHash::from(hash_bytes), | ||
| payload: self.payload.unwrap_or(EraCbor(0, vec![])), | ||
| stage: MempoolTxStage::Finalized, | ||
| confirmations: self.confirmations, | ||
| confirmed_at: self.confirmed_at.map(|b| { | ||
| ChainPoint::from_bytes(b[..].try_into().unwrap()) | ||
| }), | ||
| report: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
try_into().unwrap() on confirmed_at bytes can panic if length is unexpected.
Lines 129, 185, and 211 all do ChainPoint::from_bytes(b[..].try_into().unwrap()). If a stored confirmed_at blob has an unexpected length (e.g., due to corruption or a schema change), this panics. Similarly, line 122 does hash_bytes.copy_from_slice(&self.hash) which panics if self.hash.len() != 32.
🤖 Prompt for AI Agents
In `@crates/redb3/src/mempool.rs` around lines 120 - 133, The current
into_mempool_tx implementation uses copy_from_slice and try_into().unwrap(),
which can panic on malformed lengths; change these to safe, non-panicking
conversions: for the hash field, replace hash_bytes.copy_from_slice(&self.hash)
with a length check / try_into pattern (e.g., if let Ok(arr) =
self.hash.as_slice().try_into() { TxHash::from(arr) } else { /* fallback:
default hash or return Err */ }) and for confirmed_at (and the other sites that
call ChainPoint::from_bytes) replace
ChainPoint::from_bytes(b[..].try_into().unwrap()) with a safe match on
b[..].try_into() and only call ChainPoint::from_bytes when Ok(arr), otherwise
map to None or propagate an error; update the function signature to return
Result<MempoolTx, Error> if you prefer failing fast instead of using defaults,
and apply the same safe pattern to the other occurrences of
ChainPoint::from_bytes in the codebase.
| let limit = params.limit.unwrap_or(50); | ||
| let include_payload = params.include_payload.unwrap_or(false); | ||
|
|
||
| let mempool = context.domain.mempool(); | ||
| let peeked = mempool.peek_pending(limit + 1); | ||
|
|
||
| let has_more = peeked.len() > limit; |
There was a problem hiding this comment.
Integer overflow: limit + 1 can wrap around when limit == usize::MAX.
If a client sends a very large limit value, limit + 1 on line 268 will overflow (panic in debug, wrap to 0 in release). The same issue exists in trp_peek_inflight at line 320. Use saturating_add or cap the limit.
Proposed fix using saturating_add and a cap
let limit = params.limit.unwrap_or(50);
+ let limit = limit.min(1000); // cap to a reasonable max
let include_payload = params.include_payload.unwrap_or(false);
let mempool = context.domain.mempool();
- let peeked = mempool.peek_pending(limit + 1);
+ let peeked = mempool.peek_pending(limit.saturating_add(1));Apply the same pattern to trp_peek_inflight.
🤖 Prompt for AI Agents
In `@crates/trp/src/methods.rs` around lines 264 - 270, The code uses limit + 1
when calling mempool.peek_pending (and similarly in trp_peek_inflight), which
can overflow when params.limit is usize::MAX; change the arithmetic to safe
operations (e.g., compute let peek_count = limit.saturating_add(1) or cap limit
to a reasonable max before adding) and pass peek_count to mempool.peek_pending;
apply the same fix in trp_peek_inflight to avoid panic/wraparound and preserve
the has_more check using peek_count instead of limit + 1.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/mempool.rs (1)
262-307:⚠️ Potential issue | 🟠 MajorMempool-aware UTxO scanning only considers
pendingtransactions — inflight/acknowledged txs become invisible.
scan_mempool_utxos,exclude_inflight_stxis, andselect_mempool_utxosall callmempool.peek_pending(usize::MAX)exclusively (lines 268, 291, 312). Once a transaction transitions from Pending to Propagated/Acknowledged/Confirmed viamark_inflight, its produced UTxOs and consumed inputs will no longer be accounted for byMempoolAwareUtxoStore.This means a second mempool transaction that depends on outputs from a first (now-inflight) transaction will fail UTxO resolution. Consider also scanning inflight txs via
peek_inflight.#!/bin/bash # Check if peek_inflight is used anywhere in the UTxO-aware store or related code rg -n 'peek_inflight' --type=rust -C3
🤖 Fix all issues with AI agents
In `@crates/core/src/builtin/mempool.rs`:
- Around line 288-308: dump_finalized currently treats cursor as a positional
index into the VecDeque finalized_log which breaks when finalized_log is pruned;
change the ephemeral mempool to use a monotonic sequence number as the cursor:
add a monotonically increasing sequence counter on the mempool state and attach
the sequence (e.g., seq: u64) to each finalized item (MempoolTx or a small
wrapper stored in finalized_log), increment the counter in the finalize path
(where items are pushed and pruning via MAX_FINALIZED_LOG happens), and modify
dump_finalized to select items by seq >= cursor (not by index), return items in
seq order up to limit, and set next_cursor to the sequence after the last
returned item (or None if done) so cursors remain stable across pruning;
references: dump_finalized, finalized_log, MempoolPage, MempoolTx,
MAX_FINALIZED_LOG, RedbMempool.
- Around line 191-223: In confirm (EphemeralMempool::confirm) handle unseen_txs
rollbacks by moving the tx out of state.acknowledged and into state.pending so
it will be re-submitted: for each tx_hash in unseen_txs, take the entry from
state.acknowledged (get_mut or remove), set its stage to
MempoolTxStage::Pending, reset confirmations/confirmed_at as done, insert it
into state.pending (using the same key/TxHash), notify with the RolledBack stage
as before, and ensure the acknowledged entry is removed so the in-memory
behavior matches RedbMempool and the trait contract.
In `@crates/testing/src/harness/cardano.rs`:
- Around line 256-264: The code holds the write lock from
self.domain.write_chain() in variable chain and then calls
self.drain_with_callback() while that guard is still held, causing a deadlock
because drain_with_callback() re-acquires the same RwLock; fix by dropping the
write guard before calling drain_with_callback(): check can_receive_block()
while the guard exists, then explicitly drop(chain) (or close that scope) prior
to calling self.drain_with_callback(&mut on_work) and after the callback
finishes re-acquire the write lock (e.g., call self.domain.write_chain() again)
to call receive_block(block)? on the fresh guard.
🧹 Nitpick comments (4)
tests/epoch_pots/main.rs (2)
87-105:SeedConfigappears to be entirely unused — consider removing it instead of suppressing the warning.
config.seedsis never accessed inrun_epoch_pots_test(onlyconfig.snapshotsis used at line 418). If this struct is being kept for future use, the#[allow(dead_code)]annotations are fine, but removing unused code is generally preferable to silencing warnings.
404-415: Suppressingclippy::too_many_argumentsis acceptable for test code.If this function's parameter list grows further, consider grouping the ground-truth CSV strings into a small struct (e.g.,
GroundTruth { epochs, pparams, eras, delegation, stake, rewards }), which would also make the macro-generated call sites cleaner.crates/redb3/src/mempool.rs (2)
401-425:receive_innerperforms a full table scan for duplicate detection — O(n) per receive.Lines 408–414 iterate every entry in
PENDING_TABLEto check for a duplicate hash. Since the pending table key is[seq ++ hash](ordered by sequence), there's no efficient hash-based lookup. Under high submission rates, this linear scan could become a bottleneck.Consider maintaining a secondary hash index (e.g., a separate
PENDING_HASH_TABLEmappinghash → seq) or also checking theINFLIGHT_TABLE(which is keyed by hash) before scanning pending.
368-399:with_write_txsilently swallows all errors — callers have no indication of failure.
mark_inflight,mark_acknowledged,confirm, andfinalizeall usewith_write_tx, which logs a warning and returns without propagating errors. Since the trait methods return(), there's no way to signal failure to the caller. A failedconfirmorfinalize, for instance, would silently leave the mempool in an inconsistent state.This is a design constraint from the trait, but worth noting: if a write fails, the in-memory broadcast events are also skipped (lines 396–398), so subscribers won't be notified — but the caller also won't know to retry. Consider adding metrics or a health flag for operational visibility.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/core/src/builtin/mempool.rs (1)
96-109: Duplicate check only inspectspending, notinflightoracknowledged.If a transaction was already promoted to inflight/acknowledged, a second
receivecall with the same hash would succeed and create a duplicate inpending. The Redb backend has the same limitation (only checks pending table), so this is at least consistent, but worth noting for the overall design.crates/core/src/mempool.rs (1)
286-331:peek_pending(usize::MAX)may be expensive with a large Redb-backed mempool.
scan_mempool_utxos,exclude_inflight_stxis, andselect_mempool_utxosall callmempool.peek_pending(usize::MAX), which loads every pending tx into memory. For the Redb backend, this means a full table scan and deserialization of all entries. This is acceptable for a small mempool, but worth keeping in mind if the pending queue grows large.crates/redb3/src/mempool.rs (2)
383-414:with_write_txsilently swallows errors — operations may silently fail.If the write transaction or commit fails, the method logs a warning and returns without propagating the error. This means
mark_inflight,mark_acknowledged,confirm, andfinalizecan silently drop updates. This is a deliberate "best effort" design, but callers have no way to detect or retry failures.Consider whether at least
confirmandfinalizeshould propagate errors, since silent data loss in these paths could lead to transactions being stuck in an incorrect state indefinitely.
416-440:receive_innerscans the full pending table for duplicate detection.The duplicate check iterates every entry in
PENDING_TABLE(line 423) to compare hashes. With the compositePendingKeylayout (seq + hash), there's no direct hash-based lookup. For a small mempool this is fine, but it becomes O(n) per receive call. If the pending queue grows large, consider a secondary index or a separate hash-set table for O(1) dedup.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/redb3/src/mempool.rs`:
- Around line 725-756: with_write_tx currently swallows all errors (begin_write,
closure f, and wx.commit) by logging a warning and returning (), which makes
callers like mark_inflight, mark_acknowledged, confirm, and finalize unaware of
failures; change with_write_tx to return a Result<Vec<MempoolTx>,
RedbMempoolError> (or Result<(), RedbMempoolError> depending on caller
expectations), propagate the underlying errors from self.db.begin_write(), the
closure f(&wx), and wx.commit() instead of only warn!-logging, and update
callers (mark_inflight, mark_acknowledged, confirm, finalize) to handle or
propagate the Result so failures aren’t silently ignored.
- Around line 758-767: The receive_inner method currently only checks
PendingTable::contains and can insert a tx that already exists in InflightTable;
update receive_inner to also query InflightTable::contains(&wx, &tx.hash) (using
the same write transaction) and return MempoolError::DuplicateTx if present
before calling PendingTable::insert, ensuring no duplicate appears in both
tables and preserving the existing wx.commit() and self.notify(tx) flow.
🧹 Nitpick comments (4)
crates/redb3/src/mempool.rs (4)
445-468:PendingTable::containsandcontains_hashdo a full table scan to find a hash.Because the pending table is keyed by
(seq, tx_hash), every hash lookup requires iterating all entries — O(n) per call. This is called on everyreceiveand will degrade as the pending queue grows. Consider adding a secondary index table (hash → seq) or restructuring the key to allow direct hash lookups.
698-699: Broadcast channel capacity of 16 may be too small for burst scenarios.If mempool operations produce events faster than subscribers consume them (e.g., a batch
finalizeof many transactions), theBroadcastStreamwill returnLaggederrors and subscribers will miss events. Consider making the capacity configurable or using a larger default.
843-874: Confirm logic collects all inflight entries on every call.
InflightTable::collect_all(wx)(line 848) reads every inflight record into memory on eachconfirmcall, even though only theseen_txsandunseen_txssets are relevant. For a large inflight table, this is wasteful. Consider iterating only the hashes present inseen_txsandunseen_txs, and separately handling the "stale" mark for remaining entries.
876-895:finalizealso collects all inflight entries into memory.Same pattern as
confirm—collect_allloads everything. For large inflight tables, consider iterating in-place or only selecting confirmed entries.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/redb3/src/mempool.rs (3)
445-468:PendingTable::contains/contains_hashperform O(n) full-table scans.Because
DbPendingKeyorders by sequence number first, there is no efficient index on the tx hash. Bothcontains(called on everyreceive) andcontains_hash(called on everycheck_status) iterate the entire pending table. Under sustained mempool load this will become a bottleneck.Consider adding a secondary lookup table (e.g.,
TableDefinition<DbTxHash, ()>) that maps hash → existence, enabling O(log n) duplicate checks while keeping the seq-ordered table for FIFO iteration.
876-895:finalizeclones the payload unnecessarily.
record.to_mempool_tx(hash)on line 883 clones the payload, thenrecord.into_finalized_entry(hash)on line 884 consumes the original. You can avoid the extra allocation by consuming the record first and building the eventMempoolTxfrom theFinalizedEntryfields (or by splitting differently).♻️ Sketch
for (hash, record) in entries { if record.is_finalizable(threshold) { - let mut tx = record.to_mempool_tx(hash); - let log_entry = record.into_finalized_entry(hash); + let log_entry = record.into_finalized_entry(hash); + let mut tx = log_entry.into_mempool_tx(); + // into_mempool_tx already sets stage = Finalized InflightTable::remove(wx, &hash)?; - FinalizedTable::append(wx, log_entry)?; - tx.stage = MempoolTxStage::Finalized; + FinalizedTable::append(wx, log_entry)?; // ← but log_entry is consumed aboveThis requires either cloning
log_entry(cheaper if payload is stored by ref) or extracting the event fields before appending. One clean option: build theMempoolTxevent directly insideinto_finalized_entryreturning both.
698-699: Broadcast channel capacity of 16 may be too small under load.If consumers are slow,
BroadcastStreamreceivers will getLaggederrors and miss events. With high tx throughput (e.g., a burst of confirmations or finalizations), 16 slots can fill quickly. Consider making this configurable viaRedbMempoolConfigor increasing the default.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/core/src/builtin/mempool.rs`:
- Around line 1-6: The PR must run and pass workspace lint/build/test checks;
run the specified commands (cargo clippy --workspace --all-targets
--all-features, cargo build --workspace --all-targets --all-features, cargo test
--workspace --all-features) and fix any clippy warnings, build errors, or
failing tests introduced by changes (start by checking the crate containing
builtin/mempool.rs and any touched modules), ensuring the workspace compiles
cleanly and tests pass before merging.
🧹 Nitpick comments (3)
skills/redb-patterns-and-practices/SKILL.md (3)
19-62: Add guidance on data validation to prevent read panics.The example implementations use
unwrap()infrom_bytesandas_bytes(lines 29, 56, 59), which is unavoidable given redb's trait API. However, this means corrupted or invalid data will panic on read. Consider adding a subsection noting:
- Data should be validated before writes
- Invalid CBOR or mismatched array lengths will panic on deserialization
- Callers of
table.insert()should ensure domain invariants hold📝 Suggested addition after line 62
} + +### Validation considerations + +Since `redb::Value::from_bytes` does not return `Result`, any decode/parse failure will panic. To prevent read-time panics: +- Validate domain constraints before calling `table.insert()`. +- Ensure CBOR-encoded values are well-formed before writes. +- For fixed-width keys, ensure byte slices match the expected length.
3-3: Clarify "implementation-agnostic" claim.The statement "These are implementation-agnostic" is potentially misleading. These patterns are specific to redb and not applicable to other storage backends like fjall. Consider rephrasing to: "These apply to all redb-backed storage modules within this crate" or similar.
Based on learnings, storage implementations should maintain consistency between redb3 and fjall backends, but this document only addresses redb3 patterns.
📝 Proposed clarification
-Rules and patterns extracted from `crates/redb3/` (wal, state, archive, mempool modules). These are **implementation-agnostic** — they apply whenever adding or modifying redb-backed storage in this crate. +Rules and patterns extracted from `crates/redb3/` (wal, state, archive, mempool modules). These patterns apply to all modules within this crate that use redb-backed storage (wal, state, archive, mempool, etc.).
265-278: Consider symbol-based references for maintainability.The reference table uses hardcoded line ranges (e.g., "
64-131") which will drift as the codebase evolves. While the "" prefix signals approximate ranges, these references require manual upkeep. Consider alternatively referencing by symbol/struct name (e.g., "DbChainPoint impl") to reduce maintenance burden, or accept this as a living-document trade-off.
| //! Built-in in-memory mempool implementation. | ||
| //! | ||
| //! A basic FIFO mempool backed by in-memory data structures and a | ||
| //! broadcast channel for event notifications. Suitable for single-node | ||
| //! deployments and development/testing. | ||
|
|
There was a problem hiding this comment.
Run required workspace checks (clippy/build/test).
Please run the required Rust workspace checks before merging.
#!/bin/bash
cargo clippy --workspace --all-targets --all-features
cargo build --workspace --all-targets --all-features
cargo test --workspace --all-featuresAs per coding guidelines: “Run cargo clippy --workspace --all-targets --all-features and resolve all clippy warnings before committing changes. Ensure the project builds without warnings by running cargo build --workspace --all-targets --all-features. Run cargo test --workspace --all-features to verify functionality of all changes”.
🤖 Prompt for AI Agents
In `@crates/core/src/builtin/mempool.rs` around lines 1 - 6, The PR must run and
pass workspace lint/build/test checks; run the specified commands (cargo clippy
--workspace --all-targets --all-features, cargo build --workspace --all-targets
--all-features, cargo test --workspace --all-features) and fix any clippy
warnings, build errors, or failing tests introduced by changes (start by
checking the crate containing builtin/mempool.rs and any touched modules),
ensuring the workspace compiles cleanly and tests pass before merging.
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)
crates/core/src/mempool.rs (1)
298-362:⚠️ Potential issue | 🟠 MajorInclude inflight txs in mempool-aware UTxO scans.
scan_mempool_utxos,exclude_inflight_stxis, andselect_mempool_utxosonly iteratepeek_pending(). Once a tx moves to Propagated/Acknowledged, its produced UTxOs and spent inputs are ignored, which can cause invalid validation of chained txs. Consider iterating over both pending and inflight/acknowledged.🐛 Proposed fix (scan both pending and inflight)
- for mtx in mempool.peek_pending(usize::MAX) { + for mtx in mempool + .peek_pending(usize::MAX) + .into_iter() + .chain(mempool.peek_inflight(usize::MAX)) + {Apply the same pattern in:
exclude_inflight_stxis(Line 321)select_mempool_utxos(Line 342)
🤖 Fix all issues with AI agents
In `@crates/redb3/src/mempool.rs`:
- Around line 205-222: The from_bytes implementations (e.g., fn from_bytes for
DbEraCbor, InflightRecord, FinalizedEntry) currently call
minicbor::decode(data).unwrap() which can panic on malformed CBOR; replace the
unwrap with error handling: attempt minicbor::decode(data) and on Err either
return a safe default/sentinel instance (implement Default for the affected
wrapper types if needed) or propagate/log the decode error via your error
wrapper so the DB read doesn't panic. Ensure you update the matching type
constructors (DbEraCbor(...), InflightRecord(...), FinalizedEntry(...)) to
accept the fallback/default value or return a Result if you choose propagation,
and add logging of the corruption detail when handling the error.
🧹 Nitpick comments (2)
crates/redb3/src/mempool.rs (1)
736-747: Broadcast channel capacity of 16 may be insufficient under load.The broadcast channel created at line 738 has a capacity of 16. During high-throughput scenarios (e.g., many transactions being processed), slow subscribers could cause
Laggederrors. Consider making this configurable or using a larger default.src/adapters/storage.rs (1)
106-119:unwrap_or_default()may silently produce empty path.At line 112,
config.storage.mempool_path().unwrap_or_default()returns an emptyPathBufif the path resolution fails. This could lead to attempting to create a database at the current directory rather than failing explicitly. Other store openers (e.g.,open_wal_storeat line 75) have the same pattern, so this appears intentional, but consider whether explicit failure would be safer for persistent stores.
| fn from_bytes<'a>(data: &'a [u8]) -> Self::SelfType<'a> | ||
| where | ||
| Self: 'a, | ||
| { | ||
| Self(minicbor::decode(data).unwrap()) | ||
| } | ||
|
|
||
| fn as_bytes<'a, 'b: 'a>(value: &'a Self::SelfType<'b>) -> Self::AsBytes<'a> | ||
| where | ||
| Self: 'b, | ||
| { | ||
| minicbor::to_vec(&value.0).unwrap() | ||
| } | ||
|
|
||
| fn type_name() -> redb::TypeName { | ||
| redb::TypeName::new("mempool_era_cbor") | ||
| } | ||
| } |
There was a problem hiding this comment.
minicbor::decode().unwrap() can panic on malformed CBOR.
The from_bytes implementations for DbEraCbor, InflightRecord, and FinalizedEntry (lines 209, 268, 316) use minicbor::decode(data).unwrap() which will panic if the stored data is corrupted or incompatible with the current schema.
Consider returning a default/sentinel value or propagating the error through a wrapper that logs the corruption.
🤖 Prompt for AI Agents
In `@crates/redb3/src/mempool.rs` around lines 205 - 222, The from_bytes
implementations (e.g., fn from_bytes for DbEraCbor, InflightRecord,
FinalizedEntry) currently call minicbor::decode(data).unwrap() which can panic
on malformed CBOR; replace the unwrap with error handling: attempt
minicbor::decode(data) and on Err either return a safe default/sentinel instance
(implement Default for the affected wrapper types if needed) or propagate/log
the decode error via your error wrapper so the DB read doesn't panic. Ensure you
update the matching type constructors (DbEraCbor(...), InflightRecord(...),
FinalizedEntry(...)) to accept the fallback/default value or return a Result if
you choose propagation, and add logging of the corruption detail when handling
the error.
1 participant
Summary by CodeRabbit
Release Notes
New Features
trp.checkStatus,trp.dumpLogs,trp.peekPending,trp.peekInflightfor mempool inspectionImprovements