From b0ef121e91209e1c69ddefa0765d7a93d1dfb569 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 11:10:33 +0100 Subject: [PATCH 1/3] bitcoind: accurate connection sanity checks at startup The current checks were only testing whether a bitcoind was at all reachable. We need to make sure it is also listening to requests (and not warming up) in order for the assumptions made later on (we panic on requests if we retried for too long) to hold. This makes the startup sanity checks also wait for bitcoind to get out of warming up mode. --- src/bitcoin/d/mod.rs | 68 ++++++++++++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 9394adb9..af282dcc 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -9,7 +9,9 @@ use crate::{ }; use utils::block_before_date; -use std::{cmp, collections::HashSet, convert::TryInto, fs, io, str::FromStr, time::Duration}; +use std::{ + cmp, collections::HashSet, convert::TryInto, fs, io, str::FromStr, thread, time::Duration, +}; use jsonrpc::{ arg, @@ -177,8 +179,9 @@ impl BitcoinD { ) -> Result { let cookie_string = fs::read_to_string(&config.cookie_path).map_err(BitcoindError::CookieFile)?; + let watchonly_url = format!("http://{}/wallet/{}", config.addr, watchonly_wallet_path); - // Create a dummy client with a low timeout first to test the connection + // Create a dummy bitcoind with clients using a low timeout to sanity check the connection. let dummy_node_client = Client::with_transport( SimpleHttpTransport::builder() .url(&config.addr.to_string()) @@ -187,35 +190,35 @@ impl BitcoinD { .cookie_auth(cookie_string.clone()) .build(), ); - let req = dummy_node_client.build_request("echo", &[]); - dummy_node_client.send_request(req.clone())?; - - let node_client = Client::with_transport( + let sendonly_client = Client::with_transport( SimpleHttpTransport::builder() - .url(&config.addr.to_string()) + .url(&watchonly_url) .map_err(BitcoindError::from)? - .timeout(Duration::from_secs(RPC_SOCKET_TIMEOUT)) + .timeout(Duration::from_secs(1)) .cookie_auth(cookie_string.clone()) .build(), ); - - // Create a dummy client with a low timeout first to test the connection - let url = format!("http://{}/wallet/{}", config.addr, watchonly_wallet_path); let dummy_wo_client = Client::with_transport( SimpleHttpTransport::builder() - .url(&url) + .url(&watchonly_url) .map_err(BitcoindError::from)? .timeout(Duration::from_secs(3)) .cookie_auth(cookie_string.clone()) .build(), ); - let req = dummy_wo_client.build_request("echo", &[]); - dummy_wo_client.send_request(req.clone())?; + let dummy_bitcoind = BitcoinD { + node_client: dummy_node_client, + sendonly_client, + watchonly_client: dummy_wo_client, + watchonly_wallet_path: watchonly_wallet_path.clone(), + retries: 0, + }; + dummy_bitcoind.check_connection()?; - let watchonly_url = format!("http://{}/wallet/{}", config.addr, watchonly_wallet_path); - let watchonly_client = Client::with_transport( + // Now the connection is checked, create the clients with an appropriate timeout. + let node_client = Client::with_transport( SimpleHttpTransport::builder() - .url(&watchonly_url) + .url(&config.addr.to_string()) .map_err(BitcoindError::from)? .timeout(Duration::from_secs(RPC_SOCKET_TIMEOUT)) .cookie_auth(cookie_string.clone()) @@ -226,10 +229,17 @@ impl BitcoinD { .url(&watchonly_url) .map_err(BitcoindError::from)? .timeout(Duration::from_secs(1)) + .cookie_auth(cookie_string.clone()) + .build(), + ); + let watchonly_client = Client::with_transport( + SimpleHttpTransport::builder() + .url(&watchonly_url) + .map_err(BitcoindError::from)? + .timeout(Duration::from_secs(RPC_SOCKET_TIMEOUT)) .cookie_auth(cookie_string) .build(), ); - Ok(BitcoinD { node_client, sendonly_client, @@ -239,6 +249,27 @@ impl BitcoinD { }) } + fn check_client(&self, client: &Client) -> Result<(), BitcoindError> { + if let Err(e) = self.make_request(client, "echo", &[]) { + if e.is_warming_up() { + log::info!("bitcoind is warming up. Retrying connection sanity check in 1 second."); + thread::sleep(Duration::from_secs(1)); + return self.check_client(client); + } else { + return Err(e); + } + } + Ok(()) + } + + // Make sure bitcoind is reachable through all clients. Note we don't check the sendonly client + // since it has precisely a very low timeout for the purpose of ignoring responses. + fn check_connection(&self) -> Result<(), BitcoindError> { + self.check_client(&self.node_client)?; + self.check_client(&self.watchonly_client)?; + Ok(()) + } + /// Set how many times we'll retry a failed request. If passed None will set to default. pub fn with_retry_limit(mut self, retry_limit: Option) -> Self { self.retries = retry_limit.unwrap_or(BITCOIND_RETRY_LIMIT); @@ -531,6 +562,7 @@ impl BitcoinD { /// Try to load the watchonly wallet in bitcoind. It will continue on error (since it's /// likely the wallet is just already loaded) and log it as info instead. pub fn try_load_watchonly_wallet(&self) { + // TODO: check if it's not loaded instead of blindly trying to load it. if let Err(e) = self.make_fallible_node_request( "loadwallet", ¶ms!(Json::String(self.watchonly_wallet_path.clone()),), From 13b2b8eef4dd5deafe04b2330b469067afd0b12c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 11:26:11 +0100 Subject: [PATCH 2/3] bitcoind: immediately set the default retry limit We were previously adding it later on to fail early on startup error. This didn't handle well "warming up" errors. Now that it was reachable and listening to requests already, we can just use the real retry limit from the get-go. --- src/bitcoin/d/mod.rs | 8 +------- src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index af282dcc..2d917fc7 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -245,7 +245,7 @@ impl BitcoinD { sendonly_client, watchonly_client, watchonly_wallet_path, - retries: 0, + retries: BITCOIND_RETRY_LIMIT, }) } @@ -270,12 +270,6 @@ impl BitcoinD { Ok(()) } - /// Set how many times we'll retry a failed request. If passed None will set to default. - pub fn with_retry_limit(mut self, retry_limit: Option) -> Self { - self.retries = retry_limit.unwrap_or(BITCOIND_RETRY_LIMIT); - self - } - /// Wrapper to retry a request sent to bitcoind upon IO failure /// according to the configured number of retries. fn retry Result>( diff --git a/src/lib.rs b/src/lib.rs index 5a411824..a2cb7319 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -207,7 +207,7 @@ fn setup_bitcoind( bitcoind.sanity_check(&config.main_descriptor, config.bitcoin_config.network)?; log::info!("Connection to bitcoind established and checked."); - Ok(bitcoind.with_retry_limit(None)) + Ok(bitcoind) } #[derive(Clone)] From 0306773cdca15009d3bf2c6eb0c3ebd8e7fb7989 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 8 Dec 2022 11:26:48 +0100 Subject: [PATCH 3/3] daemon: log and exit 1 on startup error, don't panic --- src/bin/daemon.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/daemon.rs b/src/bin/daemon.rs index 7aeae860..04a4af2d 100644 --- a/src/bin/daemon.rs +++ b/src/bin/daemon.rs @@ -60,8 +60,8 @@ fn main() { }); let daemon = DaemonHandle::start_default(config).unwrap_or_else(|e| { - // The panic hook will log::error - panic!("Starting Liana daemon: {}", e); + log::error!("Error starting Liana daemon: {}", e); + process::exit(1); }); daemon .rpc_server()