fix(rewards-claim): chain_port audit, approval refusal, discovery cap - #623
Conversation
- #3357: ban RealClaimChainPort's/adapters' calls to
RewardDistributor::created_slot_value_to_slot from production code via a
workspace clippy.toml disallowed-methods lint (phantom LineageProof risk on
a chain-rebuilt distributor); allow the one legitimate in-process
test-fixture use in rewards_fixture.rs with a justifying comment.
- #3362: submit_initiate_payout refuses, by name, a distributor whose
require_payout_approval is true, before building or broadcasting anything
-- this adapter drops initiate_payout's returned conditions unconditionally,
so silently proceeding would violate the approval requirement. Regression
test launches a real simulator fixture with the flag curried true.
- #3358: discover_distributors now bounds hinted launcher candidates to
MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE (256) and reports every drop via
the new Discovery{distributors, candidates_dropped} return type, threaded
through ClaimStatus.discovery_candidates_dropped_this_cycle and a warn! log
when nonzero -- a silent cap on discovery is a censorship primitive.
- #3363: a_failing_source_reports_unavailable_everywhere now uses a
non-empty launcher index so the assertion exercises the failing source's
discover/submit paths instead of an already-empty-list shortcut.
Refs DIG-Network/dig_ecosystem#3357 #3362 #3358 #3363
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- a_capped_cycle_drops_the_candidate_past_the_cap_and_reports_it and a_cap_that_covers_every_candidate_drops_nothing exercise RealClaimChainPort::with_candidate_cap's drop-and-report behaviour without decoding hundreds of candidates. - cargo fmt across the touched rewards_claim files. - CHANGELOG.md: Unreleased / "Reward claim port hardening" entry for #3357, #3362, #3358, #3363. Refs DIG-Network/dig_ecosystem#3358 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MichaelTaylor3d
changed the title
loop-security verdict: PASSHead audited: S1 - Censorship/starvation via the discovery cap (#3358)
S2 - Refuse-vs-skip on require_payout_approval (#3362)
S3 - Phantom slot lint (#3357)
S4 - Error taxonomy
S5 - Bounded strings/logs
S6 - Fee/spend regressions
Mutation proofs (M1-M5) vs. diffAll five pasted red lines name file:line locations and test/lint names that are present in this diff ( CI (required contexts, by name)
FindingsNone LIVE. No exploit path found that survives the guards described above; the one disclosed gap (S1, attacker-influenceable candidate order under the 256 cap) is named in the diff's own doc as out of scope rather than concealed, and does not regress behavior beyond the pre-existing unbounded-hint-path exposure. Scope auditedFull diff of PR #623 at Not coveredDid not run build/test/clippy myself (per brief, another leg owns the targeted test; KG: NONE (no new failure shape; findings match existing memory patterns on cap disclosure, refuse-vs-skip, and enumeration-only lints). |
loop-decider (adversarial third leg) — CHANGES-REQUIRED (CI-only) @
|
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Verdict: CHANGES-REQUIRED
Head SHA: 836a10d0beea30c4f5c8907285a997961e5590f1
Required-context state (read by NAME, not by "green"):
- Lint commit messages: FAIL — required, currently red. The latest completed run against this exact head SHA (started 2026-09-24T18:37:07Z) still lints the PR-title-derived commit subject as
harden(rewards-claim): ... (#623)and failstype-enum(hardenis not in[feat, fix, docs, ...]), even though the PR's live title is nowfix(rewards-claim): chain_port audit, approval refusal, discovery cap. A subsequent run at 18:37:04 showscancelled. No run has completed green against the current title. This is a real blocker, not a stale-check artifact — GitHub has not re-evaluated the title-lint job since the retitle; re-run the "Lint commit messages" workflow (or push any commit) to get a fresh evaluation of the current title before this can pass. - Test + coverage: UNRUN (
in_progressas of this review) — not evidence either way; do not merge on the assumption it will pass. - Rustfmt / Clippy / Release-script tests: pass.
Given a required context is currently FAILING and another is still pending, this cannot be PASS regardless of the correctness findings below.
Correctness review — the six named questions
1. #3362 (payout-approval refusal) — YES, correctly placed. In chain_port.rs::submit_initiate_payout, the require_payout_approval check reads snapshot.distributor().info.constants.require_payout_approval immediately after the guarded snapshot read and strictly before let mut ctx = SpendContext::new() / initiate_payout(...). It returns Err(ClaimPortError::Other(bounded(...))) naming require_payout_approval in the message text — never Ok(())/Ok(None)/a skip. The regression test a_distributor_requiring_payout_approval_is_refused_by_name_before_any_broadcast launches a REAL simulator fixture via the new launch_funded_admitted_fixture_with_approval(payout_puzzle_hash, true) (curried, not a struct literal), asserts the named Other error contains "require_payout_approval", AND asserts broadcaster.sent.len() == 0. The module doc in chain_port.rs was updated to name this guard ("When the chain-curried require_payout_approval is true instead... REFUSES by name..."). own_entry is untouched — confirmed no diff hunk touches it; only a new test calls it (read-only, unaffected by the refusal).
2. #3358 (bounded discovery) — YES. MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE = 256 is the literal. Discovery { distributors, candidates_dropped } is returned from discover_distributors. candidates_dropped = total.saturating_sub(candidate_cap) is computed from candidate_ids.len() before .into_iter().take(candidate_cap) — confirmed order in the diff (chain_port.rs lines ~182-187). ClaimStatus.discovery_candidates_dropped_this_cycle is reset to 0 at the top of every run_cycle (alongside the other per-cycle counters) and then set unconditionally every cycle from discovered.candidates_dropped (engine.rs ~line 497) — not a latch. tracing::warn! fires when > 0, with dropped and cap fields. The doc block on the constant carries an explicit 5-point "does NOT cover" list (gossip hints via resolve_launch_comment's separate unbounded path, ordering/adversarial-influence over which candidates survive, no persisted rejected-cache, decode-cost is a different bound's job, authenticates nothing). with_candidate_cap is pub but confirmed (via grep/GitHub search across the repo at this SHA) called only from the new tests — production wiring (driver.rs:328, RealClaimChainPort::new(...)) uses the production constant; a cap of 0 cannot currently reach production. Every fake/test port (driver.rs EmptyPort/OneDistributorPort, engine.rs FakeChainPort/HintOnlyPort, port.rs UnavailableClaimChainPort returns Err so N/A) sets candidates_dropped: 0 or uses Discovery::default() — confirmed, nothing else in those fakes changed.
3. #3357 (clippy ban) — YES, and I verified the M3 line number is consistent rather than fabricated: the committed file has the disallowed call at rewards_fixture.rs:654 (with the #[allow(clippy::disallowed_methods)] line at 648); M3's pasted error names :653, which is exactly what you get when the #[allow] line is removed for the mutation (every line below shifts up by one) — so the paste is consistent with an actually-executed mutation against this file, not a fabricated line number. clippy.toml's path (chia_sdk_driver::RewardDistributor::created_slot_value_to_slot) matches the call site. The #[allow] is scoped to the single let reward_slots = ... statement (smallest item), with a comment stating the receiver is distributor.pending_spend.created_reward_slots — this fixture's own in-process value this same generation, not chain-rebuilt. Both the clippy.toml reason string and the module comment plainly state the lint bans the CALL, not the receiver class, and cannot distinguish provenance. The pinned string adapter_source_never_calls_created_slot_value_to_slot — not present in this diff at all (grepped); if that's an existing pinned test elsewhere in the repo it is untouched by this PR, which is correct, but flag: the brief asked to confirm it's untouched — I could not find it in-repo by that literal name via GitHub search; if it lives under a different literal, this needs the L1's own knowledge of where it is, since a grep found nothing.
4. #3363 (empty-index shortcut) — YES. a_failing_source_reports_unavailable_everywhere now builds over FixtureLauncherIndex(vec![launcher_id]) and asserts both discover_distributors().await == Err(ClaimPortError::Unavailable) and submit_initiate_payout(...).await == Err(ClaimPortError::Unavailable), in addition to the pre-existing reserve_asset_id/others assertions below it.
5. Regressions — Checked and clean: the early ClaimLoopState::CadenceNotElapsed => return Vec::new() sits in its original position relative to the fee-window roll (unmoved — confirmed by reading the committed file, not just the diff). ChainSourceUnavailable/Other handling is structurally unchanged (only the placeholder swapped from Vec::new() to Discovery::default()); no fallback to UnavailableClaimChainPort introduced. No Ok(None) is mapped to Unavailable anywhere in the diff. cycles_driven() is not referenced anywhere in this diff. CHANGELOG entries match the diff's actual behavior — no claim beyond what's implemented.
6. Readability — Names read correctly (MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE, candidates_dropped, with_candidate_cap, launch_fixture_with_approval). No dead code found. grep -i MUTATION over the full diff returns nothing — no leftover mutation-probe comments.
Findings
None blocking on correctness. One non-blocking note (posted inline, will resolve once acknowledged): the adapter_source_never_calls_created_slot_value_to_slot pin named in the brief could not be located by that literal string via repo search from this diff alone — worth the implementer confirming its location so a future audit doesn't lose it.
What I did not run
No build, no test — per brief, judged from the diff at 836a10d0 plus CI-by-name only. Coverage percentage not verified (Test + coverage is still in_progress).
Blocking reason for CHANGES-REQUIRED: the "Lint commit messages" required context is FAILING at this head SHA and "Test + coverage" has not yet attached. Re-run/verify both before re-requesting review; correctness itself is sound pending that.
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Verdict: CHANGES-REQUIRED
Head SHA: 836a10d
Required-context state (read by NAME, not by "green")
- Lint commit messages: FAIL — required, currently red. The latest completed run against this exact head SHA (started 2026-09-24T18:37:07Z) still lints the PR-title-derived commit subject as "harden(rewards-claim): ... (#623)" and fails type-enum (harden is not in [feat, fix, docs, ...]), even though the PR's live title is now "fix(rewards-claim): chain_port audit, approval refusal, discovery cap". A subsequent run at 18:37:04 shows cancelled. No run has completed green against the current title. This is a real blocker, not a stale-check artifact — GitHub has not re-evaluated the title-lint job since the retitle; re-run the "Lint commit messages" workflow (or push any commit) to get a fresh evaluation of the current title before this can pass.
- Test + coverage: UNRUN (in_progress as of this review) — not evidence either way; do not merge on the assumption it will pass.
- Rustfmt / Clippy / Release-script tests: pass.
Given a required context is currently FAILING and another is still pending, this cannot be PASS regardless of the correctness findings below.
Correctness review — the six named questions
1. #3362 (payout-approval refusal) — YES, correctly placed. In chain_port.rs::submit_initiate_payout, the require_payout_approval check reads snapshot.distributor().info.constants.require_payout_approval immediately after the guarded snapshot read and strictly before let mut ctx = SpendContext::new() / initiate_payout(...). It returns Err(ClaimPortError::Other(bounded(...))) naming require_payout_approval in the message text — never Ok(())/Ok(None)/a skip. The regression test a_distributor_requiring_payout_approval_is_refused_by_name_before_any_broadcast launches a REAL simulator fixture via the new launch_funded_admitted_fixture_with_approval(payout_puzzle_hash, true) (curried, not a struct literal), asserts the named Other error contains "require_payout_approval", AND asserts broadcaster.sent.len() == 0. The module doc in chain_port.rs was updated to name this guard ("When the chain-curried require_payout_approval is true instead... REFUSES by name..."). own_entry is untouched — confirmed no diff hunk touches it; only a new test calls it (read-only, unaffected by the refusal).
2. #3358 (bounded discovery) — YES. MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE = 256 is the literal. Discovery { distributors, candidates_dropped } is returned from discover_distributors. candidates_dropped = total.saturating_sub(candidate_cap) is computed from candidate_ids.len() before .into_iter().take(candidate_cap) — confirmed order in the diff (chain_port.rs ~lines 182-187). ClaimStatus.discovery_candidates_dropped_this_cycle is reset to 0 at the top of every run_cycle (alongside the other per-cycle counters) and then set unconditionally every cycle from discovered.candidates_dropped (engine.rs ~line 497) — not a latch. tracing::warn! fires when > 0, with dropped and cap fields. The doc block on the constant carries an explicit 5-point "does NOT cover" list (gossip hints via resolve_launch_comment's separate unbounded path, ordering/adversarial-influence over which candidates survive, no persisted rejected-cache, decode-cost is a different bound's job, authenticates nothing). with_candidate_cap is pub but confirmed (grep + GitHub search across the repo at this SHA) called only from the new tests — production wiring (driver.rs:328, RealClaimChainPort::new(...)) uses the production constant; a cap of 0 cannot currently reach production. Every fake/test port (driver.rs EmptyPort/OneDistributorPort, engine.rs FakeChainPort/HintOnlyPort, port.rs UnavailableClaimChainPort returns Err so N/A) sets candidates_dropped: 0 or uses Discovery::default() — confirmed, nothing else in those fakes changed.
3. #3357 (clippy ban) — YES, and the M3 line number is consistent rather than fabricated: the committed file has the disallowed call at rewards_fixture.rs:654 (with the #[allow(clippy::disallowed_methods)] line at 648); M3's pasted error names :653, which is exactly what you get when the #[allow] line is removed for the mutation (every line below shifts up by one) — so the paste is consistent with an actually-executed mutation against this file, not a fabricated line number. clippy.toml's path (chia_sdk_driver::RewardDistributor::created_slot_value_to_slot) matches the call site. The #[allow] is scoped to the single let reward_slots = ... statement (smallest item), with a comment stating the receiver is distributor.pending_spend.created_reward_slots — this fixture's own in-process value this same generation, not chain-rebuilt. Both the clippy.toml reason string and the module comment plainly state the lint bans the CALL, not the receiver class, and cannot distinguish provenance. NOTE: I could not locate the pinned string "adapter_source_never_calls_created_slot_value_to_slot" anywhere in this diff or via repo search at this SHA — non-blocking, but the implementer should confirm where that pin lives so a future audit doesn't lose it.
4. #3363 (empty-index shortcut) — YES. a_failing_source_reports_unavailable_everywhere now builds over FixtureLauncherIndex(vec![launcher_id]) and asserts both discover_distributors().await == Err(ClaimPortError::Unavailable) and submit_initiate_payout(...).await == Err(ClaimPortError::Unavailable), in addition to the pre-existing reserve_asset_id/others assertions below it.
5. Regressions — checked and clean: the early ClaimLoopState::CadenceNotElapsed => return Vec::new() sits in its original position relative to the fee-window roll (unmoved — confirmed by reading the committed file, not just the diff). ChainSourceUnavailable/Other handling is structurally unchanged (only the placeholder swapped from Vec::new() to Discovery::default()); no fallback to UnavailableClaimChainPort introduced. No Ok(None) is mapped to Unavailable anywhere in the diff. cycles_driven() is not referenced anywhere in this diff. CHANGELOG entries match the diff's actual behavior — no claim beyond what's implemented.
6. Readability — names read correctly (MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE, candidates_dropped, with_candidate_cap, launch_fixture_with_approval). No dead code found. grep -i MUTATION over the full diff returns nothing — no leftover mutation-probe comments.
Findings
None blocking on correctness. One non-blocking note: the adapter_source_never_calls_created_slot_value_to_slot pin named in the brief could not be located by that literal string via repo search from this diff alone — worth the implementer confirming its location.
What I did not run
No build, no test — per brief, judged from the diff at 836a10d plus CI-by-name only. Coverage percentage not verified (Test + coverage is still in_progress).
Blocking reason for CHANGES-REQUIRED: the "Lint commit messages" required context is FAILING at this head SHA and "Test + coverage" has not yet attached. Re-run/verify both before re-requesting review; correctness itself is sound pending that.
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Verdict: PASS
Head SHA: 836a10d (unchanged from prior review)
Independently re-read all five required contexts against this exact head SHA (not taking any report on faith):
- Lint commit messages: SUCCESS (run started 2026-09-24T18:43:10Z, against the current title "fix(rewards-claim): chain_port audit, approval refusal, discovery cap" — supersedes the two earlier failing runs against the stale retitled-but-uncaptured title)
- Test + coverage: SUCCESS (started 18:32:24Z)
- Rustfmt: SUCCESS
- Clippy: SUCCESS
- Release-script tests: SUCCESS
All five green on 836a10d. That was the sole blocker in my prior review; correctness findings from that review stand unchanged (head did not move, so the diff was not re-read) — all six named questions (#3362, #3358, #3357, #3363, regressions, readability) answered clean, zero inline threads were ever opened.
One item from the prior review is now resolved: the pin adapter_source_never_calls_created_slot_value_to_slot — confirmed present at chain_port.rs:671 at this head, untouched by this diff.
PASS.
1 participant
DO NOT MERGE -- gate round in progress
Refs DIG-Network/dig_ecosystem#3357 #3362 #3358 #3363
Per-ticket status
#3357 -- audit
created_slot_value_to_slot: LANDEDclippy.toml(repo root, new):disallowed-methodsentry banningchia_sdk_driver::RewardDistributor::created_slot_value_to_slotworkspace-wide, with thephantom-
LineageProofrationale in the reason string.crates/dig-node-service/tests/common/rewards_fixture.rs:648,carries
#[allow(clippy::disallowed_methods)]with a comment stating why its receiver isin-process this generation (its own
pending_spend.created_reward_slots), not chain-rebuilt.#[allow]makescargo clippy -p dig-node-service --all-targets --locked -- -D warningsfail atrewards_fixture.rs:653with the exact reason text; restored afterward.call, not a semantic check of the receiver's provenance) -- that is why every
#[allow]sitecarries an explanatory comment instead of a narrower lint.
#3362 -- refuse
require_payout_approvaldistributors by name: LANDEDcrates/dig-node-service/src/rewards_claim/chain_port.rs,submit_initiate_payout: refuses witha named
ClaimPortError::Other(...)beforeSpendContext::new()whensnapshot.distributor().info.constants.require_payout_approvalis true -- this adapter dropsinitiate_payout's returnedconditionsunconditionally, so proceeding would silently violatethe approval requirement. Never
Ok(()), neverOk(None)-- a named refusal.a_distributor_requiring_payout_approval_is_refused_by_name_before_any_broadcast(
tests/rewards_claim_chain_port_3347.rs), against a REAL simulator launch with the flag curriedtrue via the new
launch_funded_admitted_fixture_with_approval(not a struct literal)."expected a named refusal, got Ok(())"; green with the guard in place (see M1 below).
#3358 -- bound hinted-launcher candidates, report every drop: LANDED
MAX_HINTED_LAUNCHER_CANDIDATES_PER_CYCLE = 256(chain_port.rs), with a doc block stating thederivation and what it does NOT cover.
discover_distributorsnow caps candidates via.take(candidate_cap), computescandidates_dropped = total.saturating_sub(candidate_cap), and returns the newDiscovery { distributors, candidates_dropped }(types.rs) instead of a bareVec-- so abounded port cannot silently shrink its answer.
engine.rs::run_cyclethreads the drop count intoClaimStatus.discovery_candidates_dropped_this_cycle(reset every cycle, never latched) and logstracing::warn!when nonzero.RealClaimChainPort::with_candidate_cap(...)lets a test pin a small cap instead of decoding 256+real candidates.
a_capped_cycle_drops_the_candidate_past_the_cap_and_reports_it(cap=1, real launcher idpast the cap -> dropped and reported) and
a_cap_that_covers_every_candidate_drops_nothing(cap=2, nothing dropped) -- both in
tests/rewards_claim_chain_port_3347.rs.discover_distributors/hinted-launcher clause exists in this repo's SPEC.md (greppedfor "discover_distributors", "HintedLauncherIndex", "hinted launcher" -- no matches), so nothing
to amend there.
#3363 -- fix the empty-index shortcut: LANDED
a_failing_source_reports_unavailable_everywherenow builds its port overFixtureLauncherIndex(vec![launcher_id])(wasvec![]), sodiscover_distributorsandsubmit_initiate_payoutactually reach the failingMockChainSource'sUnavailableanswerinstead of short-circuiting on an empty candidate list before ever driving the source.
Mutation proofs
M1 (#3362 guard removed):
Restored; test green again (1 passed; 0 failed).
M2 (#3358: candidates_dropped forced to 0 unconditionally):
Restored; both cap tests green again.
M3 (#3357:
#[allow(clippy::disallowed_methods)]removed at the fixture call site):Confirms
--all-targetscatches the fixture-crate call too. Restored; clippy clean again.M4 (#3363: index emptied back to vec![]):
With
vec![], discovery short-circuits to an emptyOkWITHOUT ever calling the failing source --proving the pre-#3363 empty-index version passed for the wrong reason. Restored; test green again.
M5 (#3357: production-call probe): a temporary call to
created_slot_value_to_slotadded insidechain_port.rs::reserve_asset_id(production code) madecargo clippy -p dig-node-service --all-targets --locked -- -D warningsfail with the samedisallowed-method diagnostic at that production call site; reverted immediately after capture, no
trace left in the diff.
Tests
cargo test -p dig-node-service --test rewards_claim_chain_port_3347(whole file,--test-threads=1):12 passed, 0 failed (was 10 before this PR; 2 new #3358 cap tests added).
cargo test -p dig-node-service --lib rewards_claim: 100 passed, 0 failed (unchanged count --confirms the
Discoveryrefactor across engine.rs's 6 test fakes and driver.rs's 2 test fakesintroduced no regression).
a_driven_cycle_over_a_funded_admitted_distributor_pays_this_peer,a_driven_cycle_over_the_real_adapter_reaches_a_real_chain_read,discover_distributors_returns_exactly_the_real_launch,adapter_source_never_calls_created_slot_value_to_slot(chain_port.rs:666),the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window(driver.rs:1657).Local verification
cargo clippy -p dig-node-service --all-targets --locked -- -D warnings: clean.cargo fmt --check -p dig-node-service: clean (after onecargo fmtpass, committed).cargo clippy --workspaceorcargo test --workspace(brief: never--workspace;scoped to
-p dig-node-servicethroughout). Did not run the full CI matrix (.deb/.msi/.pkg builds,CodeQL) locally -- those are running on the pushed commit per
gh pr checks.Blast radius
Touched only
crates/dig-node-service/src/rewards_claim/{chain_port,port,mod,types,engine,driver}.rs,crates/dig-node-service/tests/{rewards_claim_chain_port_3347.rs,common/rewards_fixture.rs},root
clippy.toml, andCHANGELOG.md. No changes toserver.rs,meta.rs,dispatch.rs,tests/server.rs, ormodules/apps/dig-node(all read-only per brief; sibling lane PR #621).Discovery's replacement ofVec<DiscoveredDistributor>asClaimChainPort::discover_distributors'sreturn type is a breaking signature change on a crate-internal trait with exactly the implementors
listed above (grepped for every
impl ClaimChainPort/discover_distributorsincrates/dig-node-service); none live outside this crate.Not done / needs the parent
## [Unreleased]section is new to this file (no prior Unreleased headerexisted); flagging in case the sibling lane touching CHANGELOG.md for PR fix(rpc): gate node-local reward reads, rate-bound open chain reads (#3352 #3355) #621 also adds one --
those would need reconciling at merge time.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com