feat(rewards): construct and install a real RewardsChainPort over dig-rewards-coin - #614
Conversation
acc8e6b to
6ff2fd9
Compare
Adversarial gate leg 3 (loop-decider) — CHANGES-REQUIREDHead SHA read: Required contexts on I am not re-reviewing. Six attacks, six answers. 1. Is A3 evidence, or a fixture in a simulator's clothes? — It is real evidence.
The author's central claim holds as the test is written: the only path from the source to One correction to the prose, though: the module doc's "No fixture value can produce the right 2. The mutation standard applied to A3 itself — a full gut is caught; three partial guts are not.Gutting But the author mutation-proved the guard, not the report, and the report has unasserted computed fields. A3 asserts 10 of 15. Note also that three of the ten asserted values are zero ( Unasserted: The one that matters: 3. The two-site rule — sites whose revert leaves the suite greenCovered (a revert goes red): the Not covered — revert leaves the whole suite green:
And the circular one. 4. The dropped anti-silence requirement — right about the test, wrong about the silence. There is one, and it is in this diff.The scheduler anti-silence test genuinely does not apply; a test asserting "at least one cycle fired" over a reactive port could not fail. Agreed, and not what I am arguing. The silence this port can have is a different shape: installed, every call failing, operator sees nothing. Surface: the error return of 5. The dependency decision — 0.5 is the right call; the stated reasoning does not establish it.Verified independently against the published
Both deltas are off our path. Conclusion: staying on 0.5 is safe for the read-only surface this PR ships, and only for that. The moment 6. Claims true at merge?
Remedy (explicit, checked against every reader of what it reroutes)R1 — PR body. Rewrite the Test plan to describe what actually ships (A3 present and passing) and drop R2 — de-circularize the install evidence. In R3 — make the one computed field testable. Extract the epoch ordinal from R4 — one log line, this PR. A bounded R5 — stop calling a transport gap an identity verdict. Give Follow-up ticket (not blocking this PR): the untested sites in §3 items 1, 3, 4 and 5 — the puzzle-reveal substitution guard, the Failure direction if this merges as-is: the wrong call here is merging on "five greens and a simulator-backed test." It ships a port that is genuinely real on its one happy path, with its money field never executed once, its anti-substitution guard unexercised, its install-site warning proved by a test that wrote the string itself, a PR body that denies its own test, and no way for an operator ever to learn that every reward read is failing. That is not an inert-code defect — it is the #3310 defect with a production type name on it, which is strictly harder to find later than the I split from the other two legs on this. R1–R5 are all small and in-file; none require redesigning the adapter. |
Security gate verdict -- PASSHead audited: Required checks (confirmed by name): Rustfmt SUCCESS, Clippy SUCCESS, Test + coverage SUCCESS, Lint commit messages SUCCESS. S1 -- Reachability of the hang
S2 -- Guard completeness beyond epoch_seconds == 0Confirmed against S3 -- Who picks the launcher id / cost
S4 -- Money / wrap
S5 -- Honest failure
S6 -- The install
FindingsNone LIVE. No defence-in-depth notes beyond what the PR's own module docs already name accurately (the two-bound distinction in S2). Scope audited
Not coveredDid not re-audit Worktree used: |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate — PASS
Head 6ff2fd98a4cbcc22082e55bca9e1f5b694005838. Required checks confirmed by name, all SUCCESS: Rustfmt, Clippy, Test + coverage, Lint commit messages.
R1 — guard unbypassable. Grepped every read_distributor call site in the workspace: the only production caller is chain_source.rs:89's read_distributor_guarded, which runs refuse_non_terminating_epoch_seconds first (line 526) before ever calling dig_rewards_coin::state::read_distributor. No other file in the crate/workspace calls read_distributor directly. Confirmed.
R2 — guard reads only what's safe. refuse_non_terminating_epoch_seconds → read_launch_constants → RewardDistributor::from_launcher_solution (verified against the vendored chia-sdk-driver-0.36.0 source at primitives/action_layer/reward_distributor.rs:440): it only decodes the launcher's own LauncherSolution's key_value_list, no chain reads beyond the one source.coin_spend(launcher_id) call, no generation walk. Confirmed.
R3 — install site. server.rs:2140-2161 sits inside if config.enable_chain_sync {, alongside the census/mirror passes. false from install_reward_chain_port is logged at WARN, never ignored/unwrapped. Confirmed single-install OnceLock invariant not relaxed (also proven by the new test install_reward_chain_port_refuses_a_second_install_with_a_warn).
R4 — runtime-handle constraint. Both production entrypoints that reach serve_with_shutdown build multi-thread runtimes before calling it: entrypoint.rs:1454 (Builder::new_multi_thread() → block_on(serve(config))) and win_service.rs:132 (same). Handle::current() at the install site is always multi-thread in production. Confirmed.
R5 — #3303 containment test region-slicing. chain_port.rs's production_region slices each file at its own first #[cfg(test)] marker and scans only the prefix. Since the assertion string and test name (which themselves contain the literal withdraw_committed_incentives) live inside that same #[cfg(test)] mod tests block, they fall in the sliced-off region — the test cannot be satisfied by its own text. Confirmed correct, non-self-defeating.
R6 — honest failure / error mapping. guarded_read_error_to_port_error and reader_error_to_port_error (chain_port.rs) have zero _ => wildcard arms (verified via full-diff grep — the one _ => Ok(())) hit is the guard's own deliberately-permissive pass-through, documented and correct, not part of the port error mapping). GuardedReadError::NonTerminatingEpochSeconds maps to ChainPortError::Other(...), never Unavailable — confirmed against vendored dig-rewards-coin-0.5.0 source that RewardsError::ChainUnavailable is the only variant mapped to Unavailable. No method returns Ok with a zeroed/defaulted report; the four out-of-scope methods answer Err(ChainPortError::Unavailable) explicitly.
R7 — dependency shape. Confirmed by direct read of Cargo.toml: [dependencies] (from line 58) pins chia-sdk-driver/chia-sdk-types to =0.36.0 and chia-puzzle-types to =0.36.1; chia-bls/chia-protocol/chia-sha2 remain caret 0.36.1. [dev-dependencies] (from line 341) carries = pins on chia-consensus/chia-puzzles (new) — fine, dev-deps don't propagate.
R8 — prose. Checked the hang claim against the vendored source: chia-sdk-driver-0.36.0/src/layers/action_layer/actions/reward_distributor/commit_incentives.rs lines 101-111 do run while end_epoch_time > start_epoch_time { ...; start_epoch_time += epoch_seconds; } with no .await in the file — at epoch_seconds == 0 this is a genuine non-yielding infinite loop that tokio::time::timeout cannot preempt (no poll point). The module doc's line-number references (state.rs:1015, MAX_COMMIT_INCENTIVES_BACKFILL_SLOTS at state.rs:73) and the RewardsError::UnreadableDistributorConstants/UnreadableEpochSeconds variant names were all checked against the vendored dig-rewards-coin-0.5.0 source and are accurate.
No blocking findings. No inline threads opened.
Not run: a full local cargo test -p dig-node-service (background cargo check was still building at review time on a cold worktree target dir); relied on the CI Test + coverage run against this exact head SHA plus direct reading of the vendored dependency sources for the claims that check couldn't have caught anyway (doc-prose accuracy, dependency-table shape).
…d dig-rewards-coin Money-bug containment for dig_ecosystem#3303/#3286: exact `=` pins on dig-node-service's direct edge stop the resolved chia-sdk-driver/chia-sdk-types/ chia-puzzle-types versions moving under us. This constrains OUR compile target only -- Cargo.lock still carries a second, transitive chia-sdk-driver 0.30.0 and chia-puzzle-types 0.26.0 line via other crates, and this pin does not evict those. Containment, not correction. chia-protocol/chia-bls/chia-sha2 stay caret: an `=` on a wire type poisons every crate that depends on chia-protocol directly. dig-rewards-coin = "0.5" (not the ticket's stale "0.4"): 0.5.0 is the release that already refuses `epoch_seconds == 0` inside `read_distributor` itself. Refs #3310 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-rewards-coin Adds RealRewardsChainPort (crates/dig-node-service/src/rewards/): distributor_report served for real over dig-wallet's CorroboratedChainSource and dig_rewards_coin::state::read_distributor, via a guarded read (read_distributor_guarded) that refuses launch constants carrying epoch_seconds == 0 BEFORE calling into dig-rewards-coin at all -- defense in depth over that crate's own identical refusal (state.rs:1015), since chia-sdk-driver-0.36.0's commit_incentives backfill loop never terminates on that value and has no await point a timeout could interrupt. store_id/root are recovered from the launcher's creating spend's CREATE_COIN memo (chain_source.rs), following dig-mirror-coin's read_parent_outputs pattern: authenticate the puzzle reveal against the coin's puzzle hash before running it, never trust an unauthenticated memo alone. The other four RewardsChainPort methods (funded_distributors, distributor_state, submit_entry_writes, spend_new_epoch) answer Unavailable -- out of this ticket's scope (funder-registry and prover-cycle work tracked separately). Installed once from server.rs's enable_chain_sync-gated block, logging a false (already-installed) return at WARN. Refs #3310 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's `cargo fmt --all -- --check` and `--locked` builds both failed on the prior commit: rustfmt wanted several closures/return-types reformatted, and Cargo.lock was missing the `chia-sdk-test`, `clvm-traits` and `clvmr` entries the new adapter's Cargo.toml lines require, which `--locked` refuses to backfill. Refs #3310 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t, fix constants fixture
- LaunchCommentError::ChainSource/Malformed tuple payloads were flagged
dead_code by clippy: a derived Debug impl does not count as reading a
private field. Add a manual Display impl that formats each variant's
payload, and switch chain_port.rs's build_report call site from {error:?}
to {error} so the reason reaches a log reader.
- adapter_source_never_imports_withdraw_committed_incentives was
self-defeating: it include_str!s its own file and the test's own name
and assertion messages contain the literal string it searches for, so
it could never pass. Scope the scan to production_region(), everything
before the file's own #[cfg(test)] marker.
- launcher_spend_with's RewardDistributorConstants fixture set
reserve_inner_puzzle_hash/reserve_full_puzzle_hash to Bytes32::default()
without calling .with_launcher_id(launcher_id), which recomputes both
fields from curried tree hashes. chia-sdk-driver's
RewardDistributor::from_launcher_solution requires constants ==
constants.with_launcher_id(launcher_id), so this fixture deterministically
failed to decode on every invocation (not flaky/timing-dependent).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ll-path evidence Drives RealRewardsChainPort::distributor_report end to end against a distributor launched by dig_rewards_coin::launch_dig_distributor in chia-sdk-test's peer simulator, over a MockChainSource loaded from the simulator's real coin records/spends. store_id/root are asserted against the values launched with -- recoverable only by actually running the launcher's parent spend and decoding its CLVM memo, so no fixture shortcut can pass this test. Also covers install_reward_chain_port's single-install refusal (true then false, with the WARN server.rs's own call site logs). Node exposes no lighter test constructor to an external integration test crate, so the install-path test uses Node::from_env(), the same constructor openrpc_drift_guard.rs's own test uses. launch_fixture returns Box<dyn std::error::Error> rather than pulling in anyhow for one test file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…identity verdict ChainSource::parent_spend returning Ok(None) is a chain-source gap (the source does not yet hold the launcher's creating spend), not a genuine "this is not a DIG distributor" classification. Give it its own LaunchCommentError::ParentSpendUnavailable variant so chain_port.rs can map it to ChainPortError::Unavailable, agreeing with the other absence path (read_distributor_guarded's own Ok(None)), instead of rendering a transport lag as a definitive negative identity claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add one bounded tracing::warn! on distributor_report's error path, firing once per failure->success transition (not per call, via an AtomicBool), so an installed-but-degraded chain source is no longer indistinguishable from "no port installed." - Extract current_distributor_epoch's arithmetic into a pure epoch_ordinal fn and unit-test it directly (non-zero multi-epoch case, the saturating_sub clock-skew branch, and the bare-launch zero case) -- the only non-trivial computed field in the report mapping, previously unexercised by anything but a zero-valued default. - Add launch_comment_error_to_port_error, wiring chain_source.rs's new ParentSpendUnavailable variant to ChainPortError::Unavailable, with a regression test, and a companion test confirming GuardedReadError::NonTerminatingEpochSeconds still maps to the named refusal (Other), never to Unavailable. - Add a unit test proving a failing chain source surfaces as a named ChainPortError::Unavailable, never Ok(_) with a default-valued report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
install_reward_chain_port_refuses_a_second_install_with_a_warn previously captured and asserted against a warn it emitted itself inside the test's own closure -- deleting server.rs's real warn line would have left it green. Replace the self-emission with a source-text check against server.rs's own production region (the same shape adapter_source_never_imports_withdraw_committed_incentives already uses), so the assertion can only be satisfied by what server.rs actually ships. Also softens the A3 module doc's overstated "no fixture value can produce the right store_id/root" claim: true of this file today, not a structural guarantee. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e text not runtime behaviour Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
10a9044 to
660df1f
Compare
Summary
Implements DIG-Network/dig_ecosystem#3310: a production
RewardsChainPort(RealRewardsChainPort) overdig-wallet'sCorroboratedChainSource+dig_rewards_coin::state::read_distributor, installed at startup viaNode::install_reward_chain_port.crates/dig-node-service/src/rewards/chain_source.rs: the guarded read (read_distributor_guarded) that refusesepoch_seconds == 0before callingread_distributorat all (defense in depth overdig-rewards-coin's own refusal atstate.rs:1015), and launch-comment (store_id/root) recovery from the launcher's creating spend's memo. A missing parent spend (ChainSource::parent_spend->Ok(None)) is now a distinctLaunchCommentError::ParentSpendUnavailable, mapped toChainPortError::Unavailable-- a chain-source gap is no longer reported as the definitive "not a DIG distributor" identity verdict.crates/dig-node-service/src/rewards/chain_port.rs:RealRewardsChainPort, servingdistributor_reportfor real; the other fourRewardsChainPortmethods answerUnavailable(out of this ticket's scope). Extracted the report's one non-trivial computed field (current_distributor_epoch) into a pure, directly-unit-testedepoch_ordinalfunction. Added one boundedtracing::warn!(once per failure->success transition, not per call) ondistributor_report's error path, so a degraded chain source is observable rather than silently indistinguishable from "no port installed."server.rs: installs the port once, in theenable_chain_syncgated block.Cargo.toml: pinschia-sdk-driver/chia-sdk-types/chia-puzzle-typesto exact versions (containment for #3303/#3286), addsdig-rewards-coin,clvm-traits,clvmr, and dev-dependencies (dig-chainsource-interfacewith thetestingfeature,chia-sdk-test) for the simulator-backed acceptance test.dig-rewards-coinstays on0.5, not0.6The read-only surface this adapter uses is
state.rs(read_distributor,DistributorSnapshot),comment.rs(LaunchComment), andclawback.rs(recoverable_base_units) -- those three files are byte-identical betweendig-rewards-coin0.5.0 and 0.6.0 (verified against the published.cratetarballs). The only real 0.6.0 change is a fail-closed fix inentries.rs'sadd_entry/remove_entry(a permanently-open entry-set write window, now closed) -- that lives on the entry write path, which this adapter never calls (submit_entry_writesreturnsUnavailable). Staying on0.5is therefore safe for what this PR ships, and only for that: 0.6.0 becomes mandatory, not optional, the momentsubmit_entry_writesis implemented (#3249/#3250), since it carries a fail-closed fix for a write-path hazard this PR's own read-only adapter does not exercise.Test plan
chain_source.rs): refusesepoch_seconds == 0before reading; passes through a legitimate value.withdraw_committed_incentivesstring-absence test (chain_port.rs).distributor_reportintegration test (A3,tests/rewards_chain_port_a3.rs): drives the productionRealRewardsChainPort<MockChainSource>end to end against a realchia-sdk-testsimulator launch -- only the network transport is a double; every coin record/spend comes from a genuineSimulator::spend_coinscall.ChainPortError::Unavailable, neverOkwith a default-valued report (chain_port.rsunit test), andGuardedReadError::NonTerminatingEpochSecondsis confirmed never mapped toUnavailable.current_distributor_epoch's arithmetic (epoch_ordinal) is unit-tested directly, including the non-zero multi-epoch case and the clock-skewedsaturating_subbranch -- a bare-launch-only assertion could not distinguish the real computation from a0-returning gut.install_reward_chain_port_refuses_a_second_install_with_a_warn) now asserts againstserver.rs's actual production source region, not a warn the test emits itself; mutation-proved (deletingserver.rs's real warn line turns the assertion red, restoring it turns it green again).Known out of scope for this PR, filed as follow-up (dig_ecosystem#3310 epic, not blocking this merge): the
commitments/recoverable_base_unitsloop and the puzzle-reveal substitution guard inparse_launch_commentare unexercised by any test here (both need acommit_incentivesspend in the A3 fixture -- real simulator work); theZeroIdentity/InvalidWithdrawalSharerefusal cases inreport_from_snapshotare untested; and the error-mapping table's "neverUnavailable" safety claim (guarded_read_error_to_port_error) has no test holding it end to end.Refs #3310
🤖 Generated with Claude Code