From a35e724111d9f90f42bf87abf3274fda5e614756 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:25:03 +0200 Subject: [PATCH 1/8] Strip unnecessary parts from test --- protocols/gossipsub/src/config.rs | 32 ++++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/protocols/gossipsub/src/config.rs b/protocols/gossipsub/src/config.rs index 098a3eb7e0b..b8a6c19577f 100644 --- a/protocols/gossipsub/src/config.rs +++ b/protocols/gossipsub/src/config.rs @@ -980,17 +980,13 @@ mod test { #[test] fn create_config_with_protocol_id_prefix() { - let builder: Config = ConfigBuilder::default() - .protocol_id_prefix("purple") - .validation_mode(ValidationMode::Anonymous) - .message_id_fn(message_id_plain_function) - .build() - .unwrap(); - - assert_eq!(builder.protocol_id(), "purple"); - assert_eq!(builder.custom_id_version(), &None); + let protocol_config = ProtocolConfig::new( + &ConfigBuilder::default() + .protocol_id_prefix("purple") + .build() + .unwrap(), + ); - let protocol_config = ProtocolConfig::new(&builder); let protocol_ids = protocol_config.protocol_info(); assert_eq!(protocol_ids.len(), 2); @@ -1004,17 +1000,13 @@ mod test { #[test] fn create_config_with_custom_protocol_id() { - let builder: Config = ConfigBuilder::default() - .protocol_id("purple", Version::V1_0) - .validation_mode(ValidationMode::Anonymous) - .message_id_fn(message_id_plain_function) - .build() - .unwrap(); - - assert_eq!(builder.protocol_id(), "purple"); - assert_eq!(builder.custom_id_version(), &Some(Version::V1_0)); + let protocol_config = ProtocolConfig::new( + &ConfigBuilder::default() + .protocol_id("purple", Version::V1_0) + .build() + .unwrap(), + ); - let protocol_config = ProtocolConfig::new(&builder); let protocol_ids = protocol_config.protocol_info(); assert_eq!(protocol_ids.len(), 1); From fa746c2f4ec5f6b60fc27239d8867c9d4d4c44fd Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:25:25 +0200 Subject: [PATCH 2/8] Remove test without assertion --- protocols/gossipsub/src/config.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/protocols/gossipsub/src/config.rs b/protocols/gossipsub/src/config.rs index b8a6c19577f..12e088e2bdd 100644 --- a/protocols/gossipsub/src/config.rs +++ b/protocols/gossipsub/src/config.rs @@ -892,16 +892,6 @@ mod test { use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; - #[test] - fn create_thing() { - let builder: Config = ConfigBuilder::default() - .protocol_id_prefix("purple") - .build() - .unwrap(); - - dbg!(builder); - } - fn get_gossipsub_message() -> Message { Message { source: None, From 75b91493ebd30954b61c77e11f592ebf247a5c78 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:25:46 +0200 Subject: [PATCH 3/8] Move helpers to the bottom --- protocols/gossipsub/src/config.rs | 47 ++++++++++++++++--------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/protocols/gossipsub/src/config.rs b/protocols/gossipsub/src/config.rs index 12e088e2bdd..fb70048cbb1 100644 --- a/protocols/gossipsub/src/config.rs +++ b/protocols/gossipsub/src/config.rs @@ -892,29 +892,6 @@ mod test { use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; - fn get_gossipsub_message() -> Message { - Message { - source: None, - data: vec![12, 34, 56], - sequence_number: None, - topic: Topic::<IdentityHash>::new("test").hash(), - } - } - - fn get_expected_message_id() -> MessageId { - MessageId::from([ - 49, 55, 56, 51, 56, 52, 49, 51, 52, 51, 52, 55, 51, 51, 53, 52, 54, 54, 52, 49, 101, - ]) - } - - fn message_id_plain_function(message: &Message) -> MessageId { - let mut s = DefaultHasher::new(); - message.data.hash(&mut s); - let mut v = s.finish().to_string(); - v.push('e'); - MessageId::from(v) - } - #[test] fn create_config_with_message_id_as_plain_function() { let builder: Config = ConfigBuilder::default() @@ -1004,4 +981,28 @@ mod test { assert_eq!(protocol_ids[0].protocol_id, b"purple".to_vec()); assert_eq!(protocol_ids[0].kind, PeerKind::Gossipsub); } + + + fn get_gossipsub_message() -> Message { + Message { + source: None, + data: vec![12, 34, 56], + sequence_number: None, + topic: Topic::<IdentityHash>::new("test").hash(), + } + } + + fn get_expected_message_id() -> MessageId { + MessageId::from([ + 49, 55, 56, 51, 56, 52, 49, 51, 52, 51, 52, 55, 51, 51, 53, 52, 54, 54, 52, 49, 101, + ]) + } + + fn message_id_plain_function(message: &Message) -> MessageId { + let mut s = DefaultHasher::new(); + message.data.hash(&mut s); + let mut v = s.finish().to_string(); + v.push('e'); + MessageId::from(v) + } } From ab3a428dc1a2beb58cb2f037e498611dd4a88fc2 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:27:29 +0200 Subject: [PATCH 4/8] Further simplify tests --- protocols/gossipsub/src/config.rs | 52 ++++++++++++++----------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/protocols/gossipsub/src/config.rs b/protocols/gossipsub/src/config.rs index fb70048cbb1..9fe5ee8cf5e 100644 --- a/protocols/gossipsub/src/config.rs +++ b/protocols/gossipsub/src/config.rs @@ -894,54 +894,51 @@ mod test { #[test] fn create_config_with_message_id_as_plain_function() { - let builder: Config = ConfigBuilder::default() - .protocol_id_prefix("purple") + let config = ConfigBuilder::default() .message_id_fn(message_id_plain_function) .build() .unwrap(); - let result = builder.message_id(&get_gossipsub_message()); + let result = config.message_id(&get_gossipsub_message()); + assert_eq!(result, get_expected_message_id()); } #[test] fn create_config_with_message_id_as_closure() { - let closure = |message: &Message| { - let mut s = DefaultHasher::new(); - message.data.hash(&mut s); - let mut v = s.finish().to_string(); - v.push('e'); - MessageId::from(v) - }; - - let builder: Config = ConfigBuilder::default() - .protocol_id_prefix("purple") - .message_id_fn(closure) + let config = ConfigBuilder::default() + .message_id_fn(|message: &Message| { + let mut s = DefaultHasher::new(); + message.data.hash(&mut s); + let mut v = s.finish().to_string(); + v.push('e'); + MessageId::from(v) + }) .build() .unwrap(); - let result = builder.message_id(&get_gossipsub_message()); + let result = config.message_id(&get_gossipsub_message()); + assert_eq!(result, get_expected_message_id()); } #[test] fn create_config_with_message_id_as_closure_with_variable_capture() { let captured: char = 'e'; - let closure = move |message: &Message| { - let mut s = DefaultHasher::new(); - message.data.hash(&mut s); - let mut v = s.finish().to_string(); - v.push(captured); - MessageId::from(v) - }; - - let builder: Config = ConfigBuilder::default() - .protocol_id_prefix("purple") - .message_id_fn(closure) + + let config = ConfigBuilder::default() + .message_id_fn(move |message: &Message| { + let mut s = DefaultHasher::new(); + message.data.hash(&mut s); + let mut v = s.finish().to_string(); + v.push(captured); + MessageId::from(v) + }) .build() .unwrap(); - let result = builder.message_id(&get_gossipsub_message()); + let result = config.message_id(&get_gossipsub_message()); + assert_eq!(result, get_expected_message_id()); } @@ -982,7 +979,6 @@ mod test { assert_eq!(protocol_ids[0].kind, PeerKind::Gossipsub); } - fn get_gossipsub_message() -> Message { Message { source: None, From 00415efdd86a5001def1c5b1b75faf267c0e1d84 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:58:47 +0200 Subject: [PATCH 5/8] Add failing test --- protocols/gossipsub/src/protocol_priv.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/protocols/gossipsub/src/protocol_priv.rs b/protocols/gossipsub/src/protocol_priv.rs index 34d931bbaf9..a0efff940c1 100644 --- a/protocols/gossipsub/src/protocol_priv.rs +++ b/protocols/gossipsub/src/protocol_priv.rs @@ -556,8 +556,8 @@ impl Decoder for GossipsubCodec { mod tests { use super::*; use crate::config::Config; - use crate::Behaviour; use crate::IdentTopic as Topic; + use crate::{Behaviour, ConfigBuilder}; use libp2p_core::identity::Keypair; use quickcheck_ext::*; @@ -653,4 +653,24 @@ mod tests { QuickCheck::new().quickcheck(prop as fn(_) -> _) } + + #[test] + fn support_floodsub_with_custom_protocol() { + let gossipsub_config = ConfigBuilder::default() + .protocol_id("/foosub", Version::V1_1) + .support_floodsub() + .build() + .unwrap(); + + let protocol_config = ProtocolConfig::new(&gossipsub_config); + + assert_eq!( + String::from_utf8_lossy(&protocol_config.protocol_ids[0].protocol_id), + "/foosub" + ); + assert_eq!( + String::from_utf8_lossy(&protocol_config.protocol_ids[1].protocol_id), + "/floodsub/1.0.0" + ); + } } From 117f0a33f4c46e3bd68eec34200a2bceeb6367ab Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 18:59:30 +0200 Subject: [PATCH 6/8] Fix test --- protocols/gossipsub/src/protocol_priv.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/protocols/gossipsub/src/protocol_priv.rs b/protocols/gossipsub/src/protocol_priv.rs index a0efff940c1..2145cd9dd4b 100644 --- a/protocols/gossipsub/src/protocol_priv.rs +++ b/protocols/gossipsub/src/protocol_priv.rs @@ -57,7 +57,7 @@ impl ProtocolConfig { /// /// Sets the maximum gossip transmission size. pub fn new(gossipsub_config: &Config) -> ProtocolConfig { - let protocol_ids = match gossipsub_config.custom_id_version() { + let mut protocol_ids = match gossipsub_config.custom_id_version() { Some(v) => match v { Version::V1_0 => vec![ProtocolId::new( gossipsub_config.protocol_id(), @@ -71,24 +71,22 @@ impl ProtocolConfig { )], }, None => { - let mut protocol_ids = vec![ + vec![ ProtocolId::new( gossipsub_config.protocol_id(), PeerKind::Gossipsubv1_1, true, ), ProtocolId::new(gossipsub_config.protocol_id(), PeerKind::Gossipsub, true), - ]; - - // add floodsub support if enabled. - if gossipsub_config.support_floodsub() { - protocol_ids.push(ProtocolId::new("", PeerKind::Floodsub, false)); - } - - protocol_ids + ] } }; + // add floodsub support if enabled. + if gossipsub_config.support_floodsub() { + protocol_ids.push(ProtocolId::new("", PeerKind::Floodsub, false)); + } + ProtocolConfig { protocol_ids, max_transmit_size: gossipsub_config.max_transmit_size(), From 54ee4a7981e74147e0eea10d31bd6749bf14acb3 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Wed, 26 Apr 2023 19:02:33 +0200 Subject: [PATCH 7/8] Add changelog entry --- protocols/gossipsub/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/protocols/gossipsub/CHANGELOG.md b/protocols/gossipsub/CHANGELOG.md index fc63a305923..f2885573bcf 100644 --- a/protocols/gossipsub/CHANGELOG.md +++ b/protocols/gossipsub/CHANGELOG.md @@ -1,8 +1,12 @@ ## 0.44.4 - unreleased - Deprecate `metrics`, `protocol`, `subscription_filter`, `time_cache` modules to make them private. See [PR 3777]. +- Honor the `gossipsub::Config::support_floodsub` in all cases. + Previously, it was ignored when a custom protocol id was set via `gossipsub::Config::protocol_id`. + See [PR XXXX]. [PR 3777]: https://github.com/libp2p/rust-libp2p/pull/3777 +[PR XXXX]: https://github.com/libp2p/rust-libp2p/pull/XXXX ## 0.44.3 From 3856364fb9c7210a01fda32c916e6ee5ec37cffa Mon Sep 17 00:00:00 2001 From: Thomas Eizinger <thomas@eizinger.io> Date: Thu, 27 Apr 2023 03:08:22 +1000 Subject: [PATCH 8/8] Update protocols/gossipsub/CHANGELOG.md --- protocols/gossipsub/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/protocols/gossipsub/CHANGELOG.md b/protocols/gossipsub/CHANGELOG.md index f2885573bcf..8e292b87b05 100644 --- a/protocols/gossipsub/CHANGELOG.md +++ b/protocols/gossipsub/CHANGELOG.md @@ -3,10 +3,10 @@ - Deprecate `metrics`, `protocol`, `subscription_filter`, `time_cache` modules to make them private. See [PR 3777]. - Honor the `gossipsub::Config::support_floodsub` in all cases. Previously, it was ignored when a custom protocol id was set via `gossipsub::Config::protocol_id`. - See [PR XXXX]. + See [PR 3837]. [PR 3777]: https://github.com/libp2p/rust-libp2p/pull/3777 -[PR XXXX]: https://github.com/libp2p/rust-libp2p/pull/XXXX +[PR 3837]: https://github.com/libp2p/rust-libp2p/pull/3837 ## 0.44.3