diff --git a/zebra-network/src/address_book.rs b/zebra-network/src/address_book.rs index 022ae193..d1a4e82f 100644 --- a/zebra-network/src/address_book.rs +++ b/zebra-network/src/address_book.rs @@ -8,8 +8,8 @@ use ordered_map::OrderedMap; use tracing::Span; use crate::{ - meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr, types::MetaAddr, - PeerAddrState, + constants, meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr, + types::MetaAddr, PeerAddrState, }; #[cfg(test)] @@ -59,6 +59,12 @@ pub struct AddressBook { /// sorts in ascending order, but [`OrderedMap`] sorts in descending order. by_addr: OrderedMap>, + /// The maximum number of addresses in the address book. + /// + /// Always set to [`MAX_ADDRS_IN_ADDRESS_BOOK`](constants::MAX_ADDRS_IN_ADDRESS_BOOK), + /// in release builds. Lower values are used during testing. + addr_limit: usize, + /// The local listener address. local_listener: SocketAddr, @@ -107,6 +113,7 @@ impl AddressBook { let mut new_book = AddressBook { by_addr: OrderedMap::new(|meta_addr| Reverse(*meta_addr)), + addr_limit: constants::MAX_ADDRS_IN_ADDRESS_BOOK, local_listener: canonical_socket_addr(local_listener), span, last_address_log: None, @@ -117,7 +124,9 @@ impl AddressBook { } /// Construct an [`AddressBook`] with the given `local_listener`, - /// [`tracing::Span`], and addresses. + /// `addr_limit`, [`tracing::Span`], and addresses. + /// + /// `addr_limit` is enforced by this method, and by [`AddressBook::update`]. /// /// If there are multiple [`MetaAddr`]s with the same address, /// an arbitrary address is inserted into the address book, @@ -128,6 +137,7 @@ impl AddressBook { #[cfg(any(test, feature = "proptest-impl"))] pub fn new_with_addrs( local_listener: SocketAddr, + addr_limit: usize, span: Span, addrs: impl IntoIterator, ) -> AddressBook { @@ -138,6 +148,7 @@ impl AddressBook { let chrono_now = Utc::now(); let mut new_book = AddressBook::new(local_listener, span); + new_book.addr_limit = addr_limit; let addrs = addrs .into_iter() @@ -146,6 +157,7 @@ impl AddressBook { meta_addr }) .filter(MetaAddr::address_is_valid_for_outbound) + .take(addr_limit) .map(|meta_addr| (meta_addr.addr, meta_addr)); for (socket_addr, meta_addr) in addrs { @@ -270,6 +282,25 @@ impl AddressBook { } self.by_addr.insert(updated.addr, updated); + + // Security: Limit the number of peers in the address book. + // + // We only delete outdated peers when we have too many peers. + // If we deleted them as soon as they became too old, + // then other peers could re-insert them into the address book. + // And we would start connecting to those outdated peers again, + // ignoring the age limit in [`MetaAddr::is_probably_reachable`]. + while self.by_addr.len() > self.addr_limit { + let surplus_peer = self + .peers() + .next_back() + .expect("just checked there is at least one peer"); + + self.by_addr.remove(&surplus_peer.addr); + } + + assert!(self.len() <= self.addr_limit); + std::mem::drop(_guard); self.update_metrics(instant_now, chrono_now); } @@ -320,7 +351,7 @@ impl AddressBook { /// Return an iterator over all peers. /// /// Returns peers in reconnection attempt order, including recently connected peers. - pub fn peers(&'_ self) -> impl Iterator + '_ { + pub fn peers(&'_ self) -> impl Iterator + DoubleEndedIterator + '_ { let _guard = self.span.enter(); self.by_addr.descending_values().cloned() } @@ -331,7 +362,7 @@ impl AddressBook { &'_ self, instant_now: Instant, chrono_now: chrono::DateTime, - ) -> impl Iterator + '_ { + ) -> impl Iterator + DoubleEndedIterator + '_ { let _guard = self.span.enter(); // Skip live peers, and peers pending a reconnect attempt. @@ -344,7 +375,10 @@ impl AddressBook { /// Return an iterator over all the peers in `state`, /// in reconnection attempt order, including recently connected peers. - pub fn state_peers(&'_ self, state: PeerAddrState) -> impl Iterator + '_ { + pub fn state_peers( + &'_ self, + state: PeerAddrState, + ) -> impl Iterator + DoubleEndedIterator + '_ { let _guard = self.span.enter(); self.by_addr @@ -359,7 +393,7 @@ impl AddressBook { &'_ self, instant_now: Instant, chrono_now: chrono::DateTime, - ) -> impl Iterator + '_ { + ) -> impl Iterator + DoubleEndedIterator + '_ { let _guard = self.span.enter(); self.by_addr @@ -373,7 +407,7 @@ impl AddressBook { pub fn recently_live_peers( &'_ self, now: chrono::DateTime, - ) -> impl Iterator + '_ { + ) -> impl Iterator + DoubleEndedIterator + '_ { let _guard = self.span.enter(); self.by_addr diff --git a/zebra-network/src/address_book/tests/prop.rs b/zebra-network/src/address_book/tests/prop.rs index 048de0f4..cff48cdc 100644 --- a/zebra-network/src/address_book/tests/prop.rs +++ b/zebra-network/src/address_book/tests/prop.rs @@ -9,13 +9,15 @@ use tracing::Span; use zebra_chain::serialization::Duration32; use crate::{ - constants::MAX_PEER_ACTIVE_FOR_GOSSIP, - meta_addr::{arbitrary::MAX_META_ADDR, MetaAddr}, + constants::{MAX_ADDRS_IN_ADDRESS_BOOK, MAX_PEER_ACTIVE_FOR_GOSSIP}, + meta_addr::{arbitrary::MAX_META_ADDR, MetaAddr, MetaAddrChange}, AddressBook, }; const TIME_ERROR_MARGIN: Duration32 = Duration32::from_seconds(1); +const MAX_ADDR_CHANGE: usize = 10; + proptest! { #[test] fn only_recently_reachable_are_gossiped( @@ -25,7 +27,12 @@ proptest! { zebra_test::init(); let chrono_now = Utc::now(); - let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses); + let address_book = AddressBook::new_with_addrs( + local_listener, + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::none(), + addresses + ); for gossiped_address in address_book.sanitized(chrono_now) { let duration_since_last_seen = gossiped_address @@ -48,10 +55,83 @@ proptest! { let instant_now = Instant::now(); let chrono_now = Utc::now(); - let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses); + let address_book = AddressBook::new_with_addrs( + local_listener, + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::none(), + addresses + ); for peer in address_book.reconnection_peers(instant_now, chrono_now) { prop_assert!(peer.is_probably_reachable(chrono_now), "peer: {:?}", peer); } } + + /// Test that the address book limit is respected for multiple peers. + #[test] + fn address_book_length_is_limited( + local_listener in any::(), + addr_changes_lists in vec( + MetaAddrChange::addr_changes_strategy(MAX_ADDR_CHANGE), + 2..MAX_ADDR_CHANGE + ), + addr_limit in 0..=MAX_ADDR_CHANGE, + pre_fill in any::(), + ) { + zebra_test::init(); + + let initial_addrs = if pre_fill { + addr_changes_lists + .iter() + .map(|(addr, _changes)| addr) + .cloned() + .collect() + } else { + Vec::new() + }; + + // sequentially apply changes for one address, then move on to the next + + let mut address_book = AddressBook::new_with_addrs( + local_listener, + addr_limit, + Span::none(), + initial_addrs.clone(), + ); + + for (_addr, changes) in addr_changes_lists.iter() { + for change in changes { + address_book.update(*change); + + prop_assert!( + address_book.len() <= addr_limit, + "sequential test length: {} was greater than limit: {}", + address_book.len(), addr_limit, + ); + } + } + + // interleave changes for different addresses + + let mut address_book = AddressBook::new_with_addrs( + local_listener, + addr_limit, + Span::none(), + initial_addrs, + ); + + for index in 0..MAX_ADDR_CHANGE { + for (_addr, changes) in addr_changes_lists.iter() { + if let Some(change) = changes.get(index) { + address_book.update(*change); + + prop_assert!( + address_book.len() <= addr_limit, + "interleave test length: {} was greater than limit: {}", + address_book.len(), addr_limit, + ); + } + } + } + } } diff --git a/zebra-network/src/address_book/tests/vectors.rs b/zebra-network/src/address_book/tests/vectors.rs index cab6bf77..9f5d0e3f 100644 --- a/zebra-network/src/address_book/tests/vectors.rs +++ b/zebra-network/src/address_book/tests/vectors.rs @@ -7,7 +7,10 @@ use tracing::Span; use zebra_chain::serialization::{DateTime32, Duration32}; -use crate::{meta_addr::MetaAddr, protocol::external::types::PeerServices, AddressBook}; +use crate::{ + constants::MAX_ADDRS_IN_ADDRESS_BOOK, meta_addr::MetaAddr, + protocol::external::types::PeerServices, AddressBook, +}; /// Make sure an empty address book is actually empty. #[test] @@ -39,8 +42,12 @@ fn address_book_peer_order() { // Regardless of the order of insertion, the most recent address should be chosen first let addrs = vec![meta_addr1, meta_addr2]; - let address_book = - AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs); + let address_book = AddressBook::new_with_addrs( + "0.0.0.0:0".parse().unwrap(), + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::current(), + addrs, + ); assert_eq!( address_book .reconnection_peers(Instant::now(), Utc::now()) @@ -50,8 +57,12 @@ fn address_book_peer_order() { // Reverse the order, check that we get the same result let addrs = vec![meta_addr2, meta_addr1]; - let address_book = - AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs); + let address_book = AddressBook::new_with_addrs( + "0.0.0.0:0".parse().unwrap(), + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::current(), + addrs, + ); assert_eq!( address_book .reconnection_peers(Instant::now(), Utc::now()) @@ -64,8 +75,12 @@ fn address_book_peer_order() { meta_addr2.addr = addr1; let addrs = vec![meta_addr1, meta_addr2]; - let address_book = - AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs); + let address_book = AddressBook::new_with_addrs( + "0.0.0.0:0".parse().unwrap(), + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::current(), + addrs, + ); assert_eq!( address_book .reconnection_peers(Instant::now(), Utc::now()) @@ -75,8 +90,12 @@ fn address_book_peer_order() { // Reverse the order, check that we get the same result let addrs = vec![meta_addr2, meta_addr1]; - let address_book = - AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs); + let address_book = AddressBook::new_with_addrs( + "0.0.0.0:0".parse().unwrap(), + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::current(), + addrs, + ); assert_eq!( address_book .reconnection_peers(Instant::now(), Utc::now()) diff --git a/zebra-network/src/constants.rs b/zebra-network/src/constants.rs index cc244c3e..275a14ab 100644 --- a/zebra-network/src/constants.rs +++ b/zebra-network/src/constants.rs @@ -138,6 +138,27 @@ pub const GET_ADDR_FANOUT: usize = 3; /// https://zips.z.cash/zip-0155#specification pub const MAX_ADDRS_IN_MESSAGE: usize = 1000; +/// The fraction of addresses Zebra sends in response to a `Peers` request. +/// +/// Each response contains approximately: +/// `address_book.len() / ADDR_RESPONSE_LIMIT_DENOMINATOR` +/// addresses, selected at random from the address book. +/// +/// # Security +/// +/// This limit makes sure that Zebra does not reveal its entire address book +/// in a single `Peers` response. +pub const ADDR_RESPONSE_LIMIT_DENOMINATOR: usize = 3; + +/// The maximum number of addresses Zebra will keep in its address book. +/// +/// This is a tradeoff between: +/// - revealing the whole address book in a few requests, +/// - sending the maximum number of peer addresses, and +/// - making sure the limit code actually gets run. +pub const MAX_ADDRS_IN_ADDRESS_BOOK: usize = + MAX_ADDRS_IN_MESSAGE * (ADDR_RESPONSE_LIMIT_DENOMINATOR + 1); + /// Truncate timestamps in outbound address messages to this time interval. /// /// ## SECURITY @@ -261,4 +282,40 @@ mod tests { assert!(EWMA_DECAY_TIME > REQUEST_TIMEOUT, "The EWMA decay time should be higher than the request timeout, so timed out peers are penalised by the EWMA."); } + + /// Make sure that peer age limits are consistent with each other. + #[test] + fn ensure_peer_age_limits_consistent() { + zebra_test::init(); + + assert!( + MAX_PEER_ACTIVE_FOR_GOSSIP <= MAX_RECENT_PEER_AGE, + "we should only gossip peers we are actually willing to try ourselves" + ); + } + + /// Make sure the address limits are consistent with each other. + #[test] + #[allow(clippy::assertions_on_constants)] + fn ensure_address_limits_consistent() { + // Zebra 1.0.0-beta.2 address book metrics in December 2021. + const TYPICAL_MAINNET_ADDRESS_BOOK_SIZE: usize = 4_500; + + zebra_test::init(); + + assert!( + MAX_ADDRS_IN_ADDRESS_BOOK >= GET_ADDR_FANOUT * MAX_ADDRS_IN_MESSAGE, + "the address book should hold at least a fanout's worth of addresses" + ); + + assert!( + MAX_ADDRS_IN_ADDRESS_BOOK / ADDR_RESPONSE_LIMIT_DENOMINATOR > MAX_ADDRS_IN_MESSAGE, + "the address book should hold enough addresses for a full response" + ); + + assert!( + MAX_ADDRS_IN_ADDRESS_BOOK < TYPICAL_MAINNET_ADDRESS_BOOK_SIZE, + "the address book limit should actually be used" + ); + } } diff --git a/zebra-network/src/meta_addr/tests/prop.rs b/zebra-network/src/meta_addr/tests/prop.rs index 757264ae..a7f70896 100644 --- a/zebra-network/src/meta_addr/tests/prop.rs +++ b/zebra-network/src/meta_addr/tests/prop.rs @@ -19,7 +19,7 @@ use tracing::Span; use zebra_chain::serialization::DateTime32; use crate::{ - constants::{MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY}, + constants::{MAX_ADDRS_IN_ADDRESS_BOOK, MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY}, meta_addr::{ arbitrary::{MAX_ADDR_CHANGE, MAX_META_ADDR}, MetaAddr, MetaAddrChange, @@ -154,8 +154,12 @@ proptest! { let chrono_now = Utc::now(); - let address_book = - AddressBook::new_with_addrs(local_listener, Span::none(), address_book_addrs); + let address_book = AddressBook::new_with_addrs( + local_listener, + MAX_ADDRS_IN_ADDRESS_BOOK, + Span::none(), + address_book_addrs + ); let sanitized_addrs = address_book.sanitized(chrono_now); let expected_local_listener = address_book.local_listener_meta_addr(); @@ -226,6 +230,7 @@ proptest! { let address_book = Arc::new(std::sync::Mutex::new(AddressBook::new_with_addrs( SocketAddr::from_str("0.0.0.0:0").unwrap(), + MAX_ADDRS_IN_ADDRESS_BOOK, Span::none(), addrs, ))); @@ -248,9 +253,13 @@ proptest! { attempt_count += 1; prop_assert!( attempt_count <= 1, - "candidate: {:?}, change: {}, now: {:?}, earliest next attempt: {:?}, \ - attempts: {}, live peer interval limit: {}, test time limit: {:?}, \ - peer change interval: {:?}, original addr was in address book: {}", + "candidate: {:?},\n \ + change: {},\n \ + now: {:?},\n \ + earliest next attempt: {:?},\n \ + attempts: {}, live peer interval limit: {},\n \ + test time limit: {:?}, peer change interval: {:?},\n \ + original addr was in address book: {}\n", candidate_addr, i, Instant::now(), diff --git a/zebrad/src/components/inbound.rs b/zebrad/src/components/inbound.rs index 19d3a452..394d963d 100644 --- a/zebrad/src/components/inbound.rs +++ b/zebrad/src/components/inbound.rs @@ -25,7 +25,10 @@ use zebra_state as zs; use zebra_chain::block::{self, Block}; use zebra_consensus::chain::VerifyChainError; -use zebra_network::{constants::MAX_ADDRS_IN_MESSAGE, AddressBook}; +use zebra_network::{ + constants::{ADDR_RESPONSE_LIMIT_DENOMINATOR, MAX_ADDRS_IN_MESSAGE}, + AddressBook, +}; // Re-use the syncer timeouts for consistency. use super::{ @@ -39,10 +42,6 @@ mod tests; use downloads::Downloads as BlockDownloads; -/// A security parameter to return only 1/3 of available addresses as a -/// response to a `Peers` request. -const FRAC_OF_AVAILABLE_ADDRESS: f64 = 1. / 3.; - type BlockDownloadPeerSet = Buffer, zn::Request>; type State = Buffer, zs::Request>; @@ -313,9 +312,13 @@ impl Service for Inbound { let mut peers = peers.sanitized(now); // Truncate the list - let truncate_at = MAX_ADDRS_IN_MESSAGE - .min((peers.len() as f64 * FRAC_OF_AVAILABLE_ADDRESS).ceil() as usize); - peers.truncate(truncate_at); + // + // TODO: replace with div_ceil once it stabilises + // https://github.com/rust-lang/rust/issues/88581 + let address_limit = (peers.len() + ADDR_RESPONSE_LIMIT_DENOMINATOR - 1) / ADDR_RESPONSE_LIMIT_DENOMINATOR; + let address_limit = MAX_ADDRS_IN_MESSAGE + .min(address_limit); + peers.truncate(address_limit); if !peers.is_empty() { async { Ok(zn::Response::Peers(peers)) }.boxed()