From 03496992008ed60d1a2f429687ec54d3e85365cb Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Thu, 4 May 2023 12:56:12 +0200 Subject: [PATCH] node: Don't connect to recently attempted peers --- radicle-node/src/address/store.rs | 11 +++++++--- radicle-node/src/service.rs | 20 ++++++++++++------ radicle-node/src/test/peer.rs | 7 ++----- radicle-node/src/tests.rs | 35 +++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/radicle-node/src/address/store.rs b/radicle-node/src/address/store.rs index 983404cc..96de4ac2 100644 --- a/radicle-node/src/address/store.rs +++ b/radicle-node/src/address/store.rs @@ -12,6 +12,7 @@ use crate::address::types; use crate::address::{KnownAddress, Source}; use crate::service::NodeId; use crate::wire::AddressType; +use crate::LocalTime; #[derive(Error, Debug)] pub enum Error { @@ -175,7 +176,7 @@ impl Store for Book { fn entries(&self) -> Result>, Error> { let mut stmt = self .db - .prepare("SELECT node, type, value, source FROM addresses ORDER BY node")? + .prepare("SELECT node, type, value, source, last_success, last_attempt FROM addresses ORDER BY node")? .into_iter(); let mut entries = Vec::new(); @@ -184,14 +185,18 @@ impl Store for Book { let _typ = row.read::("type"); let addr = row.read::("value"); let source = row.read::("source"); + let last_success = row.read::, _>("last_success"); + let last_attempt = row.read::, _>("last_attempt"); + let last_success = last_success.map(|t| LocalTime::from_millis(t as u128)); + let last_attempt = last_attempt.map(|t| LocalTime::from_millis(t as u128)); entries.push(( node, KnownAddress { addr, source, - last_success: None, - last_attempt: None, + last_success, + last_attempt, }, )); } diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 0989924d..c5ea5e8d 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -20,7 +20,7 @@ use localtime::{LocalDuration, LocalTime}; use log::*; use crate::address; -use crate::address::AddressBook; +use crate::address::{AddressBook, KnownAddress}; use crate::crypto; use crate::crypto::{Signer, Verified}; use crate::identity::IdentityError; @@ -997,7 +997,7 @@ where timestamp, addresses .iter() - .map(|a| address::KnownAddress::new(a.clone(), address::Source::Peer)), + .map(|a| KnownAddress::new(a.clone(), address::Source::Peer)), ) { Ok(updated) => { // Only relay if we received new information. @@ -1404,7 +1404,7 @@ where } /// Get a list of peers available to connect to. - fn available_peers(&mut self) -> Vec<(NodeId, Address)> { + fn available_peers(&mut self) -> Vec<(NodeId, KnownAddress)> { let outbound = self .sessions .values() @@ -1425,7 +1425,6 @@ where entries .filter(|(nid, _)| !self.sessions.contains_key(nid)) .take(wanted) - .map(|(n, s)| (n, s.addr)) .collect() } Err(e) => { @@ -1473,8 +1472,17 @@ where } fn maintain_connections(&mut self) { - for (id, addr) in self.available_peers() { - self.connect(id, addr.clone()); + let now = self.clock; + + // Nb. We use the `MAX_RECONNECTION_DELTA` to know when it's ok to reconnect, because + // these aren't persistent peers. They could go offline for a long time and we don't want to + // be too persistent. + for (id, ka) in self + .available_peers() + .into_iter() + .filter(|(_, ka)| now - ka.last_attempt.unwrap_or_default() >= MAX_RECONNECTION_DELTA) + { + self.connect(id, ka.addr.clone()); } } diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs index 2e8abac6..4e02ac51 100644 --- a/radicle-node/src/test/peer.rs +++ b/radicle-node/src/test/peer.rs @@ -202,12 +202,9 @@ where Address::from(net::SocketAddr::from((self.ip, 8776))) } - pub fn import_addresses

(&mut self, peers: P) - where - P: AsRef<[Self]>, - { + pub fn import_addresses<'a>(&mut self, peers: impl IntoIterator) { let timestamp = self.timestamp(); - for peer in peers.as_ref() { + for peer in peers.into_iter() { let known_address = address::KnownAddress::new(peer.address(), address::Source::Peer); self.service .addresses_mut() diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index 434804d1..2f4c964c 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -1018,6 +1018,41 @@ fn test_maintain_connections() { ); } +#[test] +fn test_maintain_connections_failed_attempt() { + let bob = Peer::new("bob", [8, 8, 8, 8]); + let eve = Peer::new("eve", [9, 9, 9, 9]); + let mut alice = Peer::new("alice", [7, 7, 7, 7]); + + alice.connect_to(&bob); + // Make sure Alice knows about Eve. + alice.receive(bob.id, eve.node_announcement()); + alice.disconnected(bob.id(), &DisconnectReason::Command); + alice + .outbox() + .find(|o| matches!(o, Io::Connect(id, _) if id == &eve.id)) + .expect("Alice attempts Eve"); + alice.attempted(eve.id, eve.addr()); + + // Disconnect Eve and make sure Alice doesn't try to re-connect immediately. + alice.disconnected(eve.id(), &DisconnectReason::Command); + assert!(!alice.outbox().any(|o| matches!(o, Io::Connect(_, _)))); + + // Now pass some time and try again. + alice.elapse(MAX_RECONNECTION_DELTA); + alice + .outbox() + .find(|o| matches!(o, Io::Connect(id, _) if id == &eve.id)) + .expect("Alice attempts Eve again"); + + // Disconnect Eve and make sure Alice doesn't try to re-connect immediately. + alice.disconnected(eve.id(), &DisconnectReason::Command); + assert!(!alice.outbox().any(|o| matches!(o, Io::Connect(_, _)))); + // Or even after some short time.. + alice.elapse(MIN_RECONNECTION_DELTA); + assert!(!alice.outbox().any(|o| matches!(o, Io::Connect(_, _)))); +} + #[test] fn test_track_repo_subscribe() { let mut alice = Peer::new("alice", [7, 7, 7, 7]);