THROWAWAY: Finding 3 revert-check for #594 -- DO NOT MERGE, will be closed - #599
Closed
MichaelTaylor3d wants to merge 27 commits into
Closed
THROWAWAY: Finding 3 revert-check for #594 -- DO NOT MERGE, will be closed#599MichaelTaylor3d wants to merge 27 commits into
MichaelTaylor3d wants to merge 27 commits into
Conversation
mod.rs declared no submodules, so types.rs/port.rs/config.rs/cadence.rs/ parser.rs/engine.rs/hints.rs (1,435 lines, 25 tests) were never part of the crate and never compiled. Declare them and re-export the public surface.
…ocol 0.10->0.11.0 dig-rpc-protocol 0.11.0 is merged and tagged upstream; the other dig-*/chia-* deps of dig-node-service were already at the latest permitted-by-caret version in Cargo.lock. crates/dig-node-core/Cargo.toml is untouched (#3250's file set).
…umps
Both create a duplicate-version split in this PR's scope and neither can be
closed without editing a sibling crate's manifest this lane does not own:
- dig-rpc-protocol 0.11.0 duplicates against dig-node-core/Cargo.toml:194
("0.10.2"), which is #3250's live file set (dig-node#593).
- dig-node-control-interface 0.35.0 duplicates against
dig-wallet/Cargo.toml:81 ("0.33"), a sibling crate this lane does not own;
the observed Clippy break (BalanceAsset/Asset type-identity mismatch,
missing url_reconcile/url_current/urls fields) came from THIS duplicate,
not from dig-rpc-protocol.
Both belong to their own sequenced dep-bump unit of work, not this ticket.
…ee ceiling magnitude Three independent gates on dig-node#594 (51516e6) found four logic defects; this addresses A, B and C per the corrected fix brief (D is documented only, not fixed here per the brief's own instruction). Defect A -- the anti-silence surface laundered every real fault into `Nominal`: - A1: `fault_reported` had no fault-bearing ClaimLoopState to fall through to, so a chain adapter erroring every cycle read `Nominal` forever. Added `ClaimLoopState::Faulted { cycles }`, outranking Nominal/ClaimableButNotClaiming, under ChainSourceUnavailable. - A2: inverted the test that asserted A1's bug as correct behaviour. - A3: `ClaimableButNotClaiming` compared a per-cycle snapshot (`distributors_claimable`) against a lifetime-cumulative counter (`claims_submitted`), so it latched healthy forever after one lifetime success. Added `claims_submitted_this_cycle` (per-cycle) as the correct comparand; kept `claims_submitted` as a cumulative counter. - A4: `last_discovery_at`/`last_cycle_at` were stamped even on a failed discovery or an all-faulted cycle, destroying the staleness signal a reader depends on. Now only stamped on success; added `last_attempt_at` to prove liveness separately. `fault_reported` and `distributors_faulted` now reset per cycle instead of latching for the process's lifetime. Defect B -- "terminal, stop retrying" was implemented as a process-lifetime blacklist (`terminal_no_entry: HashSet<Bytes32>`, never cleared). That blocked SPEC 12.5 clause 2's re-entry path (evicted, re-challenged, re-admitted never claims again) and permanently punished a peer that discovered a distributor before the funder's AddEntry landed. Removed the blacklist entirely -- `own_entry` is a cheap chain read, re-issued every cycle for every candidate, matching clause 3's "never cache across cycles". `NoEntrySlot` is now a per-cycle observation, not a lifetime sentence. Defect C -- the fee ceiling didn't bind anything and there was no aggregate cap: - C1: default `CLAIM_FEE_CEILING_MOJOS_DEFAULT` lowered from 1_000_000_000 (transplanted from `MIRROR_SPEND_FEE_CEILING_MOJOS`, sized for a mirror-coin spend) to 200_000 -- 2x the observed routine Chia fee range (5,000-100,000 mojos), so it actually binds instead of leaving 4-5 orders of magnitude of slack. - C2: added a per-cycle aggregate fee budget (`max_cycle_fee_budget_mojos`, default 10x the per-claim ceiling) checked across all claims in a cycle, closing the attacker-cost gap where funding K distributors could force a victim to spend K x the per-claim ceiling per cycle. New `ClaimOutcome::SkippedCycleBudgetExhausted`. Tests: rewards_claim test count 26 -> 37 (11 new: repeated_discovery_faults_never_ read_as_nominal, failed_discovery_leaves_last_discovery_at_unchanged, a_reported_ fault_surfaces_as_faulted_not_nominal, a_lifetime_submission_does_not_mask_a_ later_cycle_that_submits_nothing, no_entry_slot_then_re_admitted_produces_a_claim_ on_the_later_cycle, distributors_each_under_ceiling_do_not_collectively_exceed_ the_cycle_budget, the_default_per_claim_ceiling_actually_binds_a_routine_fee, plus renamed/rewritten no_entry_slot_is_non_terminal_and_re_checked_every_cycle). Refs #3251
…hash CI fix: cadence.rs's RewardsClaimConfig literal was missing the max_cycle_fee_budget_mojos field added in the previous commit (E0063, caught by CI's Clippy/Test jobs -- the local cargo check for this workspace is too slow to use as the compiler here). Defect E (security-gate finding, folded in before this pass closes): submit_initiate_payout was called with entry.payout_puzzle_hash -- whatever the chain port handed back -- with no check against this node's own own_payout_puzzle_hash. UnavailableClaimChainPort is the only production adapter today so nothing can exploit this yet, but the whole point of the ClaimChainPort seam is that #3249 swaps in a real adapter with nothing above it changing, so deferring this would ship the landmine live with no review pass watching for it. Added an equality guard before the spend: a mismatch refuses to submit, counts (ClaimStatus::claims_refused_payout_mismatch), surfaces its own named outcome (ClaimOutcome::PayoutPuzzleHashMismatch), and is reported as a fault (a divergent entry means the port is confused or hostile, not that there is nothing to claim) -- never corrected by substituting our own hash and proceeding. Defect D: documented, not wired, per instruction -- added the "not yet wired into node startup" paragraph to mod.rs's module doc (the PR body carries the same paragraph) so the next reader arrives at the caveat in the code, not only in a merged PR description. Refs #3251
…arison, not a zero-test
submitted_this_cycle < claimable_this_cycle now fires the anti-silence state, carrying
the shortfall as ClaimableButNotClaiming { claimable, submitted }. The previous
submitted_this_cycle == 0 zero-test let one submission mask any number of same-cycle
skips (claimable=10, submitted=1 read Nominal).
Also folds in B3's precedence fix (ChainSourceUnavailable > Faulted >
ClaimableButNotClaiming > Idle > Nominal) and the per-distributor
payout_hash_mismatches_this_cycle counter so a per-distributor fault can no longer
pin the cycle-wide Faulted state, plus R2's rename of terminal_no_entry_slot to
no_entry_slot_this_cycle now that it is no longer terminal.
…distributor fault isolation B2: run_cycle now splits into a pre-budget phase (asset/entry/hash/threshold checks, producing the claimable set) and a budget phase, ordering the claimable set by accrued value descending before applying the fee ceiling and cycle budget. Dust distributors (low accrued value regardless of attacker-controlled fee) now sort last and are the ones the budget drops, closing the claim-suppression attack where ten high-fee dust distributors could consume the whole cycle budget ahead of a victim's real earnings. A rotation_cursor tie-breaks only WITHIN equal-accrued-value tiers so a genuinely tied honest tail that exceeds one cycle's budget every cycle still rotates through and is eventually served, rather than dropping the same tail forever. B3: the payout-hash mismatch check in evaluate_pre_budget now increments the per-distributor payout_hash_mismatches_this_cycle counter instead of setting fault_reported, so one hostile or buggy entry can no longer pin the cycle-wide Faulted state and bury ClaimableButNotClaiming for every other healthy distributor. R2: terminal_no_entry_slot -> no_entry_slot_this_cycle throughout.
…ation cursor An operator reading their own rewards-claim.json and seeing enabled: true has no way to know from that file alone that no startup path constructs a ClaimEngine yet (#3268) -- mod.rs said so, but a config file reader does not arrive at a module doc. Also gives RewardsClaimConfig a rotation_cursor: Option<Bytes32> field so B2's tie-break cursor survives a save/load round-trip -- an in-memory-only cursor resets on every restart, which would starve a legitimately tied honest tail forever on any node that restarts daily.
…fresh Collapses the nested if into the outer match arm in run_cycle (clippy::collapsible_match). Also refreshes NoEntrySlot / no_entry_slot_this_cycle doc comments now that dig-rewards-coin v0.1.3's SPEC §12.5 amendment is merged and tagged: absence is terminal for one claim attempt only, never for the distributor, must not be cached, and must not accumulate into a permanent exclusion set -- confirming rather than diverging from the re-read-every-cycle behaviour already implemented.
…est literal Struct literal in the cadence test module was not updated when RewardsClaimConfig gained rotation_cursor (R5 commit) -- CI's Clippy/Test jobs caught the missing field (E0063) that a local cargo check could not (killed by memory pressure before this workspace-wide build completed).
…a latch compute_state() compared against self.state -- last cycle's OWN computed output -- so once any cycle took an Unavailable port path, every later cycle re-asserted ChainSourceUnavailable forever, even after the chain came back and real claims were submitting. A node still syncing, or one dropped connection, was enough to trip this permanently. Add ClaimStatus::chain_unavailable_this_cycle, reset to false at the top of every run_cycle and set true only on a cycle that actually took the Unavailable path; compute_state now reads that flag instead of self.state, so the reading is live again. Test: a_transient_unavailable_cycle_does_not_latch_state_for_the_rest_of_the_process (engine.rs) drives cycle 1 unavailable, cycle 2 healthy with a submission, and asserts cycle 2 reads Nominal. Plus a compute_state-level regression in types.rs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… correct version doc F3: reset EVERY per-cycle counter (distributors_known/with_own_entry/ claimable/faulted, claims_submitted_this_cycle, no_entry_slot_this_cycle) at the TOP of run_cycle, before any early return. The three ChainUnavailable early-return paths skip the end-of-function assignment block entirely, so a cycle that hit one used to leave the PRIOR cycle's counts sitting on self.status while last_attempt_at stamped fresh for THIS cycle -- a stale count under a fresh timestamp, exactly what SPEC §2.4's staleness reasoning forbids. types.rs's doc sentence for no_entry_slot_this_cycle now correctly says it is dated by last_attempt_at (the field stamped unconditionally every cycle), not last_cycle_at. F4: dedup `candidates` by launcher id before phase 2. A real adapter scanning §1.3 launch comments across every (store_id, root) this node mirrors can plausibly return the same launcher id twice; without dedup phase 2 would evaluate it twice and submit InitiatePayout twice against one entry slot in one cycle -- the second spend is invalid but the fee is paid anyway. F5: dig-rewards-coin is v0.1.3, published on crates.io -- correct the stale "v0.1.1" module-doc claim. Tests: a_chain_unavailable_cycle_does_not_leave_prior_cycles_counters_stale (F3), a_duplicated_launcher_id_submits_exactly_once (F4). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…edicate (F2)
A payout-hash mismatch never enters the eligible set, so it was counted in
NEITHER distributors_claimable NOR claims_submitted_this_cycle -- the
shortfall lived in neither term of compute_state's magnitude comparison.
All-K-distributors mismatching therefore read Nominal (falsely healthy).
Fold payout_hash_mismatches_this_cycle into the comparison's denominator:
submitted < claimable + mismatches. The result is ClaimableButNotClaiming
(a shortfall), never the cycle-wide Faulted -- Defect B3 stays fixed.
Inverts the assertion at what was engine.rs:1305
(a_payout_mismatch_never_sets_the_cycle_wide_fault_or_masks_other_distributors):
it previously asserted ClaimLoopState::Nominal across three cycles of an
ongoing mismatch, which pinned the defect as intended behaviour (an
A2-class test). It now asserts ClaimableButNotClaiming { claimable: 1,
submitted: 1 }.
Adds all_distributors_mismatching_is_a_shortfall_not_nominal, covering the
brief's exact "what if every distributor refuses for the same reason" case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s discovery Send CI's Clippy job (the compiler for this crate, per brief) caught it: holding a std::sync::MutexGuard across the .await in FlakyThenHealthyPort and HealthyThenUnavailablePort's discover_distributors made the returned future not Send, which #[async_trait]'s generated trait signature requires. Neither fake needs a lock -- each holds one call counter, incremented once per call, never read-modify-written across an await point. AtomicU32's fetch_add removes the guard (and the Send bound violation) entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oss restart The per-cycle aggregate fee budget and the 24h cadence clock both lived only in memory: `spent_this_cycle_mojos` was a `run_cycle` local and nothing on disk recorded a completed cycle. Every fresh process got a full `max_cycle_fee_budget_mojos` and an empty cadence clock, so a node stuck in a crash-restart loop could spend unbounded XCH on fees, one full budget per restart. Adds three `#[serde(default)]` fields to `RewardsClaimConfig` (`fee_window_start_unix`, `fee_spent_in_window_mojos`, `last_cycle_completed_at`) and a new opt-in `ClaimEngine::with_persisted_fee_ window(dir, cadence_seconds)` that: - restores the window/cadence state from `dir` at construction, - refuses to start a cycle until the cadence has elapsed since the last completed one, - rolls a fresh budget window only once the cadence has elapsed since it opened, otherwise keeps enforcing the budget against the persisted spend, - persists the spend BEFORE every chain submission (write-then-spend), never batched to cycle end, and persists the completed-cycle timestamp when a cycle finishes. Engines that never call `with_persisted_fee_window` (every pre-F7 test) are unaffected -- this is additive, opt-in state beside the existing rotation cursor, not a change to B2's value-ordering or rotation mechanism. `ClaimStatus`'s own counters stay in-memory on purpose (observability, meant to reset on restart); only the spend bound and the cadence gate persist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…isted fields The three new persisted RewardsClaimConfig fields (fee_window_start_unix, fee_spent_in_window_mojos, last_cycle_completed_at) broke this crate's only remaining full struct literal outside config.rs/engine.rs's own test modules -- E0063 missing fields, caught by CI's Clippy job. Switched to ..RewardsClaimConfig::default() so the next added field cannot break this literal again, the same fix already applied once before for rotation_cursor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rrupt window Salvaged from a lane killed by a weekly cap before it could commit. Uncompiled at commit time; CI is the compile signal. Covers the fourth gate pass findings on the F7 persisted spend bound: - F8: RewardsClaimConfig::save_to now writes atomically (temp file + rename in the same directory), reusing the pattern already used by mirror/reconcile_state.rs for the same class of state. load_from distinguishes an ABSENT file (clean first run, defaults are correct) from a PRESENT but unparsable one, which fails CLOSED: the window is treated as fully spent and nothing is submitted. Never Default, and never a silent clamp downward, which would hand back the budget the corruption was hiding. - F14: the budget comparison uses saturating arithmetic so a corrupt disk-seeded fee_spent_in_window_mojos cannot panic under the release profile's overflow-checks. - F9/F10/F12/F13 in progress in the same files. Refs #3251
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…shortfall compute_state (types.rs) already reported the folded shortfall denominator (distributors_claimable + payout_hash_mismatches_this_cycle) as `claimable` -- that part of F13 landed in f478516. The two engine.rs tests asserting this state were written against the pre-fold, un-folded numbers and never updated, so CI showed the implementation producing the correct folded value (`claimable: 2`, `claimable: 1`) while the test literals still expected the stale un-folded one (`claimable: 1`, `claimable: 0`). Update both literals -- and the comments describing them -- to the folded values the F13 fix actually produces. No production code change; compute_state's predicate and payload were already correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add the seventh ClaimOutcome variant: the type could only say a peer was legitimately not paid, never that a chain call failed. Carries the launcher id, a bounded (200 char) copy of the chain port's error text, and whether a pre-committed fee was reversed, so a reader can tell no money moved. Engine wiring at the two fault arms (engine.rs:332, :377) follows in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
engine.rs:332 and :377 used to increment `faulted` and discard the outcome, leaving a definitively-failed claim absent from the outcome stream -- indistinguishable from a cycle that never touched that distributor. Both PreBudgetResult::Fault and BudgetPhaseResult::Fault now carry the chain port's (bounded) error text, and the submit_initiate_payout failure path also carries the fee it reversed, so a reader can tell no money moved. The counter stays; it is not a substitute for the outcome. 7 call sites needed updating: 3 PreBudgetResult::Fault constructions (reserve_asset_id, own_entry, payout_threshold), 2 BudgetPhaseResult::Fault constructions (required_fee_mojos, submit_initiate_payout), and the 2 consuming match arms -- exactly the set that was silently discarding a failure before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Regression for the rework: reuses F12's fixture (a submission that definitely never broadcast) to prove both facts from one cycle -- the outcome exists and carries the reversed fee, and the persisted window still reflects zero net spend. Also fixes a rustfmt diff on the PreBudgetResult::Fault variant Clippy's Rustfmt job flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…aling state Finding 1 (dig-node#594 pass 6): a future-dated clock is self-healing by construction (`t > now` goes false the moment real time passes it), but the engine ORed it into `self.fee_window_poisoned` and set that field `true` permanently -- an RTC glitch or VM resume froze the claim loop forever instead of until the skew passed. This is the third instance of one mechanism (pass 3 latched ChainSourceUnavailable, pass 4 left a stale cadence-gate `state`), so the fix removes the FIELD, not just the bug: with no `fee_window_poisoned` on `ClaimEngine`, `self.fee_window_poisoned = true` is a compile error, not a convention to remember. Per-cycle conditions (corrupt + future-dated-clock) now live in a `CycleConditions` value built fresh at the top of every `run_cycle` from `now` plus a freshly reloaded `RewardsClaimConfig`, used, and dropped -- never stored on the engine. `corrupt` is now re-read from disk every cycle too (it previously latched at construction only), matching what `ClaimLoopState::PersistedStateCorrupt`'s doc already claimed but the code never did. Rewrites the single-cycle f10 regression into a two-cycle test: cycle 1 with a future-dated clock refuses; cycle 2, after the clock catches up and the cadence elapses, MUST claim. The old one-cycle version was green whether the latch bug was present or not. Refs #594
…cements (not for merge)
Contributor
Author
|
Superseded by #600 (rebased onto clippy/fmt-fixed tip so Clippy/Rustfmt don't confound the revert-check). |
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
1 participant
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.
Throwaway revert-check PR: removes 4 corrupt-load enforcement branches in config.rs to prove the 5 named older tests are not vacuous. Will be closed and branch deleted once CI reports results. Not for review, not for merge.