fix: circuit relay v2 follow up items #1619
Conversation
Code sanitation: - splits code and config into transport and server - updates docs - consolidates relay and circuit tests - updates relay discovery to use topology - increases test coverage Bug fixes: - if multiple relays are discovered simultaneously, try to reserve a slot in them in series, otherwise we can end up listening on multiple relays by accident - relay server tags the reserving peer instead of tagging itself - incoming remote address was missing `p2p-circuit` tuple so we were allowing relay to relay forwarding - relay client tags outgoing relay connections fixes: #1608 refs: #1610
achingbrain
force-pushed
the
fix/refactor-circuit-relay-v2
branch
from
68a9456
to
5d36dec
Compare
|
|
||
| ## Circuit Relay v2 | ||
|
|
||
| [email protected] ships with support for [Circuit Relay v2](https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md). This update to the spec changes the relay from being an open relay by default, and as such quite dangerous to enable, to being a limited relay with a slot reservation mechanism that is much safer. |
There was a problem hiding this comment.
Does the JS impl use ASNs to limit the peers?
There was a problem hiding this comment.
Not out of the box, but the connection gater interface can be used to do this.
| * example, the DHT needs some peers to be able to publish) this | ||
| * value should be high enough that they will have warmed up | ||
| */ | ||
| bootDelay?: number |
There was a problem hiding this comment.
seems like a hack, no? Should the content routers know to hold off on publishing until they are warm enough? This component would have less context on that than the content router itself.
There was a problem hiding this comment.
Agreed. The bootDelay option is carried over from the previous implementation, but it would be an improvement for, say, the dht to buffer up network queries until it's first self-query has run.
| // result.expire is non-null if `ReservationStore.reserve` returns with status == OK | ||
| if (result.expire != null) { | ||
| const ttl = new Date().getTime() - result.expire | ||
| await this.peerStore.tagPeer(connection.remotePeer, RELAY_SOURCE_TAG, { value: 1, ttl }) |
There was a problem hiding this comment.
what does value: 1 mean?
There was a problem hiding this comment.
The connection manager has a max connections setting - when a node breaches this setting the connection manager chooses connections to prune.
It does this by sorting the connections by peer value and peer values are derived from their tags.
Random incoming dials will have a value of 0 so will be pruned first - setting the tag value to 1 here makes the peer ever so slightly more valuable than a random incoming dial but not as valuable as a peer that is kad-close, a bootstrap node during startup or a in a gossipsub topic mesh, for example.
src/circuit/server/index.ts
Outdated
| return | ||
| } | ||
|
|
||
| if (isRelayAddr(connection.remoteAddr)) { |
There was a problem hiding this comment.
why twice? We already did this check above
src/circuit/server/index.ts
Outdated
| } | ||
|
|
||
| const destinationConnection = connections[0] | ||
| log('hop connect request from %s to %s is valid', connection.remotePeer, dstPeer) |
There was a problem hiding this comment.
Is there a debug log level so that we don't log this all the time under normal operation?
There was a problem hiding this comment.
Yes, there's log.trace. Can probably remove this line though.
There was a problem hiding this comment.
FWIW under normal operation these log lines are all no-ops, stuff is only logged if the DEBUG env var is set.
src/circuit/server/index.ts
Outdated
| type: StopMessage.Type.CONNECT, | ||
| peer: { | ||
| id: connection.remotePeer.toBytes(), | ||
| addrs: [multiaddr(`/p2p/${connection.remotePeer.toString()}`).bytes] |
There was a problem hiding this comment.
Do we need the addrs field? It doesn't say so in the spec: https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md#stop-protocol
There was a problem hiding this comment.
I don't think so, no. There's no information there that isn't elsewhere in the message anyway.
|
I'm going to merge this to unblock the other PRs. We should hold off release until the multiaddr/webrtc naming thing is resolved. Adding metrics is a good idea, this can be done as a follow up. |
Code sanitation:
Bug fixes:
p2p-circuittuple so we were allowing relay to relay forwardingfixes: #1608
refs: #1610