From 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 9 Aug 2024 20:25:27 -0300 Subject: [PATCH 1/7] wallet: fix, detect blank legacy wallets in IsLegacy Blank legacy wallets do not have active SPKM. They can only be detected by checking the descriptors' flag or the db format. This enables the migration of blank legacy wallets in the GUI. --- src/wallet/wallet.cpp | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e31950bebc8c..20b44f7c24c6d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1037,22 +1037,22 @@ bool CWallet::IsSpentKey(const CScript& scriptPubKey) const if (IsAddressPreviouslySpent(dest)) { return true; } - if (IsLegacy()) { - LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); - assert(spk_man != nullptr); - for (const auto& keyid : GetAffectedKeys(scriptPubKey, *spk_man)) { - WitnessV0KeyHash wpkh_dest(keyid); - if (IsAddressPreviouslySpent(wpkh_dest)) { - return true; - } - ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); - if (IsAddressPreviouslySpent(sh_wpkh_dest)) { - return true; - } - PKHash pkh_dest(keyid); - if (IsAddressPreviouslySpent(pkh_dest)) { - return true; - } + + LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); + if (!spk_man) return false; + + for (const auto& keyid : GetAffectedKeys(scriptPubKey, *spk_man)) { + WitnessV0KeyHash wpkh_dest(keyid); + if (IsAddressPreviouslySpent(wpkh_dest)) { + return true; + } + ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); + if (IsAddressPreviouslySpent(sh_wpkh_dest)) { + return true; + } + PKHash pkh_dest(keyid); + if (IsAddressPreviouslySpent(pkh_dest)) { + return true; } } return false; @@ -1625,7 +1625,9 @@ isminetype CWallet::IsMine(const CScript& script) const } // Legacy wallet - if (IsLegacy()) return GetLegacyScriptPubKeyMan()->IsMine(script); + if (LegacyScriptPubKeyMan* spkm = GetLegacyScriptPubKeyMan()) { + return spkm->IsMine(script); + } return ISMINE_NO; } @@ -3558,7 +3560,8 @@ std::set CWallet::GetScriptPubKeyMans(const CScript& script) c Assume(std::all_of(spk_mans.begin(), spk_mans.end(), [&script, &sigdata](ScriptPubKeyMan* spkm) { return spkm->CanProvide(script, sigdata); })); // Legacy wallet - if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) spk_mans.insert(GetLegacyScriptPubKeyMan()); + LegacyScriptPubKeyMan* spkm = GetLegacyScriptPubKeyMan(); + if (spkm && spkm->CanProvide(script, sigdata)) spk_mans.insert(spkm); return spk_mans; } @@ -3588,7 +3591,8 @@ std::unique_ptr CWallet::GetSolvingProvider(const CScript& scri } // Legacy wallet - if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) return GetLegacyScriptPubKeyMan()->GetSolvingProvider(script); + LegacyScriptPubKeyMan* spkm = GetLegacyScriptPubKeyMan(); + if (spkm && spkm->CanProvide(script, sigdata)) return spkm->GetSolvingProvider(script); return nullptr; } @@ -3845,11 +3849,7 @@ void CWallet::DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool intern bool CWallet::IsLegacy() const { - if (m_internal_spk_managers.count(OutputType::LEGACY) == 0) { - return false; - } - auto spk_man = dynamic_cast(m_internal_spk_managers.at(OutputType::LEGACY)); - return spk_man != nullptr; + return !IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS); } DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const From 6b3676be3e5b6e407876031791172f441b359295 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Wed, 14 Aug 2024 16:43:46 +0200 Subject: [PATCH 2/7] test: refactor: move `read_xor_key`/`util_xor` helpers to util module --- test/functional/feature_reindex.py | 16 ++++++---------- test/functional/test_framework/util.py | 13 +++++++++++++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py index 504b3dfff44bc..2961a2d35645b 100755 --- a/test/functional/feature_reindex.py +++ b/test/functional/feature_reindex.py @@ -12,7 +12,11 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import MAGIC_BYTES -from test_framework.util import assert_equal +from test_framework.util import ( + assert_equal, + read_xor_key, + util_xor, +) class ReindexTest(BitcoinTestFramework): @@ -39,15 +43,7 @@ def out_of_order(self): # we're generating them rather than getting them from peers), so to # test out-of-order handling, swap blocks 1 and 2 on disk. blk0 = self.nodes[0].blocks_path / "blk00000.dat" - with open(self.nodes[0].blocks_path / "xor.dat", "rb") as xor_f: - NUM_XOR_BYTES = 8 # From InitBlocksdirXorKey::xor_key.size() - xor_dat = xor_f.read(NUM_XOR_BYTES) - - def util_xor(data, key, *, offset): - data = bytearray(data) - for i in range(len(data)): - data[i] ^= key[(i + offset) % len(key)] - return bytes(data) + xor_dat = read_xor_key(node=self.nodes[0]) with open(blk0, 'r+b') as bf: # Read at least the first few blocks (including genesis) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index de756691e0524..c3bc861cf6e7d 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -311,6 +311,13 @@ def sha256sum_file(filename): return h.digest() +def util_xor(data, key, *, offset): + data = bytearray(data) + for i in range(len(data)): + data[i] ^= key[(i + offset) % len(key)] + return bytes(data) + + # RPC/P2P connection constants and functions ############################################ @@ -508,6 +515,12 @@ def check_node_connections(*, node, num_in, num_out): assert_equal(info["connections_out"], num_out) +def read_xor_key(*, node): + with open(node.blocks_path / "xor.dat", "rb") as xor_f: + NUM_XOR_BYTES = 8 # From InitBlocksdirXorKey::xor_key.size() + return xor_f.read(NUM_XOR_BYTES) + + # Transaction/Block functions ############################# From faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Wed, 14 Aug 2024 17:21:07 +0200 Subject: [PATCH 3/7] test: add functional test for XORed block/undo files (`-blocksxor`) --- test/functional/feature_blocksxor.py | 65 ++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 66 insertions(+) create mode 100755 test/functional/feature_blocksxor.py diff --git a/test/functional/feature_blocksxor.py b/test/functional/feature_blocksxor.py new file mode 100755 index 0000000000000..88e0244cd420f --- /dev/null +++ b/test/functional/feature_blocksxor.py @@ -0,0 +1,65 @@ +#!/usr/bin/env python3 +# Copyright (c) 2024 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test support for XORed block data and undo files (`-blocksxor` option).""" +import os + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.test_node import ErrorMatch +from test_framework.util import ( + assert_equal, + assert_greater_than, + read_xor_key, + util_xor, +) +from test_framework.wallet import MiniWallet + + +class BlocksXORTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.extra_args = [[ + '-blocksxor=1', + '-fastprune=1', # use smaller block files + '-datacarriersize=100000', # needed to pad transaction with MiniWallet + ]] + + def run_test(self): + self.log.info("Mine some blocks, to create multiple blk*.dat/rev*.dat files") + node = self.nodes[0] + wallet = MiniWallet(node) + for _ in range(5): + wallet.send_self_transfer(from_node=node, target_weight=80000) + self.generate(wallet, 1) + + block_files = list(node.blocks_path.glob('blk[0-9][0-9][0-9][0-9][0-9].dat')) + undo_files = list(node.blocks_path.glob('rev[0-9][0-9][0-9][0-9][0-9].dat')) + assert_equal(len(block_files), len(undo_files)) + assert_greater_than(len(block_files), 1) # we want at least one full block file + + self.log.info("Shut down node and un-XOR block/undo files manually") + self.stop_node(0) + xor_key = read_xor_key(node=node) + for data_file in sorted(block_files + undo_files): + self.log.debug(f"Rewriting file {data_file}...") + with open(data_file, 'rb+') as f: + xored_data = f.read() + f.seek(0) + f.write(util_xor(xored_data, xor_key, offset=0)) + + self.log.info("Check that restarting with 'blocksxor=0' fails if XOR key is present") + node.assert_start_raises_init_error(['-blocksxor=0'], + 'The blocksdir XOR-key can not be disabled when a random key was already stored!', + match=ErrorMatch.PARTIAL_REGEX) + + self.log.info("Delete XOR key, restart node with '-blocksxor=0', check blk*.dat/rev*.dat file integrity") + os.remove(node.blocks_path / 'xor.dat') + self.start_node(0, extra_args=['-blocksxor=0']) + # checklevel=2 -> verify block validity + undo data + # nblocks=0 -> verify all blocks + node.verifychain(checklevel=2, nblocks=0) + + +if __name__ == '__main__': + BlocksXORTest(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 67693259d3c35..b85bf1c668f8e 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -284,6 +284,7 @@ 'mempool_package_limits.py', 'mempool_package_rbf.py', 'feature_versionbits_warning.py', + 'feature_blocksxor.py', 'rpc_preciousblock.py', 'wallet_importprunedfunds.py --legacy-wallet', 'wallet_importprunedfunds.py --descriptors', From 5d15485aafefdc759ba97e039bb1b9ccac267358 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Aug 2024 22:33:31 -0300 Subject: [PATCH 4/7] wallet: unload, notify GUI as soon as possible Releases wallet shared pointers prior to doing the final settings update and prevent GUI races trying to access a wallet that is no longer loaded. --- src/wallet/wallet.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e31950bebc8c..8abdd336a2069 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -162,10 +162,14 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet // Unregister with the validation interface which also drops shared pointers. wallet->m_chain_notifications_handler.reset(); - LOCK(context.wallets_mutex); - std::vector>::iterator i = std::find(context.wallets.begin(), context.wallets.end(), wallet); - if (i == context.wallets.end()) return false; - context.wallets.erase(i); + { + LOCK(context.wallets_mutex); + std::vector>::iterator i = std::find(context.wallets.begin(), context.wallets.end(), wallet); + if (i == context.wallets.end()) return false; + context.wallets.erase(i); + } + // Notify unload so that upper layers release the shared pointer. + wallet->NotifyUnload(); // Write the wallet setting UpdateWalletSetting(chain, name, load_on_start, warnings); @@ -249,10 +253,6 @@ void UnloadWallet(std::shared_ptr&& wallet) auto it = g_unloading_wallet_set.insert(name); assert(it.second); } - // The wallet can be in use so it's not possible to explicitly unload here. - // Notify the unload intent so that all remaining shared pointers are - // released. - wallet->NotifyUnload(); // Time to ditch our shared_ptr and wait for ReleaseWallet call. wallet.reset(); From 8872b4a6ca91a83bf8d5a118fb808c043b9e879d Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Aug 2024 23:00:45 -0300 Subject: [PATCH 5/7] wallet: rename UnloadWallet to WaitForDeleteWallet And update function's documentation. --- src/bench/wallet_create.cpp | 2 +- src/wallet/load.cpp | 2 +- src/wallet/rpc/wallet.cpp | 2 +- src/wallet/test/util.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 9 +++------ 7 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 5c0557bf6f073..1adced1e9980a 100644 --- a/src/bench/wallet_create.cpp +++ b/src/bench/wallet_create.cpp @@ -42,7 +42,7 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted) // Release wallet RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); fs::remove_all(wallet_path); }); } diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index fe35f6b223e4f..e26347d437af3 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -178,7 +178,7 @@ void UnloadWallets(WalletContext& context) wallets.pop_back(); std::vector warnings; RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } } } // namespace wallet diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index ae1c67ef2a280..83e386b3d9cbf 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -495,7 +495,7 @@ static RPCHelpMan unloadwallet() } } - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); UniValue result(UniValue::VOBJ); PushWarnings(warnings, result); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index b21a9a601d1c2..ec6c9e6f3f081 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -74,7 +74,7 @@ void TestUnloadWallet(std::shared_ptr&& wallet) // Calls SyncWithValidationInterfaceQueue wallet->chain().waitForNotificationsIfTipChanged({}); wallet->m_chain_notifications_handler.reset(); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } std::unique_ptr DuplicateMockDatabase(WalletDatabase& database) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 53f3bcc421750..44ffddb168701 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -889,7 +889,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) context.args = &m_args; auto wallet = TestLoadWallet(context); BOOST_CHECK(wallet); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8abdd336a2069..4734fc7b1d398 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -233,18 +233,18 @@ static void ReleaseWallet(CWallet* wallet) wallet->WalletLogPrintf("Releasing wallet\n"); wallet->Flush(); delete wallet; - // Wallet is now released, notify UnloadWallet, if any. + // Wallet is now released, notify WaitForDeleteWallet, if any. { LOCK(g_wallet_release_mutex); if (g_unloading_wallet_set.erase(name) == 0) { - // UnloadWallet was not called for this wallet, all done. + // WaitForDeleteWallet was not called for this wallet, all done. return; } } g_wallet_release_cv.notify_all(); } -void UnloadWallet(std::shared_ptr&& wallet) +void WaitForDeleteWallet(std::shared_ptr&& wallet) { // Mark wallet for unloading. const std::string name = wallet->GetName(); @@ -4387,7 +4387,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); was_loaded = true; } else { // Check if the wallet is BDB @@ -4531,7 +4531,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle error += _("\nUnable to cleanup failed migration"); return util::Error{error}; } - UnloadWallet(std::move(w)); + WaitForDeleteWallet(std::move(w)); } else { // Unloading for wallets in local context assert(w.use_count() == 1); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 984a2d9c482e1..3ea1cf48b22bb 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -83,12 +83,9 @@ struct bilingual_str; namespace wallet { struct WalletContext; -//! Explicitly unload and delete the wallet. -//! Blocks the current thread after signaling the unload intent so that all -//! wallet pointer owners release the wallet. -//! Note that, when blocking is not required, the wallet is implicitly unloaded -//! by the shared pointer deleter. -void UnloadWallet(std::shared_ptr&& wallet); +//! Explicitly delete the wallet. +//! Blocks the current thread until the wallet is destructed. +void WaitForDeleteWallet(std::shared_ptr&& wallet); bool AddWallet(WalletContext& context, const std::shared_ptr& wallet); bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start, std::vector& warnings); From 64e736d79efc7201768244fc297084f70c0bebc1 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 13 Aug 2024 22:52:01 -0300 Subject: [PATCH 6/7] wallet: WaitForDeleteWallet, do not expect thread safety Multiple threads could try to delete the wallet at the same time. --- src/wallet/wallet.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4734fc7b1d398..09e12d52b91de 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -250,8 +250,9 @@ void WaitForDeleteWallet(std::shared_ptr&& wallet) const std::string name = wallet->GetName(); { LOCK(g_wallet_release_mutex); - auto it = g_unloading_wallet_set.insert(name); - assert(it.second); + g_unloading_wallet_set.insert(name); + // Do not expect to be the only one removing this wallet. + // Multiple threads could simultaneously be waiting for deletion. } // Time to ditch our shared_ptr and wait for ReleaseWallet call. From f550a8e035b4603787273ea250f403f6f0be453f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 15 Aug 2024 11:08:17 -0300 Subject: [PATCH 7/7] Rename ReleaseWallet to FlushAndDeleteWallet To better describe the function's behavior. And add wallet name to logprint. --- src/wallet/wallet.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 09e12d52b91de..057fa729d7114 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -227,10 +227,10 @@ static std::set g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu static std::set g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex); // Custom deleter for shared_ptr. -static void ReleaseWallet(CWallet* wallet) +static void FlushAndDeleteWallet(CWallet* wallet) { const std::string name = wallet->GetName(); - wallet->WalletLogPrintf("Releasing wallet\n"); + wallet->WalletLogPrintf("Releasing wallet %s..\n", name); wallet->Flush(); delete wallet; // Wallet is now released, notify WaitForDeleteWallet, if any. @@ -255,7 +255,7 @@ void WaitForDeleteWallet(std::shared_ptr&& wallet) // Multiple threads could simultaneously be waiting for deletion. } - // Time to ditch our shared_ptr and wait for ReleaseWallet call. + // Time to ditch our shared_ptr and wait for FlushAndDeleteWallet call. wallet.reset(); { WAIT_LOCK(g_wallet_release_mutex, lock); @@ -2972,7 +2972,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri const auto start{SteadyClock::now()}; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), FlushAndDeleteWallet); walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1}); walletInstance->m_notify_tx_changed_script = args.GetArg("-walletnotify", "");