From 4c4efebe5bc6560a82bfc4f348ed2663e726a66d Mon Sep 17 00:00:00 2001 From: edouard Date: Wed, 30 Aug 2023 19:57:22 +0200 Subject: [PATCH] gui: encapsulate bitcoind process management To know and manage the fact that gui or installer started a bitcoind sub process, the process child is wrapped and passed by the different state of the app. --- gui/src/app/mod.rs | 13 ++- gui/src/bitcoind.rs | 123 +++++++++++++++++++---------- gui/src/installer/context.rs | 3 + gui/src/installer/message.rs | 4 +- gui/src/installer/mod.rs | 8 +- gui/src/installer/step/bitcoind.rs | 88 ++++++--------------- gui/src/installer/view.rs | 5 +- gui/src/loader.rs | 89 ++++++++++++--------- gui/src/main.rs | 17 ++-- 9 files changed, 185 insertions(+), 165 deletions(-) diff --git a/gui/src/app/mod.rs b/gui/src/app/mod.rs index 42f1aec8..dc449008 100644 --- a/gui/src/app/mod.rs +++ b/gui/src/app/mod.rs @@ -31,7 +31,7 @@ use state::{ use crate::{ app::{cache::Cache, error::Error, menu::Menu, wallet::Wallet}, - bitcoind::stop_internal_bitcoind, + bitcoind::Bitcoind, daemon::{embedded::EmbeddedDaemon, Daemon}, }; @@ -42,6 +42,7 @@ pub struct App { config: Config, wallet: Arc, daemon: Arc, + internal_bitcoind: Option, } impl App { @@ -51,6 +52,7 @@ impl App { config: Config, daemon: Arc, data_dir: PathBuf, + internal_bitcoind: Option, ) -> (App, Command) { let state: Box = Home::new(wallet.clone(), &cache.coins).into(); let cmd = state.load(daemon.clone()); @@ -62,6 +64,7 @@ impl App { config, daemon, wallet, + internal_bitcoind, }, cmd, ) @@ -116,12 +119,8 @@ impl App { if !self.daemon.is_external() { self.daemon.stop(); info!("Internal daemon stopped"); - if self.config.internal_bitcoind_exe_config.is_some() { - if let Some(daemon_config) = self.daemon.config() { - if let Some(bitcoind_config) = &daemon_config.bitcoind_config { - stop_internal_bitcoind(bitcoind_config); - } - } + if let Some(bitcoind) = &self.internal_bitcoind { + bitcoind.stop(); } } } diff --git a/gui/src/bitcoind.rs b/gui/src/bitcoind.rs index e3939740..275b5452 100644 --- a/gui/src/bitcoind.rs +++ b/gui/src/bitcoind.rs @@ -1,5 +1,6 @@ use liana::{config::BitcoindConfig, miniscript::bitcoin}; use std::path::Path; +use std::sync::Arc; use tracing::{info, warn}; @@ -46,51 +47,89 @@ impl std::fmt::Display for StartInternalBitcoindError { } } } - -/// Start internal bitcoind for the given network. -pub fn start_internal_bitcoind( - network: &bitcoin::Network, - bitcoind_datadir: &Path, - exe_path: &Path, -) -> Result { - let datadir_path_str = bitcoind_datadir - .canonicalize() - .map_err(|e| StartInternalBitcoindError::CouldNotCanonicalizeDataDir(e.to_string()))? - .to_str() - .ok_or_else(|| { - StartInternalBitcoindError::CouldNotCanonicalizeDataDir( - "Couldn't convert path to str.".to_string(), - ) - })? - .to_string(); - #[cfg(target_os = "windows")] - // See https://github.com/rust-lang/rust/issues/42869. - let datadir_path_str = datadir_path_str.replace("\\\\?\\", "").replace("\\\\?", ""); - let args = vec![ - format!("-chain={}", network.to_core_arg()), - format!("-datadir={}", datadir_path_str), - ]; - let mut command = std::process::Command::new(exe_path); - #[cfg(target_os = "windows")] - let command = command.creation_flags(CREATE_NO_WINDOW); - command - .args(&args) - .stdout(std::process::Stdio::null()) // We still get bitcoind's logs in debug.log. - .stderr(std::process::Stdio::piped()) - .spawn() - .map_err(|e| StartInternalBitcoindError::CommandError(e.to_string())) +#[derive(Debug, Clone)] +pub struct Bitcoind { + _process: Arc, + pub config: BitcoindConfig, } -/// Stop (internal) bitcoind. -pub fn stop_internal_bitcoind(bitcoind_config: &BitcoindConfig) { - match liana::BitcoinD::new(bitcoind_config, "internal_bitcoind_stop".to_string()) { - Ok(bitcoind) => { - info!("Stopping internal bitcoind..."); - bitcoind.stop(); - info!("Stopped liana managed bitcoind"); +impl Bitcoind { + /// Start internal bitcoind for the given network. + pub fn start( + network: &bitcoin::Network, + mut config: BitcoindConfig, + bitcoind_datadir: &Path, + exe_path: &Path, + ) -> Result { + let datadir_path_str = bitcoind_datadir + .canonicalize() + .map_err(|e| StartInternalBitcoindError::CouldNotCanonicalizeDataDir(e.to_string()))? + .to_str() + .ok_or_else(|| { + StartInternalBitcoindError::CouldNotCanonicalizeDataDir( + "Couldn't convert path to str.".to_string(), + ) + })? + .to_string(); + + // See https://github.com/rust-lang/rust/issues/42869. + #[cfg(target_os = "windows")] + let datadir_path_str = datadir_path_str.replace("\\\\?\\", "").replace("\\\\?", ""); + + let args = vec![ + format!("-chain={}", network.to_core_arg()), + format!("-datadir={}", datadir_path_str), + ]; + let mut command = std::process::Command::new(exe_path); + + #[cfg(target_os = "windows")] + let command = command.creation_flags(CREATE_NO_WINDOW); + + let process = command + .args(&args) + .stdout(std::process::Stdio::null()) // We still get bitcoind's logs in debug.log. + .stderr(std::process::Stdio::piped()) + .spawn() + .map_err(|e| StartInternalBitcoindError::CommandError(e.to_string()))?; + + if !crate::utils::poll_for_file(&config.cookie_path, 200, 15) { + match process.wait_with_output() { + Err(e) => { + tracing::error!("Error while waiting for bitcoind to finish: {}", e) + } + Ok(o) => { + tracing::error!("Exit status: {}", o.status); + tracing::error!("stderr: {}", String::from_utf8_lossy(&o.stderr)); + } + } + return Err(StartInternalBitcoindError::CookieFileNotFound( + config.cookie_path.to_string_lossy().into_owned(), + )); } - Err(e) => { - warn!("Could not create interface to internal bitcoind: '{}'.", e); + config.cookie_path = config.cookie_path.canonicalize().map_err(|e| { + StartInternalBitcoindError::CouldNotCanonicalizeCookiePath(e.to_string()) + })?; + + liana::BitcoinD::new(&config, "internal_bitcoind_start".to_string()) + .map_err(|e| StartInternalBitcoindError::BitcoinDError(e.to_string()))?; + + Ok(Self { + config, + _process: Arc::new(process), + }) + } + + /// Stop (internal) bitcoind. + pub fn stop(&self) { + match liana::BitcoinD::new(&self.config, "internal_bitcoind_stop".to_string()) { + Ok(bitcoind) => { + info!("Stopping internal bitcoind..."); + bitcoind.stop(); + info!("Stopped liana managed bitcoind"); + } + Err(e) => { + warn!("Could not create interface to internal bitcoind: '{}'.", e); + } } } } diff --git a/gui/src/installer/context.rs b/gui/src/installer/context.rs index 7d1a6bfb..6bca248b 100644 --- a/gui/src/installer/context.rs +++ b/gui/src/installer/context.rs @@ -8,6 +8,7 @@ use crate::{ settings::{KeySetting, Settings, WalletSetting}, wallet::DEFAULT_WALLET_NAME, }, + bitcoind::Bitcoind, hw::HardwareWalletConfig, signer::Signer, }; @@ -36,6 +37,7 @@ pub struct Context { pub bitcoind_is_external: bool, pub internal_bitcoind_config: Option, pub internal_bitcoind_exe_config: Option, + pub internal_bitcoind: Option, } impl Context { @@ -55,6 +57,7 @@ impl Context { bitcoind_is_external: true, internal_bitcoind_config: None, internal_bitcoind_exe_config: None, + internal_bitcoind: None, } } diff --git a/gui/src/installer/message.rs b/gui/src/installer/message.rs index 9aaf9477..05b2dd06 100644 --- a/gui/src/installer/message.rs +++ b/gui/src/installer/message.rs @@ -5,7 +5,7 @@ use liana::miniscript::{ use std::path::PathBuf; use super::Error; -use crate::{download::Progress, hw::HardwareWallet}; +use crate::{bitcoind::Bitcoind, download::Progress, hw::HardwareWallet}; use async_hwi::DeviceKind; #[derive(Debug, Clone)] @@ -14,7 +14,7 @@ pub enum Message { ParticipateWallet, ImportWallet, UserActionDone(bool), - Exit(PathBuf), + Exit(PathBuf, Option), Clibpboard(String), Next, Skip, diff --git a/gui/src/installer/mod.rs b/gui/src/installer/mod.rs index c8ada36d..b581c075 100644 --- a/gui/src/installer/mod.rs +++ b/gui/src/installer/mod.rs @@ -18,7 +18,6 @@ use std::sync::{Arc, Mutex}; use crate::{ app::config::InternalBitcoindExeConfig, app::{config as gui_config, settings as gui_settings}, - bitcoind::stop_internal_bitcoind, signer::Signer, }; @@ -84,11 +83,10 @@ impl Installer { .expect("There is always a step") .stop(); // Now use context to determine what to stop. - if self.context.internal_bitcoind_config.is_some() { - if let Some(bitcoind_config) = &self.context.bitcoind_config { - stop_internal_bitcoind(bitcoind_config); - } + if let Some(bitcoind) = &self.context.internal_bitcoind { + bitcoind.stop(); } + self.context.internal_bitcoind = None; } fn next(&mut self) -> Command { diff --git a/gui/src/installer/step/bitcoind.rs b/gui/src/installer/step/bitcoind.rs index fac66db0..89a3e95b 100644 --- a/gui/src/installer/step/bitcoind.rs +++ b/gui/src/installer/step/bitcoind.rs @@ -12,14 +12,14 @@ use iced::{Command, Subscription}; use liana::{config::BitcoindConfig, miniscript::bitcoin::Network}; #[cfg(any(target_os = "macos", target_os = "linux"))] use tar::Archive; -use tracing::{error, info}; +use tracing::info; use jsonrpc::{client::Client, simple_http::SimpleHttpTransport}; use liana_ui::{component::form, widget::*}; use crate::{ - bitcoind::{start_internal_bitcoind, stop_internal_bitcoind, StartInternalBitcoindError}, + bitcoind::{Bitcoind, StartInternalBitcoindError}, download, installer::{ context::Context, @@ -28,7 +28,6 @@ use crate::{ step::Step, view, Error, InternalBitcoindExeConfig, }, - utils::poll_for_file, }; // The approach for tracking download progress is taken from @@ -680,6 +679,7 @@ pub struct InternalBitcoindStep { error: Option, exe_download: Option, install_state: Option, + internal_bitcoind: Option, } impl From for Box { @@ -702,6 +702,7 @@ impl InternalBitcoindStep { error: None, exe_download: None, install_state: None, + internal_bitcoind: None, } } } @@ -727,11 +728,9 @@ impl Step for InternalBitcoindStep { if let Message::InternalBitcoind(msg) = message { match msg { message::InternalBitcoindMsg::Previous => { - if self.internal_bitcoind_config.is_some() { - if let Some(bitcoind_config) = &self.bitcoind_config { - stop_internal_bitcoind(bitcoind_config); - self.started = None; - } + if let Some(bitcoind) = &self.internal_bitcoind { + bitcoind.stop(); + self.started = None; } return Command::perform(async {}, |_| Message::Previous); } @@ -865,39 +864,9 @@ impl Step for InternalBitcoindStep { return Command::none(); } }; - let handle = match start_internal_bitcoind( - &self.network, - &exe_config.data_dir, - &exe_config.exe_path, - ) { - Err(e) => { - self.started = Some(Err(StartInternalBitcoindError::CommandError( - e.to_string(), - ))); - return Command::none(); - } - Ok(h) => h, - }; - // Need to wait for cookie file to appear. let cookie_path = internal_bitcoind_cookie_path(&self.bitcoind_datadir, &self.network); - if !poll_for_file(&cookie_path, 200, 15) { - error!("Cookie file still not present after 3 seconds. Waiting for the bitcoind process to finish."); - match handle.wait_with_output() { - Err(e) => { - error!("Error while waiting for bitcoind to finish: {}", e) - } - Ok(o) => { - error!("Exit status: {}", o.status); - error!("stderr: {}", String::from_utf8_lossy(&o.stderr)); - } - } - self.started = - Some(Err(StartInternalBitcoindError::CookieFileNotFound( - cookie_path.to_string_lossy().into_owned(), - ))); - return Command::none(); - } + let rpc_port = self .internal_bitcoind_config .as_ref() @@ -907,36 +876,30 @@ impl Step for InternalBitcoindStep { .get(&self.network) .expect("Already added") .rpc_port; - let bitcoind_config = match cookie_path.canonicalize() { - Ok(cookie_path) => BitcoindConfig { + + match Bitcoind::start( + &self.network, + BitcoindConfig { cookie_path, addr: internal_bitcoind_address(rpc_port), }, + &exe_config.data_dir, + &exe_config.exe_path, + ) { Err(e) => { - self.started = Some(Err( - StartInternalBitcoindError::CouldNotCanonicalizeCookiePath( - e.to_string(), - ), - )); + self.started = Some(Err(StartInternalBitcoindError::CommandError( + e.to_string(), + ))); return Command::none(); } - }; - match liana::BitcoinD::new( - &bitcoind_config, - "internal_bitcoind_connection_check".to_string(), - ) { - Ok(_) => { + Ok(bitcoind) => { self.error = None; - self.bitcoind_config = Some(bitcoind_config); + self.bitcoind_config = Some(bitcoind.config.clone()); self.exe_config = Some(exe_config); self.started = Some(Ok(())); + self.internal_bitcoind = Some(bitcoind); } - Err(e) => { - self.started = Some(Err( - StartInternalBitcoindError::BitcoinDError(e.to_string()), - )); - } - } + }; } } }; @@ -978,6 +941,7 @@ impl Step for InternalBitcoindStep { ctx.bitcoind_config = self.bitcoind_config.clone(); ctx.internal_bitcoind_config = self.internal_bitcoind_config.clone(); ctx.internal_bitcoind_exe_config = self.exe_config.clone(); + ctx.internal_bitcoind = self.internal_bitcoind.clone(); self.error = None; return true; } @@ -997,10 +961,8 @@ impl Step for InternalBitcoindStep { fn stop(&self) { // In case the installer is closed before changes written to context, stop bitcoind. - if let Some(Ok(_)) = self.started { - if let Some(bitcoind_config) = &self.bitcoind_config { - stop_internal_bitcoind(bitcoind_config); - } + if let Some(bitcoind) = &self.internal_bitcoind { + bitcoind.stop(); } } diff --git a/gui/src/installer/view.rs b/gui/src/installer/view.rs index ce7a0e4a..e4947e1b 100644 --- a/gui/src/installer/view.rs +++ b/gui/src/installer/view.rs @@ -1248,7 +1248,10 @@ pub fn install<'a>( .push(Container::new(text("Installed !"))) .push(Container::new( button::primary(None, "Start") - .on_press(Message::Exit(path.clone())) + .on_press(Message::Exit( + path.clone(), + context.internal_bitcoind.clone(), + )) .width(Length::Fixed(200.0)), )) .align_items(Alignment::Center) diff --git a/gui/src/loader.rs b/gui/src/loader.rs index 85347e62..6c6e45f0 100644 --- a/gui/src/loader.rs +++ b/gui/src/loader.rs @@ -24,9 +24,8 @@ use crate::{ config::{Config as GUIConfig, InternalBitcoindExeConfig}, wallet::{Wallet, WalletError}, }, - bitcoind::{start_internal_bitcoind, stop_internal_bitcoind, StartInternalBitcoindError}, + bitcoind::{Bitcoind, StartInternalBitcoindError}, daemon::{client, embedded::EmbeddedDaemon, model::*, Daemon, DaemonError}, - utils, }; type Lianad = client::Lianad; @@ -36,6 +35,7 @@ pub struct Loader { pub network: bitcoin::Network, pub gui_config: GUIConfig, pub daemon_started: bool, + pub internal_bitcoind: Option, step: Step, } @@ -55,8 +55,18 @@ pub enum Step { pub enum Message { View(ViewMessage), Syncing(Result), - Synced(Result<(Arc, Cache, Arc), Error>), - Started(Result, Error>), + Synced( + Result< + ( + Arc, + Cache, + Arc, + Option, + ), + Error, + >, + ), + Started(Result<(Arc, Option), Error>), Loaded(Result, Error>), Failure(DaemonError), } @@ -66,6 +76,7 @@ impl Loader { datadir_path: PathBuf, gui_config: GUIConfig, network: bitcoin::Network, + internal_bitcoind: Option, ) -> (Self, Command) { let path = gui_config .daemon_rpc_path @@ -79,6 +90,7 @@ impl Loader { gui_config, step: Step::Connecting, daemon_started: false, + internal_bitcoind, }, Command::perform(connect(path), Message::Loaded), ) @@ -125,9 +137,13 @@ impl Loader { Command::none() } - fn on_start(&mut self, res: Result, Error>) -> Command { + fn on_start( + &mut self, + res: Result<(Arc, Option), Error>, + ) -> Command { match res { - Ok(daemon) => { + Ok((daemon, bitcoind)) => { + self.internal_bitcoind = bitcoind; self.step = Step::Syncing { daemon: daemon.clone(), progress: 0.0, @@ -156,6 +172,7 @@ impl Loader { self.gui_config.clone(), self.datadir_path.clone(), self.network, + self.internal_bitcoind.take(), ), Message::Synced, ); @@ -183,18 +200,9 @@ impl Loader { info!("Internal daemon stopped"); } } - if self.gui_config.internal_bitcoind_exe_config.is_some() { - if let Ok(daemon_config) = - Config::from_file(self.gui_config.daemon_config_path.as_ref().cloned()) - { - if let Some(bitcoind_config) = &daemon_config.bitcoind_config { - stop_internal_bitcoind(bitcoind_config); - } else { - warn!("Liana daemon config does not have bitcoind config"); - } - } else { - warn!("Liana gui cannot access daemon config to stop bitcoind"); - } + + if let Some(bitcoind) = &self.internal_bitcoind { + bitcoind.stop(); } } @@ -205,6 +213,7 @@ impl Loader { self.datadir_path.clone(), self.gui_config.clone(), self.network, + self.internal_bitcoind.clone(), ); *self = loader; cmd @@ -239,7 +248,16 @@ pub async fn load_application( gui_config: GUIConfig, datadir_path: PathBuf, network: bitcoin::Network, -) -> Result<(Arc, Cache, Arc), Error> { + internal_bitcoind: Option, +) -> Result< + ( + Arc, + Cache, + Arc, + Option, + ), + Error, +> { let coins = daemon.list_coins().map(|res| res.coins)?; let spend_txs = daemon.list_spend_transactions()?; let cache = Cache { @@ -253,7 +271,7 @@ pub async fn load_application( let wallet = Wallet::new(info.descriptors.main).load_settings(&gui_config, &datadir_path, network)?; - Ok((Arc::new(wallet), cache, daemon)) + Ok((Arc::new(wallet), cache, daemon, internal_bitcoind)) } #[derive(Clone, Debug)] @@ -351,8 +369,9 @@ async fn connect(socket_path: PathBuf) -> Result, pub async fn start_bitcoind_and_daemon( config_path: PathBuf, bitcoind_exe_config: Option, -) -> Result, Error> { +) -> Result<(Arc, Option), Error> { let config = Config::from_file(Some(config_path)).map_err(Error::Config)?; + let mut bitcoind: Option = None; if let Some(exe_config) = bitcoind_exe_config { if let Some(bitcoind_config) = &config.bitcoind_config { // Check if bitcoind is already running before trying to start it. @@ -361,23 +380,15 @@ pub async fn start_bitcoind_and_daemon( info!("Internal bitcoind is already running"); } else { info!("Starting internal bitcoind"); - start_internal_bitcoind( - &config.bitcoin_config.network, - &exe_config.data_dir, - &exe_config.exe_path, - ) - .map_err(Error::Bitcoind)?; - if !utils::poll_for_file(&bitcoind_config.cookie_path, 200, 15) { - return Err(Error::Bitcoind( - StartInternalBitcoindError::CookieFileNotFound( - bitcoind_config.cookie_path.to_string_lossy().into_owned(), - ), - )); - } - liana::BitcoinD::new(bitcoind_config, "internal_bitcoind_start".to_string()) - .map_err(|e| { - Error::Bitcoind(StartInternalBitcoindError::BitcoinDError(e.to_string())) - })?; + bitcoind = Some( + Bitcoind::start( + &config.bitcoin_config.network, + bitcoind_config.clone(), + &exe_config.data_dir, + &exe_config.exe_path, + ) + .map_err(Error::Bitcoind)?, + ); } } } @@ -386,7 +397,7 @@ pub async fn start_bitcoind_and_daemon( let daemon = EmbeddedDaemon::start(config)?; - Ok(Arc::new(daemon)) + Ok((Arc::new(daemon), bitcoind)) } async fn sync( diff --git a/gui/src/main.rs b/gui/src/main.rs index 479d6a9a..c70f669d 100644 --- a/gui/src/main.rs +++ b/gui/src/main.rs @@ -142,7 +142,7 @@ impl Application for GUI { network, cfg.log_level().unwrap_or(LevelFilter::INFO), ); - let (loader, command) = Loader::new(datadir_path, cfg, network); + let (loader, command) = Loader::new(datadir_path, cfg, network, None); ( Self { state: State::Loader(Box::new(loader)), @@ -189,14 +189,14 @@ impl Application for GUI { network, cfg.log_level().unwrap_or(LevelFilter::INFO), ); - let (loader, command) = Loader::new(datadir_path, cfg, network); + let (loader, command) = Loader::new(datadir_path, cfg, network, None); self.state = State::Loader(Box::new(loader)); command.map(|msg| Message::Load(Box::new(msg))) } _ => l.update(*msg).map(|msg| Message::Launch(Box::new(msg))), }, (State::Installer(i), Message::Install(msg)) => { - if let installer::Message::Exit(path) = *msg { + if let installer::Message::Exit(path, internal_bitcoind) = *msg { let cfg = app::Config::from_file(&path).unwrap(); let daemon_cfg = DaemonConfig::from_file(cfg.daemon_config_path.clone()).unwrap(); @@ -212,8 +212,12 @@ impl Application for GUI { cfg.log_level().unwrap_or(LevelFilter::INFO), ); self.logger.remove_install_log_file(datadir_path.clone()); - let (loader, command) = - Loader::new(datadir_path, cfg, daemon_cfg.bitcoin_config.network); + let (loader, command) = Loader::new( + datadir_path, + cfg, + daemon_cfg.bitcoin_config.network, + internal_bitcoind, + ); self.state = State::Loader(Box::new(loader)); command.map(|msg| Message::Load(Box::new(msg))) } else { @@ -226,13 +230,14 @@ impl Application for GUI { State::Launcher(Box::new(Launcher::new(loader.datadir_path.clone()))); Command::none() } - loader::Message::Synced(Ok((wallet, cache, daemon))) => { + loader::Message::Synced(Ok((wallet, cache, daemon, bitcoind))) => { let (app, command) = App::new( cache, wallet, loader.gui_config.clone(), daemon, loader.datadir_path.clone(), + bitcoind, ); self.state = State::App(app); command.map(|msg| Message::Run(Box::new(msg)))