fix(p2p): judge ipv4-mapped addresses by their embedded ipv4 form

[nullPointerEnjoyer]

Problem

IPv4-mapped IPv6 addresses (\::ffff:a.b.c.d) were validated as plain IPv6: the routable-address check excluded only a fixed list of IPv6 classes, so the mapped form of a private, loopback or link-local address passed as public. Such addresses could be announced by an inbound peer and then dialled or re-served to other peers, they were grouped under the public IPv6 group (defeating per-group connection limits), and bans plus duplicate-connection checks keyed on the raw IpAddr let a banned host reconnect under its mapped spelling.

Change

• SocketAddress canonicalizes at construction (new, FromStr, serde): the mapped form is reduced to the embedded IPv4. Only the mapped form is reduced; other IPv4-embedded encodings (6to4, Teredo, NAT64) are treated as plain IPv6, documented at the helper.
• as_discoverable_socket_address and IsGlobalIp for Ipv6Addr judge mapped input by the embedded IPv4 rules and return the canonical form.
• AddressGroup::from_peer_address groups on the canonical IP.
• BannableAddress canonicalizes, including persisted bans deserialized on load; connection-limit and duplicate-connection checks compare canonical IPs.
• Peer db load folds legacy rows keyed by the mapped spelling into the canonical key, merging values (tried state beats new, later ban expiry wins) and reporting whether any row was rewritten so the transaction is committed only when needed.

Testing

• Unit tests for the address predicates, grouping, canonical construction via new/parse/serde, ban-key folding, and the storage fold (merge precedence, unparseable-row reporting, anchor/anchor-less tables).
• p2p and p2p-types test suites green; workspace fmt and clippy clean.

[github-actions]

🔍 OpenCodeReview found 6 issue(s) in this PR.

• ✅ Successfully posted inline: 2 comment(s)
• 📋 Routed to summary by policy: 3 comment(s)
• ⏭️ Skipped (overlap with history): 1 comment(s)

---

📄 p2p/types/src/peer_address.rs (L57-L59)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

This builds a full SocketAddr (including the port) via SocketAddr::from(self) just to extract the IP. A direct match on self returning the variant's IP would be cheaper and clearer on this classification hot path.

💡 Suggested Change

Before:

    pub fn canonical_ip(&self) -> std::net::IpAddr {
        crate::socket_address::canonical_ip_addr(std::net::SocketAddr::from(self).ip())
    }

After:

    pub fn canonical_ip(&self) -> std::net::IpAddr {
        match self {
            PeerAddress::Ip4(s) => IpAddr::V4(s.ip.into()),
            PeerAddress::Ip6(s) => crate::socket_address::canonical_ip_addr(IpAddr::V6(s.ip.into())),
        }
    }

---

📄 p2p/src/peer_manager/peerdb/storage.rs (L79-L82)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category documentation)

The trait doc says the return value indicates "whether any rows were rewritten", but rows dropped due to unparseable keys also make this return true. The caller contract (commit only on true) is still correct, but the documented guarantee is slightly inaccurate — a true result may mean only deletions were staged. Consider rewording the doc to "whether any rows were modified".

💡 Suggested Change

Before:

    /// Returns whether any rows were rewritten. A `false` result guarantees no writes were
    /// staged on this transaction, so callers may skip committing it; a `true` result means
    /// the transaction must be committed for the rewrites to take effect.
    fn normalize_legacy_mapped_address_keys(&mut self) -> crate::Result<bool>;

After:

    /// Returns whether any rows were rewritten or dropped. A `false` result guarantees no writes were
    /// staged on this transaction, so callers may skip committing it; a `true` result means
    /// the transaction must be committed for the rewrites/deletions to take effect.
    fn normalize_legacy_mapped_address_keys(&mut self) -> crate::Result<bool>;

---

📄 p2p/src/peer_manager/peerdb/storage_impl.rs (L248-L250)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

Each of the four rekey_mapped_rows calls collects every prefix-matching rewrite into a Vec before applying it. This is fine for the expected handful of legacy rows, but combined with the every-startup full scan noted in the TODO, a corrupted store containing many mapped-prefix keys would buffer all rewrites in memory at once. Not blocking, but worth keeping in mind when the one-time versioned migration replaces this scan.

💡 Suggested Change

Before:

    let mut rewrites = Vec::new();
    let mut drops = Vec::new();
    for (key, encoded) in map.prefix_iter(&())? {

After:

    // Note: all rewrites are buffered until iteration completes, since the underlying
    // iterator must not be interleaved with get/del/put on the same map.
    let mut rewrites = Vec::new();
    let mut drops = Vec::new();
    for (key, encoded) in map.prefix_iter(&())? {

[github-actions]

The migration is inconsistent in how it handles corrupt legacy rows: an unparseable key is skipped with a warning, but an unparseable value returns a hard P2pError::InvalidStorageState from load_storage_v3, aborting the whole load path and preventing the node from starting over a single corrupt row. Since this scan runs on every startup, consider skipping (or logging and dropping) rows whose values fail to decode, mirroring the lenient key policy, or at minimum documenting why a hard failure is intended here.

[nullPointerEnjoyer]

Keeping the strict behavior here: a corrupt value fails the whole load anyway through the normal read path (e.g. get_known_addresses in storage_impl.rs decodes every row via prefix_iter_decoded + process_results and maps failures to InvalidStorageState), so skipping only inside the repair pass would not get the node running — it would just move the failure a few lines down with less context. Failing in the fold also names the offending key.

[github-actions]

This short-circuit silently changes classification for every caller of IsGlobalIp for Ipv6Addr outside this crate — e.g. dns-server/src/crawler_p2p still calls Ipv6Addr::from(addr.ip).is_global_unicast_ip() directly, so mapped-form crawler addresses are now judged by IPv4 rules. The change is correct, but it is a cross-crate behavioral change with no explicit caller updates in this PR; consider auditing/updating those call sites (or routing them through PeerAddress::canonical_ip) and noting the change in release notes.

Suggestion:

Suggested change

	if let Some(ipv4) = self.to_ipv4_mapped() {
	return ipv4.is_global_unicast_ip();
	}
	// Callers that receive Ipv6Addr directly (e.g. dns-server crawler) now also
	// judge mapped addresses by the embedded IPv4; verify/align those call sites.
	if let Some(ipv4) = self.to_ipv4_mapped() {
	return ipv4.is_global_unicast_ip();
	}

[nullPointerEnjoyer]

Checked the cross-crate call sites: the crawler's IPv6 arm (dns-server/src/crawler_p2p/crawler_manager/mod.rs:370) now judges mapped addresses by their embedded IPv4 form, which is exactly the policy this PR intends — a mapped address should follow IPv4 rules everywhere it is classified. The dns-server test suite passes with the change.

[github-actions]

A single row with a corrupt value now aborts the whole startup load via P2pError::InvalidStorageState, turning a database that previously loaded fine into a hard node-start failure. That is inconsistent with how unparseable legacy keys are handled just above (warn + skip). Consider skipping the row with a warning (or failing only if the row was actually part of a rewrite) so one bad row cannot wedge the peer DB.

Suggestion:

Suggested change

	let value = <DBM::Value as serialization::DecodeAll>::decode_all(&mut encoded.as_ref())
	.map_err(|err| {
	P2pError::InvalidStorageState(format!(
	"Failed to decode the value of the row keyed by {key}: {err}"
	))
	})?;
	let value = match <DBM::Value as serialization::DecodeAll>::decode_all(&mut encoded.as_ref()) {
	Ok(value) => value,
	Err(err) => {
	logging::log::warn!(
	"Skipping legacy {table} row keyed by {key} with an undecodable value: {err}"
	);
	continue;
	}
	};

[nullPointerEnjoyer]

Skipping here would not get the node running: the regular read path decodes every value through prefix_iter_decoded (storage/src/database/mod.rs:255), which calls Encoded::decode — a corrupt value panicked the node on load before this change. The fold turns that panic into a typed error that names the offending key. Making only the repair pass lenient would just move the failure a few lines down and reintroduce the panic there.

[github-actions]

The manual Deserialize routes through BannableAddress::new, which folds ::ffff:a.b.c.d to the plain IPv4 form. This makes serialization/deserialization non-idempotent with respect to the serialized representation: a value written as "::ffff:1.2.3.4" reads back as 1.2.3.4. That is the intended behavior here (confirmed consistent with the peerdb legacy-key migration in storage_impl.rs), but any other serialization consumer of BannableAddress (RPC payloads, config files, logs) will silently see the canonical form after a round-trip. Ensure all persisted/serialized consumers of BannableAddress are aware of this normalization, and consider documenting it on the type itself, not only on the constructor.

Suggestion:

Suggested change

	// Deserialize routes through `new` so that persisted bans stored in the mapped form are
	// treated as their plain IPv4 form when loaded.
	impl<'de> serde::Deserialize<'de> for BannableAddress {
	fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
	<IpAddr as serde::Deserialize>::deserialize(deserializer).map(BannableAddress::new)
	}
	}
	/// Note: serialization round-trips normalize IPv4-mapped IPv6 spellings
	/// (`::ffff:a.b.c.d`) to the plain IPv4 form, since deserialization goes
	/// through [`BannableAddress::new`].

[github-actions]

The invariant that an IPv4-mapped IPv6 address carries zero flowinfo/scope_id is only checked by debug_assert_eq!. In release builds (and also for crafted peer input on the wire, since a SocketAddrV6 can be decoded with arbitrary flowinfo/scope_id), the metadata is silently dropped when the address is folded to the embedded IPv4 form. This can misidentify or misroute a peer without any signal. Consider rejecting such addresses (returning a Result or canonicalizing with an explicit error) rather than dropping the metadata silently in release builds.

Suggestion:

Suggested change

	Some(v4) => {
	debug_assert_eq!(
	(v6.flowinfo(), v6.scope_id()),
	(0, 0),
	"mapped IPv4 form is assumed to carry no flowinfo or scope id",
	);
	SocketAddr::new(IpAddr::V4(v4), v6.port())
	}
	Some(v4) => {
	if (v6.flowinfo(), v6.scope_id()) != (0, 0) {
	// Handle explicitly in release builds, e.g. return an error or
	// keep the V6 form, instead of silently dropping metadata.
	}
	SocketAddr::new(IpAddr::V4(v4), v6.port())
	}

[nullPointerEnjoyer]

Peer input cannot carry nonzero flowinfo/scope id: the wire type PeerAddress holds only the IP and port, and its conversion to SocketAddr (peer_address.rs:120) constructs SocketAddrV6::new(ip, port, 0, 0). So folding never discards meaningful metadata from network input; the debug assertion guards internal misuse. Rejecting in release would need an error path through every classification call site for a case the input format cannot produce.

[nullPointerEnjoyer]

Pushed ab7cc9e addressing the review items: a debug assertion pins the zero flowinfo/scope-id assumption of the mapped reduction, the legacy key scan only matches the bracketed mapped spelling, and the skip warning was reworded. Two items were left as-is on purpose: the address-group comment is accurate (the IPv6 grouping key keeps the first 4 octets, so 6to4 addresses retain per-/16-of-embedded-IPv4 granularity), and the fold keeps failing loudly on a corrupt row value because the regular read path fails the whole load on such a row anyway — details in the inline replies.

[github-actions]

Error handling is inconsistent between corrupt keys and corrupt values: a key that no longer parses is dropped with a warning specifically so it cannot 'fail the startup on every run', but a value that fails to decode aborts the entire migration (and thus startup) with InvalidStorageState, reproducing exactly the same permanent-startup-failure scenario this function was written to avoid. A single corrupt row value in a legacy mapped-key row will wedge every subsequent startup. Consider dropping such rows (with an error-level log) for consistency with the key-drop policy, or at least documenting why value corruption is treated as unrecoverable while key corruption is not.

Suggestion:

Suggested change

	let value =
	<DBM::Value as serialization::DecodeAll>::decode_all(&mut encoded.as_ref())
	.map_err(|err| {
	P2pError::InvalidStorageState(format!(
	"Failed to decode the value of the row keyed by {key}: {err}"
	))
	})?;
	rewrites.push((key, canonical, value));
	let value = match <DBM::Value as serialization::DecodeAll>::decode_all(
	&mut encoded.as_ref(),
	) {
	Ok(value) => value,
	Err(err) => {
	logging::log::error!(
	"Dropping legacy {table} row keyed by {key}: undecodable value: {err}"
	);
	drops.push(key);
	rewrote_any = true;
	continue;
	}
	};
	rewrites.push((key, canonical, value));

[github-actions]

The derived PartialEq/Ord on PeerAddress still treat PeerAddress::Ip6(::ffff:a.b.c.d) and PeerAddress::Ip4(a.b.c.d) as distinct (the variant tag participates in comparison), even though the new canonicalization goal is 'one address, one key'. The doc comment tells callers to use canonical_ip(), but nothing enforces it, so any Eq/Ord/BTreeMap use on PeerAddress directly (dedup, sorting, lookups) can silently double-count the two forms. Consider providing a canonical comparison helper or normalizing inside the peer address conversion path so equality matches the canonical semantics.

[nullPointerEnjoyer]

Addressed the remaining review items (544a5da, 80caa41):

• Unparseable legacy mapped-form rows are now dropped during the startup fold instead of being warned about on every run. A row whose key no longer parses cannot be loaded by the regular read path either, so keeping it would fail the startup load indefinitely; the warning now says the row was dropped. Covered by tests for both the known-addresses and banned-addresses tables.
• The two legacy-key spelling helpers are unified into one generic helper over the key type, with the mapped prefixes pulled into named constants.
• Removed a redundant re-import of BannableAddress in the socket-address tests.

The debug-assertion item remains as previously discussed on the inline thread: the wire-level PeerAddress type carries no flowinfo/scope id, so the documented unconditional drop cannot be reached from network input.