Skip to content

fix(rewards-claim): derive both cadences from one raw config value and inject the clock - #617

Merged
MichaelTaylor3d merged 12 commits into
developfrom
fix/3336-claim-driver-newtypes
Sep 18, 2026
Merged

MichaelTaylor3d merged 12 commits into
developfrom
fix/3336-claim-driver-newtypes

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

ClaimEngine::with_persisted_fee_window needs two cadences that must differ: the schedule-CLAMPED one the restart-safety gate is measured against, and the RAW configured one that sizes the persisted fee-budget window. Getting them the wrong way round is a money defect — the clamped value in the window slot halves the window, doubling the number of fee-budget windows (and so the fee ceiling) a long-cadence operator configured.

This PR replaces the two bare u64 arguments with one ClaimCadences derived from a single RawConfiguredCadence, and adds an injectable-clock seam so the production body itself can be tested.

What is type-enforced, and what is only test-guarded

Both halves are not closed the same way. Measured on this branch:

TYPE-ENFORCED (cannot compile):

  • The historic two-argument positional transposition. The call now takes ONE argument, so that shape is structurally gone.
  • The gate and the fee window cannot disagree with each other. ClaimCadences::from_raw is the only constructor, both fields are private, and it derives gate_clamped by clamping the very RawConfiguredCadence it stores as fee_window_raw. There is no pairing in which the window sizes off one number and the gate off another.

TEST-GUARDED ONLY (compiles fine):

  • Which u64 the call site labels raw. ClaimCadences::from_raw(RawConfiguredCadence(cadence_seconds)) — the already-clamped local instead of cfg.cadence_seconds — has the same type and compiles.
  • Measured consequence: the fee window halves. The window rolls one tick early and the tick-2 assertion fails with left: Some(5356800), right: Some(2678400) — exactly 2x the fee-budget windows the operator sized. The gate is unaffected (clamping an already-clamped value is the identity).
  • Guarded by exactly ONE test: the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window (fails at driver.rs:1467). The other money test, the_gate_tracks_the_clamped_cadence_while_the_fee_window_tracks_the_raw_one, stays GREEN under this mutation, because it hand-assembles drive and never traverses the production call site — its own doc comment says so.

A reader must not conclude that both halves are type-enforced. That misreading is how the value mutation gets reopened. The positional transposition is structurally gone -- the call takes one argument -- so what is left to get wrong is passing the clamped local instead of the raw config value, which compiles and is caught by exactly one named test (the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window). Every doc comment on the path now says that, and none of them claims a type stands behind it.

Changes

  • engine.rs: RawConfiguredCadence (the one input), ClampedGateCadence (private field, clamp the only producer), ClaimCadences::from_raw (the only constructor). with_persisted_fee_window takes one ClaimCadences and stays pub(crate)ClaimCadences is crate-private, so a pub method taking it would be uncallable from outside anyway, and no out-of-crate caller exists.
  • driver.rs: run_claim_driver_in_with_clock — the production body with drive's clock as a parameter. tokio::time::advance moves only the tokio virtual clock, not SystemTime::now(), so before this seam nothing that depends on the VALUE now() returns each cycle was observable through the production body. run_claim_driver_in stays a thin wrapper passing the real clock; no production caller changes.
  • Doc comments corrected to match the measured truth above: the previous round asserted a type-level guarantee for the value mutation that the code does not deliver, and still described "transposing the two arguments" after that shape had been removed.

Verification

  • cargo fmt --all -- --check — clean
  • cargo clippy -p dig-node-service — clean
  • cargo test -p dig-node-service --lib rewards_claim — green
  • 13 CI checks green, 0 red

Refs DIG-Network/dig_ecosystem#3336

🤖 Generated with Claude Code

MichaelTaylor3d and others added 4 commits September 16, 2026 13:25
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…clock seam

- GateCadenceSeconds / FeeWindowCadenceSeconds newtypes on
  ClaimEngine::with_persisted_fee_window so transposing the two cadence
  values at the driver.rs call site is a compile error, not a silent
  money defect (#3336).
- run_claim_driver_in_with_clock: the production body with the clock
  parameterized instead of hardcoded to unix_now_seconds, so the timing
  behaviour is testable under tokio::time::advance. run_claim_driver_in
  stays a thin wrapper over it passing the real clock.

Refs #3336

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_in_with_clock

Adds the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window,
proving the F2 gate-vs-fee-window distinction through the actual
production body (run_claim_driver_in_with_clock) rather than a
hand-assembled drive() call, so a transposition of the two cadence
arguments at with_persisted_fee_window's call site (#3336) is caught
end-to-end.

Refs #3336

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d force-pushed the fix/3336-claim-driver-newtypes branch from 36ef55f to 46d803a Compare September 16, 2026 21:12

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent correctness review at head 46d803a3f081b652f569d383454380edef68ec64.

Verdict: PASS

Answers to the five required questions:

  1. Does the newtype make positional transposition impossible? Yes for the case the ticket describes (two adjacent bare u64s at the call site) — GateCadenceSeconds and FeeWindowCadenceSeconds are distinct types, so passing one where the other is expected is a compile error; a caller cannot silently swap the positions anymore. It does not make a value swap impossible — GateCadenceSeconds(cfg.cadence_seconds) / FeeWindowCadenceSeconds(cadence_seconds) type-checks fine (both tuple fields are pub, both wrap u64). The PR's own doc comments on both types say this explicitly, so this isn't a hidden gap — it's a stated limitation.

  2. Is there a behavioural test that fails if the values are swapped inside the correctly-typed wrappers? Yes — the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window (driver.rs, new in this diff) drives the actual production call site (run_claim_driver_in_with_clock → the one with_persisted_fee_window call in driver.rs) through 3 ticks and asserts both halves of the defect: (a) the gate must open on the clamped cadence (cycle count == 2 at tick 2, not stalled — this is the #3306 no-op-gate regression check), and (b) the fee window must NOT roll until the raw cadence has elapsed (tick 2 must show the same fee_window_start_unix as tick 1). Reverting either wrapped value to the other's source (GateCadenceSeconds(cfg.cadence_seconds) or FeeWindowCadenceSeconds(cadence_seconds)) flips one of these two assertions. This is a real, non-vacuous proof test — it exercises production code, not a hand-rolled harness only.

    There's also a pre-existing hand-assembled version of the same property (the_gate_tracks_the_clamped_cadence_while_the_fee_window_tracks_the_raw_one, referenced in the new test's doc comment) that predates this PR and still exists; the new test's value is that it goes through the actual driver call site instead of reconstructing ClaimEngine by hand.

  3. Is run_claim_driver_in still a faithful thin wrapper? Yes. Diff shows run_claim_driver_in now does nothing but call run_claim_driver_in_with_clock(..., unix_now_seconds) — the exact same real-clock function it inlined before. No behavioural change for the only production caller (run_claim_driver).

  4. Other call sites of with_persisted_fee_window silently baked to the wrong value? Checked every call site in the diff (all in driver.rs and engine.rs test modules) plus a repo-wide code search (with_persisted_fee_window matches only these two files) — all updated consistently to GateCadenceSeconds(cadence)/FeeWindowCadenceSeconds(cadence), and in every test both values happen to be the same constant (CADENCE_SECONDS) so none of those tests could have distinguished a swap anyway — that's fine since the new dedicated test does distinguish it.

  5. Anything beyond the type change alters claim/fee behaviour? No. The only semantic addition is the clock-injection seam (run_claim_driver_in_with_clock) used solely to make the new test possible; production path is untouched (see Q3). No other logic changed.

CI: all required checks green (build, clippy, rustfmt, CodeQL, Analyze x2, Test+coverage, packaging). No pending/stale contexts.

Threads: none opened — no defects found; nothing to gate on. 0 open, 0 resolved (none needed).

KG: NONE — this PR closes a previously-logged defect (#3336) with the remedy already specified by the ticket; no new pattern surfaced.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate leg 3 of 3 (adversarial) — CHANGES-REQUIRED

Head judged: 46d803a3f081b652f569d383454380edef68ec64. Read from GitHub only.

The finding: the new test is green under the very defect it names

drive increments the cycle counter unconditionally, once per scheduler tick, after run_cycle returns — regardless of whether the restart-safety gate refused (driver.rs:132, called from driver.rs:168-172; run_cycle's gate does return Vec::new() at engine.rs:376-379). So handle.cycles_driven() counts sleeps, not cycles that ran. Every cycles_driven() assertion in the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window (driver.rs:1399) is below the decision it claims to test, and its message at driver.rs:1442

"a transposed call ... would gate on the raw 5_184_000s cadence instead and this cycle would never run"

— is false: under the transposition that tick still records, and the count is still 2.

Worse, the window half is masked too. Trace the test with the value-transposed call site (gate = cfg.cadence_seconds = 5_184_000, window = cadence_seconds = 2_678_400), noting the gate returns before the window-roll block (engine.rs:373-391):

  • tick 1, now=2_678_400: no last_completed_at → gate skipped; window opens at 2_678_400. after_tick_1 assertion passes.
  • tick 2, now=5_356_800: gate 5_356_800-2_678_400 = 2_678_400 < 5_184_000refuses and returns before the roll. Window unchanged. after_tick_2 == after_tick_1 passes (driver.rs:1450), cycles_driven()==2 passes.
  • tick 3, now=8_035_200: gate 5_356_800 >= 5_184_000 → runs; window 5_356_800 >= 2_678_400 → rolls. after_tick_3 != after_tick_1 passes, cycles_driven()==3 passes.

The whole test passes under the transposition. The gate's early return is exactly what hides the window defect from it. Checking the other wrong configurations: clamped-for-both is caught (rolls at tick 2); raw-for-both also passes undetected. The test catches one of three wrong wirings and not the one it was written for.

Answers to the five questions

  1. Value swap — not closed. GateCadenceSeconds(cfg.cadence_seconds) / FeeWindowCadenceSeconds(cadence_seconds) compiles and, per the trace above, is green under the new test. The fix converted an invisible hole into a differently invisible one. The cheapest real detector already exists and is unused: the gate refusal is observable as ClaimLoopState::CadenceNotElapsed on handle.status(). Asserting the state at tick 2 (must NOT be CadenceNotElapsed) distinguishes correct from transposed; the cycle counter never can.
  2. pub tuple fields. The guarantee is the call site's spelling, not the type system. Both newtypes are constructible from any u64 at zero cost, and both carry the same clamp afterwards (engine.rs:214-218), so nothing about either type constrains the value it holds. It rules out an argument reorder, nothing else.
  3. Not subtractive. It adds two types and a public-in-module seam while leaving the two-value coupling at the call site fully intact. The subtractive shape is strictly safer: have sanitized_schedule (which already owns both the raw and clamped numbers) return a single ClaimCadences { gate_clamped, fee_window_raw }, and have with_persisted_fee_window take that one value. driver.rs then never names either number, and the value swap becomes unrepresentable rather than merely undetected. That is smaller than what this PR adds.
  4. The clock seam. The seam itself is sound and the reason for it (tokio::time::advance cannot move SystemTime::now()) is correct. But the test it enables asserts a property the counter cannot carry and a window-roll timing the gate's early return masks — it proves the seam is callable and the happy path holds, not the money property.
  5. Overclaim. The PR body is honest (WIP / do-not-merge). The code docs overclaim: engine.rs "transposing them at a call site is a compile error rather than a silent money defect" is true only for a positional reorder, and driver.rs:1442's assertion message asserts detection the test does not have.

What would clear this leg

Either (a) re-shape to the single named-field cadence value (preferred — closes the value swap structurally, and is subtractive), or (b) keep the newtypes and make the test actually falsify the transposition: assert ClaimLoopState per tick (tick 2 must not be CadenceNotElapsed), and add a case that separates raw-for-both from the correct wiring. The shape question alone would be a follow-up; the test gap blocks this merge, because as it stands the PR's central claim — "no test detected it" — is still true at this head.

Verdict: CHANGES-REQUIRED at 46d803a3.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security verdict — GATE LEG 2/3

PASS

Head SHA judged: 46d803a3f081b652f569d383454380edef68ec64

Q1 — can the guard be forged?

Partially, but not in a way that reintroduces the #3336 defect today. GateCadenceSeconds(pub u64) /
FeeWindowCadenceSeconds(pub u64) are plain tuple structs with public fields and no smart
constructor — nothing stops a future call site from writing GateCadenceSeconds(cfg.cadence_seconds)
(the raw value) instead of GateCadenceSeconds(cadence_seconds) (the clamped one). The newtype does
not encode "clamped vs. raw" as an invariant; it is forgeable by construction, same class as the
Acknowledged trap.

What it does guarantee, and is the only thing #3336 actually needed: the two arguments to
with_persisted_fee_window are no longer both bare u64, so a positional transposition of the
two adjacent arguments at one call site is now a compile error
(GateCadenceSeconds and
FeeWindowCadenceSeconds are distinct types). That was the historical defect (driver.rs's call
site swapping cadence_seconds and cfg.cadence_seconds), and it is closed.

I audited every call site in the diff (driver.rs production + 2 test fns, engine.rs 13 test
fns): all wrap the semantically-correct source variable into the semantically-correct type. No
call site currently forges the value. Defense-in-depth gap, not live: recommend a follow-up
ticket to make the tuple fields private with named constructors (e.g.
GateCadenceSeconds::from_clamped(..)) so a future call site can't silently reverse the intent —
but nothing in this PR is exploitable via this gap today, since there is exactly one production
construction site and it is correct.

Q2 — worst raw cfg.cadence_seconds, upper bound on the fee-window side?

cfg.cadence_seconds is floored to CLAIM_CADENCE_FLOOR_SECONDS (60s) by RewardsClaimConfig:: load_from (config.rs) but has no upper clamp before it reaches FeeWindowCadenceSeconds — this
is deliberate and documented (engine.rs field doc: "applies NO upper clamp -- a huge raw value
only widens the spend window, which is the conservative direction for spend, never the dangerous
one"). Worst case: an operator (or anything with write access to rewards-claim.json, which is
already node-owner-level trust) sets cadence_seconds to u64::MAX; the persisted fee window then
effectively never rolls, meaning the aggregate max_cycle_fee_budget_mojos cap, once exhausted,
never resets — strictly more conservative (less total spend allowed over time), not an attack
surface. It cannot be used to raise the fee ceiling; only to shrink the number of budget windows.
Attacker-without-config-write-access has no path to this value at all (it is not attacker-input —
NC-12/trusted-config boundary, not touched by this PR).

Q3 — injectable clock seam

run_claim_driver_in_with_clock is a private (non-pub) async fn in driver.rs. Its only
callers are (a) run_claim_driver_in, which hardcodes unix_now_seconds (real wall clock) as the
now closure, and (b) #[cfg(test)] mod tests in the same file. It is not re-exported from
rewards_claim::mod.rs (only handle, spawn_claim_driver_from_config, ClaimDriverRefusal,
ClaimLoopHandle are). No production path, and no path reachable by an external caller or another
crate, can substitute a clock. run_claim_driver_in — the only production caller — is unchanged
apart from delegating to the new function with the real clock; confirmed identical behavior.

Q4 — does the change alter fee ceilings / budget accounting / claim eligibility beyond the type change?

No. Diffed every call site: the two u64 values passed into with_persisted_fee_window are
byte-identical to before, just now wrapped in the two newtypes at the same argument positions with
the same .max(CLAIM_CADENCE_FLOOR_SECONDS) floor logic preserved verbatim inside
with_persisted_fee_window. sanitized_schedule's clamp-to-CLAIM_SCHEDULE_SECONDS_MAX (31 days)
logic for the gate side is untouched. This is a mechanical type-safety + testability refactor, not
a behavior change.

Q5 — secrets / keys / identity / spend path

None touched. No key material, signing, or spend-construction code appears in this diff — only
cadence/fee-window scheduling plumbing and a test-only clock injection point.

Scope audited

crates/dig-node-service/src/rewards_claim/driver.rs,
crates/dig-node-service/src/rewards_claim/engine.rs (full diff, both prod and test hunks),
plus read (not diffed, but reviewed for context) config.rs, cadence.rs, mod.rs at
46d803a3f081b652f569d383454380edef68ec64 for the clamp/floor/export chain.

Not covered: did not re-run the test suite (read-only audit); relying on the PR's own CI for
"tests compile and the new production-body test actually asserts the claimed tick counts."

KG: NONE — no new pattern beyond what #3336's own doc comments and the existing "forgeable pub
newtype" pattern already record.

MichaelTaylor3d and others added 2 commits September 16, 2026 19:52
…Cadences value

The positional-newtype shape (GateCadenceSeconds/FeeWindowCadenceSeconds as two
adjacent with_persisted_fee_window arguments) closed the positional swap but left
the value swap open -- GateCadenceSeconds(raw)/FeeWindowCadenceSeconds(clamped)
still compiled and still passed the existing test (dig_ecosystem#3336 rework).

Bundle both cadences into one ClaimCadences { gate_clamped, fee_window_raw }
value instead: there is no longer a pair of adjacent arguments for a swap to
target at the call site. Every production and test call site updated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ept by its clamp

DIG-Network/dig_ecosystem#3336: `ClaimCadences { gate_clamped: u64, fee_window_raw: u64 }`
closed the positional swap between the two cadence values but left the value swap open --
both fields were plain u64, so nothing stopped a caller writing the RAW configured cadence
into `gate_clamped` and the CLAMPED one into `fee_window_raw`. `ClampedGateCadence`'s inner
field is now private with `ClampedGateCadence::clamp(FeeWindowCadenceSeconds) -> Self` as its
only constructor, so a raw value has no path into the gate slot without going through the
clamp. `FeeWindowCadenceSeconds` wraps the raw side so the two are no longer interchangeable
plain integers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread crates/dig-node-service/src/rewards_claim/driver.rs Outdated
Comment thread crates/dig-node-service/src/rewards_claim/driver.rs Outdated

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES-REQUIRED — head 68f4101d5e9a1d8f5ae4cef94450acea96360adb

Re-gate leg 1 of 3 (correctness), fresh context, read from GitHub at this head only. Prior verdicts on 46d803a3 treated as void.

Verdict in one line

The reshape genuinely closes two of the three swap hazards structurally, and the third — the one that costs money — is closable in this same PR with a provably behaviour-identical one-value collapse. On a money surface guarded by exactly one test, that residual is worth closing rather than documenting.

What the diff actually proves (state of each hazard at this head)

hazard status
positional transposition at driver.rs:511-517 CLOSED structurally — named fields + distinct types; a swap is E0308.
RAW value reaching the gate CLOSED structurallyClampedGateCadence::clamp re-applies min(CLAIM_SCHEDULE_SECONDS_MAX), so gate_clamped is clamped whichever source is fed. This is stronger than the PR body claims.
CLAMPED value reaching fee_window_raw OPEN — compiles; halves the window; guarded by one test. See the inline thread at driver.rs:515.

The five questions, answered

1. Is the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window above the gate decision, and is the pairing sound?
Yes, it is above the decision: it drives run_claim_driver_in_with_clock, the real production body, drive calls now() exactly once per iteration (driver.rs:168), the injected clock advances one clamped interval per tick, and the engine's gate at engine.rs:412-418 and window-roll at engine.rs:421-429 are both reached through it. The arithmetic checks out: tick 2 gives now - last_completed = 2_678_400 >= gate 2_678_400 (gate opens) and now - window_start = 2_678_400 < 5_184_000 (window holds); tick 3 gives 5_356_800 >= 5_184_000 (rolls).
The pairing is sound at this head but weaker than its own comment states: assert_ne!(state, CadenceNotElapsed) is satisfied by any other state, including PersistedStateCorrupt, whose early return (engine.rs:389-392) also sits above the roll block. Nothing makes tick 2 corrupt or future-dated today, so it holds — but it is one added fail-closed guard away from vacuous. Inline thread at driver.rs:1473.

2. Other call sites of with_persisted_fee_window with a wrong source baked in?
No. Repo-wide the symbol appears only in rewards_claim/driver.rs and rewards_claim/engine.rs. One production call site (driver.rs:511). Every other call is a test; driver.rs:1092 and driver.rs:1405+ pass effective/configured correctly and deliberately; all engine.rs tests pass CADENCE_SECONDS for both, which is intentional for engine-level properties and is not a wrong-source bake-in.

3. Does ClampedGateCadence::clamp clamp correctly, and is the field genuinely private?
Yes on both. clamp is raw.0.min(CLAIM_SCHEDULE_SECONDS_MAX) against the same 31 * 24 * 60 * 60 bound sanitized_schedule uses (driver.rs:394-407); CLAIM_SCHEDULE_SECONDS_MAX was widened to pub(crate) only, not pub. The field is a private tuple field; derives are Clone, Copy, Debug, PartialEq, Eq only — no Default, no From, no Deref, no AsRef, no serde. seconds() is a private reader. clamp is the only constructor, and grep confirms no ClampedGateCadence(...) literal anywhere outside clamp itself — including in the child mod tests, which could bypass it (child modules see private fields) but does not.

4. Anything changing claim/fee behaviour beyond the type reshape?
No. run_claim_driver_in becomes a thin wrapper passing unix_now_seconds into run_claim_driver_in_with_clock; the production body is otherwise character-identical. The extra clamp applied to an already-sanitized cadence_seconds is a no-op. The .max(CLAIM_CADENCE_FLOOR_SECONDS) flooring on both fields is unchanged. The only visibility widening is CLAIM_SCHEDULE_SECONDS_MAX to pub(crate), needed by clamp.

5. Is the PR body accurate relative to what the diff proves?
Partly, and it now understates the diff — it still describes the pre-reshape framing ("two adjacent bare u64 args"), says "Work in progress", and does not record that the gate half is now structurally closed or that the window half is not. Two smaller issues: the title still says "newtype ... args", which no longer describes the shape (one struct, one of whose fields is constructor-pinned); and Fixes #3336 resolves against dig-node, not dig_ecosystem — a cross-repo close needs DIG-Network/dig_ecosystem#3336, otherwise it is either a no-op or points at the wrong issue. Refs #3336 below it has the same ambiguity. Non-blocking, but fix before ready-for-review.

Test-vacuity gate

  • Property: "the production call site threads the CLAMPED cadence to the gate and the RAW cadence to the fee window."
  • Nearest wrong implementation: fee_window_raw: FeeWindowCadenceSeconds(cadence_seconds). Input that fails it: a 60-day config (5_184_000) — the window rolls at tick 2 instead of tick 3. Only the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window goes red; the sibling money test stays green. Non-vacuous, but a guard of ONE.
  • Second nearest wrong implementation: gate_clamped built from the raw source — no longer producible, clamp neutralises it. Correctly, no test is needed for it.

What I did NOT run

No build, no cargo test, no cargo clippy, no coverage run, and no mutation of my own — read-only from git objects at this head, per brief; I did not read or touch D:\worktrees\dig-node-3336. I did not re-derive the mutation measurements in the brief; I sanity-checked them against the code and they reconcile (including the Some(5356800) vs Some(2678400) 2x figure). Someone must confirm the suite is green at this head before merge — note this PR is still a DRAFT and its body says DO NOT MERGE.

Threads

2 inline threads posted, both blocking-or-above-note, both OPEN by design: driver.rs:515 (blocking) and driver.rs:1473 (medium). No pre-existing threads existed; none resolved. Re-review on the next head.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

ADVERSARIAL GATE (leg 3/3) — CHANGES-REQUIRED at 68f4101d5e9a1d8f5ae4cef94450acea96360adb

Head read: 68f4101d5e9a1d8f5ae4cef94450acea96360adb. Checks at that SHA: Clippy / Rustfmt / CodeQL / all builds green; Test + coverage in progress.

The shape is not re-litigated — the orchestrator's ruling (one test is the Site B guard) stands, and I agree with it, for a reason recorded under item 5. What blocks is narrower and, on a money surface, decisive: three doc claims state a type-level guarantee this diff does not deliver, and they are exactly the text a future refactorer will read when deciding the one guard test is redundant.


1. Is the one test durable? — Load-bearing and sound, but NOT what catches the measured mutation

Traced both halves against the gate at engine.rs:412-417 and the window roll at engine.rs:423-431, with the test clock at t = 2 678 400 / 5 356 800 / 8 035 200.

The measured Site B mutation does not touch the gate at all. Swapping the sources inside ClaimCadences yields gate_clamped = clamp(cfg.cadence_seconds) = clamp(5_184_000) = 2_678_400 — byte-identical to the correct value, because clamp is idempotent over an already-clamped input. Only fee_window_raw changes (5 184 000 to 2 678 400, halved). That is precisely your left: Some(5356800), right: Some(2678400) at tick 2, and it is caught entirely by the window assertion, not by the state assertion.

So the pairing is:

  • Sound. ClaimLoopState::CadenceNotElapsed is set on exactly the refusal path (engine.rs:415) and recomputed on every completing cycle (engine.rs:637); it cannot go stale across ticks. Tick 3's assert_ne!(after_tick_3, after_tick_1) additionally forecloses the "nothing ever ran" degeneracy: an inert loop never rolls the window, so tick 3 goes red. The test is not one refactor away from being silently below-the-decision in the inert direction.
  • Not load-bearing for the defect it is advertised against. The gate half is now enforced by the type, not by the test: gate_clamped has no constructor that skips the clamp, and .max(CLAIM_CADENCE_FLOOR_SECONDS) at engine.rs:250-253 floors it, so no source swap can make the gate larger than the clamped schedule. The state assertion is a tripwire against a future change that reintroduces an unclamped gate path — real value, but defence in depth, not the guard.

One genuine weakness, and the only test change I want: the assertion is an exclusion (!= CadenceNotElapsed), so it is also satisfied by PersistedStateCorrupt and ChainSourceUnavailable — both of which likewise return before the window-roll block. A refactor that makes write_config's output fail RewardsClaimConfig::load_from (a new required field, a serde tightening) flips this cycle to PersistedStateCorrupt and the tick-2 pair goes green for the wrong reason. Tick 3 still catches it, but tick 2 should assert the exact state a healthy no-work cycle computes rather than excluding one variant. One line.

2. Is ClaimCadences's asymmetry coherent? — Coherent, but one naming choice actively invites the swap

The private/pub split is defensible: one side has a clamp invariant to protect, the other has none. What is not coherent is that FeeWindowCadenceSeconds serves double duty — it is both the type of the fee-window field and the parameter type of ClampedGateCadence::clamp. The production line at driver.rs:513 reads:

gate_clamped: ClampedGateCadence::clamp(FeeWindowCadenceSeconds(cadence_seconds)),

A type named "fee window cadence" being fed into the gate constructor is the exact affordance that makes the Site B swap natural to write and unremarkable to read. Rename it to what it is (RawConfiguredCadence; the fee-window field keeps that type, because the window genuinely is sized by the raw value). Mechanical, no shape change, removes the misreading at the call site.

3. Did the rework move the hole or close it? — It closed the positional hole and moved the value hole into the documentation

This is the recurrence, and it is item 4's answer too.

  • engine.rs, ClampedGateCadence doc: "A raw value literally has no constructor to reach this type through." False as written. clamp(FeeWindowCadenceSeconds(raw)) is a raw value reaching the type, and it is the production call site. The true invariant is narrower: no value reaches this type without the clamp applied — the source is not pinned.
  • engine.rs, ClaimCadences doc: "plain u64 fields still let the wrong VALUE reach either name ... Typing the fields ... closes that too." It does not. Your own measurement says Site B still type-checks. As written, this doc tells the next reader the value swap is type-closed — which is the precise justification someone will use to delete the only test that guards it.
  • engine.rs, clamp doc: "the same bound sanitized_schedule applies at the config read." Not the same bound: sanitized_schedule substitutes CLAIM_CADENCE_SECONDS_DEFAULT for a zero cadence (driver.rs:380-394), it does not clamp it. Harmless at today's call site (the input is already sanitized); load-bearing the moment someone feeds clamp a raw config value on the strength of this sentence.

Must-fix before merge. Each doc must state what is closed (positional transposition — now E0308; an unclamped value in the gate slot — no constructor) and, in the same breath, what is not (which source fills which field), naming the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window as the sole guard so the next reader cannot delete it by accident.

4. Does the PR body overclaim? — The body, no. The doc comments, yes.

The PR body is DO NOT MERGE / work in progress and claims nothing. The overclaim is entirely in the doc comments, itemised above. Nothing in the diff says "transposition is now impossible" in those words, but the ClampedGateCadence sentence reads that way to anyone who does not re-derive the constructor signature — which is the entire population the doc exists for.

5. Is the remedy subtractive? — No, and it is worth saying plainly

Net: minus two positional params; plus three types, two methods, one production function (run_claim_driver_in_with_clock), and roughly 14 call sites each expanded from one line to seven — about +120 lines of test boilerplate for a two-argument problem. The obviously smaller shape is one constructor, ClaimCadences::from_config(raw), deriving both fields internally: it would collapse all 14 sites to one argument and close Site B outright.

I checked whether that is actually available, and it is not cleanly. sanitized_schedule is not clamp: it substitutes the default for a zero cadence (driver.rs:380-394) rather than clamping it, so from_config would have to duplicate that substitution branch or re-derive it — and a second copy of that rule on a money path is worse than the boilerplate. The orchestrator's "do not stretch the type further" ruling is correct, and I am recording the reason so this is not reopened a third time. The PR is mergeable without the shape change.

One more, unasked (minor, non-blocking)

mod.rs:63 re-exports ClaimEngine but not ClaimCadences / ClampedGateCadence / FeeWindowCadenceSeconds. ClaimEngine::with_persisted_fee_window is therefore a pub method whose argument type cannot be named outside the crate — it was callable before this PR (two u64s) and is not now. No out-of-crate caller exists (only engine.rs and driver.rs) and Clippy is green, so this is a latent API wart, not a break. Either re-export the three types or narrow the method to pub(crate).


Verdict

CHANGES-REQUIRED at 68f4101d5e9a1d8f5ae4cef94450acea96360adb.

I would not accept a follow-up ticket in place of these. They are four small edits, three of them doc-only, and the reason they cannot be deferred is that the defect they cause is the deletion of the guard test — a follow-up ticket does not run before the next person reads the doc. Land them here:

  1. ClampedGateCadence doc — replace "no constructor to reach this type" with the clamp-applied-but-source-not-pinned invariant; name the guard test.
  2. ClaimCadences doc — stop claiming field typing closed the value swap; state that the source swap type-checks and is guarded by that one test.
  3. clamp doc — drop "the same bound sanitized_schedule applies"; note the zero-cadence substitution divergence.
  4. Tick-2 assertion in the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window — assert the exact healthy state instead of != CadenceNotElapsed.

Optional in the same pass, recommended and mechanical: rename FeeWindowCadenceSeconds to RawConfiguredCadence.

Re-gating the new head is a read of those four spots only; the shape and the money property are settled. Being the epic's critical path does not change this verdict — it does mean the fix is about 20 minutes, not a round trip.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security (L2) — RE-GATE leg 2 of 3 · PASS

Head audited: 68f4101d5e9a1d8f5ae4cef94450acea96360adb (and only that head). Base develop, merge-base 6954bbe0. Diff vs base: 636 lines across exactly two files — crates/dig-node-service/src/rewards_claim/driver.rs and .../engine.rs. The prior 46d803a3 PASS is void and was not relied on.

No LIVE vulnerability. One defence-in-depth finding, ticket-worthy, not a gate.


Q1 — Can the guard be forged? No, for the gate slot. Verified structurally, not by comment.

engine.rs:63-79:

  • pub struct ClampedGateCadence(u64); — field is private (no pub on the tuple field). Struct-literal construction from driver.rs (or anywhere outside engine.rs) is a compile error.
  • Derives are Clone, Copy, Debug, PartialEq, Eq — none of these is a constructor. No Default, no serde::Deserialize, no From/Into/Deref/AsRef/TryFrom.
  • clamp is the only associated fn producing a Self; the accessor seconds() is private.
  • Repo-wide grep at this SHA for ClampedGateCadence / FeeWindowCadenceSeconds / ClaimCadences / with_persisted_fee_window returns zero hits outside engine.rs and driver.rs. No trait impl, no second call site, no re-export (mod engine; at rewards_claim/mod.rs:51 is private, and mod.rs:63 re-exports only ClaimEngine).

The stronger property, which the PR's own doc understates: clamp is total and bounds whatever it is handed. ClampedGateCadence(raw.0.min(CLAIM_SCHEDULE_SECONDS_MAX)). A future careless edit writing ClampedGateCadence::clamp(FeeWindowCadenceSeconds(cfg.cadence_seconds)) (the raw value) still yields a gate value <= 31 days. So the "restores the no-op gate #3306 fixed" half of #3336 is now structurally unreachable, not merely test-guarded. That is the material difference from the pub struct Acknowledged; trap: Acknowledged was a zero-field unit type any caller could conjure; ClampedGateCadence carries a value only a clamping function can produce.

Residual, named honestly: privacy in Rust is module-scoped, so a future edit inside engine.rs could write ClampedGateCadence(x) as a struct literal. engine.rs is where the invariant and its doc live, so that is the right blast radius, but it is not zero.

FeeWindowCadenceSeconds(pub u64) being pub-fielded is the intended asymmetry — it is the source type clamp clamps from, and the driver must construct it from cfg.cadence_seconds. It cannot be used to launder a value into the gate slot: ClaimCadences.gate_clamped is typed ClampedGateCadence, so the only path from a FeeWindowCadenceSeconds into that slot is through clamp, which bounds it. Confirmed correct.

Q2 — Worst raw cfg.cadence_seconds, and the direction. Larger raw = LONGER window = FEWER resets = LESS spend. The missing upper bound is safe.

engine.rs:423-425: window_still_open = start.is_some_and(|s| now.saturating_sub(s) < self.fee_window_seconds); engine.rs:426-430 rolls the window (spent_mojos = 0) only when it is not still open. fee_window_seconds is therefore the length of the budget window. A larger value holds one window open longer → the aggregate spend accumulator resets less often → strictly less fee spend per unit time. The direction is unambiguous, and it is the safe direction.

Worst value is u64::MAX: the window never rolls, spent_mojos accumulates monotonically, and once the aggregate budget is exhausted the node stops submitting claims permanently. That is fail-CLOSED — a liveness/rewards-forgone footgun for the operator who wrote it, not spend amplification, and not reachable by anyone but that operator. saturating_sub at engine.rs:414 and :425 means no overflow panic. The dangerous direction (small) is floored twice: CLAIM_CADENCE_FLOOR_SECONDS at the config read and again by .max(CLAIM_CADENCE_FLOOR_SECONDS) at engine.rs:250-257, so cadence_seconds = 0 yields a 60s window against a gate substituted to the 1-day default — no amplification, since run_cycle is invoked once per scheduler interval regardless.

Q3 — Is one test an acceptable control for a 2x fee-ceiling defect? For the gate half the test is now redundant (the type carries it). For the fee-window half, no — open a follow-up ticket. Not a gate.

The remaining swap this shape does not close: fee_window_raw: FeeWindowCadenceSeconds(cadence_seconds) — the already-clamped local — type-checks at driver.rs:515 exactly as well as the correct cfg.cadence_seconds. For a 60-day operator config that shortens the window from 5_184_000 to 2_678_400: 2x the windows, 2x the fee ceiling — the exact defect measured on this epic, still expressible in a one-token edit. What stands between that edit and production is behavioural coverage only: driver.rs:1417 the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window (production body, injected clock) and driver.rs:1077 the_gate_tracks_the_clamped_cadence_while_the_fee_window_tracks_the_raw_one (hand-assembled drive). Two tests, not one, and the production-body one reaches the real call site — which is why this is not a gate.

It is still the weaker leg of an asymmetric pair, and the epic's own lesson ("an incomplete money type prescribes the banned defect") applies to it. Recommended follow-up ticket (do not block this PR): give the clamped local a type that cannot be spent as a raw window length — e.g. have sanitized_schedule return ClampedGateCadence directly so no bare u64 clamped cadence exists in driver.rs scope at all, with fee_window_raw constructible only from cfg.

Q4 — Anything beyond the reshape? No.

Every hunk is accounted for: the use line (driver.rs:41); CLAIM_SCHEDULE_SECONDS_MAX widened private → pub(crate) (a read-only constant, crate-internal, value unchanged at 31*24*60*60); the run_claim_driver_in_with_clock seam; the call-site reshape at driver.rs:511-517; the with_persisted_fee_window signature at engine.rs:248; tests. Fee ceilings, budget arithmetic, per-claim caps, ordering and claim eligibility are unchanged — the two .max(CLAIM_CADENCE_FLOOR_SECONDS) floors are preserved verbatim on both fields.

Q5 — Secret / key / identity / spend path? Checked, not assumed: none touched.

No key material, signing, wallet, identity or credential code appears anywhere in the 636-line diff. own_payout_puzzle_hash is threaded unchanged. The diff influences when a spend-submitting cycle may run and how long its fee-budget window lasts, never what is signed, by whom, or to where. The injected-clock seam run_claim_driver_in_with_clock (driver.rs:472) is not pub and has no production caller other than run_claim_driver_in, which passes the real unix_now_seconds — there is no reachable "attacker supplies the clock" surface. Persisted state (RewardsClaimConfig) remains untrusted at rest: corrupt and future-dated reads still fail closed at engine.rs:390-393, before the gate and before any window roll.


Scope audited: the full develop...68f4101d diff (2 files, 636 lines), the complete current text of driver.rs and engine.rs at that SHA, and a repo-wide grep at that SHA for every reference to the three new types. Not covered: I did not execute the test suite (no build or test run) and did not evaluate rewards_claim money logic outside the hunks. No shared checkout was touched; D:\worktrees\dig-node-3336 was neither read nor written. Read-only throughout.

MichaelTaylor3d and others added 3 commits September 17, 2026 09:03
…value

Closes the remaining half of the #3336 transposition hole structurally rather
than by test. `ClaimCadences` now has private fields and a single constructor,
`ClaimCadences::from_raw`, which derives `gate_clamped` from the same raw value
it stores as `fee_window_raw`. The production call site passes ONE value, so
there is no longer a pair to transpose or a `pub` field to write the wrong
local into.

Behaviour-identical: `CLAIM_CADENCE_FLOOR_SECONDS` is 60 and `config.rs` floors
any smaller configured cadence at load, so `sanitized_schedule`'s zero branch is
unreachable from this path and its output equals `min(cadence, MAX)` -- exactly
what the clamp computes.

Also renames `FeeWindowCadenceSeconds` to `RawConfiguredCadence` (it is the
clamp's input type, not only the window's), and replaces the tick-1/tick-2
`assert_ne!(state, CadenceNotElapsed)` exclusions with exact-state assertions --
the exclusion was equally satisfied by PersistedStateCorrupt and
ChainSourceUnavailable, whose early returns also sit above the window-roll
block, so it would have gone vacuous the moment either fired.

Refs DIG-Network/dig_ecosystem#3336

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er assertions

Convert literal backslash-n escapes with trailing spaces back to proper Rust string
line continuations (trailing backslash at end of line). The strings render as single
sentences without artificial newlines or multi-space runs. Fixes continuation_guard.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…arded half

The #3336 doc comments asserted a type-level guarantee the code does not
deliver. `ClaimCadences::from_raw` does close the pairing mutation -- the gate
and the fee window cannot disagree with each other, because `from_raw` derives
`gate_clamped` from the same `RawConfiguredCadence` it stores as
`fee_window_raw`, and it is the only constructor. It does NOT close the value
mutation: `from_raw(RawConfiguredCadence(cadence_seconds))`, the already-clamped
local instead of `cfg.cadence_seconds`, has the same type and compiles. Measured
consequence: the fee window halves -- the tick-2 assertion fails with
left Some(5356800), right Some(2678400).

That mutation is caught by exactly one test,
`the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window`. The other
money test stays green under it, because it hand-assembles `drive` and never
traverses the production call site. The old docs told the next reader the value
swap was type-closed -- the exact sentence that would be cited to delete the one
guarding test.

Also corrects the stale "transposing the two arguments" description: the call
takes ONE argument, so that mutation cannot be written at all; and drops the
claim that the surviving mutation also breaks the gate (clamping an
already-clamped value is the identity, so it does not).

Docs only -- no behaviour, no type, and no assertion changed.

Refs DIG-Network/dig_ecosystem#3336

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Adversarial gate leg 3/3 — CHANGES-REQUIRED @ 97450a0b52b10020cb3ce3a0194a8ed31492652c

Read at that head only; prior verdicts (46d803a3, 68f4101d) void. One required change, doc-only, ~8 lines. Everything else on this head PASSes and I say so explicitly below so it is not reopened a fourth time.

REQUIRED — the hole moved a third time, and it landed in the same file's own method doc

crates/dig-node-service/src/rewards_claim/engine.rs:228-235, the F2 (money) paragraph on with_persisted_fee_window:

/// F2 (money): takes TWO cadence values, deliberately not one -- \gate_cadence_seconds` (the CLAMPED value …) gates WHEN a cycle is allowed to start; `fee_window_seconds` (the RAW configured value) sizes how long the persisted fee-budget window stays open.`

At this head the method takes one ClaimCadences (engine.rs:269), and gate_cadence_seconds / fee_window_seconds are no longer parameters at all — they are private fields. So this paragraph is false in its subject and, worse, normative in the wrong direction: the word deliberately tells the next reader that the two-value signature is the intended shape. That is an argument for re-expanding the argument list back to two u64s — precisely the shape whose positional transposition this PR exists to make unwritable. It sits ~20 lines ABOVE the new # …#3336 section that contradicts it, in a long doc block read top-down, on the one method #3347 must touch next. The intra-doc links still resolve (the fields exist), so rustdoc catches nothing.

This is the round-2 defect class exactly: the remedy relocated the claim rather than closing it. The PR body asserts the opposite — "the doc comments in this PR now say so explicitly at each of the four sites that used to imply otherwise" and lists "still described 'transposing the two arguments'" as fixed. Four of five were fixed; this fifth survives. That is the only overclaim I found (Q5).

Fix: delete lines 228-235 outright. The #3336 section below it plus the ClaimCadences doc (engine.rs:14-49) already carry the whole money argument, both halves, more accurately. Deleting holds; re-wording invites a third version of the same sentence.

RECOMMENDED, same commit — block the deletion argument where the reader arrives

driver.rs:1047-1068, the sibling money test's doc, is silent about its own blind spot. It says it is "Driven through the REAL production loop, drive" and that it proves the gate/clamped vs window/raw split. Literally true, and it does not claim call-site coverage — but nothing there warns that it stays green under from_raw(RawConfiguredCadence(cadence_seconds)). A reader arriving at the older, better-named, more-cited test can cite that doc to delete the new one as duplicative (same config, same three ticks, same assertions). The new test defends itself; the sibling does not point back. One sentence: "This test does NOT traverse the production call site and stays GREEN under the raw/clamped mutation there — the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window is the only guard for that; do not treat this as covering it."

The four questions, answered

Q2 — is the one test durable? YES, and the exclusion fix is what makes it so. assert_ne!(state, CadenceNotElapsed)assert_eq!(state, Nominal) converts every vacuity vector I can construct into a loud failure rather than a silent pass:

  • a new fail-closed early return above the window-roll block (PersistedStateCorrupt, ChainSourceUnavailable, or a future sibling) now fails the state assertion instead of satisfying the exclusion while the window assertion passes for the wrong reason;
  • a drive refactor changing how many times now is called per cycle shifts the injected clock's timeline and breaks the tick-2/tick-3 pair loudly (2 calls/cycle ⇒ window rolls at tick 2 ⇒ after_tick_2 == after_tick_1 fails);
  • a serde tightening or schema change that made write_config's cadence not land would make the driver sleep on the default 86 400s while the test advances 2 678 400s — cycles_driven() reads ~31, not 1. Loud.
  • EmptyPort short-circuiting before the window-roll block in a future refactor ⇒ tick-3 assert_ne fails. Loud.
    The one residual is structural, not fixable by a test: it is a single test. The doc at engine.rs:32-38 names it and says "Do not delete it on the belief that a type stands behind it -- nothing does." That is the right mitigation and it is present.

Q3 — is the derivation honest? YES. I agree with your measurement; I withdraw my earlier objection. sanitized_schedule (driver.rs:378-410) is, for every nonzero input, exactly min(raw, CLAIM_SCHEDULE_SECONDS_MAX) — byte-for-byte what ClampedGateCadence::clamp computes. The only divergent branch is cadence_seconds == 0, which substitutes CLAIM_CADENCE_SECONDS_DEFAULT where clamp would yield 0-then-floored-60. It is unreachable from this path: RewardsClaimConfig::load_from floors cadence_seconds to CLAIM_CADENCE_FLOOR_SECONDS (60) at config.rs:232-239, and the call site reads cfg.cadence_seconds from that same load_from (driver.rs:478, driver.rs:511). Both the floor and the zero branch have their own regression tests (config.rs:449-465; sanitized_schedule's own). Closed — not to be reopened. The one thing I would note if the fix above is being made anyway: the reachability argument lives in config.rs and in sanitized_schedule's doc, not at ClampedGateCadence::clamp, whose closing clause "so the gate tracks the schedule the driver really sleeps on" is stated unconditionally. Optional half-sentence, not a gate item.

Q4 — subtractive? YES. −35 net, two newtypes removed, the two-argument shape structurally gone, no new field on ClaimEngine (the module's standing shape rule held), pubpub(crate) correctly justified. The pub(crate) on CLAIM_SCHEDULE_SECONDS_MAX is the only widened visibility and it is required by clamp.

Q1/Q5 answered above. Type-enforced/test-guarded split as stated is accurate: I re-derived it and both halves hold — from_raw is the sole constructor, both fields private, so gate and window cannot disagree; which u64 is labelled raw is test-guarded only.

Not a merge blocker; merge after the doc deletion

No follow-up ticket. driver.rs/#3347 are blocked behind this and the fix is a delete of 8 doc lines in a file already open — a follow-up ticket for a doc line in the exact file #3347 rewrites would be subsumed and lost, which is how this class of claim survived twice already. Land it here.

Failure direction if I am wrong to hold this: ~10 minutes on the epic's critical path. Failure direction if it merges as-is: the method's own doc instructs the next author to restore the two-u64 signature, the transposition becomes writable again, and the money defect returns with all 13 checks green.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security (re-gate leg 2/3) — PASS

Head audited: 97450a0b52b10020cb3ce3a0194a8ed31492652c

Scope: the only two files in the PR at head — crates/dig-node-service/src/rewards_claim/driver.rs (+193/-15) and .../engine.rs (+166/-26). (.claim-driver-newtypes-stub.md was added and deleted within the branch; it is absent at head.) Read from GitHub at that SHA; D:\worktrees\dig-node-3336 untouched; no shared checkout mutated.

Prior PASSes on this PR (incl. 46d803a3) treated as void; this verdict is derived from scratch.

1. Can the guard be forged? — No in-crate path yields a ClaimCadences whose halves disagree

Enumerated at this SHA:

  • Struct literals: ClaimCadences' fields gate_clamped / fee_window_raw (engine.rs:48-49) are private, so a literal is legal only inside engine.rs and its child mod tests. The only ClaimCadences { … } literal in the crate is inside from_raw itself (engine.rs:59-62). engine.rs's own test module builds the type 16 times and every one goes through from_raw.
  • Conversions / derives: no From, Into, TryFrom, Deref, AsRef, Default, Serialize/Deserialize or any other impl for any of the three types. Derives are exactly Clone, Copy, Debug, PartialEq, Eq (engine.rs:46, 69, 81) — none can manufacture a mismatched pair. impl blocks on these types: exactly two (impl ClaimCadences engine.rs:52, impl ClampedGateCadence engine.rs:84).
  • ClampedGateCadence: field private, clamp private-to-module, sole caller from_raw (engine.rs:60).
  • RawConfiguredCadence(pub(crate) u64) (engine.rs:70): any in-crate caller can mint a Raw holding any u64 — but a Raw alone cannot produce disagreeing halves, because from_raw derives gate_clamped by clamping that same value. Disagreement is unrepresentable through the crate-visible surface.

Contrast with the pub struct Acknowledged; trap: that was forgeable in one line from any consumer. This is not — private fields plus a second half derived from the first close it. Residual: code written inside engine.rs or its mod tests could hand-write a mismatched literal. That is an author-error surface reachable only by editing the defining module, not an attacker path. Not a finding.

The one degree of freedom no type can close — WHICH u64 the call site labels raw — is correctly documented at engine.rs:26-34 and lives at exactly one production call site, driver.rs:511-514. See item 3.

2. The unbounded raw cadence — the direction is the SAFE one

fee_window_raw is the window's length: engine.rs:447-449 window_still_open = now.saturating_sub(start) < self.fee_window_seconds; on !open, start = now; spent_mojos = 0 (engine.rs:450-454). So larger raw ⇒ longer window ⇒ fewer spent_mojos resets ⇒ LESS spend per unit time. The missing upper bound throttles the operator; it cannot raise the ceiling. u64::MAX ⇒ the window never rolls ⇒ zero further spend after the first budget (a liveness/honesty condition the status surface names, not a money leak).

The dangerous direction is downward, and two independent guards cover it: RewardsClaimConfig::load_from floors cadence_seconds to CLAIM_CADENCE_FLOOR_SECONDS (60) at the read (config.rs:232-239), and with_persisted_fee_window re-floors with .max(CLAIM_CADENCE_FLOOR_SECONDS) (engine.rs:277-280). fee_window_seconds == 0 — which would roll the window every cycle, i.e. an unbounded fee ceiling — is therefore unreachable from either path.

Arithmetic: saturating_sub at engine.rs:437 and :448, saturating_add/saturating_sub for spend at :825/:844/:885, min/max for the clamps. No wrapping, no bare +/-, no division. No overflow reachable from an operator-writable u64.

Gate side: gate_clamped = min(raw, CLAIM_SCHEDULE_SECONDS_MAX).max(60)bounded above, so an unbounded raw cannot starve the gate and restore the #3306 no-op. Divergence vs the scheduler: sanitized_schedule substitutes the default for 0 and clamps > MAX down; clamp only does the min. The sole disagreement is raw == 0, unreachable past the config floor, and it would make the gate more permissive (60s) than the interval the loop sleeps on — benign, no spend above the scheduler's own rate. Defence-in-depth observation, not a finding.

3. Is ONE test an acceptable control for a 2x fee-ceiling defect? — No. Not a gate; file a ticket.

Not a LIVE vulnerability: the call site at driver.rs:511-514 passes cfg.cadence_seconds, the raw value, and is correct at this SHA. So it does not block the merge.

Why the single test is nonetheless real rather than notional (and why the earlier PASS was wrong): the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window (driver.rs:1402+) actually traverses the production body via run_claim_driver_in_with_clock, unlike the older hand-assembled test that stayed green under the mutation; and it asserts the exact ClaimLoopState::Nominal rather than != CadenceNotElapsed, which closes the vacuity route — a refused cycle returns at engine.rs:437-440, above the window-roll block, so the window assertion alone would pass for the wrong reason.

But it is a single point of failure for a 2x fee-ceiling defect. Recommended ticket (defence-in-depth, MEDIUM):

  1. Make the label structural instead of test-guarded — e.g. have sanitized_schedule return the clamped cadence as a ClampedGateCadence, so writing RawConfiguredCadence(cadence_seconds) at the call site becomes a type error; or add ClaimCadences::from_config(&RewardsClaimConfig) so the already-clamped local is never in scope at the call.
  2. Add a second, independent control that does not depend on 3-tick timing, EmptyPort's outcome or the state enum — e.g. assert fee_window_seconds == cfg.cadence_seconds through a crate-visible accessor.

4. Fee ceilings / budget accounting / claim eligibility beyond the reshape? — No

The only behavioural deltas in engine.rs are the signature and visibility of with_persisted_fee_window (engine.rs:271) plus the three new types; everything else in that file's diff is doc comment and test call sites. Untouched: cycle_fee_budget_mojos, the per-claim ceiling, spent_mojos accounting, descending-value claim ordering, the corrupt-file refusal in persist_fee_window, and the eligibility path. In driver.rs: run_claim_driver_in becomes a thin wrapper passing unix_now_seconds (driver.rs:441-447); CLAIM_SCHEDULE_SECONDS_MAX widened private → pub(crate) (crate-internal, value unchanged); two pre-existing assertions tightened assert_ne!assert_eq!(Nominal) (strictly stronger).

5. Secrets, keys, identity, spend paths — none touched; the clock seam is not reachable

No key, secret, mnemonic, signing path, puzzle-hash derivation or wire surface appears in the diff. own_payout_puzzle_hash is passed through unchanged. The only production chain port is still UnavailableClaimChainPort, so no broadcast path exists at this SHA at all.

run_claim_driver_in_with_clock (driver.rs:469) carries no visibility modifier, mod driver is private in mod.rs (which re-exports only handle, spawn_claim_driver_from_config, ClaimDriverRefusal, ClaimLoopHandle), and it has exactly two callers: the real wrapper (driver.rs:442) and the new test (driver.rs:1418). Neither an out-of-crate consumer nor a peer can inject a fake clock to force window rolls. Separately, with_persisted_fee_window narrowed pubpub(crate), which reduces exposure; repo-wide code search finds no caller outside rewards_claim/{engine,driver}.rs.

Not covered

Runtime behaviour (no build or test run here — the brief supplied 92 passed / fmt 0 / clippy 0 as measured); CI status; the correctness leg's remit; any file outside the two in this PR.

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PASS — correctness re-gate leg 1/3 at 97450a0b52b10020cb3ce3a0194a8ed31492652c

Judged this head only. Read from git objects via gh api at the SHA; no worktree touched; nothing run locally (see "what I did not run").

The five questions

1. Is the derivation behaviour-identical to the code it replaced? YES, for every input production can produce — verified independently.

  • config.rs:52CLAIM_CADENCE_FLOOR_SECONDS = 60.
  • RewardsClaimConfig::load_from has exactly three exits (config.rs:213-260): missing file → default() (cadence_seconds = CLAIM_CADENCE_SECONDS_DEFAULT = 86_400); unreadable / unparsable / F14 → poisoned() (..Self::default(), so also 86_400); parsed → floored up to 60 at config.rs:232-240. So every value of cfg.cadence_seconds reaching run_claim_driver_in_with_clock is >= 60.
  • sanitized_schedule (driver.rs:364-411) is if c == 0 { DEFAULT } else if c > MAX { MAX } else { c }. For c >= 1 that is exactly c.min(MAX), which is exactly ClampedGateCadence::clamp (engine.rs:88-90).
  • The engine-side .max(CLAIM_CADENCE_FLOOR_SECONDS) on both fields is unchanged from the old two-u64 body.

Conclusion: the gate value the engine now receives (cfg.cadence_seconds.min(MAX).max(60)) is identical to what the old call passed (sanitized_schedule(..).0.max(60)) for all reachable inputs. The zero branch is unreachable, and even if the floor were removed the divergence (gate 60 vs 86_400, while drive still sleeps on the substituted 86_400) fails in the safe direction — the gate becomes strictly more permissive, so it cannot restore the #3306 silent-non-claiming defect, and fee-window sizing is unchanged in that case (0.max(60) == 60 under both shapes). Not a behaviour change on the money path. Non-gating note posted at engine.rs:87 about the doc sentence that asserts the two transformations are "the same bound" without naming that precondition.

2. RawConfiguredCadence.0 is pub(crate) — does that reopen anything? NO.

What private ClaimCadences fields close is the pairing: gate and window sizing off two different numbers. from_raw is the only constructor (engine.rs:57-66); ClaimCadences derives only Clone, Copy, Debug, PartialEq, Eq (no Default, no Deserialize), has no setters, and no field access outside engine.rs; ClampedGateCadence::clamp is now module-private and its only caller is from_raw. So no in-crate path can produce a ClaimCadences whose gate is not the clamp of its own window value. A pub(crate) raw field lets in-crate code label an arbitrary u64 as raw — precisely the hole the doc says is test-guarded only, not a new one. Repo-wide, with_persisted_fee_window is named only in driver.rs and engine.rs, so the pubpub(crate) narrowing breaks no caller (mod engine is private to rewards_claim, and ClaimCadences is not re-exported in mod.rs:57-67).

3. Are the rewritten docs TRUE? Yes, with one over-broad sentence and one stale comment, both non-gating.

Checked every surviving guarantee claim:

  • "from_raw is the only constructor, both fields are private (so a struct literal is not an alternative path from outside this module)" — true, and correctly scoped.
  • "derives gate_clamped by clamping the very RawConfiguredCadence it stores as fee_window_raw" — true.
  • "NOT type-enforced … WHICH u64 the call site labels raw … Exactly ONE test catches that … Do not delete it" — true and confirmed: of the four tests that reach the production body, three go through run_claim_driver_in (real wall clock, so they cannot observe per-cycle now() values) and only the_production_body_tracks_the_clamped_gate_and_the_raw_fee_window injects a clock. This is the strongest sentence in the diff and it points at the test rather than licensing its removal.
  • "the gate is NOT affected (clamping an already-clamped value is the identity)" — true.
  • run_claim_driver_in_with_clock's seam rationale (tokio::time::advance cannot move SystemTime::now(); the wrapper passes the real clock so no production caller changes) — true; driver.rs:442-449 is a pure forwarding call.
  • Over-broad (non-gating, posted inline): engine.rs:87 — "the same bound sanitized_schedule applies at the config read, so the gate tracks the schedule the driver really sleeps on". clamp reproduces only sanitized_schedule's clamp arm, not its zero-substitution arm; the sentence is true only because config.rs floors the cadence at 60 first.
  • Stale (non-gating, posted inline): driver.rs:502 — the F2 comment still describes "two DIFFERENT cadence values" and names cadence_seconds as the gate argument, but the call below passes one argument and the clamped local no longer reaches it. Semantics correct, mechanism superseded.

No surviving sentence claims a type guarantees the raw-label choice, and none could be cited to justify deleting the single guarding test.

4. Do the two rewritten assertion messages still describe their assertions? Yes — and the premise that no assertion logic changed is wrong: it changed, in the strengthening direction.

In the ungated window (68f4101d..97450a0b) four assertions flipped from assert_ne!(state, CadenceNotElapsed) to assert_eq!(state, Nominal) — two in the_gate_tracks_the_clamped_cadence_while_the_fee_window_tracks_the_raw_one, two in the new production-body test. Nominal is a strict subset of != CadenceNotElapsed, so each is strictly stronger than before; it removes exactly the vacuity the prior round's MEDIUM thread named (the exclusion was equally satisfied by PersistedStateCorrupt / ChainSourceUnavailable). The new messages say precisely that, and the after_tick_2 message correctly states it is meaningful only paired with the state assertion above it. No assertion was weakened; no assert_eq compares something other than what its message names.

5. Anything altering claim/fee behaviour beyond the reshape and docs? No.

The whole diff is two files. Behaviour-bearing changes: the one-argument call site (Q1 — identical), CLAIM_SCHEDULE_SECONDS_MAX constpub(crate) const (visibility only), with_persisted_fee_window pubpub(crate) (Q2 — no caller), and the new run_claim_driver_in_with_clock seam whose production wrapper forwards unix_now_seconds unchanged. Everything else is test call-site mechanics, the four strengthened assertions, the new test, and doc prose. No change to run_cycle, the gate comparison, the window-roll condition, fee accounting, or persistence.

Test-vacuity gate on the one guarding test

Property: at the production call site, the fee window is sized by the RAW configured cadence while the gate is sized by the CLAMPED one. Nearest wrong implementation that still compiles: from_raw(RawConfiguredCadence(cadence_seconds)). I re-derived the arithmetic from source rather than taking the measurement on trust: the window opens at t = 2_678_400 (tick 1); under the mutation fee_window_seconds = 2_678_400, so at tick 2 (now = 5_356_800) now - start = 2_678_400 >= 2_678_400 → the window rolls and after_tick_2 reads Some(5_356_800) against after_tick_1 = Some(2_678_400) — exactly the reported failure. Under correct code 2_678_400 < 5_184_000 → no roll at tick 2, and at tick 3 5_356_800 >= 5_184_000 → it rolls, so the tick-3 assert_ne also distinguishes "never rolls at all". drive calls now() exactly once per iteration (driver.rs:167-169), immediately after the sleep, so the injected one-interval-per-call clock models production faithfully; write_config sets jitter_seconds: 0 through the real save_to, so the scheduler's interval equals effective_cadence and the tick counts are not coincidental.

dig-constants

No new cross-repo constant. CLAIM_CADENCE_FLOOR_SECONDS and CLAIM_SCHEDULE_SECONDS_MAX are node-local scheduling bounds no second repo must match; DIG_ASSET_ID is already consumed from dig_mirror_coin. Nothing here belongs in dig-constants, and nothing here should be using it.

Threads

Both prior-round threads (the pub-field value-swap BLOCKER and the MEDIUM vacuity note) are addressed at this head and resolved with the fixing SHA. Two non-gating doc notes posted and resolved by me. Zero open threads.

What I did not run

Nothing: no build, no test, no clippy, no fmt. I did not read or write D:\worktrees\dig-node-3336. The COUNTS I rely on are the lane's (cargo test -p dig-node-service --lib rewards_claim → 92 passed / 0 failed; fmt --check 0; clippy -D warnings 0) plus the lane's revert-proof measurement, whose arithmetic I re-derived from source rather than re-executing. CI on this SHA is the independent leg for the greens.

Comment thread crates/dig-node-service/src/rewards_claim/driver.rs
Comment thread crates/dig-node-service/src/rewards_claim/engine.rs Outdated
…e clamp precondition

Doc comments only; no code, assertion, type or visibility changed. The test
count is unchanged (92 passed, 0 failed).

- engine.rs: delete the `with_persisted_fee_window` "F2 (money): takes TWO
  cadence values, deliberately not one" block. The method takes ONE
  `ClaimCadences`; those two names are private fields, not parameters, and
  "deliberately not one" instructed the next author to restore the two-`u64`
  signature this PR makes unwritable -- twenty lines above the #3336 section
  that contradicts it.
- engine.rs: `ClampedGateCadence::clamp` no longer claims flat equivalence with
  `sanitized_schedule`. It reproduces only that function's clamp arm, not its
  zero-substitution arm; they agree because a zero cannot reach it, and that
  precondition is now named.
- driver.rs: the surviving production-call-site comment describes the
  one-argument call it sits above, and names the single test that catches
  passing the clamped local instead of the raw config value.
- driver.rs: the sibling money test now records that it stays GREEN under that
  mutation, so its doc cannot be cited to delete the test that does catch it.

Refs DIG-Network/dig_ecosystem#3336

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Adversarial gate leg 3/3 — PASS @ 6c1a01d592726d0883b881bc9e7d0cbfbab0e3b6

Converting my CHANGES-REQUIRED on 97450a0b. Judged 6c1a01d5 only. Diff vs my head is doc-comment-only and I confirmed that independently, not from the report: git compare 97450a0b...6c1a01d5 is +15/−9 driver.rs, +12/−11 engine.rs, and every changed line is inside a /// or // block — no statement, signature, literal or assertion moved.

Is the blocking finding closed? YES

engine.rs:228-235 is deleted outright, nothing substituted; the F7 paragraph now joins "Without this call" directly. I swept both files at this head for any surviving sentence normative toward the two-value signature or citable to delete the single guarding test:

  • engine.rs:18-19 is the only remaining mention of the old shapes — "Earlier shapes handed the engine two numbers the CALLER had already chosen: first two bare u64 arguments, then two distinct newtypes, then this struct with two typed fields." Past tense, framed as superseded, and accurate at this head (ClaimCadences does hold two private typed fields). Not normative. No "deliberately" survives anywhere on this path.
  • No parameter-named reference to gate_cadence_seconds / fee_window_seconds survives in a doc; both names now appear only as the private fields they are.
  • Test-deletion pressure now runs both directions: engine.rs:33, driver.rs:1394 and the new driver.rs:1069-1073 reverse pointer. A reader arriving at either money test is told the other is not a duplicate. That was my recommended item and it is the part that makes this durable rather than merely correct today.

Did the doc pass introduce a new false claim? One numeric error, non-blocking. I checked all three replacement sites against the code.

Item 3 (driver.rs:502-508) — TRUE. One argument, must be raw cfg.cadence_seconds, transposition impossible but the value still wrong-able, ~12 vs ~6 windows/year, and it names the one catching test. Matches the measured Some(5356800) / Some(2678400) and the call site immediately below it.

Item 5 (driver.rs:1069-1073) — TRUE. The sibling does build its own ClaimCadences (driver.rs:1084-1087) and therefore is genuinely blind to the call-site mutation. One imprecision, sub-nit: it names the mutated call site as being "in [run_claim_driver_in]"; the call physically lives in run_claim_driver_in_with_clock (driver.rs:509), with run_claim_driver_in the thin wrapper. Defensible as the production entry point, and it misleads nobody toward a deletion.

Item 2 (engine.rs:85-96) — substance TRUE, ONE count wrong. The equivalence is now correctly narrowed to sanitized_schedule's clamp arm, and the zero-substitution arm is correctly named as the divergence. I re-derived the precondition and it holds: poisoned() is { corrupt: true, ..Self::default() } (config.rs:196-201) and default() sets cadence_seconds: default_cadence_seconds() = CLAIM_CADENCE_SECONDS_DEFAULT = 86 400 (config.rs:161-174, 144-146), so every non-success exit yields 86 400, and the parse-success path floors to 60 (config.rs:232-239). A zero cannot reach clamp from this path. The closing caveat about a caller not fed by a loaded config is also correct.

The error: "yields CLAIM_CADENCE_SECONDS_DEFAULT (86_400) on its other two exits." load_from has four returns that are not the plain success cfgNotFound → default() (config.rs:216), read-error → poisoned() (224), parse-error → poisoned() (~260), and the F14 in-success poisoned() for a persisted spend over budget (config.rs:246-256). All four yield 86 400, so the precondition the sentence exists to establish is unaffected; only the number is wrong. Suggested edit whenever these lines are next touched, not a hold: "…on its parse-success path, and yields CLAIM_CADENCE_SECONDS_DEFAULT (86_400) on every other exit — the missing-file default and all three fail-closed poisoned() returns, which derive from default()."

I am not holding a money-path PR on the epic's critical path over "two" vs "every other" in a sentence whose substance I verified line-by-line. My hold bar on this PR has been consistent: a sentence normative toward restoring the transposable shape, or citable to delete the single guarding test. Nothing at this head is either.

Standing, unchanged from my 97450a0b verdict

Type-enforced / test-guarded split accurate; the single guard is durable (exact-state assertions turn every vacuity vector I could construct into a loud failure); the derivation is honest and closed; the remedy is subtractive (now −44 net across five commits). No follow-up ticket.

PASS — merge-ready pending the DO NOT MERGE first line coming off the body and undrafting, which are the orchestrator's calls, not mine.

… exit count

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review September 18, 2026 03:23
@MichaelTaylor3d MichaelTaylor3d changed the title fix(rewards-claim): newtype fee-window/gate args + injectable clock (#3336) fix(rewards-claim): derive both cadences from one raw config value and inject the clock Sep 18, 2026
@MichaelTaylor3d
MichaelTaylor3d merged commit 4e102c6 into develop Sep 18, 2026
16 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the fix/3336-claim-driver-newtypes branch September 18, 2026 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant