libp2p: ask for lost relay slot again

1 files changed, 87 insertions, 19 deletions

diff --git a/src/transport/libp2p.rs b/src/transport/libp2p.rs
index d621c8e..060f614 100644
--- a/src/transport/libp2p.rs
+++ b/src/transport/libp2p.rs
@@ -113,9 +113,11 @@ const RELAY_RETRY: Duration = Duration::from_secs(30);
 /// needs them.
 const AUTO_RELAYS: usize = 2;
 const RELAY_CHECK: Duration = Duration::from_secs(30);
-/// A relay that lost the slot before granting it (it doesn't answer, or another peer
-/// answers at its address: a relay restarted without its libp2p key) isn't picked again
-/// for this long.
+/// A relay that refused a slot (`relay_refused`: none left, not allowed, not a relay,
+/// another peer at its address: a relay restarted without its libp2p key) isn't picked
+/// again for this long. One that didn't grant it for another reason (it doesn't answer,
+/// the connection closed) isn't for `RELAY_RETRY`; a slot lost is reserved again at
+/// once.
 const RELAY_PAUSE: Duration = Duration::from_secs(600);
 /// A peer nobody mentioned for this long (the member list, identify, mDNS, a
 /// connection) is forgotten, unless connected: mostly old peer IDs of nodes restarted
@@ -248,7 +250,7 @@ struct Book {
     /// With `Libp2pConfig::public`: other peers that relay (identify says so), and
     /// where.
     public_relays: HashMap<PeerId, Multiaddr>,
-    /// Relays picked that never granted a slot, and when (`RELAY_PAUSE`).
+    /// Relays not to be picked, until when (`RELAY_PAUSE`, `RELAY_RETRY`).
     failed_relays: HashMap<PeerId, tokio::time::Instant>,
     /// Relays that granted us a slot we hold.
     slots: std::collections::HashSet<PeerId>,
@@ -347,6 +349,22 @@ fn relay_failure_text(debug: &str, innermost: &str) -> String {
     innermost.to_owned()
 }
 
+/// Whether a slot ended, or wasn't granted, because the relay refused it (its error's
+/// debug form and innermost cause, as for `relay_failure_text`), rather than because it
+/// didn't answer or the connection closed: only then is it worth not asking again for a
+/// while.
+fn relay_refused(debug: &str, innermost: &str) -> bool {
+    let refusals = [
+        "NoAddressesInReservation",
+        "ResourceLimitExceeded",
+        "PermissionDenied",
+        "connection gated",
+        "UnsupportedProtocol",
+        "WrongPeerId",
+    ];
+    refusals.iter().any(|pattern| debug.contains(pattern)) || innermost == "Failed"
+}
+
 /// The innermost of `error`'s causes, each distinct one once: those libp2p lists in a
 /// "Multiple dial errors occurred:" text (one per line, ` - <cause>: <its cause>...`),
 /// or the last of the source chain.
@@ -459,7 +477,7 @@ impl Book {
                 paused: self
                     .failed_relays
                     .values()
-                    .filter(|at| at.elapsed() < RELAY_PAUSE)
+                    .filter(|until| tokio::time::Instant::now() < **until)
                     .count(),
                 granted: self.counts.slots_granted,
                 lost: self.counts.slots_lost,
@@ -1080,8 +1098,8 @@ fn pick_relays(
     }
     let (direct, configured, listed, members, public) = {
         let mut book = lock(book);
-        book.failed_relays
-            .retain(|_, at| at.elapsed() < RELAY_PAUSE);
+        let now = tokio::time::Instant::now();
+        book.failed_relays.retain(|_, until| now < *until);
         let failed = book.failed_relays.clone();
         let usable = |offers: &HashMap<PeerId, Multiaddr>| -> HashMap<PeerId, Multiaddr> {
             offers
@@ -1363,31 +1381,50 @@ async fn run(
                     // A relay picked: the next check picks again, another one if this one
                     // never granted the slot.
                     if let Some(relay) = picked.remove(&listener_id) {
-                        let lost = {
+                        let refused = match &reason {
+                            Ok(()) => false,
+                            Err(error) => relay_refused(&format!("{error:?}"), &innermost_causes(error)),
+                        };
+                        let (lost, pause) = {
                             let mut book = lock(&book);
                             let lost = book.slots.remove(&relay);
+                            let pause = match (refused, lost) {
+                                (true, _) => Some(RELAY_PAUSE),
+                                (false, true) => None,
+                                (false, false) => Some(RELAY_RETRY),
+                            };
                             if lost {
                                 book.counts.slots_lost += 1;
                             } else {
                                 book.counts.slots_not_granted += 1;
-                                book.failed_relays.insert(relay, tokio::time::Instant::now());
                                 // Named without its peer ID: maybe another one now.
                                 if let Some(address) = book.find_again(relay) {
                                     let _ = again.send(Command::FindRelay { address });
                                 }
                             }
-                            lost
+                            if let Some(pause) = pause {
+                                book.failed_relays.insert(relay, tokio::time::Instant::now() + pause);
+                            }
+                            (lost, pause)
                         };
+                        // Again at once, or others meanwhile.
+                        let _ = again.send(Command::CheckRelays);
                         let reason = relay_failure(&reason);
-                        if lost {
-                            debug!(relay = %Addressed(SCHEME, &relay), reason = %reason, "Lost the slot on a relay");
-                        } else {
-                            info!(
-                                relay = %Addressed(SCHEME, &relay),
-                                pause = ?RELAY_PAUSE,
-                                reason = %reason,
-                                "A relay didn't grant a slot; picking others for a while"
-                            );
+                        match (lost, pause) {
+                            (true, None) => {
+                                info!(relay = %Addressed(SCHEME, &relay), reason = %reason, "Lost the slot on a relay; reserving again");
+                            }
+                            (true, Some(pause)) => {
+                                info!(relay = %Addressed(SCHEME, &relay), pause = ?pause, reason = %reason, "Lost the slot on a relay; picking others for a while");
+                            }
+                            (false, pause) => {
+                                info!(
+                                    relay = %Addressed(SCHEME, &relay),
+                                    pause = ?pause.unwrap_or_default(),
+                                    reason = %reason,
+                                    "A relay didn't grant a slot; picking others for a while"
+                                );
+                            }
                         }
                     }
                 }
@@ -2308,6 +2345,37 @@ mod tests {
         assert_eq!(relay_failure(&Ok(())), "closed");
     }
 
+    /// Only refusals pause a relay; a slot that ended otherwise is asked for again.
+    #[test]
+    fn only_refusals_pause_relays() {
+        let refused = [
+            ("Reservation(Protocol(NoAddressesInReservation))", "x"),
+            ("Reservation(Status(ResourceLimitExceeded))", "x"),
+            ("Reservation(Status(PermissionDenied))", "x"),
+            ("Denied { cause: \"connection gated\" }", "x"),
+            ("Upgrade(UnsupportedProtocol)", "x"),
+            ("Dial(WrongPeerId { obtained: … })", "x"),
+            (
+                "Codec(Error(Custom { kind: Other, error: Failed }))",
+                "Failed",
+            ),
+        ];
+        for (debug, innermost) in refused {
+            assert!(relay_refused(debug, innermost), "{debug}");
+        }
+        let not = [
+            ("Io(Custom { kind: Other, error: Timeout })", "Timeout"),
+            (
+                "Io(Custom { kind: Other, error: Right(Closed) })",
+                "connection is closed",
+            ),
+            ("Io(…)", "connection reset"),
+        ];
+        for (debug, innermost) in not {
+            assert!(!relay_refused(debug, innermost), "{debug}");
+        }
+    }
+
     /// libp2p's nested dial errors come out as each address's cause, once.
     #[test]
     fn dial_errors_are_short() {