From 1babe382265329a440b6b69a8b0f8b2c2b9a306f Mon Sep 17 00:00:00 2001 From: Kim De Mey Date: Wed, 29 Sep 2021 18:50:23 +0200 Subject: [PATCH] Allow for tcp/udp ports to always be configured (#402) * Allow for tcp/udp ports to always be configured - Allow for an ENR to be build with tcp and udp ports also when no IP address is provided - In the address set-up always provide best efforttcp and udp ports also when configuration of external ip (and/or ports) fails. --- eth/net/nat.nim | 26 +++++++++++++++++--------- eth/p2p/discoveryv5/enr.nim | 11 ++++++++++- tests/p2p/test_enr.nim | 16 ++++++++-------- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/eth/net/nat.nim b/eth/net/nat.nim index 49ecd83..38fc2dc 100644 --- a/eth/net/nat.nim +++ b/eth/net/nat.nim @@ -240,7 +240,10 @@ proc redirectPorts*(tcpPort, udpPort: Port, description: string): Option[(Port, proc setupNat*(natStrategy: NatStrategy, tcpPort, udpPort: Port, clientId: string): tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]] = - + ## Setup NAT port mapping and get external IP address. + ## If any of this fails, we don't return any IP address but do return the + ## original ports as best effort. + ## TODO: Allow for tcp or udp port mapping to be optional. let extIp = getExternalIP(natStrategy) if extIP.isSome: let ip = ValidIpAddress.init(extIp.get) @@ -250,13 +253,13 @@ proc setupNat*(natStrategy: NatStrategy, tcpPort, udpPort: Port, description = clientId)) if extPorts.isSome: let (extTcpPort, extUdpPort) = extPorts.get() - (some(ip), some(extTcpPort), some(extUdpPort)) + (ip: some(ip), tcpPort: some(extTcpPort), udpPort: some(extUdpPort)) else: - error "UPnP/NAT-PMP available but port forwarding failed" - (none(ValidIpAddress), none(Port), none(Port)) + warn "UPnP/NAT-PMP available but port forwarding failed" + (ip: none(ValidIpAddress), tcpPort: some(tcpPort), udpPort: some(udpPort)) else: warn "UPnP/NAT-PMP not available" - (none(ValidIpAddress), none(Port), none(Port)) + (ip: none(ValidIpAddress), tcpPort: some(tcpPort), udpPort: some(udpPort)) type NatConfig* = object @@ -293,6 +296,11 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress, tcpPort, udpPort: Port, clientId: string): tuple[ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]] {.gcsafe.} = + ## Set-up of the external address via any of the ways as configured in + ## `NatConfig`. In case all fails an error is logged and the bind ports are + ## selected also as external ports, as best effort and in hope that the + ## external IP can be figured out by other means at a later stage. + ## TODO: Allow for tcp or udp bind ports to be optional. if natConfig.hasExtIp: # any required port redirection must be done by hand @@ -307,7 +315,7 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress, # No route was found, log error and continue without IP. error "No routable IP address found, check your network connection", error = ip.error - return (none(ValidIpAddress), none(Port), none(Port)) + return (none(ValidIpAddress), some(tcpPort), some(udpPort)) elif ip.get().isPublic(): return (some(ip.get()), some(tcpPort), some(udpPort)) else: @@ -329,17 +337,17 @@ proc setupAddress*(natConfig: NatConfig, bindIp: ValidIpAddress, # No route was found, log error and continue without IP. error "No routable IP address found, check your network connection", error = ip.error - return (none(ValidIpAddress), none(Port), none(Port)) + return (none(ValidIpAddress), some(tcpPort), some(udpPort)) elif ip.get().isPublic(): return (some(ip.get()), some(tcpPort), some(udpPort)) else: error "No public IP address found. Should not use --nat:none option" - return (none(ValidIpAddress), none(Port), none(Port)) + return (none(ValidIpAddress), some(tcpPort), some(udpPort)) elif bindAddress.isPublic(): # When a specific public interface is provided, use that one. return (some(ValidIpAddress.init(bindIP)), some(tcpPort), some(udpPort)) else: error "Bind IP is not a public IP address. Should not use --nat:none option" - return (none(ValidIpAddress), none(Port), none(Port)) + return (none(ValidIpAddress), some(tcpPort), some(udpPort)) of NatUpnp, NatPmp: return setupNat(natConfig.nat, tcpPort, udpPort, clientId) diff --git a/eth/p2p/discoveryv5/enr.nim b/eth/p2p/discoveryv5/enr.nim index d92de35..5772c40 100644 --- a/eth/p2p/discoveryv5/enr.nim +++ b/eth/p2p/discoveryv5/enr.nim @@ -152,7 +152,9 @@ template toFieldPair*(key: string, value: auto): FieldPair = proc addAddress(fields: var seq[FieldPair], ip: Option[ValidIpAddress], tcpPort, udpPort: Option[Port]) = - # It makes sense to add ports only when there is an IP provided + ## Add address information in new fields. Incomplete address + ## information is allowed (example: Port but not IP) as that information + ## might be already in the ENR or added later. if ip.isSome(): let ipExt = ip.get() @@ -164,6 +166,11 @@ proc addAddress(fields: var seq[FieldPair], ip: Option[ValidIpAddress], fields.add(((if isV6: "tcp6" else: "tcp"), tcpPort.get().uint16.toField)) if udpPort.isSome(): fields.add(((if isV6: "udp6" else: "udp"), udpPort.get().uint16.toField)) + else: + if tcpPort.isSome(): + fields.add(("tcp", tcpPort.get().uint16.toField)) + if udpPort.isSome(): + fields.add(("udp", udpPort.get().uint16.toField)) proc init*(T: type Record, seqNum: uint64, pk: PrivateKey, @@ -177,6 +184,7 @@ proc init*(T: type Record, seqNum: uint64, ## Can fail in case the record exceeds the `maxEnrSize`. var fields = newSeq[FieldPair]() + # TODO: Allow for initializing ENR with both ip4 and ipv6 address. fields.addAddress(ip, tcpPort, udpPort) fields.add extraFields makeEnrAux(seqNum, pk, fields) @@ -302,6 +310,7 @@ proc update*(r: var Record, pk: PrivateKey, ## will not be altered in these cases. var fields = newSeq[FieldPair]() + # TODO: Make updating of both ipv4 and ipv6 address in ENR more convenient. fields.addAddress(ip, tcpPort, udpPort) fields.add extraFields r.update(pk, fields) diff --git a/tests/p2p/test_enr.nim b/tests/p2p/test_enr.nim index 1a20320..a448a34 100644 --- a/tests/p2p/test_enr.nim +++ b/tests/p2p/test_enr.nim @@ -83,7 +83,7 @@ suite "ENR": test "ENR without address": let keypair = KeyPair.random(rng[]) - port = some(Port(9000)) + port = none(Port) enr = Record.init( 100, keypair.seckey, none(ValidIpAddress), port, port)[] typedEnr = get enr.toTypedRecord() @@ -192,8 +192,8 @@ suite "ENR": check updated.isOk() check: r.tryGet("ip", uint).isNone() - r.tryGet("tcp", uint).isNone() - r.tryGet("udp", uint).isNone() + r.tryGet("tcp", uint).isSome() + r.tryGet("udp", uint).isSome() r.seqNum == 1 block: @@ -202,9 +202,9 @@ suite "ENR": check updated.isOk() check: r.tryGet("ip", uint).isNone() - r.tryGet("tcp", uint).isNone() - r.tryGet("udp", uint).isNone() - r.seqNum == 1 + r.tryGet("tcp", uint).isSome() + r.tryGet("udp", uint).isSome() + r.seqNum == 2 block: let updated = r.update(pk, some(ValidIpAddress.init("10.20.30.40")), @@ -223,7 +223,7 @@ suite "ENR": typedEnr.udp.isSome() typedEnr.udp.get() == 9000 - r.seqNum == 2 + r.seqNum == 3 block: let updated = r.update(pk, some(ValidIpAddress.init("10.20.30.40")), @@ -242,4 +242,4 @@ suite "ENR": typedEnr.udp.isSome() typedEnr.udp.get() == 9001 - r.seqNum == 3 + r.seqNum == 4