Skip to content

Allow binding to port 0 for OS-assigned ports#860

Open
joostjager wants to merge 1 commit intolightningdevkit:mainfrom
joostjager:bind-port-zero
Open

Allow binding to port 0 for OS-assigned ports#860
joostjager wants to merge 1 commit intolightningdevkit:mainfrom
joostjager:bind-port-zero

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Mar 31, 2026

Test port collision fix.

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 31, 2026

👋 I see @tnull was un-assigned.
If you'd like another reviewer assignment, please click here.

@joostjager joostjager requested a review from tnull March 31, 2026 09:32
@joostjager joostjager marked this pull request as ready for review March 31, 2026 09:32
Copy link
Copy Markdown
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

While I know about the flakiness issue, I can't say I'm loving all this additional complexity for very little actual (read: production) gain. Do we really need this?

src/lib.rs Outdated
payment_store: Arc<PaymentStore>,
lnurl_auth: Arc<LnurlAuth>,
/// Addresses the node is actively listening on. Set on `start()`, cleared on `stop()`.
active_listening_addresses: Arc<RwLock<Option<Vec<SocketAddress>>>>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we add all of that extra logic, etc, could we move these fields and the logic to ConnectionManager or at least connection.rs? Seems it would be the appropriate place?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to ConnectionManager, but it doesn't seem completely logical.

@joostjager
Copy link
Copy Markdown
Contributor Author

While I know about the flakiness issue, I can't say I'm loving all this additional complexity for very little actual (read: production) gain. Do we really need this?

We've discussed various options offline. Another one is to do a loop around node creation in tests only.

@joostjager joostjager force-pushed the bind-port-zero branch 2 times, most recently from ba72299 to 98323b4 Compare March 31, 2026 12:27
@joostjager joostjager requested review from tnull and removed request for tnull April 1, 2026 06:51
Add support for configuring listening addresses with port 0, letting
the OS pick a free port. After binding, the actual port is resolved
via local_addr() and stored in a new last_bound_addresses field on
ConnectionManager, preserved across restarts so the node rebinds the
same ports.

Node::listening_addresses() returns the last bound addresses when
available, falling back to configured addresses. The gossip broadcast
task and announcement_addresses() never expose port-0 or OS-assigned
addresses, since those are ephemeral and change on restart.

This eliminates the need for the deterministic port picker in tests,
which was fragile due to potential port collisions. Tests now use
127.0.0.1:0 and query the actual port after start().

The announcement propagation test is updated to use explicit
announcement addresses for node B, since listening addresses with
port 0 are (correctly) not announced.

AI tools were used in preparing this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants