diff --git a/protocols/gossipsub/CHANGELOG.md b/protocols/gossipsub/CHANGELOG.md index 70f99165e18..021b74b4f62 100644 --- a/protocols/gossipsub/CHANGELOG.md +++ b/protocols/gossipsub/CHANGELOG.md @@ -1,4 +1,8 @@ ## 0.51.0 +- Validate the default mesh parameters, the default `max_transmit_size` and all topic mesh + configurations in `ConfigBuilder::build`, not only topics with a custom max transmit size. + See [PR 6648](https://github.com/libp2p/rust-libp2p/pull/6648). + - Look up the ids of a received IDONTWANT in a `HashSet` when removing them from the peer's send queue, so the cost is O(queue + ids) instead of O(queue * ids). diff --git a/protocols/gossipsub/src/behaviour/tests/topic_config.rs b/protocols/gossipsub/src/behaviour/tests/topic_config.rs index b36e2f8bbad..efcdfbc8407 100644 --- a/protocols/gossipsub/src/behaviour/tests/topic_config.rs +++ b/protocols/gossipsub/src/behaviour/tests/topic_config.rs @@ -250,21 +250,22 @@ fn test_mesh_subtraction_with_topic_config() { ); } -/// Tests that if a mesh reaches `mesh_n_high`, -/// but is only composed of outbound peers, it is not reduced to `mesh_n`. +/// Tests that if a mesh reaches `mesh_n_high`, it is reduced to `mesh_n` without removing +/// outbound peers below the topic-specific `mesh_outbound_min`. #[test] fn test_mesh_subtraction_with_topic_config_min_outbound() { let topic = String::from("topic1"); let topic_hash = TopicHash::from_raw(topic.clone()); - let mesh_n = 5; - let mesh_n_high = 7; + let mesh_n = 6; + let mesh_n_high = 8; + let mesh_outbound_min = 3; let topic_config = TopicMeshConfig { mesh_n, mesh_n_high, - mesh_n_low: 3, - mesh_outbound_min: 7, + mesh_n_low: 4, + mesh_outbound_min, }; let config = ConfigBuilder::default() @@ -272,36 +273,41 @@ fn test_mesh_subtraction_with_topic_config_min_outbound() { .build() .unwrap(); - let peer_no = 12; - - // make all outbound connections. + // The first `mesh_outbound_min` peers are outbound connections. let (mut gs, peers, _, topics) = DefaultBehaviourTestBuilder::default() - .peer_no(peer_no) + .peer_no(mesh_n_high) .topics(vec![topic]) .to_subscribe(true) .gs_config(config.clone()) - .outbound(peer_no) + .outbound(mesh_outbound_min) .create_network(); // graft all peers - for peer in peers { - gs.handle_graft(&peer, topics.clone()); + for peer in &peers { + gs.handle_graft(peer, topics.clone()); } assert_eq!( gs.mesh.get(&topics[0]).unwrap().len(), - peer_no, - "Initially mesh should contain all {peer_no} outbound peers" + mesh_n_high, + "Initially mesh should be {mesh_n_high}" ); // run a heartbeat gs.heartbeat(); + let mesh = gs.mesh.get(&topics[0]).unwrap(); assert_eq!( - gs.mesh.get(&topics[0]).unwrap().len(), - mesh_n_high, - "After heartbeat, mesh should still be {mesh_n_high} as these are all outbound peers" + mesh.len(), + mesh_n, + "After heartbeat, mesh should be reduced to {mesh_n}" ); + for outbound_peer in peers.iter().take(mesh_outbound_min) { + assert!( + mesh.contains(outbound_peer), + "Outbound peers must be kept to satisfy mesh_outbound_min" + ); + } } /// Test behavior with multiple topics having different configs diff --git a/protocols/gossipsub/src/config.rs b/protocols/gossipsub/src/config.rs index 84a915eb372..866478dcf24 100644 --- a/protocols/gossipsub/src/config.rs +++ b/protocols/gossipsub/src/config.rs @@ -1111,22 +1111,31 @@ impl ConfigBuilder { pub fn build(&self) -> Result { // check all constraints on config - let pre_configured_topics = self.config.protocol.max_transmit_sizes.keys(); - for topic in pre_configured_topics { - if self.config.protocol.max_transmit_size_for_topic(topic) < 100 { - return Err(ConfigBuilderError::MaxTransmissionSizeTooSmall); - } - - let mesh_n = self.config.mesh_n_for_topic(topic); - let mesh_n_low = self.config.mesh_n_low_for_topic(topic); - let mesh_n_high = self.config.mesh_n_high_for_topic(topic); - let mesh_outbound_min = self.config.mesh_outbound_min_for_topic(topic); + if self.config.protocol.default_max_transmit_size < 100 + || self + .config + .protocol + .max_transmit_sizes + .values() + .any(|size| *size < 100) + { + return Err(ConfigBuilderError::MaxTransmissionSizeTooSmall); + } + let topic_configuration = &self.config.topic_configuration; + for TopicMeshConfig { + mesh_n, + mesh_n_low, + mesh_n_high, + mesh_outbound_min, + } in std::iter::once(&topic_configuration.default_mesh_params) + .chain(topic_configuration.topic_mesh_params.values()) + { if !(mesh_outbound_min <= mesh_n_low && mesh_n_low <= mesh_n && mesh_n <= mesh_n_high) { return Err(ConfigBuilderError::MeshParametersInvalid); } - if mesh_outbound_min * 2 > mesh_n { + if mesh_outbound_min * 2 > *mesh_n { return Err(ConfigBuilderError::MeshOutboundInvalid); } } @@ -1218,6 +1227,53 @@ mod test { use super::*; use crate::{Topic, topic::IdentityHash}; + #[test] + fn build_rejects_invalid_default_mesh_params() { + // mesh_n > mesh_n_high + assert!(matches!( + ConfigBuilder::default().mesh_n(20).build(), + Err(ConfigBuilderError::MeshParametersInvalid) + )); + // mesh_outbound_min > mesh_n / 2 + assert!(matches!( + ConfigBuilder::default().mesh_outbound_min(4).build(), + Err(ConfigBuilderError::MeshOutboundInvalid) + )); + } + + #[test] + fn build_rejects_invalid_topic_mesh_params() { + let topic = TopicHash::from_raw("topic"); + assert!(matches!( + ConfigBuilder::default() + .set_topic_config( + topic.clone(), + TopicMeshConfig { + mesh_n: 6, + mesh_n_low: 8, + mesh_n_high: 12, + mesh_outbound_min: 2, + }, + ) + .build(), + Err(ConfigBuilderError::MeshParametersInvalid) + )); + assert!(matches!( + ConfigBuilder::default() + .mesh_n_high_for_topic(4, topic) + .build(), + Err(ConfigBuilderError::MeshParametersInvalid) + )); + } + + #[test] + fn build_rejects_too_small_default_max_transmit_size() { + assert!(matches!( + ConfigBuilder::default().max_transmit_size(10).build(), + Err(ConfigBuilderError::MaxTransmissionSizeTooSmall) + )); + } + #[test] fn create_config_with_message_id_as_plain_function() { let config = ConfigBuilder::default()