From a9f1c189d99a7a788011245f36418932ed778796 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Mon, 1 Nov 2021 23:45:35 -0300 Subject: [PATCH] Make `services` field in `MetaAddr` optional (#2976) * Use `prop_assert` instead of `assert` Otherwise the test input isn't minimized. * Split long string into a multi-line string And add some newlines to try to improve readability. * Fix referenced issue number They had a typo in their number. * Make peer services optional It is unknown for initial peers. * Fix `preserve_initial_untrusted_values` test Now that it's optional, the services field can be written to if it was previously empty. * Fix formatting of property tests Run rustfmt on them. * Restore `TODO` comment Make it easy to find planned improvements in the code. Co-authored-by: teor * Comment on how ordering is affected Make it clear that missing services causes the peer to be chosen last. Co-authored-by: teor * Don't expect `services` to be available Avoid a panic by using the compiler to help enforce the handling of the case correctly. * Panic if received gossiped address has no services All received gossiped addresses have services. The only addresses that don't have services configured are the initial seed addresses. Co-authored-by: teor --- zebra-network/src/meta_addr.rs | 72 +++++++++++------ zebra-network/src/meta_addr/tests/check.rs | 5 +- zebra-network/src/meta_addr/tests/prop.rs | 80 +++++++++++-------- zebra-network/src/peer_set/candidate_set.rs | 7 +- .../src/peer_set/initialize/tests/vectors.rs | 4 +- 5 files changed, 103 insertions(+), 65 deletions(-) diff --git a/zebra-network/src/meta_addr.rs b/zebra-network/src/meta_addr.rs index 31d2870e..e6339445 100644 --- a/zebra-network/src/meta_addr.rs +++ b/zebra-network/src/meta_addr.rs @@ -150,9 +150,9 @@ pub struct MetaAddr { /// `services` from `NeverAttempted` peers may be invalid due to outdated /// records, older peer versions, or buggy or malicious peers. // - // TODO: make services private and optional - // split gossiped and handshake services? (#2234) - pub(crate) services: PeerServices, + // TODO: make services private + // split gossiped and handshake services? (#2324) + pub(crate) services: Option, /// The unverified "last seen time" gossiped by the remote peer that sent us /// this address. @@ -177,8 +177,7 @@ pub struct MetaAddr { /// The outcome of our most recent communication attempt with this peer. // - // TODO: make services private and optional? - // move the time and services fields into PeerAddrState? + // TODO: move the time and services fields into PeerAddrState? // then some fields could be required in some states pub(crate) last_connection_state: PeerAddrState, } @@ -260,7 +259,7 @@ impl MetaAddr { ) -> MetaAddr { MetaAddr { addr: canonical_socket_addr(addr), - services: untrusted_services, + services: Some(untrusted_services), untrusted_last_seen: Some(untrusted_last_seen), last_response: None, last_attempt: None, @@ -270,15 +269,19 @@ impl MetaAddr { } /// Returns a [`MetaAddrChange::NewGossiped`], based on a gossiped peer - /// `MetaAddr`. - pub fn new_gossiped_change(self) -> MetaAddrChange { - NewGossiped { + /// [`MetaAddr`]. + /// + /// Returns [`None`] if the gossiped peer is missing the untrusted services field. + pub fn new_gossiped_change(self) -> Option { + let untrusted_services = self.services?; + + Some(NewGossiped { addr: canonical_socket_addr(self.addr), - untrusted_services: self.services, + untrusted_services, untrusted_last_seen: self .untrusted_last_seen .expect("unexpected missing last seen"), - } + }) } /// Returns a [`MetaAddrChange::UpdateResponded`] for a peer that has just @@ -502,7 +505,12 @@ impl MetaAddr { /// - reject `NeverAttempted...` [`MetaAddrChange`]s, and /// - temporarily stop outbound connections to a [`MetaAddr`]. pub fn last_known_info_is_valid_for_outbound(&self) -> bool { - self.services.contains(PeerServices::NODE_NETWORK) && self.address_is_valid_for_outbound() + let is_node = match self.services { + Some(services) => services.contains(PeerServices::NODE_NETWORK), + None => true, + }; + + is_node && self.address_is_valid_for_outbound() } /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer. @@ -524,9 +532,10 @@ impl MetaAddr { Some(MetaAddr { addr: canonical_socket_addr(self.addr), + // initial peers are sanitized assuming they are `NODE_NETWORK` // TODO: split untrusted and direct services - // sanitize untrusted services to NODE_NETWORK only? (#2234) - services: self.services, + // consider sanitizing untrusted services to NODE_NETWORK (#2324) + services: self.services.or(Some(PeerServices::NODE_NETWORK)), // only put the last seen time in the untrusted field, // this matches deserialization, and avoids leaking internal state untrusted_last_seen: Some(last_seen), @@ -586,10 +595,10 @@ impl MetaAddrChange { NewAlternate { untrusted_services, .. } => Some(*untrusted_services), - // TODO: create a "services implemented by Zebra" constant (#2234) + // TODO: create a "services implemented by Zebra" constant (#2324) NewLocal { .. } => Some(PeerServices::NODE_NETWORK), UpdateAttempt { .. } => None, - // TODO: split untrusted and direct services (#2234) + // TODO: split untrusted and direct services (#2324) UpdateResponded { services, .. } => Some(*services), UpdateFailed { services, .. } => *services, } @@ -678,10 +687,7 @@ impl MetaAddrChange { match self { NewGossiped { .. } | NewAlternate { .. } | NewLocal { .. } => Some(MetaAddr { addr: self.addr(), - // TODO: make services optional when we add a DNS seeder change and state - services: self - .untrusted_services() - .expect("unexpected missing services"), + services: self.untrusted_services(), untrusted_last_seen: self.untrusted_last_seen(), last_response: None, last_attempt: None, @@ -727,8 +733,7 @@ impl MetaAddrChange { // so malicious peers can't keep changing our peer connection order. Some(MetaAddr { addr: self.addr(), - // TODO: or(self.untrusted_services()) when services become optional (#2234) - services: previous.services, + services: previous.services.or_else(|| self.untrusted_services()), untrusted_last_seen: previous .untrusted_last_seen .or_else(|| self.untrusted_last_seen()), @@ -752,7 +757,7 @@ impl MetaAddrChange { addr: self.addr(), // We want up-to-date services, even if they have fewer bits, // or they are applied out of order. - services: self.untrusted_services().unwrap_or(previous.services), + services: self.untrusted_services().or(previous.services), // Only NeverAttempted changes can modify the last seen field untrusted_last_seen: previous.untrusted_last_seen, // Since Some(time) is always greater than None, `max` prefers: @@ -831,7 +836,9 @@ impl Ord for MetaAddr { // So this comparison will have no impact until Zebra implements // more service features. // - // TODO: order services by usefulness, not bit pattern values (#2234) + // None is less than Some(T), so peers with missing services are chosen last. + // + // TODO: order services by usefulness, not bit pattern values (#2324) // Security: split gossiped and direct services let larger_services = self.services.cmp(&other.services); @@ -879,10 +886,23 @@ impl Eq for MetaAddr {} impl ZcashSerialize for MetaAddr { fn zcash_serialize(&self, mut writer: W) -> Result<(), std::io::Error> { self.last_seen() - .expect("unexpected MetaAddr with missing last seen time: MetaAddrs should be sanitized before serialization") + .expect( + "unexpected MetaAddr with missing last seen time: MetaAddrs should be sanitized \ + before serialization", + ) .zcash_serialize(&mut writer)?; - writer.write_u64::(self.services.bits())?; + + writer.write_u64::( + self.services + .expect( + "unexpected MetaAddr with missing peer services: MetaAddrs should be \ + sanitized before serialization", + ) + .bits(), + )?; + writer.write_socket_addr(self.addr)?; + Ok(()) } } diff --git a/zebra-network/src/meta_addr/tests/check.rs b/zebra-network/src/meta_addr/tests/check.rs index 565443d9..5a65ab0c 100644 --- a/zebra-network/src/meta_addr/tests/check.rs +++ b/zebra-network/src/meta_addr/tests/check.rs @@ -70,8 +70,9 @@ pub(crate) fn sanitize_avoids_leaks(original: &MetaAddr, sanitized: &MetaAddr) { // Sanitize to the the default state, even though it's not serialized assert_eq!(sanitized.last_connection_state, Default::default()); // Sanitize to known flags - let sanitized_peer_services = original.services & PeerServices::all(); - assert_eq!(sanitized.services, sanitized_peer_services); + let sanitized_peer_services = + original.services.unwrap_or(PeerServices::NODE_NETWORK) & PeerServices::all(); + assert_eq!(sanitized.services, Some(sanitized_peer_services)); // Remove IPv6 scope ID and flow information let sanitized_socket_addr = SocketAddr::new(original.addr.ip(), original.addr.port()); diff --git a/zebra-network/src/meta_addr/tests/prop.rs b/zebra-network/src/meta_addr/tests/prop.rs index 918a8696..2776518b 100644 --- a/zebra-network/src/meta_addr/tests/prop.rs +++ b/zebra-network/src/meta_addr/tests/prop.rs @@ -52,7 +52,10 @@ proptest! { // also check the address, port, and services individually prop_assert!(!addr.addr.ip().is_unspecified()); prop_assert_ne!(addr.addr.port(), 0); - prop_assert!(addr.services.contains(PeerServices::NODE_NETWORK)); + + if let Some(services) = addr.services { + prop_assert!(services.contains(PeerServices::NODE_NETWORK)); + } check::sanitize_avoids_leaks(&addr, &sanitized); } @@ -60,9 +63,7 @@ proptest! { /// Test round-trip serialization for gossiped MetaAddrs #[test] - fn gossiped_roundtrip( - gossiped_addr in MetaAddr::gossiped_strategy() - ) { + fn gossiped_roundtrip(gossiped_addr in MetaAddr::gossiped_strategy()) { zebra_test::init(); // We require sanitization before serialization @@ -121,14 +122,11 @@ proptest! { deserialized_addr, hex::encode(&addr_bytes2), ); - } /// Test round-trip serialization for all MetaAddr variants after sanitization #[test] - fn sanitized_roundtrip( - addr in any::() - ) { + fn sanitized_roundtrip(addr in any::()) { zebra_test::init(); // We require sanitization before serialization, @@ -194,14 +192,15 @@ proptest! { deserialized_addr, hex::encode(&addr_bytes2), ); - } /// Make sure that [`MetaAddrChange`]s: /// - do not modify the last seen time, unless it was None, and /// - only modify the services after a response or failure. #[test] - fn preserve_initial_untrusted_values((mut addr, changes) in MetaAddrChange::addr_changes_strategy(MAX_ADDR_CHANGE)) { + fn preserve_initial_untrusted_values( + (mut addr, changes) in MetaAddrChange::addr_changes_strategy(MAX_ADDR_CHANGE), + ) { zebra_test::init(); for change in changes { @@ -211,14 +210,19 @@ proptest! { if addr.untrusted_last_seen.is_some() { prop_assert_eq!(changed_addr.untrusted_last_seen, addr.untrusted_last_seen); } else { - prop_assert_eq!(changed_addr.untrusted_last_seen, change.untrusted_last_seen()); + prop_assert_eq!( + changed_addr.untrusted_last_seen, + change.untrusted_last_seen() + ); } // services: // check that we only change if there was a handshake - if changed_addr.last_connection_state.is_never_attempted() - || changed_addr.last_connection_state == AttemptPending - || change.untrusted_services().is_none() { + if addr.services.is_some() + && (changed_addr.last_connection_state.is_never_attempted() + || changed_addr.last_connection_state == AttemptPending + || change.untrusted_services().is_none()) + { prop_assert_eq!(changed_addr.services, addr.services); } @@ -256,7 +260,7 @@ proptest! { // If `change` is invalid for the current MetaAddr state, skip it. if let Some(changed_addr) = change.apply_to_meta_addr(addr) { - assert_eq!(changed_addr.addr, addr.addr); + prop_assert_eq!(changed_addr.addr, addr.addr); addr = changed_addr; } } @@ -276,16 +280,15 @@ proptest! { ) { zebra_test::init(); - let address_book = AddressBook::new_with_addrs( - local_listener, - Span::none(), - address_book_addrs - ); + let address_book = + AddressBook::new_with_addrs(local_listener, Span::none(), address_book_addrs); let sanitized_addrs = address_book.sanitized(); let expected_local_listener = address_book.local_listener_meta_addr(); let canonical_local_listener = canonical_socket_addr(local_listener); - let book_sanitized_local_listener = sanitized_addrs.iter().find(|meta_addr| meta_addr.addr == canonical_local_listener ); + let book_sanitized_local_listener = sanitized_addrs + .iter() + .find(|meta_addr| meta_addr.addr == canonical_local_listener); // invalid addresses should be removed by sanitization, // regardless of where they have come from @@ -325,9 +328,10 @@ proptest! { // Run the test for this much simulated time let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS; // Advance the clock by this much for every peer change - let peer_change_interval: Duration = overall_test_time / MAX_ADDR_CHANGE.try_into().unwrap(); + let peer_change_interval: Duration = + overall_test_time / MAX_ADDR_CHANGE.try_into().unwrap(); - assert!( + prop_assert!( u32::try_from(MAX_ADDR_CHANGE).unwrap() >= 3 * LIVE_PEER_INTERVALS, "there are enough changes for good test coverage", ); @@ -365,10 +369,10 @@ proptest! { for (i, change) in changes.into_iter().enumerate() { while let Some(candidate_addr) = candidate_set.next().await { - assert_eq!(candidate_addr.addr, addr.addr); + prop_assert_eq!(candidate_addr.addr, addr.addr); attempt_count += 1; - assert!( + prop_assert!( attempt_count <= 1, "candidate: {:?}, change: {}, now: {:?}, earliest next attempt: {:?}, \ attempts: {}, live peer interval limit: {}, test time limit: {:?}, \ @@ -395,7 +399,9 @@ proptest! { attempt_count = 0; } } - }); + + Ok(()) + })?; } /// Make sure that all disconnected [`MetaAddr`]s are retried once, before @@ -420,9 +426,10 @@ proptest! { // Run the test for this much simulated time let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS; // Advance the clock by this much for every peer change - let peer_change_interval: Duration = overall_test_time / MAX_ADDR_CHANGE.try_into().unwrap(); + let peer_change_interval: Duration = + overall_test_time / MAX_ADDR_CHANGE.try_into().unwrap(); - assert!( + prop_assert!( u32::try_from(MAX_ADDR_CHANGE).unwrap() >= 3 * LIVE_PEER_INTERVALS, "there are enough changes for good test coverage", ); @@ -449,7 +456,9 @@ proptest! { while addr.is_ready_for_connection_attempt() { *attempt_counts.entry(addr.addr).or_default() += 1; - assert!(*attempt_counts.get(&addr.addr).unwrap() <= LIVE_PEER_INTERVALS + 1); + prop_assert!( + *attempt_counts.get(&addr.addr).unwrap() <= LIVE_PEER_INTERVALS + 1 + ); // Simulate an attempt *addr = MetaAddr::new_reconnect(&addr.addr) @@ -461,17 +470,18 @@ proptest! { // If we've run out of changes for this addr, do nothing. if let Some(changed_addr) = change .map(|change| change.apply_to_meta_addr(*addr)) - .flatten() { - assert_eq!(changed_addr.addr, addr.addr); - *addr = changed_addr; - } + .flatten() + { + prop_assert_eq!(changed_addr.addr, addr.addr); + *addr = changed_addr; + } } tokio::time::advance(peer_change_interval).await; } - attempt_counts - }); + Ok(attempt_counts) + })?; let min_attempts = attempt_counts.values().min(); let max_attempts = attempt_counts.values().max(); diff --git a/zebra-network/src/peer_set/candidate_set.rs b/zebra-network/src/peer_set/candidate_set.rs index 9383f677..7edbfe53 100644 --- a/zebra-network/src/peer_set/candidate_set.rs +++ b/zebra-network/src/peer_set/candidate_set.rs @@ -267,7 +267,12 @@ where /// Add new `addrs` to the address book. fn send_addrs(&self, addrs: impl IntoIterator) { - let addrs = addrs.into_iter().map(MetaAddr::new_gossiped_change); + let addrs = addrs + .into_iter() + .map(MetaAddr::new_gossiped_change) + .map(|maybe_addr| { + maybe_addr.expect("Received gossiped peers always have services set") + }); // # Correctness // diff --git a/zebra-network/src/peer_set/initialize/tests/vectors.rs b/zebra-network/src/peer_set/initialize/tests/vectors.rs index 245fb212..a7fb38d5 100644 --- a/zebra-network/src/peer_set/initialize/tests/vectors.rs +++ b/zebra-network/src/peer_set/initialize/tests/vectors.rs @@ -1260,7 +1260,9 @@ where let addr = MetaAddr::new_gossiped_meta_addr(addr, PeerServices::NODE_NETWORK, DateTime32::now()); fake_peer = Some(addr); - let addr = addr.new_gossiped_change(); + let addr = addr + .new_gossiped_change() + .expect("created MetaAddr contains enough information to represent a gossiped address"); address_book.update(addr); }