-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Circuit relay #3112
chore: Circuit relay #3112
Conversation
You can find the image built from this PR at
Built from f4b8a6f |
a00afcb
to
d869cd3
Compare
f7a72cc
to
b0b48bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wooow, amazing PR! 😍 😍
Thanks so much!
Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>
Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its a great work! Left some little suggestions though.
Thank you!
tests/test_peer_manager.nim
Outdated
@@ -270,12 +270,12 @@ procSuite "Peer Manager": | |||
storage = WakuPeerStorage.new(database)[] | |||
node1 = newTestWakuNode( | |||
generateSecp256k1Key(), | |||
ValidIpAddress.init("127.0.0.1"), | |||
ValidIpAddress.init($getPrimaryIPAddr()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getPrimaryIPAddr can be used here directly... no need to convert to string and than from string.
tests/test_wakunode.nim
Outdated
@@ -169,7 +169,7 @@ suite "WakuNode": | |||
nodeKey = generateSecp256k1Key() | |||
bindIp = parseIpAddress("0.0.0.0") | |||
bindPort = Port(61006) | |||
extIp = some(parseIpAddress("127.0.0.1")) | |||
extIp = some(parseIpAddress($getPrimaryIPAddr())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, with getPrimaryIPAddr
, no need to conver vica-versa, that shall give a valid IP IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much! I've applied your suggestions and much more cleaner now:)
waku/factory/waku.nim
Outdated
var relay = Relay.new() | ||
if confCopy.isRelayClient: | ||
relay = RelayClient.new() | ||
|
||
let nodeRes = setupNode(confCopy, rng, relay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var relay = Relay.new() | |
if confCopy.isRelayClient: | |
relay = RelayClient.new() | |
let nodeRes = setupNode(confCopy, rng, relay) | |
let relay = | |
if confCopy.isRelayClient: RelayClient.new() | |
else: Relay.new() | |
let nodeRes = setupNode(confCopy, rng, relay) |
Maybe more straightforward and eliminates a possible useless initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! I'd like to apply it but I came across a weird compilation issue (related to RelayClient
, LPProtocol
, templates, etc.) and due to that I need to initialize the circuitRelay
component in a separate proc.
I assigned reviewers a bit prematurely in that one ¬¬ Sorry for upcoming spam!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm but I couldnt find how do you indicate to nwaku which nodes to use for circuit relay server
Thanks for checking! In the tests I've used the |
Description
Allow a node to act as a circuit-relay client/server. Also, start the Hole Punch server.
This is useful for nodes that are behind a NAT or firewall and hence are not reachable.
Special kudos to @richard-ramos , @lchenut and @diegomrsantos. This PR is yours and also inspired by this PR: #1766
Changes
Waku
aref object
so that it's always passed as by ref.autonate_service
and check self-reachability every 30 sec instead of every 2 min.relay-client
, which allows setting the node as a circuit-relay client. (we will need to document that.)0.0.0.0
or127.0.0.1
.DiscoveryManager
with rendezvous to advertise rendezvous namespaces based on the subscribed pubsub topics.getExternalIP
.How to test
Three nodes need to run. Two configured as
relay-client
and a third one configured as arelay server
(default.)Wait for two minutes until the node A determines that it is not reachable. When that happens, the node A reserves a circuit-relay slot in the relay service node.
Issue
closes #2514