Merge #344: lib: on Windows, delete leftover watchonly wallet in bitcoind datadir

f8e5bb22acd1fe975a0ec74785293da9f6438d57 bitcoind: don't crash on existing watchonly wallet, return an error. (Antoine Poinsot)
b43a90d52ad2ded33d69d3e2e4bc7ce12dcad4c6 lib: on Windows, delete leftover watchonly wallet in bitcoind datadir (Antoine Poinsot)

Pull request description:

  The Windows hack for the watchonly wallet comes back to bite us. The assumption that on a fresh data dir no watchonly wallet already exist does not hold on Windows anymore. Work around it by deleting potential leftover, and bail out of starting up if we detected one but couldn't delete it for whatever reason.

  Thanks to @kloaec for the bug report.

ACKs for top commit:
  edouardparis:
    ACK f8e5bb22acd1fe975a0ec74785293da9f6438d57

Tree-SHA512: 94026f1c944a50e89de2d82a2ade164b2fa9e3dc3858c448d04821e3eee90db51803e7767333eb914841eb7f50dc53cdffa9bb109ca97c6bdb45a8173bb09eaa

src/bitcoin/d/mod.rs

@@ -460,26 +460,28 @@ impl BitcoinD {
             })
     }
 
-    fn create_wallet(&self, wallet_path: String) -> Option<String> {
-        let res = self.make_node_request(
-            "createwallet",
-            &params!(
-                Json::String(wallet_path),
-                Json::Bool(true), // watchonly
-                Json::Bool(true), // blank
-            ),
-        );
+    fn create_wallet(&self, wallet_path: String) -> Result<(), String> {
+        let res = self
+            .make_fallible_node_request(
+                "createwallet",
+                &params!(
+                    Json::String(wallet_path),
+                    Json::Bool(true), // watchonly
+                    Json::Bool(true), // blank
+                ),
+            )
+            .map_err(|e| e.to_string())?;
 
         if let Some(warning) = res.get("warning").and_then(Json::as_str) {
             if !warning.is_empty() {
-                return Some(warning.to_string());
+                return Err(warning.to_string());
             }
         }
         if res.get("name").is_none() {
-            return Some("Unknown error when create watchonly wallet".to_string());
+            return Err("Unknown error when create watchonly wallet".to_string());
         }
 
-        None
+        Ok(())
     }
 
     // Import the receive and change descriptors from the multipath descriptor to bitcoind.
@@ -567,12 +569,11 @@ impl BitcoinD {
         }
 
         // Now create the wallet and import the main descriptor.
-        if let Some(err) = self.create_wallet(self.watchonly_wallet_path.clone()) {
-            return Err(BitcoindError::Wallet(
-                self.watchonly_wallet_path.clone(),
-                WalletError::Creating(err),
-            ));
-        }
+        self.create_wallet(self.watchonly_wallet_path.clone())
+            .map_err(|e| {
+                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) {
             return Err(BitcoindError::Wallet(
                 self.watchonly_wallet_path.clone(),

src/lib.rs

@@ -86,6 +86,8 @@ pub enum StartupError {
     DefaultDataDirNotFound,
     DatadirCreation(path::PathBuf, io::Error),
     MissingBitcoindConfig,
+    WindowsCantGuessBitcoindDatadir(path::PathBuf),
+    WindowsBitcoindWatchonlyDeletion(path::PathBuf, io::Error),
     Database(SqliteDbError),
     Bitcoind(BitcoindError),
     #[cfg(unix)]
@@ -108,6 +110,15 @@ impl fmt::Display for StartupError {
                 f,
                 "Our Bitcoin interface is bitcoind but we have no 'bitcoind_config' entry in the configuration."
             ),
+            Self::WindowsCantGuessBitcoindDatadir(cookie_path) => write!(
+                f,
+                "Cannot guess the path to the bitcoind data directory from the cookie file whose path is '{}'.",
+                cookie_path.as_path().to_string_lossy()
+            ),
+            Self::WindowsBitcoindWatchonlyDeletion(path, e) => write!(
+                f,
+                "Error deleting bitcoind watchonly wallet at '{}': {}", path.as_path().to_string_lossy(), e
+            ),
             Self::Database(e) => write!(f, "Error initializing database: '{}'.", e),
             Self::Bitcoind(e) => write!(f, "Error setting up bitcoind interface: '{}'.", e),
             #[cfg(unix)]
@@ -184,6 +195,48 @@ fn setup_sqlite(
     Ok(sqlite)
 }
 
+// Windows-specific utility to remove a leftover watchonly wallet within bitcoind's datadir.
+#[cfg(windows)]
+fn maybe_delete_watchonly_wallet(
+    bitcoind_cookie_path: &path::Path,
+    bitcoin_net: miniscript::bitcoin::Network,
+    wallet_name: &str,
+) -> Result<(), StartupError> {
+    log::info!(
+        "Trying to guess where the watchonly wallet would be stored in bitcoind's data directory from the cookie path. \
+        This might not work if you are using a custom path for the cookie file (very unlikely). In this case please delete the \
+        leftover watchonly wallet in bitcoind's datadir by hand if there is any."
+    );
+    // For the main network both the wallet and the cookie file are stored at the root of the
+    // datadir. For test networks the wallet is in "<datadir>/<network>/wallets/<wallet_name>/" and
+    // the cookie file in "<datadir>/<network>/".
+    let parent_dir = bitcoind_cookie_path.parent().ok_or_else(|| {
+        StartupError::WindowsCantGuessBitcoindDatadir(bitcoind_cookie_path.to_path_buf())
+    })?;
+    let wallet_path = match bitcoin_net {
+        miniscript::bitcoin::Network::Bitcoin => parent_dir.join(wallet_name),
+        miniscript::bitcoin::Network::Testnet
+        | miniscript::bitcoin::Network::Signet
+        | miniscript::bitcoin::Network::Regtest => parent_dir.join("wallets").join(wallet_name),
+    };
+
+    if wallet_path.exists() {
+        log::info!(
+            "Found a leftover watchonly wallet at '{}'. Deleting it.",
+            wallet_path.as_path().to_string_lossy()
+        );
+        fs::remove_dir_all(&wallet_path)
+            .map_err(|e| StartupError::WindowsBitcoindWatchonlyDeletion(wallet_path, e))?;
+    } else {
+        log::info!(
+            "No leftover watchonly wallet found at '{}'.",
+            wallet_path.as_path().to_string_lossy()
+        );
+    }
+
+    Ok(())
+}
+
 // Connect to bitcoind. Setup the watchonly wallet, and do some sanity checks.
 // If all went well, returns the interface to bitcoind.
 fn setup_bitcoind(
@@ -200,17 +253,30 @@ fn setup_bitcoind(
         .iter()
         .collect();
     #[cfg(windows)]
-    let wo_path = path::Path::new("lianad_watchonly_wallet");
+    let wo_name = "lianad_watchonly_wallet";
+    #[cfg(windows)]
+    let wo_path = path::Path::new(wo_name);
 
+    let bitcoind_config = config
+        .bitcoind_config
+        .as_ref()
+        .ok_or(StartupError::MissingBitcoindConfig)?;
     let bitcoind = BitcoinD::new(
-        config
-            .bitcoind_config
-            .as_ref()
-            .ok_or(StartupError::MissingBitcoindConfig)?,
+        bitcoind_config,
         wo_path.to_str().expect("Must be valid unicode").to_string(),
     )?;
     bitcoind.node_sanity_checks(config.bitcoin_config.network)?;
     if fresh_data_dir {
+        // Because of the hack above, the assumption that whenever the data directory is fresh a
+        // watchonly wallet doesn't exist doesn't hold for Windows. Make sure it does by removing
+        // any leftover Liana watchonly wallet from bitcoind's data dir.
+        #[cfg(windows)]
+        maybe_delete_watchonly_wallet(
+            &bitcoind_config.cookie_path,
+            config.bitcoin_config.network,
+            wo_name,
+        )?;
+
         bitcoind.create_watchonly_wallet(&config.main_descriptor)?;
         log::info!("Created a new watchonly wallet on bitcoind.");
     }