feat(dash-spv): masternode list and quorum rewind across reorg#165
Merged
Conversation
Adds a primitive that drops every piece of cached state whose anchor sits above a target height: `masternode_lists`, hash-keyed `known_snapshots` and `rotated_quorums_per_cycle` (via `block_container` lookup), and `quorum_statuses` height sets. Orphaned hashes are dropped conservatively. The container is trimmed last so hash lookups remain valid during cleanup. Idempotent under repeated invocation.
Truncates the shared masternode engine, prunes the manager's height-tracked sync state, refreshes `last_synced_block_hash` from the surviving engine tip, clears straggler/dedup state, transitions to `Syncing`, and dispatches a fresh QRInfo for the new tip.
`MasternodesManager::handle_sync_event` now intercepts `SyncEvent::ChainReorg` before any other arm and delegates to `rewind_to_height`, mirroring the cascade pattern already in `FilterHeadersManager` and `BlocksManager`.
`ChainLockManager` listens for `SyncEvent::ChainReorg` and flips `masternode_ready` back to `false` while dropping any cached `pending_validation`. Subsequent chainlocks are cached until the next `MasternodeStateUpdated` flips readiness back on, which mirrors the pre-startup path.
`InstantSendManager` listens for `SyncEvent::ChainReorg` and clears `pending_instantlocks`. Pre-reorg IS locks were validated against a quorum view that no longer matches the new chain, so retrying them would either succeed under the wrong cycle or fail spuriously. Future IS locks re-received from the network are picked up via the existing `MasternodeStateUpdated` retry path. Also drops the dead `MasternodesManager::is_synced_for_height` query and its test.
Adds a regtest integration test that orchestrates a reorg via the existing controller node (`invalidateblock` + replacement chain), observes the SPV's `ChainReorg` event, verifies the engine truncates above the fork before refilling, and confirms a post-rewind `MasternodeStateUpdated` lands above the fork. Also adds `MasternodeTestContext::mine_reorg` and a `wait_for_chain_reorg_event` helper.
Adds an integration test for the full DIP-24 rotation-cycle reorg path: mine a DKG cycle, capture its commitment hash, invalidate enough to roll back the commitment block, mine a replacement DKG cycle, and verify the engine drops the orphaned cycle from `rotated_quorums_per_cycle` and refills it under the replacement key. The replacement DKG is flaky on the existing regtest masternode harness so the test is gated behind `#[ignore]` with a tracking note tied to #142. The body still compiles to catch refactors that break the helper surface.
…at-comments, import `Range`
|
Manki — Review complete Planner (20s) Review — 17 findings Judge — 8 kept · 2 dropped (43s) Review metadataConfig:
Judge decisions:
Timing:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #165 +/- ##
==========================================
+ Coverage 73.13% 73.36% +0.23%
==========================================
Files 324 324
Lines 72512 73074 +562
==========================================
+ Hits 53029 53611 +582
+ Misses 19483 19463 -20
🚀 New features to boost your workflow:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Real bug worth fixing: rewind_to_height sets SyncState::Syncing then ?-propagates from send_qrinfo_for_tip, leaving the manager wedged in Syncing with no in-flight request if dispatch fails. The truncation logic for rotated_quorums_per_cycle is also keyed wrong — entries from cycles whose commitment landed in the orphaned branch survive truncation and only get fixed if a later QRInfo overwrites them.
📊 8 findings (3 warning, 3 suggestion, 2 nitpick) · 661 lines · 587s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "7be02407e8cfc544333bebe3f361aa9bce2b4708",
"round": 1,
"timestamp": "2026-05-25T14:52:38.993Z",
"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": 661,
"additions": 646,
"deletions": 15,
"filesReviewed": 10,
"fileTypes": {
".rs": 10
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 34,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 129,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 36,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 170,
"deletions": 3,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 5,
"reviewerEffort": "medium",
"judgeEffort": "high",
"prType": "feat",
"durationMs": 19921,
"language": "rust",
"context": "Dash cryptocurrency protocol library - SPV client",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "high"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 4,
"findingsKept": 4,
"durationMs": 514276,
"status": "success",
"responseLength": 4876,
"inputTokens": 3,
"outputTokens": 31531,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 4,
"findingsKept": 3,
"durationMs": 406587,
"status": "success",
"responseLength": 5233,
"inputTokens": 3,
"outputTokens": 24258,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 3,
"findingsKept": 2,
"durationMs": 280198,
"status": "success",
"responseLength": 5305,
"inputTokens": 3,
"outputTokens": 16494,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 3,
"findingsKept": 2,
"durationMs": 165746,
"status": "success",
"responseLength": 3775,
"inputTokens": 3,
"outputTokens": 9433,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 3,
"findingsKept": 1,
"durationMs": 210112,
"status": "success",
"responseLength": 3887,
"inputTokens": 3,
"outputTokens": 12472,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Real bug worth fixing: \u0060rewind_to_height\u0060 sets \u0060SyncState::Syncing\u0060 then \u0060?\u0060-propagates from \u0060send_qrinfo_for_tip\u0060, leaving the manager wedged in Syncing with no in-flight request if dispatch fails. The truncation logic for \u0060rotated_quorums_per_cycle\u0060 is also keyed wrong — entries from cycles whose commitment landed in the orphaned branch survive truncation and only get fixed if a later QRInfo overwrites them.",
"confidenceDistribution": {
"high": 6,
"medium": 4,
"low": 0
},
"severityChanges": 10,
"mergedDuplicates": 7,
"durationMs": 42919,
"retryCount": 0,
"verdictReason": "novel_suggestion",
"defensiveHardeningCount": 2,
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [
{
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"fingerprint": "dash/src/sml/masternode_list_engine/mod.rs:1475:1475:rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above"
},
{
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"fingerprint": "dash-spv/tests/dashd_masternode/tests_sync.rs:610:610:Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg"
},
{
"file": "dash-spv/src/sync/masternodes/manager.rs",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"fingerprint": "dash-spv/src/sync/masternodes/manager.rs:609:609:Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind"
}
],
"unresolvedPriors": []
},
"openThreadsState": "empty",
"openThreadCount": 0,
"resolvedThreadIdCount": 0,
"interRoundDiffState": "unknown",
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 7
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 8,
"severityCounts": {
"blocker": 0,
"warning": 3,
"suggestion": 3,
"nitpick": 2
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 604,
"lineEnd": 604,
"slug": "rewind-to-height-discards-events-returned-by-send-qrinfo-for-tip"
},
"severity": "nitpick",
"specialist": "Security & Safety",
"suggestedFix": "// Replace the two-line discard pattern:\nself.send_qrinfo_for_tip(requests).await?;\nOk(vec![])\n// with:\nself.send_qrinfo_for_tip(requests).await",
"title": "rewind_to_height discards events returned by send_qrinfo_for_tip",
"judgeNotes": "Merged findings 1, 12, and 16 — same issue flagged by three reviewers. send_qrinfo_for_tip currently returns Ok(vec![]) on all paths, so today this is purely a maintainability inconsistency. Impact: Low (no observable effect today), Likelihood: Possible (only if future code adds events) → suggestion. Three reviewers flagged it but two as warning and one as suggestion; the actual current behavior is benign so suggestion is appropriate.",
"judgeConfidence": "high",
"reachability": "hypothetical",
"reachabilityReasoning": "send_qrinfo_for_tip's current implementation returns an empty event vec on all paths, so no events are actually being dropped today.",
"tags": [
"defensive-hardening"
],
"originalSeverity": "suggestion"
},
{
"fingerprint": {
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"lineStart": 1475,
"lineEnd": 1475,
"slug": "rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "// Option A: during truncate_above, also drop any rotated quorum entry whose\n// commitment height is above the fork. Requires tracking commitment height per entry.\n\n// Option B (conservative, no schema change): after truncate_above, clear *all*\n// rotated_quorums_per_cycle entries for cycles whose c",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"judgeNotes": "Real correctness concern: cycle-boundary block hash maps to height ≤ fork_height even when the DKG commitment was mined in the orphaned branch above fork_height. masternode_ready=false prevents immediate misuse but the stale entry persists if the post-rewind QRInfo doesn't overwrite that exact cycle. Impact: High (potential acceptance of orphan-quorum-signed ChainLocks), Likelihood: Possible (specific reorg geometry) → warning.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"lineStart": 610,
"lineEnd": 610,
"slug": "Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg"
},
"severity": "warning",
"specialist": "Security & Safety",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"judgeNotes": "Merged findings 3, 6, 9, and 15 — four reviewers independently flagged this race. The intermediate engine assertion runs after the ChainReorg broadcast but with no synchronization barrier ensuring rewind_to_height has acquired/released the write lock and that no QRInfo response has been applied. Impact: Medium (test flake, not production bug), Likelihood: Possible (timing-dependent on fast regtest loopback) → warning. Strong consensus among reviewers.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 590,
"lineEnd": 590,
"slug": "Two-separate-engine-lock-acquisitions-in-rewind-to-height-create-logical-inconsistency-window"
},
"severity": "suggestion",
"specialist": "Security & Safety",
"suggestedFix": "{\n let mut engine = self.engine.write().await;\n engine.truncate_above(fork_height);\n let tip_entry = engine.masternode_lists.iter().next_back();\n self.sync_state.last_synced_block_hash = tip_entry.map(|(_, list)| list.block_hash);\n let engine_tip_height = tip_entry.map(|(h, _)| *h).un",
"title": "Two separate engine lock acquisitions in rewind_to_height create logical inconsistency window",
"judgeNotes": "Merged findings 4, 7, and 14 — three reviewers noted the same redundancy. No await points between the write and read lock acquisitions in the current code, so this is style/clarity, not a correctness issue. Impact: Low (cleanliness), Likelihood: Certain (every call) → suggestion.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 609,
"lineEnd": 609,
"slug": "Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind"
},
"severity": "warning",
"specialist": "Correctness & Logic",
"suggestedFix": "// After truncation and sync_state cleanup, guard the error:\nself.set_state(SyncState::Syncing);\nif let Err(e) = self.send_qrinfo_for_tip(requests).await {\n tracing::error!(\"send_qrinfo_for_tip failed during rewind: {e}\");\n // Reset to a state where BlockHeaderSyncComplete will retry\n self.",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"judgeNotes": "Genuine recovery hole: state is set to Syncing before the fallible call, and the tick handler's recovery paths don't fire for a post-rewind empty pipeline. Error propagates to the caller but the manager's own state doesn't self-heal without a reconnect. Impact: High (silent stuck state), Likelihood: Possible (network/peer failure during rewind) → warning. Only one reviewer flagged it but the reasoning checks out.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/tests/dashd_masternode/helpers.rs",
"lineStart": 205,
"lineEnd": 205,
"slug": "-wait-for-chain-reorg-event--accepts-any-ChainReorg-event--not-filtered-by-fork-height"
},
"severity": "nitpick",
"specialist": "Testing & Coverage",
"suggestedFix": "pub(super) async fn wait_for_chain_reorg_event(\n event_receiver: &mut broadcast::Receiver<SyncEvent>,\n expected_fork_height: Option<u32>, // None = accept any\n timeout_secs: u64,\n) -> (u32, dashcore::BlockHash) {\n // ...\n Ok(SyncEvent::ChainReorg { fork_height, new_tip, .. }) => {\n ",
"title": "\u0060wait_for_chain_reorg_event\u0060 accepts any ChainReorg event, not filtered by fork height",
"judgeNotes": "Docstring/behavior mismatch is real but in practice tests don't have incidental reorgs to race against. Impact: Low (test ergonomics), Likelihood: Unlikely (controlled regtest) → suggestion.",
"judgeConfidence": "medium",
"reachability": "hypothetical",
"reachabilityReasoning": "Tests construct a single deliberate reorg via mine_reorg; no background reorg source exists in the regtest harness to trigger the mismatch.",
"tags": [
"defensive-hardening"
],
"originalSeverity": "suggestion"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 1051,
"lineEnd": 1051,
"slug": "Missing-test---rewind-to-height--with-all-engine-state-above-fork-height--empty-engine-"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "#[tokio::test]\nasync fn test_rewind_to_height_empty_engine_after_truncation() {\n let (mut manager, requests, mut rx) = make_synced_incremental_manager(120).await;\n {\n let mut engine = manager.engine.write().await;\n // All lists are above the fork; truncation empties the engine.\n ",
"title": "Missing test: \u0060rewind_to_height\u0060 with all engine state above fork height (empty engine)",
"judgeNotes": "Reasonable coverage gap. Impact: Low (test breadth), Likelihood: Possible → suggestion. Only one reviewer flagged it.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"lineStart": 87,
"lineEnd": 87,
"slug": "ChainLockManager-internal-state-mutated-directly-from-sync-manager-rs--requiring-pub-super--visibility-leak"
},
"severity": "suggestion",
"specialist": "Architecture & Design",
"suggestedFix": "// In manager.rs\npub(super) fn reset_for_reorg(&mut self) {\n self.masternode_ready = false;\n self.pending_validation = None;\n}\n\n// In sync_manager.rs ChainReorg arm\nself.reset_for_reorg();\nreturn Ok(vec![]);",
"title": "ChainLockManager internal state mutated directly from sync_manager.rs, requiring pub(super) visibility leak",
"judgeNotes": "Legitimate encapsulation concern — the reset-for-reorg state transition belongs in a method. Impact: Low (code hygiene), Likelihood: Certain (every reorg) → suggestion. Single reviewer, no correctness impact.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 29,
"outputTokens": 99091,
"totalTokens": 99120,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1863,
"totalTokens": 1872
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 94188,
"totalTokens": 94203
},
"judge": {
"inputTokens": 5,
"outputTokens": 3040,
"totalTokens": 3045
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 0,
"reclassifiedPriorCount": 0
}
}Reviewed commit 7be0240
…`test_masternode_list_rewind_across_dip3_reorg` On macOS ARM, the filter pipeline had not yet downloaded filters for the 3 most recent blocks when `mine_reorg` was called. After the reorg those block hashes were orphaned, so dashd disconnected every reconnect attempt that included a `GetCFilters` for an orphaned stop hash, preventing the `GetHeaders2` fork-detection request from ever getting a response and causing the 60 s `wait_for_chain_reorg_event` timeout. Ubuntu/Windows passed because the slower VMs gave the filter pipeline enough time to finish before the reorg was triggered. Fix: add `wait_for_full_sync` (waits for `SyncProgress::is_synced()`) and call it after `wait_for_mn_state_event_above` and before `mine_reorg`, ensuring no orphan-hash filter requests remain in flight.
|
Manki — Review complete Planner (30s) Review — 2 findings Judge — 0 kept · 2 dropped (27s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Only one of eight prior threads got a fix (the test race via wait_for_full_sync); seven others remain untouched. Two new findings about that helper — the dedup suggestion is fine, the is_synced() coverage warning is borderline but not a blocker.
Blocking unresolved review threads:
- [Warning] rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above —
dash/src/sml/masternode_list_engine/mod.rs:1475 - [Warning] Integration test races against QRInfo response when asserting engine state after ChainReorg —
dash-spv/tests/dashd_masternode/tests_sync.rs:610 - [Warning] Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind —
dash-spv/src/sync/masternodes/manager.rs:609
📊 0 findings (none) · 703 lines · 142s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "0f921a2da9854714688a12c94d06161813b5d9ca",
"round": 2,
"timestamp": "2026-05-25T15:01:07.170Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 1,
"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": 703,
"additions": 689,
"deletions": 14,
"filesReviewed": 10,
"fileTypes": {
".rs": 10
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 34,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 129,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 77,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 172,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 5,
"reviewerEffort": "medium",
"judgeEffort": "medium",
"prType": "feat",
"durationMs": 29541,
"language": "rust",
"context": "Dash SPV client protocol implementation",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 21448,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 1007,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 71020,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 3613,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 41517,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 2049,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 54518,
"status": "success",
"responseLength": 2163,
"inputTokens": 3,
"outputTokens": 2063,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 52454,
"status": "success",
"responseLength": 1458,
"inputTokens": 3,
"outputTokens": 2593,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Only one of eight prior threads got a fix (the test race via wait_for_full_sync); seven others remain untouched. Two new findings about that helper — the dedup suggestion is fine, the is_synced() coverage warning is borderline but not a blocker.",
"confidenceDistribution": {
"high": 1,
"medium": 1,
"low": 0
},
"severityChanges": 2,
"mergedDuplicates": 0,
"durationMs": 27015,
"retryCount": 0,
"verdictReason": "prior_unaddressed",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1X",
"status": "not_addressed",
"reason": "Inter-round diff does not touch manager.rs; the discard pattern at end of rewind_to_height remains."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1g",
"status": "not_addressed",
"reason": "Inter-round diff does not touch dash/src/sml/masternode_list_engine/mod.rs; truncate_above still keyed by cycle-boundary hash."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1j",
"status": "addressed",
"reason": "Inter-round diff adds wait_for_full_sync call in tests_sync.rs:598 before triggering the reorg, ensuring the filter pipeline is settled and reducing the QRInfo race window."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1o",
"status": "not_addressed",
"reason": "Inter-round diff does not modify rewind_to_height; the two separate lock acquisitions remain."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1s",
"status": "not_addressed",
"reason": "Inter-round diff does not modify rewind_to_height error handling around send_qrinfo_for_tip."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1w",
"status": "not_addressed",
"reason": "Inter-round diff to helpers.rs only adds wait_for_full_sync; wait_for_chain_reorg_event signature still takes no expected_fork_height parameter."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq12",
"status": "not_addressed",
"reason": "Inter-round diff does not add a new test for the empty-engine-after-truncation case in manager.rs."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1-",
"status": "not_addressed",
"reason": "Current code window at sync_manager.rs:90-91 still shows direct field mutation (self.masternode_ready = false; self.pending_validation = None) rather than a reset_for_reorg() method call."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": [
{
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"fingerprint": "dash/src/sml/masternode_list_engine/mod.rs:1475:1475:rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above",
"round": 1,
"line": 1475,
"severity": "warning"
},
{
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"fingerprint": "dash-spv/tests/dashd_masternode/tests_sync.rs:610:610:Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg",
"round": 1,
"line": 610,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/masternodes/manager.rs",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"fingerprint": "dash-spv/src/sync/masternodes/manager.rs:609:609:Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind",
"round": 1,
"line": 609,
"severity": "warning"
}
]
},
"openThreadsState": "fetched",
"openThreadCount": 8,
"resolvedThreadIdCount": 0,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 3256,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"testNitSuppressedCount": 2,
"durationMs": 0
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 0,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 0
},
"entries": []
},
"usage": {
"inputTokens": 29,
"outputTokens": 15394,
"totalTokens": 15423,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2785,
"totalTokens": 2794
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 11325,
"totalTokens": 11340
},
"judge": {
"inputTokens": 5,
"outputTokens": 1284,
"totalTokens": 1289
},
"dedup": {
"inputTokens": 0,
"outputTokens": 0,
"totalTokens": 0
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 1,
"reclassifiedPriorCount": 0
}
}Reviewed commit 0f921a2
…_to_height` Addresses manki-review comments on PR #165 #165 (comment) #165 (comment) #165 (comment)
…eorg()` Addresses manki-review comment on PR #165 #165 (comment)
Addresses manki-review comment on PR #165 #165 (comment)
|
Manki — Review complete Planner (25s) Review — 7 findings Judge — 3 kept · 0 dropped (56s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Seven prior threads resolved cleanly, but the encapsulation refactor introduced a regression: routing on_disconnect through reset_for_reorg() now nukes pending_validation on every transient disconnect — the exact contract that field's doc-comment promises to uphold. All 5 reviewers caught this independently.
📊 3 findings (1 warning, 2 suggestion) · 757 lines · 254s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "dd1dbb4c6dd279f7e7a098783ed380d3095cf377",
"round": 3,
"timestamp": "2026-05-25T15:10:23.426Z",
"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": 757,
"additions": 741,
"deletions": 16,
"filesReviewed": 10,
"fileTypes": {
".rs": 10
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 45,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 161,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 5,
"reviewerEffort": "medium",
"judgeEffort": "high",
"prType": "feat",
"durationMs": 24550,
"language": "rust",
"context": "Dash cryptocurrency protocol library (SPV client, masternode/quorum sync)",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 130598,
"status": "success",
"responseLength": 1826,
"inputTokens": 3,
"outputTokens": 7205,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 162480,
"status": "success",
"responseLength": 3018,
"inputTokens": 3,
"outputTokens": 9083,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 66349,
"status": "success",
"responseLength": 3279,
"inputTokens": 3,
"outputTokens": 3649,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 87834,
"status": "success",
"responseLength": 1594,
"inputTokens": 3,
"outputTokens": 4481,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 71854,
"status": "success",
"responseLength": 1554,
"inputTokens": 3,
"outputTokens": 3699,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Seven prior threads resolved cleanly, but the encapsulation refactor introduced a regression: routing \u0060on_disconnect\u0060 through \u0060reset_for_reorg()\u0060 now nukes \u0060pending_validation\u0060 on every transient disconnect — the exact contract that field's doc-comment promises to uphold. All 5 reviewers caught this independently.",
"confidenceDistribution": {
"high": 2,
"medium": 1,
"low": 0
},
"severityChanges": 3,
"mergedDuplicates": 4,
"durationMs": 56052,
"retryCount": 0,
"verdictReason": "novel_suggestion",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1X",
"status": "addressed",
"reason": "Diff at manager.rs:601-605 now returns \u0060result\u0060 directly instead of discarding via \u0060?; Ok(vec![])\u0060 — any events from send_qrinfo_for_tip propagate up."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1g",
"status": "not_addressed",
"reason": "The inter-round diff does not touch dash/src/sml/masternode_list_engine/mod.rs; the orphaned-quorum retention concern in \u0060truncate_above\u0060 is unchanged."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1o",
"status": "addressed",
"reason": "Diff collapses the two-lock acquisition into one write-lock block (lines 573-578) that performs truncate, reads tip, and writes last_synced_block_hash atomically; the separate read-lock block is removed."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1s",
"status": "addressed",
"reason": "Diff at manager.rs:601-604 now transitions to \u0060SyncState::WaitForEvents\u0060 when \u0060send_qrinfo_for_tip\u0060 returns Err, so the manager is no longer stranded in Syncing."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1w",
"status": "addressed",
"reason": "Helper signature now accepts \u0060expected_fork_height: Option<u32>\u0060 and skips non-matching ChainReorg events; doc-comment updated. Callers in tests_sync.rs pass \u0060Some(fork_height)\u0060 where filtering matters."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq12",
"status": "addressed",
"reason": "Diff adds \u0060test_rewind_to_height_empty_engine_after_truncation\u0060 (manager.rs:1108-1138) asserting empty engine, None last_synced_block_hash, height 0 progress, and queued GetQRInfo."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ekq1-",
"status": "addressed",
"reason": "\u0060masternode_ready\u0060 and \u0060pending_validation\u0060 are back to private in manager.rs; new \u0060reset_for_reorg()\u0060 method encapsulates the transition; sync_manager.rs line 90 now calls \u0060self.reset_for_reorg()\u0060 instead of mutating fields directly."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [
{
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"title": "on_disconnect() now silently drops pending_validation via reset_for_reorg()",
"fingerprint": "dash-spv/src/sync/chainlock/sync_manager.rs:30:30:on-disconnect---now-silently-drops-pending-validation-via-reset-for-reorg--"
}
],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 7,
"resolvedThreadIdCount": 1,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 10606,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 4,
"durationMs": 13378
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 1,
"suggestion": 2,
"nitpick": 0
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"lineStart": 30,
"lineEnd": 30,
"slug": "on-disconnect---now-silently-drops-pending-validation-via-reset-for-reorg--"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "// In manager.rs, add a second reset method:\npub(super) fn reset_for_disconnect(&mut self) {\n self.masternode_ready = false;\n // pending_validation is intentionally retained: the chainlock\n // that arrived before masternode_ready should be re-validated\n // on the next reconnect cycle via",
"title": "on_disconnect() now silently drops pending_validation via reset_for_reorg()",
"judgeNotes": "Merged findings 1, 2, 4, 6, 7 — all five reviewers independently flagged the same issue. The previous behavior preserved \u0060pending_validation\u0060 across disconnects so a chainlock that arrived before masternode_ready could be re-validated after reconnect (the field's doc-comment explicitly states this contract). The refactor changes that contract silently. Impact: Medium (delayed best-chainlock observation; network typically re-announces via INV so not catastrophic), Likelihood: Possible (requires C",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 605,
"lineEnd": 605,
"slug": "-rewind-to-height--propagates--Err--after-already-transitioning-state-to--WaitForEvents-"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "let result = self.send_qrinfo_for_tip(requests).await;\nif result.is_err() {\n tracing::warn!(\"send_qrinfo_for_tip failed during rewind, awaiting reconnect\");\n self.set_state(SyncState::WaitForEvents);\n // Return Ok so the framework does not treat this as a fatal error;\n // the tick handle",
"title": "\u0060rewind_to_height\u0060 propagates \u0060Err\u0060 after already transitioning state to \u0060WaitForEvents\u0060",
"judgeNotes": "Design question about whether returning Err after recovery-state-set could be re-overwritten by the framework. Without visibility into the coordinator's Err handling, this is speculative. Single reviewer, suggestion-level. Impact: Low-Medium (depends on framework behavior), Likelihood: Unknown → suggestion.",
"judgeConfidence": "medium",
"reachability": "unknown"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 598,
"lineEnd": 598,
"slug": "No-test-for--send-qrinfo-for-tip--failure-path-inside--rewind-to-height-"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "#[tokio::test]\nasync fn test_rewind_to_height_sets_wait_for_events_on_send_failure() {\n // Use a closed/dropped request channel to force send_qrinfo_for_tip to error.\n let (mut manager, requests, _rx) = make_synced_incremental_manager(120).await;\n drop(_rx); // close the receiver so the sen",
"title": "No test for \u0060send_qrinfo_for_tip\u0060 failure path inside \u0060rewind_to_height\u0060",
"judgeNotes": "The new recovery path (set WaitForEvents on error) is only covered by the success-case test. A failure-path test would assert the recovery actually fires. Reasonable suggestion but not blocking. Impact: Low (test coverage), Likelihood: N/A → suggestion.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 39,
"outputTokens": 34968,
"totalTokens": 35007,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2463,
"totalTokens": 2472
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 28117,
"totalTokens": 28132
},
"judge": {
"inputTokens": 5,
"outputTokens": 3057,
"totalTokens": 3062
},
"dedup": {
"inputTokens": 10,
"outputTokens": 1331,
"totalTokens": 1341
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 2,
"reclassifiedPriorCount": 0
}
}Reviewed commit dd1dbb4
|
Review skipped for
|
…sconnect Adds `reset_for_disconnect()` that clears only `masternode_ready`, and switches `on_disconnect` to use it instead of `reset_for_reorg()`. A chainlock cached before masternode sync completes remains valid on the same chain and must survive disconnect so `on_masternode_ready` can re-validate it on the next reconnect. Addresses manki-review comment on PR #165 #165 (comment)
Adds `test_rewind_to_height_sets_wait_for_events_on_send_failure` to assert that when `send_qrinfo_for_tip` errors (dropped channel), the manager transitions to `WaitForEvents` and clears `qrinfo_in_flight`, leaving it recoverable by the next `BlockHeaderSyncComplete` event. Addresses manki-review comment on PR #165 #165 (comment)
|
Manki — Review complete Planner (18s) Review — 3 findings Judge — 3 kept · 0 dropped (27s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Previous round's three open threads all resolved: reset_for_disconnect preserves pending_validation, and a new test covers the send_qrinfo_for_tip failure path. New findings are all minor suggestions about docs and additional test coverage.
Blocking unresolved review threads:
- [Warning] rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above —
dash/src/sml/masternode_list_engine/mod.rs:1475 - [Warning] Integration test races against QRInfo response when asserting engine state after ChainReorg —
dash-spv/tests/dashd_masternode/tests_sync.rs:610 - [Warning] Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind —
dash-spv/src/sync/masternodes/manager.rs:609 - [Warning] on_disconnect() now silently drops pending_validation via reset_for_reorg() —
dash-spv/src/sync/chainlock/sync_manager.rs:30
📊 3 findings (2 suggestion, 1 nitpick) · 786 lines · 185s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "c872f44c0160106738ab3ddc95597a42f14f326b",
"round": 4,
"timestamp": "2026-05-25T15:23:40.829Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 3,
"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": 786,
"additions": 770,
"deletions": 16,
"filesReviewed": 10,
"fileTypes": {
".rs": 10
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 53,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 182,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": "medium",
"prType": "feat",
"durationMs": 17868,
"language": "rust",
"context": "Dash blockchain SPV client with chainlock/masternode consensus",
"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",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 124184,
"status": "success",
"responseLength": 1636,
"inputTokens": 3,
"outputTokens": 6766,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 129224,
"status": "success",
"responseLength": 1071,
"inputTokens": 3,
"outputTokens": 7346,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 54959,
"status": "success",
"responseLength": 2088,
"inputTokens": 3,
"outputTokens": 2699,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 49022,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 2326,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 109105,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 6121,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Previous round's three open threads all resolved: \u0060reset_for_disconnect\u0060 preserves \u0060pending_validation\u0060, and a new test covers the \u0060send_qrinfo_for_tip\u0060 failure path. New findings are all minor suggestions about docs and additional test coverage.",
"confidenceDistribution": {
"high": 2,
"medium": 1,
"low": 0
},
"severityChanges": 3,
"mergedDuplicates": 0,
"durationMs": 27324,
"retryCount": 0,
"verdictReason": "prior_unaddressed",
"defensiveHardeningCount": 1,
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6Ek4to",
"status": "addressed",
"reason": "Inter-round diff adds reset_for_disconnect() in manager.rs (only clears masternode_ready, preserves pending_validation) and sync_manager.rs:31 now calls it instead of reset_for_reorg()."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ek4t3",
"status": "not_addressed",
"reason": "Inter-round diff does not modify the rewind_to_height error-return ordering; the Err is still propagated after WaitForEvents transition."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ek4uB",
"status": "addressed",
"reason": "Inter-round diff adds test_rewind_to_height_sets_wait_for_events_on_send_failure which drops the rx channel, asserts the error, WaitForEvents state, and cleared qrinfo_in_flight."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": [
{
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"fingerprint": "dash/src/sml/masternode_list_engine/mod.rs:1475:1475:rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above",
"round": 1,
"line": 1475,
"severity": "warning"
},
{
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"fingerprint": "dash-spv/tests/dashd_masternode/tests_sync.rs:610:610:Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg",
"round": 1,
"line": 610,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/masternodes/manager.rs",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"fingerprint": "dash-spv/src/sync/masternodes/manager.rs:609:609:Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind",
"round": 1,
"line": 609,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"title": "on_disconnect() now silently drops pending_validation via reset_for_reorg()",
"fingerprint": "dash-spv/src/sync/chainlock/sync_manager.rs:30:30:on-disconnect---now-silently-drops-pending-validation-via-reset-for-reorg--",
"round": 3,
"line": 30,
"severity": "warning"
}
]
},
"openThreadsState": "fetched",
"openThreadCount": 3,
"resolvedThreadIdCount": 8,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 2865,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"durationMs": 12572
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 2,
"nitpick": 1
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 139,
"lineEnd": 139,
"slug": "reset-for-disconnect-safety-depends-on-undocumented-phase-ordering-invariant"
},
"severity": "nitpick",
"specialist": "Security & Safety",
"suggestedFix": "/// Reset state for a peer disconnect. \u0060pending_validation\u0060 is intentionally\n/// kept: a chainlock that arrived before \u0060masternode_ready\u0060 remains valid on\n/// the same chain and must be re-evaluated when \u0060on_masternode_ready\u0060 fires\n/// on the next reconnect.\n///\n/// Safety invariant: correctness her",
"title": "reset_for_disconnect safety depends on undocumented phase-ordering invariant",
"judgeNotes": "Impact: Low (documentation of invariant), Likelihood: Unlikely (would require parallelizing sync phases) → suggestion. Useful future-proofing note but not behavioral.",
"judgeConfidence": "medium",
"reachability": "hypothetical",
"reachabilityReasoning": "Current code serializes sync phases so the failure mode cannot manifest today.",
"tags": [
"defensive-hardening"
],
"originalSeverity": "suggestion"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 1185,
"lineEnd": 1185,
"slug": "Error-path-test-omits-assertion-that-engine-was-truncated-before-the-failure"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "// After the \u0060result.is_err()\u0060 assertions, add:\nlet engine = manager.engine.read().await;\nassert!(\n !engine.masternode_lists.contains_key(&120),\n \"engine must be truncated above fork_height=50 even when send fails\"\n);",
"title": "Error-path test omits assertion that engine was truncated before the failure",
"judgeNotes": "Impact: Medium (regression in truncation safety would go undetected), Likelihood: Possible → suggestion. Reasonable test hardening but only one reviewer flagged.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"lineStart": 30,
"lineEnd": 30,
"slug": "No-test-for--reset-for-disconnect--behavioral-contract--pending-validation-survives-disconnect-"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "// In a chainlock manager unit test module:\n#[tokio::test]\nasync fn test_pending_validation_survives_disconnect_and_reused_on_reconnect() {\n let mut manager = make_chainlock_manager_with_pending_validation(/* some chainlock */);\n // Precondition: pending_validation is set, masternode_ready is ",
"title": "No test for \u0060reset_for_disconnect\u0060 behavioral contract (pending_validation survives disconnect)",
"judgeNotes": "Impact: Medium (silently dropping pending_validation would be a real bug), Likelihood: Possible → suggestion. Test coverage gap worth noting but not blocking.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 39,
"outputTokens": 29091,
"totalTokens": 29130,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1522,
"totalTokens": 1531
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 25258,
"totalTokens": 25273
},
"judge": {
"inputTokens": 5,
"outputTokens": 947,
"totalTokens": 952
},
"dedup": {
"inputTokens": 10,
"outputTokens": 1364,
"totalTokens": 1374
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 3,
"reclassifiedPriorCount": 0
}
}Reviewed commit c872f44
|
Review skipped for
|
…reorg announcements dashd announces reorgs as N separate `headers2` messages (one header each, because each `generatetoaddress` block produces a `headers` push). The first batch enters the fork buffer via `ingest_fork`. Subsequent batches whose `prev_blockhash` is the previous fork tip re-entered `ingest_fork`, which then failed chain-continuity against the *active-chain* ancestor and was swallowed with a "deferred to Phase 3" debug log. The branch was stuck at one header, never outweighed the active extension, and `ChainReorg` never fired. Add `ForkBuffer::extend_branch` that looks up the existing branch by `(peer, prev_tip_hash)`, validates each continuation header (continuity off the buffered branch tip, PoW, MTP, DGW v3 against a rolling window of `history + buffered + new`), appends them, and re-keys the branch under the new tip. The caller receives a `BranchUpdate` with the new tip/height/work so it can refresh `fork_tip_index` and re-evaluate promotion. The per-header validation loop is factored out of `ingest` into `validate_chain` so both paths share identical rules. `BlockHeadersManager` routes `fork_tip_index` hits through a new `extend_fork` helper that loads the same active-chain history as `ingest_fork`, calls `extend_branch`, refreshes the index, and runs `take_winning_candidate` so a branch that just outweighs the active chain promotes immediately. Verified locally with `test_masternode_list_rewind_across_dip3_reorg` (previously timed out, now completes in ~2s). Refs [#142](#142), [#165](#165).
|
Manki — Review complete Planner (24s) Review — 10 findings Judge — 5 kept · 5 dropped (42s) Review metadataConfig:
Judge decisions:
Timing:
|
…for_disconnect`, add behavioral tests Addresses manki-review comments on PR #165: - #165 (comment) - #165 (comment) - #165 (comment)
There was a problem hiding this comment.
New fork-buffer extension path introduces two real concerns flagged by multiple reviewers: TTL reset on every extend_branch lets a slow-dripping peer keep a branch alive indefinitely, and ForkChainBreak now propagates where it was previously swallowed, risking honest-peer penalties after TTL eviction. Previously open threads remain unaddressed.
📊 5 findings (2 warning, 1 suggestion, 2 nitpick) · 1263 lines · 399s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "39dba5b917fca8409f16b8e73ad194d2e28235ec",
"round": 5,
"timestamp": "2026-05-25T15:41:58.658Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 4,
"maxAutoRounds": 5,
"skipCap": false,
"forceReview": false,
"bypassReason": "within_cap"
},
"trigger": {
"event": "pull_request:synchronize",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1263,
"additions": 1195,
"deletions": 68,
"filesReviewed": 12,
"fileTypes": {
".rs": 12
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 254,
"deletions": 17,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 172,
"deletions": 35,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 53,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 181,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 23900,
"language": "rust",
"context": "Dash blockchain 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",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 198740,
"status": "success",
"responseLength": 2869,
"inputTokens": 3,
"outputTokens": 11806,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 2,
"durationMs": 324038,
"status": "success",
"responseLength": 4162,
"inputTokens": 3,
"outputTokens": 21136,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 175617,
"status": "success",
"responseLength": 2389,
"inputTokens": 3,
"outputTokens": 10562,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 2,
"findingsKept": 0,
"durationMs": 196171,
"status": "success",
"responseLength": 3064,
"inputTokens": 3,
"outputTokens": 10902,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 2,
"findingsKept": 0,
"durationMs": 90845,
"status": "success",
"responseLength": 2888,
"inputTokens": 3,
"outputTokens": 5210,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "New fork-buffer extension path introduces two real concerns flagged by multiple reviewers: TTL reset on every extend_branch lets a slow-dripping peer keep a branch alive indefinitely, and ForkChainBreak now propagates where it was previously swallowed, risking honest-peer penalties after TTL eviction. Previously open threads remain unaddressed.",
"confidenceDistribution": {
"high": 8,
"medium": 2,
"low": 0
},
"severityChanges": 10,
"mergedDuplicates": 0,
"durationMs": 41567,
"retryCount": 0,
"verdictReason": "novel_suggestion",
"defensiveHardeningCount": 1,
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6ElClN",
"status": "uncertain",
"reason": "Inter-round diff touches chainlock/manager.rs (+89/-42) but the specific reset_for_disconnect doc-comment region was not shown; current code window unavailable. Cannot confirm the undocumented phase-ordering invariant was documented."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElClY",
"status": "uncertain",
"reason": "Inter-round diff does not include changes to masternodes/manager.rs around line 1184; current code window unavailable for the test in question. No evidence the truncation assertion was added."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElClZ",
"status": "not_addressed",
"reason": "Current code at sync_manager.rs:30 still shows on_disconnect calling reset_for_disconnect with no accompanying test. Inter-round diff does not add a test for the pending_validation-survives-disconnect contract."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [
{
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"title": "\u0060extend_branch\u0060 resets TTL clock, enabling slow memory pressure from malicious peers",
"fingerprint": "dash-spv/src/sync/block_headers/fork_buffer.rs:248:248:-extend-branch--resets-TTL-clock--enabling-slow-memory-pressure-from-malicious-peers"
},
{
"file": "dash-spv/src/sync/block_headers/manager.rs",
"title": "Stale \u0060fork_tip_index\u0060 entry surfaces \u0060ForkChainBreak\u0060 to caller after TTL eviction, risking honest-peer disconnect",
"fingerprint": "dash-spv/src/sync/block_headers/manager.rs:488:488:Stale--fork-tip-index--entry-surfaces--ForkChainBreak--to-caller-after-TTL-eviction--risking-honest-peer-disconnect"
}
],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 3,
"resolvedThreadIdCount": 11,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 28333,
"interRoundDiffTruncated": true,
"threadResolutionOverrides": {
"addressedDropped": 0,
"notAddressedOverridden": 0,
"uncertainCount": 2
}
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"durationMs": 13009
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 5,
"severityCounts": {
"blocker": 0,
"warning": 2,
"suggestion": 1,
"nitpick": 2
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"lineStart": 248,
"lineEnd": 248,
"slug": "-extend-branch--resets-TTL-clock--enabling-slow-memory-pressure-from-malicious-peers"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "arrived_at: branch.arrived_at, // preserve original ingestion time so TTL still fires",
"title": "\u0060extend_branch\u0060 resets TTL clock, enabling slow memory pressure from malicious peers",
"judgeNotes": "Impact: Medium (bounded by MAX_FORK_HEADERS_PER_PEER but defeats TTL guarantee), Likelihood: Possible (adversarial peer) → warning. Two reviewers independently flagged this; fix is trivial (preserve branch.arrived_at).",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/manager.rs",
"lineStart": 488,
"lineEnd": 488,
"slug": "Stale--fork-tip-index--entry-surfaces--ForkChainBreak--to-caller-after-TTL-eviction--risking-honest-peer-disconnect"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "match self.extend_fork(peer, prev_tip, ancestor_height, headers).await {\n Ok(()) => {}\n Err(SyncError::ForkChainBreak(msg)) => {\n // Branch was TTL-evicted but index entry outlived it; prune and ignore.\n tracing::debug!(\"fork continuation branch missing, pruning stale index: {}\",",
"title": "Stale \u0060fork_tip_index\u0060 entry surfaces \u0060ForkChainBreak\u0060 to caller after TTL eviction, risking honest-peer disconnect",
"judgeNotes": "Impact: High (honest peer penalty/disconnect if caller treats Err as misbehavior), Likelihood: Possible (requires TTL eviction race) → warning. Four reviewers independently flagged this same propagation regression — strong consensus signal. Behavioral change is clear: old code explicitly swallowed ForkChainBreak with a comment, new code propagates via \u0060?\u0060.",
"judgeConfidence": "high",
"reachability": "unknown",
"reachabilityReasoning": "Depends on how upstream handle_headers_pipeline callers treat ForkChainBreak — not visible in this diff."
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/manager.rs",
"lineStart": 352,
"lineEnd": 352,
"slug": "extend-fork-loads-active-extension-before-calling-extend-branch--wasting-I-O-on-error-paths"
},
"severity": "nitpick",
"specialist": "Correctness & Logic",
"suggestedFix": "let update = self.fork_buffer.extend_branch(peer, prev_tip_hash, headers, &history)?;\n\n// Only load active_extension when extend_branch succeeded.\nlet active_extension = storage.load_headers(ancestor_height + 1..tip_height + 1).await?;\ndrop(storage);\n\n// ... rest of candidate promotion logic ...",
"title": "extend_fork loads active_extension before calling extend_branch, wasting I/O on error paths",
"judgeNotes": "Impact: Low (extra disk I/O only on error paths), Likelihood: Possible. Pure micro-optimization with no correctness implication. At noise_level: low this is a nitpick and should be dropped.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/manager.rs",
"lineStart": 336,
"lineEnd": 336,
"slug": "extend-fork-has-a-stricter-history-length-check-than-ingest-fork--causing-asymmetric-branch-lifecycle"
},
"severity": "nitpick",
"specialist": "Correctness & Logic",
"title": "extend_fork has a stricter history-length check than ingest_fork, causing asymmetric branch lifecycle",
"judgeNotes": "Impact: Medium (asymmetric behavior near genesis or post-truncation could silently break multi-message reorg accumulation), Likelihood: Unlikely (requires specific edge case). Single reviewer; legitimate consistency concern but edge-case-bound.",
"judgeConfidence": "medium",
"reachability": "hypothetical",
"reachabilityReasoning": "Requires storage gap below ancestor or near-genesis conditions not exercised by typical sync paths.",
"tags": [
"defensive-hardening"
],
"originalSeverity": "suggestion"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"lineStart": 828,
"lineEnd": 828,
"slug": "Cap-exceeded-path-in--extend-branch--loses-test-coverage-after-refactor"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "#[test]\nfn extend_branch_cap_exceeded_preserves_branch() {\n let peer: SocketAddr = \"1.2.3.4:9999\".parse().unwrap();\n let mut buf = ForkBuffer::new(regtest_params());\n let active = build_chain(1_700_000_000, 11, BlockHash::all_zeros());\n let ancestor_height = (active.len() as u32) - 1;\n ",
"title": "Cap-exceeded path in \u0060extend_branch\u0060 loses test coverage after refactor",
"judgeNotes": "Impact: Medium (branch-preservation invariant unverified), Likelihood: Unlikely (refactor regression). Legitimate test gap — the cap path has a non-trivial remove-then-reinsert pattern worth covering.",
"judgeConfidence": "medium",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 39,
"outputTokens": 64533,
"totalTokens": 64572,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1974,
"totalTokens": 1983
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 59616,
"totalTokens": 59631
},
"judge": {
"inputTokens": 5,
"outputTokens": 1812,
"totalTokens": 1817
},
"dedup": {
"inputTokens": 10,
"outputTokens": 1131,
"totalTokens": 1141
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 4,
"reclassifiedPriorCount": 0
}
}Reviewed commit 39dba5b
|
Manki has completed 5/5 review rounds on this PR. Automatic review is paused. Tick the box to force another round, or comment
|
|
Manki — Review complete Planner (21s) Review — 6 findings Judge — 3 kept · 0 dropped (46s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Previous nitpick on the phase-ordering invariant got a doc comment, and the missing engine-truncation and pending_validation tests both landed. New round just surfaces test-strengthening nits — none blocking.
Blocking unresolved review threads:
- [Warning] rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above —
dash/src/sml/masternode_list_engine/mod.rs:1475 - [Warning] Integration test races against QRInfo response when asserting engine state after ChainReorg —
dash-spv/tests/dashd_masternode/tests_sync.rs:610 - [Warning] Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind —
dash-spv/src/sync/masternodes/manager.rs:609 - [Warning] on_disconnect() now silently drops pending_validation via reset_for_reorg() —
dash-spv/src/sync/chainlock/sync_manager.rs:30 - [Warning]
extend_branchresets TTL clock, enabling slow memory pressure from malicious peers —dash-spv/src/sync/block_headers/fork_buffer.rs:248 - [Warning] Stale
fork_tip_indexentry surfacesForkChainBreakto caller after TTL eviction, risking honest-peer disconnect —dash-spv/src/sync/block_headers/manager.rs:488
📊 3 findings (2 suggestion, 1 nitpick) · 1303 lines · 215s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "2c8e2c7286b28b5bd578a9375349c0102d98212c",
"round": 6,
"timestamp": "2026-05-25T15:54:48.745Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 5,
"maxAutoRounds": 5,
"skipCap": true,
"forceReview": false,
"bypassReason": "skip_cap"
},
"trigger": {
"event": "issue_comment:edited:tick:FORCE_CAP_MARKER",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1303,
"additions": 1235,
"deletions": 68,
"filesReviewed": 12,
"fileTypes": {
".rs": 12
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 254,
"deletions": 17,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 172,
"deletions": 35,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 86,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 188,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 5,
"reviewerEffort": "medium",
"judgeEffort": "high",
"prType": "feat",
"durationMs": 21089,
"language": "rust",
"context": "Dash SPV client reorg handling for masternodes and quorums",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 56336,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 2526,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 120687,
"status": "success",
"responseLength": 1051,
"inputTokens": 4,
"outputTokens": 6982,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 33932,
"status": "success",
"responseLength": 2159,
"inputTokens": 3,
"outputTokens": 1659,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 136671,
"status": "success",
"responseLength": 2007,
"inputTokens": 4,
"outputTokens": 5950,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 101179,
"status": "success",
"responseLength": 2416,
"inputTokens": 4,
"outputTokens": 4637,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Previous nitpick on the phase-ordering invariant got a doc comment, and the missing engine-truncation and pending_validation tests both landed. New round just surfaces test-strengthening nits — none blocking.",
"confidenceDistribution": {
"high": 2,
"medium": 1,
"low": 0
},
"severityChanges": 3,
"mergedDuplicates": 3,
"durationMs": 46497,
"retryCount": 0,
"verdictReason": "prior_unaddressed",
"ownProposalDemotedCount": 1,
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6ElClN",
"status": "addressed",
"reason": "The inter-round diff adds a Safety-invariant doc block at manager.rs:139-143 explicitly calling out the header-sync-before-MN-sync phase ordering and stating a ChainReorg check would be required if phases parallelize."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElClY",
"status": "addressed",
"reason": "The inter-round diff appends an assertion block at manager.rs:1185-1191 that reads the engine and asserts !engine.masternode_lists.contains_key(&120) with the truncation-above-fork_height message."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElClZ",
"status": "addressed",
"reason": "The inter-round diff adds test_pending_validation_survives_disconnect_and_consumed_on_reconnect (chainlock/manager.rs:697-721) which exercises process_chainlock → reset_for_disconnect → on_masternode_ready and asserts pending_validation survives the disconnect and is consumed on reconnect."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": [
{
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"fingerprint": "dash/src/sml/masternode_list_engine/mod.rs:1475:1475:rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above",
"round": 1,
"line": 1475,
"severity": "warning"
},
{
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"fingerprint": "dash-spv/tests/dashd_masternode/tests_sync.rs:610:610:Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg",
"round": 1,
"line": 610,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/masternodes/manager.rs",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"fingerprint": "dash-spv/src/sync/masternodes/manager.rs:609:609:Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind",
"round": 1,
"line": 609,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"title": "on_disconnect() now silently drops pending_validation via reset_for_reorg()",
"fingerprint": "dash-spv/src/sync/chainlock/sync_manager.rs:30:30:on-disconnect---now-silently-drops-pending-validation-via-reset-for-reorg--",
"round": 3,
"line": 30,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"title": "\u0060extend_branch\u0060 resets TTL clock, enabling slow memory pressure from malicious peers",
"fingerprint": "dash-spv/src/sync/block_headers/fork_buffer.rs:248:248:-extend-branch--resets-TTL-clock--enabling-slow-memory-pressure-from-malicious-peers",
"round": 5,
"line": 248,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/block_headers/manager.rs",
"title": "Stale \u0060fork_tip_index\u0060 entry surfaces \u0060ForkChainBreak\u0060 to caller after TTL eviction, risking honest-peer disconnect",
"fingerprint": "dash-spv/src/sync/block_headers/manager.rs:488:488:Stale--fork-tip-index--entry-surfaces--ForkChainBreak--to-caller-after-TTL-eviction--risking-honest-peer-disconnect",
"round": 5,
"line": 488,
"severity": "warning"
}
]
},
"openThreadsState": "fetched",
"openThreadCount": 3,
"resolvedThreadIdCount": 16,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 3054,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 3,
"durationMs": 29875
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 2,
"nitpick": 1
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 720,
"lineEnd": 720,
"slug": "New-test-discards--on-masternode-ready--return-value--leaving-re-broadcast-contract-untested"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "let returned = manager.on_masternode_ready().await;\nassert!(\n returned.is_some(),\n \"on_masternode_ready must return the pending chainlock for re-broadcast\"\n);\nassert!(manager.pending_validation.is_none(), ...);\nassert!(manager.masternode_ready);",
"title": "New test discards \u0060on_masternode_ready\u0060 return value, leaving re-broadcast contract untested",
"judgeNotes": "Findings 1, 2, and 5 are the same concern from three reviewers: the new test should assert the returned Option<ChainLock> is Some so a regression that drops the chainlock doesn't pass silently. Merged 1, 2, 5. Impact: Medium (test gap), Likelihood: Possible → suggestion.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 1187,
"lineEnd": 1187,
"slug": "Engine-truncation-assertion-only-checks-absence--not-the-surviving-boundary"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "{\n let engine = manager.engine.read().await;\n // Above fork_height must be gone\n assert!(\n !engine.masternode_lists.contains_key(&120),\n \"engine must be truncated above fork_height=50 even when send fails\"\n );\n // At or below fork_height must survive — verifies the bound",
"title": "Engine truncation assertion only checks absence, not the surviving boundary",
"judgeNotes": "Findings 3 and 6 describe the same gap: a single-key spot-check doesn't fully verify the truncate-above contract. Worth strengthening but the existing assertion catches the most likely regression. Merged 3, 6.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 144,
"lineEnd": 144,
"slug": "Safety-invariant-comment-has-no-companion-test-for-the-reorg-ordering-path"
},
"severity": "nitpick",
"specialist": "Architecture & Design",
"suggestedFix": "// Sketch — fill in with actual test helper shapes\n#[tokio::test]\nasync fn test_pending_validation_cleared_or_blocked_after_reorg() {\n let mut manager = create_test_manager().await;\n // Arrive a chainlock on the soon-to-be-orphaned chain\n let _ = manager.process_chainlock(&create_test_chain",
"title": "Safety invariant comment has no companion test for the reorg ordering path",
"judgeNotes": "The new doc comment documents an ordering invariant but no test exercises the reorg→on_masternode_ready interaction. A test would lock in the documented contract. Impact: Medium, Likelihood: Possible → suggestion.\nOwn-proposal follow-up: implements round 4 finding \"reset_for_disconnect safety depends on undocumented phase-ordering invariant\"",
"judgeConfidence": "medium",
"reachability": "reachable",
"tags": [
"own-proposal-followup"
],
"originalSeverity": "suggestion"
}
]
},
"usage": {
"inputTokens": 42,
"outputTokens": 28194,
"totalTokens": 28236,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1519,
"totalTokens": 1528
},
"reviewer": {
"inputTokens": 18,
"outputTokens": 21754,
"totalTokens": 21772
},
"judge": {
"inputTokens": 5,
"outputTokens": 1022,
"totalTokens": 1027
},
"dedup": {
"inputTokens": 10,
"outputTokens": 3899,
"totalTokens": 3909
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 5,
"reclassifiedPriorCount": 0
}
}Reviewed commit 2c8e2c7
…ki assertions - Assert return value and `progress.invalid()` in `test_pending_validation_survives_disconnect_and_consumed_on_reconnect` to cover the re-broadcast contract of `on_masternode_ready` - Add `engine.masternode_lists.is_empty()` boundary assertion in `test_rewind_to_height_sets_wait_for_events_on_send_failure` to verify surviving entries, not just absent ones - Add `test_reorg_prevents_stale_pending_validation_from_being_revalidated` as a companion test for the phase-ordering invariant documented on `reset_for_disconnect`
|
Manki has completed 6/5 review rounds on this PR. Automatic review is paused. Tick the box to force another round, or comment
|
|
Manki — Review complete Planner (41s) Review — 5 findings Judge — 3 kept · 2 dropped (51s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
New reorg test added for chainlock pending_validation and engine truncation strengthened with is_empty() — both prior threads materially addressed. Remaining nits are about test coverage breadth (boundary survival, ready-flag transition), not correctness.
Blocking unresolved review threads:
- [Warning] rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above —
dash/src/sml/masternode_list_engine/mod.rs:1475 - [Warning] Integration test races against QRInfo response when asserting engine state after ChainReorg —
dash-spv/tests/dashd_masternode/tests_sync.rs:610 - [Warning] Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind —
dash-spv/src/sync/masternodes/manager.rs:609 - [Warning] on_disconnect() now silently drops pending_validation via reset_for_reorg() —
dash-spv/src/sync/chainlock/sync_manager.rs:30 - [Warning]
extend_branchresets TTL clock, enabling slow memory pressure from malicious peers —dash-spv/src/sync/block_headers/fork_buffer.rs:248 - [Warning] Stale
fork_tip_indexentry surfacesForkChainBreakto caller after TTL eviction, risking honest-peer disconnect —dash-spv/src/sync/block_headers/manager.rs:488
📊 3 findings (1 suggestion, 2 nitpick) · 1350 lines · 171s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "e1b282a33679e1f3085d2407da57fc4c526b3465",
"round": 7,
"timestamp": "2026-05-25T16:17:08.991Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 6,
"maxAutoRounds": 5,
"skipCap": true,
"forceReview": false,
"bypassReason": "skip_cap"
},
"trigger": {
"event": "issue_comment:edited:tick:FORCE_CAP_MARKER",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1350,
"additions": 1282,
"deletions": 68,
"filesReviewed": 12,
"fileTypes": {
".rs": 12
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 254,
"deletions": 17,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 172,
"deletions": 35,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 129,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 192,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 5,
"reviewerEffort": "medium",
"judgeEffort": "medium",
"prType": "feat",
"durationMs": 41325,
"language": "rust",
"context": "Dash blockchain SPV synchronization",
"agents": [
{
"name": "Security & Safety",
"effort": "medium"
},
{
"name": "Correctness & Logic",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Architecture & Design",
"effort": "low"
},
{
"name": "Dependencies & Integration",
"effort": "low"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 43240,
"status": "success",
"responseLength": 1223,
"inputTokens": 3,
"outputTokens": 2250,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 68292,
"status": "success",
"responseLength": 1502,
"inputTokens": 3,
"outputTokens": 3921,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 47736,
"status": "success",
"responseLength": 1318,
"inputTokens": 3,
"outputTokens": 2704,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 11459,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 247,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "low"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 11779,
"status": "success",
"responseLength": 1261,
"inputTokens": 3,
"outputTokens": 316,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "low"
}
]
},
"judge": {
"summary": "New reorg test added for chainlock pending_validation and engine truncation strengthened with is_empty() — both prior threads materially addressed. Remaining nits are about test coverage breadth (boundary survival, ready-flag transition), not correctness.",
"confidenceDistribution": {
"high": 4,
"medium": 1,
"low": 0
},
"severityChanges": 5,
"mergedDuplicates": 0,
"durationMs": 50781,
"retryCount": 0,
"verdictReason": "prior_unaddressed",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6ElXwL",
"status": "addressed",
"reason": "Inter-round diff at chainlock/manager.rs:752-764 now captures returned, asserts is_none(), and asserts progress.invalid()==1, covering the consumption contract."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElXwN",
"status": "addressed",
"reason": "Inter-round diff at masternodes/manager.rs:1193-1196 adds engine.masternode_lists.is_empty() assertion, strengthening the truncation boundary check for the all-above-fork seeding."
},
{
"threadId": "PRRT_kwDOQSlaXs6ElXwR",
"status": "addressed",
"reason": "Inter-round diff adds test_reorg_prevents_stale_pending_validation_from_being_revalidated at chainlock/manager.rs:696-731 which exercises the reset_for_reorg → on_masternode_ready ordering invariant called out in the safety comment."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": [
{
"file": "dash/src/sml/masternode_list_engine/mod.rs",
"title": "rotated_quorums_per_cycle may retain orphaned quorum entries after truncate_above",
"fingerprint": "dash/src/sml/masternode_list_engine/mod.rs:1475:1475:rotated-quorums-per-cycle-may-retain-orphaned-quorum-entries-after-truncate-above",
"round": 1,
"line": 1475,
"severity": "warning"
},
{
"file": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"title": "Integration test races against QRInfo response when asserting engine state after ChainReorg",
"fingerprint": "dash-spv/tests/dashd_masternode/tests_sync.rs:610:610:Integration-test-races-against-QRInfo-response-when-asserting-engine-state-after-ChainReorg",
"round": 1,
"line": 610,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/masternodes/manager.rs",
"title": "Manager stuck in Syncing indefinitely if send_qrinfo_for_tip fails during rewind",
"fingerprint": "dash-spv/src/sync/masternodes/manager.rs:609:609:Manager-stuck-in-Syncing-indefinitely-if-send-qrinfo-for-tip-fails-during-rewind",
"round": 1,
"line": 609,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/sync_manager.rs",
"title": "on_disconnect() now silently drops pending_validation via reset_for_reorg()",
"fingerprint": "dash-spv/src/sync/chainlock/sync_manager.rs:30:30:on-disconnect---now-silently-drops-pending-validation-via-reset-for-reorg--",
"round": 3,
"line": 30,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"title": "\u0060extend_branch\u0060 resets TTL clock, enabling slow memory pressure from malicious peers",
"fingerprint": "dash-spv/src/sync/block_headers/fork_buffer.rs:248:248:-extend-branch--resets-TTL-clock--enabling-slow-memory-pressure-from-malicious-peers",
"round": 5,
"line": 248,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/block_headers/manager.rs",
"title": "Stale \u0060fork_tip_index\u0060 entry surfaces \u0060ForkChainBreak\u0060 to caller after TTL eviction, risking honest-peer disconnect",
"fingerprint": "dash-spv/src/sync/block_headers/manager.rs:488:488:Stale--fork-tip-index--entry-surfaces--ForkChainBreak--to-caller-after-TTL-eviction--risking-honest-peer-disconnect",
"round": 5,
"line": 488,
"severity": "warning"
}
]
},
"openThreadsState": "fetched",
"openThreadCount": 3,
"resolvedThreadIdCount": 19,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 3520,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"durationMs": 33633
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 1,
"nitpick": 2
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 722,
"lineEnd": 722,
"slug": "Test-does-not-assert-masternode-ready-transitions-back-to-true-after-reorg---on-masternode-ready"
},
"severity": "nitpick",
"specialist": "Security & Safety",
"suggestedFix": "assert!(\n manager.masternode_ready,\n \"on_masternode_ready must set masternode_ready=true even when pending_validation is None (cleared by reorg)\"\n);",
"title": "Test does not assert masternode_ready transitions back to true after reorg + on_masternode_ready",
"judgeNotes": "Single reviewer; concerns a test coverage gap rather than a code defect. Impact: Low (test thoroughness), Likelihood: Possible. At noise_level low this is below the bar to surface as warning.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/masternodes/manager.rs",
"lineStart": 1190,
"lineEnd": 1190,
"slug": "Engine-truncation-boundary-assertion-still-only-checks-absence--not-survival"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "// Seed one entry at and below the fork height, plus the orphaned ones\nengine.masternode_lists.insert(50, make_list()); // at boundary — must survive\nengine.masternode_lists.insert(30, make_list()); // below boundary — must survive\nengine.masternode_lists.insert(60, make_list()); // above — must be ",
"title": "Engine truncation boundary assertion still only checks absence, not survival",
"judgeNotes": "Two reviewers flagged this independently — moderate signal. Valid coverage gap: an off-by-one in truncate_above wouldn't be caught. Keep as suggestion per reviewer consensus.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 757,
"lineEnd": 757,
"slug": "New-test-discards--on-masternode-ready--return-value--leaving-re-broadcast-contract-untested"
},
"severity": "nitpick",
"specialist": "Dependencies & Integration",
"suggestedFix": "// After assert!(returned.is_none());\nassert_eq!(manager.progress.invalid(), 1, \"failed re-validation must be counted as invalid\");",
"title": "New test discards \u0060on_masternode_ready\u0060 return value, leaving re-broadcast contract untested",
"judgeNotes": "Original thread is now partially addressed — the disconnect test does capture returned and assert is_none plus invalid()==1. The remaining ask (full symmetry) is cosmetic.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 39,
"outputTokens": 18847,
"totalTokens": 18886,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 4412,
"totalTokens": 4421
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 9438,
"totalTokens": 9453
},
"judge": {
"inputTokens": 5,
"outputTokens": 976,
"totalTokens": 981
},
"dedup": {
"inputTokens": 10,
"outputTokens": 4021,
"totalTokens": 4031
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 6,
"reclassifiedPriorCount": 0
}
}Reviewed commit e1b282a
…assertions Assert `masternode_ready == true` after `on_masternode_ready()` in the reorg test (the flag flips unconditionally, even when `pending_validation` is None). Seed a surviving entry at height 30 in `test_rewind_to_height_sets_wait_for_events_on_send_failure` and assert `contains_key(&30)` after truncation at fork_height=50, closing the absence-only gap manki flagged. The disconnect-test `progress.invalid()` assertion (thread 3) was already applied in round 4. Addresses manki-review comments on PR #165 #165 (comment) #165 (comment) #165 (comment)
|
Manki has completed 7/5 review rounds on this PR. Automatic review is paused. Tick the box to force another round, or comment
|
|
Auto-approve withheld for To approve, request a fresh review by commenting |
|
Manki — Review complete Planner (32s) Review — 1 findings Judge — 0 kept · 0 dropped (13s) Review metadataConfig:
Timing:
|
There was a problem hiding this comment.
No findings to evaluate this round.
📊 0 findings (none) · 1355 lines · 95s
Manki context
{
"meta": {
"prNumber": 165,
"commitSha": "b53aa8da5b360bbebf7abb609a646b09b11f3c7f",
"round": 8,
"timestamp": "2026-05-25T16:35:26.118Z",
"mankiVersion": "5.3.0",
"cap": {
"priorRoundCount": 7,
"maxAutoRounds": 5,
"skipCap": true,
"forceReview": false,
"bypassReason": "skip_cap"
},
"trigger": {
"event": "issue_comment:edited:tick:FORCE_CAP_MARKER",
"sender": "xdustinface"
}
},
"config": {
"reviewLevel": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1355,
"additions": 1287,
"deletions": 68,
"filesReviewed": 12,
"fileTypes": {
".rs": 12
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 254,
"deletions": 17,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 172,
"deletions": 35,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 133,
"deletions": 6,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/manager.rs",
"additions": 30,
"deletions": 5,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/instantsend/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/manager.rs",
"additions": 193,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/masternodes/sync_manager.rs",
"additions": 14,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/test_utils/masternode_network.rs",
"additions": 46,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/helpers.rs",
"additions": 80,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 177,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash/src/sml/masternode_list_engine/mod.rs",
"additions": 159,
"deletions": 1,
"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": 31835,
"language": "rust",
"context": "Dash SPV cryptocurrency library",
"agents": [
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Security & Safety",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Testing & Coverage",
"Architecture & Design",
"Dependencies & Integration"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 12123,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 454,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 37881,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 1825,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 30768,
"status": "success",
"responseLength": 1321,
"inputTokens": 3,
"outputTokens": 1469,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 22124,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 901,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Dependencies & Integration",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 24472,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 1007,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "No findings to evaluate this round.",
"confidenceDistribution": {
"high": 0,
"medium": 0,
"low": 0
},
"severityChanges": 0,
"mergedDuplicates": 0,
"durationMs": 13461,
"retryCount": 0,
"verdictReason": "only_nit_or_suggestion",
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 0,
"resolvedThreadIdCount": 25,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 2050,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 1,
"duplicateMatches": [
{
"droppedTitle": "Truncation boundary (entry at exactly fork_height) not covered by test",
"matchedTitle": "Engine truncation assertion only checks absence, not the surviving boundary",
"matchType": "llm"
}
],
"durationMs": 9143
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 0,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 0
},
"entries": []
},
"usage": {
"inputTokens": 39,
"outputTokens": 9904,
"totalTokens": 9943,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 3405,
"totalTokens": 3414
},
"reviewer": {
"inputTokens": 15,
"outputTokens": 5656,
"totalTokens": 5671
},
"judge": {
"inputTokens": 5,
"outputTokens": 28,
"totalTokens": 33
},
"dedup": {
"inputTokens": 10,
"outputTokens": 815,
"totalTokens": 825
}
}
},
"verdict": "APPROVE",
"recap": {
"priorRoundCount": 7,
"reclassifiedPriorCount": 0
}
}Reviewed commit b53aa8d
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
Implements #142 — Phase 3c of the #137 reorg work. Trims DIP-3 masternode lists and DIP-24 rotated-quorum state back to the fork height, then re-fetches via
qrinfoso chainlock and instantlock validation can resume on the new branch.MasternodeListEngine::truncate_above(height)drops lists, snapshots, quorum statuses, and rotated quorums above the target, mirroring the storage-layer truncation primitives from #149.MasternodesManager::rewind_to_heightwires the engine truncation into the sync manager, resetslast_synced_block_hash, clears pending requests, and re-issuesqrinfofor the new tip.ChainReorgtriggers a cascade: masternodes rewind, chainlock validation hard-blocks untilmasternode_readyflips back on, instantlock validation dropspending_instantlocksand continues soft.test_masternode_list_rewind_across_dip3_reorgis live,test_qrinfo_refresh_across_dip24_cycle_reorgis marked#[ignore](regtest DKG replacement aftermine_reorgneeds follow-up scaffolding, tracked under #142).Closes #142.