Merge #659: gui: installer: more robust poll for cookie file at bitcoind startup

76bb74371f7d33603ac0864b9ecea014a3cc0cda gui: installer: more robust poll for cookie file at bitcoind startup (Antoine Poinsot)

Pull request description:

  We would formerly wait a definite number of seconds for the cookie file to appear. But this approach presents a fundamental issue: how long is enough before we can be reasonably sure bitcoind will never start?

  For instance on mainnet the checks performed at startup could take more than the 3 seconds we would previously wait for. If it does we would have incorrectly errored and let bitcoind run in the background.

  Instead, assume bitcoind would exit if it errors. Until it does so, continue polling for the cookie file. This approach also presents drawbacks (for instance what if bitcoind is performing a very long operation before creating the cookie file?), but the former approach wouldn't be an acceptable solution in this case either. And the new one is preferable as its failure scenario seems much less probable.

ACKs for top commit:
  edouardparis:
    ACK 76bb74371f7d33603ac0864b9ecea014a3cc0cda

Tree-SHA512: a0d4af1cadbea99c4c9f5ca6b7e642a79e97c7a35e829704144be5edb84c9c398872b7fbaab3af73600589194512747ac8da9cb7787a362bde3c6ef797f4ec29

gui/src/bitcoind.rs

@@ -4,6 +4,8 @@ use liana::{
 };
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
+use std::thread;
+use std::time;
 use tokio::sync::Mutex;
 
 use tracing::{info, warn};
@@ -187,24 +189,39 @@ impl Bitcoind {
             .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));
+        // We've started bitcoind in the background, however it may fail to start for whatever
+        // reason. And we need its JSONRPC interface to be available to continue. Thus wait for it
+        // to have created the cookie file, regularly checking it did not fail to start.
+        loop {
+            match process.try_wait() {
+                Ok(None) => {}
+                Err(e) => log::error!("Error while trying to wait for bitcoind: {}", e),
+                Ok(Some(status)) => {
+                    log::error!("Bitcoind exited with status '{}'", status);
+                    match process.wait_with_output() {
+                        Err(e) => {
+                            tracing::error!("Error while waiting for bitcoind to finish: {}", e)
+                        }
+                        Ok(o) => {
+                            tracing::error!("stderr: {}", String::from_utf8_lossy(&o.stderr));
+                        }
+                    }
+                    return Err(StartInternalBitcoindError::CookieFileNotFound(
+                        config.cookie_path.to_string_lossy().into_owned(),
+                    ));
                 }
             }
-            return Err(StartInternalBitcoindError::CookieFileNotFound(
-                config.cookie_path.to_string_lossy().into_owned(),
-            ));
+            if config.cookie_path.exists() {
+                log::info!("Bitcoind seems to have successfully started.");
+                break;
+            }
+            log::info!("Waiting for bitcoind to start.");
+            thread::sleep(time::Duration::from_millis(500));
         }
+
         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()))?;
 

gui/src/utils/mod.rs

@@ -1,21 +1,5 @@
-use std::path::Path;
-
 #[cfg(test)]
 pub mod sandbox;
 
 #[cfg(test)]
 pub mod mock;
-
-/// Polls for a file's existence at given interval up to a maximum number of polls.
-/// Returns `true` once file exists and otherwise `false`.
-pub fn poll_for_file(path: &Path, interval_millis: u64, max_polls: u16) -> bool {
-    for i in 0..max_polls {
-        if path.exists() {
-            return true;
-        }
-        if i < max_polls.saturating_sub(1) {
-            std::thread::sleep(std::time::Duration::from_millis(interval_millis));
-        }
-    }
-    false
-}