test(peer): lock inbound dial+accept counted once - #591
Conversation
Epic dig_ecosystem#3124 fixed an UNDER-count: `connected_peers` omitted every inbound peer. Two legs shipped -- direct inbound (dig-node#523 / PR #402) and relayed inbound (dig-node#580 / PR #579) -- each with a behavioural test asserting COUNTED + still SERVED + RELEASED over a real mTLS connection. Neither leg, and no other test in this repo, covers the cross-class case: ONE identity holding both a dialled outbound slot and an accepted inbound slot. `connected_peers_json` (crates/dig-node-core/src/peer.rs:370) maps one JSON row per `connected_pool_peers()` entry with no grouping by `peer_id`, so if the pool ever held that identity twice, `peer_count()` would read 2 for one peer and the RPC would emit two rows with the same `peer_id` and opposite `direction` -- the same defect class the epic exists for, inverted, and just as invisible to a consumer. `adopt_inbound_peer_in_pool`'s own doc (peer.rs:3658-3662) lists "a peer already holding a dialable slot" among the refusals dig-gossip can return, so the de-duplication is believed to live in dig-gossip. It has never been exercised from dig-node. This test exercises it. The test drives real connections throughout: the outbound slot is created through `adopt_nat_connection` (the single outbound adoption path, called in production from seams/dig_peer/bootstrap.rs:218 and seams/dig_peer/pex.rs:652), and the inbound slot by a real mTLS `dig_nat::connect` against `serve_peer_rpc_listener_with`. Nothing constructs a pool entry by hand. NOT YET COMPILED. This commit is a salvage checkpoint written after the authoring lane was killed mid-flight by a session rate limit, pushed so the work is durable rather than lost. The API surface it assumes (`dig_gossip::NatPeerConnection::new`, `dig_nat::PeerConnection`'s field list, `PeerSession::client`/`server`, `GossipHandle::disconnect`) is unverified against the pinned dig-gossip v0.32.0, and the sleep-then-assert ordering in steps 2-3 still needs to be replaced by a positive signal that the inbound accept path actually ran. A follow-up commit on this branch compiles it, fixes both, and records the result. Refs #3124
The first cut of this test slept 300ms and then asserted `peer_count() == 1`. That assertion passes just as happily when the inbound accept path has NOT RUN YET as when the inbound adoption was correctly refused -- it measured a race, not the property. On a loaded box a real double-count defect would have read green. Replace the sleep with a happens-before proof. `adopt_inbound_peer_in_pool` is called at peer.rs:3602, strictly before the accepted session starts answering RPC at peer.rs:3614, so a successful `dig.getNetworkInfo` round-trip over the inbound connection proves the server has already reached and returned from the adoption attempt. Moving that round-trip ahead of the count and row assertions makes it do double duty: it is still the "refusing adoption must not refuse service" proof, and it is now also the ordering barrier the count read needs to be meaningful. The second sleep, before the post-drop count read, is removed for a different reason: the inbound leg was refused adoption, so `release_inbound_pool_slot` ran with `adopted = None` and is a no-op. There is no pending async decrement for the drop to race against. Also corrects the surviving-slot comment against the real dig-gossip behaviour rather than inferring it from dig-node's doc. `adopt_direct_inbound_handle` REFUSES outright -- it does not supersede -- whenever the held slot's `dial_addr()` is `Some`: an accepted connection never supersedes a slot this node can dial. The assertion was already right; the reasoning behind it is now sourced. Refs #3124
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
loop-reviewer verdict: PASS
Head SHA reviewed: 08d696c67fe05f2cda176b42f84f086cdb8903a7
Independent verification performed (no code edited, no cargo run beyond the prebuilt test binary):
Scope/blast radius — confirmed test-only: git show --stat on both commits shows exactly one file, crates/dig-node-core/tests/inbound_pool_membership.rs. No production code touched.
Happens-before claim (item 1) — HOLDS. Read peer.rs's accept loop directly: adopt_inbound_peer_in_pool(...).await is called and its result bound to adopted in a match that is awaited to completion, and only THEN does serve_peer_session_from_with(caller, &mut session, responder, pex).await run, sequentially, same task, no spawn in between. So a successful dig.getNetworkInfo round-trip over the accepted session is real proof the adoption attempt already ran and returned before the RPC could have been answered. The barrier is real, not a race in different clothes.
release_inbound_pool_slot no-op claim (item 2) — HOLDS. Read the function: if let (Some(gossip), Some(slot)) = (gossip, adopted) { ... } — when adopted is None (the refused-adoption case this test drives), the body never runs; nothing async is queued. Removing the second sleep is justified.
dig-gossip refuse-not-supersede rule (item 7) — REAL, verbatim. Checked against the pinned rev in Cargo.lock (dig-gossip rev=1a3391662ecce1a3cbe8b74122a52bbb1b28d3ee, matching the checkout the PR cites) — adopt_direct_inbound_handle returns Err(GossipError::ConnectionFiltered(...)) early, with the comment "An accepted connection NEVER supersedes a slot this node can dial ... the #870 rule," whenever held.dial_addr().is_some(). This is a hard refusal (early return before the insert), not a supersede-by-newer-connection. The test's surviving-slot assertion and its sourcing comment are both correct.
Real connections, not hand-built pool entries (item 3) — confirmed: outbound slot goes through gossip_a.adopt_nat_connection(outbound_conn), the same production entry point used by bootstrap.rs/pex.rs; inbound slot goes through a real dig_nat::connect(...) against the listener spawned via serve_peer_rpc_listener_with. loopback_nat_conn only substitutes a tokio::io::duplex for a TCP socket underneath a real dig_nat::PeerConnection/PeerSession — the same pattern already used by this crate's own unit tests, not something newly invented to dodge the accept path. Acceptable.
Row counting (item 6) — confirmed matching_rows filters connected_pool_peers() on *peer_id == pool_id_b and counts, not len() of the whole vec.
Assertion content, not shape (item 5) — confirmed the "still served" assertion checks resp["result"]["served_method"] == "dig.getNetworkInfo", a content check, not a field-presence or length check.
API surface — spot-checked against the pinned dig-gossip source: connected_pool_peers() -> Vec<(PeerId, SocketAddr, bool)>, dial_addr() returns None whenever !is_outbound for a NAT slot, connected_pool_peers_detailed()'s is_outbound field — all match how the test and the PR body use them.
PR body's "nothing new is reachable" claims — each bullet checked against cited lines in peer.rs and dig-gossip; all four are accurate: adoption call passes only pool id/remote/tier/ObservedSession/broadcast_sink=None; dial_addr()==None for a non-outbound NAT slot (state.rs:460-468) so it is absent from dialable_pool_peers(); the broadcast_sink=None reasoning (peer.rs ~3690-3711) is the three-measured-facts comment, present and unchanged; the "service was already unconditional" claim matches adopt_inbound_peer_in_pool's own doc ("a refusal is logged and the connection is served uncounted — the behaviour that shipped before this call existed").
Test execution — ran the prebuilt binary directly (did not invoke cargo; another lane owns the build lock): binary mtime (00:24) is newer than the source file's last edit (23:37), confirming it is built from this head. 4 passed; 0 failed in 13.62s, matching the PR's own evidence block independently.
Teeth check (item 8) — the PR body honestly marks the mutation-revert teeth check as pending ("filled in before this PR leaves draft"), not claimed as done. Per brief, this is acceptable and non-blocking for a DRAFT PR — flagging as a non-blocking note rather than a defect.
Non-blocking findings (posted as review comments below, to be resolved — do NOT block merge)
crates/dig-node-core/tests/inbound_pool_membership.rs— vacuous self-check. The assertionassert_eq!(*b_identity.peer_id().as_bytes(), b_bytes, "the inbound dial must authenticate as the SAME identity as the outbound slot")is checkingb_identity's own derived bytes againstb_bytes, which was itself derived fromb_identitytwo lines earlier (let b_peer_id = b_identity.peer_id(); let b_bytes = *b_peer_id.as_bytes();). It is true by construction regardless of whether the SERVER's mTLS-derived identity for the inbound connection actually matches B — it does not touch the server side at all. The real proof that the same identity landed in both slots is the laterpeer_count() == 1invariant (if the inbound leg authenticated as a different identity, the accepted-direct admission would succeed as a NEW slot and the count would go to 2, which the test does check) — so the property IS still proven, just not by this specific line. Recommend either deleting the misleading self-check or replacing it with something that reads the identity dig-node derived server-side (e.g., via a debug hook / log assertion) if one is cheaply available; otherwise reword the assertion message so it does not overclaim what it tests.- Step numbering gap — comments jump
Step 2b->Step 3->Step 5(noStep 4). Cosmetic only.
Teeth check status
PENDING, per PR body, honestly disclosed as pending rather than falsely claimed done. Not required for this DRAFT PR per brief; should land before the PR leaves draft.
Not reviewed
pairing.rs/server.rs(owned by #3191, out of scope, confirmed not in diff).- dig-node#581 (REACHED-by-broadcast leg) — correctly out of scope per module doc.
- Did not run
cargo test/cargo build(build lock owned by another lane); ran the prebuilt binary directly instead and confirmed its mtime postdates the source.
loop-security verdict: PASSHead SHA audited: Scope
Environment note (not a PR finding)The worktree at Claim-by-claim audit ("what can an inbound peer now reach that it could not before")
The identity key
Refuse-not-supersede rule (#870) is real: Converse checked and is real but not exploitable cross-identity: an accepted connection for peer_id P can supersede an existing non-dialable (inbound) slot for the same peer_id P (gossip_handle.rs:2122-2142, "newest-wins... free"). Since One finding worth surfacing — PR-body claim is slightly overbroad, not a live vulnerability"Nothing" is correct for capability/service (bullets 1-4 above all hold), but is incomplete for visibility: once adopted, an inbound peer's Recommend: (a) tighten the PR-body sentence to "no service capability" rather than unqualified "Nothing," since a reader citing "nothing is reachable" for the visibility/PEX-amplification question would be relying on a claim this diff does not establish; (b) ticket whether Test-fixture reachability
VerdictPASS at Not covered
|
Teeth check ran, and it FAILED — the test survives a mutation it should have caughtRun by the orchestrator after the authoring lane was capped mid-check. Reporting the honest result What was mutated
Err(e) => {
tracing::debug!(peer_id = %pool_id, error = %e, "inbound peer not adopted into the pool; serving it uncounted");
Some(InboundPoolSlot { peer_id: pool_id, superseded }) // was: None
}Built clean ( ResultGreen. The test did not notice. Why — and it is the same defect class this PR already fixed once, in a place we did not lookStep 5 reads: drop(inbound_conn);
assert_eq!(gossip_a.peer_count().await, 1, "the inbound session ending must not release the outbound slot it never owned");There is no barrier between the That is exactly the failure this PR's commit What this does and does not invalidateSteps 2-4 stand. Their barrier is real and independently verified by the review gate: adoption is Step 5 does not stand. Its assertion is currently unfalsifiable and should be treated as absent. What Step 5 needsA "the count STAYS 1" check with a positive signal that the server processed the disconnect, not an Until that lands, this PR should either fix Step 5 or delete it. Keeping an assertion that cannot fail State
|
|
The security gate's non-blocking visibility finding is now its own ticket: DIG-Network/dig_ecosystem#3241 — PEX advertises an accepted inbound peer's ephemeral source port as a known-peer hint. It is pre-existing shipped behaviour from an earlier PR in the #3124 epic, not introduced by this diff, so this PR does not carry it. What this PR does carry is the corrected wording: an inbound peer gains no capability and no service, but it does gain visibility. |
Step 5 asserted the count stays 1 with a single instant read right after dropping the inbound session. A teeth check proved this cannot fail: the server's teardown does not run synchronously with the client-side drop, so the read fires before the server has acted, regardless of whether the eventual release is correct. Replace it with a poll across a 2s window that would catch an erroneous drop to 0 if it ever occurred, and explain in the comment why a window is required instead of an instant read. Also drop the same-identity assertion in Step 2 (b_bytes was derived from b_identity two lines above, so it was true by construction and checked nothing server-side); the real property is carried by peer_count() == 1. Renumber the remaining steps (2b -> 3, 3 -> 4) now that there is no gap. Refs #3124
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
loop-reviewer verdict: PASS
Head SHA reviewed: 850a077fc5af925a7a7cd965418c01abb03e96a0
Re-gate scope: the previous PASS was on 08d696c6; this reviews only the new commit 850a077f
("bound Step 5's non-event check to a real window") plus a re-check that nothing else moved.
1. Does the new Step 5 have teeth?
Yes, independently reasoned through, not taken on the PR body's word. The loop at
inbound_pool_membership.rs:690-700 runs assert_eq!(peer_count(), 1, ...) INSIDE the while body,
once per 50ms tick for up to 2s -- it panics on the FIRST tick where the count is wrong, it does not
sample once at the end and then declare victory. That is the structural difference from the original
defect (a single instant read racing the server's async teardown): here every one of ~40 samples across
the window is a live chance to catch an erroneous drop to 0, so as long as the server's release path
(if it were buggy) runs and settles within 2s of drop(inbound_conn), the loop observes it. 2s is
generous for a local in-process loopback teardown (accept-task wakeup + one release call, no network
RTT) -- the confirmed second teeth-check run (mutation applied, RED at line 691, left: 0, right: 1)
is direct evidence the window is wide enough on this box. A window that only sampled at the end would
be the same defect in a longer coat; this one does not do that.
2. Flakiness in the other direction?
Checked the fixture (inbound_pool_membership.rs:540-705): only two identities are live in this test
(3124-dualslot-server, the RPC responder; 3124-dualslot-peer-b, both the dialled and accepted leg).
No third peer, no background reconnect/ping-interval churn is driven inside this test function, and the
window sits strictly between drop(inbound_conn) and the explicit disconnect(&pool_id_b) call that
follows it -- nothing else in this fixture touches gossip_a's pool during that span. Low flake risk.
3. Did removing the vacuous assertion lose anything?
No. The removed assertion compared b_identity.peer_id().as_bytes() against b_bytes, and b_bytes
is *b_peer_id.as_bytes() where b_peer_id = b_identity.peer_id() two lines above (line 557-558) --
true by construction regardless of server behavior. The replacement comment's claim -- that a real
identity mismatch would admit a SECOND slot and peer_count() would read 2 -- holds: Step 3's
peer_count() == 1 assertion (line 626) is server-side and would fail exactly under that mismatch
scenario. No coverage lost.
4. Renumbering
Grepped the whole file: Step 1 (561), Step 2 (592), Step 3 (607, was "2b"), Step 4 (636, was "3"), Step
5 (670, unchanged) -- sequential, no gaps, no stray old-number references left anywhere (including the
one internal cross-reference at line 686, which correctly says "Step 3").
5. Steps 2-4 unchanged otherwise?
Diffed: only the same-identity assertion removed from Step 2 and the two renumbering comment edits.
The previously-verified happens-before barrier (RPC round-trip before the count read, peer.rs:3602
vs peer.rs:3614) and the #870 refuse-not-supersede check are untouched.
6. PR body honesty
Confirmed via gh api repos/DIG-Network/dig-node/pulls/591 --jq .body: the teeth-check section states
plainly that the first run FAILED and names the root cause (instant read racing async teardown),
then the fix, then a second run that goes RED-then-GREEN with the actual panic line and byte
values. Not sanded into a clean pass. Trust section reads "No capability and no service — but yes to
visibility," carries the visibility bullet citing seams/dig_peer/ping.rs:704-710 and
seams/dig_peer/pex.rs:460-468, and points at DIG-Network/dig_ecosystem#3241 as pre-existing,
out-of-scope behavior -- matches the brief's requirement exactly.
Diff scope
Confirmed test-only: git show 850a077f --stat touches only
crates/dig-node-core/tests/inbound_pool_membership.rs (34 insertions, 16 deletions). No production
code in this commit.
Threads
Both pre-existing inline threads (the vacuous-assertion note and the step-numbering-gap note) are
already isResolved: true and are directly addressed by this commit. No new blocking findings from
this re-gate -- zero threads opened, zero left open.
Not run
Did not run cargo (per brief -- machine-wide build lock, 15 min cost) and did not re-execute the
prebuilt binary; the orchestrator's own build/run at this exact clean head (4 passed, binary mtime
postdates both peer.rs and the test file) is accepted as sufficient build/green evidence and was not
re-derived.
loop-security — RE-CONFIRM at new headVerdict: PASS This is a scoped delta re-confirm over the previous PASS ( 1. Diff is test-only — confirmed
The test diff itself: Step 5's non-event check ( 2. Can a production caller release a slot its session did not adopt? — No, and the guard is explicit
The 3. PR body accuracy — accurateThe Trust section ("What an inbound peer can now reach that it could not before") now opens with "No capability and no service — but yes to visibility", keeps the four previously-verified bullets, and adds a fifth citing Scope audited
|
Gate verdicts carried forward from
|
1 participant
Outcome: GREEN-LOCKS-PROPERTY
What
Compiles and hardens the regression test salvaged from a killed lane:
a_peer_that_is_both_dialled_and_accepted_is_counted_oncein
crates/dig-node-core/tests/inbound_pool_membership.rs. This is a characterization test that LOCKS a never-provenproperty — it is not a TDD red/green from a live bug, and it is not being reported as a caught defect. The property it
locks: a peer identity that holds BOTH a dialled (outbound) pool slot and simultaneously makes an accepted (inbound) mTLS
connection to the same node is counted exactly ONCE, and the surviving slot is the pre-existing DIALLED one, per
adopt_direct_inbound_handle's documented refusal rule (the accepted connection never supersedes a slot this node candial — the #870 rule).
Refs #3124
Evidence
Teeth check — the first run FAILED, and that failure found a real defect in the test
A teeth check (deliberate mutation of
adopt_inbound_peer_in_poolincrates/dig-node-core/src/peer.rs, confirm RED,revert, confirm GREEN) was run twice.
First run, before this PR's latest commit: the mutation made the
Errarm ofadopt_inbound_peer_in_poolreturnSome(InboundPoolSlot { peer_id: pool_id, superseded })instead ofNone— i.e. a REFUSED adoption reports itself asadopted, which should make the inbound session's teardown incorrectly release the outbound slot it never owned. Built
clean, ran the test. It stayed GREEN. Root cause: Step 5's assertion read
peer_count()in a single instant rightafter
drop(inbound_conn), with no barrier between the drop and the read — dropping the client side does notsynchronously run the server's teardown, so the read fired before the server had acted, regardless of whether the
eventual release was correct. The assertion could not fail. Full write-up:
#591 (comment)
Fix: Step 5 now polls
peer_count()across a 2-second bounded window instead of reading it once. A comment abovethe check explains why an instant read cannot prove a non-event and a window is required. Also removed a vacuous
same-identity assertion in Step 2 (see Trust section) and renumbered the steps (2b -> 3, 3 -> 4) now that there is no
numbering gap.
Second run, against the fixed test:
RED, for the right reason. Mutation then fully reverted (
git diff --stat crates/dig-node-core/src/peer.rsempty,never committed), rebuilt, reran the full file:
GREEN again, confirmed against a binary whose mtime postdates the revert. Step 5 now has teeth.
Fixes applied to the salvaged test
dig-gossip/dig-nat API surface against the pinned revs (dig-gossip 0.32.0, git rev
1a3391662ecce1a3cbe8b74122a52bbb1b28d3ee; dig-nat 0.21.2 and dig-tls 0.4.1 from the registry):dig_gossip::NatPeerConnection::new,dig_nat::PeerConnection's field list,PeerId::from_bytes(re-exportedfrom
dig-tls),PeerSession::client/server,GossipHandle::disconnect, andis_outboundon the detailedpool-peer struct all matched exactly as written — no API repair was needed.
peer_count() == 1— a count that reads as "correctly de-duplicated" and "the inbound accept simply hasn't run yet"identically.
adopt_inbound_peer_in_poolis called atpeer.rs:3602, strictly BEFORE the serve loop startsanswering RPC (
peer.rs:3614), so a successfuldig.getNetworkInforound-trip over the inbound session is proof theadoption attempt already ran and returned. The RPC step was moved ahead of the count/row assertions.
adopt_direct_inbound_handle's admission code(dig-gossip
service/gossip_handle.rs): it explicitly REFUSES — never supersedes — whenever the held slot'sdial_addr()isSome. This is a refusal, not a supersede-by-newer-connection; the test's assertion that theoutbound slot survives is correct as originally written, and the comment now cites the actual admission code path
rather than only the doc comment.
the defect it fixes).
b_identity.peer_id().as_bytes()tob_bytes, a value derived fromb_identitytwo lines earlier — true byconstruction, and it never touched the server side.
Blast radius
Test-only change to one integration-test file (
crates/dig-node-core/tests/inbound_pool_membership.rs). No productioncode was committed;
peer.rswas mutated and reverted only as part of the teeth check above (never staged, neverpushed).
What an inbound peer can now reach that it could not before
No capability and no service — but yes to visibility.
adopt_direct_inbound_handle/adopt_relayed_inbound_handletake only thepool id (the 32 bytes the mTLS handshake already proved), the remote socket address, the traversal tier, an
ObservedSession, andbroadcast_sink = None(peer.rs:3602-3609,adopt_inbound_peer_in_pool).PeerSlot::dial_addr()returnsNonewhenever!is_outbound(service/state.rs), sodial_addr == None, it is absent fromdialable_pool_peers(), and it takesno outbound diversity budget. A direct inbound peer's
remoteis its ephemeral SOURCE port, never a stable dialtarget — confirmed by the existing
an_accepted_inbound_peer_is_counted_served_and_releasedtest in this file.broadcast_sink = Nonemeans dig-gossip reports the peer unreachable for broadcast; no gossip fan-out.peer.rs:3691-3711documents the three measured reasons no sink exists today (no wire frame for a broadcast, nomatching
classify_requestroute, no ingest path into dig-gossip's crate-privateinbound_tx).adopt_inbound_peer_in_pool's own doc states a refusedadoption is logged and the peer is served uncounted — "the behaviour that shipped before this call existed"
(
peer.rs, doc above the function). Adoption grants no service the peer did not already have.(peer_id, remote_addr)entersconnected_pool_peers(), which is read byknown_peers()(seams/dig_peer/ping.rs:704-710, which discards the_outboundflag at line 708) and by PEX'sspawn_pool_feeder(seams/dig_peer/pex.rs:460-468, whose own commentsays it advertises "the FULL pool (both directions)"). Neither reader distinguishes dialable from non-dialable
entries, so adoption is what causes this node to advertise that peer's ephemeral source port to the mesh as a
known-peer hint. This is pre-existing shipped behaviour from an earlier PR in the epic — not introduced by this
diff — and is bounded: PEX treats every entry as an unverified hint, not a trust grant. Not a blocker, but the
unqualified word "Nothing" that used to sit here was wrong for this axis and is corrected.
Net: adoption grants VISIBILITY (a count, a row in
connected_pool_peers_detailed, and — per the fifth bullet — aPEX/
known_peers()hint) and costs the peer a capped slot. No capability, no service change.This PR's test is the direct behavioural evidence for the "service was already unconditional" bullet: it drives an
inbound peer the pool REFUSES to adopt (because the same identity already holds a dialled slot) and shows it is still
served, by bytes — see the
dig.getNetworkInforound-trip assertion in the test, moved to run before the count/rowassertions specifically so it also serves as the ordering barrier described above.
Not attempted / out of scope
blocked, per the test file's own module doc.