Remove dependency on rand's SliceRandom shuffle implementation in gos…

…sip-support (#2555)

In gossip-support, we shuffled the list of authorities using the
`rand::seq::SliceRandom::shuffle`. This function's behavior is
unspecified beyond being `O(n)` and could change in the future, leading
to network issues between nodes using different shuffling algorithms. In
practice, the implementation was a Fisher-Yates shuffle. This PR
replaces the call with a re-implementation of Fisher-Yates and adds a
test to ensure the behavior is the same between the two at the moment.

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"

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::{Rng, SeedableRng};
 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,16 @@ 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<T, R: Rng + ?Sized>(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<Context, AD> GossipSupport<AD>
 where

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 quickcheck::quickcheck;
+use rand::seq::SliceRandom as _;
 
 use sc_network::multiaddr::Protocol;
 use sp_authority_discovery::AuthorityPair as AuthorityDiscoveryPair;
@@ -710,3 +712,23 @@ 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<i32>, 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[..]);
+		data1 == data2
+	}
+}