feat(dash-spv): EnforceBestChainLock, force reorg toward chainlocked chain#166
Merged
Conversation
`handle_reorg` (and `run_guards_and_cascade`) now take a `force: bool` parameter. When `force` is `true` the depth cap is skipped (a validated `ChainLockForcedReorg` overrides it). Checkpoint floor, chainlock floor, single-flight, and deny-list guards still apply. Updates the single in-tree caller in `BlockHeadersManager::drive_reorg` to pass `force: false` and exposes a `force` parameter so an upcoming forced-reorg path can override the cap.
…g` `SyncEvent` variants Adds two purely additive `SyncEvent` variants used by the upcoming `EnforceBestChainLock` path: - `PendingChainLockQueued` signals that a CLSig arrived for a block hash the local header chain has not resolved yet. The chainlock is held in a pending queue and re-evaluated when the matching header arrives. - `ChainLockForcedReorg` signals that a validated CLSig disagrees with the local block at its claimed height and the chain must reorg onto the chainlocked branch. Other managers fall through their existing `_` arms, so no consumer changes are required.
Replaces the previous boolean `verify_block_hash` with a 3-way [`BlockHashVerification`] enum (`Match`, `Mismatch { local_header_hash }`, `Unknown`). `process_chainlock` now routes each branch explicitly:
- `Match`: existing validated path (unchanged).
- `Unknown`: queue the CLSig in a new `pending_unknown_hash` map (5min TTL) and emit `PendingChainLockQueued`. The queued entry preserves the delivering peer for later misbehavior reporting if the hash later resolves to a `Mismatch`.
- `Mismatch`: rejected for now; the forced-reorg dispatch lands in a follow-up commit.
The existing `pending_validation` cache (masternode-not-ready) is unchanged and runs ahead of the unknown-hash queue so a pre-ready CLSig still gets retried on the not-ready → ready transition.
`tick` calls `drain_expired_pending` to evict stale entries.
`ChainLockManager::retry_pending_unknown_hash` re-evaluates every queued CLSig against the current header chain. `Match` entries are removed and fed back through `process_chainlock` so the signature path runs and the chainlock is promoted on success. `Mismatch` entries are removed and rejected for now. The forced-reorg dispatch on `Mismatch` lands in a follow-up commit. The chainlock sync manager invokes the retry on every `BlockHeadersStored` sync event so newly-stored headers immediately unstick CLSigs that were waiting on them.
…Reorg`
Wires the full DIP-0008 EnforceBestChainLock path:
1. `MisbehaviorKind::InvalidChainLockSignature` and a new `NetworkRequest::ReportMisbehavior(SocketAddr, MisbehaviorKind)` variant let any sync manager apply a reputation penalty without holding a direct handle to the `PeerReputationManager`. The network manager handles the request by calling `update_reputation(.., misbehavior_scores::INVALID_CHAINLOCK, ..)`. `RequestSender::report_misbehavior` is the new public API.
2. `ChainLockManager::process_chainlock` now takes a `requests: &RequestSender`. On a `Mismatch` branch the signature is validated first: an invalid signature reports the delivering peer for `InvalidChainLockSignature`. A valid one emits `ChainLockForcedReorg { chain_lock, fork_height }`. The retry path over the pending-unknown-hash queue runs the same logic.
3. `BlockHeadersManager` handles `ChainLockForcedReorg`: it stores the chainlock in `cl_forced_reorg_pending` and broadcasts a getheaders so peers on the chainlocked branch deliver the missing headers. When incoming headers ingest into the fork buffer, `try_drive_forced_reorg` pulls the branch by tip hash (no work check) and calls `drive_reorg(.., force: true)` to bypass the depth cap. The chainlock-floor side-list is also bypassed for the forced-reorg target so the cascade can succeed.
The `PendingChainLockQueued` and `ChainLockForcedReorg` variants are not surfaced to the FFI callback layer for now. Consumers receive the resulting `ChainReorg` once the cascade completes.
…ashd Adds a `dashd_masternode` integration test that walks the full DIP-0008 forced-reorg path: sync against the regtest masternode network, wait for a chainlock, drive `mine_reorg(4)` so the chainlocked block is orphaned, wait for a new chainlock on the replacement chain, and assert `ChainReorg` fires for the orchestrated `fork_height`. Marked `#[ignore]` because regtest CLSig re-delivery onto a peer pinned to a different branch is timing-sensitive. The controller and masternodes share one chain state, so once `mine_reorg` succeeds the masternodes pivot too and the SPV gets the new branch through normal headers sync, not through the forced-reorg path. Reproducing a true CL-vs-local disagreement in regtest requires peer-network isolation the current harness does not provide, tracked under [#141](#141).
|
Manki — Review complete Planner (17s) Review — 7 findings Judge — 7 kept · 0 dropped (19s) Review metadataConfig:
Judge decisions:
Timing:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #166 +/- ##
==========================================
+ Coverage 73.34% 73.58% +0.24%
==========================================
Files 324 324
Lines 73074 73818 +744
==========================================
+ Hits 53598 54322 +724
- Misses 19476 19496 +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.
Forced-reorg path has real gaps: the chainlock floor still rejects forced candidates (defeating eclipse recovery), a last-wins overwrite lets a malicious peer suppress retries, and the Mismatch arm in on_masternode_ready silently drops what should be a forced reorg.
📊 7 findings (1 blocker, 2 warning, 4 suggestion) · 1110 lines · 497s
Manki context
{
"meta": {
"prNumber": 166,
"commitSha": "aa218ff0cc8149a5bc175c5da689ae895086ebf5",
"round": 1,
"timestamp": "2026-05-26T00:50:27.814Z",
"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": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1110,
"additions": 1028,
"deletions": 82,
"filesReviewed": 11,
"fileTypes": {
".rs": 11
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 9,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/manager.rs",
"additions": 14,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/mod.rs",
"additions": 19,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 18,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 168,
"deletions": 24,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/sync_manager.rs",
"additions": 26,
"deletions": 4,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 524,
"deletions": 37,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 13,
"deletions": 3,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/events.rs",
"additions": 38,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/reorg.rs",
"additions": 90,
"deletions": 9,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 109,
"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": "high",
"prType": "feat",
"durationMs": 16829,
"language": "rust",
"context": "Dash SPV blockchain client",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 3,
"findingsKept": 3,
"durationMs": 272210,
"status": "success",
"responseLength": 4770,
"inputTokens": 3,
"outputTokens": 15078,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 452001,
"status": "success",
"responseLength": 6544,
"inputTokens": 4,
"outputTokens": 24403,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 181805,
"status": "success",
"responseLength": 5118,
"inputTokens": 3,
"outputTokens": 9509,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"durationMs": 128264,
"status": "success",
"responseLength": 5005,
"inputTokens": 3,
"outputTokens": 7252,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Forced-reorg path has real gaps: the chainlock floor still rejects forced candidates (defeating eclipse recovery), a last-wins overwrite lets a malicious peer suppress retries, and the Mismatch arm in on_masternode_ready silently drops what should be a forced reorg.",
"confidenceDistribution": {
"high": 5,
"medium": 2,
"low": 0
},
"severityChanges": 7,
"mergedDuplicates": 0,
"durationMs": 19104,
"retryCount": 0,
"verdictReason": "required_present",
"verdictTrace": {
"survivingBlockers": [
{
"file": "dash-spv/src/sync/reorg.rs",
"title": "Chainlock floor guard blocks forced reorg even with \u0060force=true\u0060",
"fingerprint": "dash-spv/src/sync/reorg.rs:176:176:Chainlock-floor-guard-blocks-forced-reorg-even-with--force-true-"
}
],
"novelWarnings": [],
"unresolvedPriors": []
},
"openThreadsState": "empty",
"openThreadCount": 0,
"resolvedThreadIdCount": 0,
"interRoundDiffState": "unknown",
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 7,
"severityCounts": {
"blocker": 1,
"warning": 2,
"suggestion": 4,
"nitpick": 0
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/reorg.rs",
"lineStart": 176,
"lineEnd": 176,
"slug": "Chainlock-floor-guard-blocks-forced-reorg-even-with--force-true-"
},
"severity": "blocker",
"specialist": "Security & Safety",
"suggestedFix": "Add a \u0060force\u0060 gate to the chainlock floor branch analogous to the depth-cap gate:\n\u0060\u0060\u0060rust\nif !force {\n match best_chainlock_height {\n Some(cl_height) => {\n if is_below_chainlock_floor(candidate.ancestor_height, cl_height) {\n // reject ...\n }\n }\n ",
"title": "Chainlock floor guard blocks forced reorg even with \u0060force=true\u0060",
"judgeNotes": "Impact: High (defeats the PR's stated eclipse-recovery goal and can create a permanent deny-list lockout), Likelihood: Possible (requires the eclipse scenario but is exactly the case the feature targets). Single reviewer but the logic claim is concrete and directly contradicts the PR description.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 248,
"lineEnd": 248,
"slug": "Last-wins-overwrite-in--pending-unknown-hash--lets-a-peer-suppress-chainlock-retry"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "Only overwrite if no existing entry is present, or if the incoming CLSig is for a strictly higher block height (deduplication by hash is already safe for honest quorums since the threshold signature is deterministic):\n\u0060\u0060\u0060rust\nself.pending_unknown_hash\n .entry(block_hash)\n .or_insert_with(|| (c",
"title": "Last-wins overwrite in \u0060pending_unknown_hash\u0060 lets a peer suppress chainlock retry",
"judgeNotes": "Impact: High (an attacker controlling peers can suppress legitimate CLSig retries), Likelihood: Possible (requires adversarial peer that observes the hash). Reachable via the queue path; fix is small and correct.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 281,
"lineEnd": 281,
"slug": "-progress-add-valid-1---counted-before-forced-reorg-cascade-completes"
},
"severity": "suggestion",
"specialist": "Security & Safety",
"suggestedFix": "Move \u0060progress.add_valid(1)\u0060 into \u0060try_drive_forced_reorg\u0060, emitting it only when \u0060drive_reorg\u0060 returns \u0060Some(SyncEvent::ChainReorg { .. })\u0060. Alternatively, introduce a separate \u0060progress.add_forced_reorg_pending(1)\u0060 counter distinct from \u0060valid\u0060 so the counters remain accurate.",
"title": "\u0060progress.add_valid(1)\u0060 counted before forced-reorg cascade completes",
"judgeNotes": "Impact: Low/Medium (only affects a counter, no behavioral consequence), Likelihood: Probable (any rejected forced reorg). Downgraded from warning — the metric drift is real but not a behavioral bug.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 142,
"lineEnd": 142,
"slug": "on-masternode-ready-silently-drops-Mismatch-chainlock-instead-of-emitting-ChainLockForcedReorg"
},
"severity": "warning",
"specialist": "Testing & Coverage",
"suggestedFix": "// in on_masternode_ready, replace the header_ok gate:\nmatch self.verify_block_hash(&pending).await {\n BlockHashVerification::Match | BlockHashVerification::Unknown => {\n if self.validate_signature(&pending).await { /* existing accept path */ }\n }\n BlockHashVerification::Mismatch { .",
"title": "on_masternode_ready silently drops Mismatch chainlock instead of emitting ChainLockForcedReorg",
"judgeNotes": "Impact: High (loses the EnforceBestChainLock guarantee during the initial-sync window), Likelihood: Possible (requires CLSig arriving before MN sync and a mismatching local header). Inconsistent with the post-ready Mismatch arm; legitimate behavioral gap.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 412,
"lineEnd": 412,
"slug": "Missing-test-and-stale-doc-comment-for-Mismatch-path-in-retry-pending-unknown-hash"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "/// - \u0060Mismatch\u0060: the entry is removed and re-processed through \u0060process_chainlock\u0060.\n/// If the BLS signature validates, \u0060ChainLockForcedReorg\u0060 is emitted and the\n/// delivering peer is penalised if present. If it does not validate, the entry\n/// is dropped and misbehavior is reported.",
"title": "Missing test and stale doc comment for Mismatch path in retry_pending_unknown_hash",
"judgeNotes": "Impact: Low (stale doc + missing test, not a behavioral bug), Likelihood: Certain for the doc, N/A for behavior. Downgraded from warning — code works, documentation/test gap only.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/manager.rs",
"lineStart": 98,
"lineEnd": 98,
"slug": "cl-forced-reorg-pending-not-tested-for-cleanup-on-reset-paths"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"title": "cl_forced_reorg_pending not tested for cleanup on reset paths",
"judgeNotes": "Impact: Medium (a stale pending CL could drive incorrect reorg after reconnect if not cleared), Likelihood: Possible. Reviewer flagged it as suggestion; coverage gap is legitimate.",
"judgeConfidence": "medium",
"reachability": "unknown",
"reachabilityReasoning": "Whether reset_for_reorg/on_disconnect actually clear the field cannot be confirmed from the diff excerpts alone."
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"lineStart": 308,
"lineEnd": 308,
"slug": "take-branch-by-tip-has-no-direct-unit-test-in-fork-buffer"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"title": "take_branch_by_tip has no direct unit test in fork_buffer",
"judgeNotes": "Impact: Low (indirectly covered by integration test), Likelihood: N/A. Reasonable suggestion to add a direct unit test matching the module's existing test style.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 27,
"outputTokens": 58987,
"totalTokens": 59014,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1573,
"totalTokens": 1582
},
"reviewer": {
"inputTokens": 13,
"outputTokens": 56242,
"totalTokens": 56255
},
"judge": {
"inputTokens": 5,
"outputTokens": 1172,
"totalTokens": 1177
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 0,
"reclassifiedPriorCount": 0
}
}Reviewed commit aa218ff
…_cascade` A BLS-validated CLSig driving a forced reorg has higher authority than the locally cached chainlock height. Wrapping the chainlock floor check in `if !force` mirrors the existing depth-cap gate and allows recovery when the common ancestor lies below the stored floor. Addresses manki-review comment on PR #166 #166 (comment)
…h` insert Last-wins overwrite allowed a peer sending a CLSig with the correct hash but invalid BLS signature to displace the honest peer's entry, causing the honest CLSig to be silently discarded. Keeping the first insertion prevents this without affecting TTL eviction semantics. Addresses manki-review comment on PR #166 #166 (comment)
… on `Mismatch` A CLSig arriving during initial sync that points to a competing block was silently dropped in `on_masternode_ready`. This inconsistency with the `process_chainlock` Mismatch path meant the forced-reorg window was missed for that timing window. The method now returns `Vec<SyncEvent>` and mirrors the Mismatch arm from `process_chainlock`. Addresses manki-review comment on PR #166 #166 (comment)
…ash` `Mismatch` arm The comment said forced-reorg dispatch was in a follow-up commit, but the implementation already re-routes through `process_chainlock` which handles Mismatch. Also renames and extends the corresponding unit test to assert the correct behavior (invalid-sig mismatch drops the entry and counts invalid). Addresses manki-review comment on PR #166 #166 (comment)
…uard block On disconnect the pending forced-reorg tip must be cleared so it cannot drive a reorg against a different branch sharing the same tip hash on reconnect. On guard block (deny-list or checkpoint rejection), the field is now cleared unconditionally once a candidate has been consumed from the fork buffer, since retrying the same candidate will always fail. Addresses manki-review comment on PR #166 #166 (comment)
…rkBuffer` Covers: (a) existing tip found removes the branch and returns the correct `ForkCandidate`, (b) unknown tip returns `None` and leaves the buffer unchanged, (c) two branches with different tips only the targeted branch is removed. Matches the coverage style of other `ForkBuffer` methods. Addresses manki-review comment on PR #166 #166 (comment)
|
Manki — Review complete Planner (24s) Review — 11 findings Judge — 7 kept · 0 dropped (67s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
All six previously open threads have direct fixes — floor guard now wrapped in if !force, pending_unknown_hash uses or_insert_with, Mismatch path in on_masternode_ready emits ChainLockForcedReorg, cl_forced_reorg_pending cleared on disconnect and guard-block paths with new tests, and three direct unit tests for take_branch_by_tip. New round adds one substantive concern: the Mismatch+valid-sig path emits ChainLockForcedReorg but never advances best_chainlock, leaving the chainlock floor stale until a follow-up CLSig arrives.
📊 7 findings (1 warning, 6 suggestion) · 1431 lines · 468s
Manki context
{
"meta": {
"prNumber": 166,
"commitSha": "f5e8b16f1d27dc6d71180f8772694296da0c0b26",
"round": 2,
"timestamp": "2026-05-26T01:16:00.084Z",
"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": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1431,
"additions": 1276,
"deletions": 155,
"filesReviewed": 11,
"fileTypes": {
".rs": 11
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 9,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/manager.rs",
"additions": 14,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/mod.rs",
"additions": 19,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 94,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 254,
"deletions": 24,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/sync_manager.rs",
"additions": 31,
"deletions": 4,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 574,
"deletions": 70,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 18,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/events.rs",
"additions": 38,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/reorg.rs",
"additions": 119,
"deletions": 34,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 109,
"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": "high",
"prType": "feat",
"durationMs": 24297,
"language": "rust",
"context": "Dash SPV blockchain client",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"effort": "high"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 267272,
"status": "success",
"responseLength": 2457,
"inputTokens": 3,
"outputTokens": 14887,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 3,
"findingsKept": 2,
"durationMs": 365472,
"status": "success",
"responseLength": 3827,
"inputTokens": 3,
"outputTokens": 20420,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 2,
"findingsKept": 2,
"durationMs": 108209,
"status": "success",
"responseLength": 2558,
"inputTokens": 3,
"outputTokens": 5936,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"findingsRaw": 4,
"findingsKept": 4,
"durationMs": 148476,
"status": "success",
"responseLength": 6489,
"inputTokens": 3,
"outputTokens": 7797,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
}
]
},
"judge": {
"summary": "All six previously open threads have direct fixes — floor guard now wrapped in \u0060if !force\u0060, \u0060pending_unknown_hash\u0060 uses \u0060or_insert_with\u0060, Mismatch path in \u0060on_masternode_ready\u0060 emits \u0060ChainLockForcedReorg\u0060, \u0060cl_forced_reorg_pending\u0060 cleared on disconnect and guard-block paths with new tests, and three direct unit tests for \u0060take_branch_by_tip\u0060. New round adds one substantive concern: the Mismatch+valid-sig path emits \u0060ChainLockForcedReorg\u0060 but never advances \u0060best_chainlock\u0060, leaving the chainlock floor stale until a follow-up CLSig arrives.",
"confidenceDistribution": {
"high": 3,
"medium": 3,
"low": 1
},
"severityChanges": 7,
"mergedDuplicates": 4,
"durationMs": 66893,
"retryCount": 0,
"verdictReason": "novel_suggestion",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6Epq7P",
"status": "addressed",
"reason": "reorg.rs:180-186 now wraps the chainlock-floor guard in \u0060if !force { match best_chainlock_height { ... } }\u0060, so a \u0060force=true\u0060 cascade bypasses the floor as the original concern required."
},
{
"threadId": "PRRT_kwDOQSlaXs6Epq7d",
"status": "addressed",
"reason": "Inter-round diff at manager.rs:258-262 changes \u0060.insert(block_hash, ...)\u0060 to \u0060.entry(block_hash).or_insert_with(...)\u0060, implementing first-wins so a malicious second peer cannot overwrite the honest entry."
},
{
"threadId": "PRRT_kwDOQSlaXs6Epq7l",
"status": "addressed",
"reason": "The Mismatch arm in \u0060on_masternode_ready\u0060 now validates the BLS signature and returns \u0060vec![SyncEvent::ChainLockForcedReorg { chain_lock: pending, fork_height }]\u0060 instead of silently dropping, matching the post-ready behavior."
},
{
"threadId": "PRRT_kwDOQSlaXs6Epq7o",
"status": "addressed",
"reason": "Doc comment for \u0060retry_pending_unknown_hash\u0060 is updated to describe the live Mismatch→process_chainlock dispatch and ChainLockForcedReorg emission. The test was renamed and updated to assert the new behavior (Mismatch + invalid sig → drop). Coverage of valid-sig Mismatch retry still lacks a test, but the stale doc and primary test concern is addressed."
},
{
"threadId": "PRRT_kwDOQSlaXs6Epq7q",
"status": "addressed",
"reason": "sync_manager.rs \u0060on_disconnect\u0060 now clears \u0060cl_forced_reorg_pending\u0060; manager.rs \u0060try_drive_forced_reorg\u0060 always clears on completion (including guard rejection). Two new tests \u0060cl_forced_reorg_pending_cleared_on_disconnect\u0060 and \u0060cl_forced_reorg_pending_cleared_when_guard_blocks\u0060 cover both paths."
},
{
"threadId": "PRRT_kwDOQSlaXs6Epq7r",
"status": "addressed",
"reason": "Three direct unit tests added in fork_buffer.rs covering the cases from the original concern: removal of matching branch, None for unknown tip, and isolation when multiple peers share an ancestor but have different tips."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "Mismatch forced-reorg path never advances best_chainlock floor",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:154:154:Mismatch-forced-reorg-path-never-advances-best-chainlock-floor"
}
],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 6,
"resolvedThreadIdCount": 1,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 26305,
"interRoundDiffTruncated": true
},
"dedup": {
"staticDropped": 0,
"llmDropped": 0,
"sameRoundLlmDropped": 4,
"durationMs": 10880
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 7,
"severityCounts": {
"blocker": 0,
"warning": 1,
"suggestion": 6,
"nitpick": 0
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 148,
"lineEnd": 148,
"slug": "No-test-for--on-masternode-ready--Mismatch---valid-BLS-sig----ChainLockForcedReorg--path"
},
"severity": "suggestion",
"specialist": "Security & Safety",
"suggestedFix": "Add a test that injects a \u0060Mismatch\u0060 pending CLSig whose BLS signature will pass validation, calls \u0060on_masternode_ready\u0060, and asserts the returned \u0060Vec<SyncEvent>\u0060 contains exactly one \u0060SyncEvent::ChainLockForcedReorg\u0060 with the correct \u0060block_height\u0060 and \u0060fork_height = block_height - 1\u0060. A mock or p",
"title": "No test for \u0060on_masternode_ready\u0060 Mismatch + valid BLS sig → \u0060ChainLockForcedReorg\u0060 path",
"judgeNotes": "Merged findings 1, 7, 8 — three reviewers independently flagged the same coverage gap. Real gap: the new Mismatch+valid-sig branch is uncovered by non-ignored tests. Impact: Medium (regression risk on a key new code path), Likelihood: Possible. Worth adding but not blocking.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/reorg.rs",
"lineStart": 183,
"lineEnd": 183,
"slug": "Chainlock-floor-bypass---if--force---in--handle-reorg--has-no-test-that-actually-exercises-it"
},
"severity": "suggestion",
"specialist": "Security & Safety",
"suggestedFix": "Add a test variant of \u0060force_bypasses_depth_cap_and_cascades\u0060 that sets \u0060best_chainlock_height: Some(50)\u0060 with \u0060ancestor_height = 40\u0060 (so \u0060is_below_chainlock_floor(40, 50)\u0060 is \u0060true\u0060), calls \u0060handle_reorg\u0060 with \u0060force = true\u0060, and asserts a \u0060ChainReorg\u0060 event is returned — proving the chainlock-floo",
"title": "Chainlock-floor bypass (\u0060if !force\u0060) in \u0060handle_reorg\u0060 has no test that actually exercises it",
"judgeNotes": "Merged findings 2, 4, 9 — three reviewers independently flagged that existing \u0060force_bypasses_depth_cap_and_cascades\u0060 passes \u0060cl_height=0\u0060 with \u0060ancestor=10\u0060, so the floor never fires and the new \u0060if !force\u0060 branch is untested. Real regression-protection gap. Suggestion is appropriate.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 138,
"lineEnd": 138,
"slug": "fork-height-set-to-block-height-1-in-on-masternode-ready-Mismatch-path-may-produce-wrong-getheaders-locator"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "Pass a conservative lower-bound (e.g., the best chainlock height or 0) so the getheaders locator includes the real divergence point, or walk storage backward from block_height to find the actual last common header. The cascade path is unaffected; only the locator construction for the branch-fetch ne",
"title": "fork_height set to block_height-1 in on_masternode_ready Mismatch path may produce wrong getheaders locator",
"judgeNotes": "Impact: Medium (forced reorg could stall if locator is wrong), Likelihood: Possible but the reviewer themselves notes 'If fork_height is used only for diagnostics/logging and getheaders always uses the full local-chain locator, this is benign'. Without confirmation that fork_height drives the locator, downgrading from warning to suggestion.",
"judgeConfidence": "low",
"reachability": "unknown"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 139,
"lineEnd": 139,
"slug": "on-masternode-ready-Mismatch-valid-sig-path-skips-best-chainlock-re-broadcast"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "After the early return with ChainLockForcedReorg, optionally also include a ChainLockReceived event for self.best_chainlock if it exists — or document explicitly that re-broadcasting the old best_chainlock during a forced-reorg transition is intentionally suppressed to avoid racing with the incoming",
"title": "on_masternode_ready Mismatch+valid-sig path skips best_chainlock re-broadcast",
"judgeNotes": "Single reviewer; design tradeoff rather than bug. Suppressing the old CL re-broadcast during a forced-reorg transition is arguably correct to avoid racing with the cascade. Worth documenting but not changing.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 154,
"lineEnd": 154,
"slug": "Mismatch-forced-reorg-path-never-advances-best-chainlock-floor"
},
"severity": "warning",
"specialist": "Architecture & Design",
"suggestedFix": "BlockHashVerification::Mismatch { .. } => {\n if self.validate_signature(&pending).await {\n self.progress.add_valid(1);\n // Advance the floor so the reorg cascade and future guards\n // treat the newly-validated CL height as finalized.\n let height = pending.block_height;",
"title": "Mismatch forced-reorg path never advances best_chainlock floor",
"judgeNotes": "Impact: High (chainlock floor guard wouldn't protect blocks up to the new validated CL height, so a subsequent deep reorg below that height could slip through guards), Likelihood: Possible (requires another reorg after the forced one). Real safety concern — the validated CLSig should advance the floor. Single reviewer but the analysis is concrete.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 258,
"lineEnd": 258,
"slug": "pending-unknown-hash-first-wins-fix-has-no-regression-test"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "// Call process_chainlock twice for the same unknown hash from two peers.\n// Then inspect pending_unknown_hash to assert the first peer is retained.\nlet _ = manager.process_chainlock(&cl, Some(peer_a), &requests).await.unwrap();\nlet _ = manager.process_chainlock(&cl, Some(peer_b), &requests).await.u",
"title": "pending_unknown_hash first-wins fix has no regression test",
"judgeNotes": "Single reviewer flagged a missing regression test for the \u0060or_insert_with\u0060 fix. Real coverage gap but the fix itself is correct and small; the existing prior-round thread is resolved.",
"judgeConfidence": "high",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"lineStart": 308,
"lineEnd": 308,
"slug": "take-branch-by-tip-arbitrarily-selects-when-multiple-peers-share-the-same-tip-hash---unspecified-and-untested"
},
"severity": "suggestion",
"specialist": "Testing & Coverage",
"suggestedFix": "// One possible contract: document and test that take_branch_by_tip removes\n// exactly one entry when multiple peers share the same tip hash.\nbuf.ingest(peer_a, &fork, ancestor_height, ancestor, &active).unwrap();\nbuf.ingest(peer_b, &fork, ancestor_height, ancestor, &active).unwrap();\nassert_eq!(buf",
"title": "take_branch_by_tip arbitrarily selects when multiple peers share the same tip hash — unspecified and untested",
"judgeNotes": "Single reviewer; the buffer key is \u0060(peer, tip)\u0060 so duplicates by tip can exist. Behavior is non-deterministic but in practice removing either is acceptable for the forced-reorg path (both branches share the same headers). Worth documenting but not blocking.",
"judgeConfidence": "medium",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 36,
"outputTokens": 57009,
"totalTokens": 57045,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2299,
"totalTokens": 2308
},
"reviewer": {
"inputTokens": 12,
"outputTokens": 49040,
"totalTokens": 49052
},
"judge": {
"inputTokens": 5,
"outputTokens": 4333,
"totalTokens": 4338
},
"dedup": {
"inputTokens": 10,
"outputTokens": 1337,
"totalTokens": 1347
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 1,
"reclassifiedPriorCount": 0
}
}Reviewed commit f5e8b16
…` Mismatch arm Addresses manki-review comment on PR #166 #166 (comment)
- Mismatch + invalid-sig arm side-effects - chainlock-floor bypass exercised with `force=true` in `handle_reorg` - first-wins behavior of `pending_unknown_hash` after `or_insert_with` fix - `take_branch_by_tip` removes exactly one entry when two peers share the same tip hash Addresses manki-review comments on PR #166 #166 (comment) #166 (comment) #166 (comment) #166 (comment)
|
Manki — Review complete Planner (21s) Review — 6 findings Judge — 3 kept · 0 dropped (57s) Review metadataConfig:
Judge decisions:
Timing:
|
There was a problem hiding this comment.
Round 2's open threads are largely addressed: tests added for chainlock floor bypass, first-wins, same-tip take_branch_by_tip, and Mismatch arm now advances best_chainlock. New concern: persisting the chainlock floor before the cascade actually completes creates a window where disk state and active chain disagree.
📊 3 findings (1 warning, 2 suggestion) · 1638 lines · 390s
Manki context
{
"meta": {
"prNumber": 166,
"commitSha": "defc0c3d35002c7e0918c20ed6bf6865b27817e8",
"round": 3,
"timestamp": "2026-05-26T01:39:18.656Z",
"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": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1638,
"additions": 1483,
"deletions": 155,
"filesReviewed": 11,
"fileTypes": {
".rs": 11
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 9,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/manager.rs",
"additions": 14,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/mod.rs",
"additions": 19,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 124,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 254,
"deletions": 24,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/sync_manager.rs",
"additions": 31,
"deletions": 4,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 646,
"deletions": 70,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 18,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/events.rs",
"additions": 38,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/reorg.rs",
"additions": 224,
"deletions": 34,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 109,
"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": 20924,
"language": "rust",
"context": "Dash SPV client library",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 119064,
"status": "success",
"responseLength": 2819,
"inputTokens": 3,
"outputTokens": 6435,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 2,
"findingsKept": 1,
"durationMs": 304495,
"status": "success",
"responseLength": 2648,
"inputTokens": 3,
"outputTokens": 17194,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 1,
"findingsKept": 1,
"durationMs": 95149,
"status": "success",
"responseLength": 1309,
"inputTokens": 3,
"outputTokens": 4940,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 110613,
"status": "success",
"responseLength": 1888,
"inputTokens": 3,
"outputTokens": 5666,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "Round 2's open threads are largely addressed: tests added for chainlock floor bypass, first-wins, same-tip take_branch_by_tip, and Mismatch arm now advances best_chainlock. New concern: persisting the chainlock floor before the cascade actually completes creates a window where disk state and active chain disagree.",
"confidenceDistribution": {
"high": 1,
"medium": 2,
"low": 0
},
"severityChanges": 3,
"mergedDuplicates": 0,
"durationMs": 56843,
"retryCount": 0,
"verdictReason": "novel_suggestion",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6Ep50J",
"status": "addressed",
"reason": "New test test_on_masternode_ready_mismatch_invalid_sig_counts_invalid added in manager.rs covers the Mismatch arm; documents that valid-sig path is covered by dashd integration tests since unit-test engine cannot validate BLS."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50T",
"status": "addressed",
"reason": "New test force_bypasses_chainlock_floor_and_cascades in reorg.rs explicitly exercises both force=false (floor rejects) and force=true (floor bypassed) with ancestor_height=40 and best_chainlock_height=Some(50)."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50b",
"status": "not_addressed",
"reason": "fork_height is still set to pending.block_height.saturating_sub(1) in the inter-round diff with no logic change to look up an actual common ancestor."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50i",
"status": "not_addressed",
"reason": "The Mismatch+valid-sig arm still returns early with only the ChainLockForcedReorg event; no ChainLockReceived re-broadcast of best_chainlock was added."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50n",
"status": "addressed",
"reason": "Inter-round diff adds self.best_chainlock = Some(pending.clone()), self.chainlock_height.store(height, ...), and self.save_best_chainlock().await before emitting ChainLockForcedReorg."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50s",
"status": "addressed",
"reason": "New test test_process_chainlock_unknown_hash_first_wins asserts peer_a is retained after a second process_chainlock from peer_b for the same unknown hash."
},
{
"threadId": "PRRT_kwDOQSlaXs6Ep50x",
"status": "addressed",
"reason": "New test take_branch_by_tip_removes_exactly_one_when_two_peers_share_same_tip documents the contract that exactly one of the two same-tip branches is consumed."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "best_chainlock floor persisted before reorg cascade commits",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:151:151:best-chainlock-floor-persisted-before-reorg-cascade-commits"
}
],
"unresolvedPriors": []
},
"openThreadsState": "fetched",
"openThreadCount": 7,
"resolvedThreadIdCount": 7,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 11143,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 0,
"llmDropped": 3,
"duplicateMatches": [
{
"droppedTitle": "force=true deny-list bypass not exercised in the new reorg test",
"matchedTitle": "Chainlock floor guard blocks forced reorg even with \u0060force=true\u0060",
"matchType": "llm"
},
{
"droppedTitle": "force=true reorg test uses fresh ReorgState, hiding deny-list blocking of forced reorgs",
"matchedTitle": "Chainlock floor guard blocks forced reorg even with \u0060force=true\u0060",
"matchType": "llm"
},
{
"droppedTitle": "New Mismatch+valid-sig production code has no runnable unit test",
"matchedTitle": "Missing test and stale doc comment for Mismatch path in retry_pending_unknown_hash",
"matchType": "llm"
}
],
"durationMs": 35511
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 3,
"severityCounts": {
"blocker": 0,
"warning": 1,
"suggestion": 2,
"nitpick": 0
},
"entries": [
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 151,
"lineEnd": 151,
"slug": "best-chainlock-floor-persisted-before-reorg-cascade-commits"
},
"severity": "warning",
"specialist": "Security & Safety",
"suggestedFix": "// Emit the event first; let BlockHeadersManager confirm the cascade.\nlet fork_height = pending.block_height.saturating_sub(1);\nlet event = SyncEvent::ChainLockForcedReorg { chain_lock: pending.clone(), fork_height };\n// Advance the floor only after the coordinator confirms ChainReorg was emitted.\n/",
"title": "best_chainlock floor persisted before reorg cascade commits",
"judgeNotes": "Impact: Medium-High (persisted floor diverges from active chain if cascade fails), Likelihood: Possible (requires peer disconnect or branch validation failure between event emit and cascade completion). Real correctness concern given this PR's stated goal of resolving chainlock/active-chain inconsistency.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 156,
"lineEnd": 156,
"slug": "save-best-chainlock-error-silently-discarded-in-Mismatch-forced-reorg-path"
},
"severity": "suggestion",
"specialist": "Correctness & Logic",
"suggestedFix": "// Option A — log and continue (minimal change):\nif let Err(e) = self.save_best_chainlock().await {\n tracing::warn!(\"Failed to persist best chainlock at height {}: {}\", height, e);\n}\n\n// Option B — propagate (preferred, matches process_chainlock contract):\n// Change on_masternode_ready signature ",
"title": "save_best_chainlock error silently discarded in Mismatch forced-reorg path",
"judgeNotes": "Impact: Medium (on-disk floor reverts on restart after storage error), Likelihood: Unlikely (storage write failures are rare). The Match arm has the same pattern, so this is a pre-existing inconsistency; downgrading from warning since it's not specific to this PR's new code.",
"judgeConfidence": "medium",
"reachability": "reachable"
},
{
"fingerprint": {
"file": "dash-spv/src/sync/chainlock/manager.rs",
"lineStart": 151,
"lineEnd": 151,
"slug": "Mismatch-valid-sig-arm-duplicates-the-best-chainlock-state-advance-sequence"
},
"severity": "suggestion",
"specialist": "Architecture & Design",
"suggestedFix": "async fn commit_validated_chainlock(&mut self, cl: &ChainLock) {\n let height = cl.block_height;\n self.progress.update_best_validated_height(height);\n self.best_chainlock = Some(cl.clone());\n self.chainlock_height.store(height, Ordering::Release);\n self.save_best_chainlock().await;\n}\n\n",
"title": "Mismatch valid-sig arm duplicates the best_chainlock state-advance sequence",
"judgeNotes": "Impact: Low (code duplication risk of drift), Likelihood: Possible. Reasonable refactor but optional.",
"judgeConfidence": "high",
"reachability": "reachable"
}
]
},
"usage": {
"inputTokens": 36,
"outputTokens": 42731,
"totalTokens": 42767,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 1929,
"totalTokens": 1938
},
"reviewer": {
"inputTokens": 12,
"outputTokens": 34235,
"totalTokens": 34247
},
"judge": {
"inputTokens": 5,
"outputTokens": 1512,
"totalTokens": 1517
},
"dedup": {
"inputTokens": 10,
"outputTokens": 5055,
"totalTokens": 5065
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 2,
"reclassifiedPriorCount": 0
}
}Reviewed commit defc0c3
…oor advance on `Mismatch` The `Mismatch` arm of `on_masternode_ready` was advancing `best_chainlock`, `chainlock_height`, and the disk state before the forced-reorg cascade ran. If the cascade never completes the persisted chainlock floor would point to a block not on the active chain, exactly the inconsistency this PR prevents. Aligns with `process_chainlock`'s `Mismatch` arm which emits `ChainLockForcedReorg` without advancing the floor. The floor advances only on `Match`/`Unknown` paths (active chain agrees) via the extracted `commit_validated_chainlock` helper, eliminating the duplication that would otherwise allow the two paths to drift. Addresses manki-review comments on PR #166 #166 (comment) #166 (comment)
|
Manki — Review complete Planner (24s) Review — 3 findings Judge — 0 kept · 0 dropped (26s) Review metadataConfig:
Timing:
|
There was a problem hiding this comment.
All three open threads resolved by extracting commit_validated_chainlock and removing the premature state-advance from the Mismatch+valid-sig arm — the floor now only moves on Match/Unknown paths, deferring Mismatch advancement to cascade commit as intended.
Blocking unresolved review threads:
- [Blocker] Chainlock floor guard blocks forced reorg even with
force=true—dash-spv/src/sync/reorg.rs:176 - [Warning] Last-wins overwrite in
pending_unknown_hashlets a peer suppress chainlock retry —dash-spv/src/sync/chainlock/manager.rs:248 - [Warning] on_masternode_ready silently drops Mismatch chainlock instead of emitting ChainLockForcedReorg —
dash-spv/src/sync/chainlock/manager.rs:142 - [Warning] Mismatch forced-reorg path never advances best_chainlock floor —
dash-spv/src/sync/chainlock/manager.rs:154 - [Warning] best_chainlock floor persisted before reorg cascade commits —
dash-spv/src/sync/chainlock/manager.rs:151
📊 0 findings (none) · 1641 lines · 253s
Manki context
{
"meta": {
"prNumber": 166,
"commitSha": "d931f8dfdbcd59b15687495928f2e0fd67aee517",
"round": 4,
"timestamp": "2026-05-26T01:56:11.532Z",
"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": "large",
"memoryEnabled": false,
"reviewPasses": 1
},
"diff": {
"lines": 1641,
"additions": 1486,
"deletions": 155,
"filesReviewed": 11,
"fileTypes": {
".rs": 11
},
"oversizedHandled": false,
"perFile": [
{
"path": "dash-spv-ffi/src/callbacks.rs",
"additions": 9,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/manager.rs",
"additions": 14,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/network/mod.rs",
"additions": 19,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/fork_buffer.rs",
"additions": 124,
"deletions": 2,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/manager.rs",
"additions": 254,
"deletions": 24,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/block_headers/sync_manager.rs",
"additions": 31,
"deletions": 4,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/manager.rs",
"additions": 649,
"deletions": 70,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/chainlock/sync_manager.rs",
"additions": 15,
"deletions": 18,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/events.rs",
"additions": 38,
"deletions": 0,
"changeType": "modified"
},
{
"path": "dash-spv/src/sync/reorg.rs",
"additions": 224,
"deletions": 34,
"changeType": "modified"
},
{
"path": "dash-spv/tests/dashd_masternode/tests_sync.rs",
"additions": 109,
"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": "high",
"prType": "feat",
"durationMs": 23915,
"language": "rust",
"context": "Dash SPV blockchain client with ChainLock enforcement",
"agents": [
{
"name": "Security & Safety",
"effort": "high"
},
{
"name": "Correctness & Logic",
"effort": "high"
},
{
"name": "Architecture & Design",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"effort": "medium"
}
]
},
"reviewers": {
"agents": [
"Security & Safety",
"Correctness & Logic",
"Architecture & Design",
"Testing & Coverage"
],
"agentMetrics": [
{
"name": "Security & Safety",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 126938,
"status": "success",
"responseLength": 2236,
"inputTokens": 3,
"outputTokens": 7061,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Correctness & Logic",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 191575,
"status": "success",
"responseLength": 1642,
"inputTokens": 4,
"outputTokens": 8098,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "high"
},
{
"name": "Architecture & Design",
"findingsRaw": 0,
"findingsKept": 0,
"durationMs": 28575,
"status": "success",
"responseLength": 2,
"inputTokens": 3,
"outputTokens": 1489,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
},
{
"name": "Testing & Coverage",
"findingsRaw": 1,
"findingsKept": 0,
"durationMs": 36343,
"status": "success",
"responseLength": 1617,
"inputTokens": 3,
"outputTokens": 1791,
"retryCount": 0,
"model": "claude-sonnet-4-6",
"effort": "medium"
}
]
},
"judge": {
"summary": "All three open threads resolved by extracting \u0060commit_validated_chainlock\u0060 and removing the premature state-advance from the Mismatch+valid-sig arm — the floor now only moves on Match/Unknown paths, deferring Mismatch advancement to cascade commit as intended.",
"confidenceDistribution": {
"high": 0,
"medium": 0,
"low": 0
},
"severityChanges": 0,
"mergedDuplicates": 0,
"durationMs": 26350,
"retryCount": 0,
"verdictReason": "prior_unaddressed",
"threadEvaluations": [
{
"threadId": "PRRT_kwDOQSlaXs6EqHMu",
"status": "addressed",
"reason": "Inter-round diff removes the four-statement pre-cascade advance from the Mismatch+valid-sig arm (manager.rs:144-151); the arm now only computes fork_height and emits ChainLockForcedReorg, so best_chainlock/chainlock_height/disk state are no longer mutated before the cascade commits."
},
{
"threadId": "PRRT_kwDOQSlaXs6EqHM7",
"status": "addressed",
"reason": "The \u0060save_best_chainlock().await\u0060 call has been removed entirely from the Mismatch arm in the inter-round diff — no persistence happens on that path now, so the silently-discarded Err is no longer possible there. The save still occurs only inside \u0060commit_validated_chainlock\u0060 (Match/Unknown), which is the same prior behavior the thread didn't flag."
},
{
"threadId": "PRRT_kwDOQSlaXs6EqHM-",
"status": "addressed",
"reason": "Diff introduces \u0060commit_validated_chainlock\u0060 (manager.rs:175-181) and replaces the Match/Unknown inline sequence with a single call to it; the Mismatch arm no longer performs the advance at all, eliminating the duplication the thread flagged."
}
],
"verdictTrace": {
"survivingBlockers": [],
"novelWarnings": [],
"unresolvedPriors": [
{
"file": "dash-spv/src/sync/reorg.rs",
"title": "Chainlock floor guard blocks forced reorg even with \u0060force=true\u0060",
"fingerprint": "dash-spv/src/sync/reorg.rs:176:176:Chainlock-floor-guard-blocks-forced-reorg-even-with--force-true-",
"round": 1,
"line": 176,
"severity": "blocker"
},
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "Last-wins overwrite in \u0060pending_unknown_hash\u0060 lets a peer suppress chainlock retry",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:248:248:Last-wins-overwrite-in--pending-unknown-hash--lets-a-peer-suppress-chainlock-retry",
"round": 1,
"line": 248,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "on_masternode_ready silently drops Mismatch chainlock instead of emitting ChainLockForcedReorg",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:142:142:on-masternode-ready-silently-drops-Mismatch-chainlock-instead-of-emitting-ChainLockForcedReorg",
"round": 1,
"line": 142,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "Mismatch forced-reorg path never advances best_chainlock floor",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:154:154:Mismatch-forced-reorg-path-never-advances-best-chainlock-floor",
"round": 2,
"line": 154,
"severity": "warning"
},
{
"file": "dash-spv/src/sync/chainlock/manager.rs",
"title": "best_chainlock floor persisted before reorg cascade commits",
"fingerprint": "dash-spv/src/sync/chainlock/manager.rs:151:151:best-chainlock-floor-persisted-before-reorg-cascade-commits",
"round": 3,
"line": 151,
"severity": "warning"
}
]
},
"openThreadsState": "fetched",
"openThreadCount": 3,
"resolvedThreadIdCount": 14,
"interRoundDiffState": "changed",
"interRoundDiffBytes": 4254,
"interRoundDiffTruncated": false
},
"dedup": {
"staticDropped": 1,
"llmDropped": 2,
"duplicateMatches": [
{
"droppedTitle": "Mismatch forced-reorg path never commits best_chainlock after cascade",
"matchedTitle": "Mismatch forced-reorg path never advances best_chainlock floor",
"matchType": "word_overlap"
},
{
"droppedTitle": "Chainlock floor never advanced after forced reorg completes",
"matchedTitle": "Mismatch forced-reorg path never advances best_chainlock floor",
"matchType": "llm"
},
{
"droppedTitle": "Mismatch + valid-sig branch has no active unit test for its key invariant",
"matchedTitle": "No test for \u0060on_masternode_ready\u0060 Mismatch + valid BLS sig → \u0060ChainLockForcedReorg\u0060 path",
"matchType": "llm"
}
],
"durationMs": 5902
},
"memory": {
"loadStatus": "disabled"
},
"findings": {
"count": 0,
"severityCounts": {
"blocker": 0,
"warning": 0,
"suggestion": 0,
"nitpick": 0
},
"entries": []
},
"usage": {
"inputTokens": 37,
"outputTokens": 22768,
"totalTokens": 22805,
"perStage": {
"planner": {
"inputTokens": 9,
"outputTokens": 2469,
"totalTokens": 2478
},
"reviewer": {
"inputTokens": 13,
"outputTokens": 18439,
"totalTokens": 18452
},
"judge": {
"inputTokens": 5,
"outputTokens": 1340,
"totalTokens": 1345
},
"dedup": {
"inputTokens": 10,
"outputTokens": 520,
"totalTokens": 530
}
}
},
"verdict": "REQUEST_CHANGES",
"recap": {
"priorRoundCount": 3,
"reclassifiedPriorCount": 0
}
}Reviewed commit d931f8d
|
Manki — Auto-approved (all findings resolved) |
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 #141 — Phase 3b of the #137 reorg work. Treats a valid
CLSIGpointing to a different block at our chain's height as the strongest signal that we are on the wrong chain and forces a reorg to the chainlocked branch regardless of chain work. Todayverify_block_hashsilently rejects mismatches atsync/chainlock/manager.rs. After this PR, mismatches trigger the reorg cascade through the same machinery landed in #164 and #165, withMAX_REORG_DEPTHbypassed whenforce=true.handle_reorg(force=true)bypasses the depth cap (checkpoint floor, chainlock floor, single-flight, and deny-list still apply).ChainLockManager::verify_block_hashreturns a 3-way result.Unknownqueues the CLSIG in a TTL-evictedpending_unknown_hashmap for retry onBlockHeadersStored.Mismatchvalidates the BLS signature, then either reportsInvalidChainLockSignaturemisbehavior on the delivering peer or emitsChainLockForcedReorg.BlockHeadersManager::try_drive_forced_reorgfetches the chainlocked branch via a broadcastgetheadersand bypassestake_winning_candidateonce the new tip arrives.NetworkRequest::ReportMisbehaviorroutes peer scoring through the existing MPSC channel intoPeerReputationManager::update_reputation.test_chainlock_forced_reorg_end_to_endis marked#[ignore](mirrors the existing DIP-24 ignore, blocked on regtest masternode-network fixture not deterministically producing chainlocks).Closes #141.