[Merged by Bors] - Allow libp2p to determine listening addresses #4700

jmcph4 · 2023-09-05T03:12:31Z

Issue Addressed

Proposed Changes

Update local ENR (only port numbers) with local addresses received from libp2p (via SwarmEvent::NewListenAddr)
Only use the zero port for CLI tests

Additional Info

@@ -1036,12 +1038,107 @@ impl<TSpec: EthSpec> NetworkBehaviour for Discovery<TSpec> {
            FromSwarm::DialFailure(DialFailure { peer_id, error, .. }) => {
                self.on_dial_failure(peer_id, error)
            }
+            FromSwarm::NewListenAddr(ev) => {
+                /* TODO(jmcph4): plumb user override status into config somehow */


Can we handle this before merging?

AgeManning · 2023-09-20T06:23:11Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+                        }
+                    };
+
+                    let mut should_update_ip: bool = true;


I think we should never update the ENR IP here.

A few reasons:

The local address we are binding to (i.e the address that libp2p will return) will almost never be a globally routable address. Most computers will be behind a NAT. Unless they are using ipv6 etc. But in any case its better to let discovery handle the IP field of the ENR.

Discovery sets this field based on how others connect to us. We should not interfere with this I dont think.

We should only really set the TCP or QUIC fields of the ENR based on libp2p's response.

AgeManning · 2023-09-20T06:25:08Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+                        Protocol::Tcp(port) => port,
+                        Protocol::Udp(port) => port,
+                        _ => {
+                            crit!(self.log, "Attempt to update discovery with unsupported transport (this should never occur)"; "transport" => ?transport);


I think on every start we now also listen on the QUICv1 protocol.

I would imagine that means running this code would immediately emit this crit.

I usually run an instance of lighthouse locally (with backfill disabled and weak subjectivity sync) to see if it starts up and connects and doesn't emit crits.

It's usually a good self-test to catch bugs like this.

I think on every start we now also listen on the QUICv1 protocol.

As of #4402 this is correct.

I would imagine that means running this code would immediately emit this crit.

When we listen on QUICv1, the multiaddress looks like this: /ip4/127.0.0.1/udp/9001/quic-v1/. In this handler (for FromSwarm::NewListenAddr), we parse the multiaddress from left-to-right and only ever two Protocols deep -- so we'll never encounter this crit! log emission during normal operation of the BN. This is still a bug obviously as only ever parsing two Protocols deep will completely miss QUICv1 altogether.

Oh right. I saw this line:
https://github.com/jmcph4/lighthouse/blob/10263d3e003218c529f420d7e34ad658ff2f1ca1/beacon_node/lighthouse_network/src/discovery/mod.rs#L1131

match addr.iter().nth(1) { Some(Protocol::Tcp(port)) => update_enr(ip, Protocol::Tcp(port)), Some(Protocol::Udp(port)) => update_enr(ip, Protocol::Udp(port)), Some(Protocol::Quic) => update_enr(ip, Protocol::Quic), _ => { debug!(self.log, "Encountered unacceptable multiaddr for listening (invalid transport)"; "addr" => ?addr); } };

which explicitly passes the Quic protocol to that function. I guess this case just never occurs because you only look at the nth(1).

Yeah, it's somewhat regrettable that the current fix (i.e., 599c706) duplicates the port number itself (as the Protocol::QuicV1 variant doesn't store the port unlike its TCP and UDP counterparts) but is much more explicit about the mapping from multiaddress to ENR update information.

AgeManning · 2023-09-20T06:29:47Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+                        match ip {
+                            IpAddr::V4(_) => {
+                                if transport_is_tcp {
+                                    insert_into_enr("tcp4", &port);


We also probably should check that the tcp port isn't already set from a previous run. I.e we load it from disk. I can't recall maybe the insert() function already checks. But we probably dont want to be increasing the sequence number if we don't have to.

We also probably should check that the tcp port isn't already set from a previous run. I.e we load it from disk.

Are you referring to previous instances of us entering this swarm event handler throughout the duration of the BN runtime? Namely, if we receive multiple local addresses to listen on from the swarm (which virtually always happens during normal operation), when we perform the updates serially, we keep thrashing the ENR's sequence number.

I was thinking of the case like, we run Lighthouse on port 9000 as default. It creates an ENR from fresh because its our first ever time running. We set the tcp port on the ENR.

We then shutdown lighthouse and save the ENR to disk.

Then we run lighthouse again. This loads the saved ENR from disk which has the TCP field already set. If we are using the exact same CLI params (i.e all we have done is restart lighthouse), the tcp field in the ENR is set to the one that libp2p would return to us so we probably don't need to reset it.

AgeManning · 2023-09-20T06:30:37Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+                match addr.iter().nth(1) {
+                    Some(Protocol::Tcp(port)) => update_enr(ip, Protocol::Tcp(port)),
+                    Some(Protocol::Udp(port)) => update_enr(ip, Protocol::Udp(port)),
+                    Some(Protocol::Quic) => update_enr(ip, Protocol::Quic),


I think we might be using QuicV1. Should double check this.

In either case a simple run should show this debug error.

I think we might be using QuicV1.

We are; good catch.

In either case a simple run should show this debug error.

This will almost never occur (see above for context)

AgeManning

All the logic looks good to me. Just a few nitpicks.

beacon_node/lighthouse_network/src/discovery/mod.rs

Co-authored-by: Age Manning <[email protected]>

AgeManning

Lets gooo!

paulhauner · 2023-09-29T02:39:22Z

bors r+

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

bors · 2023-09-29T03:16:01Z

Build failed (retrying...):

release-tests-ubuntu

paulhauner · 2023-09-29T03:20:02Z

bors r-

bors · 2023-09-29T03:20:04Z

Canceled.

paulhauner · 2023-09-29T03:20:28Z

bors r+

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

paulhauner · 2023-09-29T05:54:31Z

bors r-

bors · 2023-09-29T05:54:33Z

Canceled.

paulhauner · 2023-09-29T05:58:13Z

bors r+

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

bors · 2023-09-29T06:28:37Z

Build failed (retrying...):

release-tests-windows

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

bors · 2023-09-29T07:09:13Z

Build failed:

debug-tests-ubuntu

paulhauner · 2023-10-03T04:56:51Z

bors r+

## Issue Addressed #4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - #4705 - #4402 - #4745

bors · 2023-10-03T05:54:53Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed sigp#4675 ## Proposed Changes - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`) - Only use the zero port for CLI tests ## Additional Info ### See Also ### - sigp#4705 - sigp#4402 - sigp#4745

jmcph4 changed the base branch from stable to unstable September 5, 2023 03:12

jmcph4 self-assigned this Sep 5, 2023

jmcph4 added enhancement New feature or request test improvement Improve tests Networking labels Sep 5, 2023

jmcph4 closed this Sep 19, 2023

jmcph4 force-pushed the 4675-bind-zero-port branch from 804a9a0 to 0c23c86 Compare September 19, 2023 00:55

Define constants for default address and ports

a0672d2

jmcph4 reopened this Sep 19, 2023

Update ENR based on libp2p listening addresses

f459dfd

jmcph4 marked this pull request as ready for review September 19, 2023 03:55

jmcph4 added work-started work-in-progress PR is a work-in-progress labels Sep 19, 2023

jmcph4 added 3 commits September 19, 2023 14:00

Only use zero ports in CLI tests

6c88ce5

Actually use zero ports for tests (missed transports)

dda7199

Another attempt!

a6ec46e

Write ENR back to disk

7a4f2bf

michaelsproul mentioned this pull request Sep 20, 2023

[Merged by Bors] - Prevent port re-use in HTTP API tests #4745

Closed

jmcph4 requested review from AgeManning, michaelsproul and jxs September 20, 2023 01:33

Merge branch 'unstable' into 4675-bind-zero-port

10263d3

michaelsproul approved these changes Sep 20, 2023

View reviewed changes

michaelsproul added ready-for-review The code is ready for review v4.5.0 ETA Q4 2023 and removed work-in-progress PR is a work-in-progress work-started labels Sep 20, 2023

AgeManning reviewed Sep 20, 2023

View reviewed changes

Avoid ENR IP address mutation

9395e32

Remove unused_ports import

0b82f5b

AgeManning reviewed Sep 25, 2023

View reviewed changes

jmcph4 and others added 2 commits September 25, 2023 16:20

Apply suggestions from code review

0f17c54

Co-authored-by: Age Manning <[email protected]>

Explicitly match on result of ENR update

4e6f0b2

AgeManning approved these changes Sep 25, 2023

View reviewed changes

AgeManning added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 25, 2023

bors bot changed the title ~~Allow libp2p to determine listening addresses~~ [Merged by Bors] - Allow libp2p to determine listening addresses Oct 3, 2023

bors bot closed this Oct 3, 2023

jmcph4 mentioned this pull request Oct 4, 2023

Enable binding to zero port in networking stack #4675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Allow libp2p to determine listening addresses #4700

[Merged by Bors] - Allow libp2p to determine listening addresses #4700

jmcph4 commented Sep 5, 2023 •

edited

Loading

jmcph4 commented Sep 19, 2023

michaelsproul left a comment

AgeManning left a comment

AgeManning Sep 20, 2023

AgeManning Sep 20, 2023

AgeManning Sep 20, 2023

jmcph4 Sep 20, 2023 •

edited

Loading

AgeManning Sep 20, 2023

jmcph4 Sep 20, 2023

AgeManning Sep 20, 2023

jmcph4 Sep 20, 2023

AgeManning Sep 20, 2023

AgeManning Sep 20, 2023

jmcph4 Sep 20, 2023

AgeManning left a comment

AgeManning left a comment

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Oct 3, 2023

bors bot commented Oct 3, 2023

[Merged by Bors] - Allow libp2p to determine listening addresses #4700

[Merged by Bors] - Allow libp2p to determine listening addresses #4700

Conversation

jmcph4 commented Sep 5, 2023 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

See Also

jmcph4 commented Sep 19, 2023

michaelsproul left a comment

Choose a reason for hiding this comment

AgeManning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcph4 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgeManning left a comment

Choose a reason for hiding this comment

AgeManning left a comment

Choose a reason for hiding this comment

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Sep 29, 2023

bors bot commented Sep 29, 2023

bors bot commented Sep 29, 2023

paulhauner commented Oct 3, 2023

bors bot commented Oct 3, 2023

jmcph4 commented Sep 5, 2023 •

edited

Loading

jmcph4 Sep 20, 2023 •

edited

Loading