diff options
| -rw-r--r-- | src/wallet/transaction.cpp | 5 | ||||
| -rw-r--r-- | src/wallet/transaction.h | 8 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 107 | ||||
| -rwxr-xr-x | test/functional/wallet_migration.py | 56 |
4 files changed, 146 insertions, 30 deletions
diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp index a46846c1d4..4f78fe7520 100644 --- a/src/wallet/transaction.cpp +++ b/src/wallet/transaction.cpp @@ -24,4 +24,9 @@ int64_t CWalletTx::GetTxTime() const int64_t n = nTimeSmart; return n ? n : nTimeReceived; } + +void CWalletTx::CopyFrom(const CWalletTx& _tx) +{ + *this = _tx; +} } // namespace wallet diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 3b9cde9832..0d0821e857 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -334,11 +334,15 @@ public: const uint256& GetWitnessHash() const { return tx->GetWitnessHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } +private: // Disable copying of CWalletTx objects to prevent bugs where instances get // copied in and out of the mapWallet map, and fields are updated in the // wrong copy. - CWalletTx(CWalletTx const &) = delete; - void operator=(CWalletTx const &x) = delete; + CWalletTx(const CWalletTx&) = default; + CWalletTx& operator=(const CWalletTx&) = default; +public: + // Instead have an explicit copy function + void CopyFrom(const CWalletTx&); }; struct WalletTxOrderComparator { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cb8671c8e7..162d7f9ec7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3908,6 +3908,14 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector<uint256> txids_to_delete; + std::unique_ptr<WalletBatch> watchonly_batch; + if (data.watchonly_wallet) { + watchonly_batch = std::make_unique<WalletBatch>(data.watchonly_wallet->GetDatabase()); + // Copy the next tx order pos to the watchonly wallet + LOCK(data.watchonly_wallet->cs_wallet); + data.watchonly_wallet->nOrderPosNext = nOrderPosNext; + watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext); + } for (const auto& [_pos, wtx] : wtxOrdered) { if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) { // Check it is the watchonly wallet's @@ -3916,12 +3924,20 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) LOCK(data.watchonly_wallet->cs_wallet); if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { // Add to watchonly wallet - if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) { - error = _("Error: Could not add watchonly tx to watchonly wallet"); + const uint256& hash = wtx->GetHash(); + const CWalletTx& to_copy_wtx = *wtx; + if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { + if (!new_tx) return false; + ins_wtx.SetTx(to_copy_wtx.tx); + ins_wtx.CopyFrom(to_copy_wtx); + return true; + })) { + error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); return false; } + watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); // Mark as to remove from this wallet - txids_to_delete.push_back(wtx->GetHash()); + txids_to_delete.push_back(hash); continue; } } @@ -3930,6 +3946,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } } + watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { std::vector<uint256> deleted_txids; @@ -4066,6 +4083,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseOptions options; options.require_existing = false; options.require_create = true; + options.require_format = DatabaseFormat::SQLITE; + + WalletContext empty_context; + empty_context.args = context.args; // Make the wallets options.create_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_DESCRIPTORS; @@ -4081,8 +4102,14 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector<bilingual_str> warnings; std::string wallet_name = wallet.GetName() + "_watchonly"; - data->watchonly_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_name, options, status, error); + if (!database) { + error = strprintf(_("Wallet file creation failed: %s"), error); + return false; + } + + data->watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->watchonly_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -4112,8 +4139,14 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector<bilingual_str> warnings; std::string wallet_name = wallet.GetName() + "_solvables"; - data->solvable_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(wallet_name, options, status, error); + if (!database) { + error = strprintf(_("Wallet file creation failed: %s"), error); + return false; + } + + data->solvable_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->solvable_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -4216,47 +4249,69 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle success = DoMigration(*local_wallet, context, error, res); } + // In case of reloading failure, we need to remember the wallet dirs to remove + // Set is used as it may be populated with the same wallet directory paths multiple times, + // both before and after reloading. This ensures the set is complete even if one of the wallets + // fails to reload. + std::set<fs::path> wallet_dirs; if (success) { - // Migration successful, unload the wallet locally, then reload it. - assert(local_wallet.use_count() == 1); - local_wallet.reset(); - res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + // Migration successful, unload all wallets locally, then reload them. + const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) { + assert(to_reload.use_count() == 1); + std::string name = to_reload->GetName(); + wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path()); + to_reload.reset(); + to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + return to_reload != nullptr; + }; + // Reload the main wallet + success = reload_wallet(local_wallet); + res.wallet = local_wallet; res.wallet_name = wallet_name; - } else { + if (success && res.watchonly_wallet) { + // Reload watchonly + success = reload_wallet(res.watchonly_wallet); + } + if (success && res.solvables_wallet) { + // Reload solvables + success = reload_wallet(res.solvables_wallet); + } + } + if (!success) { // Migration failed, cleanup // Copy the backup to the actual wallet dir fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); - // Remember this wallet's walletdir to remove after unloading - std::vector<fs::path> wallet_dirs; - wallet_dirs.emplace_back(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); - - // Unload the wallet locally - assert(local_wallet.use_count() == 1); - local_wallet.reset(); - // Make list of wallets to cleanup std::vector<std::shared_ptr<CWallet>> created_wallets; + if (local_wallet) created_wallets.push_back(std::move(local_wallet)); if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet)); if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); // Get the directories to remove after unloading for (std::shared_ptr<CWallet>& w : created_wallets) { - wallet_dirs.emplace_back(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); + wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); } // Unload the wallets for (std::shared_ptr<CWallet>& w : created_wallets) { - if (!RemoveWallet(context, w, /*load_on_start=*/false)) { - error += _("\nUnable to cleanup failed migration"); - return util::Error{error}; + if (w->HaveChain()) { + // Unloading for wallets that were loaded for normal use + if (!RemoveWallet(context, w, /*load_on_start=*/false)) { + error += _("\nUnable to cleanup failed migration"); + return util::Error{error}; + } + UnloadWallet(std::move(w)); + } else { + // Unloading for wallets in local context + assert(w.use_count() == 1); + w.reset(); } - UnloadWallet(std::move(w)); } // Delete the wallet directories - for (fs::path& dir : wallet_dirs) { + for (const fs::path& dir : wallet_dirs) { fs::remove_all(dir); } diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 286dcb5fda..aede9281d5 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -6,11 +6,15 @@ import random import shutil +import struct +import time + from test_framework.address import ( script_to_p2sh, key_to_p2pkh, key_to_p2wpkh, ) +from test_framework.bdb import BTREE_MAGIC from test_framework.descriptors import descsum_create from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework @@ -20,6 +24,7 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, find_vout_for_address, + sha256sum_file, ) from test_framework.wallet_util import ( get_generate_key, @@ -311,12 +316,17 @@ class WalletMigrationTest(BitcoinTestFramework): sent_watchonly_txid = send["txid"] self.generate(self.nodes[0], 1) + received_watchonly_tx_info = imports0.gettransaction(received_watchonly_txid, True) + received_sent_watchonly_tx_info = imports0.gettransaction(received_sent_watchonly_txid, True) balances = imports0.getbalances() spendable_bal = balances["mine"]["trusted"] watchonly_bal = balances["watchonly"]["trusted"] assert_equal(len(imports0.listtransactions(include_watchonly=True)), 4) + # Mock time forward a bit so we can check that tx metadata is preserved + self.nodes[0].setmocktime(int(time.time()) + 100) + # Migrate imports0.migratewallet() assert_equal(imports0.getwalletinfo()["descriptors"], True) @@ -334,8 +344,12 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(watchonly_info["descriptors"], True) self.assert_is_sqlite("imports0_watchonly") assert_equal(watchonly_info["private_keys_enabled"], False) - watchonly.gettransaction(received_watchonly_txid) - watchonly.gettransaction(received_sent_watchonly_txid) + received_migrated_watchonly_tx_info = watchonly.gettransaction(received_watchonly_txid) + assert_equal(received_watchonly_tx_info["time"], received_migrated_watchonly_tx_info["time"]) + assert_equal(received_watchonly_tx_info["timereceived"], received_migrated_watchonly_tx_info["timereceived"]) + received_sent_migrated_watchonly_tx_info = watchonly.gettransaction(received_sent_watchonly_txid) + assert_equal(received_sent_watchonly_tx_info["time"], received_sent_migrated_watchonly_tx_info["time"]) + assert_equal(received_sent_watchonly_tx_info["timereceived"], received_sent_migrated_watchonly_tx_info["timereceived"]) watchonly.gettransaction(sent_watchonly_txid) assert_equal(watchonly.getbalance(), watchonly_bal) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", watchonly.gettransaction, received_txid) @@ -827,6 +841,43 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.unloadwallet() + def test_failed_migration_cleanup(self): + self.log.info("Test that a failed migration is cleaned up") + wallet = self.create_legacy_wallet("failed") + + # Make a copy of the wallet with the solvables wallet name so that we are unable + # to create the solvables wallet when migrating, thus failing to migrate + wallet.unloadwallet() + solvables_path = self.nodes[0].wallets_path / "failed_solvables" + shutil.copytree(self.nodes[0].wallets_path / "failed", solvables_path) + original_shasum = sha256sum_file(solvables_path / "wallet.dat") + + self.nodes[0].loadwallet("failed") + + # Add a multisig so that a solvables wallet is created + wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) + wallet.importaddress(get_generate_key().p2pkh_addr) + + assert_raises_rpc_error(-4, "Failed to create database", wallet.migratewallet) + + assert "failed" in self.nodes[0].listwallets() + assert "failed_watchonly" not in self.nodes[0].listwallets() + assert "failed_solvables" not in self.nodes[0].listwallets() + + assert not (self.nodes[0].wallets_path / "failed_watchonly").exists() + # Since the file in failed_solvables is one that we put there, migration shouldn't touch it + assert solvables_path.exists() + new_shasum = sha256sum_file(solvables_path / "wallet.dat") + assert_equal(original_shasum, new_shasum) + + wallet.unloadwallet() + # Check the wallet we tried to migrate is still BDB + with open(self.nodes[0].wallets_path / "failed" / "wallet.dat", "rb") as f: + data = f.read(16) + _, _, magic = struct.unpack("QII", data) + assert_equal(magic, BTREE_MAGIC) + + def run_test(self): self.generate(self.nodes[0], 101) @@ -845,6 +896,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_migrate_raw_p2sh() self.test_conflict_txs() self.test_hybrid_pubkey() + self.test_failed_migration_cleanup() if __name__ == '__main__': WalletMigrationTest().main() |