Skip to content

fix: update ENR with actual port when bound to ephemeral port 0#198

Merged
lucassaldanha merged 7 commits intoConsensys:masterfrom
usmansaleem:fix/ephemeral-port-enr-update
Feb 24, 2026
Merged

fix: update ENR with actual port when bound to ephemeral port 0#198
lucassaldanha merged 7 commits intoConsensys:masterfrom
usmansaleem:fix/ephemeral-port-enr-update

Conversation

@usmansaleem
Copy link
Copy Markdown
Contributor

@usmansaleem usmansaleem commented Feb 23, 2026

Summary

When `DiscoverySystem` is started with port `0` (e.g. Besu `--p2p-port=0`), the OS assigns an ephemeral port at bind time. Previously the local NodeRecord (ENR) was never updated with the actual bound port, so the advertised address was unreachable by peers.

Changes

`NettyDiscoveryServerImpl.getListenAddress()`
Returns the configured IP with the actual OS-assigned port after bind, but only when the configured port was `0`. Non-zero port behaviour is completely unchanged.

`LocalNodeRecordStore.onBoundPortResolved(InetSocketAddress)`
New method that updates the UDP or UDP6 port in the ENR after the OS assigns an actual port for an ephemeral bind. Unlike `onSocketAddressChanged`, this method bypasses `NewAddressHandler` because ephemeral port resolution is an internal OS operation, not an externally-reported address change. The advertised IP is preserved from the existing ENR; only the port is updated. No-op if the current ENR port is not `0`.

`DiscoveryManagerImpl.start()`
Calls `localNodeRecordStore.onBoundPortResolved(boundAddress)` in the per-server `thenAccept` callback after each server binds. The `NewAddressHandler` is intentionally bypassed here — callers such as Besu configure their handler to block all external address changes (to prevent peers from altering the advertised address), which would otherwise silently suppress the ephemeral port update.

Motivation

Fixes the ephemeral UDP port issue in Besu where `--p2p-port=0` (or `--p2p-discovery-port=0`) caused the ENR to advertise `udp=0` / `udp6=0`, making the node unreachable by peers.

The initial fix attempt used `onSocketAddressChanged` but that routes through `NewAddressHandler`. Besu's handler returns the old record unchanged (to prevent peers from overriding the advertised address), so the ENR update was silently dropped. `onBoundPortResolved` is the correct path for trusted internal updates.

After this fix, Besu's `warnIfEphemeralPortsWithDiscV5()` warning can be removed since ephemeral UDP ports now work correctly.

Test plan

  • `NettyDiscoveryServerImplTest` — `getListenAddress()` returns port `0` before start, and the actual bound port after start with an ephemeral address
  • `DiscoveryIntegrationTest.ephemeralPortIsReflectedInLocalNodeRecord` — starts a full `DiscoverySystem` with `listen("127.0.0.1", 0)` and asserts `getLocalNodeRecord().getUdpAddress().getPort()` is non-zero after startup
  • `LocalNodeRecordStoreTest.onBoundPortResolvedUpdatesEphemeralUdpPort` — unit test: ephemeral port is resolved and listener is notified
  • `LocalNodeRecordStoreTest.onBoundPortResolvedDoesNotUpdateNonEphemeralPort` — unit test: non-zero port nodes are unaffected
  • Full test suite passes with no regressions (`./gradlew test`)

Note

Medium Risk
Touches discovery startup and ENR mutation logic; incorrect port/IP-family handling or concurrency bugs could make nodes advertise unreachable addresses, though changes are scoped and covered by new unit/integration tests.

Overview
Ensures nodes started with an ephemeral UDP port (port=0) advertise the actual OS-assigned port in their local ENR after binding.

NettyDiscoveryServerImpl.getListenAddress() now reports the bound port once started (while preserving the configured IP), and DiscoveryManagerImpl.start() uses that bound address to both select the channel’s IP family and to call the new LocalNodeRecordStore.onBoundPortResolved().

LocalNodeRecordStore is made thread-safe via AtomicReference and adds onBoundPortResolved() to update only the UDP/UDP6 port (bypassing NewAddressHandler) with a CAS loop to avoid losing concurrent dual-stack updates; new tests cover server listen address behavior, ENR port resolution in an integration run, and the new store logic including dual-stack concurrency.

Written by Cursor Bugbot for commit 8e5a224. This will update automatically on new commits. Configure here.

When DiscoverySystem is started with port 0, the OS assigns an ephemeral
port at bind time. Previously the local NodeRecord (ENR) was never updated
with the actual bound port, so advertised addresses were unreachable.

- NettyDiscoveryServerImpl.getListenAddress() now returns the configured IP
  with the actual OS-assigned port after bind when port was 0; non-zero
  port behaviour is unchanged.
- DiscoveryManagerImpl.start() calls LocalNodeRecordStore.onSocketAddressChanged()
  with the real port after bind, but only when the ENR was initialised with
  port 0. The advertised IP is preserved (not replaced with the bind address).
  For fixed-port nodes the callback is never triggered, so existing behaviour
  is identical.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 23, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@usmansaleem
Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@usmansaleem
Copy link
Copy Markdown
Contributor Author

recheck

github-actions Bot added a commit that referenced this pull request Feb 23, 2026
Comment thread src/main/java/org/ethereum/beacon/discovery/DiscoveryManagerImpl.java Outdated
Replace the onSocketAddressChanged approach in DiscoveryManagerImpl with
onBoundPortResolved, a new LocalNodeRecordStore method that directly
updates the UDP/UDP6 port without going through NewAddressHandler.

The previous approach was broken for callers (e.g. Besu) that configure
a NewAddressHandler which rejects all external address changes. Because
onSocketAddressChanged routes through the handler, the ENR port was
silently left at 0 even after the OS assigned an actual port.

onBoundPortResolved is intentionally exempt from NewAddressHandler
because ephemeral port resolution is an internal OS operation, not an
externally-reported address change.  The advertised IP from the ENR is
preserved; only the port field is updated.

- Add LocalNodeRecordStore.onBoundPortResolved(InetSocketAddress)
- Simplify DiscoveryManagerImpl.thenAccept to call onBoundPortResolved
- Add unit tests: ephemeral port is updated, non-ephemeral is no-op

Signed-off-by: Usman Saleem <usman@usmans.info>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

…ual-stack race

In dual-stack configurations with both IPv4 and IPv6 on port 0, the two
thenAccept callbacks in DiscoveryManagerImpl.start() run concurrently on
separate Netty threads. Both called onBoundPortResolved simultaneously,
read the same latestRecord snapshot, each built a new record updating only
their own IP family's port, and the second write overwrote the first —
leaving one family's ENR port stuck at 0.

Replace volatile NodeRecord with AtomicReference<NodeRecord> and use a
CAS retry loop in onBoundPortResolved. If Thread 2's compareAndSet fails
(Thread 1 already wrote), it retries against the fresh record which already
has Thread 1's port update, so the second build correctly preserves it.

Other methods (onSocketAddressChanged, onCustomFieldValueChanged) are
updated to use AtomicReference.get/set for consistency but do not use CAS
loops as they are not called concurrently in the same way.

Signed-off-by: Usman Saleem <usman@usmans.info>
Add onBoundPortResolvedHandlesConcurrentDualStackUpdates to verify
that simultaneous IPv4 and IPv6 port resolution in dual-stack mode
does not lose either update. Uses CyclicBarrier to force both threads
to race, then asserts both udp and udp6 ports are non-zero in the
final ENR. Demonstrates correctness of the AtomicReference CAS loop.
Signed-off-by: Usman Saleem <usman@usmans.info>
usmansaleem added a commit to usmansaleem/besu that referenced this pull request Feb 23, 2026
When --p2p-port=0 the OS assigns the UDP port only after bind. Previously
updateNodeRecord() was called before the library resolved the port, leaving
udp/udp6=0 in the persisted ENR indefinitely.

Requires Consensys/discovery#198 which adds onBoundPortResolved() to
LocalNodeRecordStore. The localNodeRecordListener in PeerDiscoveryAgentV5
detects port-0 → real-port transitions and calls
NodeRecordManager.onDiscoveryPortResolved() to sync them before writing.

In dual-stack mode both UDP servers bind concurrently; the listener defers
the ENR write until both ports are resolved so the seq counter increments
exactly once. A ReentrantLock guards onDiscoveryPortResolved and
updateNodeRecord against concurrent RocksDB commits.

Signed-off-by: Usman Saleem <usman@usmans.info>
usmansaleem added a commit to usmansaleem/besu that referenced this pull request Feb 23, 2026
Strips the ephemeral port handling so this branch contains only Fix 1
(RLPx dual-stack TCP binding) and Fix 2 (ENR tcp/tcp6 port fields).

Removed:
- NodeRecordManager.onDiscoveryPortResolved() and ReentrantLock
- Full localNodeRecordListener in PeerDiscoveryAgentV5
- hasEphemeralPort helper

Ephemeral port resolution (Fix 3) depends on Consensys/discovery#198
and will be tracked separately.

Signed-off-by: Usman Saleem <usman@usmans.info>
@lucassaldanha lucassaldanha enabled auto-merge (squash) February 24, 2026 21:04
@lucassaldanha lucassaldanha merged commit 20cfa1e into Consensys:master Feb 24, 2026
4 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants