fix: hook mempool config to sync pipeline#904
Conversation
📝 WalkthroughWalkthroughThis change introduces mempool update tracking across the cardano crate. A new Changes
Sequence Diagram(s)sequenceDiagram
participant sync as Sync Layer
participant wu as WorkUnit
participant domain as Domain
participant mempool as Mempool Store
sync->>wu: execute_work_unit()
wu->>wu: process blocks
sync->>wu: mempool_updates()
wu-->>sync: Vec<MempoolUpdate>
sync->>domain: update_mempool(updates)
loop for each MempoolUpdate
domain->>mempool: confirm(point, seen_txs,<br/>FINALIZE_THRESHOLD,<br/>DROP_THRESHOLD)
end
mempool-->>domain: result
alt on error
domain->>domain: emit warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 1
🤖 Fix all issues with AI agents
In `@crates/core/src/sync.rs`:
- Around line 114-126: The call in update_mempool uses a hardcoded empty slice
for the unseen_txs parameter to domain.mempool().confirm because MempoolUpdate
currently only exposes seen_txs; fix this by either (A) documenting the API
limitation on MempoolUpdate and adding a clear comment in update_mempool
explaining why &[] is used, or (B) extending the MempoolUpdate type to include
an unseen_txs field and thread that through: add unseen_txs to the MempoolUpdate
struct, update any constructors/parsers producing MempoolUpdate, change
work.mempool_updates() consumers to return the new field, and replace the &[]
with &update.unseen_txs when calling domain.mempool().confirm in update_mempool.
🧹 Nitpick comments (2)
crates/core/src/work_unit.rs (1)
10-14: Consider adding#[derive(Debug)]toMempoolUpdate.Other core types in this crate derive
Debug. Without it, any code that needs to log or inspect aMempoolUpdate(e.g., viatracing's?formatting) won't compile.Proposed fix
/// An update for the mempool based on a confirmed block. +#[derive(Debug)] pub struct MempoolUpdate { pub point: ChainPoint, pub seen_txs: Vec<TxHash>, }crates/core/src/sync.rs (1)
16-17: Consider adding threshold values to mempool configuration.The PR mentions "hook mempool config to sync pipeline," but
MEMPOOL_FINALIZE_THRESHOLD(6) andMEMPOOL_DROP_THRESHOLD(2) remain hardcoded constants. While theconfirm()method signature supports configurable thresholds (as shown in tests with varying values), there's no way to adjust them without modifying code. If the intent is to fully hook mempool configuration, these thresholds could be added toRedbMempoolConfig(or a new mempool behavior config section) and read during sync.
| fn update_mempool<D: Domain>(domain: &D, work: &D::WorkUnit) { | ||
| for update in work.mempool_updates() { | ||
| if let Err(e) = domain.mempool().confirm( | ||
| &update.point, | ||
| &update.seen_txs, | ||
| &[], | ||
| MEMPOOL_FINALIZE_THRESHOLD, | ||
| MEMPOOL_DROP_THRESHOLD, | ||
| ) { | ||
| warn!(?e, point = %update.point, "mempool confirm failed"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the confirm function definition in Rust files
rg -n "fn confirm" --type=rust -B2 -A10 | head -80Repository: txpipe/dolos
Length of output: 4924
🏁 Script executed:
#!/bin/bash
# Find mempool_updates definition and other confirm calls
echo "=== Searching for mempool_updates ==="
rg -n "mempool_updates" --type=rust -B2 -A5 | head -100
echo -e "\n=== Searching for all confirm() calls ==="
rg -n "\.confirm\(" --type=rust -B2 -A2 | head -150Repository: txpipe/dolos
Length of output: 9057
🏁 Script executed:
#!/bin/bash
# Find MempoolUpdate structure definition
rg -n "struct MempoolUpdate" --type=rust -A10
rg -n "pub struct MempoolUpdate" --type=rust -A10Repository: txpipe/dolos
Length of output: 1525
Clarify the design constraint for the empty unseen_txs parameter or enhance MempoolUpdate to support it.
The &[] passed as unseen_txs to confirm() is unavoidable with the current API design: MempoolUpdate only provides seen_txs, not unseen_txs. Either document this architectural limitation or extend MempoolUpdate to include unseen_txs if this data becomes available in the future.
🤖 Prompt for AI Agents
In `@crates/core/src/sync.rs` around lines 114 - 126, The call in update_mempool
uses a hardcoded empty slice for the unseen_txs parameter to
domain.mempool().confirm because MempoolUpdate currently only exposes seen_txs;
fix this by either (A) documenting the API limitation on MempoolUpdate and
adding a clear comment in update_mempool explaining why &[] is used, or (B)
extending the MempoolUpdate type to include an unseen_txs field and thread that
through: add unseen_txs to the MempoolUpdate struct, update any
constructors/parsers producing MempoolUpdate, change work.mempool_updates()
consumers to return the new field, and replace the &[] with &update.unseen_txs
when calling domain.mempool().confirm in update_mempool.
1 participant
Summary by CodeRabbit