feat(dash-spv): filter re-match across reorg and safe auto-rebroadcast#170
Merged
Conversation
…iant New variant emitted by the SPV mempool manager when an outgoing transaction demoted by reorg has exhausted its rebroadcast retry budget. UI consumers surface this so the user can decide to abandon, re-sign, or wait. The `wallet_id` field is `Option<WalletId>` because the rebroadcast loop may emit this for a txid that is no longer attributable to any managed wallet. `WalletEvent::wallet_id()` is widened to `Option<WalletId>` to accommodate.
… `ChainReorg` `FiltersManager` now calls `WalletInterface::rewind_to_height` when it observes a `ChainReorg` event, then resets its filter scan cursor to the effective per-wallet floor (`min(fork_height, wallet.synced_height)`) instead of always to `fork_height`. A wallet whose ingest tip is behind the fork never saw the orphaned chain, so the filter pipeline does not need to re-match the segment between its tip and the fork. If the wallet refuses the rewind (e.g. chainlock floor, defense in depth since the SPV cascade already enforces this upstream), log and fall back to `fork_height` for the floor without touching wallet state. Renames `reset_for_reorg`'s parameter to `effective_floor` to make the contract explicit.
…ty checks `MempoolManager` now subscribes to `WalletEvent` broadcasts and drives an auto-rebroadcast pipeline for transactions demoted by a chain reorg: - `WalletEvent::Reorg` enqueues each `demoted_txid` after a chainlock-floor defense check (the wallet already refuses to rewind below its chainlock floor upstream, this branch is belt-and-braces). - Block-height `nLockTime` is compared against the current chain tip; unsatisfied locktimes defer the broadcast. Timestamp locktimes are compared best-effort against wall clock, never against an MTP cache the SPV side does not maintain. - An input-conflict check evicts pending entries whose inputs collide with any transaction confirming on the new chain (delivered via `WalletEvent::BlockProcessed`). - Each attempt advances an exponential backoff capped at one hour. After `MAX_REORG_REBROADCAST_ATTEMPTS` (10) failures the entry is dropped and a `WalletEvent::TxRepeatedlyOrphaned` is emitted so the UI can surface it for user intervention. The manager owns a forwarder `broadcast::channel` so it can both subscribe to wallet events (replaceable via `set_wallet_event_subscription`) and emit `TxRepeatedlyOrphaned` without changing the trait surface. The constructor gains a `chainlock_height: Arc<AtomicU32>` parameter threaded through from `lifecycle.rs`. `MempoolManager` also picks up a `current_tip_height` field updated via `SyncManager::update_target_height`. Tests cover the happy path, input-conflict eviction, locktime defer followed by tip advance, and the retry-cap orphan event.
…et-bridged Add the missing match arm in `FFIWalletEventCallbacks::dispatch`. The variant is left without an FFI surface for now (matching the existing `Reorg` arm) so the C ABI stays unchanged. Wiring a dedicated callback is deferred to a follow-up that surfaces orphaned-tx state to the UI.
|
Manki — Review complete Planner (31s) Review — 9 findings Judge — 7 kept · 0 dropped (24s) Review metadataConfig:
Judge decisions:
Timing:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings 1 and 4 identify the same real bug — TxRepeatedlyOrphaned events go to a dead channel in production, which breaks the PR's stated UI notification goal. The lagged-channel concerns (3 and 5) are also the same issue and a legitimate warning. The test-coverage findings are reasonable suggestions but mostly not warnings at low noise.
📊 7 findings (1 blocker, 1 warning, 4 suggestion, 1 nitpick) · 781 lines · 326s
Manki context
{
"meta": {
"prNumber": 170,
"commitSha": "12fd8438fb95bc0aa9a36ea38667935724d71c5b",
"round": 1,
"timestamp": "2026-05-26T10:36:12.973Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 0,
"maxAutoRounds": 5,
"skipCap": false,
"forceReview": false,
"bypassReason": "within_cap"
},
"trigger": {
"event": "pull_request:opened",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "medium",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 781,
"additions": 748,
"deletions": 33,
"filesReviewed": 8,
"fileTypes": {
".rs": 8
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 8,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/client/lifecycle.rs",
"additions": 6,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/manager.rs",
"additions": 69,
"deletions": 8,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/sync_manager.rs",
"additions": 7,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/manager.rs",
"additions": 578,
"deletions": 13,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/sync_manager.rs",
"additions": 40,
"deletions": 3,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/events.rs",
"additions": 26,
"deletions": 2,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/test_utils/mock_wallet.rs",
"additions": 14,
"deletions": 3,
"changeType": "modified"
}
]
},
"models": {
"planner": "claude-haiku-4-5",
"reviewer": "claude-sonnet-4-6",
"judge": "claude-opus-4-7",
"dedup": "claude-haiku-4-5"
},
"planner": {
"source": "planner",
"used": true,
"coreAgentInjections": [],
"priorRoundEffortDowngrades": [],
"teamSize": 3,
"reviewerEffort": "medium",
"judgeEffort": "high",
"prType": "feat",
"durationMs": 30766,
"language": "rust",
"context": "SPV client for Dash blockchain",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 3,
"findingsKept": 3,
"durationMs": 252014,
"status": "success",
"responseLength": 4400,
"inputTokens": 3,
"outputTokens": 14569,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 263666,
"status": "success",
"responseLength": 3623,
"inputTokens": 3,
"outputTokens": 16255,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"durationMs": 137590,
"status": "success",
"responseLength": 5622,
"inputTokens": 3,
"outputTokens": 8176,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Findings 1 and 4 identify the same real bug — TxRepeatedlyOrphaned events go to a dead channel in production, which breaks the PR's stated UI notification goal. The lagged-channel concerns (3 and 5) are also the same issue and a legitimate warning. The test-coverage findings are reasonable suggestions but mostly not warnings at low noise.",
"confidenceDistribution": {
"high": 2,
"medium": 5,
"low": 0
},
"severityChanges": 7,
"mergedDuplicates": 2,
"durationMs": 23734,
"retryCount": 0,
"verdictReason": "required_present",
"verdictTrace": {
"survivingBlockers": [
{
"file": "dash-spv/src/sync/mempool/manager.rs",
"title": "TxRepeatedlyOrphaned events silently lost in production",
"fingerprint": "dash-spv/src/sync/mempool/manager.rs:725:725:TxRepeatedlyOrphaned-events-silently-lost-in-production"
}
],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "empty",
"openThreadCount": 0,
"resolvedThreadIdCount": 0,
"interRoundDiffState": "unknown",
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 2
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 7,
"severityCounts": {
"blocker": 1,
"warning": 1,
"suggestion": 4,
"nitpick": 1
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 725,
"lineEnd": 725,
"slug": "TxRepeatedlyOrphaned-events-silently-lost-in-production"
},
"severity": "blocker",
"specialist": "Security & Safety",
"suggestedFix": "// In WalletInterface trait, add:\nasync fn emit_event(&self, event: WalletEvent);\n\n// In drive_reorg_rebroadcast_at, replace:\nlet _ = self.wallet_event_tx.send(WalletEvent::TxRepeatedlyOrphaned { ... });\n// with:\nself.wallet.read().await.emit_event(WalletEvent::TxRepeatedlyOrphaned { ... }).await;\n/",
"title": "TxRepeatedlyOrphaned events silently lost in production",
"judgeNotes": "Merged findings 1 and 4 — same issue. Two reviewers independently confirmed that wallet_event_tx has no production subscriber after set_wallet_event_subscription replaces the only receiver, so every orphan notification is silently discarded. Impact: High (user-visible feature broken — stranded txs never surfaced to UI), Likelihood: Certain (every retry-cap hit) → blocker.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 105,
"lineEnd": 105,
"slug": "locktime-satisfied-ignores-input-sequence-numbers--incorrectly-deferring-final-transactions"
},
"severity": "suggestion",
"specialist": "Security & Safety",
"suggestedFix": "fn locktime_satisfied(&self, tip_height: u32, now: SystemTime) -> bool {\n if self.lock_time == 0 {\n return true;\n }\n // Per BIP-65: lock_time is ignored if all inputs have sequence=0xffffffff\n if self.inputs_all_sequence_max {\n return true;\n }\n if self.lock_time < LOC",
"title": "locktime_satisfied ignores input sequence numbers, incorrectly deferring final transactions",
"judgeNotes": "Technically correct per BIP-65 semantics, but the consequence is only a deferred rebroadcast (not incorrect output or fund loss), and only one reviewer flagged it. Impact: Low-Medium (delay until tip catches up), Likelihood: Possible (requires non-final-looking but actually-final tx) → suggestion at low noise.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 581,
"lineEnd": 581,
"slug": "Lagged-wallet-event-channel-silently-drops-Reorg-events--leaving-demoted-txids-unqueued"
},
"severity": "warning",
"specialist": "Security & Safety",
"title": "Lagged wallet event channel silently drops Reorg events, leaving demoted txids unqueued",
"judgeNotes": "Merged findings 3 and 5 — same issue, two reviewers. Real data-loss path: a lagged broadcast subscription permanently drops Reorg events, leaving demoted txids unqueued with no recovery. Impact: High (silent abandonment of user txs), Likelihood: Possible (requires lag under load with default capacity) → warning.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 556,
"lineEnd": 556,
"slug": "Input-conflict-eviction-not-tested-through-the-channel-path"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "async fn test_input_conflict_via_channel_evicts_rebroadcast() {\n let (mut manager, wallet, _requests, _rx) = manager_with_wallet_events().await;\n // ... enqueue a demoted tx as in the existing test ...\n manager.drain_wallet_events().await;\n assert!(manager.pending_rebroadcast.contains_ke",
"title": "Input-conflict eviction not tested through the channel path",
"judgeNotes": "Reasonable test-coverage gap but the direct unit test does cover the eviction logic itself; a channel-path test would only catch a wiring regression. Single reviewer, low impact → suggestion.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 617,
"lineEnd": 617,
"slug": "Chainlock-floor-defense-in-enqueue-demoted-for-rebroadcast-is-untested"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "async fn test_chainlock_floor_prevents_enqueue() {\n let chainlock_height = Arc::new(AtomicU32::new(150)); // > current_tip (0)\n let wallet = Arc::new(RwLock::new(MockWallet::new()));\n let (tx_chan, _rx) = mpsc::unbounded_channel::<NetworkRequest>();\n let mut manager = MempoolManager::new",
"title": "Chainlock-floor defense in enqueue_demoted_for_rebroadcast is untested",
"judgeNotes": "The author explicitly comments this as belt-and-braces defense; a test would be nice but its absence is not a defect in the PR. Single reviewer → suggestion.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/sync_manager.rs",
"lineStart": 94,
"lineEnd": 94,
"slug": "BlockProcessed-handler-s-pending-rebroadcast-removal-is-not-asserted-in-existing-tests"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "Add to or extend \u0060test_block_processed_removes_confirmed_txids\u0060:\n// Also insert the same txid into pending_rebroadcast before firing the event\nmanager.pending_rebroadcast.insert(\n txids[0],\n // construct a minimal RebroadcastState via the public API or test helper\n);\n// ... fire BlockProcessed",
"title": "BlockProcessed handler's pending_rebroadcast removal is not asserted in existing tests",
"judgeNotes": "Valid test gap — the new removal line is effectively untested because the existing test never populates pending_rebroadcast. Worth fixing but not blocking. Single reviewer → suggestion.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 680,
"lineEnd": 680,
"slug": "No-test-verifies-that-the-exponential-backoff-prevents-a-second-immediate-rebroadcast"
},
"severity": "nitpick",
"specialist": "Testing & Coverage",
"suggestedFix": "At the end of \u0060test_reorg_demoted_tx_rebroadcast_happy_path\u0060, after the first broadcast assertion:\n// Immediately calling again should NOT produce a second broadcast\nmanager.drive_reorg_rebroadcast(&requests).await;\nlet second_call_broadcast = std::iter::from_fn(|| rx.try_recv().ok())\n .any(|m| m",
"title": "No test verifies that the exponential backoff prevents a second immediate rebroadcast",
"judgeNotes": "Original severity was already suggestion; at low noise level this is a minor test addition with no specific known regression risk. Single reviewer → drop to nitpick/ignore boundary; keeping as nitpick.",
"judgeConfidence": "medium",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 23,
"outputTokens": 43073,
"totalTokens": 43096,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2888,
"totalTokens": 2897
},
"reviewer": {
"inputTokens": 9,
"outputTokens": 39000,
"totalTokens": 39009
},
"judge": {
"inputTokens": 5,
"outputTokens": 1185,
"totalTokens": 1190
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 0,
"reclassifiedPriorCount": 0
}
}Reviewed commit 12fd843
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #170 +/- ##
==========================================
+ Coverage 67.34% 73.75% +6.41%
==========================================
Files 235 325 +90
Lines 54891 75204 +20313
==========================================
+ Hits 36965 55467 +18502
- Misses 17926 19737 +1811
🚀 New features to boost your workflow:
|
…el-driven paths Addresses four `manki-review` testing-coverage findings on PR #170: - Extend `test_reorg_demoted_tx_rebroadcast_happy_path` with a second immediate `drive_reorg_rebroadcast` call to assert the exponential backoff suppresses a duplicate broadcast and does not bump `attempts`. - Add `test_chainlock_floor_prevents_enqueue` exercising the chainlock-floor defense in `enqueue_demoted_for_rebroadcast` when `chainlock_height > current_tip_height`. - Add `test_input_conflict_via_channel_evicts_rebroadcast` so the `WalletEvent::BlockProcessed` arm of `drain_wallet_events` is exercised through the channel, not by calling `evict_conflicting_rebroadcasts` directly. - Extend `test_block_processed_removes_confirmed_txids` to seed `pending_rebroadcast` via `enqueue_demoted_for_rebroadcast` and assert the confirmed txid is removed from it, locking in the `BlockProcessed` handler's `pending_rebroadcast.remove(txid)` line. Promotes `enqueue_demoted_for_rebroadcast` to `pub(super)` so sibling-module tests can drive the rebroadcast queue through the public API.
Owner
Author
|
@manki review |
|
Manki — Review complete Planner (20s) Review — 4 findings Judge — 3 kept · 0 dropped (25s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Findings 1 and 4 are duplicates flagging the same pub(super) widening for test convenience — legitimate suggestion at most. Findings 2 and 3 are minor test-quality nits that don't affect correctness.
📊 3 findings (1 suggestion, 2 nitpick) · 927 lines · 278s
Manki context
{
"meta": {
"prNumber": 170,
"commitSha": "d046c3d6f93aa8cf234745fe712203b24ee2ec66",
"round": 2,
"timestamp": "2026-05-26T15:03:12.229Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 1,
"maxAutoRounds": 5,
"skipCap": true,
"forceReview": true,
"bypassReason": "manual_review_command"
},
"trigger": {
"event": "issue_comment:created:@manki review",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "medium",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 927,
"additions": 891,
"deletions": 36,
"filesReviewed": 8,
"fileTypes": {
".rs": 8
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 8,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/client/lifecycle.rs",
"additions": 6,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/manager.rs",
"additions": 69,
"deletions": 8,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/sync_manager.rs",
"additions": 7,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/manager.rs",
"additions": 691,
"deletions": 13,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/sync_manager.rs",
"additions": 70,
"deletions": 6,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/events.rs",
"additions": 26,
"deletions": 2,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/test_utils/mock_wallet.rs",
"additions": 14,
"deletions": 3,
"changeType": "modified"
}
]
},
"models": {
"planner": "claude-haiku-4-5",
"reviewer": "claude-sonnet-4-6",
"judge": "claude-opus-4-7",
"dedup": "claude-haiku-4-5"
},
"planner": {
"source": "planner",
"used": true,
"coreAgentInjections": [],
"priorRoundEffortDowngrades": [],
"teamSize": 4,
"reviewerEffort": "medium",
"judgeEffort": "high",
"prType": "feat",
"durationMs": 19884,
"language": "rust",
"context": "Dash SPV blockchain library",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Architecture & Design",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 63965,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 3409,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 219589,
"status": "success",
"responseLength": 2390,
"inputTokens": 3,
"outputTokens": 12291,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 92950,
"status": "success",
"responseLength": 974,
"inputTokens": 3,
"outputTokens": 4499,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 43758,
"status": "success",
"responseLength": 1710,
"inputTokens": 3,
"outputTokens": 2247,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Findings 1 and 4 are duplicates flagging the same \u0060pub(super)\u0060 widening for test convenience — legitimate suggestion at most. Findings 2 and 3 are minor test-quality nits that don't affect correctness.",
"confidenceDistribution": {
"high": 3,
"medium": 0,
"low": 0
},
"severityChanges": 3,
"mergedDuplicates": 1,
"durationMs": 24557,
"retryCount": 0,
"verdictReason": "only_nit_or_suggestion",
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 0,
"resolvedThreadIdCount": 7,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 9115,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 1,
"durationMs": 12444
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 1,
"nitpick": 2
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 588,
"lineEnd": 588,
"slug": "pub-super--visibility-leak-driven-by-test-convenience"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "// In manager.rs — conditional visibility so it doesn't appear in production builds:\n#[cfg(test)]\npub(super) async fn enqueue_demoted_for_rebroadcast(\n &mut self,\n wallet_id: WalletId,\n demoted_txids: &[Txid],\n) { ... }",
"title": "pub(super) visibility leak driven by test convenience",
"judgeNotes": "Merged findings 1 and 4 — same issue flagged by two reviewers. Impact: Low (test-only API hygiene), Likelihood: Possible (visible to future sync::mempool code). Real but minor; #[cfg(test)] gating or routing through the channel path would be cleaner. Two reviewers agreeing keeps it at suggestion rather than dropping to nitpick.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 2164,
"lineEnd": 2164,
"slug": "test-chainlock-floor-prevents-enqueue-relies-on-implicit-implementation-ordering"
},
"severity": "nitpick",
"specialist": "Correctness & Logic",
"suggestedFix": "// After the assert on pending_rebroadcast, clarify the ordering invariant:\nassert!(\n !manager.transactions.contains_key(&txid),\n // The chainlock-floor guard must fire before get_transaction/insert;\n // if the guard fired *after* insertion, this would be a stray entry.\n \"skipped enqueue",
"title": "test_chainlock_floor_prevents_enqueue relies on implicit implementation ordering",
"judgeNotes": "Impact: Low (test could silently weaken if implementation reorders), Likelihood: Unlikely (the pending_rebroadcast assertion still catches the real bug). Single reviewer, documentation-only suggestion. Per low-noise calibration, demote to nitpick.",
"judgeConfidence": "high",
"reachability": "hypothetical",
"reachabilityReasoning": "The concern only manifests if the production code is later reordered to insert before checking the guard; current code doesn't exhibit this."
},
{
"fingerprint": {
"file": "dash-spv/src/sync/mempool/manager.rs",
"lineStart": 2159,
"lineEnd": 2159,
"slug": "Unused-event-subscription-setup-in-chainlock-floor-test-is-misleading"
},
"severity": "nitpick",
"specialist": "Testing & Coverage",
"suggestedFix": "// Remove these two lines — direct call path doesn't need a channel subscription:\n// let wallet_event_rx = wallet.read().await.subscribe_events();\n// manager.set_wallet_event_subscription(wallet_event_rx);",
"title": "Unused event subscription setup in chainlock-floor test is misleading",
"judgeNotes": "Impact: Low (test reader confusion only), Likelihood: Possible. Pure cosmetic test cleanup, single reviewer. Demote to nitpick under low-noise calibration.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 36,
"outputTokens": 26257,
"totalTokens": 26293,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1884,
"totalTokens": 1893
},
"reviewer": {
"inputTokens": 12,
"outputTokens": 22446,
"totalTokens": 22458
},
"judge": {
"inputTokens": 5,
"outputTokens": 607,
"totalTokens": 612
},
"dedup": {
"inputTokens": 10,
"outputTokens": 1320,
"totalTokens": 1330
}
}
},
"verdict": "APPROVE",
"recap": {
"priorRoundCount": 1,
"reclassifiedPriorCount": 0
}
}Reviewed commit d046c3d
…nd clarify chainlock-floor test Keep `enqueue_demoted_for_rebroadcast` private and expose a `#[cfg(test)] pub(super)` wrapper so sibling-module tests in `sync_manager.rs` can seed `pending_rebroadcast` without widening production visibility. Drop the unused `wallet_event_rx` / `set_wallet_event_subscription` setup from `test_chainlock_floor_prevents_enqueue` (the test exercises the direct call, not the channel path) and document the ordering invariant the `transactions` postcondition relies on (the guard must short-circuit before the wallet fetch/insert path).
|
Manki — Review complete Planner (21s) Review — 0 findings Judge — 0 kept · 0 dropped (4s) Review metadataConfig:
Timing:
|
There was a problem hiding this comment.
No findings submitted by reviewers this round — nothing to evaluate.
📊 0 findings (none) · 940 lines · 104s
Manki context
{
"meta": {
"prNumber": 170,
"commitSha": "5d8af355a85fd44648207bfbbab9e6793b59e1ad",
"round": 3,
"timestamp": "2026-05-26T15:09:54.412Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 2,
"maxAutoRounds": 5,
"skipCap": false,
"forceReview": false,
"bypassReason": "within_cap"
},
"trigger": {
"event": "pull_request:synchronize",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "medium",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 940,
"additions": 904,
"deletions": 36,
"filesReviewed": 8,
"fileTypes": {
".rs": 8
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 8,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/client/lifecycle.rs",
"additions": 6,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/manager.rs",
"additions": 69,
"deletions": 8,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/filters/sync_manager.rs",
"additions": 7,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/manager.rs",
"additions": 704,
"deletions": 13,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/mempool/sync_manager.rs",
"additions": 70,
"deletions": 6,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/events.rs",
"additions": 26,
"deletions": 2,
"changeType": "modified"
},
{
"path": "key-wallet-manager/src/test_utils/mock_wallet.rs",
"additions": 14,
"deletions": 3,
"changeType": "modified"
}
]
},
"models": {
"planner": "claude-haiku-4-5",
"reviewer": "claude-sonnet-4-6",
"judge": "claude-opus-4-7",
"dedup": "claude-haiku-4-5"
},
"planner": {
"source": "planner",
"used": true,
"coreAgentInjections": [],
"priorRoundEffortDowngrades": [],
"teamSize": 3,
"reviewerEffort": "medium",
"judgeEffort": "medium",
"prType": "feat",
"durationMs": 20965,
"language": "rust",
"context": "Dash cryptocurrency SPV client",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "high"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 29118,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 1350,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 38577,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 2083,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 67845,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 4079,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 63092,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 3228,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "No findings submitted by reviewers this round — nothing to evaluate.",
"confidenceDistribution": {
"high": 0,
"medium": 0,
"low": 0
},
"severityChanges": 0,
"mergedDuplicates": 0,
"durationMs": 3729,
"retryCount": 0,
"verdictReason": "only_nit_or_suggestion",
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 0,
"resolvedThreadIdCount": 10,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 3250,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 0,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 0
},
"entries": []
},
"usage": {
"inputTokens": 26,
"outputTokens": 12678,
"totalTokens": 12704,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1898,
"totalTokens": 1907
},
"reviewer": {
"inputTokens": 12,
"outputTokens": 10740,
"totalTokens": 10752
},
"judge": {
"inputTokens": 5,
"outputTokens": 40,
"totalTokens": 45
}
}
},
"verdict": "APPROVE",
"recap": {
"priorRoundCount": 2,
"reclassifiedPriorCount": 0
}
}Reviewed commit 5d8af35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 5 of #137. Builds on the wallet rewind from #145 (merged in #169).
FiltersManagerowns theSyncEvent::ChainReorgflow, drivesWalletInterface::rewind_to_heightfirst, then computes the effective floor asmin(fork_height, wallet.synced_height())so a wallet behind the fork re-matches from its own ingest tip instead of the fork height. Re-fetches cfilters across[floor, new_tip]via the existing batch pipeline.MempoolManagerauto-rebroadcast subscribes toWalletEvent::Reorg(via abroadcast::channelforwarder so the trait stays untouched, set up inlifecycle.rs). For eachdemoted_txid:WalletInterface::get_transaction.WalletEvent::BlockProcessed): if any input of a demoted tx collides with an outpoint surfaced on the new chain, it's evicted frompending_rebroadcast.tip_height); timestamp locktimes are checked best-effort against wall clock (wall clock ≥ MTP, conservative — never broadcasts too early, may delay slightly).chainlock_height > current_tip_height, the only inconsistent state where a demoted record could end up under the floor.MempoolManager::rebroadcast_if_duemachinery with per-txid retry counter and exponential backoff.WalletEvent::TxRepeatedlyOrphaned { txid }and stops rebroadcasting. NewWalletEventvariant added inkey-wallet-manager.dash-spvtests: per-wallet effective floor (2) + rebroadcast paths (happy, input-conflict eviction, locktime defer/advance, retry-cap orphan).Deferred (per issue scope)
Conflicted { previous }demotion on input collision:MempoolManagerrefuses to rebroadcast and evicts frompending_rebroadcast, which is the user-visible behavior the spec asked for. Promoting the wallet record toConflicted(vs leaving itMempool) still requires a wallet-side API — that's the self-conflict-detection follow-up explicitly deferred by #145.header_storageintoMempoolManageror surfacing MTP as a manager-level signal is out of scope. Wall-clock substitution is documented as conservative.WalletEvent::TxRepeatedlyOrphaned: scoped out the same way asWalletEvent::Reorgin #168 — exhaustive-match arm consumes/drops with a TODO so the C ABI keeps compiling.Verification
cargo fmt --checkcleancargo clippy --lib --tests -p dash-spv -p dash-spv-ffi -p key-wallet -p key-wallet-manager -- -D warningscleancargo test --lib: dash-spv 544, key-wallet-manager 55, key-wallet 487Closes #146.