fix(relay): charge the proxy allowance last so a refusal leaves no trace (#512)

[MichaelTaylor3d]

Closes #512

DRAFT — the gates run after this lane returns. Do not merge, do not undraft.

The leak

The operator's onion-relay opt-in was observable to a peer through a side effect, not a frame. #356/#504 already pin that every relay refusal returns a byte-identical RESOURCE_UNAVAILABLE; that guard passes under this defect, which is exactly why this ticket exists.

Mechanism, verified on origin/main @ 04079d57:

• module_relay.rs:124 — gate 3 called content.allow_proxy_fetch(requestor).
• download.rs:1643 — that is requestor.is_local() || self.proxy_rate_limiter.check(requestor).
• rate_limit.rs:120 — TokenBucket::try_acquire decrements on admit; a refused acquire decrements nothing.
• module_relay.rs:127 — the capsule-warmer gate ran after, so a build with no warmer had already been charged.
• download.rs:2721 — miss_outcome leg 2 reads the same proxy_rate_limiter.

So token-consumed held exactly when (requestor asked ∧ engine present ∧ operator opted in). The attacker controls the first, the second is stable — leaving the opt-in as the only variable. Draining the bucket flips a later dig.fetchRange/dig.getContent from fetch-through to redirect, and the stranger reads DIG_NODE_ONION_RELAY off a different method.

The fix — a pure reorder, no new machinery

The capsule-warmer check moves above allow_proxy_fetch, making the allowance the last gate, immediately before the spawn. Ticket option (a), check-then-consume, with no refund path, no second bucket, no new state. The tracing::debug! already sat after both, so it no longer announces a pull that is then refused.

Post-fix the invariant is exact and total: every path returning RelayStatus::Refused consumes nothing. A spend therefore implies gates 1–3 passed and gate 4 admitted, which implies the call returns Landed or Pending — a frame that already tells the requestor relaying is on. Gate 4 refusing also consumes nothing (try_acquire returns false without decrementing), so the exhausted-allowance case is equally traceless.

Also fixes the ticket's smaller ordering defect: on a build with no warmer, a requestor's allowance was spent on a relay that could never happen.

No DoS regression — the gate that moved earlier is an Option clone, cheaper than the limiter's mutex + hashmap.

Wall-clock timing is an explicit non-goal and was not attempted; the gates are cheap reads whose cost differences sit far below network noise.

Blast radius checked

gitnexus's registered indexes are stale by construction for lane worktrees (CLAUDE.md §2.0), so this was done by grep + direct read and is stated as such.

• relay_capsule — 3 call sites, all in-crate: lib.rs:3130 (dig.getModuleInfo), lib.rs:3216 (dig.fetchModuleRange), peer.rs:1595 (the streaming peer surface). pub(crate), so the radius is closed at the crate boundary. Its signature, arity, return type and every RelayStatus variant are unchanged — only statement order inside the body moved, so all three call sites are unaffected by construction.
• allow_proxy_fetch — 2 callers: this one and download.rs:2721 (miss_outcome leg 2). Untouched; the second caller is what the new guard reads through.
• capsule_warmer() — untouched, called once here.
• No public API, no wire format, no error code, no persisted state changed. RESOURCE_UNAVAILABLE remains the answer on every refusal path.

How verified

Red first, at the decision, on the RPC surface a stranger sees. New test a_refused_relay_leaves_no_trace_in_the_proxy_allowance (crates/dig-node-core/src/lib.rs) runs two arms differing only in pc.set_onion_relay(false|true) — same node shape, same RequestorId::Peer, same no-refill proxy allowance of exactly 1, same uncached capsule. attach_p2p wires no capsule warmer, which is the real build shape the defect lives in. Each arm: fire a proxy: true dig.getModuleInfo for an uncached capsule, assert both come back RESOURCE_UNAVAILABLE (a fixture precondition — otherwise the arms are not comparable), then probe the shared bucket through dig.fetchRange and compare what the two arms observe.

Asserting equal refusal frames would be vacuous here; the observation is deliberately taken through the other method, where the bucket's state is legible.

Red evidence — the test was written and run against unmodified origin/main code (the fix commit 834139bf comes after the test commit 7c14b975), which is a stronger revert-proof than reverting afterwards:

thread 'tests::a_refused_relay_leaves_no_trace_in_the_proxy_allowance' panicked at crates\dig-node-core\src\lib.rs:15850:9:
assertion `left == right` failed: the operator's onion-relay opt-in is observable through the proxy
allowance: a refused relay charged a token on the opt-in arm, so a later dig.fetchRange changed shape.
relay ON saw {"error_code":-32008,"served":false}, relay OFF saw {"error_code":null,"served":true}
  left: Object {"error_code": Number(-32008), "served": Bool(false)}
 right: Object {"error_code": Null, "served": Bool(true)}
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1101 filtered out

-32008 is CONTENT_REDIRECT — the relay-ON arm's stolen token degraded the probe, exactly the predicted mechanism.

Green — full crate lib suite after the fix:

test result: ok. 1102 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 848.85s

Test count checked, not just the exit status: 1 test ran in the red run, 1102 in the green (1101 pre-existing + 1 new), 0 filtered out.

Docs

• module_relay.rs module doc: the "three gates" list is now four, in the shipped order, with the reason the allowance is charged last — a refusal must not leave evidence of how far it got — plus the timing non-goal.
• RelayStatus::Refused: the variant doc now says the refusal must leave no trace as well as no narration, and that no path to it consumes a token.
• lib.rs #504 doc comment, which explicitly named this open channel: rewritten to describe it as closed and pointed at the new guard.
• SPEC.md needs no change — checked. It describes relay_staged_bytes (§21.1) and the relay budget scoping, but does not describe this leg's gate set or their ordering, so there is no clause left describing behaviour the code no longer has.

SemVer — 0.257.0 (minor)

origin/main is 0.252.4; 0.253.0–0.256.0 are held by in-flight PRs. Read from Cargo.toml on disk after the merge, not from the commit log.

Minor, not patch, deliberately. This is a fix(...) commit and no public API changed, which argues patch — but it is a behaviour change on a refusal path that operators may be relying on for accounting: a warmerless build, or a build whose relay is refused at the warmer gate, no longer charges the requestor's proxy allowance, so a peer's observed proxy budget is now strictly more permissive than on 0.252.4. That is a compatible change in capability granted, which §2.4's table puts at minor. Gating up on the ambiguity rather than down.

Local checks

• cargo test -p dig-node-core --lib → 1102 passed; 0 failed; 0 filtered out (RC 0).
• cargo clippy -p dig-node-core --lib → clean, RC 0, zero warnings.
• cargo fmt on the two touched files only (never --all).
• cargo clippy --all-targets could not complete on this host: third-party crates fail to build (cranelift-assembler-x64 rustc STATUS_STACK_BUFFER_OVERRUN 0xc0000409, aws-lc-sys v0.44.0 build script, chacha20). Machine-level and unrelated to this diff — the lib itself compiles and lints clean. CI's Linux runners cover it.
• The closest neighbouring guards are green in the full-suite run above: a_relay_refusal_is_indistinguishable_from_a_plain_miss (audit(nc-12): 15 doc-comment-only NC-12 claims in seams/dig_peer — measure which are tested and which are merely asserted #356/test(nc-12): measure the NC-12 doc claims in seams/dig_peer, guard 2 #504) and proxy_fetch_is_bounded_by_its_own_allowance_independent_of_lookups (#2189).
• git merge origin/main (never rebase) → already up to date at 04079d57. Version re-read from Cargo.toml on disk afterwards: 0.257.0.

[MichaelTaylor3d]

CORRECTNESS GATE — CHANGES-REQUIRED (one finding, three sites, comment-only)

Head reviewed: 834139bfa998b45fddbe54437733db34fbd9e688 (re-resolved from the remote).

The substantive change is correct, total, and genuinely guarded. The single finding is
superseded gate-numbering wording that survived the renumbering this PR performed — including one
site inside the file the PR renumbered. That is the stale-doc-after-a-shape-change class, so it is
posted as an open thread rather than a note, but it is a comment-only fix.

What I verified (each independently, against this SHA)

1. The invariant is TOTAL. relay_capsule (module_relay.rs:125-182) has exactly five Refused
returns: :134 (requestor did not ask), :138 (no p2p content), :142 (operator opt-in), :148
(no capsule warmer), :157 (allowance). The first four are pure reads. The fifth cannot consume:
TokenBucket::try_acquire (rate_limit.rs:120-134) decrements inside the state.tokens >= 1.0
arm and returns false untouched otherwise — read, not assumed. So no path to Refused spends a
token, and a spend implies Landed/Pending.

2. All THREE callers are covered, not just the two the ticket walks. lib.rs:3130
(dig.getModuleInfo), lib.rs:3216 (dig.fetchModuleRange), and peer.rs:1595 (the streaming peer
surface). I read the peer surface specifically: peer.rs:1586-1606 charges no allowance of its own
around the call and only re-reads the window on Landed. The fix is inside relay_capsule, so it
holds for all three.

3. Nothing else moved. allow_proxy_fetch, TokenBucket, MissRateLimiter and miss_outcome
are untouched. No refund path, no second bucket — this is option (a), not (b). The
tracing::debug!("module relay: pulling a capsule…") at :159 remains strictly after the allowance
check, so the node still never logs a pull it refuses.

4. The test is NOT vacuous, and the revert-proof survives inspection. Commit topology:
7c14b975 adds the test only (lib.rs +135, module_relay.rs untouched); 834139bf applies the
fix. The fix commit's lib.rs hunk is a rustdoc-only edit to the neighbour test
a_relay_refusal_is_indistinguishable_from_a_plain_miss — the new guard's body is byte-identical
between the red run and the green run, so the red evidence is evidence about this test. Checked the
six ways it could be wrong:

• arms differ only in set_onion_relay(false|true) — same node shape, peer id, allowance, capsule;
• both arms assert RESOURCE_UNAVAILABLE on the drain as a stated fixture precondition, so the
  equality below cannot be comparing incomparable journeys;
• the drain goes through dig.getModuleInfo and the probe through dig.fetchRange — the
  cross-method coupling is the attack, not a same-method restatement;
• set_proxy_rate_limit(1.0, 0.0) — capacity 1, refill 0, so no wall-clock race, and
  set_miss_rate_limit(100.0, 0.0) keeps the probe from being refused for the wrong reason;
• the assertion is on the RPC shape a stranger observes, not on allow_proxy_fetch directly;
• the reported red — relay ON {"error_code":-32008,"served":false} vs
  relay OFF {"error_code":null,"served":true} — is exactly the discrimination the code path
  predicts, and is reachable only if the probe genuinely reads the drained bucket.

5. Merge preconditions. check-merge-preconditions.sh (unpiped, rc=0): all six required
contexts present and SUCCESS by name — Lint commit messages, Check version increment, Rustfmt,
Clippy, Test + coverage, Release-script tests. None absent. The four red build .deb/.msi/.pkg
packaging jobs are not in the required set. Zero unresolved threads. closingIssuesReferences
resolves to 512 (not swallowed by a code span). Version 0.257.0: the gate-up from patch to
minor is justified — this changes the observable ordering of a peer-facing gate, and minor is the
right call for a behaviour change that is compatible but not invisible.

Non-goals, as briefed — not raised

Wall-clock timing; miss_outcome leg 2 charging then falling through to redirect (real, different
defect, leaks the requestor's own allowance not the operator's opt-in); read-once-at-construction
of ONION_RELAY_ENV.

[MichaelTaylor3d]

loop-security — IN PROGRESS, not the verdict

Head audited: 834139bfa998b45fddbe54437733db34fbd9e688 (resolved from gh pr view 517 --json headRefOid).
Merge-base: 04079d5711e512904fef7abfef7df410137d38c5. Read-only, via git show <sha>:<path> — no worktree touched.

Posting as I go so nothing is lost to a watchdog. Four items resolved so far.

1. The reorder does NOT put expensive work in front of the only per-requestor bound — CLEAR

The three gates that now precede the allowance are all pure, non-allocating reads. Verified individually,
not taken on the PR's word:

gate	site	cost
(1) proxy_requested(params)	download.rs:3055	Value field read on a payload already parsed
engine present	download.rs:2652 p2p_content()	Option<&Arc<NodeContent>> accessor
(2) onion_relay_enabled()	download.rs:1621-1623	AtomicBool::load(Relaxed) — one atomic load
(3) capsule_warmer().cloned()	download.rs:2350-2352	OnceLock::get() + Arc refcount increment

No lock acquisition, no heap allocation, no I/O, no map insert, no unbounded work. Nothing keyed on the
requestor is touched before gate 4. No DoS is introduced by moving the allowance later — the work a
stranger can now drive for free is four loads and one refcount bump, strictly cheaper than the mutex
acquisition inside TokenBucket::try_acquire (rate_limit.rs:120-134) that used to run first.

Note in passing, in the same direction: MissRateLimiter::check (rate_limit.rs:289) also touches a
GLOBAL bounded table (MAX_TRACKED_REQUESTORS, rate_limit.rs:296-304), so the old order let a warmer-less
node's table be pressured from this path too. The fix reduces reach there as well.

2. Neither test-only switch was widened — CLEAR

• set_onion_relay — #[cfg(test)] at download.rs:1629, unchanged in the diff.
• set_proxy_rate_limit — #[cfg(test)] at download.rs:1495, unchanged in the diff.
• MissRateLimiter::reconfigure — #[cfg(test)] at rate_limit.rs:276, unchanged.

The field doc at download.rs:1625-1628 ("a running node's amplification posture is fixed") still holds.
The new test reaches both switches from inside mod tests, so nothing had to be widened to make it compile.

3. The "no allowance consumed on a refusal" invariant is EXACT — confirmed at the token level

allow_proxy_fetch (download.rs:1642-1644) -> MissRateLimiter::check (rate_limit.rs:289) ->
TokenBucket::try_acquire (rate_limit.rs:120). The refusal branch (:131-133) decrements nothing, and
the table-saturation branch (:301-303) returns before any insert. A gate-4 refusal therefore cannot
occur on a freshly-created bucket (a fresh bucket is at capacity and admits), so it only fires on an
already-exhausted bucket, where no token exists to spend. The invariant as stated in the module doc is
true, not approximately true.

4. Diff scope

4 files. Only module_relay.rs:144-157 is behavioural (a 5-line move); the rest is doc, one new test
(lib.rs:15726-15859), and the workspace version 0.252.4 -> 0.257.0.

Still open, and where the real risk sits: the THREE call sites around relay_capsule
(lib.rs:3130, lib.rs:3216, peer.rs:1595), the is_local() exemption reachability on each, residual
opt-in oracles beyond the frame and the allowance, and whether the new guard proves the invariant for a
build that DOES wire a warmer. Continuing.

[MichaelTaylor3d]

loop-security — IN PROGRESS, not the verdict (2/2)

Head 834139bfa998b45fddbe54437733db34fbd9e688. Continuing from the previous comment.

5. The is_local() exemption cannot be claimed by a peer, on any of the three call paths — CLEAR

allow_proxy_fetch short-circuits on RequestorId::is_local() (download.rs:1643), which is
matches!(self, RequestorId::Local) (rate_limit.rs:63-65). Traced every one of the three
relay_capsule call sites to where its RequestorId is constructed, not where it is passed:

call site	requestor origin	can it be Local?
peer.rs:1595 (streaming dig.fetchModuleRange)	RequestorId::Peer(conn_key) written inline at peer.rs:1600	No — structurally impossible, the variant is a literal
lib.rs:3130 / lib.rs:3216 via peer JSON-RPC	peer.rs:1501-1507 handle_rpc_as(.., ReadOrigin::Peer, .., RequestorId::Peer(conn_key))	No — same, a literal
lib.rs:3130 / lib.rs:3216 via HTTP JSON-RPC	server.rs:934-940 requestor_for(peer_addr)	No — derived from the ACCEPTING SOCKET's remote address via is_loopback_addr; a non-loopback caller becomes Anonymous(ip)

Supporting checks:

• No X-Forwarded-For / Forwarded trust anywhere in crates/ — grep returns nothing, so a
  reverse proxy header cannot promote a caller to loopback. The classification is socket-derived only.
• The three RequestorId::Local literals in peer.rs (:6358, :6388, :6496) are all inside
  #[tokio::test] bodies. The peer wire never constructs Local.
• control.rs:999 does pass ReadOrigin::Local (hence RequestorId::Local via
  RequestorId::from_origin, rate_limit.rs:53-58), but dispatch_control is reached only for
  control.* methods and only after control::is_authorized (server.rs:1180, :1204). Neither
  dig.getModuleInfo nor dig.fetchModuleRange routes through it.

6. No NEW consumption channel is possible — the spend-set strictly shrinks

This is the cleanest way to answer "what does the reorder open". Let G1..G4 be the gates.

• Old order spends a token iff G1 && engine && G2 && bucket_has_token.
• New order spends a token iff G1 && engine && G2 && G3 && bucket_has_token.

The new predicate is the old one conjoined with G3. The set of inputs on which a token is now
consumed is a strict subset of the set on which it used to be consumed. No input that previously
consumed nothing now consumes something, so no attacker-observable state can move in the direction that
would create a channel. The change is monotone in the safe direction, which is why it cannot open a
consumption oracle of any shape — including ones I did not think to enumerate.

I also confirmed there is no early return between gate 4 and the function's exit
(module_relay.rs:155-183): once allow_proxy_fetch admits, the function unconditionally spawns and
returns Landed or Pending. No ?, no fallible call, no third exit. The stated invariant
("a spend implies Landed/Pending") is therefore total, not approximate.

7. FINDING (informational, NOT gating) — the oracle this PR closes is unreachable in any SHIPPED configuration

The behavioural delta between the two orders is non-empty only when
p2p_content().is_some() && capsule_warmer().is_none(). On a build with a warmer, both orders charge on
exactly the same inputs, so the fix is a no-op there. I traced whether a shipped node can be in that
state, and it cannot:

• NodeContent::for_dht has exactly one caller in the whole tree: peer.rs:2802.
• That caller invokes content.wire_capsule_reshare(...) at peer.rs:2832, which calls
  set_capsule_warmer unconditionally (download.rs:2442, an infallible OnceLock::set).
• Only then, at peer.rs:2899, does it call node.set_p2p_content(content) — the same
  if let Some(dht) block, straight-line, no await between.
• set_p2p_content is pub(crate) (download.rs:2647), so no external embedder can attach an
  engine at all, warmer or not. The FFI/in-process path attaches none (download.rs:779).

So in production the warmer is installed before the engine becomes reachable, and
p2p_content().is_some() && capsule_warmer().is_none() holds only in this crate's own test fixture
(attach_p2p, lib.rs:14795-14805), which is precisely what the new guard uses.

What this means, stated plainly and in both directions:

• The ticket's live-severity framing ("a stranger reads the operator's opt-in in about five cheap
  requests") overstates what was reachable on a shipped binary. On a real node the old order charged
  the token and then relayed, returning Landed/Pending — the disclosing frame — so there was no
  cross-method inference to make. The defect was real as written but latent in every shipped build.
• Symmetrically, and this is why it argues FOR the change: because the two orders are semantically
  identical on every configuration that ships, the reorder cannot regress production behaviour,
  cannot change amplification posture, and cannot introduce a DoS. It converts a
  true-only-by-accident property into a true-by-construction one, at zero cost.
• It also means the new guard proves the invariant only in the warmer-absent configuration. See the
  coverage note in my verdict.

Still to land: residual-oracle enumeration beyond the frame and the allowance, and the probe
(cargo test -p dig-node-core --lib tests::a_refused_relay_leaves_no_trace_in_the_proxy_allowance)
running in my own worktree at C:\tmp\worktrees\sec517-gate — the lane's worktree was not touched.