fix(rewards): install FundedDistributorRegistry at startup; refuse malformed launcher_id - #626
Conversation
… with -32602
`dig.getRewardProverStatus`'s optional `launcher_id` filter degraded a malformed
value to a hex-string equality comparison that simply never matched, so the
response read as a legitimate empty list (`Consulted { items: [] }`) instead of
the caller-error it actually is. Reuse the same `parse_launcher_id_arg` validator
`dig.getRewardDistributor` already returns `-32602` from: absent means no filter,
present-and-malformed is refused before any filter runs, present-and-well-formed
compares the decoded `[u8; 32]` directly instead of a case-insensitive hex string.
Refs #3280
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tup path `Node::install_funded_distributor_registry` had zero non-test callers and was `pub(crate)`, so `dig-node-service` (the crate that owns the state directory) could not call it — the registry was empty on every shipped node regardless of what an operator had funded. Make the installer `pub`, mirroring `install_reward_chain_port`, and call it from `serve_with_shutdown`'s startup path against the service's own hardened state dir, unconditionally (this is local state, not a chain read, so it does not need the `enable_chain_sync` gate the reward-chain-port install below it uses). Also drops the now-stale `allow(dead_code)` on `FundedDistributorRegistry:: with_state_dir`/`read` (a real caller exists now), and fixes the `funded.rs` regression test's docstring, which overclaimed that its hand-built variant array alone catches a future enum variant — the actual guard is `FundedDistributorsRead::determined`'s wildcard-free match, which fails to compile on an unhandled variant. The registry stays empty (`NotConfigured(NoRecordWritten)`) on a fresh node until #3291's writer runs — that is success, not a bug: the node cannot observe its own funding because funding spends from a wallet it never holds. Refs #3292 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refs #3292, #3280 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS — independent correctness review at 4d6d72c096192cffe4f2466e907192519b04e953
Reviewed in a detached read-only worktree (C:/worktrees/dig-node-gate-rev), no builds run (per brief, workspace-scope cargo build/test in this repo is prohibitively slow and forbidden). Traced the real call chain by reading source, not by trusting the source-text test.
1. Does the install actually run? — YES, unconditionally.
Traced dig-node-service/src/server.rs::serve_with_shutdown:
- The only earlier early-return is
host_override_refusal(an unrelated remote-bind guard), which returns beforebuild_state— not a path that could skip the install once reached. let state = build_state(&config).await;—build_statereturnsAppStatedirectly (notResult), so there is no?or branch here that could short-circuit.- Between that line and the new
state.node.install_funded_distributor_registry(...)call there is exactly one statement,bring_up_collateral_records()(synchronous, infallible, no early return), plus two.clone()s. state.state_diris a plainPathBuffield (server.rs:79), computed synchronously insidebuild_stateviaresolve_state_dir_and_token()(line 592) beforestateis returned — neverOption, never populated lazily.- The new call is not inside the
if config.enable_chain_sync { ... }block below it (confirmed by reading the block boundaries) — it sits above that gate, so it fires even under an integration harness with sync disabled, exactly as claimed.
Confirmed the flagged test (server_startup_calls_install_funded_distributor_registry_in_production_code) is indeed a source-text/string-containment test, as the brief warned — it alone would not distinguish "called" from "unreachable." I did not rely on it; the manual trace above is the actual evidence for PASS on this point.
2. Placement/ordering — correct.
state.state_dir is fully resolved before build_state returns (see above), so it is valid and populated at the call site. Nothing between bring_up_collateral_records() and the new call reads the funded-distributor registry, and no earlier spawned task in this function reads it either (the only prior lines are the wallet clones and the collateral bring-up). No stale-read hazard.
3. Absent launcher_id stays absent (three-way split) — confirmed genuinely three-way.
dispatch.rs:923-943: params.get("launcher_id").is_some() gates whether parse_launcher_id_arg runs at all — absence takes the None branch directly, never touching the validator, so it cannot be turned into a -32602. Malformed present values return Err → -32602 (new test get_reward_prover_status_with_a_malformed_launcher_id_is_refused). Well-formed-but-unknown present values return Ok([u8;32]), filter to an empty items list, consulted (new test ..._is_empty_not_refused). The pre-existing test get_reward_prover_status_filters_by_launcher_id_when_given still exercises the true-absent case (second handle_rpc call with no params key at all) and asserts all items returned unfiltered — this still passes against the new match on [u8;32] since it's unaffected by the presence check.
4. Hex-string→[u8;32] comparison change — this is a fix, not a regression.
Old code: lowercase the raw string (no 0x strip, no length validation), then hex::encode(bytes).eq_ignore_ascii_case(want). hex::encode never emits 0x, so a caller who included a 0x prefix would silently get zero matches forever — exactly the "reassuring empty list" defect this epic targets, just on the input side. New code runs the same parse_launcher_id_arg already trusted by GetRewardDistributor/ListRewardDistributorCommitments — it strips 0x, enforces exactly 64 hex chars, and is case-insensitive via hex::decode — then compares [u8;32] directly. Net effect: a 0x-prefixed or malformed launcher_id now gets a -32602 (was: silent empty match); a correctly-cased/uncased plain-hex value behaves identically to before. No caller that worked correctly before now sees a behavior change; callers that were silently broken before now get an explicit error. Judged a fix.
5. Closed-enum read surface — preserved, not touched by this diff.
The consuming match at dispatch.rs:1148 (ListRewardDistributors) is unchanged by this PR and remains wildcard-free over all five FundedDistributorsRead variants (Funded, FundsNothing, and a combined arm for NotConfigured | PersistedStateCorrupt | IoFailed, each rendering NotConsulted, never []). Grepped the entire diff for _ => — none introduced anywhere in the 5 changed files.
6. NotConfigured(NoRecordWritten) on a fresh node — correctly treated as success, not asked to invent a writer.
The new test in funded_distributor_registry_startup_3292.rs explicitly asserts a fresh tempdir-backed registry reads NotConfigured(NoRecordWritten), and the server.rs comment states this is the correct post-install answer until #3291's writer lands. No attempt in this diff to fabricate a funded set or make the empty case read more reassuring.
Dead-code attribute removals — both genuine.
FundedDistributorRegistry::with_state_dir— now called fromserver.rs's production startup path (confirmed above).FundedDistributorRegistry::read()— reached viaNode::funded_distributors_read(), called fromdispatch.rs:1148inside the (pre-existing, unchanged)ListRewardDistributorshandler — a genuine non-test caller, not just warning suppression.
Out of scope, correctly not touched
rewards_claim/**, the dig-rpc-protocol version bump, prover-loop data production (#3265), the four Unavailable RewardsChainPort methods (#3421), writes.rs (#3422) — none of these appear in the diff.
Findings
None blocking. No inline threads opened.
Not run
No cargo build/cargo test at any scope (brief's hard constraint; rustfmt/clippy already reported green at this SHA). Verified behavior by reading source and existing/new unit-test bodies, not by executing them.
Verdict: PASS at 4d6d72c096192cffe4f2466e907192519b04e953.
|
PASS Audited head FindingsNone LIVE. Two defence-in-depth notes below (tickets, not gates).
Answers to the six prompts
Scope auditedFiles: Not coveredNo Worktree: created my own detached read-only worktree at 🤖 Generated with Claude Code |
server_startup_calls_install_funded_distributor_registry_in_production_code was a plain `.contains(...)` check, so it survives the call being MOVED (e.g. behind the enable_chain_sync gate) rather than deleted -- and it turns out it does not even reliably catch deletion, since the surrounding doc comment repeats both needles verbatim. Adds: - install_call_precedes_the_enable_chain_sync_gate_in_production_code: asserts the install call's source offset precedes the enable_chain_sync gate's, which reddens under both mutating the call away (deletion) and moving it inside that gate -- the exact shape #3292 originally shipped as. - production_startup_reaches_the_install_call_site_without_panicking: boots the real serve_with_shutdown path end to end on an ephemeral loopback port and proves the install call site is reachable without panicking, which the existing tempdir-only test cannot (it never drives server.rs at all). Documents, in the new runtime test's doc comment, why a true black-box runtime distinction between "installed and empty" and "never installed" is not reachable from this integration-test crate today: Node's registry accessors are pub(crate) (invisible outside dig-node-core), AppState's node field is private with no getter, and dig.listRewardDistributors wildcards every NotConfigured/PersistedStateCorrupt/IoFailed reason into one identical Half::NotConsulted response with no reason on the wire -- so the two states are genuinely indistinguishable from any observer available here without a production-surface change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Delta re-gate at 2fc2ce7d0ff705025ec1c4ea6303ec10352656be
PASS. Scope confirmed independently: git diff --stat 4d6d72c0 2fc2ce7d = one file,
crates/dig-node-service/tests/funded_distributor_registry_startup_3292.rs, +164/-0.
git diff --name-only filtered for non-test paths returns nothing. No production file moved.
The prior correctness/security PASS at 4d6d72c0 stands unchanged; this review covers only the
delta.
Is the new test self-satisfying the way the old one was? No — confirmed by direct read of src/server.rs.
The old test's defect was a bare-identifier needle ("install_funded_distributor_registry",
no trailing paren) plus "state.state_dir". Both strings are repeated verbatim in the doc comment
directly above the real call site (src/server.rs:2230-2240, e.g. `install_funded_distributor_registry`'s `OnceLock` means... and (`state.state_dir`, resolved above)). That comment survives even if the call statement itself is deleted, so the old test cannot redden under mutation (a) or (b). I verified this by reading the comment text myself, not just trusting the report — it's real, and it fully explains why the old test is inert rather than merely weak.
The two new tests use different needles:
install_funded_distributor_registry(— with the trailing paren, i.e. only matches a call
expression, never the bare-identifier prose in the comment.grep -nover the whole file
(production region and test region) shows exactly one occurrence in the entire file:
server.rs:2242, the real call. Nowhere else, including comments, does this exact substring
appear.if config.enable_chain_sync {— the fullifstatement text, not just the flag name.grep -c
shows exactly two occurrences in the whole file, both realifstatements (server.rs:2259
and:2330); every other mention ofenable_chain_syncin comments (lines 181, 578-579, 2233,
2256, 2271-2272, 2327, 2362, 2375) uses different surrounding text and does not match this literal
string. So this needle cannot be satisfied by prose either.
What convinced me: the source read, not the mutation numbers alone — a green mutation result
is a claim, and the old test's own green history under real regressions is exactly why I traced it
by hand rather than accept it at face value last time. Here I found the actual, unique location of
both needles and confirmed no doc/comment text anywhere in the file duplicates them. The reported
mutation exit codes (101 red under both deletions, 0 green on revert) are consistent with, and now
explained by, that mechanism — a second, independent line of evidence, not the only one. I did not
re-run the mutation myself: a scoped -p dig-node-service test in a fresh worktree still triggers a
full transitive dependency rebuild (confirmed — my own attempt hit >300s and was still compiling
dig-node-core/dig-wallet when I stopped it), so I relied on static confirmation of the mechanism
instead of re-executing a build that could run long. I did not treat my own killed build as a test
result — it isn't one, it's an incomplete compile I terminated.
1. production_startup_reaches_the_install_call_site_without_panicking — name vs. proof
Accurate, not overstated. It boots the real serve_with_shutdown (the same fn dig-node run
calls) on an ephemeral loopback port and polls /health until serving, then asserts clean
shutdown. Its own doc comment (lines 133-162) is unusually careful about the limit: it explicitly
says it can only prove "reachable, does not panic," and names exactly why "installed" can't also be
asserted here (no accessor crosses the pub(crate) boundary from this integration-test
compilation unit, and the RPC surface wildcards both NotConfigured reasons into one identical
Half::NotConsulted — the pre-existing #3324 gap, correctly left alone here). The name says
"reaches ... without panicking," which is exactly what it checks — no gap between name and proof.
2. Flakiness / secrets / hardcoded paths — clean
No hardcoded absolute paths (uses tempfile::tempdir()/tempfile::Builder), no secrets, no
machine-specific assumptions. The ephemeral-port bind-then-drop-then-rebind pattern has a narrow
theoretical TOCTOU race (something else grabs the port between drop and serve_with_shutdown's
bind) but mirrors an existing pattern already in tests/server.rs per the test's own comment, so
it's a pre-existing, accepted risk shape in this crate, not new exposure introduced here. The
ENV_LOCK mutex around the process-global DIG_NODE_CACHE/DIG_NODE_STATE_DIR/DIG_PEER_NETWORK
env vars correctly serializes against every other test in the same binary that touches them.
3. Should server_startup_calls_install_funded_distributor_registry_in_production_code be deleted?
Agree with your inclination — delete it. It is not merely redundant with the new tests, it is
actively worse than absent: it reads as coverage for exactly the property (production call site is
wired) that it cannot detect a regression in, for a reason (doc-comment self-satisfaction) that is
now proven, not suspected. Leaving it beside the two new tests invites a future reader to trust
three tests where only two hold weight, and a future refactor of the comment above the call site
could accidentally make it fail (or, worse, someone "fixes" it by rewording the comment, which does
nothing for the real property). This PR's delta doesn't touch it, so it's outside this delta's
strict scope, but it is not a new problem — it's the same PR, same file. Your call on whether to ask
for a same-PR follow-up push or file it as a fast-follow; either way I'd remove it before merge
rather than after.
Not run
Did not re-execute cargo test -p dig-node-service --test funded_distributor_registry_startup_3292
to completion — a fresh worktree triggers a full transitive rebuild that exceeded the time I was
willing to hold open; verified the mechanism by direct source read instead (see above), which is
sufficient to answer the one question this gate turns on.
Verdict: PASS at 2fc2ce7d0ff705025ec1c4ea6303ec10352656be.
|
PASS Head audited: Scoped delta re-gate over PR #626 since my prior PASS at
Context from the prior full audit stands unchanged (not re-derived): the Scope audited: |
…ource-text test server_startup_calls_install_funded_distributor_registry_in_production_code asserted on bare identifiers that its own doc comment repeated verbatim, so it passed both when the install call was deleted and when it was moved inside the enable_chain_sync gate. A test that passes under the exact mutation it exists to catch is worse than no test, because it reads as coverage. Refs #3292, #3280 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS at 64f6296
Third delta re-gate, scoped confirmation only.
- Production diff since
4d6d72c0is genuinely empty.git diff --stat 4d6d72c0 64f62961 -- ':!*/tests/*'returns nothing. The full correctness review at4d6d72c0still stands unchanged. - Only the inert test was deleted.
git diff --stat 2fc2ce7d 64f62961= one file, +3/-24. The three tests that must remain do remain:install_funded_distributor_registry_installs_a_tempdir_backed_registry_once,install_call_precedes_the_enable_chain_sync_gate_in_production_code,production_startup_reaches_the_install_call_site_without_panicking. Onlyserver_startup_calls_install_funded_distributor_registry_in_production_code(the proven-inert source-text.contains(...)test) was removed, exactly as recommended in the prior review. Both distinctive needles survive byte-intact:install_funded_distributor_registry(with trailing paren (line 81) and the full statementif config.enable_chain_sync {(line 88), both inside the retainedinstall_call_precedes_the_enable_chain_sync_gate_in_production_codetest. - The +3 insertion is formatting only. One
dig_local: false,comment-alignment fix (extra space removed) and one long.expect(...)call rewrapped across two lines by rustfmt. No new logic, no weakened assertion.
cargo fmt --all -- --check reported to exit 0; not independently re-run per the brief's hard constraint against triggering a cold rebuild — diff review is sufficient to answer all three questions.
Not this PR's problem, already filed: #3324.
Head verified via gh pr view 626 at time of review = 64f6296154fb61fc138e57ffc2b65656ead7cbd9, matching the brief.
|
PASS
Scope audited: |
What was broken
FundedDistributorRegistrywas never installed on any production node.Node::install_funded_distributor_registrywas declaredpub(crate)indig-node-core, sodig-node-service— the crate that owns startup — could not call it even in principle. It had no non-test caller anywhere in the tree.The registry is how a node knows which reward distributors it funds. With the slot empty,
dig.listRewardDistributorsreportsNotConfiguredforever, discovery never runs, and the code reads as finished the whole time. Nothing errors. Rewards simply are not distributed.That is the epic's acceptance line made literal: "anytime the process isn't running then rewards are not being distributed." The process was not running, in every shipped binary, and no test or lint could see it — a visibility modifier is invisible to rustdoc, to clippy, and to CI.
What this changes
#3292 — install the registry on the real startup path
crates/dig-node-core/src/lib.rs:652—install_funded_distributor_registryis nowpub, mirroring the already-pubinstall_reward_chain_port.crates/dig-node-service/src/server.rs— installed insideserve_with_shutdown, immediately afterbring_up_collateral_records(), unconditionally. It is deliberately not gated onenable_chain_sync: the registry is local state with no chain dependency, and gating it there would reintroduce the same silent-inert failure under a different name.crates/dig-node-core/src/rewards/funded.rs— corrected a docstring that overclaimed which mechanism guarantees future-variant coverage (it isdetermined()'s wildcard-free match, not the hand-built test array), and dropped two now-staleallow(dead_code)attributes.On a fresh node the registry now reads
NotConfigured(NoRecordWritten). That is the correct answer, not a bug. The registry's writer is #3291, an operator declaration — the node cannot observe its own funding, because funding spends from a user wallet the node never holds. A surface that says what it does not know is the goal here.#3280 — refuse a malformed
launcher_idinstead of silently returning nothingcrates/dig-node-core/src/seams/dig_rpc/dispatch.rs:923-985—GetRewardProverStatusnow reuses the existingparse_launcher_id_arghelper (already used byGetRewardDistributor): absent means no filter, malformed means-32602, well-formed compares[u8;32]rather than hex strings.The hex-to-bytes change is a fix rather than a regression: the previous string comparison silently failed to match any
0x-prefixed input forever, where the new path refuses it by name.Why the wiring has a regression test
A wiring fix with no regression test is one refactor away from reverting silently, and this particular breakage is invisible when it happens. The tests here are written to fail if the registry is ever un-installed — including if the call is not deleted but merely moved somewhere it never executes.
Gates
Correctness and security both PASS at
4d6d72c0, zero unresolved threads. The correctness gate did not accept the source-text test as proof the install runs; it traced the call chain throughserve_with_shutdownby hand and confirmed no early return, no?, and no branch can skip it.Refs #3292, #3280, #3246
🤖 Generated with Claude Code