From 65f87ab4f523127a6f8a1392060eaf3a0b742041 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 09:48:10 +0100 Subject: [PATCH 01/11] poller: make the updating process into its own function. --- src/bitcoin/poller/looper.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 0b984d86c..49786be53 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -325,6 +325,17 @@ fn sync_poll_interval() -> time::Duration { time::Duration::from_secs(0) } +/// Update our state from the Bitcoin backend. +pub fn poll( + bit: &sync::Arc>, + db: &sync::Arc>, + secp: &secp256k1::Secp256k1, + descs: &[descriptors::SinglePathLianaDesc], +) { + updates(bit, db, descs, secp); + rescan_check(bit, db, descs, secp); +} + /// Main event loop. Repeatedly polls the Bitcoin interface until told to stop through the /// `shutdown` atomic. pub fn looper( @@ -378,7 +389,6 @@ pub fn looper( } } - updates(&bit, &db, &descs, &secp); - rescan_check(&bit, &db, &descs, &secp); + poll(&bit, &db, &secp, &descs); } } From bdbedb8802a4e452c4220977f27facd9dd50330a Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 09:56:52 +0100 Subject: [PATCH 02/11] poller: hold the same database lock across one update round --- src/bitcoin/poller/looper.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 49786be53..033b9cc25 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -208,13 +208,11 @@ fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdat } fn updates( + db_conn: &mut Box, bit: &impl BitcoinInterface, - db: &impl DatabaseInterface, descs: &[descriptors::SinglePathLianaDesc], secp: &secp256k1::Secp256k1, ) { - let mut db_conn = db.connection(); - // Check if there was a new block before updating ourselves. let current_tip = db_conn.chain_tip().expect("Always set at first startup"); let latest_tip = match new_tip(bit, ¤t_tip) { @@ -225,18 +223,18 @@ fn updates( // between our former chain and the new one, then restart fresh. db_conn.rollback_tip(&new_tip); log::info!("Tip was rolled back to '{}'.", new_tip); - return updates(bit, db, descs, secp); + return updates(db_conn, bit, descs, secp); } }; // Then check the state of our coins. Do it even if the tip did not change since last poll, as // we may have unconfirmed transactions. - let updated_coins = update_coins(bit, &mut db_conn, ¤t_tip, descs, secp); + let updated_coins = update_coins(bit, db_conn, ¤t_tip, descs, secp); // If the tip changed while we were polling our Bitcoin interface, start over. if bit.chain_tip() != latest_tip { log::info!("Chain tip changed while we were updating our state. Starting over."); - return updates(bit, db, descs, secp); + return updates(db_conn, bit, descs, secp); } // The chain tip did not change since we started our updates. Record them and the latest tip. @@ -258,13 +256,12 @@ fn updates( // Check if there is any rescan of the backend ongoing or one that just finished. fn rescan_check( + db_conn: &mut Box, bit: &impl BitcoinInterface, - db: &impl DatabaseInterface, descs: &[descriptors::SinglePathLianaDesc], secp: &secp256k1::Secp256k1, ) { log::debug!("Checking the state of an ongoing rescan if there is any"); - let mut db_conn = db.connection(); // Check if there is an ongoing rescan. If there isn't and we previously asked for a rescan of // the backend, we treat it as completed. @@ -299,7 +296,7 @@ fn rescan_check( "Rolling back our internal tip to '{}' to update our internal state with past transactions.", rescan_tip ); - updates(bit, db, descs, secp) + updates(db_conn, bit, descs, secp) } else { log::debug!("No ongoing rescan."); } @@ -332,8 +329,9 @@ pub fn poll( secp: &secp256k1::Secp256k1, descs: &[descriptors::SinglePathLianaDesc], ) { - updates(bit, db, descs, secp); - rescan_check(bit, db, descs, secp); + let mut db_conn = db.connection(); + updates(&mut db_conn, bit, descs, secp); + rescan_check(&mut db_conn, bit, descs, secp); } /// Main event loop. Repeatedly polls the Bitcoin interface until told to stop through the From d8c59e30eda74df3e2fe5801207d806d4e4089ba Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 10:49:56 +0100 Subject: [PATCH 03/11] lib: externalize the Bitcoin poller thread handling The caller is now responsible for running the poller in a thread. --- src/bin/daemon.rs | 24 +++++++- src/bitcoin/poller/looper.rs | 69 ++------------------- src/bitcoin/poller/mod.rs | 110 +++++++++++++++++++++++----------- src/lib.rs | 112 ++++++++++++++--------------------- src/testutils.rs | 5 +- 5 files changed, 146 insertions(+), 174 deletions(-) diff --git a/src/bin/daemon.rs b/src/bin/daemon.rs index 51e3c1347..1e8a4c819 100644 --- a/src/bin/daemon.rs +++ b/src/bin/daemon.rs @@ -2,7 +2,9 @@ use std::{ env, io::{self, Write}, path::PathBuf, - process, thread, time, + process, + sync::{atomic, Arc}, + thread, time, }; use liana::{config::Config, DaemonHandle}; @@ -70,13 +72,29 @@ fn main() { process::exit(1); }); - let daemon = DaemonHandle::start_default(config).unwrap_or_else(|e| { + let poll_interval = config.bitcoin_config.poll_interval_secs; + let DaemonHandle { + control, + bitcoin_poller, + } = DaemonHandle::start_default(config).unwrap_or_else(|e| { log::error!("Error starting Liana daemon: {}", e); process::exit(1); }); - daemon + let poller_shutdown = Arc::from(atomic::AtomicBool::from(false)); + let poller_thread = thread::Builder::new() + .name("Bitcoin Core poller".to_string()) + .spawn({ + let shutdown = poller_shutdown.clone(); + move || bitcoin_poller.poll_forever(poll_interval, shutdown) + }) + .expect("Spawning the poller thread must never fail."); + control .rpc_server() .expect("JSONRPC server must terminate cleanly"); + poller_shutdown.store(true, atomic::Ordering::Relaxed); + poller_thread + .join() + .expect("Bitcoin Core poller must terminate cleanly"); // We are always logging to stdout, should it be then piped to the log file (if self) or // not. So just make sure that all messages were actually written. diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 033b9cc25..83111c4a8 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -4,11 +4,7 @@ use crate::{ descriptors, }; -use std::{ - collections::HashSet, - sync::{self, atomic}, - thread, time, -}; +use std::{collections::HashSet, sync, time}; use miniscript::bitcoin::{self, secp256k1}; @@ -302,8 +298,8 @@ fn rescan_check( } } -// If the database chain tip is NULL (first startup), initialize it. -fn maybe_initialize_tip(bit: &impl BitcoinInterface, db: &impl DatabaseInterface) { +/// If the database chain tip is NULL (first startup), initialize it. +pub fn maybe_initialize_tip(bit: &impl BitcoinInterface, db: &impl DatabaseInterface) { let mut db_conn = db.connection(); if db_conn.chain_tip().is_none() { @@ -312,7 +308,7 @@ fn maybe_initialize_tip(bit: &impl BitcoinInterface, db: &impl DatabaseInterface } } -fn sync_poll_interval() -> time::Duration { +pub fn sync_poll_interval() -> time::Duration { // TODO: be smarter, like in revaultd, but more generic too. #[cfg(not(test))] { @@ -333,60 +329,3 @@ pub fn poll( updates(&mut db_conn, bit, descs, secp); rescan_check(&mut db_conn, bit, descs, secp); } - -/// Main event loop. Repeatedly polls the Bitcoin interface until told to stop through the -/// `shutdown` atomic. -pub fn looper( - bit: sync::Arc>, - db: sync::Arc>, - shutdown: sync::Arc, - poll_interval: time::Duration, - desc: descriptors::LianaDescriptor, -) { - let mut last_poll = None; - let mut synced = false; - let descs = [ - desc.receive_descriptor().clone(), - desc.change_descriptor().clone(), - ]; - let secp = secp256k1::Secp256k1::verification_only(); - - maybe_initialize_tip(&bit, &db); - - while !shutdown.load(atomic::Ordering::Relaxed) || last_poll.is_none() { - let now = time::Instant::now(); - - if let Some(last_poll) = last_poll { - let time_since_poll = now.duration_since(last_poll); - let poll_interval = if synced { - poll_interval - } else { - // Until we are synced we poll less often to avoid harassing bitcoind and impeding - // the sync. As a function since it's mocked for the tests. - sync_poll_interval() - }; - if time_since_poll < poll_interval { - thread::sleep(time::Duration::from_millis(500)); - continue; - } - } - last_poll = Some(now); - - // Don't poll until the Bitcoin backend is fully synced. - if !synced { - let progress = bit.sync_progress(); - log::info!( - "Block chain synchronization progress: {:.2}% ({} blocks / {} headers)", - progress.rounded_up_progress() * 100.0, - progress.blocks, - progress.headers - ); - synced = progress.is_complete(); - if !synced { - continue; - } - } - - poll(&bit, &db, &secp, &descs); - } -} diff --git a/src/bitcoin/poller/mod.rs b/src/bitcoin/poller/mod.rs index 7731af2e1..522021476 100644 --- a/src/bitcoin/poller/mod.rs +++ b/src/bitcoin/poller/mod.rs @@ -1,60 +1,100 @@ mod looper; -use crate::{ - bitcoin::{poller::looper::looper, BitcoinInterface}, - database::DatabaseInterface, - descriptors, -}; +use crate::{bitcoin::BitcoinInterface, database::DatabaseInterface, descriptors}; use std::{ sync::{self, atomic}, thread, time, }; +use miniscript::bitcoin::secp256k1; + /// The Bitcoin poller handler. pub struct Poller { - handle: thread::JoinHandle<()>, - shutdown: sync::Arc, + bit: sync::Arc>, + db: sync::Arc>, + secp: secp256k1::Secp256k1, + // The receive and change descriptors (in this order). + descs: [descriptors::SinglePathLianaDesc; 2], } impl Poller { - pub fn start( + pub fn new( bit: sync::Arc>, db: sync::Arc>, - poll_interval: time::Duration, desc: descriptors::LianaDescriptor, ) -> Poller { - let shutdown = sync::Arc::from(atomic::AtomicBool::from(false)); - let handle = thread::Builder::new() - .name("Bitcoin poller".to_string()) - .spawn({ - let shutdown = shutdown.clone(); - move || looper(bit, db, shutdown, poll_interval, desc) - }) - .expect("Must not fail"); - - Poller { shutdown, handle } - } + let secp = secp256k1::Secp256k1::verification_only(); + let descs = [ + desc.receive_descriptor().clone(), + desc.change_descriptor().clone(), + ]; - pub fn trigger_stop(&self) { - self.shutdown.store(true, atomic::Ordering::Relaxed); - } + // On first startup the tip may be NULL. Make sure it's set as the poller relies on it. + looper::maybe_initialize_tip(&bit, &db); - pub fn stop(self) { - self.trigger_stop(); - self.handle.join().expect("The poller loop must not fail"); + Poller { + bit, + db, + secp, + descs, + } } - #[cfg(feature = "nonblocking_shutdown")] - pub fn is_stopped(&self) -> bool { - // Doc says "This might return true for a brief moment after the thread’s main function has - // returned, but before the thread itself has stopped running.". But it's not an issue for - // us, as long as the main poller function has returned we are good. - self.handle.is_finished() + /// Update our state from the Bitcoin backend. + pub fn poll_once(&self) { + looper::poll(&self.bit, &self.db, &self.secp, &self.descs) } - #[cfg(test)] - pub fn test_stop(&mut self) { - self.shutdown.store(true, atomic::Ordering::Relaxed); + /// Continuously update our state from the Bitcoin backend. + /// - `poll_interval`: how frequently to perform an update. + /// - `shutdown`: set to true to stop continuously updating and make this function return. + /// + /// Typically this would run for the whole duration of the program in a thread, and the main + /// thread would set the `shutdown` atomic to `true` when shutting down. + pub fn poll_forever( + &self, + poll_interval: time::Duration, + shutdown: sync::Arc, + ) { + let mut last_poll = None; + let mut synced = false; + + while !shutdown.load(atomic::Ordering::Relaxed) || last_poll.is_none() { + let now = time::Instant::now(); + + if let Some(last_poll) = last_poll { + let time_since_poll = now.duration_since(last_poll); + let poll_interval = if synced { + poll_interval + } else { + // Until we are synced we poll less often to avoid harassing bitcoind and impeding + // the sync. As a function since it's mocked for the tests. + looper::sync_poll_interval() + }; + if time_since_poll < poll_interval { + thread::sleep(time::Duration::from_millis(500)); + continue; + } + } + last_poll = Some(now); + + // Don't poll until the Bitcoin backend is fully synced. + if !synced { + let progress = self.bit.sync_progress(); + log::info!( + "Block chain synchronization progress: {:.2}% ({} blocks / {} headers)", + progress.rounded_up_progress() * 100.0, + progress.blocks, + progress.headers + ); + synced = progress.is_complete(); + if !synced { + continue; + } + } + + looper::poll(&self.bit, &self.db, &self.secp, &self.descs); + } } } diff --git a/src/lib.rs b/src/lib.rs index a58b6b30a..1d6244ae1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -272,6 +272,28 @@ impl DaemonControl { } } + /// Start the JSONRPC server and listen for incoming commands until we die. + #[cfg(feature = "daemon")] + pub fn rpc_server(self) -> Result<(), io::Error> { + let rpc_socket: path::PathBuf = [ + self.config + .data_dir() + .expect("Didn't fail at startup, must not now") + .as_path(), + path::Path::new(&self.config.bitcoin_config.network.to_string()), + path::Path::new("lianad_rpc"), + ] + .iter() + .collect(); + let listener = rpcserver_setup(&rpc_socket)?; + log::info!("JSONRPC server started."); + + rpcserver_loop(listener, self)?; + log::info!("JSONRPC server stopped."); + + Ok(()) + } + // Useful for unit test to directly mess up with the DB #[cfg(test)] pub fn db(&self) -> sync::Arc> { @@ -281,7 +303,7 @@ impl DaemonControl { pub struct DaemonHandle { pub control: DaemonControl, - bitcoin_poller: poller::Poller, + pub bitcoin_poller: poller::Poller, } impl DaemonHandle { @@ -350,13 +372,9 @@ impl DaemonHandle { } } - // Spawn the bitcoind poller with a retry limit high enough that we'd fail after that. - let bitcoin_poller = poller::Poller::start( - bit.clone(), - db.clone(), - config.bitcoin_config.poll_interval_secs, - config.main_descriptor.clone(), - ); + // Setup the Bitcoin poller. Caller is responsible for running it. + let bitcoin_poller = + poller::Poller::new(bit.clone(), db.clone(), config.main_descriptor.clone()); // Finally, set up the API. let control = DaemonControl::new(config, bit, db, secp); @@ -372,61 +390,6 @@ impl DaemonHandle { pub fn start_default(config: Config) -> Result { DaemonHandle::start(config, Option::::None, Option::::None) } - - /// Start the JSONRPC server and listen for incoming commands until we die. - /// Like DaemonHandle::shutdown(), this stops the Bitcoin poller at teardown. - #[cfg(feature = "daemon")] - pub fn rpc_server(self) -> Result<(), io::Error> { - let DaemonHandle { - control, - bitcoin_poller: poller, - } = self; - - let rpc_socket: path::PathBuf = [ - control - .config - .data_dir() - .expect("Didn't fail at startup, must not now") - .as_path(), - path::Path::new(&control.config.bitcoin_config.network.to_string()), - path::Path::new("lianad_rpc"), - ] - .iter() - .collect(); - let listener = rpcserver_setup(&rpc_socket)?; - log::info!("JSONRPC server started."); - - rpcserver_loop(listener, control)?; - log::info!("JSONRPC server stopped."); - - poller.stop(); - - Ok(()) - } - - /// Shut down the Liana daemon. - pub fn shutdown(self) { - self.bitcoin_poller.stop(); - } - - /// Tell the daemon to shut down. This will return before the shutdown completes. The structure - /// must not be reused after triggering shutdown. - #[cfg(feature = "nonblocking_shutdown")] - pub fn trigger_shutdown(&self) { - self.bitcoin_poller.trigger_stop() - } - - /// Whether the daemon has finished shutting down. - #[cfg(feature = "nonblocking_shutdown")] - pub fn shutdown_complete(&self) -> bool { - self.bitcoin_poller.is_stopped() - } - - // We need a shutdown utility that does not move for implementing Drop for the DummyLiana - #[cfg(test)] - pub fn test_shutdown(&mut self) { - self.bitcoin_poller.test_stop(); - } } #[cfg(all(test, unix))] @@ -678,11 +641,15 @@ mod tests { }; // Start the daemon in a new thread so the current one acts as the bitcoind server. + let shutdown = sync::Arc::from(sync::atomic::AtomicBool::from(false)); let daemon_thread = thread::spawn({ + let shutdown = shutdown.clone(); let config = config.clone(); move || { - let handle = DaemonHandle::start_default(config).unwrap(); - handle.shutdown(); + let poll_interval = config.bitcoin_config.poll_interval_secs; + let DaemonHandle { bitcoin_poller, .. } = + DaemonHandle::start_default(config).unwrap(); + bitcoin_poller.poll_forever(poll_interval, shutdown) } }); complete_sanity_check(&server); @@ -694,12 +661,20 @@ mod tests { complete_desc_check(&server, &receive_desc.to_string(), &change_desc.to_string()); complete_tip_init(&server); complete_sync_check(&server); + shutdown.store(true, sync::atomic::Ordering::Relaxed); daemon_thread.join().unwrap(); // The datadir is created now, so if we restart it it won't create the wo wallet. - let daemon_thread = thread::spawn(move || { - let handle = DaemonHandle::start_default(config).unwrap(); - handle.shutdown(); + shutdown.store(false, sync::atomic::Ordering::Relaxed); + let daemon_thread = thread::spawn({ + let shutdown = shutdown.clone(); + let config = config.clone(); + move || { + let poll_interval = config.bitcoin_config.poll_interval_secs; + let DaemonHandle { bitcoin_poller, .. } = + DaemonHandle::start_default(config).unwrap(); + bitcoin_poller.poll_forever(poll_interval, shutdown) + } }); complete_sanity_check(&server); complete_version_check(&server); @@ -708,6 +683,7 @@ mod tests { complete_wallet_check(&server, &wo_path); complete_desc_check(&server, &receive_desc.to_string(), &change_desc.to_string()); complete_sync_check(&server); + shutdown.store(true, sync::atomic::Ordering::Relaxed); daemon_thread.join().unwrap(); fs::remove_dir_all(&tmp_dir).unwrap(); diff --git a/src/testutils.rs b/src/testutils.rs index c175c1566..2aa8bcafb 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -480,13 +480,12 @@ impl DummyLiana { #[cfg(feature = "daemon")] pub fn rpc_server(self) -> Result<(), io::Error> { - self.handle.rpc_server()?; + self.handle.control.rpc_server()?; fs::remove_dir_all(&self.tmp_dir)?; Ok(()) } pub fn shutdown(self) { - self.handle.shutdown(); - fs::remove_dir_all(&self.tmp_dir).unwrap(); + fs::remove_dir_all(self.tmp_dir).unwrap(); } } From 706b7eed97c8a1e00d340475485165e0c9c8544e Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 10:52:39 +0100 Subject: [PATCH 04/11] lib: add a TODO to harmonize the interface of the JSONRPC server with the Bitcoin poller We won't do it here to avoid further increasing a large diff. --- src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 1d6244ae1..bf70c92b6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -272,6 +272,9 @@ impl DaemonControl { } } + // TODO: Likewise poller::Poller, this could go into its own jsonrpc::Server structure, with a + // new() which would do the setup and a run_forever() which would take a shutdown AtomicBool + // parameter. /// Start the JSONRPC server and listen for incoming commands until we die. #[cfg(feature = "daemon")] pub fn rpc_server(self) -> Result<(), io::Error> { From 82bec1ef781eadf10c4be348272b535b84f8fd87 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 15:24:14 +0100 Subject: [PATCH 05/11] bitcoind: make all node requests faillible For now this just moves the expect()s one layer up. --- src/bitcoin/d/mod.rs | 169 +++++++++++++++++++++++-------------------- src/bitcoin/mod.rs | 32 ++++++-- 2 files changed, 116 insertions(+), 85 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index a1b1317c4..86e5a643c 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -436,21 +436,12 @@ impl BitcoinD { } } - fn make_fallible_node_request( - &self, - method: &str, - params: Option<&serde_json::value::RawValue>, - ) -> Result { - self.make_request(&self.node_client, method, params) - } - fn make_node_request( &self, method: &str, params: Option<&serde_json::value::RawValue>, - ) -> Json { + ) -> Result { self.make_request(&self.node_client, method, params) - .expect("We must not fail to make a request for more than a minute") } fn make_wallet_request( @@ -470,23 +461,26 @@ impl BitcoinD { self.make_request(&self.watchonly_client, method, params) } - fn get_bitcoind_version(&self) -> u64 { - self.make_node_request("getnetworkinfo", None) + fn get_bitcoind_version(&self) -> Result { + Ok(self + .make_node_request("getnetworkinfo", None)? .get("version") .and_then(Json::as_u64) - .expect("Missing or invalid 'version' in 'getnetworkinfo' result?") + .expect("Missing or invalid 'version' in 'getnetworkinfo' result?")) } - fn get_network_bip70(&self) -> String { - self.make_node_request("getblockchaininfo", None) + fn get_network_bip70(&self) -> Result { + Ok(self + .make_node_request("getblockchaininfo", None)? .get("chain") .and_then(Json::as_str) .expect("Missing or invalid 'chain' in 'getblockchaininfo' result?") - .to_string() + .to_string()) } - fn list_wallets(&self) -> Vec { - self.make_node_request("listwallets", None) + fn list_wallets(&self) -> Result, BitcoindError> { + Ok(self + .make_node_request("listwallets", None)? .as_array() .expect("API break, 'listwallets' didn't return an array.") .iter() @@ -496,7 +490,7 @@ impl BitcoinD { .expect("API break: 'listwallets' contains a non-string value") .to_string() }) - .collect() + .collect()) } // Get a warning from the result of a wallet command. It was modified in v25 so it's a bit @@ -528,16 +522,16 @@ impl BitcoinD { None } - fn unload_wallet(&self, wallet_path: String) -> Option { - let res = self.make_node_request("unloadwallet", params!(Json::String(wallet_path),)); - self.warning_from_res(&res) + fn unload_wallet(&self, wallet_path: String) -> Result, BitcoindError> { + let res = self.make_node_request("unloadwallet", params!(Json::String(wallet_path),))?; + Ok(self.warning_from_res(&res)) } fn create_wallet(&self, wallet_path: String) -> Result<(), String> { // NOTE: we set load_on_startup to make sure the wallet will get updated before the // historical blocks are deleted in case the bitcoind is pruned. let res = self - .make_fallible_node_request( + .make_node_request( "createwallet", params!( Json::String(wallet_path), @@ -627,10 +621,13 @@ impl BitcoinD { .collect() } - fn maybe_unload_watchonly_wallet(&self, watchonly_wallet_path: String) { - while self.list_wallets().contains(&watchonly_wallet_path) { + fn maybe_unload_watchonly_wallet( + &self, + watchonly_wallet_path: String, + ) -> Result<(), BitcoindError> { + while self.list_wallets()?.contains(&watchonly_wallet_path) { log::info!("Found a leftover watchonly wallet loaded on bitcoind. Removing it."); - if let Some(e) = self.unload_wallet(watchonly_wallet_path.clone()) { + if let Some(e) = self.unload_wallet(watchonly_wallet_path.clone())? { log::error!( "Unloading wallet '{}': '{}'", &self.watchonly_wallet_path, @@ -638,6 +635,8 @@ impl BitcoinD { ); } } + + Ok(()) } /// Create the watchonly wallet on bitcoind, and import it the main descriptor. @@ -647,7 +646,7 @@ impl BitcoinD { ) -> Result<(), BitcoindError> { // Remove any leftover. This can happen if we delete the watchonly wallet but don't restart // bitcoind. - self.maybe_unload_watchonly_wallet(self.watchonly_wallet_path.clone()); + self.maybe_unload_watchonly_wallet(self.watchonly_wallet_path.clone())?; // Now create the wallet and import the main descriptor. self.create_wallet(self.watchonly_wallet_path.clone()) @@ -667,10 +666,10 @@ impl BitcoinD { /// Load the watchonly wallet on bitcoind, if it isn't already. pub fn maybe_load_watchonly_wallet(&self) -> Result<(), BitcoindError> { - if self.list_wallets().contains(&self.watchonly_wallet_path) { + if self.list_wallets()?.contains(&self.watchonly_wallet_path) { return Ok(()); } - let res = self.make_fallible_node_request( + let res = self.make_node_request( "loadwallet", params!(Json::String(self.watchonly_wallet_path.clone()),), ); @@ -680,7 +679,7 @@ impl BitcoinD { log::warn!("The watchonly wallet is already loading on bitcoind. Waiting for completion."); loop { thread::sleep(Duration::from_secs(3)); - if self.list_wallets().contains(&self.watchonly_wallet_path) { + if self.list_wallets()?.contains(&self.watchonly_wallet_path) { log::warn!("Watchonly wallet now loaded. Continuing."); return Ok(()); } @@ -701,13 +700,13 @@ impl BitcoinD { config_network: bitcoin::Network, ) -> Result<(), BitcoindError> { // Check the minimum supported bitcoind version - let version = self.get_bitcoind_version(); + let version = self.get_bitcoind_version()?; if version < MIN_BITCOIND_VERSION { return Err(BitcoindError::InvalidVersion(version)); } // Check bitcoind is running on the right network - let bitcoind_net = self.get_network_bip70(); + let bitcoind_net = self.get_network_bip70()?; let bip70_net = match config_network { bitcoin::Network::Bitcoin => "main", bitcoin::Network::Testnet => "test", @@ -732,7 +731,7 @@ impl BitcoinD { ) -> Result<(), BitcoindError> { // Check our watchonly wallet is loaded if self - .list_wallets() + .list_wallets()? .iter() .filter(|s| s == &&self.watchonly_wallet_path) .count() @@ -764,13 +763,13 @@ impl BitcoinD { Ok(()) } - fn block_chain_info(&self) -> Json { + fn block_chain_info(&self) -> Result { self.make_node_request("getblockchaininfo", None) } - pub fn sync_progress(&self) -> SyncProgress { + pub fn sync_progress(&self) -> Result { // TODO: don't harass lianad, be smarter like in revaultd. - let chain_info = self.block_chain_info(); + let chain_info = self.block_chain_info()?; let percentage = chain_info .get("verificationprogress") .and_then(Json::as_f64) @@ -783,16 +782,16 @@ impl BitcoinD { .get("blocks") .and_then(Json::as_u64) .expect("No valid 'blocks' in getblockchaininfo response?"); - SyncProgress { + Ok(SyncProgress { percentage, headers, blocks, - } + }) } - pub fn chain_tip(&self) -> BlockChainTip { + pub fn chain_tip(&self) -> Result { // We use getblockchaininfo to avoid a race between getblockcount and getblockhash - let chain_info = self.block_chain_info(); + let chain_info = self.block_chain_info()?; let hash = bitcoin::BlockHash::from_str( chain_info .get("bestblockhash") @@ -807,12 +806,12 @@ impl BitcoinD { .try_into() .expect("Must fit by Bitcoin consensus"); - BlockChainTip { hash, height } + Ok(BlockChainTip { hash, height }) } pub fn get_block_hash(&self, height: i32) -> Option { Some( - self.make_fallible_node_request("getblockhash", params!(Json::Number(height.into()),)) + self.make_node_request("getblockhash", params!(Json::Number(height.into()),)) .ok()? .as_str() .and_then(|s| bitcoin::BlockHash::from_str(s).ok()) @@ -845,17 +844,18 @@ impl BitcoinD { } /// Efficient check that a coin is spent. - pub fn is_spent(&self, op: &bitcoin::OutPoint) -> bool { + pub fn is_spent(&self, op: &bitcoin::OutPoint) -> Result { // The result of gettxout is empty if the outpoint is spent. - self.make_node_request( - "gettxout", - params!( - Json::String(op.txid.to_string()), - Json::Number(op.vout.into()) - ), - ) - .get("bestblock") - .is_none() + Ok(self + .make_node_request( + "gettxout", + params!( + Json::String(op.txid.to_string()), + Json::Number(op.vout.into()) + ), + )? + .get("bestblock") + .is_none()) } /// So, bitcoind has no API for getting the transaction spending a wallet UTXO. Instead we are @@ -864,7 +864,10 @@ impl BitcoinD { /// So, what we do there is listing all outgoing transactions of the wallet since the last poll /// and iterating through each of those to check if it spends the transaction we are interested /// in (requiring an other RPC call for each!!). - pub fn get_spender_txid(&self, spent_outpoint: &bitcoin::OutPoint) -> Option { + pub fn get_spender_txid( + &self, + spent_outpoint: &bitcoin::OutPoint, + ) -> Result, BitcoindError> { // Get the hash of the spent transaction's block parent. If the spent transaction is still // unconfirmed, just use the tip. let req = self.make_wallet_request( @@ -873,9 +876,9 @@ impl BitcoinD { ); let list_since_height = match req.get("blockheight").and_then(Json::as_i64) { Some(h) => h as i32, - None => self.chain_tip().height, + None => self.chain_tip()?.height, }; - let block_hash = if let Ok(res) = self.make_fallible_node_request( + let block_hash = if let Ok(res) = self.make_node_request( "getblockhash", params!(Json::Number((list_since_height - 1).into())), ) { @@ -884,7 +887,7 @@ impl BitcoinD { .to_string() } else { // Possibly a race. - return None; + return Ok(None); }; // Now we can get all transactions related to us since the spent transaction confirmed. @@ -969,23 +972,26 @@ impl BitcoinD { break; } - return Some(spending_txid); + return Ok(Some(spending_txid)); } } } - None + Ok(None) } - pub fn get_block_stats(&self, blockhash: bitcoin::BlockHash) -> Option { - let res = match self.make_fallible_node_request( + pub fn get_block_stats( + &self, + blockhash: bitcoin::BlockHash, + ) -> Result, BitcoindError> { + let res = match self.make_node_request( "getblockheader", params!(Json::String(blockhash.to_string()),), ) { Ok(res) => res, Err(e) => { log::warn!("Error when fetching block header {}: {}", &blockhash, e); - return None; + return Ok(None); } }; let confirmations = res @@ -1015,18 +1021,18 @@ impl BitcoinD { .and_then(Json::as_u64) .expect("Invalid median timestamp in `getblockheader` response: not an u64") as u32; - Some(BlockStats { + Ok(Some(BlockStats { confirmations, previous_blockhash, height, blockhash, time, median_time_past, - }) + })) } pub fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), BitcoindError> { - self.make_fallible_node_request( + self.make_node_request( "sendrawtransaction", params!(bitcoin::consensus::encode::serialize_hex(tx).into()), )?; @@ -1054,7 +1060,7 @@ impl BitcoinD { // Make sure the bitcoind has enough blocks to rescan up to this timestamp. fn check_prune_height(&self, timestamp: u32) -> Result<(), BitcoindError> { - let chain_info = self.block_chain_info(); + let chain_info = self.block_chain_info()?; let first_block_height = if let Some(h) = chain_info.get("pruneheight") { h } else { @@ -1066,7 +1072,7 @@ impl BitcoinD { .expect("Height must be an integer") .try_into() .expect("Height must fit in a i32"); - if let Some(tip) = self.tip_before_timestamp(timestamp) { + if let Some(tip) = self.tip_before_timestamp(timestamp)? { if tip.height >= prune_height { return Ok(()); } @@ -1155,13 +1161,19 @@ impl BitcoinD { } /// Get the height and hash of the last block with a timestamp below the given one. - pub fn tip_before_timestamp(&self, timestamp: u32) -> Option { - block_before_date( + pub fn tip_before_timestamp( + &self, + timestamp: u32, + ) -> Result, BitcoindError> { + Ok(block_before_date( timestamp, - self.chain_tip(), + self.chain_tip()?, |h| self.get_block_hash(h), - |h| self.get_block_stats(h), - ) + |h| { + self.get_block_stats(h) + .expect("We assume bitcoind connection never fails") + }, + )) } /// Whether this transaction is in the mempool. @@ -1172,9 +1184,7 @@ impl BitcoinD { /// Get mempool entry of the given transaction. /// Returns `None` if it is not in the mempool. pub fn mempool_entry(&self, txid: &bitcoin::Txid) -> Option { - match self - .make_fallible_node_request("getmempoolentry", params!(Json::String(txid.to_string()))) - { + match self.make_node_request("getmempoolentry", params!(Json::String(txid.to_string()))) { Ok(json) => Some(MempoolEntry::from(json)), Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, @@ -1190,12 +1200,13 @@ impl BitcoinD { pub fn mempool_txs_spending_prevouts( &self, outpoints: &[bitcoin::OutPoint], - ) -> Vec { + ) -> Result, BitcoindError> { let prevouts: Json = outpoints .iter() .map(|op| serde_json::json!({"txid": op.txid.to_string(), "vout": op.vout})) .collect(); - self.make_node_request("gettxspendingprevout", params!(prevouts)) + Ok(self + .make_node_request("gettxspendingprevout", params!(prevouts))? .as_array() .expect("Always returns an array") .iter() @@ -1206,12 +1217,12 @@ impl BitcoinD { .expect("Must be a valid txid if present") }) }) - .collect() + .collect()) } /// Stop bitcoind. - pub fn stop(&self) { - self.make_node_request("stop", None); + pub fn stop(&self) -> Result<(), BitcoindError> { + self.make_node_request("stop", None).map(|_| ()) } } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index 60388c1e0..86016ed81 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -129,10 +129,12 @@ impl BitcoinInterface for d::BitcoinD { fn sync_progress(&self) -> SyncProgress { self.sync_progress() + .expect("We expect the connection to bitcoind to never fail.") } fn chain_tip(&self) -> BlockChainTip { self.chain_tip() + .expect("We expect the connection to bitcoind to never fail.") } fn is_in_chain(&self, tip: &BlockChainTip) -> bool { @@ -223,8 +225,14 @@ impl BitcoinInterface for d::BitcoinD { let mut spent = Vec::with_capacity(outpoints.len()); for op in outpoints { - if self.is_spent(op) { - let spending_txid = if let Some(txid) = self.get_spender_txid(op) { + if self + .is_spent(op) + .expect("We expect the connection to bitcoind to never fail.") + { + let spending_txid = if let Some(txid) = self + .get_spender_txid(op) + .expect("We expect the connection to bitcoind to never fail.") + { txid } else { // TODO: better handling of this edge case. @@ -304,11 +312,15 @@ impl BitcoinInterface for d::BitcoinD { } fn common_ancestor(&self, tip: &BlockChainTip) -> Option { - let mut stats = self.get_block_stats(tip.hash)?; + let mut stats = self + .get_block_stats(tip.hash) + .expect("We expect the connection to bitcoind to never fail.")?; let mut ancestor = *tip; while stats.confirmations == -1 { - stats = self.get_block_stats(stats.previous_blockhash?)?; + stats = self + .get_block_stats(stats.previous_blockhash?) + .expect("We expect the connection to bitcoind to never fail.")?; ancestor = BlockChainTip { hash: stats.blockhash, height: stats.height, @@ -346,11 +358,18 @@ impl BitcoinInterface for d::BitcoinD { fn block_before_date(&self, timestamp: u32) -> Option { self.tip_before_timestamp(timestamp) + .expect("We expect the connection to bitcoind to never fail.") } fn tip_time(&self) -> Option { - let tip = self.chain_tip(); - Some(self.get_block_stats(tip.hash)?.time) + let tip = self + .chain_tip() + .expect("We expect the connection to bitcoind to never fail."); + Some( + self.get_block_stats(tip.hash) + .expect("We expect the connection to bitcoind to never fail.")? + .time, + ) } fn wallet_transaction( @@ -362,6 +381,7 @@ impl BitcoinInterface for d::BitcoinD { fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec { self.mempool_txs_spending_prevouts(outpoints) + .expect("We expect the connection to bitcoind to never fail.") .into_iter() .filter_map(|txid| self.mempool_entry(&txid)) .collect() From 9d5c0bf5bf0407a26b8d3b562ecba93492c63a2f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 15:33:11 +0100 Subject: [PATCH 06/11] bitcoind: make wallet requests fallible Same as previous commit but for wallet requests --- src/bitcoin/d/mod.rs | 97 ++++++++++++++++++++++---------------------- src/bitcoin/mod.rs | 5 ++- 2 files changed, 52 insertions(+), 50 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 86e5a643c..c315af8cd 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -448,15 +448,6 @@ impl BitcoinD { &self, method: &str, params: Option<&serde_json::value::RawValue>, - ) -> Json { - self.make_request(&self.watchonly_client, method, params) - .expect("We must not fail to make a request for more than a minute") - } - - fn make_faillible_wallet_request( - &self, - method: &str, - params: Option<&serde_json::value::RawValue>, ) -> Result { self.make_request(&self.watchonly_client, method, params) } @@ -556,7 +547,7 @@ impl BitcoinD { } // Import the receive and change descriptors from the multipath descriptor to bitcoind. - fn import_descriptor(&self, desc: &LianaDescriptor) -> Option { + fn import_descriptor(&self, desc: &LianaDescriptor) -> Result, BitcoindError> { let descriptors = [desc.receive_descriptor(), desc.change_descriptor()] .iter() .map(|desc| { @@ -568,7 +559,8 @@ impl BitcoinD { }) .collect(); - let res = self.make_wallet_request("importdescriptors", params!(Json::Array(descriptors))); + let res = + self.make_wallet_request("importdescriptors", params!(Json::Array(descriptors)))?; let all_succeeded = res .as_array() .map(|results| { @@ -577,15 +569,16 @@ impl BitcoinD { .all(|res| res.get("success").and_then(Json::as_bool).unwrap_or(false)) }) .unwrap_or(false); - if all_succeeded { + Ok(if all_succeeded { None } else { Some(res.to_string()) - } + }) } - fn list_descriptors(&self) -> Vec { - self.make_wallet_request("listdescriptors", None) + fn list_descriptors(&self) -> Result, BitcoindError> { + Ok(self + .make_wallet_request("listdescriptors", None)? .get("descriptors") .and_then(Json::as_array) .expect("Missing or invalid 'descriptors' field in 'listdescriptors' response") @@ -618,7 +611,7 @@ impl BitcoinD { timestamp, } }) - .collect() + .collect()) } fn maybe_unload_watchonly_wallet( @@ -654,7 +647,7 @@ impl BitcoinD { BitcoindError::Wallet(self.watchonly_wallet_path.clone(), WalletError::Creating(e)) })?; // TODO: make it return an error instead of an option. - if let Some(err) = self.import_descriptor(main_descriptor) { + if let Some(err) = self.import_descriptor(main_descriptor)? { return Err(BitcoindError::Wallet( self.watchonly_wallet_path.clone(), WalletError::ImportingDescriptor(err), @@ -747,7 +740,7 @@ impl BitcoinD { let receive_desc = main_descriptor.receive_descriptor(); let change_desc = main_descriptor.change_descriptor(); let desc_list: Vec = self - .list_descriptors() + .list_descriptors()? .into_iter() .map(|entry| entry.desc) .collect(); @@ -819,28 +812,29 @@ impl BitcoinD { ) } - pub fn list_since_block(&self, block_hash: &bitcoin::BlockHash) -> LSBlockRes { - self.make_wallet_request( - "listsinceblock", - params!( - Json::String(block_hash.to_string()), - Json::Number(1.into()), // Default for min_confirmations for the returned - Json::Bool(true), // Whether to include watchonly - Json::Bool(false), // Whether to include an array of txs that were removed in reorgs - Json::Bool(true) // Whether to include UTxOs treated as change. - ), - ) - .into() + pub fn list_since_block( + &self, + block_hash: &bitcoin::BlockHash, + ) -> Result { + Ok(self + .make_wallet_request( + "listsinceblock", + params!( + Json::String(block_hash.to_string()), + Json::Number(1.into()), // Default for min_confirmations for the returned + Json::Bool(true), // Whether to include watchonly + Json::Bool(false), // Whether to include an array of txs that were removed in reorgs + Json::Bool(true) // Whether to include UTxOs treated as change. + ), + )? + .into()) } pub fn get_transaction(&self, txid: &bitcoin::Txid) -> Option { - // TODO: Maybe assert we got a -5 error, and not any other kind of error? - self.make_faillible_wallet_request( - "gettransaction", - params!(Json::String(txid.to_string())), - ) - .ok() - .map(|res| res.into()) + // TODO: assert we got a -5 error, and not any other kind of error? + self.make_wallet_request("gettransaction", params!(Json::String(txid.to_string()))) + .ok() + .map(|res| res.into()) } /// Efficient check that a coin is spent. @@ -873,7 +867,7 @@ impl BitcoinD { let req = self.make_wallet_request( "gettransaction", params!(Json::String(spent_outpoint.txid.to_string())), - ); + )?; let list_since_height = match req.get("blockheight").and_then(Json::as_i64) { Some(h) => h as i32, None => self.chain_tip()?.height, @@ -902,7 +896,7 @@ impl BitcoinD { Json::Bool(false), // Whether to include an array of txs that were removed in reorgs Json::Bool(true) // Whether to include UTxOs treated as change. ), - ); + )?; let transactions = lsb_res .get("transactions") .and_then(Json::as_array) @@ -935,7 +929,7 @@ impl BitcoinD { Json::Bool(true), // watchonly Json::Bool(true) // verbose ), - ); + )?; let vin = gettx_res .get("decoded") .and_then(|d| d.get("vin").and_then(Json::as_array)) @@ -1041,8 +1035,12 @@ impl BitcoinD { // For the given descriptor strings check if they are imported at this timestamp in the // watchonly wallet. - fn check_descs_timestamp(&self, descs: &[String], timestamp: u32) -> bool { - let current_descs = self.list_descriptors(); + fn check_descs_timestamp( + &self, + descs: &[String], + timestamp: u32, + ) -> Result { + let current_descs = self.list_descriptors()?; for desc in descs { let present = current_descs @@ -1051,11 +1049,11 @@ impl BitcoinD { .map(|entry| entry.timestamp == timestamp) .unwrap_or(false); if !present { - return false; + return Ok(false); } } - true + Ok(true) } // Make sure the bitcoind has enough blocks to rescan up to this timestamp. @@ -1091,7 +1089,7 @@ impl BitcoinD { // have a range inclusive of the existing ones. We always use 0 as the initial index so // this is just determining the maximum index to use. let max_range = self - .list_descriptors() + .list_descriptors()? .into_iter() // 1_000 is bitcoind's default and what we use at initial import. .fold(1_000, |range, entry| { @@ -1139,7 +1137,7 @@ impl BitcoinD { } i += 1; - if self.check_descs_timestamp(&desc_str, timestamp) { + if self.check_descs_timestamp(&desc_str, timestamp)? { return Ok(()); } else if i >= NUM_RETRIES { return Err(BitcoindError::StartRescan); @@ -1151,13 +1149,14 @@ impl BitcoinD { } /// Get the progress of the ongoing rescan, if there is any. - pub fn rescan_progress(&self) -> Option { - self.make_wallet_request("getwalletinfo", None) + pub fn rescan_progress(&self) -> Result, BitcoindError> { + Ok(self + .make_wallet_request("getwalletinfo", None)? .get("scanning") // If no rescan is ongoing, it will fail cause it would be 'false' .and_then(Json::as_object) .and_then(|map| map.get("progress")) - .and_then(Json::as_f64) + .and_then(Json::as_f64)) } /// Get the height and hash of the last block with a timestamp below the given one. diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index 86016ed81..2ae9db743 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -148,7 +148,9 @@ impl BitcoinInterface for d::BitcoinD { tip: &BlockChainTip, descs: &[descriptors::SinglePathLianaDesc], ) -> Vec { - let lsb_res = self.list_since_block(&tip.hash); + let lsb_res = self + .list_since_block(&tip.hash) + .expect("We expect the connection to bitcoind to never fail."); lsb_res .received_coins @@ -354,6 +356,7 @@ impl BitcoinInterface for d::BitcoinD { fn rescan_progress(&self) -> Option { self.rescan_progress() + .expect("We expect the connection to bitcoind to never fail.") } fn block_before_date(&self, timestamp: u32) -> Option { From 236af57394c9db2a6d9d930d8a2a27ac62b6b948 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 15:44:02 +0100 Subject: [PATCH 07/11] bitcoind: make get_block_hash return a result It's less straightforward for calls which return a JSONRPC error on not found --- src/bitcoin/d/mod.rs | 26 +++++++++++++++++--------- src/bitcoin/mod.rs | 2 ++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index c315af8cd..3f2806ad4 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -802,14 +802,19 @@ impl BitcoinD { Ok(BlockChainTip { hash, height }) } - pub fn get_block_hash(&self, height: i32) -> Option { - Some( - self.make_node_request("getblockhash", params!(Json::Number(height.into()),)) - .ok()? - .as_str() - .and_then(|s| bitcoin::BlockHash::from_str(s).ok()) - .expect("bitcoind must send valid block hashes"), - ) + pub fn get_block_hash(&self, height: i32) -> Result, BitcoindError> { + match self.make_node_request("getblockhash", params!(Json::Number(height.into()),)) { + Ok(json) => Ok(Some( + json.as_str() + .and_then(|s| bitcoin::BlockHash::from_str(s).ok()) + .expect("Block hashes returned by bitcoind must be valid"), + )), + Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { + code: -5, + .. + }))) => Ok(None), + Err(e) => Err(e), + } } pub fn list_since_block( @@ -1167,7 +1172,10 @@ impl BitcoinD { Ok(block_before_date( timestamp, self.chain_tip()?, - |h| self.get_block_hash(h), + |h| { + self.get_block_hash(h) + .expect("We assume bitcoind connection never fails") + }, |h| { self.get_block_stats(h) .expect("We assume bitcoind connection never fails") diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index 2ae9db743..e072c6c48 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -123,6 +123,7 @@ impl BitcoinInterface for d::BitcoinD { let height = 0; let hash = self .get_block_hash(height) + .expect("We expect the connection to bitcoind to never fail.") .expect("Genesis block hash must always be there"); BlockChainTip { hash, height } } @@ -139,6 +140,7 @@ impl BitcoinInterface for d::BitcoinD { fn is_in_chain(&self, tip: &BlockChainTip) -> bool { self.get_block_hash(tip.height) + .expect("We expect the connection to bitcoind to never fail.") .map(|bh| bh == tip.hash) .unwrap_or(false) } From 21c9f16d9f99217fa54342425c32c82317a4d861 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 15:51:42 +0100 Subject: [PATCH 08/11] bitcoind: don't panic in mempool_entry, propagate the error --- src/bitcoin/d/mod.rs | 19 ++++++++++--------- src/bitcoin/mod.rs | 15 ++++++++++++--- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 3f2806ad4..8d7dd8336 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -966,7 +966,7 @@ impl BitcoinD { .get("walletconflicts") .and_then(Json::as_array) .expect("A valid list of wallet conflicts must always be present."); - if confs == 0 && !conflicts.is_empty() && !self.is_in_mempool(&spending_txid) { + if confs == 0 && !conflicts.is_empty() && !self.is_in_mempool(&spending_txid)? { log::debug!("Noticed '{}' as spending '{}', but is unconfirmed with conflicts and is not in mempool anymore. Discarding it.", &spending_txid, &spent_outpoint); break; } @@ -1184,22 +1184,23 @@ impl BitcoinD { } /// Whether this transaction is in the mempool. - pub fn is_in_mempool(&self, txid: &bitcoin::Txid) -> bool { - self.mempool_entry(txid).is_some() + pub fn is_in_mempool(&self, txid: &bitcoin::Txid) -> Result { + Ok(self.mempool_entry(txid)?.is_some()) } /// Get mempool entry of the given transaction. /// Returns `None` if it is not in the mempool. - pub fn mempool_entry(&self, txid: &bitcoin::Txid) -> Option { + pub fn mempool_entry( + &self, + txid: &bitcoin::Txid, + ) -> Result, BitcoindError> { match self.make_node_request("getmempoolentry", params!(Json::String(txid.to_string()))) { - Ok(json) => Some(MempoolEntry::from(json)), + Ok(json) => Ok(Some(MempoolEntry::from(json))), Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, .. - }))) => None, - Err(e) => { - panic!("Unexpected error returned by bitcoind {}", e); - } + }))) => Ok(None), + Err(e) => Err(e), } } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index e072c6c48..383a366b6 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -214,7 +214,10 @@ impl BitcoinInterface for d::BitcoinD { } // If the transaction was dropped from the mempool, discard the coin. - if !self.is_in_mempool(&op.txid) { + if !self + .is_in_mempool(&op.txid) + .expect("We expect the connection to bitcoind to never fail.") + { expired.push(*op); } } @@ -307,7 +310,10 @@ impl BitcoinInterface for d::BitcoinD { // If the transaction was not confirmed, a conflicting transaction spending this coin // too wasn't mined, but still isn't in our mempool anymore, mark the spend as expired. - if !self.is_in_mempool(txid) { + if !self + .is_in_mempool(txid) + .expect("We expect the connection to bitcoind to never fail.") + { expired.push(*op); } } @@ -388,7 +394,10 @@ impl BitcoinInterface for d::BitcoinD { self.mempool_txs_spending_prevouts(outpoints) .expect("We expect the connection to bitcoind to never fail.") .into_iter() - .filter_map(|txid| self.mempool_entry(&txid)) + .filter_map(|txid| { + self.mempool_entry(&txid) + .expect("We expect the connection to bitcoind to never fail.") + }) .collect() } } From 5664a79a144f0dccd4c9950b777962062e4168ff Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Fri, 5 Jan 2024 16:02:29 +0100 Subject: [PATCH 09/11] bitcoind: don't ignore unexpected error in get_transaction --- src/bitcoin/d/mod.rs | 26 ++++++++++++++++---------- src/bitcoin/mod.rs | 41 ++++++++++++++++++++++++++--------------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 8d7dd8336..c695d9bc0 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -835,11 +835,15 @@ impl BitcoinD { .into()) } - pub fn get_transaction(&self, txid: &bitcoin::Txid) -> Option { - // TODO: assert we got a -5 error, and not any other kind of error? - self.make_wallet_request("gettransaction", params!(Json::String(txid.to_string()))) - .ok() - .map(|res| res.into()) + pub fn get_transaction(&self, txid: &bitcoin::Txid) -> Result, BitcoindError> { + match self.make_wallet_request("gettransaction", params!(Json::String(txid.to_string()))) { + Ok(json) => Ok(Some(json.into())), + Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { + code: -5, + .. + }))) => Ok(None), + Err(e) => Err(e), + } } /// Efficient check that a coin is spent. @@ -1476,16 +1480,18 @@ impl<'a> CachedTxGetter<'a> { /// Query a transaction. Tries to get it from the cache and falls back to calling /// `gettransaction` on bitcoind. If both fail, returns None. - pub fn get_transaction(&mut self, txid: &bitcoin::Txid) -> Option { - // TODO: work around the borrow checker to avoid having to clone. - if let Some(res) = self.cache.get(txid) { + pub fn get_transaction( + &mut self, + txid: &bitcoin::Txid, + ) -> Result, BitcoindError> { + Ok(if let Some(res) = self.cache.get(txid) { Some(res.clone()) - } else if let Some(res) = self.bitcoind.get_transaction(txid) { + } else if let Some(res) = self.bitcoind.get_transaction(txid)? { self.cache.insert(*txid, res); self.cache.get(txid).cloned() } else { None - } + }) } } diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index 383a366b6..11a8df5c6 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -195,7 +195,10 @@ impl BitcoinInterface for d::BitcoinD { let mut tx_getter = CachedTxGetter::new(self); for op in outpoints { - let res = if let Some(res) = tx_getter.get_transaction(&op.txid) { + let res = if let Some(res) = tx_getter + .get_transaction(&op.txid) + .expect("We expect the connection to bitcoind to never fail.") + { res } else { log::error!("Transaction not in wallet for coin '{}'.", op); @@ -272,7 +275,10 @@ impl BitcoinInterface for d::BitcoinD { let mut tx_getter = CachedTxGetter::new(self); for (op, txid) in outpoints { - let res = if let Some(res) = tx_getter.get_transaction(txid) { + let res = if let Some(res) = tx_getter + .get_transaction(txid) + .expect("We expect the connection to bitcoind to never fail.") + { res } else { log::error!("Could not get tx {} spending coin {}.", txid, op); @@ -288,20 +294,23 @@ impl BitcoinInterface for d::BitcoinD { // If a conflicting transaction was confirmed instead, replace the txid of the // spender for this coin with it and mark it as confirmed. let conflict = res.conflicting_txs.iter().find_map(|txid| { - tx_getter.get_transaction(txid).and_then(|tx| { - tx.block.and_then(|block| { - // Being part of our watchonly wallet isn't enough, as it could be a - // conflicting transaction which spends a different set of coins. Make sure - // it does actually spend this coin. - tx.tx.input.iter().find_map(|txin| { - if &txin.previous_output == op { - Some((*txid, block)) - } else { - None - } + tx_getter + .get_transaction(txid) + .expect("We expect the connection to bitcoind to never fail.") + .and_then(|tx| { + tx.block.and_then(|block| { + // Being part of our watchonly wallet isn't enough, as it could be a + // conflicting transaction which spends a different set of coins. Make sure + // it does actually spend this coin. + tx.tx.input.iter().find_map(|txin| { + if &txin.previous_output == op { + Some((*txid, block)) + } else { + None + } + }) }) }) - }) }); if let Some((txid, block)) = conflict { spent.push((*op, txid, block)); @@ -387,7 +396,9 @@ impl BitcoinInterface for d::BitcoinD { &self, txid: &bitcoin::Txid, ) -> Option<(bitcoin::Transaction, Option)> { - self.get_transaction(txid).map(|res| (res.tx, res.block)) + self.get_transaction(txid) + .expect("We expect the connection to bitcoind to never fail.") + .map(|res| (res.tx, res.block)) } fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec { From fb768e0030bb635d067cbb2b115f55921a20af58 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 8 Jan 2024 14:45:22 +0100 Subject: [PATCH 10/11] bitcoin: propagate errors occuring in our Bitcoin backend interface --- src/bin/daemon.rs | 6 +- src/bitcoin/mod.rs | 306 ++++++++++++++++++----------------- src/bitcoin/poller/looper.rs | 72 +++++---- src/bitcoin/poller/mod.rs | 19 ++- src/commands/mod.rs | 123 ++++++++++---- src/jsonrpc/api.rs | 6 +- src/jsonrpc/mod.rs | 3 +- src/lib.rs | 13 +- src/testutils.rs | 79 +++++---- 9 files changed, 368 insertions(+), 259 deletions(-) diff --git a/src/bin/daemon.rs b/src/bin/daemon.rs index 1e8a4c819..fd85c9807 100644 --- a/src/bin/daemon.rs +++ b/src/bin/daemon.rs @@ -85,7 +85,11 @@ fn main() { .name("Bitcoin Core poller".to_string()) .spawn({ let shutdown = poller_shutdown.clone(); - move || bitcoin_poller.poll_forever(poll_interval, shutdown) + move || { + bitcoin_poller + .poll_forever(poll_interval, shutdown) + .expect("We assume the bitcoind connection never fails") // FIXME + } }) .expect("Spawning the poller thread must never fail."); control diff --git a/src/bitcoin/mod.rs b/src/bitcoin/mod.rs index 11a8df5c6..c081ef5a9 100644 --- a/src/bitcoin/mod.rs +++ b/src/bitcoin/mod.rs @@ -6,12 +6,12 @@ pub mod d; pub mod poller; use crate::{ - bitcoin::d::{BitcoindError, CachedTxGetter, LSBlockEntry}, + bitcoin::d::{CachedTxGetter, LSBlockEntry}, descriptors, }; pub use d::{MempoolEntry, SyncProgress}; -use std::{fmt, sync}; +use std::{error, fmt, sync}; use miniscript::bitcoin::{self, address}; @@ -40,58 +40,66 @@ impl fmt::Display for BlockChainTip { /// Our Bitcoin backend. pub trait BitcoinInterface: Send { - fn genesis_block(&self) -> BlockChainTip; + fn genesis_block(&self) -> Result>; /// Get the progress of the block chain synchronization. /// Returns a rounded up percentage between 0 and 1. Use the `is_synced` method to be sure the /// backend is completely synced to the best known tip. - fn sync_progress(&self) -> SyncProgress; + fn sync_progress(&self) -> Result>; /// Get the best block info. - fn chain_tip(&self) -> BlockChainTip; + fn chain_tip(&self) -> Result>; /// Get the timestamp set in the best block's header. - fn tip_time(&self) -> Option; + fn tip_time(&self) -> Result, Box>; /// Check whether this former tip is part of the current best chain. - fn is_in_chain(&self, tip: &BlockChainTip) -> bool; + fn is_in_chain(&self, tip: &BlockChainTip) -> Result>; /// Get coins received since the specified tip. fn received_coins( &self, tip: &BlockChainTip, descs: &[descriptors::SinglePathLianaDesc], - ) -> Vec; + ) -> Result, Box>; /// Get all coins that were confirmed, and at what height and time. Along with "expired" /// unconfirmed coins (for instance whose creating transaction may have been replaced). + #[allow(clippy::type_complexity)] fn confirmed_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> (Vec<(bitcoin::OutPoint, i32, u32)>, Vec); + ) -> Result<(Vec<(bitcoin::OutPoint, i32, u32)>, Vec), Box>; /// Get all coins that are being spent, and the spending txid. fn spending_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid)>; + ) -> Result, Box>; /// Get all coins that are spent with the final spend tx txid and blocktime. Along with the /// coins for which the spending transaction "expired" (a conflicting transaction was mined and /// it wasn't spending this coin). + #[allow(clippy::type_complexity)] fn spent_coins( &self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)], - ) -> ( - Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, - Vec, - ); + ) -> Result< + ( + Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, + Vec, + ), + Box, + >; /// Get the common ancestor between the Bitcoin backend's tip and the given tip. - fn common_ancestor(&self, tip: &BlockChainTip) -> Option; + fn common_ancestor( + &self, + tip: &BlockChainTip, + ) -> Result, Box>; /// Broadcast this transaction to the Bitcoin P2P network - fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), String>; + fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), Box>; /// Trigger a rescan of the block chain for transactions related to this descriptor since /// the given date. @@ -99,62 +107,64 @@ pub trait BitcoinInterface: Send { &self, desc: &descriptors::LianaDescriptor, timestamp: u32, - ) -> Result<(), String>; + ) -> Result<(), Box>; /// Rescan progress percentage. Between 0 and 1. - fn rescan_progress(&self) -> Option; + fn rescan_progress(&self) -> Result, Box>; /// Get the last block chain tip with a timestamp below this. Timestamp must be a valid block /// timestamp. - fn block_before_date(&self, timestamp: u32) -> Option; + fn block_before_date( + &self, + timestamp: u32, + ) -> Result, Box>; /// Get a transaction related to the wallet along with potential confirmation info. + #[allow(clippy::type_complexity)] fn wallet_transaction( &self, txid: &bitcoin::Txid, - ) -> Option<(bitcoin::Transaction, Option)>; + ) -> Result)>, Box>; /// Get the details of unconfirmed transactions spending these outpoints, if any. - fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec; + fn mempool_spenders( + &self, + outpoints: &[bitcoin::OutPoint], + ) -> Result, Box>; } impl BitcoinInterface for d::BitcoinD { - fn genesis_block(&self) -> BlockChainTip { + fn genesis_block(&self) -> Result> { let height = 0; let hash = self - .get_block_hash(height) - .expect("We expect the connection to bitcoind to never fail.") + .get_block_hash(height)? .expect("Genesis block hash must always be there"); - BlockChainTip { hash, height } + Ok(BlockChainTip { hash, height }) } - fn sync_progress(&self) -> SyncProgress { - self.sync_progress() - .expect("We expect the connection to bitcoind to never fail.") + fn sync_progress(&self) -> Result> { + Ok(self.sync_progress()?) } - fn chain_tip(&self) -> BlockChainTip { - self.chain_tip() - .expect("We expect the connection to bitcoind to never fail.") + fn chain_tip(&self) -> Result> { + Ok(self.chain_tip()?) } - fn is_in_chain(&self, tip: &BlockChainTip) -> bool { - self.get_block_hash(tip.height) - .expect("We expect the connection to bitcoind to never fail.") + fn is_in_chain(&self, tip: &BlockChainTip) -> Result> { + Ok(self + .get_block_hash(tip.height)? .map(|bh| bh == tip.hash) - .unwrap_or(false) + .unwrap_or(false)) } fn received_coins( &self, tip: &BlockChainTip, descs: &[descriptors::SinglePathLianaDesc], - ) -> Vec { - let lsb_res = self - .list_since_block(&tip.hash) - .expect("We expect the connection to bitcoind to never fail."); + ) -> Result, Box> { + let lsb_res = self.list_since_block(&tip.hash)?; - lsb_res + Ok(lsb_res .received_coins .into_iter() .filter_map(|entry| { @@ -181,13 +191,14 @@ impl BitcoinInterface for d::BitcoinD { None } }) - .collect() + .collect()) } fn confirmed_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> (Vec<(bitcoin::OutPoint, i32, u32)>, Vec) { + ) -> Result<(Vec<(bitcoin::OutPoint, i32, u32)>, Vec), Box> + { // The confirmed and expired coins to be returned. let mut confirmed = Vec::with_capacity(outpoints.len()); let mut expired = Vec::new(); @@ -195,10 +206,7 @@ impl BitcoinInterface for d::BitcoinD { let mut tx_getter = CachedTxGetter::new(self); for op in outpoints { - let res = if let Some(res) = tx_getter - .get_transaction(&op.txid) - .expect("We expect the connection to bitcoind to never fail.") - { + let res = if let Some(res) = tx_getter.get_transaction(&op.txid)? { res } else { log::error!("Transaction not in wallet for coin '{}'.", op); @@ -217,32 +225,23 @@ impl BitcoinInterface for d::BitcoinD { } // If the transaction was dropped from the mempool, discard the coin. - if !self - .is_in_mempool(&op.txid) - .expect("We expect the connection to bitcoind to never fail.") - { + if !self.is_in_mempool(&op.txid)? { expired.push(*op); } } - (confirmed, expired) + Ok((confirmed, expired)) } fn spending_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid)> { + ) -> Result, Box> { let mut spent = Vec::with_capacity(outpoints.len()); for op in outpoints { - if self - .is_spent(op) - .expect("We expect the connection to bitcoind to never fail.") - { - let spending_txid = if let Some(txid) = self - .get_spender_txid(op) - .expect("We expect the connection to bitcoind to never fail.") - { + if self.is_spent(op)? { + let spending_txid = if let Some(txid) = self.get_spender_txid(op)? { txid } else { // TODO: better handling of this edge case. @@ -257,16 +256,19 @@ impl BitcoinInterface for d::BitcoinD { } } - spent + Ok(spent) } fn spent_coins( &self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)], - ) -> ( - Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, - Vec, - ) { + ) -> Result< + ( + Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, + Vec, + ), + Box, + > { // Spend coins to be returned. let mut spent = Vec::with_capacity(outpoints.len()); // Coins whose spending transaction isn't in our local mempool anymore. @@ -275,10 +277,7 @@ impl BitcoinInterface for d::BitcoinD { let mut tx_getter = CachedTxGetter::new(self); for (op, txid) in outpoints { - let res = if let Some(res) = tx_getter - .get_transaction(txid) - .expect("We expect the connection to bitcoind to never fail.") - { + let res = if let Some(res) = tx_getter.get_transaction(txid)? { res } else { log::error!("Could not get tx {} spending coin {}.", txid, op); @@ -294,10 +293,8 @@ impl BitcoinInterface for d::BitcoinD { // If a conflicting transaction was confirmed instead, replace the txid of the // spender for this coin with it and mark it as confirmed. let conflict = res.conflicting_txs.iter().find_map(|txid| { - tx_getter - .get_transaction(txid) - .expect("We expect the connection to bitcoind to never fail.") - .and_then(|tx| { + tx_getter.get_transaction(txid).transpose().and_then(|tx| { + tx.map(|tx| { tx.block.and_then(|block| { // Being part of our watchonly wallet isn't enough, as it could be a // conflicting transaction which spends a different set of coins. Make sure @@ -311,123 +308,123 @@ impl BitcoinInterface for d::BitcoinD { }) }) }) + .transpose() + }) }); - if let Some((txid, block)) = conflict { - spent.push((*op, txid, block)); - continue; + match conflict { + Some(Ok((txid, block))) => { + spent.push((*op, txid, block)); + continue; + } + Some(Err(e)) => return Err(e.into()), + _ => {} } // If the transaction was not confirmed, a conflicting transaction spending this coin // too wasn't mined, but still isn't in our mempool anymore, mark the spend as expired. - if !self - .is_in_mempool(txid) - .expect("We expect the connection to bitcoind to never fail.") - { + if !self.is_in_mempool(txid)? { expired.push(*op); } } - (spent, expired) + Ok((spent, expired)) } - fn common_ancestor(&self, tip: &BlockChainTip) -> Option { - let mut stats = self - .get_block_stats(tip.hash) - .expect("We expect the connection to bitcoind to never fail.")?; + fn common_ancestor( + &self, + tip: &BlockChainTip, + ) -> Result, Box> { + let mut stats = if let Some(stats) = self.get_block_stats(tip.hash)? { + stats + } else { + return Ok(None); + }; let mut ancestor = *tip; while stats.confirmations == -1 { - stats = self - .get_block_stats(stats.previous_blockhash?) - .expect("We expect the connection to bitcoind to never fail.")?; + let prev_hash = if let Some(h) = stats.previous_blockhash { + h + } else { + return Ok(None); + }; + stats = if let Some(stats) = self.get_block_stats(prev_hash)? { + stats + } else { + return Ok(None); + }; ancestor = BlockChainTip { hash: stats.blockhash, height: stats.height, }; } - Some(ancestor) + Ok(Some(ancestor)) } - fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), String> { - match self.broadcast_tx(tx) { - Ok(()) => Ok(()), - Err(BitcoindError::Server(e)) => Err(e.to_string()), - // We assume the Bitcoin backend doesn't fail, so it must be a JSONRPC error. - Err(e) => panic!( - "Unexpected Bitcoin error when broadcast transaction: '{}'.", - e - ), - } + fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), Box> { + Ok(self.broadcast_tx(tx)?) } fn start_rescan( &self, desc: &descriptors::LianaDescriptor, timestamp: u32, - ) -> Result<(), String> { + ) -> Result<(), Box> { // FIXME: in theory i think this could potentially fail to actually start the rescan. - self.start_rescan(desc, timestamp) - .map_err(|e| e.to_string()) + Ok(self.start_rescan(desc, timestamp)?) } - fn rescan_progress(&self) -> Option { - self.rescan_progress() - .expect("We expect the connection to bitcoind to never fail.") + fn rescan_progress(&self) -> Result, Box> { + Ok(self.rescan_progress()?) } - fn block_before_date(&self, timestamp: u32) -> Option { - self.tip_before_timestamp(timestamp) - .expect("We expect the connection to bitcoind to never fail.") + fn block_before_date( + &self, + timestamp: u32, + ) -> Result, Box> { + Ok(self.tip_before_timestamp(timestamp)?) } - fn tip_time(&self) -> Option { - let tip = self - .chain_tip() - .expect("We expect the connection to bitcoind to never fail."); - Some( - self.get_block_stats(tip.hash) - .expect("We expect the connection to bitcoind to never fail.")? - .time, - ) + fn tip_time(&self) -> Result, Box> { + let tip = self.chain_tip()?; + Ok(self.get_block_stats(tip.hash)?.map(|stats| stats.time)) } fn wallet_transaction( &self, txid: &bitcoin::Txid, - ) -> Option<(bitcoin::Transaction, Option)> { - self.get_transaction(txid) - .expect("We expect the connection to bitcoind to never fail.") - .map(|res| (res.tx, res.block)) + ) -> Result)>, Box> { + Ok(self.get_transaction(txid)?.map(|res| (res.tx, res.block))) } - fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec { - self.mempool_txs_spending_prevouts(outpoints) - .expect("We expect the connection to bitcoind to never fail.") + fn mempool_spenders( + &self, + outpoints: &[bitcoin::OutPoint], + ) -> Result, Box> { + let spenders: Result<_, _> = self + .mempool_txs_spending_prevouts(outpoints)? .into_iter() - .filter_map(|txid| { - self.mempool_entry(&txid) - .expect("We expect the connection to bitcoind to never fail.") - }) - .collect() + .filter_map(|txid| self.mempool_entry(&txid).transpose()) + .collect(); + Ok(spenders?) } } // FIXME: do we need to repeat the entire trait implemenation? Isn't there a nicer way? impl BitcoinInterface for sync::Arc> { - fn genesis_block(&self) -> BlockChainTip { + fn genesis_block(&self) -> Result> { self.lock().unwrap().genesis_block() } - fn sync_progress(&self) -> SyncProgress { + fn sync_progress(&self) -> Result> { self.lock().unwrap().sync_progress() } - fn chain_tip(&self) -> BlockChainTip { + fn chain_tip(&self) -> Result> { self.lock().unwrap().chain_tip() } - fn is_in_chain(&self, tip: &BlockChainTip) -> bool { + fn is_in_chain(&self, tip: &BlockChainTip) -> Result> { self.lock().unwrap().is_in_chain(tip) } @@ -435,39 +432,46 @@ impl BitcoinInterface for sync::Arc> &self, tip: &BlockChainTip, descs: &[descriptors::SinglePathLianaDesc], - ) -> Vec { + ) -> Result, Box> { self.lock().unwrap().received_coins(tip, descs) } fn confirmed_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> (Vec<(bitcoin::OutPoint, i32, u32)>, Vec) { + ) -> Result<(Vec<(bitcoin::OutPoint, i32, u32)>, Vec), Box> + { self.lock().unwrap().confirmed_coins(outpoints) } fn spending_coins( &self, outpoints: &[bitcoin::OutPoint], - ) -> Vec<(bitcoin::OutPoint, bitcoin::Txid)> { + ) -> Result, Box> { self.lock().unwrap().spending_coins(outpoints) } fn spent_coins( &self, outpoints: &[(bitcoin::OutPoint, bitcoin::Txid)], - ) -> ( - Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, - Vec, - ) { + ) -> Result< + ( + Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, + Vec, + ), + Box, + > { self.lock().unwrap().spent_coins(outpoints) } - fn common_ancestor(&self, tip: &BlockChainTip) -> Option { + fn common_ancestor( + &self, + tip: &BlockChainTip, + ) -> Result, Box> { self.lock().unwrap().common_ancestor(tip) } - fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), String> { + fn broadcast_tx(&self, tx: &bitcoin::Transaction) -> Result<(), Box> { self.lock().unwrap().broadcast_tx(tx) } @@ -475,30 +479,36 @@ impl BitcoinInterface for sync::Arc> &self, desc: &descriptors::LianaDescriptor, timestamp: u32, - ) -> Result<(), String> { + ) -> Result<(), Box> { self.lock().unwrap().start_rescan(desc, timestamp) } - fn rescan_progress(&self) -> Option { + fn rescan_progress(&self) -> Result, Box> { self.lock().unwrap().rescan_progress() } - fn block_before_date(&self, timestamp: u32) -> Option { + fn block_before_date( + &self, + timestamp: u32, + ) -> Result, Box> { self.lock().unwrap().block_before_date(timestamp) } - fn tip_time(&self) -> Option { + fn tip_time(&self) -> Result, Box> { self.lock().unwrap().tip_time() } fn wallet_transaction( &self, txid: &bitcoin::Txid, - ) -> Option<(bitcoin::Transaction, Option)> { + ) -> Result)>, Box> { self.lock().unwrap().wallet_transaction(txid) } - fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec { + fn mempool_spenders( + &self, + outpoints: &[bitcoin::OutPoint], + ) -> Result, Box> { self.lock().unwrap().mempool_spenders(outpoints) } } diff --git a/src/bitcoin/poller/looper.rs b/src/bitcoin/poller/looper.rs index 83111c4a8..121979ecc 100644 --- a/src/bitcoin/poller/looper.rs +++ b/src/bitcoin/poller/looper.rs @@ -4,7 +4,7 @@ use crate::{ descriptors, }; -use std::{collections::HashSet, sync, time}; +use std::{collections::HashSet, error, sync, time}; use miniscript::bitcoin::{self, secp256k1}; @@ -30,14 +30,14 @@ fn update_coins( previous_tip: &BlockChainTip, descs: &[descriptors::SinglePathLianaDesc], secp: &secp256k1::Secp256k1, -) -> UpdatedCoins { +) -> Result> { let network = db_conn.network(); let curr_coins = db_conn.coins(&[], &[]); log::debug!("Current coins: {:?}", curr_coins); // Start by fetching newly received coins. let mut received = Vec::new(); - for utxo in bit.received_coins(previous_tip, descs) { + for utxo in bit.received_coins(previous_tip, descs)? { let UTxO { outpoint, amount, @@ -102,7 +102,7 @@ fn update_coins( } }) .collect(); - let (confirmed, expired) = bit.confirmed_coins(&to_be_confirmed); + let (confirmed, expired) = bit.confirmed_coins(&to_be_confirmed)?; log::debug!("Newly confirmed coins: {:?}", confirmed); log::debug!("Expired coins: {:?}", expired); @@ -126,7 +126,7 @@ fn update_coins( } }) .collect(); - let spending = bit.spending_coins(&to_be_spent); + let spending = bit.spending_coins(&to_be_spent)?; log::debug!("Newly spending coins: {:?}", spending); // Mark coins in a spending state whose Spend transaction was confirmed as such. Note we @@ -139,21 +139,21 @@ fn update_coins( .map(|coin| (coin.outpoint, coin.spend_txid.expect("Coin is spending"))) .chain(spending.iter().cloned()) .collect(); - let (spent, expired_spending) = bit.spent_coins(spending_coins.as_slice()); + let (spent, expired_spending) = bit.spent_coins(spending_coins.as_slice())?; let spent = spent .into_iter() .map(|(oupoint, txid, block)| (oupoint, txid, block.height, block.time)) .collect(); log::debug!("Newly spent coins: {:?}", spent); - UpdatedCoins { + Ok(UpdatedCoins { received, confirmed, expired, spending, expired_spending, spent, - } + }) } #[derive(Debug, Clone, Copy)] @@ -167,19 +167,22 @@ enum TipUpdate { } // Returns the new block chain tip, if it changed. -fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdate { - let bitcoin_tip = bit.chain_tip(); +fn new_tip( + bit: &impl BitcoinInterface, + current_tip: &BlockChainTip, +) -> Result> { + let bitcoin_tip = bit.chain_tip()?; // If the tip didn't change, there is nothing to update. if current_tip == &bitcoin_tip { - return TipUpdate::Same; + return Ok(TipUpdate::Same); } if bitcoin_tip.height > current_tip.height { // Make sure we are on the same chain. - if bit.is_in_chain(current_tip) { + if bit.is_in_chain(current_tip)? { // All good, we just moved forward. - return TipUpdate::Progress(bitcoin_tip); + return Ok(TipUpdate::Progress(bitcoin_tip)); } } @@ -187,13 +190,13 @@ fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdat // block chain re-organisation. Find the common ancestor between our current chain and // the new chain and return that. The caller will take care of rewinding our state. log::info!("Block chain reorganization detected. Looking for common ancestor."); - if let Some(common_ancestor) = bit.common_ancestor(current_tip) { + if let Some(common_ancestor) = bit.common_ancestor(current_tip)? { log::info!( "Common ancestor found: '{}'. Starting rescan from there. Old tip was '{}'.", common_ancestor, current_tip ); - TipUpdate::Reorged(common_ancestor) + Ok(TipUpdate::Reorged(common_ancestor)) } else { log::error!( "Failed to get common ancestor for tip '{}'. Starting over.", @@ -208,10 +211,10 @@ fn updates( bit: &impl BitcoinInterface, descs: &[descriptors::SinglePathLianaDesc], secp: &secp256k1::Secp256k1, -) { +) -> Result<(), Box> { // Check if there was a new block before updating ourselves. let current_tip = db_conn.chain_tip().expect("Always set at first startup"); - let latest_tip = match new_tip(bit, ¤t_tip) { + let latest_tip = match new_tip(bit, ¤t_tip)? { TipUpdate::Same => current_tip, TipUpdate::Progress(new_tip) => new_tip, TipUpdate::Reorged(new_tip) => { @@ -225,10 +228,10 @@ fn updates( // Then check the state of our coins. Do it even if the tip did not change since last poll, as // we may have unconfirmed transactions. - let updated_coins = update_coins(bit, db_conn, ¤t_tip, descs, secp); + let updated_coins = update_coins(bit, db_conn, ¤t_tip, descs, secp)?; // If the tip changed while we were polling our Bitcoin interface, start over. - if bit.chain_tip() != latest_tip { + if bit.chain_tip()? != latest_tip { log::info!("Chain tip changed while we were updating our state. Starting over."); return updates(db_conn, bit, descs, secp); } @@ -248,6 +251,7 @@ fn updates( } log::debug!("Updates done."); + Ok(()) } // Check if there is any rescan of the backend ongoing or one that just finished. @@ -256,7 +260,7 @@ fn rescan_check( bit: &impl BitcoinInterface, descs: &[descriptors::SinglePathLianaDesc], secp: &secp256k1::Secp256k1, -) { +) -> Result<(), Box> { log::debug!("Checking the state of an ongoing rescan if there is any"); // Check if there is an ongoing rescan. If there isn't and we previously asked for a rescan of @@ -264,7 +268,7 @@ fn rescan_check( // Upon completion of the rescan from the given timestamp on the backend, we rollback our state // down to the height before this timestamp to rescan everything that happened since then. let rescan_timestamp = db_conn.rescan_timestamp(); - if let Some(progress) = bit.rescan_progress() { + if let Some(progress) = bit.rescan_progress()? { log::info!("Rescan progress: {:.2}%.", progress * 100.0); if rescan_timestamp.is_none() { log::warn!("Backend is rescanning but we didn't ask for it."); @@ -276,14 +280,14 @@ fn rescan_check( // no use for the bitcoind implementation of the backend, since bitcoind will always set // the timestamp of the descriptors in the wallet first (and therefore consider it as // rescanned from this height even if it aborts the rescan by being stopped). - let rescan_tip = match bit.block_before_date(timestamp) { + let rescan_tip = match bit.block_before_date(timestamp)? { Some(block) => block, None => { log::error!( "Could not retrieve block height for timestamp '{}'", timestamp ); - return; + return Ok(()); } }; db_conn.rollback_tip(&rescan_tip); @@ -292,20 +296,27 @@ fn rescan_check( "Rolling back our internal tip to '{}' to update our internal state with past transactions.", rescan_tip ); - updates(db_conn, bit, descs, secp) + return updates(db_conn, bit, descs, secp); } else { log::debug!("No ongoing rescan."); } + + Ok(()) } /// If the database chain tip is NULL (first startup), initialize it. -pub fn maybe_initialize_tip(bit: &impl BitcoinInterface, db: &impl DatabaseInterface) { +pub fn maybe_initialize_tip( + bit: &impl BitcoinInterface, + db: &impl DatabaseInterface, +) -> Result<(), Box> { let mut db_conn = db.connection(); if db_conn.chain_tip().is_none() { // TODO: be smarter. We can use the timestamp of the descriptor to get a newer block hash. - db_conn.update_tip(&bit.genesis_block()); + db_conn.update_tip(&bit.genesis_block()?); } + + Ok(()) } pub fn sync_poll_interval() -> time::Duration { @@ -324,8 +335,11 @@ pub fn poll( db: &sync::Arc>, secp: &secp256k1::Secp256k1, descs: &[descriptors::SinglePathLianaDesc], -) { +) -> Result<(), Box> { let mut db_conn = db.connection(); - updates(&mut db_conn, bit, descs, secp); - rescan_check(&mut db_conn, bit, descs, secp); + + updates(&mut db_conn, bit, descs, secp)?; + rescan_check(&mut db_conn, bit, descs, secp)?; + + Ok(()) } diff --git a/src/bitcoin/poller/mod.rs b/src/bitcoin/poller/mod.rs index 522021476..54c073ccb 100644 --- a/src/bitcoin/poller/mod.rs +++ b/src/bitcoin/poller/mod.rs @@ -3,6 +3,7 @@ mod looper; use crate::{bitcoin::BitcoinInterface, database::DatabaseInterface, descriptors}; use std::{ + error, sync::{self, atomic}, thread, time, }; @@ -23,7 +24,7 @@ impl Poller { bit: sync::Arc>, db: sync::Arc>, desc: descriptors::LianaDescriptor, - ) -> Poller { + ) -> Result> { let secp = secp256k1::Secp256k1::verification_only(); let descs = [ desc.receive_descriptor().clone(), @@ -31,18 +32,18 @@ impl Poller { ]; // On first startup the tip may be NULL. Make sure it's set as the poller relies on it. - looper::maybe_initialize_tip(&bit, &db); + looper::maybe_initialize_tip(&bit, &db)?; - Poller { + Ok(Poller { bit, db, secp, descs, - } + }) } /// Update our state from the Bitcoin backend. - pub fn poll_once(&self) { + pub fn poll_once(&self) -> Result<(), Box> { looper::poll(&self.bit, &self.db, &self.secp, &self.descs) } @@ -56,7 +57,7 @@ impl Poller { &self, poll_interval: time::Duration, shutdown: sync::Arc, - ) { + ) -> Result<(), Box> { let mut last_poll = None; let mut synced = false; @@ -81,7 +82,7 @@ impl Poller { // Don't poll until the Bitcoin backend is fully synced. if !synced { - let progress = self.bit.sync_progress(); + let progress = self.bit.sync_progress()?; log::info!( "Block chain synchronization progress: {:.2}% ({} blocks / {} headers)", progress.rounded_up_progress() * 100.0, @@ -94,7 +95,9 @@ impl Poller { } } - looper::poll(&self.bit, &self.db, &self.secp, &self.descs); + looper::poll(&self.bit, &self.db, &self.secp, &self.descs)?; } + + Ok(()) } } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 9eb224497..f6f502f4f 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -62,6 +62,9 @@ pub enum CommandError { /// Overflowing or unhardened derivation index. InvalidDerivationIndex, RbfError(RbfErrorInfo), + // TODO: is anything nicer than a string possible while keeping Clone and Eq? + /// An error when speaking to our Bitcoin backend. + BitcoinInterface(String), } impl fmt::Display for CommandError { @@ -114,6 +117,7 @@ impl fmt::Display for CommandError { write!(f, "Unhardened or overflowing BIP32 derivation index.") } Self::RbfError(e) => write!(f, "RBF error: '{}'.", e), + Self::BitcoinInterface(e) => write!(f, "Bitcoin backend error: {}", e), } } } @@ -169,7 +173,14 @@ impl<'a> BitcoindTxGetter<'a> { impl<'a> TxGetter for BitcoindTxGetter<'a> { fn get_tx(&mut self, txid: &bitcoin::Txid) -> Option { if let hash_map::Entry::Vacant(entry) = self.cache.entry(*txid) { - entry.insert(self.bitcoind.wallet_transaction(txid).map(|wtx| wtx.0)); + match self.bitcoind.wallet_transaction(txid) { + Ok(wtx) => { + entry.insert(wtx.map(|wtx| wtx.0)); + } + Err(e) => { + log::error!("Getting tx {} from bitcoind: {}", txid, e); + } + } } self.cache.get(txid).cloned().flatten() } @@ -278,23 +289,29 @@ impl DaemonControl { impl DaemonControl { /// Get information about the current state of the daemon - pub fn get_info(&self) -> GetInfoResult { + pub fn get_info(&self) -> Result { let mut db_conn = self.db.connection(); let block_height = db_conn.chain_tip().map(|tip| tip.height).unwrap_or(0); let rescan_progress = db_conn .rescan_timestamp() - .map(|_| self.bitcoin.rescan_progress().unwrap_or(1.0)); - GetInfoResult { + .map(|_| self.bitcoin.rescan_progress().map(|p| p.unwrap_or(1.0))) + .transpose() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))?; + Ok(GetInfoResult { version: VERSION.to_string(), network: self.config.bitcoin_config.network, block_height, - sync: self.bitcoin.sync_progress().rounded_up_progress(), + sync: self + .bitcoin + .sync_progress() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))? + .rounded_up_progress(), descriptors: GetInfoDescriptors { main: self.config.main_descriptor.clone(), }, rescan_progress, - } + }) } /// Get a new deposit address. This will always generate a new deposit address, regardless of @@ -612,7 +629,7 @@ impl DaemonControl { let final_tx = spend_psbt.extract_tx_unchecked_fee_rate(); self.bitcoin .broadcast_tx(&final_tx) - .map_err(CommandError::TxBroadcast) + .map_err(|e| CommandError::TxBroadcast(e.to_string())) } /// Create PSBT to replace the given transaction using RBF. @@ -686,6 +703,7 @@ impl DaemonControl { let (min_feerate_vb, descendant_fees) = self .bitcoin .mempool_spenders(&prev_outpoints) + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))? .into_iter() .fold( (1, bitcoin::Amount::from_sat(0)), @@ -870,12 +888,19 @@ impl DaemonControl { let future_timestamp = self .bitcoin .tip_time() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))? .map(|t| timestamp >= t) .unwrap_or(false); if timestamp < MAINNET_GENESIS_TIME || future_timestamp { return Err(CommandError::InsaneRescanTimestamp(timestamp)); } - if db_conn.rescan_timestamp().is_some() || self.bitcoin.rescan_progress().is_some() { + if db_conn.rescan_timestamp().is_some() + || self + .bitcoin + .rescan_progress() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))? + .is_some() + { return Err(CommandError::AlreadyRescanning); } @@ -884,7 +909,7 @@ impl DaemonControl { // rescan of the wallet just after we checked above and did now. self.bitcoin .start_rescan(&self.config.main_descriptor, timestamp) - .map_err(CommandError::RescanTrigger)?; + .map_err(|e| CommandError::RescanTrigger(e.to_string()))?; db_conn.set_rescan(timestamp); Ok(()) @@ -896,7 +921,7 @@ impl DaemonControl { start: u32, end: u32, limit: u64, - ) -> ListTransactionsResult { + ) -> Result { let mut db_conn = self.db.connection(); let txids = db_conn.list_txids(start, end, limit); let transactions = txids @@ -904,35 +929,46 @@ impl DaemonControl { .filter_map(|txid| { // TODO: batch those calls to the Bitcoin backend // so it can in turn optimize its queries. - self.bitcoin - .wallet_transaction(txid) - .map(|(tx, block)| TransactionInfo { - tx, - height: block.map(|b| b.height), - time: block.map(|b| b.time), - }) + Some( + self.bitcoin + .wallet_transaction(txid) + .transpose()? + .map(|(tx, block)| TransactionInfo { + tx, + height: block.map(|b| b.height), + time: block.map(|b| b.time), + }), + ) }) - .collect(); - ListTransactionsResult { transactions } + .collect::>() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))?; + Ok(ListTransactionsResult { transactions }) } /// list_transactions retrieves the transactions with the given txids. - pub fn list_transactions(&self, txids: &[bitcoin::Txid]) -> ListTransactionsResult { + pub fn list_transactions( + &self, + txids: &[bitcoin::Txid], + ) -> Result { let transactions = txids .iter() .filter_map(|txid| { // TODO: batch those calls to the Bitcoin backend // so it can in turn optimize its queries. - self.bitcoin - .wallet_transaction(txid) - .map(|(tx, block)| TransactionInfo { - tx, - height: block.map(|b| b.height), - time: block.map(|b| b.time), - }) + Some( + self.bitcoin + .wallet_transaction(txid) + .transpose()? + .map(|(tx, block)| TransactionInfo { + tx, + height: block.map(|b| b.height), + time: block.map(|b| b.time), + }), + ) }) - .collect(); - ListTransactionsResult { transactions } + .collect::>() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))?; + Ok(ListTransactionsResult { transactions }) } /// Create a transaction that sweeps all coins for which a timelocked recovery path is @@ -957,7 +993,11 @@ impl DaemonControl { // Query the coins that we can spend through the specified recovery path (if no recovery // path specified, use the first available one) from the database. - let current_height = self.bitcoin.chain_tip().height; + let current_height = self + .bitcoin + .chain_tip() + .map_err(|e| CommandError::BitcoinInterface(e.to_string()))? + .height; let timelock = timelock.unwrap_or_else(|| self.config.main_descriptor.first_timelock_value()); let height_delta: i32 = timelock.into(); @@ -1148,7 +1188,7 @@ mod tests { fn getinfo() { let ms = DummyLiana::new(DummyBitcoind::new(), DummyDatabase::new()); // We can query getinfo - ms.handle.control.get_info(); + ms.handle.control.get_info().unwrap(); ms.shutdown(); } @@ -2038,7 +2078,10 @@ mod tests { let control = &ms.handle.control; - let transactions = control.list_confirmed_transactions(0, 4, 10).transactions; + let transactions = control + .list_confirmed_transactions(0, 4, 10) + .unwrap() + .transactions; assert_eq!(transactions.len(), 4); assert_eq!(transactions[0].time, Some(4)); @@ -2053,14 +2096,20 @@ mod tests { assert_eq!(transactions[3].time, Some(1)); assert_eq!(transactions[3].tx, deposit1); - let transactions = control.list_confirmed_transactions(2, 3, 10).transactions; + let transactions = control + .list_confirmed_transactions(2, 3, 10) + .unwrap() + .transactions; assert_eq!(transactions.len(), 2); assert_eq!(transactions[0].time, Some(3)); assert_eq!(transactions[1].time, Some(2)); assert_eq!(transactions[1].tx, deposit2); - let transactions = control.list_confirmed_transactions(2, 3, 1).transactions; + let transactions = control + .list_confirmed_transactions(2, 3, 1) + .unwrap() + .transactions; assert_eq!(transactions.len(), 1); assert_eq!(transactions[0].time, Some(3)); @@ -2170,12 +2219,16 @@ mod tests { let control = &ms.handle.control; - let transactions = control.list_transactions(&[tx1.txid()]).transactions; + let transactions = control + .list_transactions(&[tx1.txid()]) + .unwrap() + .transactions; assert_eq!(transactions.len(), 1); assert_eq!(transactions[0].tx, tx1); let transactions = control .list_transactions(&[tx1.txid(), tx2.txid(), tx3.txid()]) + .unwrap() .transactions; assert_eq!(transactions.len(), 3); diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index f99ee9ef7..e513be7cf 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -222,7 +222,7 @@ fn list_confirmed(control: &DaemonControl, params: Params) -> Result Result Result { @@ -373,7 +373,7 @@ pub fn handle_request(control: &DaemonControl, req: Request) -> Result serde_json::json!(&control.get_info()), + "getinfo" => serde_json::json!(&control.get_info()?), "getnewaddress" => serde_json::json!(&control.get_new_address()), "listcoins" => { let params = req.params; diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index e21ac0e71..ea21fe36e 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -168,7 +168,8 @@ impl From for Error { | commands::CommandError::RecoveryNotAvailable => { Error::new(ErrorCode::InvalidParams, e.to_string()) } - commands::CommandError::RescanTrigger(..) => { + commands::CommandError::RescanTrigger(..) + | commands::CommandError::BitcoinInterface(..) => { Error::new(ErrorCode::InternalError, e.to_string()) } commands::CommandError::TxBroadcast(_) => { diff --git a/src/lib.rs b/src/lib.rs index bf70c92b6..f485f98d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -92,6 +92,7 @@ pub enum StartupError { MissingBitcoindConfig, Database(SqliteDbError), Bitcoind(BitcoindError), + BitcoinInterface(Box), #[cfg(unix)] Daemonization(&'static str), #[cfg(windows)] @@ -116,6 +117,7 @@ impl fmt::Display for StartupError { ), Self::Database(e) => write!(f, "Error initializing database: '{}'.", e), Self::Bitcoind(e) => write!(f, "Error setting up bitcoind interface: '{}'.", e), + Self::BitcoinInterface(e) => write!(f, "Talking to our Bitcoin backend: {}", e), #[cfg(unix)] Self::Daemonization(e) => write!(f, "Error when daemonizing: '{}'.", e), #[cfg(windows)] @@ -377,7 +379,8 @@ impl DaemonHandle { // Setup the Bitcoin poller. Caller is responsible for running it. let bitcoin_poller = - poller::Poller::new(bit.clone(), db.clone(), config.main_descriptor.clone()); + poller::Poller::new(bit.clone(), db.clone(), config.main_descriptor.clone()) + .map_err(StartupError::BitcoinInterface)?; // Finally, set up the API. let control = DaemonControl::new(config, bit, db, secp); @@ -652,7 +655,9 @@ mod tests { let poll_interval = config.bitcoin_config.poll_interval_secs; let DaemonHandle { bitcoin_poller, .. } = DaemonHandle::start_default(config).unwrap(); - bitcoin_poller.poll_forever(poll_interval, shutdown) + bitcoin_poller + .poll_forever(poll_interval, shutdown) + .unwrap() } }); complete_sanity_check(&server); @@ -676,7 +681,9 @@ mod tests { let poll_interval = config.bitcoin_config.poll_interval_secs; let DaemonHandle { bitcoin_poller, .. } = DaemonHandle::start_default(config).unwrap(); - bitcoin_poller.poll_forever(poll_interval, shutdown) + bitcoin_poller + .poll_forever(poll_interval, shutdown) + .unwrap() } }); complete_sanity_check(&server); diff --git a/src/testutils.rs b/src/testutils.rs index 2aa8bcafb..c29aed447 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -7,7 +7,7 @@ use crate::{ use std::{ collections::{HashMap, HashSet}, - env, fs, io, path, process, + env, error, fs, io, path, process, str::FromStr, sync, thread, time, }; @@ -32,94 +32,111 @@ impl DummyBitcoind { } impl BitcoinInterface for DummyBitcoind { - fn genesis_block(&self) -> BlockChainTip { + fn genesis_block(&self) -> Result> { let hash = bitcoin::BlockHash::from_str( "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f", ) .unwrap(); - BlockChainTip { hash, height: 0 } + Ok(BlockChainTip { hash, height: 0 }) } - fn sync_progress(&self) -> SyncProgress { - SyncProgress::new(1.0, 1_000, 1_000) + fn sync_progress(&self) -> Result> { + Ok(SyncProgress::new(1.0, 1_000, 1_000)) } - fn chain_tip(&self) -> BlockChainTip { + fn chain_tip(&self) -> Result> { let hash = bitcoin::BlockHash::from_str( "000000007bc154e0fa7ea32218a72fe2c1bb9f86cf8c9ebf9a715ed27fdb229a", ) .unwrap(); let height = 100; - BlockChainTip { hash, height } + Ok(BlockChainTip { hash, height }) } - fn is_in_chain(&self, _: &BlockChainTip) -> bool { + fn is_in_chain(&self, _: &BlockChainTip) -> Result> { // No reorg - true + Ok(true) } fn received_coins( &self, _: &BlockChainTip, _: &[descriptors::SinglePathLianaDesc], - ) -> Vec { - Vec::new() + ) -> Result, Box> { + Ok(Vec::new()) } fn confirmed_coins( &self, _: &[bitcoin::OutPoint], - ) -> (Vec<(bitcoin::OutPoint, i32, u32)>, Vec) { - (Vec::new(), Vec::new()) + ) -> Result<(Vec<(bitcoin::OutPoint, i32, u32)>, Vec), Box> + { + Ok((Vec::new(), Vec::new())) } - fn spending_coins(&self, _: &[bitcoin::OutPoint]) -> Vec<(bitcoin::OutPoint, bitcoin::Txid)> { - Vec::new() + fn spending_coins( + &self, + _: &[bitcoin::OutPoint], + ) -> Result, Box> { + Ok(Vec::new()) } fn spent_coins( &self, _: &[(bitcoin::OutPoint, bitcoin::Txid)], - ) -> ( - Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, - Vec, - ) { - (Vec::new(), Vec::new()) + ) -> Result< + ( + Vec<(bitcoin::OutPoint, bitcoin::Txid, Block)>, + Vec, + ), + Box, + > { + Ok((Vec::new(), Vec::new())) } - fn common_ancestor(&self, _: &BlockChainTip) -> Option { + fn common_ancestor( + &self, + _: &BlockChainTip, + ) -> Result, Box> { todo!() } - fn broadcast_tx(&self, _: &bitcoin::Transaction) -> Result<(), String> { + fn broadcast_tx(&self, _: &bitcoin::Transaction) -> Result<(), Box> { todo!() } - fn start_rescan(&self, _: &descriptors::LianaDescriptor, _: u32) -> Result<(), String> { + fn start_rescan( + &self, + _: &descriptors::LianaDescriptor, + _: u32, + ) -> Result<(), Box> { todo!() } - fn rescan_progress(&self) -> Option { - None + fn rescan_progress(&self) -> Result, Box> { + Ok(None) } - fn block_before_date(&self, _: u32) -> Option { + fn block_before_date(&self, _: u32) -> Result, Box> { todo!() } - fn tip_time(&self) -> Option { + fn tip_time(&self) -> Result, Box> { todo!() } fn wallet_transaction( &self, txid: &bitcoin::Txid, - ) -> Option<(bitcoin::Transaction, Option)> { - self.txs.get(txid).cloned() + ) -> Result)>, Box> { + Ok(self.txs.get(txid).cloned()) } - fn mempool_spenders(&self, _: &[bitcoin::OutPoint]) -> Vec { - Vec::new() + fn mempool_spenders( + &self, + _: &[bitcoin::OutPoint], + ) -> Result, Box> { + Ok(Vec::new()) } } From 72920b2d030e24caf8523da025fe2d190b29a392 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 8 Jan 2024 16:07:28 +0100 Subject: [PATCH 11/11] bitcoind: remove most expect()'s for invalid fields, propagate errors. --- src/bitcoin/d/mod.rs | 507 ++++++++++++++++++++++++++++--------------- 1 file changed, 326 insertions(+), 181 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index c695d9bc0..d8c447012 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -13,7 +13,7 @@ use utils::{block_before_date, roundup_progress}; use std::{ cmp, collections::{HashMap, HashSet}, - convert::TryInto, + convert::{TryFrom, TryInto}, fs, io, str::FromStr, thread, @@ -59,6 +59,10 @@ pub enum BitcoindError { NetworkMismatch(String /*config*/, String /*bitcoind*/), StartRescan, RescanPastPruneHeight, + MissingOrInvalidField { + command: &'static str, + field: &'static str, + }, } impl BitcoindError { @@ -152,6 +156,11 @@ impl std::fmt::Display for BitcoindError { "Trying to rescan the block chain past the prune block height." ) } + BitcoindError::MissingOrInvalidField { command, field } => write!( + f, + "Missing or invalid field '{}' in result of RPC command '{}'.", + field, command + ), } } } @@ -453,11 +462,13 @@ impl BitcoinD { } fn get_bitcoind_version(&self) -> Result { - Ok(self - .make_node_request("getnetworkinfo", None)? + self.make_node_request("getnetworkinfo", None)? .get("version") .and_then(Json::as_u64) - .expect("Missing or invalid 'version' in 'getnetworkinfo' result?")) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getnetworkinfo", + field: "version", + }) } fn get_network_bip70(&self) -> Result { @@ -465,23 +476,31 @@ impl BitcoinD { .make_node_request("getblockchaininfo", None)? .get("chain") .and_then(Json::as_str) - .expect("Missing or invalid 'chain' in 'getblockchaininfo' result?") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "chain", + })? .to_string()) } fn list_wallets(&self) -> Result, BitcoindError> { - Ok(self - .make_node_request("listwallets", None)? + self.make_node_request("listwallets", None)? .as_array() - .expect("API break, 'listwallets' didn't return an array.") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listwallets", + field: "Didn't return an array", + })? .iter() - .map(|json_str| { - json_str + .map(|json_str| -> Result<_, BitcoindError> { + Ok(json_str .as_str() - .expect("API break: 'listwallets' contains a non-string value") - .to_string() + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listwallets", + field: "Non-string value in array", + })? + .to_string()) }) - .collect()) + .collect::>() } // Get a warning from the result of a wallet command. It was modified in v25 so it's a bit @@ -577,41 +596,58 @@ impl BitcoinD { } fn list_descriptors(&self) -> Result, BitcoindError> { - Ok(self - .make_wallet_request("listdescriptors", None)? + self.make_wallet_request("listdescriptors", None)? .get("descriptors") .and_then(Json::as_array) - .expect("Missing or invalid 'descriptors' field in 'listdescriptors' response") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listdescriptors", + field: "descriptors", + })? .iter() - .map(|elem| { + .map(|elem| -> Result<_, BitcoindError> { let desc = elem .get("desc") .and_then(Json::as_str) - .expect( - "Missing or invalid 'desc' field in 'listdescriptors' response's entries", - ) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listdescriptors", + field: "desc", + })? .to_string(); - let range = elem.get("range").and_then(Json::as_array).map(|a| { - a.iter() - .map(|e| e.as_u64().expect("Invalid range index") as u32) - .collect::>() - .try_into() - .expect("Range is always an array of size 2") - }); + let range = elem + .get("range") + .and_then(Json::as_array) + .map(|a| -> Result<_, BitcoindError> { + a.iter() + .map(|e| -> Result<_, BitcoindError> { + Ok(e.as_u64().ok_or(BitcoindError::MissingOrInvalidField { + command: "listdescriptors", + field: "range", + })? as u32) + }) + .collect::, _>>()? + .try_into() + .map_err(|_| BitcoindError::MissingOrInvalidField { + command: "listdescriptors", + field: "range", + }) + }) + .transpose()?; let timestamp = elem .get("timestamp") .and_then(Json::as_u64) - .expect("A valid timestamp is always present") - .try_into() - .expect("timestamp must fit"); + .and_then(|ts| ts.try_into().ok()) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listdescriptors", + field: "timestamp", + })?; - ListDescEntry { + Ok(ListDescEntry { desc, range, timestamp, - } + }) }) - .collect()) + .collect::>() } fn maybe_unload_watchonly_wallet( @@ -766,15 +802,22 @@ impl BitcoinD { let percentage = chain_info .get("verificationprogress") .and_then(Json::as_f64) - .expect("No valid 'verificationprogress' in getblockchaininfo response?"); - let headers = chain_info - .get("headers") - .and_then(Json::as_u64) - .expect("No valid 'verificationprogress' in getblockchaininfo response?"); - let blocks = chain_info - .get("blocks") - .and_then(Json::as_u64) - .expect("No valid 'blocks' in getblockchaininfo response?"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "verificationprogress", + })?; + let headers = chain_info.get("headers").and_then(Json::as_u64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "headers", + }, + )?; + let blocks = chain_info.get("blocks").and_then(Json::as_u64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "blocks", + }, + )?; Ok(SyncProgress { percentage, headers, @@ -785,19 +828,22 @@ impl BitcoinD { pub fn chain_tip(&self) -> Result { // We use getblockchaininfo to avoid a race between getblockcount and getblockhash let chain_info = self.block_chain_info()?; - let hash = bitcoin::BlockHash::from_str( - chain_info - .get("bestblockhash") - .and_then(Json::as_str) - .expect("No valid 'bestblockhash' in 'getblockchaininfo' response?"), - ) - .expect("Invalid blockhash from bitcoind?"); + let hash = chain_info + .get("bestblockhash") + .and_then(Json::as_str) + .and_then(|s| bitcoin::BlockHash::from_str(s).ok()) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "bestblockhash", + })?; let height: i32 = chain_info .get("blocks") .and_then(Json::as_i64) - .expect("No valid 'blocks' in 'getblockchaininfo' response?") - .try_into() - .expect("Must fit by Bitcoin consensus"); + .and_then(|i| i.try_into().ok()) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockchaininfo", + field: "blocks", + })?; Ok(BlockChainTip { hash, height }) } @@ -807,7 +853,10 @@ impl BitcoinD { Ok(json) => Ok(Some( json.as_str() .and_then(|s| bitcoin::BlockHash::from_str(s).ok()) - .expect("Block hashes returned by bitcoind must be valid"), + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockhash", + field: "", + })?, )), Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, @@ -821,23 +870,22 @@ impl BitcoinD { &self, block_hash: &bitcoin::BlockHash, ) -> Result { - Ok(self - .make_wallet_request( - "listsinceblock", - params!( - Json::String(block_hash.to_string()), - Json::Number(1.into()), // Default for min_confirmations for the returned - Json::Bool(true), // Whether to include watchonly - Json::Bool(false), // Whether to include an array of txs that were removed in reorgs - Json::Bool(true) // Whether to include UTxOs treated as change. - ), - )? - .into()) + self.make_wallet_request( + "listsinceblock", + params!( + Json::String(block_hash.to_string()), + Json::Number(1.into()), // Default for min_confirmations for the returned + Json::Bool(true), // Whether to include watchonly + Json::Bool(false), // Whether to include an array of txs that were removed in reorgs + Json::Bool(true) // Whether to include UTxOs treated as change. + ), + )? + .try_into() } pub fn get_transaction(&self, txid: &bitcoin::Txid) -> Result, BitcoindError> { match self.make_wallet_request("gettransaction", params!(Json::String(txid.to_string()))) { - Ok(json) => Ok(Some(json.into())), + Ok(json) => Ok(Some(json.try_into()?)), Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, .. @@ -886,7 +934,10 @@ impl BitcoinD { params!(Json::Number((list_since_height - 1).into())), ) { res.as_str() - .expect("'getblockhash' result isn't a string") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockhash", + field: "", + })? .to_string() } else { // Possibly a race. @@ -906,10 +957,12 @@ impl BitcoinD { Json::Bool(true) // Whether to include UTxOs treated as change. ), )?; - let transactions = lsb_res - .get("transactions") - .and_then(Json::as_array) - .expect("tx array must be there"); + let transactions = lsb_res.get("transactions").and_then(Json::as_array).ok_or( + BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "transactions", + }, + )?; // Get the spent txid to ignore the entries about this transaction let spent_txid = spent_outpoint.txid.to_string(); @@ -921,10 +974,12 @@ impl BitcoinD { continue; } - let spending_txid = transaction - .get("txid") - .and_then(Json::as_str) - .expect("A valid txid must be present"); + let spending_txid = transaction.get("txid").and_then(Json::as_str).ok_or( + BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "txid", + }, + )?; if visited_txs.contains(&spending_txid) || spent_txid == spending_txid { continue; } else { @@ -942,34 +997,52 @@ impl BitcoinD { let vin = gettx_res .get("decoded") .and_then(|d| d.get("vin").and_then(Json::as_array)) - .expect("A valid vin array must be present"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "decoded or vin", + })?; for input in vin { let txid = input .get("txid") .and_then(Json::as_str) .and_then(|t| bitcoin::Txid::from_str(t).ok()) - .expect("A valid txid must be present"); - let vout = input - .get("vout") - .and_then(Json::as_u64) - .expect("A valid vout must be present") as u32; + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "txid", + })?; + let vout = input.get("vout").and_then(Json::as_u64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "vout", + }, + )? as u32; let input_outpoint = bitcoin::OutPoint { txid, vout }; if spent_outpoint == &input_outpoint { - let spending_txid = - bitcoin::Txid::from_str(spending_txid).expect("Must be a valid txid"); + let spending_txid = bitcoin::Txid::from_str(spending_txid).map_err(|_| { + BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "txid", + } + })?; // If the spending transaction is unconfirmed, there may more than one of them. // Make sure to not return one that RBF'd. let confs = gettx_res .get("confirmations") .and_then(Json::as_i64) - .expect("A valid number of confirmations must always be present."); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "confirmations", + })?; let conflicts = gettx_res .get("walletconflicts") .and_then(Json::as_array) - .expect("A valid list of wallet conflicts must always be present."); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "walletconflicts", + })?; if confs == 0 && !conflicts.is_empty() && !self.is_in_mempool(&spending_txid)? { log::debug!("Noticed '{}' as spending '{}', but is unconfirmed with conflicts and is not in mempool anymore. Discarding it.", &spending_txid, &spent_outpoint); break; @@ -997,33 +1070,41 @@ impl BitcoinD { return Ok(None); } }; - let confirmations = res - .get("confirmations") - .and_then(Json::as_i64) - .expect("Invalid confirmations in `getblockheader` response: not an i64") - as i32; + let confirmations = res.get("confirmations").and_then(Json::as_i64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getblockheader", + field: "confirmations", + }, + )? as i32; let previous_blockhash = res .get("previousblockhash") .and_then(Json::as_str) .map(|s| { - bitcoin::BlockHash::from_str(s) - .expect("Invalid previousblockhash in `getblockheader` response") - }); - let height = res - .get("height") - .and_then(Json::as_i64) - .expect("Invalid height in `getblockheader` response: not an i64") - as i32; - let time = res - .get("time") - .and_then(Json::as_u64) - .expect("Invalid timestamp in `getblockheader` response: not an u64") - as u32; - let median_time_past = res - .get("mediantime") - .and_then(Json::as_u64) - .expect("Invalid median timestamp in `getblockheader` response: not an u64") - as u32; + bitcoin::BlockHash::from_str(s).map_err(|_| BitcoindError::MissingOrInvalidField { + command: "getblockheader", + field: "previousblockhash", + }) + }) + .transpose()?; + let height = res.get("height").and_then(Json::as_i64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getblockheader", + field: "height", + }, + )? as i32; + let time = + res.get("time") + .and_then(Json::as_u64) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getblockheader", + field: "time", + })? as u32; + let median_time_past = res.get("mediantime").and_then(Json::as_u64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getblockheader", + field: "mediantime", + }, + )? as u32; Ok(Some(BlockStats { confirmations, previous_blockhash, @@ -1177,10 +1258,12 @@ impl BitcoinD { timestamp, self.chain_tip()?, |h| { + // FIXME: make block_before_date() return a Result and get rid of this expect. self.get_block_hash(h) .expect("We assume bitcoind connection never fails") }, |h| { + // FIXME: make block_before_date() return a Result and get rid of this expect. self.get_block_stats(h) .expect("We assume bitcoind connection never fails") }, @@ -1199,7 +1282,7 @@ impl BitcoinD { txid: &bitcoin::Txid, ) -> Result, BitcoindError> { match self.make_node_request("getmempoolentry", params!(Json::String(txid.to_string()))) { - Ok(json) => Ok(Some(MempoolEntry::from(json))), + Ok(json) => Ok(Some(MempoolEntry::try_from(json)?)), Err(BitcoindError::Server(jsonrpc::Error::Rpc(jsonrpc::error::RpcError { code: -5, .. @@ -1217,19 +1300,24 @@ impl BitcoinD { .iter() .map(|op| serde_json::json!({"txid": op.txid.to_string(), "vout": op.vout})) .collect(); - Ok(self - .make_node_request("gettxspendingprevout", params!(prevouts))? + self.make_node_request("gettxspendingprevout", params!(prevouts))? .as_array() - .expect("Always returns an array") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettxspendingprevout", + field: "Not an array", + })? .iter() .filter_map(|e| { - e.get("spendingtxid").map(|e| { + e.get("spendingtxid").map(|e| -> Result<_, BitcoindError> { e.as_str() .and_then(|s| bitcoin::Txid::from_str(s).ok()) - .expect("Must be a valid txid if present") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettxspendingprevout", + field: "spendingtxid", + }) }) }) - .collect()) + .collect::>() } /// Stop bitcoind. @@ -1294,17 +1382,25 @@ pub struct LSBlockEntry { pub is_immature: bool, } -impl From<&Json> for LSBlockEntry { - fn from(json: &Json) -> LSBlockEntry { +impl TryFrom<&Json> for LSBlockEntry { + type Error = BitcoindError; + + fn try_from(json: &Json) -> Result { let txid = json .get("txid") .and_then(Json::as_str) .and_then(|s| bitcoin::Txid::from_str(s).ok()) - .expect("bitcoind can't give a bad block hash"); - let vout = json - .get("vout") - .and_then(Json::as_u64) - .expect("bitcoind can't give a bad vout") as u32; + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "txid", + })?; + let vout = + json.get("vout") + .and_then(Json::as_u64) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "vout", + })? as u32; let outpoint = bitcoin::OutPoint { txid, vout }; // Must be a received entry, hence not negative. @@ -1312,7 +1408,10 @@ impl From<&Json> for LSBlockEntry { .get("amount") .and_then(Json::as_f64) .and_then(|a| bitcoin::Amount::from_btc(a).ok()) - .expect("bitcoind won't give us a bad amount"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "amount", + })?; let block_height = json .get("blockheight") .and_then(Json::as_i64) @@ -1322,7 +1421,10 @@ impl From<&Json> for LSBlockEntry { .get("address") .and_then(Json::as_str) .and_then(|s| bitcoin::Address::from_str(s).ok()) - .expect("bitcoind can't give a bad address"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "address", + })?; let parent_descs = json .get("parent_descs") .and_then(Json::as_array) @@ -1335,22 +1437,26 @@ impl From<&Json> for LSBlockEntry { }) .collect::>>() }) - .expect("bitcoind can't give invalid descriptors"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "parent_descs", + })?; - let is_immature = json - .get("category") - .and_then(Json::as_str) - .expect("must be present") - == "immature"; + let is_immature = json.get("category").and_then(Json::as_str).ok_or( + BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "category", + }, + )? == "immature"; - LSBlockEntry { + Ok(LSBlockEntry { outpoint, amount, block_height, address, parent_descs, is_immature, - } + }) } } @@ -1359,12 +1465,17 @@ pub struct LSBlockRes { pub received_coins: Vec, } -impl From for LSBlockRes { - fn from(json: Json) -> LSBlockRes { +impl TryFrom for LSBlockRes { + type Error = BitcoindError; + + fn try_from(json: Json) -> Result { let received_coins = json .get("transactions") .and_then(Json::as_array) - .expect("Array must be present") + .ok_or(BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "transactions", + })? .iter() .filter_map(|j| { // From 'listunspent' help: @@ -1373,20 +1484,25 @@ impl From for LSBlockRes { // "generate" Coinbase transactions received with more than 100 confirmations. // "immature" Coinbase transactions received with 100 or fewer confirmations. // "orphan" Orphaned coinbase transactions received. - let category = j - .get("category") - .and_then(Json::as_str) - .expect("must be present"); + let category = j.get("category").and_then(Json::as_str).ok_or( + BitcoindError::MissingOrInvalidField { + command: "listsinceblock", + field: "category", + }, + ); + let category = match category { + Ok(c) => c, + Err(e) => return Some(Err(e)), + }; if ["receive", "generate", "immature"].contains(&category) { - let lsb_entry: LSBlockEntry = j.into(); - Some(lsb_entry) + Some(j.try_into()) } else { None } }) - .collect(); + .collect::>()?; - LSBlockRes { received_coins } + Ok(LSBlockRes { received_coins }) } } @@ -1399,11 +1515,14 @@ pub struct GetTxRes { pub confirmations: i32, } -impl From for GetTxRes { - fn from(json: Json) -> GetTxRes { - let block_hash = json.get("blockhash").and_then(Json::as_str).map(|s| { - bitcoin::BlockHash::from_str(s).expect("Invalid blockhash in `gettransaction` response") - }); +impl TryFrom for GetTxRes { + type Error = BitcoindError; + + fn try_from(json: Json) -> Result { + let block_hash = json + .get("blockhash") + .and_then(Json::as_str) + .and_then(|s| bitcoin::BlockHash::from_str(s).ok()); let block_height = json .get("blockheight") .and_then(Json::as_i64) @@ -1415,41 +1534,47 @@ impl From for GetTxRes { let conflicting_txs = json .get("walletconflicts") .and_then(Json::as_array) - .map(|array| { + .and_then(|array| { array .iter() - .map(|v| { - bitcoin::Txid::from_str(v.as_str().expect("wrong json format")).unwrap() - }) - .collect() - }); + .map(|v| v.as_str().and_then(|s| bitcoin::Txid::from_str(s).ok())) + .collect::>() + }) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "walletconflicts", + })?; let block = match (block_hash, block_height, block_time) { (Some(hash), Some(height), Some(time)) => Some(Block { hash, time, height }), _ => None, }; - let hex = json + let tx = json .get("hex") .and_then(Json::as_str) - .expect("Must be present in bitcoind response"); - let bytes = Vec::from_hex(hex).expect("bitcoind returned a wrong transaction format"); - let tx: bitcoin::Transaction = bitcoin::consensus::encode::deserialize(&bytes) - .expect("bitcoind returned a wrong transaction format"); + .and_then(|hex| Vec::from_hex(hex).ok()) + .and_then(|bytes| bitcoin::consensus::encode::deserialize(&bytes).ok()) + .ok_or(BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "hex", + })?; let is_coinbase = json .get("generated") .and_then(Json::as_bool) .unwrap_or(false); - let confirmations = json - .get("confirmations") - .and_then(Json::as_i64) - .expect("Must be present in the response") as i32; + let confirmations = json.get("confirmations").and_then(Json::as_i64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "gettransaction", + field: "confirmations", + }, + )? as i32; - GetTxRes { - conflicting_txs: conflicting_txs.unwrap_or_default(), + Ok(GetTxRes { + conflicting_txs, block, tx, is_coinbase, confirmations, - } + }) } } @@ -1501,19 +1626,26 @@ pub struct MempoolEntry { pub fees: MempoolEntryFees, } -impl From for MempoolEntry { - fn from(json: Json) -> MempoolEntry { - let vsize = json - .get("vsize") - .and_then(Json::as_u64) - .expect("Must be present in bitcoind response"); +impl TryFrom for MempoolEntry { + type Error = BitcoindError; + + fn try_from(json: Json) -> Result { + let vsize = json.get("vsize").and_then(Json::as_u64).ok_or( + BitcoindError::MissingOrInvalidField { + command: "getmempoolentry", + field: "vsize", + }, + )?; let fees = json .get("fees") .as_ref() - .expect("Must be present in bitcoind response") - .into(); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getmempoolentry", + field: "fees", + })? + .try_into()?; - MempoolEntry { vsize, fees } + Ok(MempoolEntry { vsize, fees }) } } @@ -1523,19 +1655,32 @@ pub struct MempoolEntryFees { pub descendant: bitcoin::Amount, } -impl From<&&Json> for MempoolEntryFees { - fn from(json: &&Json) -> MempoolEntryFees { - let json = json.as_object().expect("fees must be an object"); +impl TryFrom<&&Json> for MempoolEntryFees { + type Error = BitcoindError; + + fn try_from(json: &&Json) -> Result { + let json = json + .as_object() + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getmempoolentry", + field: "fees", + })?; let base = json .get("base") .and_then(Json::as_f64) .and_then(|a| bitcoin::Amount::from_btc(a).ok()) - .expect("Must be present and a valid amount"); + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getmempoolentry", + field: "base", + })?; let descendant = json .get("descendant") .and_then(Json::as_f64) .and_then(|a| bitcoin::Amount::from_btc(a).ok()) - .expect("Must be present and a valid amount"); - MempoolEntryFees { base, descendant } + .ok_or(BitcoindError::MissingOrInvalidField { + command: "getmempoolentry", + field: "descendant", + })?; + Ok(MempoolEntryFees { base, descendant }) } }