octo
/
Zebra
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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 <teor@riseup.net>

* Comment on how ordering is affected

Make it clear that missing services causes the peer to be chosen last.

Co-authored-by: teor <teor@riseup.net>

* 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 <teor@riseup.net>
This commit is contained in:
Janito Vaqueiro Ferreira Filho 2021-11-01 23:45:35 -03:00 committed by GitHub
parent 9963471b7c
commit a9f1c189d9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 103 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

72
zebra-network/src/meta_addr.rs
View File

@ -150,9 +150,9 @@ pub struct MetaAddr {
/// `services` from `NeverAttempted` peers may be invalid due to outdated /// `services` from `NeverAttempted` peers may be invalid due to outdated
/// records, older peer versions, or buggy or malicious peers. /// records, older peer versions, or buggy or malicious peers.
// //
// TODO: make services private and optional // TODO: make services private
// split gossiped and handshake services? (#2234) // split gossiped and handshake services? (#2324)
pub(crate) services: PeerServices, pub(crate) services: Option<PeerServices>,
/// The unverified "last seen time" gossiped by the remote peer that sent us /// The unverified "last seen time" gossiped by the remote peer that sent us
/// this address. /// this address.
@ -177,8 +177,7 @@ pub struct MetaAddr {
/// The outcome of our most recent communication attempt with this peer. /// The outcome of our most recent communication attempt with this peer.
// //
// TODO: make services private and optional? // TODO: move the time and services fields into PeerAddrState?
// move the time and services fields into PeerAddrState?
// then some fields could be required in some states // then some fields could be required in some states
pub(crate) last_connection_state: PeerAddrState, pub(crate) last_connection_state: PeerAddrState,
} }
@ -260,7 +259,7 @@ impl MetaAddr {
) -> MetaAddr { ) -> MetaAddr {
MetaAddr { MetaAddr {
addr: canonical_socket_addr(addr), addr: canonical_socket_addr(addr),
services: untrusted_services, services: Some(untrusted_services),
untrusted_last_seen: Some(untrusted_last_seen), untrusted_last_seen: Some(untrusted_last_seen),
last_response: None, last_response: None,
last_attempt: None, last_attempt: None,
@ -270,15 +269,19 @@ impl MetaAddr {
} }
/// Returns a [`MetaAddrChange::NewGossiped`], based on a gossiped peer /// Returns a [`MetaAddrChange::NewGossiped`], based on a gossiped peer
/// `MetaAddr`. /// [`MetaAddr`].
pub fn new_gossiped_change(self) -> MetaAddrChange { ///
NewGossiped { /// Returns [`None`] if the gossiped peer is missing the untrusted services field.
pub fn new_gossiped_change(self) -> Option<MetaAddrChange> {
let untrusted_services = self.services?;
Some(NewGossiped {
addr: canonical_socket_addr(self.addr), addr: canonical_socket_addr(self.addr),
untrusted_services: self.services, untrusted_services,
untrusted_last_seen: self untrusted_last_seen: self
.untrusted_last_seen .untrusted_last_seen
.expect("unexpected missing last seen"), .expect("unexpected missing last seen"),
} })
} }
/// Returns a [`MetaAddrChange::UpdateResponded`] for a peer that has just /// Returns a [`MetaAddrChange::UpdateResponded`] for a peer that has just
@ -502,7 +505,12 @@ impl MetaAddr {
/// - reject `NeverAttempted...` [`MetaAddrChange`]s, and /// - reject `NeverAttempted...` [`MetaAddrChange`]s, and
/// - temporarily stop outbound connections to a [`MetaAddr`]. /// - temporarily stop outbound connections to a [`MetaAddr`].
pub fn last_known_info_is_valid_for_outbound(&self) -> bool { 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. /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer.
@ -524,9 +532,10 @@ impl MetaAddr {
Some(MetaAddr { Some(MetaAddr {
addr: canonical_socket_addr(self.addr), addr: canonical_socket_addr(self.addr),
// initial peers are sanitized assuming they are `NODE_NETWORK`
// TODO: split untrusted and direct services // TODO: split untrusted and direct services
// sanitize untrusted services to NODE_NETWORK only? (#2234) // consider sanitizing untrusted services to NODE_NETWORK (#2324)
services: self.services, services: self.services.or(Some(PeerServices::NODE_NETWORK)),
// only put the last seen time in the untrusted field, // only put the last seen time in the untrusted field,
// this matches deserialization, and avoids leaking internal state // this matches deserialization, and avoids leaking internal state
untrusted_last_seen: Some(last_seen), untrusted_last_seen: Some(last_seen),
@ -586,10 +595,10 @@ impl MetaAddrChange {
NewAlternate { NewAlternate {
untrusted_services, .. untrusted_services, ..
} => Some(*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), NewLocal { .. } => Some(PeerServices::NODE_NETWORK),
UpdateAttempt { .. } => None, UpdateAttempt { .. } => None,
// TODO: split untrusted and direct services (#2234) // TODO: split untrusted and direct services (#2324)
UpdateResponded { services, .. } => Some(*services), UpdateResponded { services, .. } => Some(*services),
UpdateFailed { services, .. } => *services, UpdateFailed { services, .. } => *services,
} }
@ -678,10 +687,7 @@ impl MetaAddrChange {
match self { match self {
NewGossiped { .. } | NewAlternate { .. } | NewLocal { .. } => Some(MetaAddr { NewGossiped { .. } | NewAlternate { .. } | NewLocal { .. } => Some(MetaAddr {
addr: self.addr(), addr: self.addr(),
// TODO: make services optional when we add a DNS seeder change and state services: self.untrusted_services(),
services: self
.untrusted_services()
.expect("unexpected missing services"),
untrusted_last_seen: self.untrusted_last_seen(), untrusted_last_seen: self.untrusted_last_seen(),
last_response: None, last_response: None,
last_attempt: None, last_attempt: None,
@ -727,8 +733,7 @@ impl MetaAddrChange {
// so malicious peers can't keep changing our peer connection order. // so malicious peers can't keep changing our peer connection order.
Some(MetaAddr { Some(MetaAddr {
addr: self.addr(), addr: self.addr(),
// TODO: or(self.untrusted_services()) when services become optional (#2234) services: previous.services.or_else(|| self.untrusted_services()),
services: previous.services,
untrusted_last_seen: previous untrusted_last_seen: previous
.untrusted_last_seen .untrusted_last_seen
.or_else(|| self.untrusted_last_seen()), .or_else(|| self.untrusted_last_seen()),
@ -752,7 +757,7 @@ impl MetaAddrChange {
addr: self.addr(), addr: self.addr(),
// We want up-to-date services, even if they have fewer bits, // We want up-to-date services, even if they have fewer bits,
// or they are applied out of order. // 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 // Only NeverAttempted changes can modify the last seen field
untrusted_last_seen: previous.untrusted_last_seen, untrusted_last_seen: previous.untrusted_last_seen,
// Since Some(time) is always greater than None, `max` prefers: // 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 // So this comparison will have no impact until Zebra implements
// more service features. // 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 // Security: split gossiped and direct services
let larger_services = self.services.cmp(&other.services); let larger_services = self.services.cmp(&other.services);
@ -879,10 +886,23 @@ impl Eq for MetaAddr {}
impl ZcashSerialize for MetaAddr { impl ZcashSerialize for MetaAddr {
fn zcash_serialize<W: Write>(&self, mut writer: W) -> Result<(), std::io::Error> { fn zcash_serialize<W: Write>(&self, mut writer: W) -> Result<(), std::io::Error> {
self.last_seen() 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)?; .zcash_serialize(&mut writer)?;
writer.write_u64::<LittleEndian>(self.services.bits())?;
writer.write_u64::<LittleEndian>(
self.services
.expect(
"unexpected MetaAddr with missing peer services: MetaAddrs should be \
sanitized before serialization",
)
.bits(),
)?;
writer.write_socket_addr(self.addr)?; writer.write_socket_addr(self.addr)?;
Ok(()) Ok(())
} }
} }

5
zebra-network/src/meta_addr/tests/check.rs
View File

@ -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 // Sanitize to the the default state, even though it's not serialized
assert_eq!(sanitized.last_connection_state, Default::default()); assert_eq!(sanitized.last_connection_state, Default::default());
// Sanitize to known flags // Sanitize to known flags
let sanitized_peer_services = original.services & PeerServices::all(); let sanitized_peer_services =
assert_eq!(sanitized.services, 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 // Remove IPv6 scope ID and flow information
let sanitized_socket_addr = SocketAddr::new(original.addr.ip(), original.addr.port()); let sanitized_socket_addr = SocketAddr::new(original.addr.ip(), original.addr.port());

80
zebra-network/src/meta_addr/tests/prop.rs
View File

@ -52,7 +52,10 @@ proptest! {
// also check the address, port, and services individually // also check the address, port, and services individually
prop_assert!(!addr.addr.ip().is_unspecified()); prop_assert!(!addr.addr.ip().is_unspecified());
prop_assert_ne!(addr.addr.port(), 0); 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); check::sanitize_avoids_leaks(&addr, &sanitized);
} }
@ -60,9 +63,7 @@ proptest! {
/// Test round-trip serialization for gossiped MetaAddrs /// Test round-trip serialization for gossiped MetaAddrs
#[test] #[test]
fn gossiped_roundtrip( fn gossiped_roundtrip(gossiped_addr in MetaAddr::gossiped_strategy()) {
gossiped_addr in MetaAddr::gossiped_strategy()
) {
zebra_test::init(); zebra_test::init();
// We require sanitization before serialization // We require sanitization before serialization
@ -121,14 +122,11 @@ proptest! {
deserialized_addr, deserialized_addr,
hex::encode(&addr_bytes2), hex::encode(&addr_bytes2),
); );
} }
/// Test round-trip serialization for all MetaAddr variants after sanitization /// Test round-trip serialization for all MetaAddr variants after sanitization
#[test] #[test]
fn sanitized_roundtrip( fn sanitized_roundtrip(addr in any::<MetaAddr>()) {
addr in any::<MetaAddr>()
) {
zebra_test::init(); zebra_test::init();
// We require sanitization before serialization, // We require sanitization before serialization,
@ -194,14 +192,15 @@ proptest! {
deserialized_addr, deserialized_addr,
hex::encode(&addr_bytes2), hex::encode(&addr_bytes2),
); );
} }
/// Make sure that [`MetaAddrChange`]s: /// Make sure that [`MetaAddrChange`]s:
/// - do not modify the last seen time, unless it was None, and /// - do not modify the last seen time, unless it was None, and
/// - only modify the services after a response or failure. /// - only modify the services after a response or failure.
#[test] #[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(); zebra_test::init();
for change in changes { for change in changes {
@ -211,14 +210,19 @@ proptest! {
if addr.untrusted_last_seen.is_some() { if addr.untrusted_last_seen.is_some() {
prop_assert_eq!(changed_addr.untrusted_last_seen, addr.untrusted_last_seen); prop_assert_eq!(changed_addr.untrusted_last_seen, addr.untrusted_last_seen);
} else { } 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: // services:
// check that we only change if there was a handshake // check that we only change if there was a handshake
if changed_addr.last_connection_state.is_never_attempted() if addr.services.is_some()
|| changed_addr.last_connection_state == AttemptPending && (changed_addr.last_connection_state.is_never_attempted()
|| change.untrusted_services().is_none() { || changed_addr.last_connection_state == AttemptPending
|| change.untrusted_services().is_none())
{
prop_assert_eq!(changed_addr.services, addr.services); 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 `change` is invalid for the current MetaAddr state, skip it.
if let Some(changed_addr) = change.apply_to_meta_addr(addr) { 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; addr = changed_addr;
} }
} }
@ -276,16 +280,15 @@ proptest! {
) { ) {
zebra_test::init(); zebra_test::init();
let address_book = AddressBook::new_with_addrs( let address_book =
local_listener, AddressBook::new_with_addrs(local_listener, Span::none(), address_book_addrs);
Span::none(),
address_book_addrs
);
let sanitized_addrs = address_book.sanitized(); let sanitized_addrs = address_book.sanitized();
let expected_local_listener = address_book.local_listener_meta_addr(); let expected_local_listener = address_book.local_listener_meta_addr();
let canonical_local_listener = canonical_socket_addr(local_listener); 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, // invalid addresses should be removed by sanitization,
// regardless of where they have come from // regardless of where they have come from
@ -325,9 +328,10 @@ proptest! {
// Run the test for this much simulated time // Run the test for this much simulated time
let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS; let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS;
// Advance the clock by this much for every peer change // 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, u32::try_from(MAX_ADDR_CHANGE).unwrap() >= 3 * LIVE_PEER_INTERVALS,
"there are enough changes for good test coverage", "there are enough changes for good test coverage",
); );
@ -365,10 +369,10 @@ proptest! {
for (i, change) in changes.into_iter().enumerate() { for (i, change) in changes.into_iter().enumerate() {
while let Some(candidate_addr) = candidate_set.next().await { 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; attempt_count += 1;
assert!( prop_assert!(
attempt_count <= 1, attempt_count <= 1,
"candidate: {:?}, change: {}, now: {:?}, earliest next attempt: {:?}, \ "candidate: {:?}, change: {}, now: {:?}, earliest next attempt: {:?}, \
attempts: {}, live peer interval limit: {}, test time limit: {:?}, \ attempts: {}, live peer interval limit: {}, test time limit: {:?}, \
@ -395,7 +399,9 @@ proptest! {
attempt_count = 0; attempt_count = 0;
} }
} }
});
Ok(())
})?;
} }
/// Make sure that all disconnected [`MetaAddr`]s are retried once, before /// Make sure that all disconnected [`MetaAddr`]s are retried once, before
@ -420,9 +426,10 @@ proptest! {
// Run the test for this much simulated time // Run the test for this much simulated time
let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS; let overall_test_time: Duration = MIN_PEER_RECONNECTION_DELAY * LIVE_PEER_INTERVALS;
// Advance the clock by this much for every peer change // 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, u32::try_from(MAX_ADDR_CHANGE).unwrap() >= 3 * LIVE_PEER_INTERVALS,
"there are enough changes for good test coverage", "there are enough changes for good test coverage",
); );
@ -449,7 +456,9 @@ proptest! {
while addr.is_ready_for_connection_attempt() { while addr.is_ready_for_connection_attempt() {
*attempt_counts.entry(addr.addr).or_default() += 1; *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 // Simulate an attempt
*addr = MetaAddr::new_reconnect(&addr.addr) *addr = MetaAddr::new_reconnect(&addr.addr)
@ -461,17 +470,18 @@ proptest! {
// If we've run out of changes for this addr, do nothing. // If we've run out of changes for this addr, do nothing.
if let Some(changed_addr) = change if let Some(changed_addr) = change
.map(|change| change.apply_to_meta_addr(*addr)) .map(|change| change.apply_to_meta_addr(*addr))
.flatten() { .flatten()
assert_eq!(changed_addr.addr, addr.addr); {
*addr = changed_addr; prop_assert_eq!(changed_addr.addr, addr.addr);
} *addr = changed_addr;
}
} }
tokio::time::advance(peer_change_interval).await; tokio::time::advance(peer_change_interval).await;
} }
attempt_counts Ok(attempt_counts)
}); })?;
let min_attempts = attempt_counts.values().min(); let min_attempts = attempt_counts.values().min();
let max_attempts = attempt_counts.values().max(); let max_attempts = attempt_counts.values().max();

7
zebra-network/src/peer_set/candidate_set.rs
View File

@ -267,7 +267,12 @@ where
/// Add new `addrs` to the address book. /// Add new `addrs` to the address book.
fn send_addrs(&self, addrs: impl IntoIterator<Item = MetaAddr>) { fn send_addrs(&self, addrs: impl IntoIterator<Item = MetaAddr>) {
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 // # Correctness
// //

4
zebra-network/src/peer_set/initialize/tests/vectors.rs
View File

@ -1260,7 +1260,9 @@ where
let addr = let addr =
MetaAddr::new_gossiped_meta_addr(addr, PeerServices::NODE_NETWORK, DateTime32::now()); MetaAddr::new_gossiped_meta_addr(addr, PeerServices::NODE_NETWORK, DateTime32::now());
fake_peer = Some(addr); 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); address_book.update(addr);
} }