Skip to content

sec(peers): dig.getPeers serves the wildcard address to REMOTE peers as a dial candidate — move is_usable_contact to the type boundary #349

Description

@MichaelTaylor3d

Task

A fifth unguarded site emits the wildcard address — and this one serves it to REMOTE PEERS as a dial
candidate, not merely to a human as display text.

crates/dig-node-core/src/peer.rs:1307, inside pool_peers, emits the pool address unguarded:

{"host": addr.ip().to_string(), "port": addr.port()}

Its caller at peer.rs:1373 answers dig.getPeers on the peer-RPC server, so a wildcard renders as
{"host":"::","port":0} to a remote peer.

That is materially worse than #253's symptom. #253 was a display falsehood in dign peers — a human
reads [::]:0 and is misled. This is a non-destination advertised as a dial candidate to other nodes,
which is a peer-discovery correctness problem: a peer that dials it wastes a connection attempt, and a
peer that caches it caches a hole.

The pattern this confirms — and the fix that follows from it

The lane that fixed #253 predicted this in its own report:

is_usable_contact was applied at three of four sites. A guard adopted call-site-by-call-site rather
than at the type boundary will keep missing one — and the fourth was the only one a human ever looked
at.

It was right, and the count is now five. net.rs:104 is_usable_contact currently guards
dht_contact_from_pool_addr, download.rs:1555, the DHT seam, and — since 90ed299f — the
connected_peers_json display site. pool_peers is the one nobody looked at.

So do NOT write a fifth if. Move the guard to the type boundary: make it structurally impossible
to emit a pool address without having answered "is this a destination?" — the predicate already asks
exactly that question, and it deliberately accepts loopback while rejecting unspecified-IP and port-0.

The constraint on the fix

Withhold the ADDRESS, do not drop the PEER. A relay-reached peer is still a real peer and still
reachable via the relay — removing it from dig.getPeers entirely would be a different and worse
regression. Omit the address (or the host/port pair), keep the row.

Check what a remote consumer does with a missing host/port versus a wildcard one. dig.getPeers is a
peer-facing wire surface, so an omitted field is a shape change for other nodes — unlike #253's
control.peerStatus, where an ecosystem grep confirmed no external consumer destructures peer rows.
That check has not been done for this surface. Do it before choosing between omission and an explicit
null.

Evidence

  • Reproduce first: a relay-reached peer must currently appear in a dig.getPeers answer with
    {"host":"::","port":0}.
  • Prove it with a placement revert — guard call removed, predicate intact and still compiling — not a
    deletion, which fails as a compile error and proves nothing behavioural.
  • The fixture must discriminate in both directions: a relay-reached peer with no endpoint and a
    truthful peer at a real destination, so neither "emit everything" nor "blank every relay peer" passes.
  • Read passed-counts, never ok. Any mutation asserts its patch applied; parse the test result:
    line, never stderr
    . One cargo process at a time.

Provenance

Found by the review gate on DIG-Network/dig-node PR #347 (90ed299f, eco#2760 / #253), which fixed
the fourth site. Pre-existing and outside #253's scope, so explicitly non-gating there.

Release hazard: dig-node's cron cuts a stable tag at midnight UTC unattended, so main reaches
real users without a human. The merge gate is the last line of defence.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions