From ef1bbc794a8c8c16a71f1c8d70ffeb1751103828 Mon Sep 17 00:00:00 2001 From: Mykhailo Kremniov Date: Fri, 29 Sep 2023 23:29:58 +0300 Subject: [PATCH 1/2] exponential_rand can no longer produce infinity; next_connect_time's random delay is now clamped --- Cargo.lock | 2 ++ p2p/Cargo.toml | 1 + .../peer_manager/peerdb/address_data/mod.rs | 25 +++++++-------- .../peer_manager/peerdb/address_data/tests.rs | 32 +++++++++++++++++++ utils/Cargo.toml | 1 + utils/src/exp_rand/mod.rs | 9 +++++- utils/src/exp_rand/test.rs | 14 +++++++- 7 files changed, 68 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 053f5cdcd..4d9976cd4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4295,6 +4295,7 @@ dependencies = [ "p2p-types", "parity-scale-codec", "portpicker", + "rand 0.8.5", "rpc", "rstest", "serde", @@ -7024,6 +7025,7 @@ dependencies = [ "parity-scale-codec", "probabilistic-collections", "qrcodegen", + "rand 0.8.5", "rstest", "serialization", "slave-pool", diff --git a/p2p/Cargo.toml b/p2p/Cargo.toml index 8f9f9d9cf..1e0fbdd81 100644 --- a/p2p/Cargo.toml +++ b/p2p/Cargo.toml @@ -60,6 +60,7 @@ test-utils = { path = "../test-utils" } criterion.workspace = true ctor.workspace = true portpicker.workspace = true +rand.workspace = true rstest.workspace = true [[test]] diff --git a/p2p/src/peer_manager/peerdb/address_data/mod.rs b/p2p/src/peer_manager/peerdb/address_data/mod.rs index 5621f9cef..d7c74bbc5 100644 --- a/p2p/src/peer_manager/peerdb/address_data/mod.rs +++ b/p2p/src/peer_manager/peerdb/address_data/mod.rs @@ -34,6 +34,14 @@ const PURGE_REACHABLE_TIME: Duration = Duration::from_secs(3600 * 24 * 7 * 4); const PURGE_REACHABLE_FAIL_COUNT: u32 = (PURGE_REACHABLE_TIME.as_secs() / MAX_DELAY_REACHABLE.as_secs()) as u32; +/// The maximum value for the random factor by which reconnection delays will be multiplied. +/// +/// Note that the value was chosen based on bitcoin's implementation of GetExponentialRand +/// (https://github.com/bitcoin/bitcoin/blob/5bbf735defac20f58133bea95226e13a5d8209bc/src/random.cpp#L689) +/// which they use to scale delays. In their implementation, the maximum scale factor will be +/// -ln(0.0000000000000035527136788) which is about 33. +const MAX_DELAY_FACTOR: u32 = 30; + pub enum AddressState { Connected {}, @@ -164,20 +172,9 @@ impl AddressData { } fn next_connect_time(now: Time, fail_count: u32, reserved: bool, rng: &mut impl Rng) -> Time { - let random_offset = Self::next_connect_delay(fail_count, reserved) - .mul_f64(utils::exp_rand::exponential_rand(rng)); - loop { - let res = now + random_offset; - match res { - Some(r) => return r, - None => { - // The random offset is too large, try again - logging::log::debug!( - "next_connect_time: Random_offset too large (now: {now:?}, offset: {random_offset:?}), trying again" - ); - } - } - } + let factor = utils::exp_rand::exponential_rand(rng).clamp(0.0, MAX_DELAY_FACTOR as f64); + let offset = Self::next_connect_delay(fail_count, reserved).mul_f64(factor); + (now + offset).expect("Unexpected time addition overflow") } pub fn transition_to( diff --git a/p2p/src/peer_manager/peerdb/address_data/tests.rs b/p2p/src/peer_manager/peerdb/address_data/tests.rs index 7cd815bf9..7c22ac5ef 100644 --- a/p2p/src/peer_manager/peerdb/address_data/tests.rs +++ b/p2p/src/peer_manager/peerdb/address_data/tests.rs @@ -13,6 +13,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use rand::rngs::mock::StepRng; + use crypto::random::{ distributions::{Distribution, WeightedIndex}, Rng, @@ -89,3 +91,33 @@ fn reachable_reconnects(#[case] seed: Seed) { "invalid time until removed: {time_until_removed:?}" ); } + +fn next_connect_time_test_impl(rng: &mut impl Rng) { + let limit_reserved = MAX_DELAY_RESERVED * MAX_DELAY_FACTOR; + let limit_reachable = MAX_DELAY_REACHABLE * MAX_DELAY_FACTOR; + + let start_time = Time::from_secs_since_epoch(0); + let max_time_reserved = (start_time + limit_reserved).unwrap(); + let max_time_reachable = (start_time + limit_reachable).unwrap(); + + let time = AddressData::next_connect_time(start_time, 0, true, rng); + assert!(time <= max_time_reserved); + + let time = AddressData::next_connect_time(start_time, 0, false, rng); + assert!(time <= max_time_reachable); + + let time = AddressData::next_connect_time(start_time, u32::MAX, true, rng); + assert!(time <= max_time_reserved); + + let time = AddressData::next_connect_time(start_time, u32::MAX, false, rng); + assert!(time <= max_time_reachable); +} + +#[test] +fn next_connect_time() { + let mut always_zero_rng = StepRng::new(0, 0); + next_connect_time_test_impl(&mut always_zero_rng); + + let mut always_max_rng = StepRng::new(u64::MAX, 0); + next_connect_time_test_impl(&mut always_max_rng); +} diff --git a/utils/Cargo.toml b/utils/Cargo.toml index 4c785cd3c..40baa3970 100644 --- a/utils/Cargo.toml +++ b/utils/Cargo.toml @@ -23,6 +23,7 @@ zeroize.workspace = true test-utils = { path = "../test-utils" } criterion.workspace = true +rand.workspace = true rstest.workspace = true static_assertions.workspace = true diff --git a/utils/src/exp_rand/mod.rs b/utils/src/exp_rand/mod.rs index 1736409ec..f65232588 100644 --- a/utils/src/exp_rand/mod.rs +++ b/utils/src/exp_rand/mod.rs @@ -17,8 +17,15 @@ use crypto::random::Rng; /// Returns a value sampled from an exponential distribution with a mean of 1.0 pub fn exponential_rand(rng: &mut impl Rng) -> f64 { + let mut random_f64 = rng.gen::(); + // The generated number will be in the range [0, 1). Turn it into (0, 1) to avoid + // infinity when taking the logarithm. + if random_f64 == 0.0 { + random_f64 = f64::MIN_POSITIVE; + } + #[allow(clippy::float_arithmetic)] - -rng.gen::().ln() + -random_f64.ln() } #[cfg(test)] diff --git a/utils/src/exp_rand/test.rs b/utils/src/exp_rand/test.rs index 5bbb78fa7..ffb2221e1 100644 --- a/utils/src/exp_rand/test.rs +++ b/utils/src/exp_rand/test.rs @@ -15,13 +15,14 @@ use super::*; +use rand::rngs::mock::StepRng; use rstest::rstest; use test_utils::random::{make_seedable_rng, Seed}; #[rstest] #[trace] #[case(Seed::from_entropy())] -fn test_exponential_rand(#[case] seed: Seed) { +fn test_average_value(#[case] seed: Seed) { let mut rng = make_seedable_rng(seed); let count = 1000; @@ -29,3 +30,14 @@ fn test_exponential_rand(#[case] seed: Seed) { let average = sum / count as f64; assert!(0.8 < average && average < 1.2); } + +#[test] +fn expect_finite_values_in_degenerate_cases() { + let mut always_zero_rng = StepRng::new(0, 0); + let val = exponential_rand(&mut always_zero_rng); + assert!(val.is_finite()); + + let mut always_max_rng = StepRng::new(u64::MAX, 0); + let val = exponential_rand(&mut always_max_rng); + assert!(val.is_finite()); +} From c1569e0f9645b0743f54faadd00e9bdd1e200d8d Mon Sep 17 00:00:00 2001 From: Mykhailo Kremniov Date: Mon, 2 Oct 2023 14:19:49 +0300 Subject: [PATCH 2/2] Expose StepRng through "crypto" instead of using "rand" directly. --- Cargo.lock | 2 -- crypto/src/random/mod.rs | 1 + p2p/Cargo.toml | 1 - p2p/src/peer_manager/peerdb/address_data/tests.rs | 3 +-- utils/Cargo.toml | 1 - utils/src/exp_rand/test.rs | 2 +- 6 files changed, 3 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4d9976cd4..053f5cdcd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4295,7 +4295,6 @@ dependencies = [ "p2p-types", "parity-scale-codec", "portpicker", - "rand 0.8.5", "rpc", "rstest", "serde", @@ -7025,7 +7024,6 @@ dependencies = [ "parity-scale-codec", "probabilistic-collections", "qrcodegen", - "rand 0.8.5", "rstest", "serialization", "slave-pool", diff --git a/crypto/src/random/mod.rs b/crypto/src/random/mod.rs index d123c1053..c84df3c7b 100644 --- a/crypto/src/random/mod.rs +++ b/crypto/src/random/mod.rs @@ -26,6 +26,7 @@ pub mod distributions { } pub mod rngs { + pub use rand::rngs::mock::StepRng; pub use rand::rngs::OsRng; } diff --git a/p2p/Cargo.toml b/p2p/Cargo.toml index 1e0fbdd81..8f9f9d9cf 100644 --- a/p2p/Cargo.toml +++ b/p2p/Cargo.toml @@ -60,7 +60,6 @@ test-utils = { path = "../test-utils" } criterion.workspace = true ctor.workspace = true portpicker.workspace = true -rand.workspace = true rstest.workspace = true [[test]] diff --git a/p2p/src/peer_manager/peerdb/address_data/tests.rs b/p2p/src/peer_manager/peerdb/address_data/tests.rs index 7c22ac5ef..0c70ef531 100644 --- a/p2p/src/peer_manager/peerdb/address_data/tests.rs +++ b/p2p/src/peer_manager/peerdb/address_data/tests.rs @@ -13,10 +13,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use rand::rngs::mock::StepRng; - use crypto::random::{ distributions::{Distribution, WeightedIndex}, + rngs::StepRng, Rng, }; use rstest::rstest; diff --git a/utils/Cargo.toml b/utils/Cargo.toml index 40baa3970..4c785cd3c 100644 --- a/utils/Cargo.toml +++ b/utils/Cargo.toml @@ -23,7 +23,6 @@ zeroize.workspace = true test-utils = { path = "../test-utils" } criterion.workspace = true -rand.workspace = true rstest.workspace = true static_assertions.workspace = true diff --git a/utils/src/exp_rand/test.rs b/utils/src/exp_rand/test.rs index ffb2221e1..857b2634f 100644 --- a/utils/src/exp_rand/test.rs +++ b/utils/src/exp_rand/test.rs @@ -15,7 +15,7 @@ use super::*; -use rand::rngs::mock::StepRng; +use crypto::random::rngs::StepRng; use rstest::rstest; use test_utils::random::{make_seedable_rng, Seed};