From 2a7b90ec215816a6e2f47226a8a9d58fdec24fec Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Nov 2023 20:06:08 -0600 Subject: [PATCH 1/3] limit dependency on rand's SliceRandom shuffle implementation --- Cargo.lock | 13 ++++++++++++ .../node/network/gossip-support/Cargo.toml | 1 + .../node/network/gossip-support/src/lib.rs | 17 ++++++++++++++-- .../node/network/gossip-support/src/tests.rs | 20 +++++++++++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 346ce2f933f1..4eaf92f83c55 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4789,6 +4789,16 @@ dependencies = [ "syn 2.0.38", ] +[[package]] +name = "env_logger" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a19187fea3ac7e84da7dacf48de0c45d63c6a76f9490dae389aead16c243fce3" +dependencies = [ + "log", + "regex", +] + [[package]] name = "env_logger" version = "0.9.3" @@ -11850,6 +11860,7 @@ dependencies = [ "polkadot-node-subsystem-test-helpers", "polkadot-node-subsystem-util", "polkadot-primitives", + "quickcheck", "rand 0.8.5", "rand_chacha 0.3.1", "sc-network", @@ -13676,6 +13687,8 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "588f6378e4dd99458b60ec275b4477add41ce4fa9f64dcba6f15adccb19b50d6" dependencies = [ + "env_logger 0.8.4", + "log", "rand 0.8.5", ] diff --git a/polkadot/node/network/gossip-support/Cargo.toml b/polkadot/node/network/gossip-support/Cargo.toml index a9f68261addf..64a9743d1db2 100644 --- a/polkadot/node/network/gossip-support/Cargo.toml +++ b/polkadot/node/network/gossip-support/Cargo.toml @@ -35,3 +35,4 @@ polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } assert_matches = "1.4.0" async-trait = "0.1.57" lazy_static = "1.4.0" +quickcheck = "1.0.3" diff --git a/polkadot/node/network/gossip-support/src/lib.rs b/polkadot/node/network/gossip-support/src/lib.rs index 674c86e5ce27..a64b7f7bb853 100644 --- a/polkadot/node/network/gossip-support/src/lib.rs +++ b/polkadot/node/network/gossip-support/src/lib.rs @@ -32,7 +32,7 @@ use std::{ use futures::{channel::oneshot, select, FutureExt as _}; use futures_timer::Delay; -use rand::{seq::SliceRandom as _, SeedableRng}; +use rand::{SeedableRng, Rng}; use rand_chacha::ChaCha20Rng; use sc_network::{config::parse_addr, Multiaddr}; @@ -607,7 +607,7 @@ async fn update_gossip_topology( .map(|(i, a)| (a.clone(), ValidatorIndex(i as _))) .collect(); - canonical_shuffling.shuffle(&mut rng); + fisher_yates_shuffle(&mut rng, &mut canonical_shuffling[..]); for (i, (_, validator_index)) in canonical_shuffling.iter().enumerate() { shuffled_indices[validator_index.0 as usize] = i; } @@ -627,6 +627,19 @@ async fn update_gossip_topology( Ok(()) } +// Durstenfeld algorithm for the Fisher-Yates shuffle +// https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle#The_modern_algorithm +fn fisher_yates_shuffle( + rng: &mut R, + items: &mut [T], +) { + for i in (1..items.len()).rev() { + // invariant: elements with index > i have been locked in place. + let index = rng.gen_range(0u32..(i as u32 + 1)); + items.swap(i, index as usize); + } +} + #[overseer::subsystem(GossipSupport, error = SubsystemError, prefix = self::overseer)] impl GossipSupport where diff --git a/polkadot/node/network/gossip-support/src/tests.rs b/polkadot/node/network/gossip-support/src/tests.rs index 2e909bb0a674..e7c87343b222 100644 --- a/polkadot/node/network/gossip-support/src/tests.rs +++ b/polkadot/node/network/gossip-support/src/tests.rs @@ -22,6 +22,8 @@ use assert_matches::assert_matches; use async_trait::async_trait; use futures::{executor, future, Future}; use lazy_static::lazy_static; +use rand::seq::SliceRandom as _; +use quickcheck::quickcheck; use sc_network::multiaddr::Protocol; use sp_authority_discovery::AuthorityPair as AuthorityDiscoveryPair; @@ -710,3 +712,21 @@ fn issues_a_connection_request_when_last_request_was_mostly_unresolved() { assert_eq!(state.last_session_index, Some(1)); assert!(state.last_failure.is_none()); } + +// note: this test was added at a time where the default `rand::SliceRandom::shuffle` +// function was used to shuffle authorities for the topology and ensures backwards compatibility. +// +// in the same commit, an explicit fisher-yates implementation was added in place of the unspecified +// behavior of that function. If this test begins to fail at some point in the future, it can simply +// be removed as the desired behavior has been preserved. +quickcheck! { + fn rng_shuffle_equals_fisher_yates(x: Vec, seed_base: u8) -> bool { + let mut rng1: ChaCha20Rng = SeedableRng::from_seed([seed_base; 32]); + let mut rng2: ChaCha20Rng = SeedableRng::from_seed([seed_base; 32]); + + let mut data1 = x.clone(); + let mut data2 = x; + + data1.shuffle(&mut rng1) == crate::fisher_yates_shuffle(&mut rng2, &mut data2[..]) + } +} From 5b361050febf7e29cf588f8d746324499fb52f5a Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Nov 2023 20:13:47 -0600 Subject: [PATCH 2/3] fmt --- polkadot/node/network/gossip-support/src/lib.rs | 7 ++----- polkadot/node/network/gossip-support/src/tests.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/polkadot/node/network/gossip-support/src/lib.rs b/polkadot/node/network/gossip-support/src/lib.rs index a64b7f7bb853..0d1b04f2ba23 100644 --- a/polkadot/node/network/gossip-support/src/lib.rs +++ b/polkadot/node/network/gossip-support/src/lib.rs @@ -32,7 +32,7 @@ use std::{ use futures::{channel::oneshot, select, FutureExt as _}; use futures_timer::Delay; -use rand::{SeedableRng, Rng}; +use rand::{Rng, SeedableRng}; use rand_chacha::ChaCha20Rng; use sc_network::{config::parse_addr, Multiaddr}; @@ -629,10 +629,7 @@ async fn update_gossip_topology( // Durstenfeld algorithm for the Fisher-Yates shuffle // https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle#The_modern_algorithm -fn fisher_yates_shuffle( - rng: &mut R, - items: &mut [T], -) { +fn fisher_yates_shuffle(rng: &mut R, items: &mut [T]) { for i in (1..items.len()).rev() { // invariant: elements with index > i have been locked in place. let index = rng.gen_range(0u32..(i as u32 + 1)); diff --git a/polkadot/node/network/gossip-support/src/tests.rs b/polkadot/node/network/gossip-support/src/tests.rs index e7c87343b222..db341fbd9bc5 100644 --- a/polkadot/node/network/gossip-support/src/tests.rs +++ b/polkadot/node/network/gossip-support/src/tests.rs @@ -22,8 +22,8 @@ use assert_matches::assert_matches; use async_trait::async_trait; use futures::{executor, future, Future}; use lazy_static::lazy_static; -use rand::seq::SliceRandom as _; use quickcheck::quickcheck; +use rand::seq::SliceRandom as _; use sc_network::multiaddr::Protocol; use sp_authority_discovery::AuthorityPair as AuthorityDiscoveryPair; From 3a820fb9cb8cc484978ab30cd26bd9d7db732680 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Nov 2023 21:02:39 -0600 Subject: [PATCH 3/3] fix test --- polkadot/node/network/gossip-support/src/tests.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/polkadot/node/network/gossip-support/src/tests.rs b/polkadot/node/network/gossip-support/src/tests.rs index db341fbd9bc5..e5ee101c31d8 100644 --- a/polkadot/node/network/gossip-support/src/tests.rs +++ b/polkadot/node/network/gossip-support/src/tests.rs @@ -727,6 +727,8 @@ quickcheck! { let mut data1 = x.clone(); let mut data2 = x; - data1.shuffle(&mut rng1) == crate::fisher_yates_shuffle(&mut rng2, &mut data2[..]) + data1.shuffle(&mut rng1); + crate::fisher_yates_shuffle(&mut rng2, &mut data2[..]); + data1 == data2 } }