feat(p2p): RLPx IPv6 dual-stack TCP binding and fix DiscV5 ENR tcp/tcp6 port fields

[usmansaleem]

PR description

Two related improvements to the DiscV5 IPv6 dual-stack P2P networking stack:

Fix 1 — RLPx IPv6 dual-stack TCP binding

Previously, even when DiscV5 dual-stack discovery was active (two UDP sockets — one IPv4, one IPv6), RLPx only ever bound one TCP socket. IPv6-only peers that discovered us via the IPv6 discovery socket would fail to establish an RLPx connection if our primary TCP socket was IPv4-only.

• RlpxConfiguration gains bindHostIpv6, bindPortIpv6, and isDualStackEnabled()
• NettyConnectionInitializer binds a second ServerBootstrap on the IPv6 address using the same shared Netty event loop groups. IPv6 bind failure degrades gracefully with a warning so IPv4 is never affected.
• ConnectionInitializer.start() returns a ListeningAddresses record containing both bound addresses.
• RlpxAgent stores the ListeningAddresses after startup and exposes getIpv6ListeningPort() for use by the discovery layer.
• RunnerBuilder gates RLPx dual-stack on DiscV5 being enabled (--Xv5-discovery-enabled). IPv6 dual-stack support was introduced alongside DiscV5; Besu does not implement dual-stack for DiscV4, so binding a second TCP socket only makes sense when DiscV5 is active. This guard can be removed once DiscV4 is deprecated.

Fix 2 — Fix DiscV5 ENR tcp/tcp6 port fields

PeerDiscoveryAgentFactoryV5 was eagerly building the local NodeRecord before the RLPx TCP port was known, causing the ENR's tcp/tcp6 fields to always reflect the UDP discovery bind port instead of the actual RLPx listening port.

Defer NodeRecord initialisation and DiscoverySystem construction to PeerDiscoveryAgentV5.start(tcpPort) where the effective ports are known:

• tcp = IPv4 RLPx port from RlpxAgent.start()
• tcp6 = IPv6 RLPx port from RlpxAgent.getIpv6ListeningPort() — omitted when no IPv6 socket was bound
• LocalNodeKeySigner moved from PeerDiscoveryAgentFactoryV5 into PeerDiscoveryAgentV5 (co-located with its usage)
• DefaultP2PNetwork simplified: the conditional listeningPort fallback in peerDiscoveryAgent.start() is removed; listeningPort is now always passed directly since the discovery agent reads its own UDP port independently
• warnIfEphemeralPortsWithDiscV5() removed: the warning was premature; ephemeral UDP port support is a separate concern tracked in Consensys/discovery#198
• newAddressHandler corrected: previously returned Optional.of(nodeRecord) which accepted the external-address-update call but silently stored the unchanged record (a no-op). Now returns Optional.empty() to properly reject external address suggestions from peers.

Note: Ephemeral UDP port resolution (udp/udp6 fields when --p2p-port=0) is intentionally excluded — it depends on Consensys/discovery#198 and will follow in a separate PR.

Fixed Issue(s)

Part of #9686

Checklist

• Checked out our contribution guidelines
• Considered documentation and added the doc-change-required label if updates are required
• Considered the changelog and included an update if required
• spotless: ./gradlew spotlessApply
• unit tests: ./gradlew :ethereum:p2p:test

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The IPv6 shutdown path waits on serverIpv6.channel().closeFuture() but does not initiate a close on the IPv6 channel in this method. If the IPv6 channel is not closed elsewhere, stop() can hang waiting for ipv6Close. Consider explicitly calling serverIpv6.channel().close() (and similarly ensure the IPv4 server channel is explicitly closed) before awaiting the close futures.

[Copilot]

This log line drops the throwable/stack trace by only logging cause().getMessage(). Consider passing the throwable itself as the final logger argument (or otherwise including it) so bind failures are diagnosable from logs.

Suggested change

	"Failed to bind IPv6 RLPx socket on {}:{}, continuing with IPv4 only: {}",
	ipv6Host,
	ipv6Port,
	ipv6Future.cause().getMessage());
	"Failed to bind IPv6 RLPx socket on {}:{}, continuing with IPv4 only",
	ipv6Host,
	ipv6Port,
	ipv6Future.cause());

[Copilot]

Creating a Socket(host, port) can block for a long time if something goes wrong, which can make the test suite hang. Prefer creating an unconnected socket and using connect(SocketAddress, timeoutMillis) so failures are bounded and tests fail fast.

[Copilot]

NettyConnectionInitializer.start() can intentionally degrade to IPv4-only (i.e., ipv6Address() empty) when the IPv6 bind fails, but this test unconditionally calls addrs.ipv6Address().get(). To avoid flaky failures on hosts where IPv6 is available but binding ::1 fails, add an assumption/assertion that ipv6Address() is present before accessing it (or otherwise handle the IPv4-only result).

Suggested change

	final ListeningAddresses addrs = initializer.start().get(10, TimeUnit.SECONDS);
	final ListeningAddresses addrs = initializer.start().get(10, TimeUnit.SECONDS);
	assumeTrue(
	addrs.ipv6Address().isPresent(),
	"IPv6 bind failed; initializer degraded to IPv4-only");

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment should use 'doesn't' instead of 'don't' for grammatical correctness.

Suggested change

	ipv6Close.complete(null); // best-effort: don't block shutdown on IPv6 close failure
	ipv6Close.complete(null); // best-effort: doesn't block shutdown on IPv6 close failure

[Copilot]

The comment explanation spans lines 159-170 but the actual implementation is a simple lambda returning Optional.empty(). Consider moving the detailed explanation to a dedicated method or condensing the inline comment to avoid overwhelming the builder chain with documentation.

Suggested change

	// Called when ≥2 peers independently report the same external address for this
	// node via PONG responses (the discovery library requires multi-peer consensus
	// before firing). The handler decides whether to update the local ENR with the
	// peer-observed address.
	//
	// Currently returns Optional.empty() to ignore all peer feedback. This is safe
	// for IPv4 because NatService (UPnP/NAT-PMP) handles external address discovery.
	// For IPv6, however, NatService does not apply — peer-observed addresses would be
	// the only auto-discovery mechanism. A future improvement could accept
	// peer-suggested IPv6 addresses when --p2p-host-ipv6 is not explicitly configured,
	// since IPv6 has no NAT and the peer-observed address is the real routable address.
	// See: https://github.com/hyperledger/besu/issues/9874
	// Ignore peer-reported external addresses for now (always returns Optional.empty()).
	// For IPv4 this is covered by NatService; future IPv6 auto-discovery may relax this:
	// https://github.com/hyperledger/besu/issues/9874

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The shutdown order is reversed: the event loop groups are shut down before closing the server channels. In Netty, closing channels should happen first, and then event loop groups should be shut down (optionally awaiting their termination). As written, shutdownGracefully() may prevent queued close tasks from executing promptly or reliably, risking hangs or incomplete shutdown. Consider: (1) close IPv4/IPv6 channels first, (2) compose both close futures, then (3) shutdown boss/workers and complete stop when both group terminations resolve.

[usmansaleem]

done

[Copilot]

This assertion is not guaranteed: many OSes allow the same port number to be bound simultaneously on different local IPs (e.g., 127.0.0.1:P and ::1:P), so two ephemeral binds can legitimately return the same port and make the test flaky. To validate 'independence', prefer asserting that each socket binds to its configured port (e.g., configure IPv6 to a specific known-free port while IPv4 remains ephemeral, then assert IPv6 uses the configured port), or remove/relax the inequality requirement.

Suggested change

	// Both sockets are bound with ephemeral ports — they should differ
	assertThat(addrs.ipv4Address().getPort()).isNotEqualTo(addrs.ipv6Address().get().getPort());
	// Both sockets are bound successfully and have valid (non-zero) ports
	assertThat(addrs.ipv4Address().getPort()).isGreaterThan(0);
	assertThat(addrs.ipv6Address().get().getPort()).isGreaterThan(0);

[usmansaleem]

done

[Copilot]

The ipv6Address Javadoc is slightly inaccurate given the implementation: NettyConnectionInitializer returns Optional.empty() when dual-stack is configured but the IPv6 bind fails (graceful degradation). Update the doc to reflect 'present only when dual-stack is configured and the IPv6 bind succeeds' (or similar), so callers don’t assume presence solely from configuration.

Suggested change

	* @param ipv6Address the IPv6 socket address, present only when dual-stack is active
	* @param ipv6Address the IPv6 socket address, present only when dual-stack is configured and
	* the IPv6 bind succeeds

[usmansaleem]

done

[Copilot]

This can advertise IPv6 ENR fields even when discovery itself is not listening on IPv6. HostEndpoint(host, discoveryConfig.getBindPortIpv6(), port) will set udp6 from the configured IPv6 discovery bind port regardless of whether discoveryConfig.isDualStackEnabled() (and thus whether builder.listen(ipv4, ipv6) is used). Consider additionally requiring discoveryConfig.isDualStackEnabled() (or otherwise confirming an IPv6 discovery socket is in use) before including the IPv6 HostEndpoint, so udp6/ip6 aren’t advertised incorrectly.

Suggested change

	// Include IPv6 ENR fields only when both an advertised IPv6 host is configured and an IPv6
	// RLPx socket was successfully bound. Omitting them when the socket is absent avoids
	// advertising an incorrect tcp6 value to remote peers.
	final Optional<HostEndpoint> ipv6Endpoint =
	discoveryConfig
	.getAdvertisedHostIpv6()
	.flatMap(
	host ->
	ipv6TcpPort.map(
	port -> new HostEndpoint(host, discoveryConfig.getBindPortIpv6(), port)));
	// Include IPv6 ENR fields only when:
	// * an advertised IPv6 host is configured,
	// * an IPv6 RLPx socket was successfully bound, and
	// * discovery is running in dual-stack mode (i.e. an IPv6 discovery socket is in use).
	// Omitting them when any of these conditions is not met avoids advertising incorrect ip6/udp6
	// or tcp6 values to remote peers.
	final Optional<HostEndpoint> ipv6Endpoint;
	if (discoveryConfig.isDualStackEnabled()) {
	ipv6Endpoint =
	discoveryConfig
	.getAdvertisedHostIpv6()
	.flatMap(
	host ->
	ipv6TcpPort.map(
	port ->
	new HostEndpoint(host, discoveryConfig.getBindPortIpv6(), port)));
	} else {
	ipv6Endpoint = Optional.empty();
	}

[usmansaleem]

done

[Matilda-Clerke]

LGTM, just address the copilot comments before merging