Include relay addresses in the SPR and refactoring

storage/discovery.nim

@@ -180,7 +180,7 @@ proc getSpr*(d: Discovery): SignedPeerRecord =
   ## Returns the node's current Signed Peer Record as registered in the DHT.
   d.protocol.getRecord()
 
-proc updateRecordsAndSpr*(
+proc announceDirectAddrs*(
     d: Discovery, announceAddrs: openArray[MultiAddress], udpPort: Port
 ) =
   # UDP addresses are derived from TCP announce addresses by remapping protocol and port.
@@ -204,15 +204,18 @@ proc updateRecordsAndSpr*(
       .expect("Should construct signed record").some
     d.protocol.updateRecord(spr).expect("Should update SPR")
 
-proc updateAnnounceRecord*(d: Discovery, addrs: openArray[MultiAddress]) =
-  # Updates announce addresses only, not the DHT routing record.
-  # Relay addresses should not pollute DHT routing.
+proc announceRelayAddrs*(d: Discovery, addrs: openArray[MultiAddress]) =
+  ## Updates only announce addresses
+  ## When using relay, the DHT routing record is not updated to not pollute the DHT.
   d.announceAddrs = @addrs
   info "Updating announce record", addrs = d.announceAddrs
   d.providerRecord = SignedPeerRecord
     .init(d.key, PeerRecord.init(d.peerId, d.announceAddrs))
     .expect("Should construct signed record").some
 
+  if not d.protocol.isNil:
+    d.protocol.updateRecord(d.providerRecord).expect("Should update SPR")
+
 proc start*(d: Discovery) {.async: (raises: []).} =
   try:
     d.protocol.open()
@@ -264,7 +267,7 @@ proc new*(
 
   # Called even when announceAddrs is empty: newProtocol below requires
   # providerRecord to be set, and it will be updated with real addresses in start().
-  self.updateRecordsAndSpr(announceAddrs, udpPort = discoveryPort)
+  self.announceDirectAddrs(announceAddrs, udpPort = discoveryPort)
 
   let discoveryConfig =
     DiscoveryConfig(tableIpLimits: tableIpLimits, bitsPerHop: DefaultBitsPerHop)

storage/nat.nim

@@ -137,7 +137,7 @@ proc announcePeerInfoAddrs*(discovery: Discovery, peerInfo: PeerInfo, udpPort: P
   let addrs = peerInfo.addrs.filterIt(not it.isCircuitRelayMA())
   if addrs.len == 0 or addrs == discovery.announceAddrs:
     return
-  discovery.updateRecordsAndSpr(addrs, udpPort = udpPort)
+  discovery.announceDirectAddrs(addrs, udpPort = udpPort)
 
 proc setupPeerInfoObserver*(
     switch: Switch,
@@ -193,7 +193,7 @@ method handleNatStatus*(
       if m.tcpMappingId.isSome and m.udpMappingId.isSome:
         m.close()
 
-      discovery.updateRecordsAndSpr(@[], udpPort = discoveryPort)
+      discovery.announceDirectAddrs(@[], udpPort = discoveryPort)
     elif m.tcpMappingId.isSome and m.udpMappingId.isSome:
       warn "Not Reachable with active port mapping. The port mapping will be deleted and relay will start."
 
@@ -203,7 +203,7 @@ method handleNatStatus*(
 
       # We remove the announced records.
       # Eventually, it will we updated by the relay when it started
-      discovery.updateRecordsAndSpr(@[], udpPort = discoveryPort)
+      discovery.announceDirectAddrs(@[], udpPort = discoveryPort)
     elif autoRelayService.isRunning:
       # The mapping was already tried and did not make the node reachable.
       # If the relay is running, there is nothing to do.
@@ -233,7 +233,7 @@ method handleNatStatus*(
         # The client mode will be updated on the next iteration of autonat.
         # Trying to check manually that the node is reachable is not trivial,
         # this is exactly what Autonat is for.
-        discovery.updateRecordsAndSpr(@[announceAddress], udpPort = udpPort)
+        discovery.announceDirectAddrs(@[announceAddress], udpPort = udpPort)
         hasPortMapping = true
       else:
         # In case of failure, close the port mapping in order to rerun discover

storage/storage.nim

@@ -113,7 +113,7 @@ proc start*(s: StorageServer) {.async.} =
         ip = some(s.config.nat.extIp), port = none(Port)
       )
     ]
-    s.storageNode.discovery.updateRecordsAndSpr(
+    s.storageNode.discovery.announceDirectAddrs(
       announceAddresses, udpPort = s.config.discoveryPort
     )
   else:
@@ -468,7 +468,7 @@ proc new*(
       onReservation = proc(addresses: seq[MultiAddress]) {.gcsafe, raises: [].} =
         info "Relay reservation updated", addresses
         # relay addresses are for download traffic only, not DHT routing
-        discovery.updateAnnounceRecord(addresses),
+        discovery.announceRelayAddrs(addresses),
       rng = random.Rng.instance(),
     )
 

tests/storage/helpers/nodeutils.nim

@@ -224,7 +224,7 @@ proc generateNodes*(
 
         if config.enableBootstrap:
           waitFor switch.peerInfo.update()
-          blockDiscovery.updateRecordsAndSpr(
+          blockDiscovery.announceDirectAddrs(
             switch.peerInfo.addrs, udpPort = bindPort.Port
           )
           bootstrapNodes.add blockDiscovery.getSpr()

tests/storage/testdiscovery.nim

@@ -23,18 +23,18 @@ suite "Discovery - SPR record logic":
     key = PrivateKey.random(Rng.instance()).get()
     disc = Discovery.new(key, announceAddrs = @[])
 
-  test "updateRecordsAndSpr sets the SPR with both TCP and UDP addresses":
-    disc.updateRecordsAndSpr(@[directAddr], udpPort)
+  test "announceDirectAddrs sets the SPR with both TCP and UDP addresses":
+    disc.announceDirectAddrs(@[directAddr], udpPort)
 
     let spr = disc.getSpr()
     let addrs = spr.data.addresses.mapIt($it.address)
     check addrs.anyIt(it.contains("/tcp/"))
     check addrs.anyIt(it.contains("/udp/"))
 
-  test "updateAnnounceRecord does not update the SPR":
-    disc.updateRecordsAndSpr(@[directAddr], udpPort)
-    let sprBefore = disc.getSpr()
+  test "announceRelayAddrs updates the SPR with the announce addresses":
+    disc.announceDirectAddrs(@[directAddr], udpPort)
 
-    disc.updateAnnounceRecord(@[relayAddr])
+    disc.announceRelayAddrs(@[relayAddr])
 
-    check disc.getSpr() == sprBefore
+    let addrs = disc.getSpr().data.addresses.mapIt($it.address)
+    check addrs == @[$relayAddr]