From c9d86f1c75be693e796591be9f1f468687728600 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 30 Aug 2023 15:53:48 +0200 Subject: [PATCH 1/2] lib: implement a superior workaround for the watchonly wallet on Windows See the added comment for the details. No need to store the watchonly wallet under bitcoind's datadir anymore. :tada: :tada: --- src/bitcoin/d/mod.rs | 2 +- src/lib.rs | 96 ++++++-------------------------------------- 2 files changed, 14 insertions(+), 84 deletions(-) diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs index 395dfdee..3efbaa25 100644 --- a/src/bitcoin/d/mod.rs +++ b/src/bitcoin/d/mod.rs @@ -607,7 +607,7 @@ impl BitcoinD { .collect() } - pub fn maybe_unload_watchonly_wallet(&self, watchonly_wallet_path: String) { + fn maybe_unload_watchonly_wallet(&self, watchonly_wallet_path: String) { 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()) { diff --git a/src/lib.rs b/src/lib.rs index 7ed4f670..dd8ee8a5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -89,8 +89,6 @@ pub enum StartupError { DefaultDataDirNotFound, DatadirCreation(path::PathBuf, io::Error), MissingBitcoindConfig, - WindowsCantGuessBitcoindDatadir(path::PathBuf), - WindowsBitcoindWatchonlyDeletion(path::PathBuf, io::Error), Database(SqliteDbError), Bitcoind(BitcoindError), #[cfg(unix)] @@ -113,15 +111,6 @@ 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)] @@ -198,54 +187,6 @@ 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: &BitcoinD, - 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 "//wallets//" and - // the cookie file in "//". - 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), - net => panic!( - "Unsupported network '{}', unknown at the time of writing.", - net - ), - }; - - if wallet_path.exists() { - bitcoind.maybe_unload_watchonly_wallet(wallet_path.to_string_lossy().to_string()); - log::info!( - "Deleting the leftover watchonly wallet at '{}'.", - 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( @@ -253,40 +194,29 @@ fn setup_bitcoind( data_dir: &path::Path, fresh_data_dir: bool, ) -> Result { - // NOTE: this is a hack! We normally store the watchonly wallet within our data directory. - // But on windows bitcoind would prefix the wallet path with "C:\\\\?" when calling - // 'loadwallet'. Therefore instead on Windows store the wallet.dat in bitcoind's data directory - // instead by not providing an absolute path but the name of a wallet. - #[cfg(not(windows))] let wo_path: path::PathBuf = [data_dir, path::Path::new("lianad_watchonly_wallet")] .iter() .collect(); - #[cfg(windows)] - let wo_name = "lianad_watchonly_wallet"; - #[cfg(windows)] - let wo_path = path::Path::new(wo_name); + let wo_path_str = wo_path.to_str().expect("Must be valid unicode").to_string(); + // NOTE: On Windows, paths are canonicalized with a "\\?\" prefix to tell Windows to interpret + // the string "as is" and to ignore the maximum size of a path. HOWEVER this is not properly + // handled by most implementations of the C++ STL's std::filesystem. Therefore bitcoind would + // fail to find the wallet if we didn't strip this prefix. It's not ideal, but a lesser evil + // than other workarounds i could think about. + // See https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#win32-file-namespaces + // about the prefix. + // See https://stackoverflow.com/questions/71590689/how-to-properly-handle-windows-paths-with-the-long-path-prefix-with-stdfilesys + // for a discussion of how one C++ STL implementation handles this. + #[cfg(target_os = "windows")] + let wo_path_str = wo_path_str.replace("\\\\?\\", "").replace("\\\\?", ""); let bitcoind_config = config .bitcoind_config .as_ref() .ok_or(StartupError::MissingBitcoindConfig)?; - let bitcoind = BitcoinD::new( - bitcoind_config, - wo_path.to_str().expect("Must be valid unicode").to_string(), - )?; + let bitcoind = BitcoinD::new(bitcoind_config, wo_path_str)?; 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, - &bitcoind_config.cookie_path, - config.bitcoin_config.network, - wo_name, - )?; - log::info!("Creating a new watchonly wallet on bitcoind."); bitcoind.create_watchonly_wallet(&config.main_descriptor)?; log::info!("Watchonly wallet created."); From 5680ad27ecce8409d582ebf834bdecb3650b9233 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 30 Aug 2023 17:11:24 +0200 Subject: [PATCH 2/2] lib: on Windows, migrate the watchonly wallet from bitcoind datadir We used to store it there, if it's not within our own datadir copy it from where it would have been stored by Liana v1. Note we don't conditionally compile this on Windows so the codepath can be tested with a functional test. --- src/lib.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++++ tests/test_misc.py | 21 ++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index dd8ee8a5..b0a577e9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -187,6 +187,70 @@ fn setup_sqlite( Ok(sqlite) } +// Try to copy the watchonly wallet from its former location on Windows to its new location. +fn copy_watchonly_wallet( + bitcoind_cookie_path: &path::Path, + bitcoin_net: miniscript::bitcoin::Network, + wallet_name: &str, + new_wallet_path: &path::Path, +) -> bool { + // 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 "//wallets//" and + // the cookie file in "//". + let parent_dir = match bitcoind_cookie_path.parent() { + Some(d) => d, + None => { + log::error!("Could not find the parent directory of the bitcoind cookie file."); + return false; + } + }; + 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) + .join("wallet.dat"), + net => panic!( + "Unsupported network '{}', unknown at the time of writing.", + net + ), + }; + + if wallet_path.exists() { + log::info!( + "Found the watchonly wallet file at the former location: '{}'. Copying it to our own datadir.", + wallet_path.as_path().to_string_lossy() + ); + if let Err(e) = fs::create_dir(new_wallet_path) { + log::error!( + "Error while creating the watchonly wallet directory at {}: {}", + wallet_path.to_string_lossy(), + e + ); + return false; + } + let new_wallet_dat_path = new_wallet_path.join("wallet.dat"); + if let Err(e) = fs::copy(wallet_path, new_wallet_dat_path) { + log::error!( + "Error while copying the watchonly wallet to our own datadir: {}", + e + ); + false + } else { + log::info!("Successfully copied the watchonly wallet file."); + true + } + } else { + log::error!( + "No watchonly wallet found at '{}'.", + wallet_path.as_path().to_string_lossy() + ); + false + } +} + // Connect to bitcoind. Setup the watchonly wallet, and do some sanity checks. // If all went well, returns the interface to bitcoind. fn setup_bitcoind( @@ -220,6 +284,24 @@ fn setup_bitcoind( log::info!("Creating a new watchonly wallet on bitcoind."); bitcoind.create_watchonly_wallet(&config.main_descriptor)?; log::info!("Watchonly wallet created."); + } else if !wo_path.exists() && !cfg!(test) { + // TODO: remove this hack. + // NOTE: this is Windows-specific but we don't gate it upon a windows target to be able to + // test this codepath in a functional test. + log::info!( + "A data directory exists with no watchonly wallet. This is most likely due to \ + having been created to an older version of Liana on Windows, where the watchonly \ + wallet would live under bitcoind's datadir. Trying to find the older wallet and copy it in our own datadir." + ); + let wo_name = "lianad_watchonly_wallet"; + if !copy_watchonly_wallet( + &bitcoind_config.cookie_path, + config.bitcoin_config.network, + wo_name, + wo_path.as_path(), + ) { + panic!("Cannot continue without a watchonly wallet. Please contact support."); + } } log::info!("Loading our watchonly wallet on bitcoind."); bitcoind.maybe_load_watchonly_wallet()?; diff --git a/tests/test_misc.py b/tests/test_misc.py index 6156a865..55841759 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1,5 +1,6 @@ import logging import pytest +import shutil import time from fixtures import * @@ -337,3 +338,23 @@ def test_retry_on_workqueue_exceeded(lianad, bitcoind, executor): # We should have retried the request to bitcoind, which should now succeed along with the call. # This just checks the response we get is sane, nothing particular with this field. assert "block_height" in f_liana.result(TIMEOUT) + + +def test_wo_wallet_copy_from_bitcoind_datadir(lianad, bitcoind): + """Simulate an old install on Windows and make sure the watchonly wallet gets migrated.""" + wo_name = "lianad_watchonly_wallet" + old_wo_path = os.path.join(bitcoind.bitcoin_dir, "regtest", "wallets", wo_name) + new_wo_path = os.path.join(lianad.datadir, "regtest", wo_name) + + # Start lianad. It'll have created the watchonly within our datadir. Move it where it would + # have been created by Liana v1 on Windows. + lianad.stop() + shutil.copytree(new_wo_path, old_wo_path) + shutil.rmtree(new_wo_path) + assert not os.path.isdir(new_wo_path) + + # Now restart lianad. It should detect it within the bitcoind datadir and copy it to its own. + lianad.start() + assert os.path.isdir(new_wo_path) + assert lianad.is_in_log("A data directory exists with no watchonly wallet. This is most likely due to.*") + assert lianad.is_in_log("Successfully copied the watchonly wallet file.")