fix(network): Ignore out of order Address Book changes, unless they a…

…re concurrent (#6717)

* Ignore out of order Address Book changes, and restructure the function

* Handle concurrent changes using the connection state machine order

* Handle out of order changes correctly

* Pass times through the call stack so they are consistent in tests

* Add time arguments to tests

* Fix tests that were broken by the address order checks

* fastmod wall_ local_ zebra*

* cargo fmt --all

* Fix a bug in the concurrent change check

* Test all the new apply and skip checks for address changes

* Document more edge cases and increase the concurrency time slightly

* Simplify enum ordering matches

* Fix comment typos

Co-authored-by: Arya <[email protected]>

---------

Co-authored-by: Arya <[email protected]>

zebra-network/src/address_book.rs

@@ -14,7 +14,7 @@ use ordered_map::OrderedMap;
 use tokio::sync::watch;
 use tracing::Span;
 
-use zebra_chain::parameters::Network;
+use zebra_chain::{parameters::Network, serialization::DateTime32};
 
 use crate::{
     constants,
@@ -228,10 +228,11 @@ impl AddressBook {
     /// Get the local listener address.
     ///
     /// This address contains minimal state, but it is not sanitized.
-    pub fn local_listener_meta_addr(&self) -> MetaAddr {
+    pub fn local_listener_meta_addr(&self, now: chrono::DateTime<Utc>) -> MetaAddr {
+        let now: DateTime32 = now.try_into().expect("will succeed until 2038");
+
         MetaAddr::new_local_listener_change(self.local_listener)
-            .into_new_meta_addr()
-            .expect("unexpected invalid new local listener addr")
+            .local_listener_into_new_meta_addr(now)
     }
 
     /// Get the local listener [`SocketAddr`].
@@ -249,7 +250,7 @@ impl AddressBook {
         // Unconditionally add our local listener address to the advertised peers,
         // to replace any self-connection failures. The address book and change
         // constructors make sure that the SocketAddr is canonical.
-        let local_listener = self.local_listener_meta_addr();
+        let local_listener = self.local_listener_meta_addr(now);
         peers.insert(local_listener.addr, local_listener);
 
         // Then sanitize and shuffle
@@ -313,7 +314,7 @@ impl AddressBook {
         let instant_now = Instant::now();
         let chrono_now = Utc::now();
 
-        let updated = change.apply_to_meta_addr(previous);
+        let updated = change.apply_to_meta_addr(previous, instant_now, chrono_now);
 
         trace!(
             ?change,

zebra-network/src/constants.rs

@@ -90,6 +90,24 @@ pub const REQUEST_TIMEOUT: Duration = Duration::from_secs(20);
 /// nodes, and on testnet.
 pub const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(3);
 
+/// The maximum time difference for two address book changes to be considered concurrent.
+///
+/// This prevents simultaneous or nearby important changes or connection progress
+/// being overridden by less important changes.
+///
+/// This timeout should be less than:
+/// - the [peer reconnection delay](MIN_PEER_RECONNECTION_DELAY), and
+/// - the [peer keepalive/heartbeat interval](HEARTBEAT_INTERVAL).
+///
+/// But more than:
+/// - the amount of time between connection events and address book updates,
+///   even under heavy load (in tests, we have observed delays up to 500ms),
+/// - the delay between an outbound connection failing,
+///   and the [CandidateSet](crate::peer_set::CandidateSet) registering the failure, and
+/// - the delay between the application closing a connection,
+///   and any remaining positive changes from the peer.
+pub const CONCURRENT_ADDRESS_CHANGE_PERIOD: Duration = Duration::from_secs(5);
+
 /// We expect to receive a message from a live peer at least once in this time duration.
 ///
 /// This is the sum of: