fix(rewards): split the three conditions behind REWARD_CHAIN_UNAVAILABLE - #618
Conversation
Refs #3342 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an absence `build_report` maps `read_distributor_guarded`'s `Ok(None)` -- the chain answering "no distributor at this launcher" -- onto the same `ChainPortError::Unavailable` an unreachable chain produces. A funder deciding whether to claw back cannot tell "you have nothing there" from "we cannot see the chain", and one wire shape for both makes that call a guess on a money surface. This test is RED at this commit, deliberately: it is the acceptance evidence for the split that follows, written before the fix so the fix has something to turn green. Refs #3342 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ChainPortError::Unavailable` stood for three conditions a caller must tell apart: 1. no chain-read adapter installed on this Node at all; 2. an installed adapter could not reach the chain -- a real outage; 3. the chain ANSWERED and holds no distributor at that launcher id (`read_distributor_guarded` -> `Ok(None)`). Case 3 is the money-surface defect: an absence rendered as an outage. A funder deciding whether to claw back cannot distinguish "you have nothing there" from "we cannot see the chain", and one wire shape for both makes that call a guess. `Ok(None)` now gets its own `ChainPortError::NotADistributor` and its own machine code, so the two can never share a shape again. The message "no chain-read adapter is wired yet" was also false at the site that emitted it -- on a default install an adapter IS wired and IS answering. It is not reworded; it is confined to `reward_chain_port_absent_response`, the one case where it is true, and the `Unavailable` arm now names its real cause. A diagnostic that misnames its own cause sends the next reader to the wrong subsystem. Turns green the red test added in the previous commit. Refs #3342 Refs #3246 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial leg (loop-decider) — CHANGES-REQUIRED at
|
|
IN PROGRESS — not the verdict Traced reachability of
Net: the "CONTROL plane: loopback admin / in-process FFI ONLY" comment this PR adds to |
|
CHANGES-REQUIRED Finding 1 (LIVE) — the split turns a pre-existing reachability gap into a real enumeration oracle
What is actually true, traced end to end:
Exploit: any co-resident process, or any web page that DNS-rebinds to the node's loopback port This is LIVE, not defense-in-depth: the underlying HTTP-surface gap is pre-existing and out of this Answers to the five questions
Scope auditedFiles: crates/dig-node-core/src/rewards/port.rs, crates/dig-node-core/src/seams/dig_rpc/dispatch.rs, Not covered
Shared checkoutUsed |
Orchestrator decision on the three gate verdicts — with reasons, not just rulingsNot merging at H1 — wire distinction unpinned: ACCEPTED, blockingThe gate produced a two-line edit that re-collapses the split while all 3431 tests stay green. That is decisive: the guard is forgeable, so it does not hold. The port-level test is also too loose — H2 — the absence arms the degradation latch: ACCEPTED, blocking, and the most valuable finding here
Security finding — reachability gap: REAL, but DOWNGRADED as an exploit, and filed separatelyThe gate is right on the mechanism and I am not disputing it: the reward read methods are documented "loopback admin / in-process FFI ONLY" at Where I differ is on calling it an enumeration oracle created by this PR:
So the exploit delta from this PR is disclosure of public data that was already free to obtain. That is not a merge blocker. What is genuinely wrong is the gap between the documented tier and the enforced one — a method whose doc says "loopback admin ONLY" while answering anonymously is a false statement in the code about the code, and the next reader will build on it. Same class as the defect this PR fixes, which is why it deserves its own ticket rather than being absorbed here. Filed at H3 — PR body stale: already fixedThe body was rewritten before this verdict landed and now states the defect, the mutation proof table, and what the PR does not fix. The gate is right that "deleted, not reworded" was imprecise — the sentence is relocated to Q2, carried forward
Re-gate will run on the new head. A verdict on a replaced head covers nothing. |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Verdict: CHANGES-REQUIRED
Head: 60ba02f718bd2aa6faae5c049b40c3e352c73f5c
What's solid
- Q2 (call sites): all three
let Some(port) = node.reward_chain_port() else { .. }arms (GetRewardDistributor,ListRewardDistributorCommitments,ListRewardDistributors) now route throughreward_chain_port_absent_response, not theUnavailablearm ofreward_chain_port_error_response. Verified by grep againstcrates/dig-node-core/src/seams/dig_rpc/dispatch.rslines 1013, 1055, 1141. - Q3 (single "wired" site):
grep -rn "adapter is wired" crates/returns exactly the doc comment (line 111) and the message string (line 119) insidereward_chain_port_absent_response— one function, one site. - Q4/Q5 (no weakened assertions): diffed
chain_port.rs— the two neighbouring tests (parent_spend_gap_is_reported_as_unavailable_not_as_a_distributor_identity_verdict,a_failing_chain_source_reports_a_named_unavailable_never_an_ok_default) still assertUnavailable; only their doc comments changed, and correctly — the old doc justifiedParentSpendUnavailable => Unavailableby appeal toOk(None)'s (now-defunct) sharing of that shape; the new doc justifies it on its own merit (a chain source that never reached the parent spend can't say anything about identity at all). Nocontains/assertion was widened or deleted — confirmed no-lines in the diff touch anyassert/assert_eq/assert_neline. - Q6 (no wire crate touched):
dig-rpc-protocolis a separate repo/submodule, not present at all in this diff or indig-node's own tree; the new machine codes are local&strconsts indispatch.rs, exactly as designed. - Port-level mutation proof (already measured, reconfirmed by the doc/diff read):
an_answering_chain_with_no_distributor_is_an_absence_not_an_outageis a real red/green pair acrossa0740145→60ba02f7.
The gap — Q1, unanswered by any test
Nothing pins the WIRE-level distinction. dig-app (and any RPC caller) reads data.code on the JSON-RPC error body, not the Rust ChainPortError enum. I checked every test that exercises reward_chain_port_error_response/the dispatch handlers for the two reward-distributor methods:
reward_distributor_methods_chain_unavailable_is_a_distinct_error_never_a_zero_or_empty(crates/dig-node-core/src/lib.rs:10324) only callstest_node(None)— no port installed — so it exercisesreward_chain_port_absent_response(the "no adapter" case), not an installed port answeringErr(ChainPortError::NotADistributor). It assertsREWARD_CHAIN_UNAVAILABLE!=REWARD_INVALID_WITHDRAWAL_SHARE, but never compares againstREWARD_NOT_A_DISTRIBUTOR.grep -rn "NotADistributor" crates/dig-node-core/src/lib.rsreturns zero hits. No test in this file installs aFakeRewardsChainPortreturningErr(ChainPortError::NotADistributor)and checks the resultingdata.code.- I mechanically confirmed the hole: collapsing
reward_chain_port_error_response'sNotADistributorarm'sdata.codeontoREWARD_CHAIN_UNAVAILABLE_MACHINE(same string as theUnavailablearm) is a one-line, exhaustive-match-safe change — it compiles — and no test in the repo's dispatch/lib-level suite would go red for it. The only test that would catch a REGRESSION at the port level (an_answering_chain_with_no_distributor_is_an_absence_not_an_outage) checksChainPortError::Unavailabledirectly, not the wire shape, so it stays green even if the wire collapses the twodata.codes back together.
This means the actual deliverable this PR claims — "a funder can tell 'nothing there' from 'chain unreachable' over the wire" — is unproven. The port-level split is real and tested; the wire-level split (the only thing dig-app or any external caller can see) rides on doc comments and code review, not a test.
Ask: add a dispatch-level test — install a FakeRewardsChainPort returning Err(ChainPortError::NotADistributor) for a dig.getRewardDistributor/dig.listRewardDistributorCommitments call, assert resp["error"]["data"]["code"] == "REWARD_NOT_A_DISTRIBUTOR", and assert it != the REWARD_CHAIN_UNAVAILABLE value from the neighbouring Unavailable-port test. The fix must not just add a Unavailable-vs-NotADistributor equality check inside a Rust match (that's the port-level test that already exists) — it must round-trip through handle_rpc/JSON exactly like reward_distributor_methods_chain_unavailable_is_a_distinct_error_never_a_zero_or_empty does, because that's the only thing a caller ever observes.
Everything else in this diff — the split itself, the absent-adapter response, the doc corrections — is correct and I have no other blocking finding.
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Inline finding on the wire-level test gap (Q1) -- see thread.
… latch Two pre-merge gate findings, both proven by mutation. H1 -- the wire-level distinction was unpinned. No test asserted any REWARD_* string on a JSON-RPC response body, so pointing the `NotADistributor` arm's `data.code` at `REWARD_CHAIN_UNAVAILABLE_MACHINE` re-collapsed the split, compiled clean (the match stays exhaustive) and left all 3431 tests green. The port-level test was also too loose: `assert_ne!(.., Unavailable)` passes if the variant is later re-mapped to `Other`. Now a dispatch test installs a port answering `Err(NotADistributor)` and asserts `data.code` by value for both methods, and the port test asserts the variant by name. H2 -- an absence armed the degradation latch. Every `Err`, including `NotADistributor`, set the adapter-wide `report_degraded` latch and warned that reads "stay refused until the chain source recovers". The warn fires only on a false->true transition, so one probe for a launcher id that simply is not a distributor silenced the warning for the next GENUINE outage. That is worse than the defect this branch set out to fix: it trades a misleading error for missing telemetry on a money surface. `NotADistributor` is now excluded from both the latch and the warn; `Ok` still clears it, matching a real recovery. Refs #3342 Refs #3246 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed and proven at
The exact re-collapse you identified was executed as a mutation and the test went RED: Reverted, GREEN again ( The port-level test was also tightened per your finding, from Resolving: the defect this thread names is fixed, and the fix is pinned by a test proven to fail without it. |
The defect: one wire shape stood for three conditions
ChainPortError::Unavailable— machine codeREWARD_CHAIN_UNAVAILABLE, numeric-32032— was returned for all three of:Nodeat all. The only case where "no chain-read adapter is wired yet" is true.read_distributor_guardedreturningOk(None), atchain_port.rsbuild_report.Case 3 is the money defect.
dig.getRewardDistributoranddig.listRewardDistributorCommitmentsare what a funder reads before deciding whether to claw back. With one shape for cases 2 and 3, that funder cannot tell "you have nothing there" from "we cannot see the chain" — so the clawback decision becomes a guess. This is the same shape asClaimOutcomenaming six ways a peer was legitimately unpaid and none to say something broke.And the message named a cause that was false at the site that emitted it: on a default install (
enable_chain_syncdefaults true) an adapter is installed and is answering. The traced path isdispatch.rs→node.reward_chain_port()(a populatedOnceLock,dig-node-core/src/lib.rs:608, installed atserver.rs:2159) →RealRewardsChainPort::distributor_report→build_report. A diagnostic that misnames its own cause sends the next reader to the wrong subsystem.The remedy
Ok(None)gets its ownChainPortError::NotADistributorand its own machine codeREWARD_NOT_A_DISTRIBUTOR, so absence and outage can never share a wire shape again.reward_chain_port_absent_response, the one site where it is true.grep -rn "adapter is wired" crates/returns exactly one hit. Reusing theUnavailablearm for the unset-OnceLockcase would merely have relocated the false message.dig-rpc-protocol: these machine codes are local&strconsts by design, matchingREWARD_INVALID_WITHDRAWAL_SHAREandREWARD_ZERO_IDENTITYbeside them. An org-wide code search finds no consumer ofREWARD_CHAIN_UNAVAILABLEoutside this repo.Mutation proof — the guard is pinned, not merely asserted
Reading proves a guard correct; only a revert going red proves it held. The two commits on this branch are that revert, in TDD order:
build_reportarman_answering_chain_with_no_distributor_is_an_absence_not_an_outagea0740145.ok_or(ChainPortError::Unavailable)?assertion left != right failed: ... got Err(Unavailable)60ba02f7.ok_or(ChainPortError::NotADistributor)?Suite size is 3431 at both heads — no test was deleted or relaxed to reach green.
What this does NOT fix
The epic's exit condition needs two more things, neither in scope here:
rewards_claim::port::ClaimChainPort) whose only production adapter isUnavailableClaimChainPort, so every claim cycle reportsChainSourceUnavailable. Sequenced behind #3336 / PR fix(rewards-claim): derive both cadences from one raw config value and inject the clock #617, which ownsdriver.rs.dig-account#58— no distributor has ever been minted on mainnet, minting having been blocked on the money-path signing seam. The read path was never broken. This PR makes the answer legible; that one makes there be something to answer about.Refs #3342
Refs #3246
🤖 Generated with Claude Code