Repository: bitcoin/bitcoin
File: src/wallet/wallet.cpp (C++)

Code before:
DBErrors CWallet::LoadWallet()
{
    LOCK(cs_wallet);

    Assert(m_spk_managers.empty());
    Assert(m_wallet_flags == 0);
    DBErrors nLoadWalletRet = WalletBatch(GetDatabase()).LoadWallet(this);
    if (nLoadWalletRet == DBErrors::NEED_REWRITE)
    {
        if (GetDatabase().Rewrite("\x04pool"))
        {
            for (const auto& spk_man_pair : m_spk_managers) {
                spk_man_pair.second->RewriteDB();
            }
        }
    }

    if (m_spk_managers.empty()) {
        assert(m_external_spk_managers.empty());
        assert(m_internal_spk_managers.empty());
    }

    return nLoadWalletRet;
}

util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove)
{
    AssertLockHeld(cs_wallet);
    bilingual_str str_err;  // future: make RunWithinTxn return a util::Result
    bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
        util::Result<void> result{RemoveTxs(batch, txs_to_remove)};
        if (!result) str_err = util::ErrorString(result);
        return result.has_value();
    });
    if (!str_err.empty()) return util::Error{str_err};
    if (!was_txn_committed) return util::Error{_("Error starting/committing db txn for wallet transactions removal process")};
    return {}; // all good
}

util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<uint256>& txs_to_remove)
{
    AssertLockHeld(cs_wallet);
    if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};

    // Check for transaction existence and remove entries from disk
    using TxIterator = std::unordered_map<uint256, CWalletTx, SaltedTxidHasher>::const_iterator;
    std::vector<TxIterator> erased_txs;
    bilingual_str str_err;
    for (const uint256& hash : txs_to_remove) {
        auto it_wtx = mapWallet.find(hash);
        if (it_wtx == mapWallet.end()) {

Review comment (performance):
nit: could be a good time to update this to `std::span`?

<details>
<summary>git diff on afbb1a1bfd</summary>

```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 89176c01f5..6b246f9cbb 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -395,9 +395,7 @@ RPCHelpMan removeprunedfunds()
     LOCK(pwallet->cs_wallet);
 
     Txid hash = Txid::FromUint256((ParseHashV(request.params[0], "txid")));
-    std::vector<Txid> vHash;
-    vHash.push_back(hash);
-    if (auto res = pwallet->RemoveTxs(vHash); !res) {
+    if (auto res = pwallet->RemoveTxs(std::span{&hash, 1}); !res) {
         throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original);
     }
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 2b8d3149e6..0b9c1da0be 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -951,8 +951,7 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup)
         BOOST_CHECK(wallet->HasWalletSpend(prev_tx));
         BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u);
 
-        std::vector<Txid> vHashIn{ block_hash };
-        BOOST_CHECK(wallet->RemoveTxs(vHashIn));
+        BOOST_CHECK(wallet->RemoveTxs(std::span{&block_hash, 1}));
 
         BOOST_CHECK(!wallet->HasWalletSpend(prev_tx));
         BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c401909003..fff100ddbe 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -75,6 +75,7 @@
 #include <condition_variable>
 #include <exception>
 #include <optional>
+#include <span>
 #include <stdexcept>
 #include <thread>
 #include <tuple>
@@ -2405,7 +2406,7 @@ DBErrors CWallet::LoadWallet()
     return nLoadWalletRet;
 }
 
-util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)
+util::Result<void> CWallet::RemoveTxs(std::span<Txid> txs_to_remove)
 {
     AssertLockHeld(cs_wallet);
     bilingual_str str_err;  // future: make RunWithinTxn return a util::Result
@@ -2419,7 +2420,7 @@ util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)
     return {}; // all good
 }
 
-util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs_to_remove)
+util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::span<Txid> txs_to_remove)
 {
     AssertLockHeld(cs_wallet);
     if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 17ab3ec7c4..f5dabea51d 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -44,6 +44,7 @@
 #include <memory>
 #include <optional>
 #include <set>
+#include <span>
 #include <string>
 #include <unordered_map>
 #include <utility>
@@ -793,8 +794,8 @@ public:
     DBErrors LoadWallet();
 
     /** Erases the provided transactions from the wallet. */
-    util::Result<void> RemoveTxs(std::vector<Txid>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    util::Result<void> RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<void> RemoveTxs(std::span<Txid> txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<void> RemoveTxs(WalletBatch& batch, std::span<Txid> txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose);
 

```
</details>


Code after:
DBErrors CWallet::LoadWallet()
{
    LOCK(cs_wallet);

    Assert(m_spk_managers.empty());
    Assert(m_wallet_flags == 0);
    DBErrors nLoadWalletRet = WalletBatch(GetDatabase()).LoadWallet(this);
    if (nLoadWalletRet == DBErrors::NEED_REWRITE)
    {
        if (GetDatabase().Rewrite("\x04pool"))
        {
            for (const auto& spk_man_pair : m_spk_managers) {
                spk_man_pair.second->RewriteDB();
            }
        }
    }

    if (m_spk_managers.empty()) {
        assert(m_external_spk_managers.empty());
        assert(m_internal_spk_managers.empty());
    }

    return nLoadWalletRet;
}

util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)
{
    AssertLockHeld(cs_wallet);
    bilingual_str str_err;  // future: make RunWithinTxn return a util::Result
    bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
        util::Result<void> result{RemoveTxs(batch, txs_to_remove)};
        if (!result) str_err = util::ErrorString(result);
        return result.has_value();
    });
    if (!str_err.empty()) return util::Error{str_err};
    if (!was_txn_committed) return util::Error{_("Error starting/committing db txn for wallet transactions removal process")};
    return {}; // all good
}

util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs_to_remove)
{
    AssertLockHeld(cs_wallet);
    if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};

    // Check for transaction existence and remove entries from disk
    std::vector<decltype(mapWallet)::const_iterator> erased_txs;
    bilingual_str str_err;
    for (const Txid& hash : txs_to_remove) {
        auto it_wtx = mapWallet.find(hash);
        if (it_wtx == mapWallet.end()) {
            return util::Error{strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex())};