test(dash-spv): dashd integration tests for reorg cascade#171
Merged
Conversation
Drives real chain reorgs against a single regtest dashd via `invalidateblock` + `generatetoaddress` and asserts the [`SyncEvent::ChainReorg`](https://github.com/xdustinface/rust-dashcore/blob/dev/dash-spv/src/sync/events.rs) and [`WalletEvent::Reorg`](https://github.com/xdustinface/rust-dashcore/blob/dev/key-wallet-manager/src/events.rs) events fire with the expected `fork_height` and a bumped generation counter. Exposes `invalidate_block`, `reconsider_block`, and `get_block_hash` on `DashCoreNode` so the new module can drive forks without reaching into the underlying RPC client. Phase 6 of [#147](#147). Cases that need infrastructure that is not yet present in the harness (CLSIG injection, masternode-backed LLMQs for IS-locks and DIP-24 rotation, post-cascade downstream resync) ship as scaffolded `#[ignore]` tests with a link to the gating issue so the matrix stays visible and can be re-enabled in place once the gap is closed.
|
Manki — Review complete Planner (21s) Review — 5 findings Judge — 2 kept · 0 dropped (11s) Review metadataConfig:
Judge decisions:
Timing:
|
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reorg integration tests land, but the 'descendant chain' test doesn't actually build a parent→child UTXO spend — it's two independent receives, so cascade-demotion logic goes uncovered.
📊 2 findings (1 suggestion, 1 nitpick) · 393 lines · 253s
Manki context
{
"meta": {
"prNumber": 171,
"commitSha": "21210d350922c35e9cf2e00054cac95332d21279",
"round": 1,
"timestamp": "2026-05-26T16:02:07.230Z",
"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": 393,
"additions": 393,
"deletions": 0,
"filesReviewed": 3,
"fileTypes": {
".rs": 3
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/test_utils/node.rs",
"additions": 21,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_sync/main.rs",
"additions": 1,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_sync/tests_reorg.rs",
"additions": 371,
"deletions": 0,
"changeType": "added"
}
]
},
"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": 2,
"reviewerEffort": "medium",
"judgeEffort": "low",
"prType": "test",
"durationMs": 21443,
"language": "rust",
"context": "Dash SPV blockchain client",
"agents": [
{
"name": "Testing & Coverage",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
}
]
},
"reviewers": {
"agents": [
"Testing & Coverage",
"Correctness & Logic"
],
"agentMetrics": [
{
"name": "Testing & Coverage",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 188948,
"status": "success",
"responseLength": 3166,
"inputTokens": 4,
"outputTokens": 9426,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 2,
"durationMs": 211499,
"status": "success",
"responseLength": 3685,
"inputTokens": 3,
"outputTokens": 12513,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
}
]
},
"judge": {
"summary": "Reorg integration tests land, but the 'descendant chain' test doesn't actually build a parent→child UTXO spend — it's two independent receives, so cascade-demotion logic goes uncovered.",
"confidenceDistribution": {
"high": 1,
"medium": 1,
"low": 0
},
"severityChanges": 2,
"mergedDuplicates": 3,
"durationMs": 11188,
"retryCount": 0,
"verdictReason": "only_nit_or_suggestion",
"defensiveHardeningCount": 1,
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "empty",
"openThreadCount": 0,
"resolvedThreadIdCount": 0,
"interRoundDiffState": "unknown",
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 3
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 2,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 1,
"nitpick": 1
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/tests/dashd_sync/tests_reorg.rs",
"lineStart": 50,
"lineEnd": 50,
"slug": "Broadcast-RecvError--Lagged-silently-treated-as-fatal-in-event-waiter-helpers"
},
"severity": "nitpick",
"specialist": "Testing & Coverage",
"suggestedFix": "recv = receiver.recv() => match recv {\n Ok(event @ SyncEvent::ChainReorg { fork_height, .. })\n if fork_height == expected_fork_height => return event,\n Ok(_) => continue,\n Err(broadcast::error::RecvError::Lagged(_)) => continue,\n Err(err) => panic!(\"sync event channel closed waiti",
"title": "Broadcast RecvError::Lagged silently treated as fatal in event-waiter helpers",
"judgeNotes": "With a 10k-slot buffer lag is unlikely in practice, and on lag the test would just panic instead of silently passing — diagnostic improvement, not a correctness bug. Merged findings 2 and 5.",
"judgeConfidence": "high",
"reachability": "hypothetical",
"reachabilityReasoning": "Broadcast buffer is 10,000 slots and tests emit far fewer events before the awaited reorg, so Lagged is not reached under normal test execution.",
"tags": [
"defensive-hardening"
],
"originalSeverity": "suggestion"
},
{
"fingerprint": {
"file": "dash-spv/tests/dashd_sync/tests_reorg.rs",
"lineStart": 328,
"lineEnd": 328,
"slug": "test-event-handler-observes-chain-reorg-and-wallet-reorg--fresh-wallet-may-not-observe-the-sent-transaction"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "// after wait_for_sync(original_tip)\nlet txid = ctx.dashd.node.send_to_address(...);\n// ...\nwait_for_sync(&mut client.progress_receiver, original_tip).await;\nassert!(ctx.has_transaction(&txid).await, \"wallet must observe tx before reorg\");",
"title": "test_event_handler_observes_chain_reorg_and_wallet_reorg: fresh wallet may not observe the sent transaction",
"judgeNotes": "Single reviewer; the concern is real but the failure mode is a 60s timeout with a clear panic message rather than silent pass. Adding a precondition assert is a diagnostic improvement, not a correctness fix.",
"judgeConfidence": "medium",
"reachability": "unknown"
}
]
},
"usage": {
"inputTokens": 21,
"outputTokens": 24462,
"totalTokens": 24483,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1937,
"totalTokens": 1946
},
"reviewer": {
"inputTokens": 7,
"outputTokens": 21939,
"totalTokens": 21946
},
"judge": {
"inputTokens": 5,
"outputTokens": 586,
"totalTokens": 591
}
}
},
"verdict": "APPROVE",
"recap": {
"priorRoundCount": 0,
"reclassifiedPriorCount": 0
}
}Reviewed commit 21210d3
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #171 +/- ##
==========================================
- Coverage 73.82% 73.76% -0.06%
==========================================
Files 325 325
Lines 75204 75204
==========================================
- Hits 55518 55476 -42
- Misses 19686 19728 +42
🚀 New features to boost your workflow:
|
Map `broadcast::error::RecvError::Lagged` to a logged `continue` in `wait_for_chain_reorg` and `wait_for_wallet_reorg` so a saturated broadcast buffer surfaces as a clear timeout instead of an opaque panic. The closed-channel branch still panics with a more specific message. Addresses manki-review comment on PR #171 #171 (comment)
|
Manki — Review complete Planner (25s) Review — 1 findings Judge — 0 kept · 0 dropped (14s) Review metadataConfig:
Timing:
|
There was a problem hiding this comment.
Lag handling in the event waiters is fixed cleanly, but the fresh-wallet derivation concern in test_event_handler_observes_chain_reorg_and_wallet_reorg is untouched — same code shape as round 1.
📊 0 findings (none) · 401 lines · 75s
Manki context
{
"meta": {
"prNumber": 171,
"commitSha": "ae48b4854a1bde2bd09be7bc1c2f3a77462739ee",
"round": 2,
"timestamp": "2026-05-26T16:11:19.123Z",
"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": 401,
"additions": 401,
"deletions": 0,
"filesReviewed": 3,
"fileTypes": {
".rs": 3
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv/src/test_utils/node.rs",
"additions": 21,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_sync/main.rs",
"additions": 1,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_sync/tests_reorg.rs",
"additions": 379,
"deletions": 0,
"changeType": "added"
}
]
},
"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": 2,
"reviewerEffort": "medium",
"judgeEffort": "medium",
"prType": "test",
"durationMs": 25399,
"language": "rust",
"context": "Dash cryptocurrency SPV client",
"agents": [
{
"name": "Testing & Coverage",
"effort": "medium"
},
{
"name": "Correctness & Logic",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Testing & Coverage",
"Correctness & Logic"
],
"agentMetrics": [
{
"name": "Testing & Coverage",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 3282,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 4,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 25806,
"status": "success",
"responseLength": 1179,
"inputTokens": 3,
"outputTokens": 932,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Lag handling in the event waiters is fixed cleanly, but the fresh-wallet derivation concern in test_event_handler_observes_chain_reorg_and_wallet_reorg is untouched — same code shape as round 1.",
"confidenceDistribution": {
"high": 0,
"medium": 0,
"low": 0
},
"severityChanges": 0,
"mergedDuplicates": 0,
"durationMs": 13515,
"retryCount": 0,
"verdictReason": "only_nit_or_suggestion",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6E2fsW",
"status": "not_addressed",
"reason": "The inter-round diff only modifies the wait_for_chain_reorg/wait_for_wallet_reorg helpers to handle Lagged. The test at line 348+ still creates a fresh wallet via create_test_wallet(&ctx.dashd.wallet.mnemonic, ...) and uses ctx.receive_address() without any reconciliation of derivation state."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 1,
"resolvedThreadIdCount": 1,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 1547,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 1,
"duplicateMatches": [
{
"droppedTitle": "Target reorg event may be silently dropped when receiver lags",
"matchedTitle": "Broadcast RecvError::Lagged silently treated as fatal in event-waiter helpers",
"matchType": "llm"
}
],
"durationMs": 5437
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 0,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 0
},
"entries": []
},
"usage": {
"inputTokens": 30,
"outputTokens": 3932,
"totalTokens": 3962,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2231,
"totalTokens": 2240
},
"reviewer": {
"inputTokens": 6,
"outputTokens": 936,
"totalTokens": 942
},
"judge": {
"inputTokens": 5,
"outputTokens": 312,
"totalTokens": 317
},
"dedup": {
"inputTokens": 10,
"outputTokens": 453,
"totalTokens": 463
}
}
},
"verdict": "APPROVE",
"recap": {
"priorRoundCount": 1,
"reclassifiedPriorCount": 0
}
}Reviewed commit ae48b48
|
Review skipped for
|
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
dash-spv/tests/dashd_sync/tests_reorg.rswith 13 reorg integration tests against a regtest dashd, driven viainvalidateblock+generatetoaddress(single-node).DashCoreNodewithinvalidate_block,reconsider_block, andget_block_hashhelpers so future reorg tests don't reach around the harness.dash-spv/tests/dashd_sync/main.rs.SyncEvent::ChainReorgandWalletEvent::Reorgfire on the cascade boundary for shallow, deep-within-cap, descendant-tx, in-flight-filter-fetch, and event-handler join scenarios.#[ignore]'d with one-line reasons linking the gating issue (#141, #142, #143).Test plan
cargo fmt --checkcargo clippy -p dash-spv --all-targets --all-features -- -D warningscargo test -p dash-spv --test dashd_sync reorg::with a live regtest dashd (eval $(python3 contrib/setup-dashd.py)); the 5 active tests pass.Observations from this work (worth raising separately, not addressed here)
handle_reorgtruncates and downstream managers logcascading ChainReorg, resetting pipeline, none of them re-request data. Header sync halts one block short of dashd's tip, filter/block pipelines sit inWaitForEvents. This is the root cause of several#[ignore]'d tests. Likely belongs against the Phase 3 cascade work (#140).DeepReorgDetectedunreachable on a fresh client. The fresh-client fork floor short-circuits before the depth-cap guard runs, so the cap-exceeded case rejects via the floor rather than emittingDeepReorgDetected. Either the floor should defer to the cap check, or the floor rejection should also emitDeepReorgDetected.generatetoaddresson the same miner across aninvalidateblockproduces a duplicate coinbase that dashd rejects. Worked around by minting fork blocks to a freshgetnewaddress. Worth documenting onDashCoreNode.Refs #147.