From 0a19473a3b04444bf9add33402c2bf7347bf8d48 Mon Sep 17 00:00:00 2001 From: Ivan FB <128452529+Ivansete-status@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:56:12 +0200 Subject: [PATCH] fix: don't apply reconnect backoff to discovered relay peers (#4029) --- .../waku/node/peer_manager/peer_manager.nim | 38 +++++++++------ logos_delivery/waku/waku_core/peers.nim | 1 + tests/node/test_wakunode_peer_manager.nim | 5 ++ tests/test_peer_manager.nim | 48 +++++++++++++++++++ 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/logos_delivery/waku/node/peer_manager/peer_manager.nim b/logos_delivery/waku/node/peer_manager/peer_manager.nim index 31c6133ab..5ffc185b0 100644 --- a/logos_delivery/waku/node/peer_manager/peer_manager.nim +++ b/logos_delivery/waku/node/peer_manager/peer_manager.nim @@ -223,7 +223,10 @@ proc loadFromStorage(pm: PeerManager) {.gcsafe.} = pm.switch.peerStore[ConnectionBook][peerId] = NotConnected # Reset connectedness state pm.switch.peerStore[DisconnectBook][peerId] = remotePeerInfo.disconnectTime - pm.switch.peerStore[SourceBook][peerId] = remotePeerInfo.origin + # Mark as Cache so it is distinguishable from live-discovered peers. If the + # same peer is later rediscovered, addPeer overrides this with the live + # origin, and reconnect backoff no longer applies to it. + pm.switch.peerStore[SourceBook][peerId] = Cache if remotePeerInfo.enr.isSome(): pm.switch.peerStore[ENRBook][peerId] = remotePeerInfo.enr.get() @@ -695,21 +698,28 @@ proc reconnectPeers*( info "Reconnecting peers", proto = proto - # Proto is not persisted, we need to iterate over all peers. - for peerInfo in pm.switch.peerStore.peers(protocolMatcher(proto)): - # Check that the peer can be connected - if peerInfo.connectedness == CannotConnect: - error "Not reconnecting to unreachable or non-existing peer", - peerId = peerInfo.peerId - continue + # Only reconnect peers that come from persistent storage (Cache). Freshly + # discovered peers must not be delayed by the reconnect backoff: they are + # connected right away by the relay connectivity loop. Rediscovered peers get + # their origin overridden by addPeer, so they naturally drop out of this set. + let peersToReconnect = pm.switch.peerStore.peers(protocolMatcher(proto)).filterIt( + it.origin == Cache and it.connectedness != CannotConnect + ) - if backoffTime > ZeroDuration: - info "Backing off before reconnect", - peerId = peerInfo.peerId, backoffTime = backoffTime - # We disconnected recently and still need to wait for a backoff period before connecting - await sleepAsync(backoffTime) + if peersToReconnect.len == 0: + return - await pm.connectToNodes(@[peerInfo]) + # We disconnected recently and still need to wait a backoff period before + # reconnecting. The wait is shared across all peers rather than applied once + # per peer, so reconnection can't stall behind a slow/unreachable peer and + # keep the node from taking on freshly discovered ones. + if backoffTime > ZeroDuration: + info "Backing off before reconnect", backoffTime = backoffTime + await sleepAsync(backoffTime) + + # Dial all reconnectable peers in parallel; a single slow dial must not delay + # the rest. + await allFutures(peersToReconnect.mapIt(pm.connectToNodes(@[it]))) proc getNumStreams*(pm: PeerManager, protocol: string): (int, int) = var diff --git a/logos_delivery/waku/waku_core/peers.nim b/logos_delivery/waku/waku_core/peers.nim index 6e23c848f..c28ae5427 100644 --- a/logos_delivery/waku/waku_core/peers.nim +++ b/logos_delivery/waku/waku_core/peers.nim @@ -40,6 +40,7 @@ type PeerExchange Dns Kademlia + Cache # Loaded from persistent peer storage (not a live discovery source) PeerDirection* = enum UnknownDirection diff --git a/tests/node/test_wakunode_peer_manager.nim b/tests/node/test_wakunode_peer_manager.nim index 0f85d61fe..73ccfe2b9 100644 --- a/tests/node/test_wakunode_peer_manager.nim +++ b/tests/node/test_wakunode_peer_manager.nim @@ -614,6 +614,11 @@ suite "Peer Manager": serverPeerStore.getPeer(clientPeerId).connectedness == Connectedness.CanConnect + # reconnectPeers only acts on peers restored from persistent storage, + # so tag the server as such. Peers coming from live discovery are + # deliberately left out of the reconnect (and its backoff) path. + client.peerManager.addPeer(serverRemotePeerInfo, origin = Cache) + # When triggering the reconnection await client.peerManager.reconnectPeers(WakuRelayCodec) diff --git a/tests/test_peer_manager.nim b/tests/test_peer_manager.nim index b364fc8c3..1dc42d4ce 100644 --- a/tests/test_peer_manager.nim +++ b/tests/test_peer_manager.nim @@ -300,6 +300,54 @@ procSuite "Peer Manager": await allFutures(nodes.mapIt(it.stop())) + asyncTest "reconnectPeers only reconnects persisted (Cache) peers": + ## Regression: freshly discovered relay peers were wrongly swept into the + ## reconnect backoff path. reconnectPeers must act only on peers loaded from + ## persistent storage (origin == Cache); discovered peers are left for the + ## connectivity loop to dial without any backoff. + let node = newTestWakuNode(generateSecp256k1Key()) + await node.start() + + let basePeerId = "QmeuZJbXrszW2jdT7GdduSjQskPU3S7vvGWKtKgDfkDvW" + var cachePeerId, discoveredPeerId: PeerId + require cachePeerId.init(basePeerId & "1") + require discoveredPeerId.init(basePeerId & "2") + + # Both peers advertise relay and point at an unreachable address, so a dial + # attempt fails fast and is observable via the failed-connection counter. + let unreachableAddr = MultiAddress.init("/ip4/127.0.0.1/tcp/1").tryGet() + + node.peerManager.addPeer( + RemotePeerInfo.init( + peerId = cachePeerId, addrs = @[unreachableAddr], protocols = @[WakuRelayCodec] + ), + origin = Cache, + ) + node.peerManager.addPeer( + RemotePeerInfo.init( + peerId = discoveredPeerId, + addrs = @[unreachableAddr], + protocols = @[WakuRelayCodec], + ), + origin = Discv5, + ) + + # Default backoff is zero, so the test stays fast; the origin filter is what + # we are validating here. + await node.peerManager.reconnectPeers(WakuRelayCodec) + + let peerStore = node.peerManager.switch.peerStore + check: + # The Cache peer was dialed (and failed against the unreachable address). + peerStore[NumberFailedConnBook][cachePeerId] == 1 + peerStore.connectedness(cachePeerId) == CannotConnect + + # The discovered peer was never touched by reconnectPeers. + peerStore[NumberFailedConnBook][discoveredPeerId] == 0 + peerStore.connectedness(discoveredPeerId) == NotConnected + + await node.stop() + asyncTest "Peer manager can use persistent storage and survive restarts": let database = SqliteDatabase.new(":memory:")[]