feat(discv4): migrate DiscV4 UDP transport from Vert.x to Netty#10716
Open
usmansaleem wants to merge 8 commits into
Open
feat(discv4): migrate DiscV4 UDP transport from Vert.x to Netty#10716usmansaleem wants to merge 8 commits into
usmansaleem wants to merge 8 commits into
Conversation
23a0fd1 to
5b6aa88
Compare
Replaces VertxPeerDiscoveryAgent and VertxTimerUtil with a Netty-backed implementation using NioDatagramChannel (pure Java, no native epoll). - NettyPeerDiscoveryAgent: wires ScheduledExecutorService for timers and a fixed-2-thread pool for crypto work; dispatches inbound packets on the timer thread to preserve single-threaded event-loop ordering - NettyV4Transport: binds an IPv4 NioDatagramChannel, encodes/decodes UDP frames via Netty pipeline, surfaces start/stop/send as CompletableFutures - ScheduledExecutorTimerUtil / ScheduledExecutorAsyncExecutor: drop-in replacements for the Vert.x timer and worker abstractions - stopGate (AtomicBoolean) in PeerDiscoveryAgentV4 prevents sends and swallows errors after stop() is called - IPv6 peers are silently skipped on the IPv4-only transport (TRACE log) - Removes Vert.x dependency from ethereum:p2p; adds netty-transport and netty-handler - Fixes ENR equality bugs in NodeRecordManager (Bytes vs hex comparison) - Adds NettyPeerDiscoveryAgentTest covering stopGate and all handleOutgoingPacketError branches; adds NettyV4TransportTest and ScheduledExecutorTimerUtilTest Closes besu-eth#10624 Signed-off-by: Usman Saleem <usman@usmans.info>
5b6aa88 to
ec79189
Compare
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the DiscV4 UDP discovery transport from a Vert.x-based implementation to a Netty-based transport with a new V4Transport abstraction, aiming to preserve Vert.x event-loop ordering semantics while removing Vert.x from the DiscV4 UDP layer. It also hardens ENR reuse logic in NodeRecordManager and updates DiscV4 PING handling for EIP-8 tolerance around malformed to fields, with broad test updates.
Changes:
- Introduces
V4Transportand a Netty UDP transport (NettyV4Transport), wiring a newNettyPeerDiscoveryAgentvia the factory paths. - Refactors
PeerDiscoveryAgentV4to use the transport abstraction and a single-thread dispatch executor for ordered controller mutations. - Fixes ENR “reuse-if-unchanged” equality checks (compressed pubkey + fork-id wrapper shape) and updates PING packet
tohandling to be optional for EIP-8 compliance.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTestBuilder.java | Removes Vert.x wiring from discovery factory usage in tests. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/transport/NettyV4TransportTest.java | Adds transport-level UDP bind/send/stop tests for Netty transport. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/PeerDiscoveryBootstrappingTest.java | Updates assertions for PingPacketData.getTo() becoming optional. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/PeerDiscoveryAgentV4Test.java | Updates ENR reuse assertions and packet serializer API change (Bytes.size()). |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/NettyPeerDiscoveryAgentTest.java | Adds focused tests for new Netty agent send/error-stop gating behavior. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/ScheduledExecutorTimerUtilTest.java | Adds tests for JDK scheduled-executor-backed timer util. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/PeerDiscoveryControllerTest.java | Updates mocks/matchers for optional PING to. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataRlpWriterTest.java | Updates construction for optional to. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataRlpReaderTest.java | Updates reader construction and assertions for optional to. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataFactoryTest.java | Updates factory expectations for optional to. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/PacketSerializerTest.java | Updates serializer return type from Vert.x Buffer to Bytes. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/PacketDeserializerTest.java | Updates deserializer input type from Vert.x Buffer to Bytes. |
| ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/MockPeerDiscoveryAgent.java | Refactors mock agent to route through V4Transport and new dispatch executor model. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java | Adds prepareHandlers() hook for pre-bind handler registration. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/NodeRecordManager.java | Fixes ENR reuse equality checks (compressed pubkey + wrapped fork-id field). |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/VertxPeerDiscoveryAgent.java | Removes Vert.x-based DiscV4 UDP implementation (deleted). |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/V4Transport.java | Introduces transport abstraction interface for DiscV4 UDP send/receive. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/transport/NettyV4Transport.java | Adds Netty NIO UDP channel transport implementation (IPv4-only). |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/PeerDiscoveryAgentV4.java | Ports agent to V4Transport, adds ordered dispatch executor and raw inbound decode path. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/PeerDiscoveryAgentFactoryV4.java | Switches factory wiring from Vert.x agent to Netty agent + Netty transport. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/NettyPeerDiscoveryAgent.java | Adds Netty-backed DiscV4 agent with scheduled-executor timer + worker pool. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/VertxTimerUtil.java | Removes Vert.x timer util (deleted). |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/ScheduledExecutorTimerUtil.java | Adds scheduled-executor-based timer util implementation. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/ScheduledExecutorAsyncExecutor.java | Adds executor-service-based async worker wrapper for controller packet creation. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataRlpWriter.java | Updates writer to encode optional to (currently assumes present for outbound). |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataRlpReader.java | Updates reader to produce PingPacketData with optional to for malformed tolerance. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketDataFactory.java | Updates factory to wrap to in Optional. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/ping/PingPacketData.java | Makes to optional and updates string representation accordingly. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/PacketSerializer.java | Switches serializer output from Vert.x Buffer to Bytes. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/internal/packet/PacketDeserializer.java | Switches deserializer input from Vert.x Buffer to Bytes. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/discv4/Endpoint.java | Improves standalone endpoint decode list-leave behavior on decode errors. |
| ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/DefaultPeerDiscoveryAgentFactory.java | Removes Vert.x from default factory builder/wiring. |
| ethereum/p2p/build.gradle | Adds Netty handler/transport dependencies for new UDP transport/agent. |
| ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TestNode.java | Removes Vert.x dependency from discovery factory usage in tests. |
| ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcHttpServiceRpcApisTest.java | Removes Vert.x dependency from discovery factory usage in tests. |
| app/src/test/java/org/hyperledger/besu/RunnerBuilderTest.java | Updates test setup for ENR reuse behavior by configuring fork blocks. |
| app/src/main/java/org/hyperledger/besu/RunnerBuilder.java | Removes Vert.x dependency from discovery agent factory wiring. |
- Endpoint.maybeDecodeStandalone: catch any RuntimeException, not just IllegalPortException, so malformed "to" fields don't abort PING processing (EIP-8). decodeStandalone now skips to the end of a malformed endpoint list before leaving it, keeping the RLP cursor in sync with the enclosing list regardless of where decoding failed. - ScheduledExecutorAsyncExecutor: complete the future exceptionally instead of throwing synchronously when submit() is rejected (e.g. during executor shutdown). - ScheduledExecutorTimerUtil/TimerUtil: catch exceptions from periodic handlers so one failure doesn't permanently cancel the scheduled task, and name each periodic timer for clearer error logs. Addresses Copilot review comments on PR besu-eth#10716. Signed-off-by: Usman Saleem <usman@usmans.info>
…tty migration - PingPacketDataRlpReader: restore inbound PING expiry validation (dropped when the reader bypassed PingPacketDataFactory to tolerate malformed "to" fields per EIP-8). - NettyPeerDiscoveryAgent.stop(): run controller.stop() on the single dispatch thread instead of whichever Netty thread completes transport.stop(), closing a shutdown-time race with in-flight timer/dispatch work. - PeerDiscoveryController.createPacket(): route the post-signing continuation through the same dispatch executor used for inbound packet matching, closing a race on PeerInteractionState's filter. - PeerDiscoveryAgentV4/NettyPeerDiscoveryAgent/MockPeerDiscoveryAgent: add a dedicated single-thread decode executor, separate from the signing pool, restoring the inbound packet arrival-order guarantee Vert.x provided via an ordered executor. - NettyV4Transport: wrap UDP bind failures in PeerDiscoveryServiceException with a descriptive message again, and restore the DEBUG/ERROR severity split in the inbound exception handler. - NettyV4Transport/NettyPeerDiscoveryAgent: create executors/event-loop group lazily in start() instead of eagerly in the constructor, so discovery-disabled nodes don't carry idle threads. - MockPeerDiscoveryAgent: exclude stopped agents from the mock transport's address-based routing match, and replace the constructor-chain/mutable-array back-reference hack with a post-construction setter mirroring V4Transport.setInboundHandler. Fixes 9 findings from a full review of PR besu-eth#10716. Signed-off-by: Usman Saleem <usman@usmans.info>
Netty's parameterless shutdownGracefully() defaults to a 2s quiet period (plus a 15s timeout), unconditionally waited out even when the event loop is already idle. Confirmed via a live mainnet run that this accounted for the entire ~2.1s delay observed during node shutdown (NettyV4Transport's event-loop-group teardown, not RLPx/eth-subprotocol as initially suspected). Nothing is legitimately in flight by the time this transport shuts down its dedicated single-thread event loop, so use an explicit 0ms quiet period / 500ms timeout instead. Also add INFO-level lifecycle logging around NettyPeerDiscoveryAgent and NettyV4Transport start/stop, matching the pattern already used by other components (Synchronizer, NetworkRunner, EthProtocolManager) — this is what surfaced the timing data above. Signed-off-by: Usman Saleem <usman@usmans.info>
- NettyPeerDiscoveryAgent: give each crypto-pool thread a unique name (discv4-crypto-0/1) instead of both sharing "discv4-crypto", making thread dumps and logs unambiguous. - NettyV4Transport: only install the TRACE LoggingHandler on the UDP pipeline when TRACE is actually enabled for it, avoiding per-packet overhead when it's off. - PeerDiscoveryController: update a stale comment in createPacket() that still referenced the "vertx event thread"; this path is now Vert.x-free and the real concern is the discovery dispatch/timer thread. Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description
This PR ports the DiscV4 peer discovery UDP layer from Vert.x to Netty,
replacing
VertxPeerDiscoveryAgentandVertxTimerUtilwith a clean,Vert.x-free implementation. It is a focused extraction from #10624 so
that the foundational transport change can be reviewed and merged
independently.
What changes
New files
V4Transport— interface abstracting UDP send/receive from the agenttransport/NettyV4Transport— Netty NIO UDP channel implementation (IPv4)NettyPeerDiscoveryAgent— concrete agent wiringScheduledExecutorServicethreads for timers and crypto
internal/ScheduledExecutorTimerUtil— replacesVertxTimerUtilinternal/ScheduledExecutorAsyncExecutor— replaces the Vert.x worker poolDeleted files
VertxPeerDiscoveryAgentinternal/VertxTimerUtilModified
PeerDiscoveryAgentV4— ported toV4Transportinterface; addeddispatchExecutorordering guarantee (timer callbacks and inbound packetdispatch share a single thread, preserving the Vert.x event-loop ordering);
a dedicated single-thread decode executor was added afterward (see "Fixes
from review")
PeerDiscoveryAgentFactoryV4— wiresNettyPeerDiscoveryAgentDefaultPeerDiscoveryAgentFactory— updated to matchPeerDiscoveryController— adds adispatchExecutorused to run outboundpacket-creation callbacks on the same thread as inbound packet handling
(see "Fixes from review")
ethereum/p2p/build.gradle— addsnetty-transportandnetty-handlerNodeRecordManager— fixes two ENR equality bugs (compressed pubkeycomparison; fork-id wrapper list shape) that caused seq to increment on
every restart even when nothing changed
PingPacketData.getTo()— returnsOptional<Endpoint>for EIP-8compliance (malformed
tofield must not prevent packet processing)Fixes from review
Since the initial migration, this PR picked up several concurrency/
correctness fixes from Copilot's automated review, a full manual code
review, and a live mainnet dry-run used to validate startup/shutdown
behavior:
PeerDiscoveryController .createPacket()'s post-signing continuation now runs on the samesingle dispatch executor used for inbound packet matching, instead of
whichever crypto-pool thread finished signing — this was a real race on
PeerInteractionState's response-matching filter (a fast-arriving PONGcould be matched before the filter update was visible).
PeerDiscoveryAgentV4nowuses a dedicated single-thread decode executor
(
createDecodeExecutor()), separate from the 2-thread signing pool,restoring the packet arrival-order guarantee the removed Vert.x
implementation provided via an ordered
executeBlocking.NettyPeerDiscoveryAgent.stop()now runsPeerDiscoveryController.stop()ontimerScheduleritself rather thanwhatever Netty thread completes
transport.stop(), closing a race within-flight timer/dispatch work during shutdown.
PingPacketDataRlpReaderwasrewritten to bypass
PingPacketDataFactory(to tolerate a malformedtofield per EIP-8), which also silently dropped the factory's expiry check.
Expiry is now validated directly in the reader; malformed-
totoleranceis unaffected.
NettyV4Transporthardening:PeerDiscoveryServiceExceptionwith adescriptive host:port message again, matching the old Vert.x behavior.
is restored (previously collapsed to unconditional DEBUG).
NettyPeerDiscoveryAgent's timer/crypto/decodeexecutors) are now created lazily in
start()instead of eagerly in theconstructor, so a node running with discovery disabled carries no idle
discv4 threads.
default — confirmed via a live mainnet run that this was adding ~2s to
every node shutdown; it now completes in ~10-15ms.
EndpointEIP-8 decoding hardened:maybeDecodeStandalone()'s catchwas broadened from
IllegalPortExceptionto anyRuntimeException, soany malformed
tofield is tolerated, not just an out-of-range port.decodeStandalone()was also fixed to skip to the end of the malformedRLP list before leaving it, keeping the cursor in sync with the enclosing
list regardless of where decoding failed.
ScheduledExecutorAsyncExecutornow completesits future exceptionally instead of throwing synchronously when the
underlying executor rejects a task (e.g. during shutdown);
ScheduledExecutorTimerUtilnow catches exceptions from periodichandlers so one failure doesn't permanently cancel the scheduled task.
MockPeerDiscoveryAgent's mock transport no longerrisks matching a stopped/replaced agent at a reused address, and its
constructor-chain/mutable-array back-reference hack was replaced with a
post-construction setter mirroring the production
setInboundHandlerpattern.
Limitations / known constraints
NettyV4Transportis pinned toSocketProtocolFamily.INET. Peers that advertise an IPv6-only addressare silently skipped (TRACE log). Dual-stack IPv4+IPv6 peers work
normally via their IPv4 address. This is an intentional temporary
restriction; the follow-on feat(p2p): DiscV4 + DiscV5 on shared UDP socket with --discovery-mode selector #10624 introduces a shared dual-stack channel
that lifts this limitation.
DNSDaemon/DNSResolverremainVert.x-based. The
ethereum/p2pmodule retains itsvertx-coredependency for that subsystem.
vertx_eventloop_pending_tasksmetric dropped: That gauge wasVert.x-specific and has no direct equivalent. All other discovery metrics
(
besu_network_discovery_*) come fromPeerDiscoveryControllerand areunaffected.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests