diff --git a/Cargo.lock b/Cargo.lock index d0877c9e0..131d64280 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1385,6 +1385,12 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "ip_network" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa2f047c0a98b2f299aa5d6d7088443570faae494e9ae1305e48be000c9e0eb1" + [[package]] name = "ipconfig" version = "0.3.2" @@ -1874,6 +1880,7 @@ dependencies = [ "hex-literal", "hickory-resolver", "indexmap", + "ip_network", "libc", "libp2p", "mockall", diff --git a/Cargo.toml b/Cargo.toml index 723eb4e8a..722a88837 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ ed25519-dalek = { version = "2.1.1", features = ["rand_core"] } futures = "0.3.27" futures-timer = "3.0.3" indexmap = { version = "2.9.0", features = ["std"] } +ip_network = "0.4" libc = "0.2.158" mockall = "0.13.1" multiaddr = "0.17.0" diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 68325d7df..a812a4f49 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -20,6 +20,7 @@ use crate::{error::DialError, PeerId}; +use ip_network::IpNetwork; use multiaddr::{Multiaddr, Protocol}; use multihash::Multihash; @@ -38,8 +39,14 @@ pub mod scores { /// Score for providing an invalid address. /// - /// This address can never be reached. + /// This address can never be reached and is effectively banned. pub const ADDRESS_FAILURE: i32 = i32::MIN; + + /// Initial score for public/global addresses. + /// + /// This gives public addresses a slight priority over private addresses + /// when all addresses are untested (private addresses start at 0). + pub const PUBLIC_ADDRESS_BONUS: i32 = 1i32; } #[allow(clippy::derived_hash_with_manual_eq)] @@ -70,7 +77,7 @@ impl AddressRecord { address }; - Self { address, score } + Self::from_raw_multiaddr_with_score(address, score) } /// Create `AddressRecord` from `Multiaddr`. @@ -82,10 +89,7 @@ impl AddressRecord { return None; } - Some(AddressRecord { - address, - score: 0i32, - }) + Some(Self::from_raw_multiaddr_with_score(address, 0)) } /// Create `AddressRecord` from `Multiaddr`. @@ -94,10 +98,7 @@ impl AddressRecord { /// /// Please consider using [`Self::from_multiaddr`] from the transport manager code. pub fn from_raw_multiaddr(address: Multiaddr) -> AddressRecord { - AddressRecord { - address, - score: 0i32, - } + Self::from_raw_multiaddr_with_score(address, 0) } /// Create `AddressRecord` from `Multiaddr`. @@ -106,7 +107,7 @@ impl AddressRecord { /// /// Please consider using [`Self::from_multiaddr`] from the transport manager code. pub fn from_raw_multiaddr_with_score(address: Multiaddr, score: i32) -> AddressRecord { - AddressRecord { address, score } + Self { address, score } } /// Get address score. @@ -122,8 +123,29 @@ impl AddressRecord { /// Update score of an address. pub fn update_score(&mut self, score: i32) { - self.score = self.score.saturating_add(score); + self.score = score; + } +} + +/// Check if a multiaddr represents a global/public address. +/// +/// DNS addresses are considered potentially public. +fn is_global_multiaddr(address: &Multiaddr) -> bool { + for protocol in address.iter() { + match protocol { + Protocol::Ip4(ip) => return IpNetwork::from(ip).is_global(), + Protocol::Ip6(ip) => return IpNetwork::from(ip).is_global(), + // DNS addresses could resolve to public IPs, treat as potentially public. + // Ideally we need to resolve DNS to check the actual IPs. However, this + // is a more complex operation that requires async DNS resolution in the + // transport manager context / transport layer. + Protocol::Dns(_) | Protocol::Dns4(_) | Protocol::Dns6(_) => return true, + _ => continue, + } } + + // Consider the address as non-global if no IP or DNS component is found + false } impl PartialEq for AddressRecord { @@ -219,14 +241,31 @@ impl AddressStore { /// Insert the address record into [`AddressStore`] with the provided score. /// - /// If the address is not in the store, it will be inserted. - /// Otherwise, the score and connection ID will be updated. + /// If the address is not in the store, it will be inserted with a bonus for public addresses. + /// Otherwise, the score will be updated only for connection events (non-zero scores), + /// not for re-adding the same address which should not overwrite connection history. pub fn insert(&mut self, record: AddressRecord) { if let Entry::Occupied(mut occupied) = self.addresses.entry(record.address.clone()) { - occupied.get_mut().update_score(record.score); + // Only update score for connection events (non-zero scores). + // Re-adding an address (score 0) via rediscovery should not wipe out + // connection success/failure history. + if record.score != 0 { + occupied.get_mut().update_score(record.score); + } return; } + // Reward public addresses with a bonus. + let is_public = is_global_multiaddr(&record.address); + let record = if is_public { + AddressRecord { + score: record.score.saturating_add(scores::PUBLIC_ADDRESS_BONUS), + ..record + } + } else { + record + }; + // The eviction algorithm favours addresses with higher scores. // // This algorithm has the following implications: @@ -275,10 +314,10 @@ mod tests { let peer = PeerId::random(); let address = std::net::SocketAddr::V4(SocketAddrV4::new( Ipv4Addr::new( - rng.gen_range(1..=255), - rng.gen_range(0..=255), + 10, rng.gen_range(0..=255), rng.gen_range(0..=255), + rng.gen_range(1..=255), ), rng.gen_range(1..=65535), )); @@ -297,10 +336,10 @@ mod tests { let peer = PeerId::random(); let address = std::net::SocketAddr::V4(SocketAddrV4::new( Ipv4Addr::new( - rng.gen_range(1..=255), - rng.gen_range(0..=255), + 10, rng.gen_range(0..=255), rng.gen_range(0..=255), + rng.gen_range(1..=255), ), rng.gen_range(1..=65535), )); @@ -320,10 +359,10 @@ mod tests { let peer = PeerId::random(); let address = std::net::SocketAddr::V4(SocketAddrV4::new( Ipv4Addr::new( - rng.gen_range(1..=255), - rng.gen_range(0..=255), + 10, rng.gen_range(0..=255), rng.gen_range(0..=255), + rng.gen_range(1..=255), ), rng.gen_range(1..=65535), )); @@ -479,12 +518,98 @@ mod tests { assert_eq!(store.addresses.len(), 1); assert_eq!(store.addresses.get(record.address()).unwrap(), &record); - // This time the record is updated. + // This time the record score is replaced (not accumulated). store.insert(record.clone()); assert_eq!(store.addresses.len(), 1); let store_record = store.addresses.get(record.address()).unwrap(); - assert_eq!(store_record.score, record.score * 2); + assert_eq!(store_record.score, record.score); + } + + #[test] + fn insert_record_does_not_accumulate_public_bonus() { + let mut store = AddressStore::new(); + let peer = PeerId::random(); + + // Create a public address (8.8.8.8 is global) using from_multiaddr. + // The bonus is NOT applied at construction time, only when first inserted. + let address = Multiaddr::empty() + .with(Protocol::Ip4(Ipv4Addr::new(8, 8, 8, 8))) + .with(Protocol::Tcp(9999)) + .with(Protocol::P2p( + multihash::Multihash::from_bytes(&peer.to_bytes()).unwrap(), + )); + + let record = AddressRecord::from_multiaddr(address.clone()).unwrap(); + assert_eq!(record.score, 0); + + store.insert(record.clone()); + assert_eq!(store.addresses.len(), 1); + // Bonus applied on first insert. + assert_eq!( + store.addresses.get(&address).unwrap().score, + scores::PUBLIC_ADDRESS_BONUS + ); + + // Re-adding the same address should NOT accumulate the bonus. + let record2 = AddressRecord::from_multiaddr(address.clone()).unwrap(); + store.insert(record2); + + assert_eq!(store.addresses.len(), 1); + // Score should still be 1, not 2. + assert_eq!( + store.addresses.get(&address).unwrap().score, + scores::PUBLIC_ADDRESS_BONUS + ); + + // However, connection events should still update (replace) the score. + let connection_record = + AddressRecord::new(&peer, address.clone(), scores::CONNECTION_ESTABLISHED); + store.insert(connection_record); + + assert_eq!(store.addresses.len(), 1); + // Score should now be CONNECTION_ESTABLISHED (bonus only applied on first insert). + assert_eq!( + store.addresses.get(&address).unwrap().score, + scores::CONNECTION_ESTABLISHED + ); + } + + #[test] + fn rediscovery_does_not_wipe_dial_failure() { + let mut store = AddressStore::new(); + let peer = PeerId::random(); + + // Public address (8.8.8.8 is global). + let address = Multiaddr::empty() + .with(Protocol::Ip4(Ipv4Addr::new(8, 8, 8, 8))) + .with(Protocol::Tcp(9999)) + .with(Protocol::P2p( + multihash::Multihash::from_bytes(&peer.to_bytes()).unwrap(), + )); + + // First, add the address normally. + let record = AddressRecord::from_multiaddr(address.clone()).unwrap(); + store.insert(record); + assert_eq!( + store.addresses.get(&address).unwrap().score, + scores::PUBLIC_ADDRESS_BONUS + ); + + // Dial failure occurs (bonus only applied on first insert, not on updates). + let failure_record = AddressRecord::new(&peer, address.clone(), scores::CONNECTION_FAILURE); + store.insert(failure_record); + let failure_score = scores::CONNECTION_FAILURE; + assert_eq!(store.addresses.get(&address).unwrap().score, failure_score); + + // Address is rediscovered via Kademlia (creates record with score 0). + // This should NOT wipe out the dial failure score. + let rediscovered = AddressRecord::from_multiaddr(address.clone()).unwrap(); + assert_eq!(rediscovered.score, 0); + store.insert(rediscovered); + + // Score should still reflect the failure, not 0. + assert_eq!(store.addresses.get(&address).unwrap().score, failure_score); } #[test]