Fix next_connect_time

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;
 }
 

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(

p2p/src/peer_manager/peerdb/address_data/tests.rs

@@ -15,6 +15,7 @@
 
 use crypto::random::{
     distributions::{Distribution, WeightedIndex},
+    rngs::StepRng,
     Rng,
 };
 use rstest::rstest;
@@ -89,3 +90,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);
+}

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::<f64>();
+    // 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::<f64>().ln()
+    -random_f64.ln()
 }
 
 #[cfg(test)]

utils/src/exp_rand/test.rs

@@ -15,17 +15,29 @@
 
 use super::*;
 
+use crypto::random::rngs::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;
     let sum: f64 = (0..count).map(|_| exponential_rand(&mut rng)).sum();
     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());
+}