feat: implement circuit v2 #1533

ckousik · 2023-01-05T14:53:28Z

Reopen the earlier circuit v2 implementation by @mpetrunic with a few fixes to ensure it is up to date with libp2p.

@mpetrunic looks mostly okay, but could you do a rough pass to make sure I haven't missed anything?

Todo:

Interop testing feat!: add circuitv2 relay tests interop#83
Update relayv2 to proto3: circuitv2: upgrade definition to proto3 specs#509

mpetrunic · 2023-01-05T21:00:06Z

src/circuit/relay.ts

+  active?: boolean
+}
+
+export interface AutoRelayConfig {


Do we still use this? We should either remove or rename it

We use it to automatically initiate a reservation on newly connected peers or if the circuit relay protocol is added to an existing peer.

p-shahi · 2023-01-17T05:20:21Z

Is there anything blocking the PR review?

mpetrunic · 2023-01-17T09:26:11Z

Is there anything blocking the PR review?

I think todo's are:

revisit configs
add go interop tests

But I think this shouldn't block reviews^^

achingbrain

First pass of reviewing

The RelayConfig, HopConfig RelayAdvertiseConfig and AutoRelayConfig interface fields all need jsdoc comments to explain what they do
The doc/CONFIGURATION.md sections detailing relay config needs updating

src/circuit/client.ts

.gitignore

tsconfig.json

src/circuit/relay.ts

src/circuit/v2/stop.ts

src/circuit/client.ts

src/circuit/v1/protocol/index.proto

Circuit Relay v2 is being implemented in libp2p/js-libp2p#1533 so enable the tests

2color · 2023-01-19T18:27:29Z

I'm curious how this PR relates to AutoNAT (#1005).

Isn't AutoNAT generally a prerequisite prior to doing NAT hole punching?

p-shahi · 2023-01-19T19:01:32Z

I'm curious how this PR relates to AutoNAT (#1005).

Isn't AutoNAT generally a prerequisite prior to doing NAT hole punching?

@2color Yes AutoNAT is a requirement. You can see our roadmap issue for what's all needed: #1461
RelayV2 is required for WebRTC browser-to-browser as well which is why we're doing this right now.

src/circuit/client.ts

ckousik · 2023-01-24T17:08:12Z

package.json

@@ -91,7 +92,8 @@
    "test:firefox": "aegir test -t browser -f \"./dist/test/**/*.spec.js\" -- --browser firefox",
    "test:firefox-webworker": "aegir test -t webworker -f \"./dist/test/**/*.spec.js\" -- --browser firefox",
    "test:examples": "cd examples && npm run test:all",
-    "test:interop": "aegir test -t node -f dist/test/interop.js"
+    "test:interop": "aegir test -t node -f dist/test/interop.js",
+    "test:relay": "aegir test -t node -f \"./dist/test/**/relay.{node,spec}.js\""


TODO: Remove this before merging. This is in place to ease testing.

ckousik · 2023-01-24T17:28:39Z

test/relay/relay.node.ts

-    const stream = await connection.newStream('/libp2p/circuit/relay/0.1.0')
-
+    /* eslint-disable-next-line no-console */
+    console.log('>>>>>> connection established')


NOTE: If the connection is established, connection.newStream fails because of a reset.

ckousik · 2023-01-25T07:06:21Z

src/circuit/transport.ts

+    return this.components.connectionManager.getConnections(dstPeer)[0] ?? undefined
+  }
+
+  async _onProtocolV1 (data: IncomingStreamData) {


@mpetrunic Should we disable circuit v1 relaying entirely? There seems to be no reason to register this handler as it will not relay.

Yeah, sure! I was just copying logic that go-libp2p had but I'm pretty sure nowadays, there is no need for that!

ckousik · 2023-01-25T07:08:01Z

src/circuit/index.ts

-  addressSorter?: AddressSorter
+export interface HopConfig {
+  enabled?: boolean
+  active?: boolean


How are active and enabled different? Also, we only use enabled in Relay to decide whether or not to advertise the node as a relay.

src/circuit/index.ts

ckousik · 2023-01-25T07:09:14Z

src/circuit/relay.ts

+  ttl?: number
+}
+
+export interface HopConfig {


This is duplicated.

ckousik · 2023-01-25T07:14:55Z

src/circuit/transport.ts


-    await this.components.registrar.handle(RELAY_CODEC, (data) => {
-      void this._onProtocol(data).catch(err => {
+    await this.components.registrar.handle(RELAY_V2_HOP_CODEC, (data) => {


Should adding this listener be conditional on HopConfig.enabled | HopConfig.active === true ?

Since circuit v2 kinda changed interaction, maybe it makes sense to make up new configs and names that are more dev friendly

ckousik · 2023-01-25T07:15:44Z

src/circuit/v2/reservation-store.ts

+   * @param limit - maximum number of reservations to store
+   * @param reservationClearInterval - interval to check for expired reservations in millisecons
+   */
+  constructor (private readonly limit = 15, private readonly reservationClearInterval = 300 * 1000) {


Move these values to constants.ts.

src/circuit/client.ts

achingbrain · 2023-02-28T19:00:22Z

For data limits, the Go code discards any data read (silently) after the limit is exceeded, while for the duration limit, it fails with a ErrDeadlineExceeded once the duration is completed. In the js case, the current implementation closes the source/sink silently once either limit is exceeded. Is this the required behaviour?

Having talked with @Jorropo and @mxinden this isn't the correct behaviour - I've opened libp2p/specs#526 to document what should happen - if everyone agrees with it then the stream should be reset.

src/circuit/v2/util.ts

achingbrain · 2023-02-28T19:06:46Z

src/circuit/index.ts

-  private readonly autoRelay?: AutoRelay
-  private timeout?: any
-  private started: boolean
+  enabled?: boolean


Under what circumstances would a user want to disable the reservation manager?

Are there tests for when it's disabled to ensure libp2p can run without it?

We would want to disable the reservation manager when we do not want to automatically listen on relays. It replaces the old name AutoRelay. #1533 (comment).

The relay tests disable this by default https://github.com/libp2p/js-libp2p/pull/1533/files#diff-81971332c1841fd8324647bd37da55f446e332af9e07d4f28632d5768302dac9R27

src/circuit/v2/util.ts

achingbrain · 2023-03-01T11:07:01Z

src/circuit/transport.ts

-  async _onProtocol (data: IncomingStreamData) {
-    const { connection, stream } = data
+  async onHop ({ connection, stream }: IncomingStreamData) {
+    log('received circuit v2 hop protocol stream from %s', connection.remotePeer)
    const controller = new TimeoutController(this._init.hop.timeout)


What's the benefit of having a hop timeout separate from the underlying transport timeout?

Abortable streams are expensive because they race each buffer against the abort controller so if it's not necessary this could be simplified.

This is the time in which the hop protocol must be completed. The hop timeout is much lower than the transport timeout limit, and is only used while 1) a new reservation is being created, or 2) a client is attempting to connect over the relay. Once the operations are complete, the timeout is cleared.

I think I can avoid the abortableDuplex though.

…am from it-pb-stream

ckousik · 2023-03-02T07:14:33Z

src/circuit/v2/util.ts

-const dataLimitSource = (source: Source<Uint8ArrayList>, abort: (err: Error) => void, limit: bigint): Source<Uint8ArrayList> => {
-  if (limit === BigInt(0)) {
-    return source
+const dataLimitSource = (stream: Stream, limit: bigint): Stream => {


Is this safe? While the implementation of it-pb-stream returns the underlying stream, the return type only guarantees returning a Duplex. The stream functions could be erased from the returned value in a future release without changing the API, which would cause this to break.

3.0.1 changes the unwrap return type to be the type of the wrapped stream, so it should be safe.

achingbrain

Nearly there, comments inline.

achingbrain · 2023-03-02T07:11:19Z

test/circuit/v2/hop.spec.ts

+      // source to dest, write 4 bytes
+      const sender = pushable()
+      void pipe(sender, sourceStream)
+      sender.push(uint8arrayFromString('01234'))
+      // source to dest, exceed stream limit
+      sender.push(uint8arrayFromString('extra'))
+      const data = await all(destStream.source)
+      expect(data).to.have.length(1)
+      expect(data[0]).to.have.length(5)
+      expect(srcServerAbort.callCount).to.equal(1)


This sends five bytes, not four. When four bytes are sent the test fails.

The dialler should send 0123 then extra, then the relay should relay five bytes before reset the stream.

Also, the test needs to assert that both streams were reset - from the dialler to the relay and from the relay to the listener.

Fixed. I was resetting the stream if the sender attempted to send more than the limit.

Also, the test needs to assert that both streams were reset - from the dialler to the relay and from the relay to the listener.

The relay enforces the limit, so the abort is called from the relay to the dialler and from the relay to the listener. This is checked.

achingbrain · 2023-03-02T07:20:10Z

src/circuit/v2/util.ts

-const dataLimitSource = (source: Source<Uint8ArrayList>, abort: (err: Error) => void, limit: bigint): Source<Uint8ArrayList> => {
-  if (limit === BigInt(0)) {
-    return source
+const dataLimitSource = (stream: Stream, limit: bigint): Stream => {


3.0.1 changes the unwrap return type to be the type of the wrapped stream, so it should be safe.

test/circuit/v2/hop.spec.ts

src/circuit/v2/hop.ts

src/circuit/v2/util.ts

…move references to v2 in the code

achingbrain · 2023-03-02T17:38:50Z

src/circuit/reservation-store.ts

@@ -38,8 +46,10 @@ export class ReservationStore implements IReservationStore, Startable {
    this.init = {
      maxReservations: options?.maxReservations ?? 15,
      reservationClearInterval: options?.reservationClearInterval ?? 300 * 1000,
-      applyDefaultLimit: options?.applyDefaultLimit === false,


This is fun, it would have turned every js-libp2p node with the relay server enabled into an unlimited relay 😱

achingbrain

I am going to merge this which will publish an RC to unblock other efforts as it is functional though it still needs some work in a follow-up before being released.

Split the code into two parts - the transport and the relay server
- At the moment the transport handles some of the server responsibilities
- The hop/stop implementations accept a load of context as arguments that might be easier to reason about as a class
Configure the transport and relay server separately - at the moment the server is partially configured under the config.relay.hop key and config.relay.advertise - the fact that the docs say "Does not make you a relay" in several places mean it's really not obvious
Maybe even have a separate CircuitRelay transport that you can configure
More tests

ckousik force-pushed the ckousik/feat/circuit-v2 branch from 2fe7457 to 2bd733a Compare January 5, 2023 15:40

p-shahi requested review from achingbrain and mpetrunic January 5, 2023 16:26

p-shahi mentioned this pull request Jan 5, 2023

[WIP] feat: circuitv2 #1475

Closed

1 task

p-shahi linked an issue Jan 5, 2023 that may be closed by this pull request

Implement Circuit Relay v2 in JS #1029

Closed

mpetrunic reviewed Jan 5, 2023

View reviewed changes

achingbrain requested changes Jan 18, 2023

View reviewed changes

achingbrain added a commit to ipfs/interop that referenced this pull request Jan 18, 2023

feat: enable relay v2 tests for js-ipfs

8b2c6e6

Circuit Relay v2 is being implemented in libp2p/js-libp2p#1533 so enable the tests

achingbrain mentioned this pull request Jan 18, 2023

feat: enable relay v2 tests for js-ipfs ipfs/interop#576

Closed

This was referenced Jan 18, 2023

Implement Circuit Relay v2 in JS #1029

Closed

WebRTC: Browser to Browser #1462

Closed

This was referenced Jan 19, 2023

Support Circuit Relay v2 for Hole Punching ipfs/js-ipfs#3945

Closed

Updated version michaelneale/ipfs-play#1

Closed

p-shahi added this to the WebRTC browser-to-brow milestone Jan 19, 2023

ckousik commented Jan 24, 2023

View reviewed changes

src/circuit/client.ts Outdated Show resolved Hide resolved

ckousik force-pushed the ckousik/feat/circuit-v2 branch from 94d4d46 to 99cd723 Compare January 24, 2023 10:18

ckousik commented Jan 24, 2023

View reviewed changes

ckousik requested a review from achingbrain January 24, 2023 17:10

ckousik commented Jan 24, 2023

View reviewed changes

ckousik commented Jan 25, 2023

View reviewed changes

ckousik force-pushed the ckousik/feat/circuit-v2 branch from a2d9a6e to 625753a Compare January 25, 2023 09:17

ckousik commented Jan 25, 2023

View reviewed changes

src/circuit/client.ts Outdated Show resolved Hide resolved

ckousik requested a review from mpetrunic January 30, 2023 17:23

mxinden mentioned this pull request Feb 28, 2023

feat: Browser to Browser libp2p/js-libp2p-webrtc#90

Merged

4 tasks

p-shahi mentioned this pull request Feb 28, 2023

fix: replace err-code with CodeError #1532

Merged

1 task

achingbrain reviewed Feb 28, 2023

View reviewed changes

src/circuit/v2/util.ts Outdated Show resolved Hide resolved

achingbrain reviewed Feb 28, 2023

View reviewed changes

src/circuit/v2/util.ts Outdated Show resolved Hide resolved

achingbrain reviewed Feb 28, 2023

View reviewed changes

ckousik added 2 commits March 1, 2023 14:37

remove pipe from utils

7e2b848

spawn microtasks to handle circuit connections

ed86c65

ckousik commented Mar 1, 2023

View reviewed changes

src/circuit/v2/util.ts Outdated Show resolved Hide resolved

reset stream on exceeding relay limits

ee77185

achingbrain reviewed Mar 1, 2023

View reviewed changes

ckousik added 4 commits March 1, 2023 18:43

replace reset with abort

edad075

abort takes error param

a5dc1ab

fix dep-check

8885cad

Fix abortable

15696d8

ckousik requested a review from achingbrain March 1, 2023 13:38

chore: remove unecessary types as we can now unwrap the original Stre…

e9cbdd9

…am from it-pb-stream

ckousik commented Mar 2, 2023

View reviewed changes

achingbrain requested changes Mar 2, 2023

View reviewed changes

ckousik added 2 commits March 2, 2023 14:09

fix tests

72b695a

add test for tagging peer

5d7afa3

ckousik requested a review from achingbrain March 2, 2023 10:01

achingbrain reviewed Mar 2, 2023

View reviewed changes

src/circuit/v2/util.ts Outdated Show resolved Hide resolved

apply suggestion

38eeb31

ckousik requested a review from achingbrain March 2, 2023 10:56

fix: allow configuring limits, actually apply limits, update docs, re…

70e1b49

…move references to v2 in the code

achingbrain reviewed Mar 2, 2023

View reviewed changes

p-shahi mentioned this pull request Mar 2, 2023

Circuit Relay v2 follow up work items #1608

Closed

achingbrain merged commit d605cbe into libp2p:master Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement circuit v2 #1533

feat: implement circuit v2 #1533

ckousik commented Jan 5, 2023 •

edited by p-shahi

Loading

mpetrunic Jan 5, 2023

ckousik Jan 24, 2023

p-shahi commented Jan 17, 2023

mpetrunic commented Jan 17, 2023

achingbrain left a comment

2color commented Jan 19, 2023

p-shahi commented Jan 19, 2023

ckousik Jan 24, 2023

ckousik Jan 24, 2023

ckousik Jan 25, 2023

mpetrunic Jan 25, 2023

ckousik Jan 25, 2023

ckousik Jan 25, 2023

ckousik Jan 25, 2023

mpetrunic Jan 25, 2023

ckousik Jan 25, 2023

achingbrain commented Feb 28, 2023 •

edited

Loading

achingbrain Feb 28, 2023

ckousik Mar 1, 2023

achingbrain Mar 1, 2023

ckousik Mar 1, 2023

ckousik Mar 1, 2023

ckousik Mar 2, 2023

achingbrain Mar 2, 2023

achingbrain left a comment

achingbrain Mar 2, 2023

ckousik Mar 2, 2023

achingbrain Mar 2, 2023

achingbrain Mar 2, 2023

achingbrain left a comment

feat: implement circuit v2 #1533

feat: implement circuit v2 #1533

Conversation

ckousik commented Jan 5, 2023 • edited by p-shahi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-shahi commented Jan 17, 2023

mpetrunic commented Jan 17, 2023

achingbrain left a comment

Choose a reason for hiding this comment

2color commented Jan 19, 2023

p-shahi commented Jan 19, 2023

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 commented Feb 28, 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

achingbrain 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

achingbrain left a comment

Choose a reason for hiding this comment

ckousik commented Jan 5, 2023 •

edited by p-shahi

Loading

achingbrain commented Feb 28, 2023 •

edited

Loading