From 1f2ccaea05c8508f5cc916f8ae0ba60c102c5cf3 Mon Sep 17 00:00:00 2001
From: asynchronous rob <rphmeier@gmail.com>
Date: Thu, 30 Nov 2023 11:55:33 -0600
Subject: [PATCH] Remove dependency on rand's SliceRandom shuffle
implementation in gossip-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.
---
Cargo.lock | 13 +++++++++++
.../node/network/gossip-support/Cargo.toml | 1 +
.../node/network/gossip-support/src/lib.rs | 14 ++++++++++--
.../node/network/gossip-support/src/tests.rs | 22 +++++++++++++++++++
4 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index 05715ad8dfb..c83eeb050df 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4844,6 +4844,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"
@@ -11905,6 +11915,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",
@@ -13731,6 +13742,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 a9f68261add..64a9743d1db 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 674c86e5ce2..0d1b04f2ba2 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::{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
diff --git a/polkadot/node/network/gossip-support/src/tests.rs b/polkadot/node/network/gossip-support/src/tests.rs
index 2e909bb0a67..e5ee101c31d 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 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
+ }
+}
--
GitLab