feat: carry the PEX-known DIG peer count, not just the connected one - #10
Conversation
Lane report — and my brief's premise was WRONGThe field shipped, but not from Why: What it used instead: Also relevant: the node's PEX engine is wired only to the mTLS node↔node listener. Relay-discovered The field, for the #2569 lanepub known_dig_peer_count: Option<u32>, // wire key: "known_dig_peer_count"
Render as "known" / "discovered", never "total" or "network size". The pair is the diagnostic:
Version-claim collision — the recurring shape
Two concurrent branches independently claiming the same version is now the third time this shape has Deliberately left undonedig-node still pins GatesContract: fmt, clippy Four mutations, each applied to a committed tree by file copy and restored:
UnverifiedThe field's live value — everything is proven by test; nothing was measured against a running The investigation answer, and it is bigger than this ticket(a) — introductions arrive and every dial fails. Root cause: the peers advertise IPv6-only |
544f503 to
a5b23bd
Compare
…t holds `PeerCountsResult` could say a node has zero DIG peers but not why: a node connected to nobody looks identical to a node with nobody to connect to, and the two are different faults with different remedies. `known_dig_peer_count` adds the node's own discovered-peer address book size beside the connected count, so a client can tell a reachability failure from a discovery failure. The field is view-scoped by construction and documented as such. It is ONE node's lower bound, never the size of the network -- naming it a total would assert global knowledge from a local view. The SPEC forbids both deriving it from `dig_peer_count` and presenting it as a network size. Additive and optional in both directions: an older node omits it and decodes as `null` (unknown, never a measured zero -- pinned by a KAT that a default-to-zero implementation fails), and an older client ignores the extra key. Refs dig_ecosystem#2570 Co-Authored-By: Claude <noreply@anthropic.com>
0.11.0 and 0.12.0 both landed on main while this branch was open, so the version this change ships under moved twice beneath it. A new OPTIONAL field on an existing result type is a compatible addition, so the bump from main's 0.12.0 is MINOR. Co-Authored-By: Claude <noreply@anthropic.com>
a5b23bd to
6fc4155
Compare
|
Rebased onto
Conflicts, both resolved as a UNION (neither side contradicted the other):
Zero conflict markers remain (grepped after each step — Local gate, all green: Left DRAFT deliberately — the orchestrator undrafts immediately before merging. Remaining: CI to terminal + the gate round. |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate: PASS — head 6fc415543eb4bc2ea9de575deb150705e5d24a4b
Reviewed by reading (disk at 100%; no local build). CI green on this exact tip: 8/8 incl. Coverage 98.41%, Format/Clippy/Build/Docs, version-increment, commitlint, CodeQL x4.
1. Stale-duplicate trap — REFUTED
SPEC.md carries exactly one WalletSyncStatusResult bullet (line 337), and it is the five-phase version with watched_addresses:
{phase:"not_started"|"syncing"|"synced"|"no_wallet_enrolled"|"wallet_not_unlocked", peak_height:u32|null, chia_peer_count:u32|null, watched_addresses:u32|null}
…followed by "A conforming node MUST emit one of the five tokens above". No pre-0.11.0 three-phase copy survives anywhere in the file. Exactly onePeerCountsResultbullet (line 297). README: onecontrol.peerCountstable row (117) + one prose mention (47, correctly updated to "three integers"), onecontrol.wallet.syncStatusrow (136).
2. Union resolution lost nothing
Test-function set diffed against BOTH parents: vs a5b23bd (pre-rebase) and vs origin/main — additions only, zero deletions in either direction. HEAD's set = main ∪ a5b23bd ∪ the two new #2570 tests.
3. Compatibility claims are true, not merely asserted
- No
deny_unknown_fieldsanywhere insrc/(only the explanatory comment atsrc/kats.rs:833). Old-client-ignores-extra-field holds. Option<u32>decodes an absent key asNoneby serde's own rule;an_older_nodes_peer_counts_decode_with_the_known_count_unknown_not_zeropins it and is not vacuous: a#[serde(default = …)]-style zero-default landsSome(0)and fails, and the fixture's non-zerodig_peer_count: 6means an aliasing decoder landsSome(6)and also fails. Both wrong implementations are distinguished.knowing_of_peers_while_connected_to_none_is_expressibledecodes0 / 3 / 41; an aliased decoder yieldsSome(0)known and fails.each_peer_count_key_names_its_networkadditionally pins the exact key SET and the exact emitted byte string.
4. SemVer / derives — correct
Field is appended last, so wire order is unchanged ({"dig_peer_count":6,"chia_peer_count":3,"known_dig_peer_count":41}). No renames, no derive changes; Option<u32> is Copy, so Copy/Eq still hold. Under cargo's 0.x rules 0.12.0 → 0.13.0 is the compatibility-breaking bump, which correctly covers the source-break for downstream struct-literal constructors.
5. Versions / changelog
Cargo.toml 0.13.0 and Cargo.lock 0.13.0 agree. CHANGELOG.md untouched vs origin/main.
6. Emission ordering — satisfied
The 0.12.0 rule (SPEC.md:420-427) is scoped to new PHASE TOKENS, which break an older consumer's parse. An appended optional field cannot: with no deny_unknown_fields, an older client ignores it. SPEC.md documents the omission direction explicitly ("The field is OPTIONAL on the wire… a consumer MUST decode that omission as null").
Non-gating notes (no fix required to merge)
src/kats.rs:757— assertion message still reads "give the two counts independently"; there are now three. Cosmetic string only, inside a passing assertion.SPEC.md:297block documents the omission direction but not the mirror (an older CLIENT ignoring the extra field) that the rustdoc atsrc/results.rs:~492does state. A reimplementer reading only SPEC.md could over-apply the emission-ordering rule to this field. One sentence would close it.PeerCountsResultis not#[non_exhaustive]— consistent with every other type inresults.rs, so this PR introduces no drift; noting only that each future additive field will keep costing a 0.x-breaking bump. Crate-wide decision, not this PR's.
No inline threads opened; zero unresolved threads on the PR.
DO NOT MERGE — work in progress, lane is live
Early draft per the standing directive that all work carries a PR from the start.
Intent (#2570)
Surface the number of DIG-network peers this node knows of, not only those it is connected to.
PeerCountsResultcurrently carriesdig_peer_count(connected) andchia_peer_count. The node alreadycomputes
known_count()via PEX, but the contract does not carry it, so dig-app cannot render it. On mymachine that is the difference between a useless number and a useful one:
connected_peers: 0while therelay reports 6.
PEX already exists on both sides — nothing is being built here. The relay has
dig-relay/src/pex.rs(RLY-008), embeddingdig_pex::PexEnginein its introducer role, one engine pernetwork_id, advertising only first-hand peers and never re-advertising node-sent data ("an introduceris not a gossip amplifier"). The node has the node↔node half (#166). This change is exposure only.
The honesty constraint that decides the field name
No count anywhere is "the total DIG network." The node's
known_count()is peers this node haslearned of — a lower bound from one node's view. The relay's is peers registered with that relay —
nodes on another relay, or none, are invisible to it. Naming either "total network peers" asserts global
knowledge from a local view, the same class of false claim as reporting a wrong balance. The field is
named and documented as view-scoped, and its doc states what it does NOT include.
Existing
Optionsemantics are preserved: the contract already documents "0is an observed zero;nullis unobservable" — an unknown count must not render as zero.Also being answered with evidence
connected_peers: 0againstrelay.peer_count: 6has two very different explanations: introductionsarriving and every dial failing, or introductions never arriving at all. The lane has a live node and
logs and is measuring which, rather than inferring.
Rendering belongs to #2569's lane, which owns the header — not this branch.
Refs #2570, #2569.