diff --git a/zebra-network/src/meta_addr.rs b/zebra-network/src/meta_addr.rs index 48d05300..5057f12d 100644 --- a/zebra-network/src/meta_addr.rs +++ b/zebra-network/src/meta_addr.rs @@ -752,21 +752,15 @@ impl MetaAddrChange { /// If this change can create a new `MetaAddr`, return that address. pub fn into_new_meta_addr(self) -> Option { - match self { - NewInitial { .. } | NewGossiped { .. } | NewAlternate { .. } | NewLocal { .. } => { - Some(MetaAddr { - addr: self.addr(), - services: self.untrusted_services(), - untrusted_last_seen: self.untrusted_last_seen(), - last_response: None, - last_attempt: None, - last_failure: None, - last_connection_state: self.peer_addr_state(), - }) - } - - UpdateAttempt { .. } | UpdateResponded { .. } | UpdateFailed { .. } => None, - } + Some(MetaAddr { + addr: self.addr(), + services: self.untrusted_services(), + untrusted_last_seen: self.untrusted_last_seen(), + last_response: self.last_response(), + last_attempt: self.last_attempt(), + last_failure: self.last_failure(), + last_connection_state: self.peer_addr_state(), + }) } /// Apply this change to a previous `MetaAddr` from the address book, diff --git a/zebra-network/src/meta_addr/tests/prop.rs b/zebra-network/src/meta_addr/tests/prop.rs index 731a1234..de2a5c83 100644 --- a/zebra-network/src/meta_addr/tests/prop.rs +++ b/zebra-network/src/meta_addr/tests/prop.rs @@ -1,4 +1,4 @@ -//! Randomised property tests for MetaAddr. +//! Randomised property tests for MetaAddr and MetaAddrChange. use std::{ collections::HashMap, convert::TryFrom, env, net::SocketAddr, str::FromStr, sync::Arc, @@ -174,10 +174,107 @@ proptest! { canonical_local_listener, ); } + + /// Make sure that [`MetaAddrChange`]s are correctly applied + /// when there is no [`MetaAddr`] in the address book. + /// + /// TODO: Make sure that [`MetaAddrChange`]s are correctly applied, + /// regardless of the [`MetaAddr`] that is currently in the address book. + #[test] + fn new_meta_addr_from_meta_addr_change( + (addr, changes) in MetaAddrChange::addr_changes_strategy(MAX_ADDR_CHANGE) + ) { + zebra_test::init(); + + let local_listener = "0.0.0.0:0".parse().expect("unexpected invalid SocketAddr"); + + for change in changes { + // Check direct application + let new_addr = change.apply_to_meta_addr(None); + + prop_assert!( + new_addr.is_some(), + "applying a change to `None` should always result in a new address,\n \ + change: {:?}", + change, + ); + + let new_addr = new_addr.expect("just checked is_some"); + prop_assert_eq!(new_addr.addr, addr.addr); + + // Check address book update - return value + let mut address_book = AddressBook::new_with_addrs( + local_listener, + 1, + Span::none(), + Vec::new(), + ); + + let expected_result = new_addr; + let book_result = address_book.update(change); + let book_contents: Vec = address_book.peers().collect(); + + // Ignore the same addresses that the address book ignores + let expected_result = if !expected_result.address_is_valid_for_outbound() + || ( !expected_result.last_known_info_is_valid_for_outbound() + && expected_result.last_connection_state.is_never_attempted()) + { + None + } else { + Some(expected_result) + }; + + prop_assert_eq!( + book_result.is_some(), + expected_result.is_some(), + "applying a change to an empty address book should return a new address,\n \ + unless its info is invalid,\n \ + change: {:?},\n \ + address book returned: {:?},\n \ + expected result: {:?}", + change, + book_result, + expected_result, + ); + + if let Some(book_result) = book_result { + prop_assert_eq!(book_result.addr, addr.addr); + // TODO: pass times to MetaAddrChange::apply_to_meta_addr and AddressBook::update, + // so the times are equal + // prop_assert_eq!(new_addr, book_result); + } + + // Check address book update - address book contents + prop_assert_eq!( + !book_contents.is_empty(), expected_result.is_some(), + "applying a change to an empty address book should add a new address,\n \ + unless its info is invalid,\n \ + change: {:?},\n \ + address book contains: {:?},\n \ + expected result: {:?}", + change, + book_contents, + expected_result, + ); + + if let Some(book_contents) = book_contents.first() { + prop_assert_eq!(book_contents.addr, addr.addr); + // TODO: pass times to MetaAddrChange::apply_to_meta_addr and AddressBook::update, + // so the times are equal + //prop_assert_eq!(new_addr, *book_contents); + } + + prop_assert_eq!(book_result.as_ref(), book_contents.first()); + + // TODO: do we need to check each field is calculated correctly as well? + } + } } proptest! { - // These tests can produce a lot of debug output, so we use a smaller number of cases by default. + // These tests can produce a lot of debug output, + // so we let developers configure them with a smaller number of cases. + // // Set the PROPTEST_CASES env var to override this default. #![proptest_config(proptest::test_runner::Config::with_cases(env::var("PROPTEST_CASES") .ok() diff --git a/zebra-network/src/meta_addr/tests/vectors.rs b/zebra-network/src/meta_addr/tests/vectors.rs index 59630d17..54de4a21 100644 --- a/zebra-network/src/meta_addr/tests/vectors.rs +++ b/zebra-network/src/meta_addr/tests/vectors.rs @@ -1,4 +1,4 @@ -//! Test vectors for MetaAddr. +//! Fixed test cases for MetaAddr and MetaAddrChange. use std::net::SocketAddr; diff --git a/zebra-network/src/peer_set/initialize.rs b/zebra-network/src/peer_set/initialize.rs index 7406280e..82d42cbd 100644 --- a/zebra-network/src/peer_set/initialize.rs +++ b/zebra-network/src/peer_set/initialize.rs @@ -237,7 +237,7 @@ where /// Use the provided `outbound_connector` to connect to the configured initial peers, /// then send the resulting peer connections over `peerset_tx`. /// -/// Sends any unused initial peers to the `address_book_updater`. +/// Also sends every initial peer address to the `address_book_updater`. #[instrument(skip(config, outbound_connector, peerset_tx, address_book_updater))] async fn add_initial_peers( config: Config, @@ -375,9 +375,10 @@ where /// Limit the number of `initial_peers` addresses entries to the configured /// `peerset_initial_target_size`. /// -/// Returns randomly chosen entries from the provided set of addresses. +/// Returns randomly chosen entries from the provided set of addresses, +/// in a random order. /// -/// Sends any unused entries to the `address_book_updater`. +/// Also sends every initial peer to the `address_book_updater`. async fn limit_initial_peers( config: &Config, address_book_updater: tokio::sync::mpsc::Sender, @@ -385,27 +386,29 @@ async fn limit_initial_peers( let all_peers = config.initial_peers().await; let peers_count = all_peers.len(); - if peers_count <= config.peerset_initial_target_size { - return all_peers; + // # Correctness + // + // We can't exit early if we only have a few peers, + // because we still need to shuffle the connection order. + + if all_peers.len() > config.peerset_initial_target_size { + info!( + "limiting the initial peers list from {} to {}", + peers_count, config.peerset_initial_target_size + ); } - // Limit the number of initial peers to `config.peerset_initial_target_size` - info!( - "Limiting the initial peers list from {} to {}", - peers_count, config.peerset_initial_target_size - ); + // Split out the `initial_peers` that will be shuffled and returned. + let mut initial_peers: Vec = all_peers.iter().cloned().collect(); + let (initial_peers, _unused_peers) = + initial_peers.partial_shuffle(&mut rand::thread_rng(), config.peerset_initial_target_size); - // Split all the peers into the `initial_peers` that will be returned and - // `unused_peers` that will be sent to the address book. - let mut all_peers: Vec = all_peers.into_iter().collect(); - let (initial_peers, unused_peers) = - all_peers.partial_shuffle(&mut rand::thread_rng(), config.peerset_initial_target_size); - - // Send the unused peers to the address book. - for peer in unused_peers { - let peer_addr = MetaAddr::new_initial_peer(*peer); + // Send every initial peer to the address book. + // (This treats initial peers the same way we treat gossiped peers.) + for peer in all_peers { + let peer_addr = MetaAddr::new_initial_peer(peer); // `send` only waits when the channel is full. - // The address book updater is a separate task, so we will only wait for a short time. + // The address book updater runs in its own thread, so we will only wait for a short time. let _ = address_book_updater.send(peer_addr).await; }