feat(pool): direct-inbound adoption entry point (#3124) - #83
Conversation
Adds the failing regression suite for dig_ecosystem#3124: dig-gossip has no direct-inbound adoption entry point, so every inbound peer is uncounted. Asserts the three properties this family has each shipped a defect against -- COUNTED, still SERVED, and REACHED by a broadcast (bytes received, never a send-list length) -- plus varying-field controls so `via`, `is_outbound` and `dial_addr` cannot pass vacuously. Refs: DIG-Network/dig_ecosystem#3124 Co-Authored-By: Claude <noreply@anthropic.com>
Adds `GossipHandle::adopt_direct_inbound_handle`, the fifth adoption entry point and the first that accepts a DIRECT connection this node accepted. A node serving inbound peers previously had nowhere to register them, so `connected_peers` under-reported every one. Makes dialability explicit rather than a tier derivation: a `dig-nat` slot's `remote` is a dial target only when THIS node chose it. An accepted slot's `remote` is the peer's ephemeral source port, so it reports `dial_addr = None` for a reason independent of the relayed tier's. Bounds the accepted-direct tier by the same reserved quarter as the relayed one, so inbound peers cannot fill the pool and choose this node's peer set. Refs: DIG-Network/dig_ecosystem#3124 Co-Authored-By: Claude <noreply@anthropic.com>
…124) Adds the normative clauses for the direct-inbound entry point: every ACCEPTED connection is a pool member, dialability is a property of the tier AND the direction, and the accepted-direct tier is capped by the same reserved quarter as the relayed one. Per CLAUDE.md 2.4b, brings the crate's own deps to latest published: dig-nat 0.20 -> 0.21, dig-ip 0.1.1 -> 0.1.2. The chia-* set stays on 0.36.1 deliberately -- dig-peer-protocol 0.9.0 still declares ^0.36.1, so moving this crate alone to 0.48 would ship it split across two chia lines. Version: 0.30.0 -> 0.31.0 (minor: additive public API). Refs: DIG-Network/dig_ecosystem#3124 Co-Authored-By: Claude <noreply@anthropic.com>
Lane resumed (progress log, resume-ready)The original lane died to the 600s silence watchdog. Its pushed work at Branch: DONE
§2.4b dependency sweep — measured, not assumedEvery
The chia 0.36.1 → 0.48.0 uplift is NOT taken here, deliberatelyTaking it in this PR would ship dig-gossip internally split across two chia lines, which is the Logged rather than fixed, with the shape stated so the cascade can be scoped: it is a whole-line uplift Also logged and left — a pre-existing split, NOT introduced by this PR
IN PROGRESSFull NEXT ACTIONRead the clippy result; fix any lint in the new code; commit fmt + the Staying DRAFT — the gate round has not returned. |
…ant to_string
CI's `Format, Clippy & Docs` job failed at its first step, `cargo fmt --all -- --check`,
which exits before the clippy and doc steps run. Clippy had therefore never actually
been measured against this branch's 900 lines, and it was hiding a real lint behind the
formatting failure:
error: unnecessary use of `to_string`
--> src/service/gossip_handle.rs:1999
`-D clippy::unnecessary-to-owned`
`SafeText::from_untrusted` already takes the string by value conversion, so the relayed
refusal message allocated twice. Removing `.to_string()` leaves the literal, which is
what clippy asks for and what the other refusal sites in this function already do.
Three rustfmt diffs, all in code this PR adds: one call in `src/service/gossip_handle.rs`
and two in `tests/con_3124_adopt_direct_inbound_tests.rs`.
Verified locally with the exact commands CI runs: `cargo fmt --all -- --check`,
`cargo clippy --all-targets --all-features -- -D warnings`, and
`cargo doc --no-deps --all-features` all exit 0.
Co-Authored-By: Claude <noreply@anthropic.com>
Green and gate-ready — still DRAFTHEAD Run unpiped, exit code read from The red check was hiding a second defect
With formatting applied, clippy failed on its own: The relayed-tier refusal allocated its message twice, where the other refusal sites in the same Dependency sweep — one bump found, deliberately NOT taken hereFull detail is in the previous comment. The outcome:
#82 is the named owner and was explicitly sequenced behind this lane. Taking it inside a feature PR The bump is also not a chia-line hazard, which is worth stating because it is the usual reason to Logged and leftThe chia Separately, Review notes for the gateThe three admission decisions are the ones worth adversarial attention, and each reads as consistent
Staying DRAFT until the gate round returns. |
Correctness gate — IN PROGRESS, not the verdictHead read: Established so far (posting as formed, per the gate's durability rule):
Still open and being probed: whether the two inbound caps, counted separately, still leave the |
loop-security — IN PROGRESS, NOT THE VERDICTAudited head: Recording two findings now so they survive a stall. Neither is the verdict; the audit continues. Finding 1 (HIGH, introduced here) — an accepted-direct slot can NEVER be chosen as a displacement victim
The new entry point inserts into Both sibling adoption paths do:
And pub(crate) fn publish(&self, event: PoolEvent) {
match &event {
PoolEvent::PeerAdded { peer_id, .. } => {
self.record_admission(*peer_id, metric_unix_timestamp_secs());
}Its own doc says so: "Admission bookkeeping lives HERE because this is already the one place every admission path funnels through to announce itself." The new path is now the one admission path that does not funnel through it. The consequence is not a missing metric — it is displacement immunity. So an accepted-direct peer:
Attacker scenario. A stranger completes N mTLS handshakes with N free BLS identities and is adopted. Those N slots are permanently un-displaceable. Every displacement the discovery path (#3128 req. 8) attempts now falls on an honest peer — one admitted by a path that did register itself. Push it further and This inverts NC-12: peers are meant to be cycled, and here the only slots that cannot be cycled are the ones a stranger chose to open. Same omission also drops Finding 2 (MEDIUM-HIGH, introduced here) — the two inbound caps sum to 150% of the pool, so the reserve each one documents is not preserved under composition
fn reserving_a_quarter(n: usize) -> usize { n.saturating_sub((n / 4).max(1)) }With the shipped default
The new doc comment states the separate counting as the safety property — "The caps are counted separately, so the two inbound tiers cannot pool their budgets" ( Each cap's stated purpose is to reserve a quarter of the pool for peers this node chooses. That reserve held while there was one inbound tier. With two, 38 accepted-direct + 12 accepted-relayed = 50 = Compounds with Finding 1: those 38 direct slots are also the un-displaceable ones. Still open and being worked: per-source (/16 / AS) bounding on this path, fail-direction of |
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate — CHANGES-REQUIRED
Head reviewed: cd13d215b6a6d5ddbfe7880cbcd393eca416843f (resolved from the remote, not from the dispatch brief).
Three gating findings, posted as inline threads on their lines. Ranked.
- The two inbound caps do not compose — inbound peers can still take 8/8 slots (
src/service/peer_pool.rs:337). Measured with a probe, not reasoned: 6 accepted-direct + 2 relayed-inbound leavesadopt_nat_connectionreturningErr(MaxConnectionsReached(8)). This is a regression of a property that held before this PR. - Test vacuity on both admission branches that carry the security argument (
src/service/gossip_handle.rs:2028,:2045). Deleting either keeps the suite green; a blanketheld.is_some()exemption — the bypass the sibling entry point explicitly warns about — also passes. - §2.4b:
dig-peer-protocoldeclared0.7, latest published0.9.0(Cargo.toml:49).
What is CORRECT, verified (so a fix round does not re-derive it)
- No rival-implementation divergence among the five
adopt_*entry points.adopt_direct_inbound_handlemirrorsadopt_relayed_inbound_handleexactly in ordering —require_running→ tier refusal → self → ban → onepeers-lock hold for budgets and insert (#1710 atomicity) →retire_slotoutside the lock (#71). The tier refusals are exact complements, so no slot can be accounted against the other tier. No failure-direction disagreement found. - The
dial_addrwidening is sound and genuinely narrow (src/service/state.rs:462). The newPeerSlot::Nat(n) if !n.is_outboundarm cannot change any previously-dialable slot: the only pre-existing producer of a non-outboundNatslot is the relayed path, which already returnedNoneviais_relayed.Live/Stubuntouched, so nothing relied upon narrows. The two undialability reasons are kept independent rather than collapsed, which is the right call. - Outbound diversity is not charged, as documented —
outbound_diversity_conflict(state.rs:528) filtersis_outbound() && !is_relayed(), so an accepted slot occupies no INT-006 /16 or INT-007 AS group. Verified in the code, not taken from the doc comment. - The cap is not a durable denial primitive. An
Observedslot answersis_closed()throughObservedSession, and the departed-peer reaper evicts it, so a dead inbound session cannot permanently hold a quarter of the pool or permanently lock its peer out of the other adoption paths. The fail-closed branch's triggering state does not survive the refusal. - The main fixture is not vacuous where it does assert.
a_direct_inbound_peer_is_typed_direct_inbound_and_is_not_dialablecarries a control slot with the opposite value for each ofvia,is_outboundanddial_addr, so none of the three can pass as a constant, and delivery is asserted as bytes the peer's owner received rather than a send-list length. §5.2 holds: every fixture address is IPv6 and nothing in this path dials.
Logged and left (non-gating, resolved by me — do not let these block merge)
- Predicate asymmetry at
gossip_handle.rs:2045vs:2053.replaces_accepted_directaccepts any slot kind (!is_relayed && !is_outbound) while the occupancy count requiresPeerSlot::Nat(_). Today this is vacuous — aLive/Stubinbound slot is dialable and refused by the:2028guard before it reaches here — but the two predicates describe the same set and disagree, so the exemption becomes uncharged occupancy the moment any non-Natslot can be undialable. Worth aligning while fixing finding 2. - The PR body is stale: it still opens
DO NOT MERGE — WIP, gate round not yet run, leaves three checkboxes unticked, and saysBlast radius: to be stated before this leaves draft. §2.4a requires the body to state the blast radius it checked. Update it with the fix round. - Reviewed read-only from git objects plus a scratch probe in
C:\tmp\worktrees\gossip-3124; the probe file was deleted andgit status --porcelainis empty atcd13d215.
loop-security — CHANGES-REQUIREDHead audited: Three GATING findings, all introduced by this diff, all on the attacker-reachable path. They compose: one attacker action triggers all three. GATING 1 (HIGH) — an accepted-direct slot can NEVER be displaced, so eviction pressure lands only on honest peers
Both siblings do:
Its own doc says so: "Admission bookkeeping lives HERE because this is already the one place every admission path funnels through to announce itself." This is now the one admission path that does not. The record can be created nowhere else. I checked every mutation of
So an accepted-direct peer holds no Why that is a security defect, not a missing metric. An accepted-direct peer therefore:
Attacker scenario. A stranger completes N mTLS handshakes under N identities and is adopted. Those N slots are permanently un-displaceable. Every displacement the #3128 discovered-holder path attempts now evicts an honest peer — one admitted by a path that registered itself. Push far enough that This inverts NC-12. Peers are meant to be cycled; here the only slots that cannot be cycled are the ones a stranger chose to open. The same omission also drops Uncovered by the new suite: GATING 2 (HIGH) — the accepted-direct tier has no PER-SOURCE bound; one host with free identities takes the whole tier
The diff's reasoning for not charging the outbound budget is correct and I am not disputing it. The finding is that nothing was put in its place, on the one inbound tier where a per-source bound is actually meaningful: a relayed slot's Identities are free, which is what makes this cheap. Attacker scenario. One machine, one IP, 38 locally-minted NodeCerts against the shipped default The PR's own test demonstrates the gap and passes. GATING 3 (MEDIUM-HIGH) — the two inbound caps sum to 150% of the pool, so the reserved quarter each one documents is not preserved
At the shipped default The new doc offers the separate counting as the safety property — "The caps are counted separately, so the two inbound tiers cannot pool their budgets" ( Attacker scenario. 38 accepted-direct + 12 accepted-relayed = 50 = Compounds with GATING 1: those 38 slots are also the un-displaceable ones, so the 12 remaining honest slots absorb all eviction pressure. The test at NON-GATINGA. This is NOT a second instance of the dig-gossip#82 shape, and I checked specifically. Every guard this diff adds fails closed: the relayed-tier refusal ( B. C. The D. The dependency split does not reach a cryptographic path this diff touches — non-gating, and correctly not ticketed. Checked and clean
What would clear the gate
Each wants a test that fails without it; the current suite passes under all three defects. Re-gate scope on the fix: this leg only. |
…#3124) Gate round on PR #83 returned three composing findings on the direct-inbound adoption entry point. They are fixed together because they are one attack. F1 — the entry point omitted `PoolEvent::PeerAdded`, which is the admission LEDGER rather than an announcement: `publish` is the only production path that creates a peer's `ActivityRecord`, `begin_activity` refuses to create one, and `activity_of` silently drops a recordless peer before the displacement planner sees it. An accepted slot therefore counted toward `connected`, sat in `cyclable`, and could never be the victim — so the only un-cyclable slots in the pool were the ones a stranger opened. Both sibling paths publish here; this one now does too, along with the Plumtree membership and connection count they also take. F3 — `max_direct_inbound` and `max_relayed_inbound` were each a reserved quarter of `max_connections` and were counted separately, so they bounded each tier and neither tier's share of the sum: 6 + 2 fills a `max_connections` of 8 and the next adoption failed with `MaxConnectionsReached`, strictly worse than before the direct tier existed. `max_inbound_total` is a new AGGREGATE bound charged by BOTH inbound entry points, and `max_direct_inbound` is now a reserved quarter of that budget rather than of the pool — which also makes it capable of binding at all, and reserves room on the tier a NAT'd peer has no alternative to. F2 — no per-source bound existed on any inbound path, and identities are free here (leaves are minted locally under a public CA). `max_direct_inbound_per_group` bounds accepted direct peers sharing one /16 at a quarter of the tier, minimum two, so one host cannot occupy the tier by minting certificates. F4 — the branches carrying the security argument were vacuous: every fixture used a fresh identity holding no slot, so neither the dialable-supersede refusal nor the `replaces_accepted_direct` exemption was ever reached, and the cap fixture held one tier from one source address. Four fixtures now offer an identity that already holds a slot, hold BOTH tiers at once, and vary the source group with a same-group/other-group control. Co-Authored-By: Claude <noreply@anthropic.com>
A 5 + 1 arrangement survived deleting the direct path's charge of the shared budget entirely, because its seventh adoption was relayed and the relayed path's charge refused it. At 4 + 2 neither tier is at its own cap, so a seventh peer of either tier can only be refused by the shared budget, and both are asserted. Co-Authored-By: Claude <noreply@anthropic.com>
…ger (#3124) The round-1 clause offered separate per-tier counting as the property that keeps the two inbound tiers from pooling their budgets. It does not: two reserved quarters counted separately sum to the whole pool. Replaces it with the three bounds that actually hold, in order, and states the admission-ledger requirement that makes an accepted slot displaceable at all. Co-Authored-By: Claude <noreply@anthropic.com>
Gate round 2 — all five findings fixed in one roundBranch What changed, per finding
At F3's probe, flippedThe reviewer measured Each fixture proved RED by reverting only its own fixTree committed first, then one mutation at a time, restored and verified clean between each. All seven
One false green was caught by this and is worth recording. The first version of the F3 fixture held Evidence
Not re-litigatedEverything both gates cleared: the five |
Check state at
|
loop-security re-audit of PR #83 — IN PROGRESS, not the verdictHead audited: Posting confirmed items as they resolve so they survive an interruption. This is not the verdict. Confirmed CLEAR so far
Confirmed measurements (evidence for findings still being written up)
Two things fall out of that table and are being written up as findings:
Still open (in progress)Displacement consequences of the new |
Correctness RE-GATE — PASSHead read: The three prior gating findings are genuinely fixed, and I proved it by mutation rather than by readingI re-ran four mutations myself in a private worktree at this head, restoring from a byte copy between each Baseline at this head: 9/9 green.
M1 and M2 are the load-bearing pair and they fail at DIFFERENT assertion lines (584 vs 565). That is the The vacuity was real. FindingsAll are non-gating; none blocks merge. Ranked. 1. 2. 3. 4. 5. Fixture-field uniformity — clean, with one uniform field. Source group varies (own group per peer, and The other gate-focus items
Merge stateVerdict recorded as a comment review, not an event — GitHub rejects both APPROVE and REQUEST_CHANGES from |
loop-security re-audit — FINDING 1, confirmed by runtime probe (IN PROGRESS, not the verdict)Head: F-NEW-1 — the RELAYED tier alone consumes the entire shared inbound budget, so the direct tier can be reduced to ZERO. Introduced by this delta.Severity: MEDIUM-HIGH. Introduced here ( The arithmetic. Both are the same one-line body applied to the same argument:
So Consequence 1 — Consequence 2 — the reservation is one-way, and it is a live denial. The design note at Probe (run at this head, in a throwaway integration test, since deleted). Six relayed circuits, then This is a regression, provable by inspection of the delta. At Attacker scenario. Identities are free on this path — the delta's own rationale for the new No attacker is required. A NAT'd node serving six relayed peers in normal operation reaches the same The property that is missing, stated rather than prescribed: neither inbound tier may be able to Remaining checks (test hook reachability, SPEC clause truth, the from-below bounds) follow in the verdict. |
loop-security re-audit — FINDINGS 2-5 (IN PROGRESS, not the verdict)Head: F-NEW-2 — the new per-group bound is a
|
loop-security RE-AUDIT — CHANGES-REQUIREDHead audited: The five round-2 findings are FIXED — I re-checked each and none survives. What follows is NEW, all of GATING
NOT GATING — name it, ticket it, do not hold the merge on it
What I checked and found CLEAR
Note on the fixtures, for the fix round
Read-only audit. No file in any checkout was modified; the probe cited in the F-NEW-1 comment ran in a |
…124) Four findings from the loop-security re-audit of PR #83, all introduced by this PR's own delta (cd13d21..31b8d64). F-NEW-1 — the relayed tier could consume the whole shared inbound budget. `max_relayed_inbound` and `max_inbound_total` were the identical body on the identical argument, so they were equal for every input while the direct tier was re-based one level down. The relayed cap was therefore vacuous (the aggregate is charged immediately after it on the same path, with an equivalent exemption), and the reservation ran one way: direct was held to 5 of 6 so a circuit always had a slot, relayed was held to 6 of 6 so six circuits left the direct tier zero. Probe-confirmed: six relayed circuits then one direct inbound, with no accepted-direct peer held at all, returned ConnectionFiltered. The relayed tier is also the one that can never be source-bounded, because a circuit's `remote` is the relay endpoint. `max_relayed_inbound` is now the same reserved quarter OF THE INBOUND BUDGET that `max_direct_inbound` is — 5 and 5 under an aggregate of 6 — so neither tier can exhaust the shared budget. F-NEW-2 — the per-group bound keyed IPv6 on a /32, a hosting provider's RIR allocation, because it borrowed the OUTBOUND diversity key. A group wider than the unit an attacker controls is conservative when it diversifies this node's own dials and is a denial primitive when it refuses a peer: at a cap of 2 a default node accepted two direct inbound peers from an entire provider worldwide, and two rented hosts there locked out every other customer. IPv6 is the preferred family (§5.2), so this was the common case. A new family-aware `inbound_source_group` keys IPv4 on its /16 (unchanged) and IPv6 on its /48, the end-site allocation unit, as distinct variants so the two key spaces cannot collide. The doc, the user-visible error string and SPEC clause 3 all said /16 and are corrected. F-NEW-5 — "at least two slots are always free for a peer THIS node dials, whatever mixture arrives", and the SPEC's "MUST be charged by every inbound entry point", were false as written: the aggregate is scoped to accepted `dig-nat` slots, while the Chia WebSocket listener inserts inbound Stub slots into the same map under only `max_connections`. Resolved by making the claim HONEST rather than TRUE — charging the aggregate at the listener would retighten this crate's most reachable inbound path inside a PR about direct inbound adoption, which is its own unit of work. The doc and the SPEC now state exactly which slots the budget counts and which it does not. F-NEW-4 — `__begin_pool_activity_for_tests` was `pub`, unbalanced, and incremented `in_flight` with no guard, permanently pinning a peer as non-displaceable and falsifying the "the count equals the number of live guards by construction" invariant. Removed: the balanced public `peer_activity_guard` already observes the same thing, and its guard decrements on drop. Tests: `a_full_relayed_tier_still_leaves_the_direct_tier_a_slot` (0 direct + a full relayed tier — the existing 4+2 fixture structurally cannot see F-NEW-1) and `two_sites_in_one_ipv6_provider_prefix_are_different_source_groups` (two /48 sites inside one provider /32 — the existing group fixture uses three addresses in ONE /48 and stays green under either keying). Both pin their bound from above and below. Co-Authored-By: Claude <noreply@anthropic.com>
Regenerate Cargo.lock for dig-ip 0.1.2 bump and apply rustfmt formatting fixes to test assertions that exceed line length. Fixes three failing required checks: - Cargo.lock in sync (--locked validation) - rustls inbound tests (--locked) - Test Suite + Coverage Co-Authored-By: Claude <noreply@anthropic.com>
…(#3124) The symmetric derivation took a reserved quarter twice — once to reach the inbound budget, once to reach a tier's share of it — and that collapses to ZERO for every max_connections <= 3, because reserving_a_quarter(1) == 0. A cap of zero does not reserve a tier's room, it closes the tier: at max_connections = 2 every relayed circuit this node serves was refused with "accepted relayed circuit cap reached (0)". That is the same starvation the symmetric derivation exists to prevent, inverted onto the relayed side. a_reserved_share_of floors a tier's share at one slot while the inbound budget can hold a peer, clamped to that budget so an empty budget stays empty. Both tiers keep the identical body, so the reservation still runs in both directions, and the aggregate max_inbound_total is charged on the same path — two floored tiers still cannot outgrow the budget they share. Also corrects a stale numeral in con_870's held-slot probe. That test's fixture was retuned by this branch from six circuits to five when max_relayed_inbound(8) became 5, but its post-refusal count was left at 6, which the retuned fixture cannot produce. The asserted PROPERTY — the count equals the cap after a refusal — is unchanged; only the constant tracking the cap moved. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Debug round: both gate failures fixed at
|
| config | fn | before | after |
|---|---|---|---|
max_connections = 2 (pool_3128) |
max_relayed_inbound |
0 — every circuit refused | 1 |
max_connections = 2 |
max_direct_inbound |
0 | 1 |
max_connections = 8 (con_870) |
max_relayed_inbound |
5 | 5 (unchanged) |
max_connections = 8 |
max_inbound_total |
6 | 6 (unchanged) |
The fix is a_reserved_share_of: a reserved quarter of the inbound budget, floored at one slot while the budget can hold a peer, clamped to the budget so an empty budget stays empty. Both tiers keep the identical body, so the reservation still runs in both directions; the aggregate max_inbound_total is charged on the same path, so two floored tiers still cannot outgrow the budget they share. A new unit test asserts the no-zero property over max_connections 0..=64 in both directions, plus the aggregate that keeps the floor honest.
The one test line that changed, and why that is not ratifying a defect
con_870's a_held_slot_does_not_exempt_a_circuit_from_the_accepted_relayed_cap asserted accepted == 6 after the refusal. That branch had already retuned this test's own fixture in a394ef9 from six circuits to five, when max_relayed_inbound(8) became 5 — and left the post-refusal count at 6. Five adoptions plus one refusal cannot produce six, at any cap, so the assertion was unsatisfiable by arithmetic rather than by behaviour: an internal inconsistency in this branch's own edit, not a shipped invariant.
The asserted property — the accepted count equals the cap after a refusal, i.e. the refusal was real — is unchanged and still asserted. Only the numeral tracking the cap moved. No test was relaxed, and pool_3128 was not touched at all.
Verification
Locally green: peer_pool unit tests (33), con_870 (9), pool_3128 (5), con_3124_adopt_direct_inbound (12). cargo fmt --check clean, cargo clippy --all-targets -D warnings exit 0.
Head c6e9a29. DO NOT MERGE — this PR remains DRAFT pending its gate round.
loop-security round 4 — IN PROGRESS, not the verdictHead resolved from the remote myself: One correction to the fix-round writeup before anything else: The cap table, derived independently from the source
The three adversarial questions put to this fix, answered:
F-NEW-1's property holds in BOTH directions at the default
|
loop-security round 4 — IN PROGRESS (2/2), not the verdictHead F-NEW-5 — FIXED, and the replacement claim is accurateI verified the new SPEC paragraph against the code rather than accepting it:
Stating the limitation rather than quietly narrowing the wording is the right disposition, and NEW GATING FINDING — F-R4-1: the crate now VIOLATES its own normative SPEC
That is
The proof needs no probe — it is two committed artifacts contradicting each other. This PR's own new assert_eq!(
max_relayed_inbound(2),
1,
"a two-slot pool still serves a circuit"
);SPEC says "At most 0". The code admits 1 and a shipped test pins it at 1. That is a conformance Why it gates. §4.2 makes The fix is one clause, and NOT GATINGF-R4-2 — a fourth F-R4-3 — special-purpose IPv6 prefixes are not canonicalized (defense-in-depth, follow-up ticket).
I am not gating this. All three are deprecated (RFC 7526 withdrew the 6to4 anycast relay) or do not |
loop-security RE-AUDIT round 4 — CHANGES-REQUIREDHead audited: Range: All four round-3 findings are FIXED. The The round-3 findings
The floor, adversariallyDerived independently.
Honest limit, not a defect: at
|
value at max_connections = 2 |
|
|---|---|
SPEC formula: 1 - max(1/4, 1) = 1 - max(0, 1) |
0 |
a_reserved_share_of(1) = reserving_a_quarter(1).max(1).min(1) = 0.max(1).min(1) |
1 |
The proof needs no probe — it is two committed artifacts contradicting each other. This PR's own new
unit test asserts the value the SPEC forbids:
assert_eq!(
max_relayed_inbound(2),
1,
"a two-slot pool still serves a circuit"
);SPEC says "At most 0". The code admits 1 and a shipped test pins it at 1. That is a conformance
failure, not a documentation gap.
Why it gates. CLAUDE.md 4.2 makes SPEC.md the authoritative contract an independent
reimplementation is built against, and the implementation this clause prescribes reproduces exactly
the denial regression this round exists to fix — a cap of 0 that closes the tier and refuses every
circuit with accepted relayed circuit cap reached (0). 4.2 also makes a behaviour change that leaves
SPEC describing the old behaviour INCOMPLETE, and 00e67c4 is precisely that. It is the same class the
round-3 verdict gated as F-NEW-5 — a normative clause that is false about the code in the commit that
ships it — and this PR has now produced three of them, which is why I am holding the line rather than
filing it.
Fix: one clause. Restate the relayed bound with the floor, and give clause 2's prose ("each a
reserved quarter of the INBOUND budget") and max_direct_inbound the same treatment — all three are
currently silent about it. This needs only a scoped doc re-gate, not another full round.
NOT GATING — fold in or ticket, do not hold the merge
F-R4-2 — a fourth /16 surface survived the F-NEW-2 sweep. src/service/state.rs:578 still reads
"the per-/16 bound [max_direct_inbound_per_group]", false for IPv6 now the key is a /48. F-NEW-2's
fix corrected three surfaces — the error string (gossip_handle.rs:2117), the peer_pool.rs:404 doc,
and the test helper — and missed this one. Crate-private doc comment, not normative, not user-visible;
fold into F-R4-1's commit. I swept every other /16 in src/: the rest correctly describe the OUTBOUND
subnet_group (INT-006) or the addrman tried-buckets, which genuinely are /16.
F-R4-3 — special-purpose IPv6 prefixes are not canonicalized (defense-in-depth, follow-up ticket).
canonical_ip folds only IPv4-mapped (::ffff:a.b.c.d), deliberately and correctly. It does not
fold the prefixes where a /48 boundary does not correspond to a site:
- 6to4
2002::/16embeds the IPv4 in bytes 2-5, so2002:<v4>::/48yields one group per IPv4
address. An attacker holding a single IPv4/24— one group under the v4 keying — would obtain 256
distinct v6 groups, andgroup_cap = 2means three suffice to fill the tier. - Teredo
2001:0::/32and NAT6464:ff9b::/96fail the other way, collapsing unrelated sources
into one group — the denial direction.
Not gated: all three are deprecated (RFC 7526 withdrew the 6to4 anycast relay) or do not arise on an
inbound server path, and I could not construct a scenario an attacker can drive today. Worth a ticket
for if the group bound is ever tightened.
What I checked and found CLEAR
- Secrets / credentials — nothing key-, token- or credential-shaped anywhere in the delta. The one
new error string isSafeText::from_untrustedover two numeric caps; no attacker-supplied string
reaches a log or a message. - Panics on adversarial input — the only new indexing in
src/iso[0]/o[1]on
Ipv4Addr::octets()([u8; 4]) and&o[..6]onIpv6Addr::octets()([u8; 16]), both statically in
bounds. No newunwrap,expect,panic!,unsafe, or truncating cast insrc/. - Reachability — who can invoke this — the delta adds no remote entry point. It changes the numeric
value of caps on the two existingdig-natadoption paths and re-keys one group predicate. The single
net loosening (0 -> 1inbound atmc <= 3) restores the pre-PR relayed value and is bounded by the
aggregate. - Cost asymmetry — the group scan is O(n) over the peer map per adoption, n bounded by
max_connections, unchanged in shape from round 3; the new key is two shifts over six bytes. - Public API + SemVer —
inbound_source_groupandInboundSourceGroupare additive;0.31.0to
0.32.0minor is the correct judgement. - Dependencies —
dig-ip "0.1"to"0.1.2", and0.1.2is the latest on the index (checked with
the requiredUser-Agent). The resolved version in the lock did not move. No dep added, no pin
loosened. The lock also moves three packages'windows-sysfrom0.61.2to0.52.0/0.60.2— a
resolver re-unification of a Windows FFI bindings crate with no logic; noted, not a finding. - Test quality — the new fixtures are non-vacuous and built to distinguish the fix from the defect:
the relayed fixture loops past its expected bound and assertspeer_countgrew, so anOkwithout an
insert would fail; the/48fixture asserts refusal at site A and admission at site B in one test;
the IPv4 test pins the unchanged/16and the mapped-v6 canonicalization. - No stray files —
.pr-progress.mdin the worktree is untracked and not part of the PR.
Read-only audit. No file in any checkout was modified, no probe was run against source, no git stash,
no mutating git command. I did not merge and did not undraft. PR remains DRAFT.
Once F-R4-1 lands with F-R4-2 folded in, a scoped doc-leg re-check is sufficient — the code findings are
all resolved and I would not re-open them.
The floor commit 00e67c4 changed a normative bound at every small pool and updated no SPEC clause, leaving SPEC.md carrying the pre-floor closed form `max_inbound_total - max(max_inbound_total/4, 1)`. That form evaluates to 0 at max_connections = 2, where this PR's own unit test pins max_relayed_inbound(2) at 1 -- a conformance failure between two committed artifacts, and one whose prescribed implementation reproduces exactly the denial regression #3124 exists to remove (a cap of 0 closes the tier rather than reserving room for its sibling). Clause 2 of the accepted-peer caps now states the bound both inbound tiers actually implement, including the floor and the clamp: min(max(b - max(b / 4, 1), 1), b), b = max_inbound_total(max_connections) with the floor stated as normative (an implementation MUST NOT emit a tier cap of 0 while b >= 1), the reason it cannot over-admit (clause 1's aggregate is charged on the same path), and worked values at max_connections 2, 3, 8 and 16. The relayed-registration "Bounded." clause defers to it rather than repeating a closed form that can drift. Also corrects a fourth stale `/16` doc reference missed by the F-NEW-2 sweep: is_accepted_direct named the per-group bound as `/16`, false for IPv6 since the group key became a `/48`. Documentation only -- no logic, cap, test or dependency change. Refs: dig_ecosystem#3124 Co-Authored-By: Claude <noreply@anthropic.com>
F-R4-1 FIXED + F-R4-2 folded in —
|
mc |
b = max_inbound_total(mc) |
code: each tier | new clause evaluated: min(max(b − max(b/4,1), 1), b) |
agrees | non-inbound slots left |
|---|---|---|---|---|---|
| 0 | 0 − max(0,1) sat → 0 |
0.max(1).min(0) = 0 |
min(max(0−1 sat, 1), 0) = min(1,0) = 0 |
✅ | 0 |
| 1 | 1 − max(0,1) = 0 |
0 | min(max(0,1), 0) = 0 |
✅ | 1 |
| 2 | 2 − max(0,1) = 1 |
0.max(1).min(1) = 1 |
min(max(1−1,1), 1) = min(1,1) = 1 |
✅ | 1 |
| 3 | 3 − max(0,1) = 2 |
1.max(1).min(2) = 1 |
min(max(2−1,1), 2) = min(1,2) = 1 |
✅ | 1 |
| 4 | 4 − max(1,1) = 3 |
2 | min(max(3−1,1), 3) = 2 |
✅ | 1 |
| 8 | 8 − max(2,1) = 6 |
5 | min(max(6−1,1), 6) = 5 |
✅ | 2 |
| 16 | 16 − max(4,1) = 12 |
9 | min(max(12−3,1), 12) = 9 |
✅ | 4 |
The four values called out in the brief — mc = 1, 2, 3, 8 — are the emphasised rows and the clause
agrees at every one. It agrees with the shipped unit assertions specifically at
max_relayed_inbound(2) == 1, max_relayed_inbound(3) == 1, max_relayed_inbound(8) == 5,
max_direct_inbound(8) == 5, max_inbound_total(8) == 6 (peer_pool.rs:1173-1180) — the assertions the
old clause contradicted at mc = 2.
Two subsidiary claims in the new text, checked rather than asserted:
- "
b <= max_connections − 1for everymax_connections >= 1" —b = n − max(n/4, 1)subtracts at
least 1 for alln >= 1, so a non-inbound slot survives at every configuration (rightmost column,
never 0 formc >= 1). - "the floor cannot over-admit" — it can only fire where
b >= 1(the.min(b)), and the aggregate is
charged after the tier cap on both adoption paths (gossip_handle.rs:2084,:2290), so atmc = 2
two tiers reading 1 against a budget of 1 admit one peer, refused thereafter by the aggregate.
The retained worked example 5 at max_connections = 8 is re-verified as still true (row 8 above); the
small-pool example added is mc = 2 → 1, which is exactly the value the old clause forbade.
F-R4-2 — the fourth stale /16
src/service/state.rs:578, is_accepted_direct's doc comment, named the per-group bound as per-/16.
Now: "The source group is an IPv4 /16 or an IPv6 /48, as inbound_source_group derives it." —
matching peer_pool.rs:404-405 and util/ip_address.rs:88-104. Crate-private doc comment; no behaviour.
What I deliberately did not touch
peer_pool.rs logic, the cap functions, any test, and the dig-ip bump — all CLEARED in round 4, and
changing them would invalidate a verdict that took four rounds to earn. I did not conclude the code was
wrong: the code is the correct behaviour and the SPEC was the stale artifact, so this is a
documentation-conformance commit in the direction the verdict specified.
cargo fmt --check clean. Ready for the scoped doc re-gate.
loop-security scoped doc re-gate (round 5) — IN PROGRESS, not the verdictHead audited: Posting these as they resolve so they survive a kill. Scope is the doc leg only — round 4's clearance Confirmed so farF-R4-1 closed form — the replacement clause is CORRECT at every value, not just the worked example.
The clause reproduces F-R4-2 — FIXED, and the sweep is complete. The One NEW finding, severity being settledThe new clause carries a rationale sentence that is false at
The unfloored value is This is new to Still working: whether this is gating, plus the normative-voice check. |
loop-security scoped doc re-gate (round 5) — PASSHead audited: Range: Scope was the doc leg only. Round 4's clearance of the peer_pool logic, caps, tests and the F-R4-1 — FIXED. The clause is true at every value, not only at the worked example.I transcribed the new closed form literally out of the SPEC and evaluated it against the code
Unit tests in this PR pin The clause reproduces I also checked the clause's two supporting invariants mechanically rather than accepting them:
Normative voice (section 4.2): satisfied. The clause states what IS — both caps are the same F-R4-2 — FIXED, and the sweep is complete
The
|
Both the SPEC clause and the peer_pool doc comment said the twice-applied
quarter "yields ZERO for every max_connections <= 3". It is zero only for
{0, 1, 2}: max_inbound_total(3) is 2, and the unfloored share of 2 is 1, so
the floor changes the value at exactly one input, max_connections = 2.
The overstatement is in the safe direction -- it describes the rejected
alternative rather than the shipped bound, and the closed form, the MUST and
the worked values all already pin max_connections = 3 to 1 -- but this PR has
carried three normative clauses that were false in the commit that wrote them,
and dig-gossip publishes on merge, so a false clause would ship permanently.
Found by the scoped doc re-gate and deliberately left non-gating there.
Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
DRAFT — gate round 2 fixes are in; awaiting the re-gate verdict. Do not merge.
Release-first half of dig_ecosystem#3124 — https://github.com/DIG-Network/dig_ecosystem/issues/3124
Consumer, blocked on this publishing 0.31.0: DIG-Network/dig-node#402
What
Adds a dedicated direct-inbound adoption entry point to
GossipHandle. dig-gossip exposed fouradoption paths and none accepted a direct inbound connection, so a node that ACCEPTS a direct mTLS
connection and serves the peer had nowhere to register it —
connected_peersunder-reported everyinbound peer.
Why not reuse an existing entry point
Each available reuse corrupts a different downstream decision:
adopt_relayed_inbound_handleRelayed;viaand the relayed caps derive from that tieradopt_nat_connectionis_outbound = true, charging outbound diversity budgets for a peer never dialedGate round 1 — three findings that composed into one attack, fixed together
F1 — accepted slots could never be displaced. The entry point omitted
pool.publish(PoolEvent::PeerAdded), which both siblings make. That call is the admission LEDGER, notan announcement: it is the only production path that creates a peer's
ActivityRecord,begin_activityrefuses to create one, andactivity_ofsilently drops a recordless peer before thedisplacement planner sees it. The slot counted toward
connectedand sat incyclablewhile beingstructurally incapable of ever being the victim — so displacement pressure landed entirely on peers
this node chose, and the only un-cyclable slots in the pool were the ones a stranger opened. That
inverts NC-12. The direct path now publishes, and takes the Plumtree membership and connection count
its siblings take.
F3 — the two inbound caps did not compose (a REGRESSION).
max_direct_inboundandmax_relayed_inboundwere each a reserved quarter ofmax_connections, counted separately:6 + 2fills a
max_connectionsof 8, and the next adoption returnedMaxConnectionsReachedwith nodisplacement. Before this PR the relayed cap alone held inbound to 6 of 8, so two slots always
survived. New
max_inbound_totalis an AGGREGATE bound charged by both inbound entry points, andmax_direct_inboundis now a reserved quarter of that budget rather than of the pool — whichmakes it capable of binding at all (it was previously equal to the aggregate and could never fire) and
reserves room on the tier a NAT'd peer has no alternative to. At the default 8: at most 5 accepted
direct, at most 6 accepted overall, always ≥2 slots for a peer this node dials.
F2 — no per-source bound. Nothing on any inbound path bounded /16, AS or per-IP;
outbound_diversity_conflictis gated onis_outbound(), and identities are free here (leaves areminted locally under the shipped public CA). One host could present one identity per slot and, with
F1, hold them un-evictably. New
max_direct_inbound_per_groupbounds accepted direct peers sharingone
/16at a quarter of the tier, minimum two (2at the default 8,8at 50) — two so a genuinepair of nodes behind one NAT is never refused.
F4 — test vacuity. Every fixture used a fresh identity holding no slot, so neither the
dialable-supersede refusal nor the
replaces_accepted_directexemption was ever reached, and the capfixture held one tier from one source address. Four fixtures added; each proved RED by reverting only
its own fix (table below).
F5 —
dig-peer-protocolis one minor behind (0.7declared,0.9.0published). Deliberatelydeferred to #82, which owns that uplift and already
carries the compiler's exact answer for all three break sites; it is semver-incompatible and not
mechanical. Every other edge is current:
dig-nat0.21.0,dig-tls0.4.0,dig-ip0.1.x.Blast radius checked
adopt_direct_inbound_handleis NEW, so it has no callers in this repo; its only consumer isdig-node#402, which is blocked on this release. The edits that do have a radius:
max_direct_inbound6 → 5at the default.max_relayed_inboundadopt_relayed_inbound_handleMaxConnectionsReached.con_870_relayed_inbound_pool_tests(relayed-only fixtures, cap 6) is unaffected and green.is_accepted_inbound/is_accepted_directPeerSlot::dial_addrNo public signature changed in this round. Risk concentrates on one behaviour: a node already holding
six accepted peers now refuses the seventh as
ConnectionFilteredwhere it previously either admittedit (relayed, pre-PR) or failed with
MaxConnectionsReached(this PR, round 1). That is the intendedfix, and the reserve it restores is asserted directly.
Not changed, deliberately
Both gates cleared these and they are not re-litigated here: the five
adopt_*paths carry identicalordering and complementary tier refusals;
dial_addr's new arm regresses nothing and outbounddiversity is correctly uncharged; §5.2 holds (no dial on this path). Two PRE-EXISTING issues are left
where they are —
is_peer_id_banned_atfails open on a poisoned mutex (state.rs), andPeerSlot::Liveinbound slots report ephemeral addresses as dialable (
listener.rs). This diff adds no new fail-open:all six new guards fail closed under one lock hold.
Status