Uh oh!
There was an error while loading. Please reload this page.
feat(exchange): adopt dig-sex for selection residency, acquisition and recursive discovery - #289
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
…act (#287) Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED (recorded as a comment review: self-approval/request-changes is a 422 on an own-authored PR). Gate round 1 on head 5cb07523ac0a80334b2dc313e1ead9325440c708. Four findings inline; two gating, both on the amplification + hop-budget claims rather than on the enforcement itself. Suite + revert-proofs still running; verdict comment follows.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
MichaelTaylor3d
commented
Aug 21, 2026
CHANGES-REQUIREDHead reviewed: Two gating findings, both posted inline. Neither is a defect in the enforcement — the code moves fail-closed in every branch I could reach. Both are about a claim the PR makes being wrong or unguarded, which on an amplification change is the thing a reimplementer and an operator act on. Gating
Non-gating (inline)
Verification I ran myself, not carried from the body
The two corrections to the brief — both were right
|
Full subject: fix(spec): correct the recruitment figure to the sum over hops, and restore the hop-counterless MUST (#272) The amplification claim quoted `worst_case_nodes_recruited()` = fan_out ^ hop_cap = 9, which is the LEAF COUNT of the last hop only. One admitted frame recruits 3 + 3^2 = 12 nodes, or 48 against a full relay burst — so the reduction from dig-node's rival is ~28x, not ~150x, and the stated disclosure radius was 25% low. The deleted FORWARDED_ASK_FANOUT doc warned about exactly this reading. Restoring the warning next to the figure, and pinning fan_out and hop_cap separately since their product does not identify them. Also restores the MUST requiring a hop-counter-less request shape to be treated as fully spent — removed while peer.rs still implemented it, and unguarded by any test. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
b79ea0a to
eff1a01CompareUh oh!
There was an error while loading. Please reload this page.
What this adds
Consolidates the recursive-discovery rival onto
dig_sex::discovery(#272), which resolves theunbounded-reach defect on the way (#281), and carries the self-exclusion half of #266.
Continues the dig-sex integration batch already on this branch (dig-sex 0.4, tier-0 residency #287,
acquisition #270/#282).
The split: dig-sex owns the DECISION, dig-node keeps the WIRE
forwarded_ask.rsstays as transport — opcode, framing, dial. Its decision half is gone, replaced bydecide_forward/parse_enabledcalled fromNodeContent::forwarded_holders. dig-node's in-treefan-out constant and its hand-rolled peer selection are deleted.
Two gates stay in dig-node because they are properties of this node's resources rather than of the
protocol: the per-requestor relay bucket and the node-wide concurrency semaphore. The relay bucket is
passed IN as
relay_budget_availablerather than checked around the decision, so the crate'sRelayBudgetSpentarm stays reachable instead of being shadowed by a local guard.BEHAVIOUR CHANGE — stated plainly, two of them
1. Reach drops ~28x. dig-node fanned out to 4 peers over a hop cap of 4 — about 1,360 dials and
DHT walks per admitted frame against a full relay burst. Under the canonical bounds (
fan_out3,hop_cap2) one admitted frame recruits 12 nodes —3 + 3^2, the SUM over hops — or 48against the same relay burst. That 12 is also the disclosure radius.
Corrected after the gate caught it. An earlier revision of this PR quoted
worst_case_nodes_recruited()= 9 as both the recruitment and the disclosure radius. That functionreturns
fan_out ^ hop_cap, which is the leaf count of the last hop only and omits everyintermediate node — nodes that do the same work and learn the same triple. So the reduction was
overstated (~150x for ~28x) and the disclosure radius was 25% low.
The part worth recording: the
FORWARDED_ASK_FANOUTdoc-comment this PR deleted warned aboutprecisely this reading — "the leaf count of ONE question, not the cost of a frame... understates by
about 5x." The consolidation removed the warning and then made the mistake it warned about. Deleting
a rival's documentation deserves the same care as deleting its code; the warning is now restored
next to the figure, in
download.rs,forwarded_ask.rsand SPEC §10.4.4.The root cause is upstream —
dig-sexdiscovery.rs:77-83calls a leaf count "the number of nodesone admitted request can recruit" and "the disclosure radius". This PR cites the real numbers and
pins the difference explicitly; the crate's own naming is filed separately against dig-sex.
Enforcement was never wrong — only the claim was. No bounds changed.
Is 3x2 too small for real use? Possibly — on a sparse pool a holder three hops out becomes
unreachable. But that is a tuning question, and
RecursionConfigis a value this node passes in atthe composition root, so config can carry it whenever measurement says so. Fail-closed and
off-by-default are not negotiable and are not being traded for reach — this path spends other
nodes' bandwidth.
2. An unreadable hop budget now REFUSES.
redirect_depthwas parsed with.and_then(Value::as_u64).unwrap_or(0), so a present-but-unparseable value — an attacker-suppliedfield — became
0, the most permissive value the field has, at every hop, forever. A request whosehop budget cannot be read is a request whose reach is unbounded.
The new
HopBudgettype keeps the three states distinct, and deliberately gives the same field tworeadings:
UnreadableHopBudget)The redirect keeps its tolerant reading because it spends only this node's own lookup; forwarding is
gated more tightly because it spends other nodes'. That asymmetry is deliberate, documented on the
type, tested (
the_redirect_leg_keeps_its_tolerant_reading_of_the_same_field), and confines thebehaviour change to the forwarding decision.
3. A hop-counter-less request shape is fully spent — a MUST restored.
maincarried a clauserequiring the dig-nat mux
AvailabilityRequest, which has no field able to hold a hop count, to betreated as fully spent. An earlier revision of this PR deleted the clause while
peer.rs:1391stillimplemented it via
HopBudget::spent(). The gate measured the consequence: replacingspent()withfresh()there left 864 passed / 0 failed — nothing guarded the one shape that cannot counthops, and that flip is the same failure direction as the
.unwrap_or(0)defect this PR exists to fix.Restored in SPEC §10.4.4 over the class rather than one message type: any inbound shape with no
field able to hold a hop count MUST declare its budget spent, so a second such shape inherits it
without the clause being rewritten. Guarded by a new test on the mux responder (below).
RecursionConfigis now installed with the leg at the composition root rather than read from env atdecision time, so the amplification posture is fixed at start-up and the decision is exercisable
without process-global state.
What I carried from #266, and what I dropped
I carried the self-exclusion fix. The brief said it was redundant under the crate; measured against
the code, it is not, and dropping it would have reintroduced #261.
decide_forwardexcludes requestor and self from the peers this node asks. #266 fixes selfappearing in the answer — a peer is free to answer with a record naming us, and
merge_answersdoes not filter one out. The two rules travel in opposite directions; adopting the crate discharges
only one. Carried:
retain_excluding_self(one shared filter at the merge, covering every sourceincluding any added later) plus both of #266's tests — the forwarded-leg assertion and its DHT-leg
control.
There was no A→B→C round-trip test on #266 to carry. Its 84 added test lines are the
self-exclusion pair; nothing in that branch or on
maincomposes two hops. So I wrote one:a_holder_two_hops_away_is_reached_through_the_middle_nodechains a real secondNodeContentas themiddle hop, so B runs the same consolidated decision against its own peers and budget. Its control —
the same topology with B's onward leg removed — is the load-bearing half: without it the test cannot
tell "the middle hop recursed" from "the middle hop knew the holder".
Nothing from #266 was dropped. #266 can be closed as subsumed.
Blast radius checked
locate_holders/forwarded_holders/miss_outcome/availability_batchand their callers(
range_miss_envelope,content_miss_envelope,availability_answer, the peer stream leg,dig_rpc::dispatch). ThreadingHopBudgetin place of a bareu64is what surfaced the full callerset at compile time rather than by grep.
forwarded_requestand the wire parser are untouched, sothe on-wire shape is byte-identical.
Risk assessment: MEDIUM, not high. The change is confined to the miss-enrichment path; both
behaviour changes move in the fail-closed direction; the leg is off by default in production, so a
node that has not opted in sees no change at all.
Verification
cargo test -p dig-node-core --lib— 865 passed, 0 failed (371s).cargo clippy --workspace --all-targets --all-features— clean.cargo fmt --allapplied.One intermittent failure seen and chased down, reported rather than buried. An earlier run of the
same suite failed
tests::cache_lock_is_exclusive_then_released(864/1). It passes in isolation andpassed on the clean re-run above. It is a test-isolation hazard, not a regression from this work:
several tests drive
evict_modules_if_needed, which takes the cross-processacquire_cache_lock(),while
cache_lock_is_exclusive_then_releasedasserts exclusivity on a lockfile path derived fromDIG_NODE_CACHEunderENV_GUARD— the eviction tests do not hold that guard, so under parallelexecution the two can resolve to the same lockfile. Nothing in my commits touches either. Flagged as
a follow-up rather than fixed here; it will flake in CI eventually and should be pinned properly.
Revert-proofs, with the assertion that fired:
Restore
.unwrap_or(0)on the unreadable budget →an_unreadable_hop_budget_forwards_nothing_while_a_readable_one_forwardsfails atforwarded_ask_tests.rs:362, theasked().is_empty()refusal assertion — the right one, not theanswer-shape assertion beside it.
Let only an originating request forward (a relayed hop never does) →
a_holder_two_hops_away_is_reached_through_the_middle_nodefails at line 715, theholder-reached-A assertion, and
the_hop_budget_is_pinned_from_both_sidesfails alongside it.Flip
HopBudget::spent()tofresh()at the mux leg (peer.rs:1391) — the flip the gate showedwas invisible against the whole suite →
the_hop_counterless_mux_shape_forwards_nothingfails atpeer.rs:4505, theasked().is_empty()refusal assertion, with a diagnostic naming the ask thatleaked. Its control drives the same content through
availability_batchwith a readable budgetand observes an ask go out, so the test distinguishes "the mux leg refuses" from "this fixture
never forwards".
fan_outandhop_capare now pinned separately rather than only through their product, since9/1satisfies any assertion on the product while being a pool-wide broadcast. Recruitment isasserted as the computed sum over hops (12), with
worst_case_nodes_recruited() == 9pinned besideit and labelled as the leaf count, so the difference between the two stays visible in the test rather
than only in prose.
Docs — one sibling NOT done, deliberately
docs.dig.net/docs/protocol/peer-network.md:818still says "on the order of 1,300+ outbound dials",now wrong by ~150x, and the same line exists in 13 i18n copies. That is a different repo and I was
scoped as the single writer in dig-node only; editing a shared checkout mid-flight risks colliding
with another lane. Flagging rather than silently dropping it — it needs its own PR.
Closes#272
Closes#281
Closes#261