feat: add DiscV5 config and DiscV5 service (MutableDiscoverySystem) + test case for Service #9238

pr9t · 2025-09-29T10:04:46Z

PR description

Gradle: pins tech.pegasys.discovery:discovery:<25.4.0> and excludes org.apache.tuweni to avoid clashes with existing io.consensys.tuweni jars used elsewhere.
AddedDiscoveryV5Config.java (builder-style config used by the service)
Introduces DiscoveryV5Service (thin wrapper around MutableDiscoverySystem).
Adds a smoke test (DiscoveryV5ServiceTest) that starts/stops and asserts ENR presence.

Fixed Issue(s)

fixes #4089

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

spotless: ./gradlew spotlessApply
unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests
hive tests: Engine or other RPCs modified?

Signed-off-by: Preeti <[email protected]>

fab-10 · 2025-09-29T17:40:27Z

ethereum/p2p/build.gradle

+  implementation("tech.pegasys.discovery:discovery:25.4.0") {
+    exclude group: "org.apache.tuweni"
+  }


Versions are managed in the platform module, and should not be re-declared here, and also exclusion of old tuweni should be centralized, probably that could be done in the main gradle.build

Updated: move discovery version to platform; exclude org.apache.tuweni centrally

….tuweni centrally Signed-off-by: Preeti <[email protected]>

fab-10

Code wise, I have just few small suggestions, then about the discv5 I am not confident in that part, so I prefer that also someone else reviews it.

One question, how and when this service is called/invoked?

fab-10 · 2025-10-03T15:26:03Z

...reum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Config.java

+    this.bootEnrs = Collections.unmodifiableList(new ArrayList<>(b.bootEnrs));
+    this.bindInterfaces = Collections.unmodifiableList(new ArrayList<>(b.bindInterfaces));
+    this.advertisedIps = Collections.unmodifiableList(new ArrayList<>(b.advertisedIps));


Suggested change

this.bootEnrs = Collections.unmodifiableList(new ArrayList<>(b.bootEnrs));

this.bindInterfaces = Collections.unmodifiableList(new ArrayList<>(b.bindInterfaces));

this.advertisedIps = Collections.unmodifiableList(new ArrayList<>(b.advertisedIps));

this.bootEnrs = List.copyOf(b.bootEnrs);

this.bindInterfaces = List.copyOf(b.bindInterfaces);

this.advertisedIps = List.copyOf(b.advertisedIps);

fab-10 · 2025-10-03T15:32:16Z

...reum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Config.java

+
+      // Can happen on containers or when no interfaces are up; safe to ignore.
+      // We log at debug to satisfy Error Prone and keep behavior.
+      LOG.debug("Ignoring SocketException while enumerating network interfaces: {}", e.toString());


Suggested change

LOG.debug("Ignoring SocketException while enumerating network interfaces: {}", e.toString());

LOG.debug("Ignoring SocketException while enumerating network interfaces", e);

fab-10 · 2025-10-03T15:36:54Z

...p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5ServiceTest.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright ConsenSys AG.


Suggested change

* Copyright ConsenSys AG.

* Copyright contributors to Hyperledger Besu.

fab-10 · 2025-10-03T15:46:57Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+  private final ScheduledExecutorService scheduler =
+      Executors.newSingleThreadScheduledExecutor(
+          r -> {
+            final Thread t = new Thread(r, "besu-discv5-poller");
+            t.setDaemon(true);
+            return t;
+          });


Instead of creating a new scheduler, could be possible to use EthScheduler::scheduleFutureTaskWithFixedDelay

Yes—switching to EthScheduler::scheduleFutureTaskWithFixedDelay. I’ll add a small adapter to avoid a direct dependency and refactor the wiring; working on it now and will push updates.

Signed-off-by: Preeti <[email protected]>

pr9t · 2025-10-05T15:58:06Z

Thank you for reviewing!

Just to share some context — I’m part of the Ethereum Protocol Fellowship (EPF), and this DiscV5 work is my main project under that program. My mentor (Sally) suggested breaking it into smaller, reviewable PRs that can be merged incrementally instead of one large feature branch.

So, this PR is the first step — it only adds the DiscV5 config and service (a wrapper around MutableDiscoverySystem) along with initial tests. It isn’t wired into Besu’s runtime yet, so the service isn’t being invoked anywhere at this stage.

In the next PR, I’ll handle the wiring part — integrating the service into Besu’s startup flow, gated behind a hidden flag (--Xdiscv5-enabled), and ensuring no regressions for DiscV4.

fab-10

LGTM, @macfarla is there anyone expert in the discovery part that could take a look?

macfarla · 2025-10-16T00:08:21Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+
+    // In some versions stop() returns void; in others it returns a future.
+    try {
+      final Method m = discovery.getClass().getMethod("stop");


think I would prefer to avoid reflection here - how else could you achieve this?

I guess we can just check what the current version of the discV5 library returns ...

macfarla · 2025-10-16T00:08:50Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+  private static boolean tryRegister(
+      final Object target, final String methodName, final Consumer<NodeRecord> handler) {
+    try {
+      final Method m = target.getClass().getMethod(methodName, Consumer.class);


how could you do this without using reflection?

Thanks for the feedback — I think we can avoid reflection by directly calling the current handler methods (setNewPeerHandler / setPeerRemovedHandler) since Besu now uses a fixed discovery version.

macfarla · 2025-10-16T00:10:27Z

hey @pinges could you have a look from p2p POV - any changes you would like to see on this approach?

pinges

There are a few things that are still missing:

I think we should make sure to ping the bootnodes regularly, to increase our chances of being discovered
We should kick off random finds to discover other nodes quicker, which is more important right after start, maybe less once we have found max peers
We need to handle the sequence number of out node record (store it, retrieve it at startup, increment when it changes (fork id), ...)
Before pushing peers up to the p2p level for connection we should probably filter on the fork id

pinges · 2025-10-17T05:28:48Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+
+    // In some versions stop() returns void; in others it returns a future.
+    try {
+      final Method m = discovery.getClass().getMethod("stop");


I guess we can just check what the current version of the discV5 library returns ...

pinges · 2025-10-17T05:33:30Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+
+  /** Stop asynchronously. */
+  public CompletableFuture<Void> stopAsync() {
+    final ScheduledFuture<?> t = pollTask;


nit: variable is not necessary.

pinges · 2025-10-17T05:51:33Z

...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/v5/DiscoveryV5Service.java

+
+  /** Start the DiscV5 system (idempotent). */
+  public synchronized void start() {
+    if (started) return;


probably want to make started atomic

I’ll update started to use AtomicBoolean for safe concurrent access.

pr9t · 2025-10-17T13:58:51Z

There are a few things that are still missing:

I think we should make sure to ping the bootnodes regularly, to increase our chances of being discovered

We should kick off random finds to discover other nodes quicker, which is more important right after start, maybe less once we have found max peers

We need to handle the sequence number of out node record (store it, retrieve it at startup, increment when it changes (fork id), ...)

Before pushing peers up to the p2p level for connection we should probably filter on the fork id

Thanks for the feedback! I’ll work on adding these improvements.

Signed-off-by: Preeti <[email protected]>

feat: add DiscV5 service (MutableDiscoverySystem) + test case

ae05697

Signed-off-by: Preeti <[email protected]>

fab-10 requested changes Sep 29, 2025

View reviewed changes

build(gradle): move discovery version to platform; exclude org.apache…

0e27bb3

….tuweni centrally Signed-off-by: Preeti <[email protected]>

pr9t requested a review from fab-10 October 2, 2025 07:10

fab-10 reviewed Oct 3, 2025

View reviewed changes

updated: LOG.debug(...,e) and switched to List.copyOf(...)

4c70b78

Signed-off-by: Preeti <[email protected]>

pr9t requested a review from fab-10 October 6, 2025 09:11

fab-10 reviewed Oct 7, 2025

View reviewed changes

macfarla reviewed Oct 16, 2025

View reviewed changes

macfarla assigned pinges and pr9t Oct 16, 2025

pinges reviewed Oct 17, 2025

View reviewed changes

refactor: Make started atomic and simplify stopAsync

55fac11

Signed-off-by: Preeti <[email protected]>

	LOG.debug("Ignoring SocketException while enumerating network interfaces: {}", e.toString());
	LOG.debug("Ignoring SocketException while enumerating network interfaces", e);

	* Copyright ConsenSys AG.
	* Copyright contributors to Hyperledger Besu.

Uh oh!

feat: add DiscV5 config and DiscV5 service (MutableDiscoverySystem) + test case for Service #9238

Are you sure you want to change the base?

feat: add DiscV5 config and DiscV5 service (MutableDiscoverySystem) + test case for Service #9238

Conversation

pr9t commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fab-10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pr9t commented Oct 5, 2025

Uh oh!

fab-10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macfarla commented Oct 16, 2025

Uh oh!

pinges left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pr9t commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pr9t commented Sep 29, 2025 •

edited

Loading