Disconnect reason

[pinges]

PR description

This PR makes sure that Besu sends the right disconnect reason in some cases where previous the disconnect reason UNKNOWN was sent.

Related to #9731

[Copilot]

Pull request overview

This PR updates the “should connect” decision flow so that connection rejections can return a specific RLPx disconnect reason instead of defaulting to UNKNOWN.

Changes:

• Changed ShouldConnectCallback to return an Optional<DisconnectReason> (empty = allow, present = reject with reason).
• Replaced “subscribe connect request” (multi-subscriber) with a single setShouldConnectCallback on RlpxAgent, wiring it via NetworkRunner.
• Updated P2P + ETH tests and P2PNetwork API to expose Optional<RlpxAgent>.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/wire/ShouldConnectCallback.java	Changes callback contract to return an optional disconnect reason.
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/RlpxAgent.java	Uses callback-provided disconnect reason for incoming rejects; switches from list subscribers to single callback.
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/P2PNetwork.java	Removes connect-request subscription API; exposes Optional<RlpxAgent>.
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java	Implements getRlpxAgent() returning Optional.of(rlpxAgent).
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/NoopP2PNetwork.java	Implements getRlpxAgent() returning Optional.empty().
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/NetworkRunner.java	Wires should-connect callback into the RlpxAgent during build; updates callback type.
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java	Returns concrete disconnect reasons for common rejection cases.
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/rlpx/RlpxAgentTest.java	Updates tests to the new callback contract.
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/P2PPlainNetworkTest.java	Updates tests to set the callback via getRlpxAgent().ifPresent(...).
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/P2PNetworkTest.java	Updates tests to set the callback via getRlpxAgent().ifPresent(...).
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/NetworkRunnerTest.java	Updates mocks/types for Optional<RlpxAgent> and optional disconnect reasons.
ethereum/mock-p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/testing/MockNetwork.java	Implements getRlpxAgent() returning Optional.empty().
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TestNode.java	Updates wiring to use Optional<RlpxAgent> and new callback signature.
app/src/main/java/org/hyperledger/besu/RunnerBuilder.java	Updates ETH peers wiring to use Optional<RlpxAgent>.

Comments suppressed due to low confidence (1)

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/RlpxAgent.java:16

• shouldConnectCallback is written via setShouldConnectCallback and read from connection-handling paths, but it’s not volatile (or otherwise safely published). If it can be set after the agent has started (or from a different thread), other threads may not reliably see the updated reference. Mark the field volatile, use an AtomicReference, or set it once during construction/startup and make it effectively immutable.

---

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

[macfarla]

couple of claude comments that are worth reviewing:

1. Semantic inversion is subtle — The old API was "should connect? → true/false", the new one is "reason NOT to connect? → empty/present". The method name shouldConnect on ShouldConnectCallback no longer reads naturally —
   Optional.empty() means "yes, connect" which is counterintuitive. Consider renaming to something like ShouldRejectCallback / rejectReason(), or at least adding a Javadoc clarification.
2. checkWhetherToConnect null check — In RlpxAgent.java:
   return shouldConnectCallback != null
   ? shouldConnectCallback.shouldConnect(peer, incoming)
   : Optional.empty();
3. If the callback isn't set, it allows all connections. Previously with the subscriber list, an empty list meant anyMatch returned false → connections were rejected. This is a behavioral change. If there are code paths where no
   callback is set, connections that were previously rejected would now be accepted. Verify this is intentional.

[macfarla]

@pinges do you have any data on disconnect reasons from testing this on hoodi or mainnet? Has it reduced the incidence of UNKNOWN disconnects?

[pinges]

@pinges do you have any data on disconnect reasons from testing this on hoodi or mainnet? Has it reduced the incidence of UNKNOWN disconnects?

@macfarla This will reduce the UNKNOWN disconnects by a fair bit, as, once we have reached max peers, we will now disconnect with TOO_MANY_PEERS. But I have not tested that.

[macfarla]

I have tested this a bit, when trying to test other SNAP stuff, haven't seen any adverse effects. I haven't quantified it the improvement