Merge remote-tracking branch 'upstream/master'

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);
     });
 }

src/wallet/load.cpp

@@ -178,7 +178,7 @@ void UnloadWallets(WalletContext& context)
         wallets.pop_back();
         std::vector<bilingual_str> warnings;
         RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings);
-        UnloadWallet(std::move(wallet));
+        WaitForDeleteWallet(std::move(wallet));
     }
 }
 } // namespace wallet

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);

src/wallet/test/util.cpp

@@ -74,7 +74,7 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
     // Calls SyncWithValidationInterfaceQueue
     wallet->chain().waitForNotificationsIfTipChanged({});
     wallet->m_chain_notifications_handler.reset();
-    UnloadWallet(std::move(wallet));
+    WaitForDeleteWallet(std::move(wallet));
 }
 
 std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database)

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)

src/wallet/wallet.cpp

@@ -162,10 +162,14 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
 
     // Unregister with the validation interface which also drops shared pointers.
     wallet->m_chain_notifications_handler.reset();
-    LOCK(context.wallets_mutex);
-    std::vector<std::shared_ptr<CWallet>>::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<std::shared_ptr<CWallet>>::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);
@@ -223,38 +227,35 @@ static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu
 static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
 
 // Custom deleter for shared_ptr<CWallet>.
-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 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<CWallet>&& wallet)
+void WaitForDeleteWallet(std::shared_ptr<CWallet>&& wallet)
 {
     // Mark wallet for unloading.
     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.
     }
-    // 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.
+    // Time to ditch our shared_ptr and wait for FlushAndDeleteWallet call.
     wallet.reset();
     {
         WAIT_LOCK(g_wallet_release_mutex, lock);
@@ -1037,22 +1038,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 +1626,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;
 }
@@ -2971,7 +2974,7 @@ std::shared_ptr<CWallet> 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<CWallet> walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet);
+    std::shared_ptr<CWallet> 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", "");
 
@@ -3558,7 +3561,8 @@ std::set<ScriptPubKeyMan*> 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 +3592,8 @@ std::unique_ptr<SigningProvider> 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 +3850,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<LegacyScriptPubKeyMan*>(m_internal_spk_managers.at(OutputType::LEGACY));
-    return spk_man != nullptr;
+    return !IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS);
 }
 
 DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const
@@ -4387,7 +4388,7 @@ util::Result<MigrationResult> 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 +4532,7 @@ util::Result<MigrationResult> 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);

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<CWallet>&& wallet);
+//! Explicitly delete the wallet.
+//! Blocks the current thread until the wallet is destructed.
+void WaitForDeleteWallet(std::shared_ptr<CWallet>&& wallet);
 
 bool AddWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
 bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet, std::optional<bool> load_on_start, std::vector<bilingual_str>& warnings);

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()

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)

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
 #############################
 

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',