test: add tests for non-unilateral DCUtR over TCP #1932

SgtPooki · 2023-08-03T23:43:07Z

fix: get DCUtR test passing for TCP

This change builds on top of #1928 and adds a lot more debugging output that is
probably not as necessary for someone more familiar (aka @achingbrain)

Implements the [DCUtR protocol](https://github.com/libp2p/specs/blob/master/relay/DCUtR.md). Only supports TCP Simultaneous Connect since there is no QUIC support yet

SgtPooki

self review and some comments/questions for @achingbrain

SgtPooki · 2023-08-03T23:45:55Z

packages/libp2p/src/dcutr/dcutr.ts

+      // TODO: Why is this outbound? This is supposed to be A, handling an incoming connection from B
+      // which means on A, data.connection.direction should be 'inbound'


@achingbrain this is confusing. maybe i'm misunderstanding how registrar and topology works. Is this supposed to be handleOutgoingUpgrade ?

When you register a protocol handler with the registrar, it's invoked when a stream for that protocol is opened by the remote peer.

To do it in the other direction you'd call connection.openStream(multicodec) if you already have a connection or libp2p.dialProtocol(peer, multicodec) if you don't.

The actual connection direction isn't taken into account, but in this case the protocol is clear on which end opens the stream.

I'm still confused why we're calling handleIncomingUpgrade when direction === outbound. that direction implies that the current node is B.. I'm missing something and could probably benefit from a live chat

Why is this outbound? This is supposed to be A, handling an incoming connection from B

The spec says:

B opens a stream to A using the /libp2p/dcutr protocol

So this is A, handling the incoming stream opened by B.

The connection was originally opened in the other direction, from A to B.

The Incoming in handleIncomingUpgrade refers to what's happening in the protocol, not the connection direction.

packages/libp2p/src/dcutr/dcutr.ts

SgtPooki · 2023-08-03T23:48:27Z

packages/libp2p/src/dcutr/dcutr.ts

    // Upon observing the new connection, the inbound peer (here B) checks the
    // addresses advertised by A via identify.
    //
    // If that set includes public addresses, then A may be reachable by a direct
    // connection, in which case B attempts a unilateral connection upgrade by
    // initiating a direct connection to A.
-    if (await this.attemptUnilateralConnectionUpgrade(relayedConnection)) {
+    if (await this.attemptUnilateralConnectionUpgrade(connection)) {
+      debugLog('unilateral connection upgrade succeeded')
      return
    }


Moved this into the dcutr proto registration's onConnect instead of in the DCUtR process itself, since the spec also breaks this step out as something to be attempted prior to DCUtR instead of as a part of the DCUtR process.

The onConnectAttempt function is bound to this and set as the onConnect topology callback - it's an async function but onConnect's signature is sync.

This is flagged by the linter.

SgtPooki · 2023-08-03T23:49:20Z

packages/libp2p/src/dcutr/dcutr.ts

+const logA = logger('libp2p:dcutr:A')
+const logB = logger('libp2p:dcutr:B')


I found these to be much more helpful in debugging than prefixing with "A this" "B that" because we know who the one in control of operations is when we logA or logB, no matter what we put in the message.

SgtPooki · 2023-08-03T23:50:04Z

packages/libp2p/src/dcutr/dcutr.ts


 const log = logger('libp2p:dcutr')
+const logA = logger('libp2p:dcutr:A')
+const logB = logger('libp2p:dcutr:B')
+const debugLog = logger('libp2p:dcutr:debug')


why doesn't libp2p/logger let us call .extend() to add debug...? log{A,B}.debug would be much more useful

Usually you use .trace for debug-level logging.

Please can you replace all instances of debugLog with debug.trace and log happy-path messages with .trace, errors with .error and anything notable with debug().

To see the trace-level logging you have to turn it on explicitly with DEBUG=*:trace.

I believe this is done but let me know if I misused the logs

65b6b73

&

63a531e

SgtPooki · 2023-08-03T23:55:06Z

packages/libp2p/src/dcutr/dcutr.ts

        await relayedConnection.close(options)
+        logB('closed relayed connection')
+        // stop the for loop.
+        break;


If the connection here is successful, there is no further need to retry, so we need to break out of the loop.

packages/libp2p/src/dcutr/dcutr.ts

packages/libp2p/test/dcutr/dcutr.node.ts

SgtPooki · 2023-08-04T01:02:59Z

two other things:

I can't seem to run lint locally because node is out of memory. will try a restart
We should add a test that emulates the gateway -> browser node scenario, but I can't think of a way to do that.

Based on ipfs/helia#182 (comment), it seems like we could just enable/disable some transports on each node to get the effect we want, but I don't believe the third item in that comment is accurate

the gateway does not need to be able to dial the browser (e.g. WebRTC)

In the spec, and in this code, both of the following are true (B=browser, A=gateway):

A tells B the multiaddrs it's available at, and B attempts to dial them
B tells A the multiaddrs it's available at, and A attempts to dial them

packages/libp2p/src/dcutr/dcutr.ts

packages/libp2p/test/dcutr/dcutr.node.ts

achingbrain · 2023-08-04T11:57:46Z

I don't believe the third item in that comment is accurate

the gateway does not need to be able to dial the browser (e.g. WebRTC)

In the spec, and in this code, both of the following are true (B=browser, A=gateway):

A tells B the multiaddrs it's available at, and B attempts to dial them

B tells A the multiaddrs it's available at, and A attempts to dial them

The scenario is:

Browser (B) has a relay address
B adds a block to itself, gets a CID, publishes a provider record containing the relay address
Gateway (A) looks up the provider record, dials B over the relay address
Identify happens
B's onConnect handler is notified that A supports DCUtR
B attempts to unilaterally upgrade the incoming connection
B checks the reported addresses of A, notices it listens on a public wss address (cos it's the gateway) and dials the wss address
B closes the relayed connection to A
Identify happens on the non-relayed connection
Bitswap is reported as supported
Bitswap happens and the block is sent from the browser to the gateway
???
Profit

achingbrain and others added 3 commits August 3, 2023 15:37

feat: direct connection through relay protocol (DCUtR)

1cc42b5

Implements the [DCUtR protocol](https://github.com/libp2p/specs/blob/master/relay/DCUtR.md). Only supports TCP Simultaneous Connect since there is no QUIC support yet

chore: fix linting

a474be1

fix: get DCUtR test passing for TCP

9a6abad

SgtPooki changed the base branch from master to feat/dcutr August 3, 2023 23:43

SgtPooki commented Aug 4, 2023

View reviewed changes

SgtPooki requested a review from achingbrain August 4, 2023 00:26

fix: add placeholder for QUIC simultaneousConnect

047cbed

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/src/dcutr/dcutr.ts Outdated Show resolved Hide resolved

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/src/dcutr/dcutr.ts Outdated Show resolved Hide resolved

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/src/dcutr/dcutr.ts Outdated Show resolved Hide resolved

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/src/dcutr/dcutr.ts Outdated Show resolved Hide resolved

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/src/dcutr/dcutr.ts Outdated Show resolved Hide resolved

achingbrain reviewed Aug 4, 2023

View reviewed changes

packages/libp2p/test/dcutr/dcutr.node.ts Outdated Show resolved Hide resolved

achingbrain changed the title ~~fix: non-unilateral tests DCUtR for TCP pass~~ test: add tests for non-unilateral DCUtR over TCP Aug 4, 2023

SgtPooki added 7 commits August 4, 2023 11:13

chore: allow relay to auto-dial

685badf

test: await closedRelay expectation

7b5c1a4

fix: abort pending dials on simulConn success

99f23d6

fix: use mafmt/matchers to split simulConn mAddrs

9bc8680

fix: logging is no-op & .trace instead of debug

65b6b73

chore: use consistent logging patterns

63a531e

chore: fix lint errors

be62a4f

This comment was marked as outdated.

Sign in to view

SgtPooki added a commit to SgtPooki/helia-playground that referenced this pull request Aug 10, 2023

feat: latest-dcutr updates from libp2p/js-libp2p/pull/1932

9370e54

Base automatically changed from feat/dcutr to master August 16, 2023 06:52

SgtPooki added 4 commits August 23, 2023 16:59

Merge branch 'master' into feat/dcutr-update-1

62ec44f

fix(libp2p): lint errors

54a206e

chore: slight code cleanup

d4de6a1

feat(libp2p:dcutr): dcutr upgrade test passes

c8b742a

achingbrain force-pushed the main branch 29 times, most recently from 242fd96 to bca8d6e Compare November 30, 2023 21:12

SgtPooki closed this Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add tests for non-unilateral DCUtR over TCP #1932

test: add tests for non-unilateral DCUtR over TCP #1932

SgtPooki commented Aug 3, 2023

SgtPooki left a comment

SgtPooki Aug 3, 2023

achingbrain Aug 4, 2023

SgtPooki Aug 4, 2023

achingbrain Aug 15, 2023

SgtPooki Aug 3, 2023

achingbrain Aug 4, 2023

SgtPooki Aug 3, 2023

SgtPooki Aug 3, 2023

achingbrain Aug 4, 2023 •

edited

Loading

SgtPooki Aug 4, 2023

SgtPooki Aug 4, 2023

SgtPooki Aug 3, 2023

SgtPooki commented Aug 4, 2023 •

edited

Loading

achingbrain commented Aug 4, 2023

This comment was marked as outdated.

		// TODO: Why is this outbound? This is supposed to be A, handling an incoming connection from B
		// which means on A, data.connection.direction should be 'inbound'

		const logA = logger('libp2p:dcutr:A')
		const logB = logger('libp2p:dcutr:B')

test: add tests for non-unilateral DCUtR over TCP #1932

test: add tests for non-unilateral DCUtR over TCP #1932

Conversation

SgtPooki commented Aug 3, 2023

SgtPooki 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

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

achingbrain Aug 4, 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

SgtPooki commented Aug 4, 2023 • edited Loading

achingbrain commented Aug 4, 2023

This comment was marked as outdated.

achingbrain Aug 4, 2023 •

edited

Loading

SgtPooki commented Aug 4, 2023 •

edited

Loading