From 665484c17b10f21db393d0dfc92b47a6e9c8befa Mon Sep 17 00:00:00 2001 From: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com> Date: Wed, 17 May 2023 18:32:53 +0200 Subject: [PATCH] refactor: proper use of setupNat (#1740) Notice that I had to adapt to use 'rlpx_connected_peers' instead of 'connected_peers' in 'wakunode1.nim' because due to the update of the 'vendor/nim-eth', which adds the dependency-break with 'confutils' but also includes another changes. Aside note: we cannot have 'confutils' dependency in 'nim-eth' because that will prevent the generation of any waku dynamic library. --- apps/chat2/chat2.nim | 8 ++- apps/chat2bridge/chat2bridge.nim | 10 +++- apps/wakubridge/wakubridge.nim | 22 ++++++-- apps/wakunode2/app.nim | 48 +--------------- examples/v1/example.nim | 12 +++- library/libwaku.nim | 49 +--------------- tests/v1/test_waku_connect.nim | 8 +-- tests/whisper/test_shh_connect.nim | 2 +- vendor/nim-eth | 2 +- vendor/nim-secp256k1 | 2 +- waku/common/utils/nat.nim | 91 +++++++++++++++--------------- waku/v1/node/wakunode1.nim | 13 +++-- 12 files changed, 100 insertions(+), 167 deletions(-) diff --git a/apps/chat2/chat2.nim b/apps/chat2/chat2.nim index 61bac6105..1cae36a25 100644 --- a/apps/chat2/chat2.nim +++ b/apps/chat2/chat2.nim @@ -404,11 +404,15 @@ proc processInput(rfd: AsyncFD, rng: ref HmacDrbgContext) {.async.} = if conf.logLevel != LogLevel.NONE: setLogLevel(conf.logLevel) - let - (extIp, extTcpPort, extUdpPort) = setupNat(conf.nat, clientId, + let natRes = setupNat(conf.nat, clientId, Port(uint16(conf.tcpPort) + conf.portsShift), Port(uint16(conf.udpPort) + conf.portsShift)) + if natRes.isErr(): + raise newException(ValueError, "setupNat error " & natRes.error) + + let (extIp, extTcpPort, extUdpPort) = natRes.get() + let node = block: var builder = WakuNodeBuilder.init() builder.withNodeKey(nodeKey) diff --git a/apps/chat2bridge/chat2bridge.nim b/apps/chat2bridge/chat2bridge.nim index 857add79f..9d5e6e1aa 100644 --- a/apps/chat2bridge/chat2bridge.nim +++ b/apps/chat2bridge/chat2bridge.nim @@ -248,11 +248,15 @@ when isMainModule: if conf.logLevel != LogLevel.NONE: setLogLevel(conf.logLevel) + let natRes = setupNat(conf.nat, clientId, + Port(uint16(conf.libp2pTcpPort) + conf.portsShift), + Port(uint16(conf.udpPort) + conf.portsShift)) + if natRes.isErr(): + error "Error in setupNat", error = natRes.error + # Load address configuration let - (nodev2ExtIp, nodev2ExtPort, _) = setupNat(conf.nat, clientId, - Port(uint16(conf.libp2pTcpPort) + conf.portsShift), - Port(uint16(conf.udpPort) + conf.portsShift)) + (nodev2ExtIp, nodev2ExtPort, _) = natRes.get() ## The following heuristic assumes that, in absence of manual ## config, the external port is the same as the bind port. extPort = if nodev2ExtIp.isSome() and nodev2ExtPort.isNone(): diff --git a/apps/wakubridge/wakubridge.nim b/apps/wakubridge/wakubridge.nim index 1b3b71088..bed56bb82 100644 --- a/apps/wakubridge/wakubridge.nim +++ b/apps/wakubridge/wakubridge.nim @@ -349,11 +349,23 @@ when isMainModule: ## actually a supported transport. let udpPort = conf.devp2pTcpPort + let natRes = setupNat(conf.nat, ClientIdV1, + Port(conf.devp2pTcpPort + conf.portsShift), + Port(udpPort + conf.portsShift)) + if natRes.isErr(): + error "failed setupNat", error = natRes.error + quit(QuitFailure) + + let natRes2 = setupNat(conf.nat, clientId, + Port(uint16(conf.libp2pTcpPort) + conf.portsShift), + Port(uint16(udpPort) + conf.portsShift)) + if natRes2.isErr(): + error "failed setupNat", error = natRes2.error + quit(QuitFailure) + # Load address configuration let - (nodev1ExtIp, _, _) = setupNat(conf.nat, ClientIdV1, - Port(conf.devp2pTcpPort + conf.portsShift), - Port(udpPort + conf.portsShift)) + (nodev1ExtIp, _, _) = natRes.get() # TODO: EthereumNode should have a better split of binding address and # external address. Also, can't have different ports as it stands now. nodev1Address = if nodev1ExtIp.isNone(): @@ -364,9 +376,7 @@ when isMainModule: Address(ip: nodev1ExtIp.get(), tcpPort: Port(conf.devp2pTcpPort + conf.portsShift), udpPort: Port(udpPort + conf.portsShift)) - (nodev2ExtIp, nodev2ExtPort, _) = setupNat(conf.nat, clientId, - Port(uint16(conf.libp2pTcpPort) + conf.portsShift), - Port(uint16(udpPort) + conf.portsShift)) + (nodev2ExtIp, nodev2ExtPort, _) = natRes2.get() # Topic interest and bloom var topicInterest: Option[seq[waku_protocol.Topic]] diff --git a/apps/wakunode2/app.nim b/apps/wakunode2/app.nim index 9e5e84b00..08dfffa7d 100644 --- a/apps/wakunode2/app.nim +++ b/apps/wakunode2/app.nim @@ -13,12 +13,12 @@ import libp2p/protocols/pubsub/gossipsub, libp2p/peerid, eth/keys, - eth/net/nat, json_rpc/rpcserver, presto, metrics, metrics/chronos_httpserver import + ../../waku/common/utils/nat, ../../waku/common/sqlite, ../../waku/v2/waku_core, ../../waku/v2/waku_node, @@ -317,51 +317,6 @@ proc setupDyamicBootstrapNodes*(app: var App): AppResult[void] = ## Init waku node instance -proc setupNat(natConf, clientId: string, tcpPort, udpPort: Port): - AppResult[tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]]] {.gcsafe.} = - - let strategy = case natConf.toLowerAscii(): - of "any": NatAny - of "none": NatNone - of "upnp": NatUpnp - of "pmp": NatPmp - else: NatNone - - var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]] - - if strategy != NatNone: - let extIp = getExternalIP(strategy) - if extIP.isSome(): - endpoint.ip = some(ValidIpAddress.init(extIp.get())) - # RedirectPorts in considered a gcsafety violation - # because it obtains the address of a non-gcsafe proc? - var extPorts: Option[(Port, Port)] - try: - extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort, - udpPort = udpPort, - description = clientId)) - except CatchableError: - # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now. - error "unable to determine external ports" - extPorts = none((Port, Port)) - - if extPorts.isSome(): - let (extTcpPort, extUdpPort) = extPorts.get() - endpoint.tcpPort = some(extTcpPort) - endpoint.udpPort = some(extUdpPort) - - else: # NatNone - if not natConf.startsWith("extip:"): - return err("not a valid NAT mechanism: " & $natConf) - - try: - # any required port redirection is assumed to be done by hand - endpoint.ip = some(ValidIpAddress.init(natConf[6..^1])) - except ValueError: - return err("not a valid IP address: " & $natConf[6..^1]) - - return ok(endpoint) - proc initNode(conf: WakuNodeConf, rng: ref HmacDrbgContext, peerStore: Option[WakuPeerStorage], @@ -401,7 +356,6 @@ proc initNode(conf: WakuNodeConf, let (extIp, extTcpPort, _) = natRes.get() - let dns4DomainName = if conf.dns4DomainName != "": some(conf.dns4DomainName) else: none(string) diff --git a/examples/v1/example.nim b/examples/v1/example.nim index ebb83f7e2..14c56cb40 100644 --- a/examples/v1/example.nim +++ b/examples/v1/example.nim @@ -11,10 +11,16 @@ import const clientId = "Waku example v1" proc run(config: WakuNodeConf, rng: ref HmacDrbgContext) = + + let natRes = setupNat(config.nat, clientId, + Port(config.tcpPort + config.portsShift), + Port(config.udpPort + config.portsShift)) + if natRes.isErr(): + fatal "setupNat failed", error = natRes.error + quit(1) + # Set up the address according to NAT information. - let (ipExt, tcpPortExt, udpPortExt) = setupNat(config.nat, clientId, - Port(config.tcpPort + config.portsShift), - Port(config.udpPort + config.portsShift)) + let (ipExt, tcpPortExt, udpPortExt) = natRes.get() # TODO: EthereumNode should have a better split of binding address and # external address. Also, can't have different ports as it stands now. let address = if ipExt.isNone(): diff --git a/library/libwaku.nim b/library/libwaku.nim index 6227225ee..995d7f7a3 100644 --- a/library/libwaku.nim +++ b/library/libwaku.nim @@ -7,8 +7,7 @@ import chronicles, chronos, libp2p/crypto/secp, - stew/shims/net, - eth/net/nat as todo_delete_this_module + stew/shims/net import ../vendor/nim-libp2p/libp2p/crypto/crypto, ../../waku/common/utils/nat, @@ -81,52 +80,6 @@ proc errResp(message: string): string = # } return $(%* { "error": message }) -proc setupNat(natConf, clientId: string, tcpPort, udpPort: Port): - Result[tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]], string] {.gcsafe.} = - # TODO reorganize all setupNat calls and use only one commom proc - - let strategy = case natConf.toLowerAscii(): - of "any": NatAny - of "none": NatNone - of "upnp": NatUpnp - of "pmp": NatPmp - else: NatNone - - var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]] - - if strategy != NatNone: - let extIp = getExternalIP(strategy) - if extIP.isSome(): - endpoint.ip = some(ValidIpAddress.init(extIp.get())) - # RedirectPorts in considered a gcsafety violation - # because it obtains the address of a non-gcsafe proc? - var extPorts: Option[(Port, Port)] - try: - extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort, - udpPort = udpPort, - description = clientId)) - except CatchableError: - # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now. - error "unable to determine external ports" - extPorts = none((Port, Port)) - - if extPorts.isSome(): - let (extTcpPort, extUdpPort) = extPorts.get() - endpoint.tcpPort = some(extTcpPort) - endpoint.udpPort = some(extUdpPort) - - else: # NatNone - if not natConf.startsWith("extip:"): - return err("not a valid NAT mechanism: " & $natConf) - - try: - # any required port redirection is assumed to be done by hand - endpoint.ip = some(ValidIpAddress.init(natConf[6..^1])) - except ValueError: - return err("not a valid IP address: " & $natConf[6..^1]) - - return ok(endpoint) - proc parseConfig(config: ConfigNode, privateKey: var PrivateKey, netConfig: var NetConfig, diff --git a/tests/v1/test_waku_connect.nim b/tests/v1/test_waku_connect.nim index 7631b9b29..eba35c80d 100644 --- a/tests/v1/test_waku_connect.nim +++ b/tests/v1/test_waku_connect.nim @@ -58,9 +58,9 @@ procSuite "Waku connections": p2 = await n2.rlpxConnect(newNode(n3.toENode())) p3 = await n4.rlpxConnect(newNode(n3.toENode())) check: - p1.isNil - p2.isNil == false - p3.isNil == false + p1.isErr() == true + p2.isErr() == false + p3.isErr() == false asyncTest "Filters with encryption and signing": var node1 = setupTestNode(rng, Waku) @@ -386,7 +386,7 @@ procSuite "Waku connections": ln2.startListening() let peer = await ln1.rlpxConnect(newNode(ln2.toENode())) - check peer.isNil == true + check peer.isErr() == true asyncTest "Waku set-topic-interest": var diff --git a/tests/whisper/test_shh_connect.nim b/tests/whisper/test_shh_connect.nim index 7ce035d07..1f5161d40 100644 --- a/tests/whisper/test_shh_connect.nim +++ b/tests/whisper/test_shh_connect.nim @@ -326,4 +326,4 @@ procSuite "Whisper connections": ln2.startListening() let peer = await ln1.rlpxConnect(newNode(ln2.toENode())) - check peer.isNil == true + check peer.isErr() == true diff --git a/vendor/nim-eth b/vendor/nim-eth index 72c985892..285da12bf 160000 --- a/vendor/nim-eth +++ b/vendor/nim-eth @@ -1 +1 @@ -Subproject commit 72c98589278aec949c13435d9bcacdb306faa5a8 +Subproject commit 285da12bf318a2e21182dc2453a4a30c58f73067 diff --git a/vendor/nim-secp256k1 b/vendor/nim-secp256k1 index 4c41c5029..5fd813578 160000 --- a/vendor/nim-secp256k1 +++ b/vendor/nim-secp256k1 @@ -1 +1 @@ -Subproject commit 4c41c5029ffc73b732233f06018cd52f3ed47dce +Subproject commit 5fd81357839d57ef38fb17647bd5e31dfa9f55b8 diff --git a/waku/common/utils/nat.nim b/waku/common/utils/nat.nim index dcf125cf9..de36ffcd8 100644 --- a/waku/common/utils/nat.nim +++ b/waku/common/utils/nat.nim @@ -1,70 +1,67 @@ + when (NimMajor, NimMinor) < (1, 4): {.push raises: [Defect].} else: {.push raises: [].} import - std/[strutils, options], - chronicles, stew/shims/net as stewNet, - eth/net/nat + std/[options, strutils] +import + chronicles, + eth/net/nat, + stew/results, + stew/shims/net, + nativesockets logScope: topics = "nat" -proc setupNat*(natConf, clientId: string, tcpPort, udpPort: Port): - tuple[ip: Option[ValidIpAddress], - tcpPort: Option[Port], - udpPort: Option[Port]] {.gcsafe, deprecated: - "Unsafe: this proc quits the app if something is not ok".} = +proc setupNat*(natConf, clientId: string, + tcpPort, udpPort: Port): + Result[tuple[ip: Option[ValidIpAddress], + tcpPort: Option[Port], + udpPort: Option[Port]], string] + {.gcsafe.} = - var - endpoint: tuple[ip: Option[ValidIpAddress], - tcpPort: Option[Port], - udpPort: Option[Port]] - nat: NatStrategy + let strategy = case natConf.toLowerAscii(): + of "any": NatAny + of "none": NatNone + of "upnp": NatUpnp + of "pmp": NatPmp + else: NatNone - case natConf.toLowerAscii: - of "any": - nat = NatAny - of "none": - nat = NatNone - of "upnp": - nat = NatUpnp - of "pmp": - nat = NatPmp - else: - if natConf.startsWith("extip:"): - try: - # any required port redirection is assumed to be done by hand - endpoint.ip = some(ValidIpAddress.init(natConf[6..^1])) - nat = NatNone - except ValueError: - error "not a valid IP address", address = natConf[6..^1] - quit QuitFailure - else: - error "not a valid NAT mechanism", value = natConf - quit QuitFailure + var endpoint: tuple[ip: Option[ValidIpAddress], tcpPort: Option[Port], udpPort: Option[Port]] - if nat != NatNone: - let extIp = getExternalIP(nat) - if extIP.isSome: - endpoint.ip = some(ValidIpAddress.init extIp.get) - # TODO redirectPorts in considered a gcsafety violation + if strategy != NatNone: + let extIp = getExternalIP(strategy) + if extIP.isSome(): + endpoint.ip = some(ValidIpAddress.init(extIp.get())) + # RedirectPorts in considered a gcsafety violation # because it obtains the address of a non-gcsafe proc? var extPorts: Option[(Port, Port)] try: - extPorts = ({.gcsafe.}: - redirectPorts(tcpPort = tcpPort, - udpPort = udpPort, - description = clientId)) - except Exception: - # @TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now. + extPorts = ({.gcsafe.}: redirectPorts(tcpPort = tcpPort, + udpPort = udpPort, + description = clientId)) + except CatchableError: + # TODO: nat.nim Error: can raise an unlisted exception: Exception. Isolate here for now. error "unable to determine external ports" extPorts = none((Port, Port)) - if extPorts.isSome: + if extPorts.isSome(): let (extTcpPort, extUdpPort) = extPorts.get() endpoint.tcpPort = some(extTcpPort) endpoint.udpPort = some(extUdpPort) - return endpoint + else: # NatNone + if not natConf.startsWith("extip:"): + return err("not a valid NAT mechanism: " & $natConf) + + try: + # any required port redirection is assumed to be done by hand + endpoint.ip = some(ValidIpAddress.init(natConf[6..^1])) + except ValueError: + return err("not a valid IP address: " & $natConf[6..^1]) + + return ok(endpoint) + diff --git a/waku/v1/node/wakunode1.nim b/waku/v1/node/wakunode1.nim index da7b42efd..b9cffd86c 100644 --- a/waku/v1/node/wakunode1.nim +++ b/waku/v1/node/wakunode1.nim @@ -21,10 +21,15 @@ proc run(config: WakuNodeConf, rng: ref HmacDrbgContext) ## `udpPort` is only supplied to satisfy underlying APIs but is not ## actually a supported transport. let udpPort = config.tcpPort + let natRes = setupNat(config.nat, clientId, + Port(config.tcpPort + config.portsShift), + Port(udpPort + config.portsShift)) + if natRes.isErr(): + fatal "setupNat failed", error = natRes.error + quit(1) + let - (ipExt, tcpPortExt, _) = setupNat(config.nat, clientId, - Port(config.tcpPort + config.portsShift), - Port(udpPort + config.portsShift)) + (ipExt, tcpPortExt, _) = natRes.get() # TODO: EthereumNode should have a better split of binding address and # external address. Also, can't have different ports as it stands now. address = if ipExt.isNone(): @@ -119,7 +124,7 @@ proc run(config: WakuNodeConf, rng: ref HmacDrbgContext) logMetrics = proc(udata: pointer) = {.gcsafe.}: let - connectedPeers = connected_peers + connectedPeers = rlpx_connected_peers validEnvelopes = waku_protocol.envelopes_valid droppedEnvelopes = waku_protocol.envelopes_dropped