From 8b38fbbc69b57e4b1c5f2caa97915fad55cd6e56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20RIBEAU?= Date: Tue, 7 Nov 2023 12:54:48 +0100 Subject: [PATCH 1/3] fix(kad): potentially incorrect return value of Addresses::remove --- Cargo.lock | 2 +- Cargo.toml | 2 +- protocols/kad/CHANGELOG.md | 5 +++ protocols/kad/Cargo.toml | 2 +- protocols/kad/src/addresses.rs | 66 +++++++++++++++++++++++++++++++++- 5 files changed, 73 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f1e0c52e6e1..3d4af120bc6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2724,7 +2724,7 @@ dependencies = [ [[package]] name = "libp2p-kad" -version = "0.45.0" +version = "0.45.1" dependencies = [ "arrayvec", "async-std", diff --git a/Cargo.toml b/Cargo.toml index 89790baf82e..a1a40a0d749 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -82,7 +82,7 @@ libp2p-floodsub = { version = "0.44.0", path = "protocols/floodsub" } libp2p-gossipsub = { version = "0.46.0", path = "protocols/gossipsub" } libp2p-identify = { version = "0.44.0", path = "protocols/identify" } libp2p-identity = { version = "0.2.7" } -libp2p-kad = { version = "0.45.0", path = "protocols/kad" } +libp2p-kad = { version = "0.45.1", path = "protocols/kad" } libp2p-mdns = { version = "0.45.0", path = "protocols/mdns" } libp2p-memory-connection-limits = { version = "0.2.0", path = "misc/memory-connection-limits" } libp2p-metrics = { version = "0.14.0", path = "misc/metrics" } diff --git a/protocols/kad/CHANGELOG.md b/protocols/kad/CHANGELOG.md index 842204cbef2..99f1f0a6105 100644 --- a/protocols/kad/CHANGELOG.md +++ b/protocols/kad/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.45.1 + +- Fix a bug where calling `Behaviour::remove_address` with an address not in the peer's bucket would remove the peer from the routing table if the bucket has only one address left. + See [PR 4816](https://github.com/libp2p/rust-libp2p/pull/4816) + ## 0.45.0 - Remove deprecated `kad::Config::set_connection_idle_timeout` in favor of `SwarmBuilder::idle_connection_timeout`. diff --git a/protocols/kad/Cargo.toml b/protocols/kad/Cargo.toml index 9410ff0eebe..51e9656441f 100644 --- a/protocols/kad/Cargo.toml +++ b/protocols/kad/Cargo.toml @@ -3,7 +3,7 @@ name = "libp2p-kad" edition = "2021" rust-version = { workspace = true } description = "Kademlia protocol for libp2p" -version = "0.45.0" +version = "0.45.1" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" diff --git a/protocols/kad/src/addresses.rs b/protocols/kad/src/addresses.rs index 3c2af4173fd..d4ceacf81f2 100644 --- a/protocols/kad/src/addresses.rs +++ b/protocols/kad/src/addresses.rs @@ -67,7 +67,7 @@ impl Addresses { /// otherwise unreachable. #[allow(clippy::result_unit_err)] pub fn remove(&mut self, addr: &Multiaddr) -> Result<(), ()> { - if self.addrs.len() == 1 { + if self.addrs.len() == 1 && self.addrs[0] == *addr { return Err(()); } @@ -113,3 +113,67 @@ impl fmt::Debug for Addresses { f.debug_list().entries(self.addrs.iter()).finish() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn given_one_address_when_removing_different_one_returns_ok() { + let addrs = &[tcp_addr("1234")]; + let mut addresses = make_addresses(addrs); + + assert!(addresses.remove(&tcp_addr("4321")).is_ok()); + // assert that content did not change since the removed value was not present in the addresses list + assert_eq!(addresses.addrs.as_slice(), addrs); + } + + #[test] + fn given_one_address_when_removing_correct_one_returns_err() { + let addrs = &[tcp_addr("1234")]; + let mut addresses = make_addresses(addrs); + + assert!(addresses.remove(&tcp_addr("1234")).is_err()); + // assert that content did not change since the removed value, while present in the addresses list, was the last one + assert_eq!(addresses.addrs.as_slice(), addrs); + } + + #[test] + fn given_many_addresses_when_removing_different_one_does_not_remove_and_returns_ok() { + let addrs = &[tcp_addr("1234"), tcp_addr("4321")]; + let mut addresses = make_addresses(addrs); + + assert!(addresses.remove(&tcp_addr("5678")).is_ok()); + // assert that content did not change since the removed value was not present in the addresses list + assert_eq!(addresses.addrs.as_slice(), addrs); + } + + #[test] + fn given_many_addresses_when_removing_correct_one_removes_and_returns_ok() { + let mut addresses = make_addresses(&[tcp_addr("1234"), tcp_addr("4321")]); + + assert!(addresses.remove(&tcp_addr("1234")).is_ok()); + // assert that content did change since the removed value was present in the addresses list and was not the last one + assert_eq!(addresses.addrs.as_slice(), &[tcp_addr("4321")]); + } + + /// Helper function to easily initialize Addresses struct + /// with multiple addresses + fn make_addresses(addrs: &[Multiaddr]) -> Addresses { + let mut addrs = addrs.into_iter(); + + let first = addrs.next().expect("Addresses must have at least one addr"); + let mut addresses = Addresses::new(first.clone()); + + while let Some(addr) = addrs.next() { + addresses.insert(addr.clone()); + } + + addresses + } + + /// Helper function to create a tcp Multiaddr with a specific port + fn tcp_addr(port: &str) -> Multiaddr { + format!("/ip4/127.0.0.1/tcp/{port}").parse().unwrap() + } +} From c52a78d1df3939254093e11cebc0e4665623a2d4 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 10 Nov 2023 13:16:05 +1100 Subject: [PATCH 2/3] Refactor tests towards arrange - act - assert --- protocols/kad/src/addresses.rs | 73 +++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/protocols/kad/src/addresses.rs b/protocols/kad/src/addresses.rs index d4ceacf81f2..da0754c9823 100644 --- a/protocols/kad/src/addresses.rs +++ b/protocols/kad/src/addresses.rs @@ -120,60 +120,69 @@ mod tests { #[test] fn given_one_address_when_removing_different_one_returns_ok() { - let addrs = &[tcp_addr("1234")]; - let mut addresses = make_addresses(addrs); + let mut addresses = make_addresses([tcp_addr(1234)]); - assert!(addresses.remove(&tcp_addr("4321")).is_ok()); - // assert that content did not change since the removed value was not present in the addresses list - assert_eq!(addresses.addrs.as_slice(), addrs); + let result = addresses.remove(&tcp_addr(4321)); + + assert!(result.is_ok()); + assert_eq!( + addresses.into_vec(), + vec![tcp_addr(1234)], + "`Addresses` to not change because we tried to remove a non-present address" + ); } #[test] fn given_one_address_when_removing_correct_one_returns_err() { - let addrs = &[tcp_addr("1234")]; - let mut addresses = make_addresses(addrs); + let mut addresses = make_addresses([tcp_addr(1234)]); + + let result = addresses.remove(&tcp_addr(1234)); - assert!(addresses.remove(&tcp_addr("1234")).is_err()); - // assert that content did not change since the removed value, while present in the addresses list, was the last one - assert_eq!(addresses.addrs.as_slice(), addrs); + assert!(result.is_err()); + assert_eq!( + addresses.into_vec(), + vec![tcp_addr(1234)], + "`Addresses` to not be empty because it would have been the last address to be removed" + ); } #[test] fn given_many_addresses_when_removing_different_one_does_not_remove_and_returns_ok() { - let addrs = &[tcp_addr("1234"), tcp_addr("4321")]; - let mut addresses = make_addresses(addrs); + let mut addresses = make_addresses([tcp_addr(1234), tcp_addr(4321)]); - assert!(addresses.remove(&tcp_addr("5678")).is_ok()); - // assert that content did not change since the removed value was not present in the addresses list - assert_eq!(addresses.addrs.as_slice(), addrs); + let result = addresses.remove(&tcp_addr(5678)); + + assert!(result.is_ok()); + assert_eq!( + addresses.into_vec(), + vec![tcp_addr(1234), tcp_addr(4321)], + "`Addresses` to not change because we tried to remove a non-present address" + ); } #[test] fn given_many_addresses_when_removing_correct_one_removes_and_returns_ok() { - let mut addresses = make_addresses(&[tcp_addr("1234"), tcp_addr("4321")]); + let mut addresses = make_addresses([tcp_addr(1234), tcp_addr(4321)]); - assert!(addresses.remove(&tcp_addr("1234")).is_ok()); - // assert that content did change since the removed value was present in the addresses list and was not the last one - assert_eq!(addresses.addrs.as_slice(), &[tcp_addr("4321")]); - } + let result = addresses.remove(&tcp_addr(1234)); - /// Helper function to easily initialize Addresses struct - /// with multiple addresses - fn make_addresses(addrs: &[Multiaddr]) -> Addresses { - let mut addrs = addrs.into_iter(); - - let first = addrs.next().expect("Addresses must have at least one addr"); - let mut addresses = Addresses::new(first.clone()); + assert!(result.is_ok()); + assert_eq!( + addresses.into_vec(), + vec![tcp_addr(4321)], + "`Addresses to no longer contain address was present and then removed`" + ); + } - while let Some(addr) = addrs.next() { - addresses.insert(addr.clone()); + /// Helper function to easily initialize Addresses struct with multiple addresses. + fn make_addresses(addresses: impl IntoIterator) -> Addresses { + Addresses { + addrs: SmallVec::from_iter(addresses), } - - addresses } /// Helper function to create a tcp Multiaddr with a specific port - fn tcp_addr(port: &str) -> Multiaddr { + fn tcp_addr(port: u16) -> Multiaddr { format!("/ip4/127.0.0.1/tcp/{port}").parse().unwrap() } } From 7c103372b61ab0549e6446fe3e25470364201f15 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 10 Nov 2023 13:16:33 +1100 Subject: [PATCH 3/3] Add unreleased suffix --- protocols/kad/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/CHANGELOG.md b/protocols/kad/CHANGELOG.md index 99f1f0a6105..61472a5f5d9 100644 --- a/protocols/kad/CHANGELOG.md +++ b/protocols/kad/CHANGELOG.md @@ -1,4 +1,4 @@ -## 0.45.1 +## 0.45.1 - unreleased - Fix a bug where calling `Behaviour::remove_address` with an address not in the peer's bucket would remove the peer from the routing table if the bucket has only one address left. See [PR 4816](https://github.com/libp2p/rust-libp2p/pull/4816)