fix(network): remove every address book entry for a banned peer IP (#11173)

Author: mergify[bot]

CHANGELOG.md

@@ -50,6 +50,10 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 - Reject blocks whose total chain value pool balance would exceed `MAX_MONEY`,
   enforcing the cap on the total monetary base
   ([#10817](https://github.com/ZcashFoundation/zebra/pull/10817))
+- Banning a misbehaving peer now removes every address book entry for that IP, and a banned IP is
+  never selected as a reconnection candidate. Previously an entry on a different port could survive
+  the ban and occupy the first candidate slot until the node restarted
+  ([#11134](https://github.com/ZcashFoundation/zebra/issues/11134)).
 
 ### Security
 

zebra-network/CHANGELOG.md

@@ -23,6 +23,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Peer-set, crawler-handshake, and address-book gauges now include a `network` label, so multiple
   network instances in one process do not overwrite each other's values.
+- `AddressBook::update()` logs a change rejected for a banned peer IP at `debug` instead of `warn`,
+  since remote peers control how often it fires
+  ([#11134](https://github.com/ZcashFoundation/zebra/issues/11134)).
+
+### Fixed
+
+- Banning a peer IP now removes every address book entry for that IP, and `reconnection_peers()`
+  never returns an address whose IP is banned. Previously an entry for the banned IP on another
+  port could survive the ban and stay at the front of the reconnection order for the lifetime of
+  the process ([#11134](https://github.com/ZcashFoundation/zebra/issues/11134)).
 
 ### Security
 

zebra-network/src/address_book.rs

@@ -416,7 +416,8 @@ impl AddressBook {
     #[allow(clippy::unwrap_in_result)]
     pub fn update(&mut self, change: MetaAddrChange) -> Option<MetaAddr> {
         if self.bans_by_ip.contains_key(&change.addr().ip()) {
-            tracing::warn!(
+            // Remote peers control how often this fires, so keep it below `warn` (#11134).
+            tracing::debug!(
                 ?change,
                 "attempted to add a banned peer addr to address book"
             );
@@ -464,8 +465,7 @@ impl AddressBook {
                 let banned_addrs: Vec<_> = self
                     .by_addr
                     .descending_keys()
-                    .skip_while(|addr| addr.ip() != banned_ip)
-                    .take_while(|addr| addr.ip() == banned_ip)
+                    .filter(|addr| addr.ip() == banned_ip)
                     .cloned()
                     .collect();
 
@@ -647,12 +647,13 @@ impl AddressBook {
     ) -> impl DoubleEndedIterator<Item = MetaAddr> + '_ {
         let _guard = self.span.enter();
 
-        // Skip live peers, and peers pending a reconnect attempt.
+        // Skip live peers, banned peers, and peers pending a reconnect attempt.
         // The peers are already stored in sorted order.
         self.by_addr
             .descending_values()
             .filter(move |peer| {
-                peer.is_ready_for_connection_attempt(instant_now, chrono_now, &self.network)
+                !self.bans_by_ip.contains_key(&peer.addr.ip())
+                    && peer.is_ready_for_connection_attempt(instant_now, chrono_now, &self.network)
                     && self.is_ready_for_connection_attempt_with_ip(&peer.addr.ip(), chrono_now)
             })
             .cloned()

zebra-network/src/address_book/tests/vectors.rs

@@ -250,3 +250,69 @@ fn test_reconnection_peers_skips_recently_updated_ip<
         assert_ne!(next_reconnection_peer, None,);
     }
 }
+
+/// Regression test for <https://github.com/ZcashFoundation/zebra/issues/11134>.
+///
+/// `by_addr` is ordered by reconnection order, not grouped by IP, so the ban path's old
+/// `skip_while(ip != banned).take_while(ip == banned)` scan stopped at the first entry for a
+/// different IP, and any later entry on the banned IP survived. That survivor then stayed at the
+/// front of the reconnection order for the lifetime of the process: it was selected as a candidate
+/// on every crawl, and `update()` rejected the resulting `UpdateAttempt` because the IP was
+/// banned, so its state never changed.
+#[test]
+fn ban_removes_every_entry_for_the_banned_ip() {
+    let banned_addr: crate::PeerSocketAddr = "127.0.0.1:8233".parse().unwrap();
+    let unrelated_addr: crate::PeerSocketAddr = "127.0.0.2:8233".parse().unwrap();
+    // An ephemeral-port entry for the same IP, like the one in #11134.
+    let zombie_addr: crate::PeerSocketAddr = "127.0.0.1:43562".parse().unwrap();
+
+    // `max_connections_per_ip` is above one, so `reconnection_peers` does not skip the second
+    // entry on the banned IP for being a duplicate IP.
+    let mut address_book =
+        AddressBook::new("0.0.0.0:0".parse().unwrap(), &Mainnet, 2, Span::current());
+
+    // `MetaAddr`'s `Ord` sorts more recently gossiped addresses first, so these last seen times
+    // place the unrelated IP between the two entries for the banned IP.
+    for (addr, last_seen) in [(banned_addr, 2), (unrelated_addr, 1), (zombie_addr, 0)] {
+        address_book.update(gossiped_change(
+            addr,
+            PeerServices::NODE_NETWORK,
+            DateTime32::MIN.saturating_add(Duration32::from_seconds(last_seen)),
+        ));
+    }
+
+    // Without this ordering the test would also pass before the fix, because a contiguous scan
+    // removes contiguous entries correctly.
+    assert_eq!(
+        address_book.by_addr.descending_keys().collect::<Vec<_>>(),
+        vec![&banned_addr, &unrelated_addr, &zombie_addr],
+        "test setup: the unrelated IP must sort between the two entries for the banned IP",
+    );
+
+    address_book.update(MetaAddrChange::UpdateMisbehavior {
+        addr: banned_addr,
+        score_increment: MAX_PEER_MISBEHAVIOR_SCORE,
+    });
+
+    assert!(
+        address_book.bans().contains_key(&banned_addr.ip()),
+        "ban-threshold misbehavior should ban the peer IP",
+    );
+    assert_eq!(
+        address_book.by_addr.descending_keys().collect::<Vec<_>>(),
+        vec![&unrelated_addr],
+        "the ban should remove every entry for the banned IP, including the one that does not \
+         sort next to the banned address",
+    );
+
+    let candidates: Vec<_> = address_book
+        .reconnection_peers(Instant::now(), Utc::now())
+        .map(|peer| peer.addr)
+        .collect();
+
+    assert_eq!(
+        candidates,
+        vec![unrelated_addr],
+        "a banned IP must never be a reconnection candidate",
+    );
+}