p2p: resolve DNS-named addnodes before advertising them (#910)

[frstrtr]

Fixes #910.

A DNS-named --addnode was advertised to peers as 127.0.0.1.

Verified root cause

src/core/netaddress.cpp — NetAddress::Write_IPV4 opens at line 11; the defect is the fallback block at lines 23-28 (pre-fix):

std::vector<std::string> split_res;
boost::algorithm::split(split_res, ip, boost::is_any_of("."));
if (split_res.size() != 4)
{
    LOG_WARNING << "Write_IPV4: non-dotted address '" << ip << "', substituting 127.0.0.1";
    split_res = {"127", "0", "0", "1"};
}

NetAddress stores the address as a string, and for a hostname seed that string is the hostname — deliberately, so every reconnect re-resolves it. The serializer split it on ., saw 3 parts for rov.p2p-spb.xyz, and wrote loopback. Write_IPV4 is what we put on the wire, so any peer that learned that seed from us got an address pointing back at itself.

Confirmed on this tree:

• The outbound connect path resolves correctly — src/core/factory.hpp:157, Client::resolve() issues async_resolve(addr.address(), addr.port_str()) per dial, and pool::BaseNode (src/pool/node.hpp:37) inherits core::Factory<Server, Client>, so this is the one dialler for all five lanes. That matches the field observation that the hotel is connected to 66.151.242.154 while logging the substitution warning for usa.p2p-spb.xyz.
• The hostname reaches the wire via the addr store: bootstrap seeds go in verbatim (core::AddrStore::load), get_good_peers() (src/pool/node.hpp:169) hands them back verbatim, and the ten per-coin HANDLER(getaddrs) sites serialize them into an addrs reply.

Two corrections to the reported details, neither of which changes the fix:

1. The cited netaddress.cpp:11 is the function signature, not the substitution — that is lines 23-28.
2. The loopback substitution only catches names with a label count other than 4. A four-label name (seed.node.example.com) passed the size() != 4 check and fell through to lexical_cast, which threw per label and produced 0.0.0.0 plus four LOG_ERROR lines. So the pre-fix behaviour was "3-label name -> loopback, 4-label name -> garbage". Both are covered here.

Which resolver was reused, and why

The existing one — core::Factory's Client::resolve() in src/core/factory.hpp. Its completion handler already holds both the configured host string and the resolved endpoints, so the fix memoises the A record there (core::record_resolved_host). No second lookup is introduced anywhere, which is the point: two independent resolution paths that can disagree would be its own defect. It also means the fix is cross-coin by construction — every lane dials through this one Client.

Where resolution now happens

Not at serialization, and not blocking anywhere.

• Resolve: on the outbound dial, where it already happened. Async, on the io_context, unchanged.
• Serialize: Write_IPV4 does a shared_lock map lookup — NetAddress::wire_ipv4_literal(). Never DNS. A dotted quad short-circuits before the lookup is even reached.
• Store: the hostname stays the stored and dialled identity. address(), to_string(), the addr store key and the JSON persistence are all untouched, so a DNS seed still survives an IP change — that is the entire reason to use a hostname. A re-resolution that returns a different address simply overwrites the memo (logged at INFO).

"localhost" is handled explicitly as a genuine resolution to 127.0.0.1, not as a fallback. That also keeps the default-constructed NetAddress (whose m_ip is "localhost") serializing to exactly the bytes it always has.

On resolution failure

Never loopback. Two layers:

1. Skip the entry. NetAddress::is_wire_advertisable() is false for a name with no memoised A record, and all ten HANDLER(getaddrs) sites now continue past such entries instead of putting them in the addrs reply. The seed is still dialled and still re-offered on the next sweep once a dial has resolved it — only the advertisement is withheld.
2. Loud, unroutable, if one slips through. Write_IPV4 cannot skip — the message has a count prefix and the field is fixed-width — so an unrenderable address becomes 0.0.0.0 with a LOG_ERROR naming the address. 0.0.0.0 is classified AddrClass::unspecified and rejected by the existing is_connectable() at the receiving end, so it misdirects nobody. Loopback is worse than no address precisely because it is valid-looking.

A memoised AAAA answer is also treated as "unknown" rather than being fed into the four-byte IPv4 encoder.

Wire format

Unchanged. Write_IPV4 still emits exactly 16 bytes (12-byte IPv4-in-IPv6 prefix + 4 address bytes), and the byte-producing code — the split/lexical_cast/append loop — is untouched. The only change is what string it is handed: for a numeric literal, itself, byte-for-byte as before.

The one behavioural difference for non-hostname input is that a malformed 4-part string (1.2.3.999, a.b.c.d) now takes the loud 0.0.0.0 path instead of silently emitting 1.2.3.231 / 0.0.0.0. That input was already producing garbage.

What the tests prove

src/core/test/netaddress_resolution_test.cpp, folded into the existing allowlisted core_test target — no new add_executable, so it cannot land as a "Not Run" (the #769 trap).

Wire pins (must not move a byte):

• DottedQuadWireBytesAreUnchanged — seven KATs, including both hotel seed IPs, 0.0.0.0 and 255.255.255.255, pinned to their exact 4 bytes inside the exact 16-byte envelope.
• GenuineLoopbackStillSerializesAndRoundTrips — 127.0.0.1 still gives 7f 00 00 01, and NetService("127.0.0.1", 18999) still round-trips through Serialize/Unserialize at 18 bytes. The hotel legitimately runs --addnode 127.0.0.1:18999.
• LocalhostStillMapsToLoopback — "localhost" and the default-constructed NetAddress still serialize to loopback.
• DottedQuadRoundTripsThroughUnserialize.

The defect (these fail on the pre-fix serializer):

• UnresolvedHostnameDoesNotSerializeToLoopback — rov.p2p-spb.xyz must not produce 7f 00 00 01. Pre-fix it produces exactly that.
• ResolvedHostnameSerializesToItsARecord — after the connect path records the A record, both hotel hostnames serialize byte-identically to their literals.
• HostnameWithFourLabelsDoesNotSerializeAsGarbage — covers the second pre-fix path described above.
• ReResolutionTracksAnIpChange, ResolvedHostnameKeepsItsNameForReconnect — the seed keeps its name and follows its IP.
• MemoRefusesNonNumericAnswersAndLiteralKeys — the memo can never become a second source of names, and an AAAA answer does not corrupt the IPv4 envelope.
• AdvertisabilityGate — the skip predicate, including that genuine loopback stays advertisable.

Regression fence:

• EveryLaneFiltersUnrenderableAddrsFromGetaddrs — a source KAT asserting the is_wire_advertisable() guard exists and precedes the addrs.push_back(...) in all ten src/impl/{btc,ltc,bch,dgb,dash}/protocol_{actual,legacy}.cpp getaddrs handlers, so no lane can quietly regress.

Blast radius

src/core/netaddress.cpp and src/core/factory.hpp are shared core: all five lanes (ltc / btc / bch / dgb / dash), both Actual and Legacy protocols. This is not DASH-only — src/impl/btc/config_pool.hpp:212-222, src/impl/ltc/config_pool.hpp:216-228 and src/impl/bch/config_pool.hpp:251-259 ship hardcoded DNS-name bootstrap seeds (p2p-spb.xyz, ekb.p2p-spb.xyz, rov.p2p-spb.xyz, usa.p2p-spb.xyz, ml.toom.im, btc.p2pool.leblancnet.us, bch.p2pool.leblancnet.us, siberia.mine.nu), so a default-configured BTC/LTC/BCH node was advertising those as loopback with no operator action at all.

Not reward-affecting: no share, block, payout or consensus path is touched. Peer-discovery quality only.

Not changed here

• version's addr_from is still the hardcoded NetService{"0.0.0.0", <compile-time default port>} in every lane, and the coin-facing version still sends 192.168.0.1 / 0.0.0.0. Both are pre-existing and separate from this issue.
• p2p(addrme): legacy self-probe checks 127.0.0.0 instead of 127.0.0.1; DASH never originates addrme #882 (addrme self-probe comparing 127.0.0.0) is untouched — that is the receive end of the same loopback mishandling.