chore(deps)!: bump dig-gossip to v0.17.12 (duplicate-connection fixes #1691/#1703/#1762) - #118
Conversation
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate: PASS (0 gating findings)
Verified by execution in an isolated worktree, not by reading. cargo test -p dig-node-core --lib on b54a62ca: 452 passed / 0 failed.
Test vacuity - MUTATION EXECUTED, non-vacuous
Reverted the ordering fix in crates/dig-node-core/src/download.rs (the retain + insert(0, *addr) pair, back to the old append-if-absent) and re-ran:
test peer::tests::the_selector_pool_feed_upserts_a_readopted_peer_instead_of_counting_it_twice ... FAILED
assertion failed: the upsert carries the newest session's address
left: "203.0.113.7" right: "203.0.113.70"
The new test genuinely discriminates the fix from the nearest wrong implementation (append-if-absent), and the ordering assertion is load-bearing: PoolProviderLocator::find_providers maps addrs in vec order, and dig_download::addr::dial_candidates uses a STABLE sort_by_key, so within one address family the node's order survives to the dialer. File restored; full suite re-run green.
Behaviour change 1 - PeerAdded republished + total_connections incremented on a SUPERSEDE
Every dig-node consumer audited by file:line; none over-counts:
src/peer.rs:1852DHT routing feed ->DhtHandle::add_peer-> dig-dht 0.8routing.rs:114 offer()REMOVES the old contact and pushes the new one, so the id stays unique AND the addresses are replaced (not merged stale-first). Correct.src/peer.rs:1881map_gossip_pool_event-> selector registry: keyed upsert by peer_id. Correct.src/seams/dig_peer/pex.rs:474-> dig-pexengine.rs:183 self.known.insert(peer_id, entry): keyed replace, newest address advertised. Correct.src/download.rs:830connected pool:HashMap<peer_id_hex, Vec<SocketAddr>>upsert - one candidate per identity (asserted by the new test's found.len() == 1 plus its distinct-identity control).total_connectionsis READ NOWHERE in dig-node (git grep finds only the Cargo.toml comment). The user/RPC-facing countcontrol.peerStatus.connected_peersis fed fromGossipStats::connected_peers(src/peer.rs:2176-2178), which upstream builds aspeers.len()of the live keyed map (gossip_handle.rs:2233). No metric over-counts.
Behaviour change 2 - DuplicateConnection no longer raised on adopt
git grep DuplicateConnection over crates/ returns ZERO dig-node match sites (only the Cargo.toml prose). No branch goes dead, no guard goes vacuous, no error is silently reclassified. The upstream refusal removal is confirmed at gossip_handle.rs:1269-1287 (supersede + the replaces_held_slot / is_outbound_reconnect budget carve-outs), and the new node-side test a_stale_relayed_slot_does_not_refuse_the_direct_adoption pins reachability of that behaviour from dig-node with a genuinely killed session, a via/address assertion (which distinguishes a real supersede from an Ok that leaves the dead relayed slot in place), and a different-/16 control peer so the INT-006 diversity cap cannot filter the control for the wrong reason. That fixture design is right.
Version
0.66.0 on the workspace/binary is correct: minor for a 0.x dep-major adaptation that changes runtime behaviour without breaking dig-node's own RPC contract.
SPEC / DEVELOPMENT_LOG coherence
Root SPEC.md:2953-2960 now states the newest-session-first candidate ordering, the retained trailing fallbacks, and that a re-adopted peer is ONE candidate. DEVELOPMENT_LOG.md correctly records the rev-pin, the 3-rev lockstep and the behavioural half. Nothing in either SPEC still asserts a DuplicateConnection/refusal on adopt (swept for "already connected", "supersed", "adopt_nat_connection", "DuplicateConnection"), so there is NO contract split.
NON-GATING follow-ups (do not block merge; ticket these)
crates/dig-node-core/SPEC.md:864(7.8 connected-pool contract) is SILENT on the new shape. It says churn events feed the selector registry + PEX feeder but never states that PeerAdded may be REPUBLISHED for an already-added peer_id on a supersede, nor that every consumer MUST therefore be keyed by peer_id and idempotent. Not false, so not gating - but a reimplementer building from 7.8 would not learn the invariant this whole audit rests on. One bullet fixes it (ideally in this PR, since the root SPEC already got its half).- Unbounded stale-address retention + a truncation interaction (
crates/dig-node-core/src/download.rs:836). The retain+insert never caps the vec; only PeerRemoved clears it, and a supersede emits no PeerRemoved.dig_download::addr::dial_candidatessorts IPv6 before IPv4 then truncates to MAX_DIAL_CANDIDATES = 4. So a peer that re-adopts repeatedly over IPv6 and then lands a working IPv4 direct dial can have its NEWEST address truncated out of the dial set - the exact failure this commit removes, reached from the address-family side. Pre-existing in shape (append-if-absent grew unbounded too), so not a regression here. Suggested fix:addrs.truncate(4)after the insert. - Cross-family ordering is not under the node's control (same site). Because of that v6-first sort, "the newest session leads the dial order" holds only WITHIN an address family; a relayed-IPv6 -> direct-IPv4 supersede still spends the first dial on the dead circuit. Either weaken the comment or move the guarantee into dig-download.
crates/dig-node-core/Cargo.toml:3- dig-node-core stays 0.23.0 while its public surface now carries dig-nat 0.14 / dig-dht 0.8 types (semver-breaking for the library). Harmless today (it has git deps so it cannot be published, and has no out-of-repo consumer) and consistent with the repo's "library crates keep independent versions" note, but the lib version now misstates its API.
Disclosure (shared checkout)
All reading + the mutation probe ran in a throwaway worktree (created from b54a62c, removed, git worktree prune run). In the shared primary checkout modules/apps/dig-node I ran only non-mutating commands (git fetch, git grep, git show, git worktree add/prune/list) and wrote no file there. Note for the orchestrator: that primary checkout ALREADY carries uncommitted modifications to Cargo.toml, Cargo.lock and crates/dig-node-core/Cargo.toml, present independently of this review (another lane's working state); I did not touch or restore them.
Bump dig-gossip pin to v0.17.12 (3e352d91) so the three duplicate-connection fixes (#1691 inbound, #1703 connect_to, #1762 adopt_nat_connection) reach production. Co-Authored-By: Claude <noreply@anthropic.com>
…ck cascade dig-gossip 0.17.0 is a `dig-nat` 0.13 -> 0.14 cascade, so the rev bump drags the whole peer stack with it or the graph resolves two dig-nat instances and the NatRuntime/RelayStatus values this crate builds stop typechecking: dig-nat 0.13 -> 0.14 dig-dht 0.7 -> 0.8 dig-download 0.11 -> 0.12 dig-peer 0.6 -> 0.7 dig-peer-selector 0.6 -> 0.7 The 0.14 line is fully published, so the manifest's own prediction that this needed dig-gossip and dig-peer-selector published first is now satisfied. The workspace [patch.crates-io] revs for dig-gossip's vendored additive chia-protocol/chia-sdk-client forks move to the same rev (the #1529 3-rev lockstep). Their vendored trees are byte-identical between dbb03430 and 3e352d91, so this is coherence, not a content change. Co-Authored-By: Claude <noreply@anthropic.com>
…ssion address dig-gossip republishes `PoolEvent::PeerAdded` when a fresh verified session supersedes a stale slot for the same identity (#1691/#1703/#1762) — behaviour the v0.17.12 bump makes reachable for the first time. The connected-pool feed appended the new address, so the superseded (typically dead) address stayed FIRST in the candidate's dial order and every later fetch spent a failed dial on it. The newest address now leads; older ones trail as fallbacks rather than being discarded. Adds the two #1771 regressions on the node side: a stale relayed adoption followed by a direct adoption of the same peer_id succeeds and both node-derived peer views report ONE peer at the newest address over the newest tier; and the download-side feed upserts a re-adopted peer instead of counting it twice. Each carries a distinct-identity control so neither can pass against a pool that stopped admitting peers. Co-Authored-By: Claude <noreply@anthropic.com>
…-stack bump MINOR 0.65.1 -> 0.66.0. No public API is removed or renamed, and the wire is unchanged, so this is not major; it is more than a patch because the node gains an observable capability it did not have: a peer whose pool slot is stale can now be re-adopted instead of refused, which is what lets peers reconnect at all. SPEC.md §pool-locator now states the candidate dial order (newest session first, superseded addresses retained as trailing fallbacks) and that re-adoption upserts rather than growing the candidate set. DEVELOPMENT_LOG.md records why a rev-pinned peer-stack dep hides shipped fixes indefinitely, that the dig-nat major moves as one atomic cascade, and the behavioural half of the bump (republished PeerAdded, total_connections as a lifetime counter). The dig-node-core manifest comment predicting this blocker is replaced with what is now true, including that --all-features is not a runnable dig-gossip configuration. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
b54a62c to
78b9156
Compare
Closes DIG-Network/dig_ecosystem#1771.
What this is
The
dig-gossipgit pin moves fromb065e70c(a v0.16.0-era rev) to v0.17.12 (3e352d91),recovering three duplicate-connection fixes that were released upstream and never reached any
running node: #1691 (inbound
precheck_inbound_peer), #1703 (connect_to), and #1762(
adopt_nat_connection— the path EVERY dig-nat connection is adopted through). A live EC2 runreproduced all three symptoms simultaneously because of this one stale pin.
dig-gossip is pinned by rev because it cannot publish to crates.io until dig-peer-protocol (#681)
lands, so nothing bumps it automatically and
cargo updatewill not move it. It drifted elevenminors behind.
Why it is not a one-line rev bump
dig-gossip 0.17.0 is
chore(deps)!: bump dig-nat to 0.14, and dig-node-core constructs its ownNodeCert/NatConfig/NatRuntime/RelayStatus/TraversalKindvalues and passes them INTOdig-download, dig-gossip and dig-peer-selector — so two dig-nat instances do not merely bloat the
tree, those calls stop typechecking. The dig-nat major therefore moves as ONE atomic cascade:
b065e70c(0.16.0)3e352d91(0.17.12)cargo updateresolved this with no duplicate instances added, andtests/dependency_tree.rs(which asserts single-instance against the resolved lock) passes.
API changes the 11-minor jump required
Exactly one, workspace-wide: dig-download 0.12 marked
DownloadConfig#[non_exhaustive], sodownload.rsbuilds it by mutating adefault()instead of a struct literal. No rename or signaturechange implied a behavioural difference.
[patch.crates-io]revsYes, moved — to
3e352d91, keeping the #1529 3-rev lockstep (a git dep's own patches do not applytransitively, so the workspace re-declares dig-gossip's vendored additive
chia-protocol/chia-sdk-client).git diff dbb03430 3e352d91 -- vendor/chia-protocol vendor/chia-sdk-clientisempty, so this is coherence rather than a content change — but leaving the patch rev behind would
have resolved two copies of a patched crate, which builds green and behaves oddly.
The behavioural half — how this node consumes the superseding pool
Both flagged risks were checked on this side:
PoolEvent::PeerAddedis republished on a supersede. Every dig-node consumer is keyed bypeer_idand idempotent — the DHT routing feed (peer.rs), the selector registry, and thedownload-side connected pool (
download.rs::on_pool_event) — so none over-counts. One realdefect found and fixed: the connected pool APPENDED the new address, leaving the superseded
(typically dead relay) address FIRST in the candidate's dial order, so every later fetch spent a
failed dial on it. The newest address now leads; older ones trail as fallbacks rather than being
discarded.
total_connectionsis incremented on a supersede. dig-node never reads it. It is documentedupstream as a lifetime counter that never decrements, so incrementing on a supersede is consistent
with its own contract; the node reads
GossipStats::connected_peersandpool_stats().connected,which are live keyed-map sizes (
peers.len()) and therefore unique-peer counts.DuplicateConnectionno longer raised. Zero references anywhere in dig-node — no code or testdepended on the refusal.
Blast radius checked
gitnexus is disabled in the loop (worktree hangs), so this was done by direct symbol sweep + code
read: every
PeerAdded/total_connections/pool_stats/DuplicateConnection/adopt_nat_connectionreference in the workspace, plus every peer-stack version declaration acrossall four manifests, plus the
dependency_tree.rssingle-instance invariant. The touched symbols areNodeContent::on_pool_eventand theDownloadConfigconstruction inNodeContent::new; bothin-crate, no public signature changed.
Verification
cargo test --workspace— 31 test binaries, all green, 0 failures (452 dig-node-core unittests + 246 dig-node-service + integration suites).
cargo clippy --all-targets --locked -- -D warnings(CI's exact command, dig-node's real featureset) — clean.
cargo fmt --all --check— clean.Publish to crates.ioallsuccesson3e352d91. Not bumped onto an unknown tip.a_stale_relayed_slot_does_not_refuse_the_direct_adoptionfailswith
DuplicateConnection(abab…)— the exact defect, not a compile error;the_selector_pool_feed_upserts_a_readopted_peer…fails onthe address assertion.
silently stopped admitting peers. Assertions are on observable pool state (
pool_stats_json,connected_peers_json, the locator's candidate set) — never a log line.What remains for an e2e: these run over loopback duplex sessions, so they prove the supersession
logic and the node's consumption of it, not a real relayed circuit dying mid-flight. A two-node
ephemeral-EC2 run (#1062) is what proves the live reconnect.
Version
0.65.1 → 0.66.0 (minor). No public API removed or renamed and the wire is unchanged, so not
major; more than a patch because the node gains an observable capability it lacked — a peer whose pool
slot is stale can be re-adopted instead of refused, which is what lets peers reconnect at all.
SPEC.md §pool-locator now states the candidate dial order and the upsert-not-grow rule;
DEVELOPMENT_LOG.md records the rev-pin trap, the atomic-cascade coupling, and the behavioural half.
No docs.dig.net change: nothing user-facing (command, flag, endpoint, URN) changed.