Stop ignoring some peers when updating the address book (#3292)

* Make sure MetaAddrChanges are correctly applied to an empty address book

* Add all initial peers to the address book

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<MetaAddr> {
-        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,

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<MetaAddr> = 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()

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;
 

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<S>(
     config: Config,
@@ -375,37 +375,40 @@ 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<MetaAddrChange>,
 ) -> HashSet<SocketAddr> {
     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.
 
-    // 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
-    );
+    if all_peers.len() > config.peerset_initial_target_size {
+        info!(
+            "limiting the initial peers list from {} to {}",
+            peers_count, 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<SocketAddr> = all_peers.into_iter().collect();
-    let (initial_peers, unused_peers) =
-        all_peers.partial_shuffle(&mut rand::thread_rng(), config.peerset_initial_target_size);
+    // Split out the `initial_peers` that will be shuffled and returned.
+    let mut initial_peers: Vec<SocketAddr> = all_peers.iter().cloned().collect();
+    let (initial_peers, _unused_peers) =
+        initial_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;
     }