From 8c178c3ee44ff2a61b21dbf0be8545c21f6e60c2 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 27 May 2020 17:40:12 -0700 Subject: [PATCH] fix panic in seed subcommand (#401) Co-authored-by: Jane Lusby Prior to this change, the seed subcommand would consistently encounter a panic in one of the background tasks, but would continue running after the panic. This is indicative of two bugs. First, zebrad was not configured to treat panics as non recoverable and instead defaulted to the tokio defaults, which are to catch panics in tasks and return them via the join handle if available, or to print them if the join handle has been discarded. This is likely a poor fit for zebrad as an application, we do not need to maximize uptime or minimize the extent of an outage should one of our tasks / services start encountering panics. Ignoring a panic increases our risk of observing invalid state, causing all sorts of wild and bad bugs. To deal with this we've switched the default panic behavior from `unwind` to `abort`. This makes panics fail immediately and take down the entire application, regardless of where they occur, which is consistent with our treatment of misbehaving connections. The second bug is the panic itself. This was triggered by a duplicate entry in the initial_peers set. To fix this we've switched the storage for the peers from a `Vec` to a `HashSet`, which has similar properties but guarantees uniqueness of its keys. --- Cargo.toml | 6 ++++++ zebra-network/src/config.rs | 12 ++++++------ zebra-network/src/peer_set/initialize.rs | 2 +- zebra-network/src/peer_set/set.rs | 1 + zebrad/src/application.rs | 4 +++- zebrad/src/commands/connect.rs | 2 +- 6 files changed, 18 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 04c64569..5937b598 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,3 +9,9 @@ members = [ "zebra-client", "zebrad", ] + +[profile.dev] +panic = "abort" + +[profile.release] +panic = "abort" \ No newline at end of file diff --git a/zebra-network/src/config.rs b/zebra-network/src/config.rs index a71c21ce..d8e47ade 100644 --- a/zebra-network/src/config.rs +++ b/zebra-network/src/config.rs @@ -1,7 +1,7 @@ use std::{ net::{SocketAddr, ToSocketAddrs}, string::String, - time::Duration, + time::Duration, collections::HashSet, }; use zebra_chain::Network; @@ -21,11 +21,11 @@ pub struct Config { /// A list of initial peers for the peerset when operating on /// mainnet. - pub initial_mainnet_peers: Vec, + pub initial_mainnet_peers: HashSet, /// A list of initial peers for the peerset when operating on /// testnet. - pub initial_testnet_peers: Vec, + pub initial_testnet_peers: HashSet, /// The outgoing request buffer size for the peer set. pub peerset_request_buffer_size: usize, @@ -49,16 +49,16 @@ pub struct Config { } impl Config { - fn parse_peers(peers: Vec) -> Vec { + fn parse_peers(peers: HashSet) -> HashSet { peers .iter() .flat_map(|s| s.to_socket_addrs()) .flatten() - .collect::>() + .collect() } /// Get the initial seed peers based on the configured network. - pub fn initial_peers(&self) -> Vec { + pub fn initial_peers(&self) -> HashSet { match self.network { Network::Mainnet => Config::parse_peers(self.initial_mainnet_peers.clone()), Network::Testnet => Config::parse_peers(self.initial_testnet_peers.clone()), diff --git a/zebra-network/src/peer_set/initialize.rs b/zebra-network/src/peer_set/initialize.rs index 7dc2c1b4..1327d50b 100644 --- a/zebra-network/src/peer_set/initialize.rs +++ b/zebra-network/src/peer_set/initialize.rs @@ -148,7 +148,7 @@ where /// the results over `tx`. #[instrument(skip(initial_peers, connector, tx))] async fn add_initial_peers( - initial_peers: Vec, + initial_peers: std::collections::HashSet, connector: S, mut tx: mpsc::Sender, ) where diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index 9930657f..eeedb98e 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -163,6 +163,7 @@ where self.ready_services.insert(key, svc); } Poll::Ready(Some(Err((key, UnreadyError::Canceled)))) => { + trace!(?key, "service was canceled"); debug_assert!(!self.cancel_handles.contains_key(&key)) } Poll::Ready(Some(Err((key, UnreadyError::Inner(e))))) => { diff --git a/zebrad/src/application.rs b/zebrad/src/application.rs index 5291b9a2..84a3eb7a 100644 --- a/zebrad/src/application.rs +++ b/zebrad/src/application.rs @@ -108,7 +108,9 @@ impl Application for ZebradApp { /// Get logging configuration from command-line options fn tracing_config(&self, command: &EntryPoint) -> trace::Config { - if command.verbose { + if let Ok(env) = std::env::var("ZEBRAD_LOG") { + trace::Config::from(env) + } else if command.verbose { trace::Config::verbose() } else { trace::Config::default() diff --git a/zebrad/src/commands/connect.rs b/zebrad/src/commands/connect.rs index a599f009..a76f8808 100644 --- a/zebrad/src/commands/connect.rs +++ b/zebrad/src/commands/connect.rs @@ -60,7 +60,7 @@ impl ConnectCmd { // Use a different listen addr so that we don't conflict with another local node. config.listen_addr = "127.0.0.1:38233".parse().unwrap(); // Connect only to the specified peer. - config.initial_mainnet_peers = vec![self.addr.to_string()]; + config.initial_mainnet_peers.insert(self.addr.to_string()); let (mut peer_set, _address_book) = zebra_network::init(config, node).await; let mut retry_peer_set =