diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt index e138e67341..7e9668570f 100644 --- a/src/wallet/test/CMakeLists.txt +++ b/src/wallet/test/CMakeLists.txt @@ -13,5 +13,6 @@ target_sources(test_firo ${CMAKE_CURRENT_SOURCE_DIR}/spark_wallet_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/wallet_test_fixture.cpp ${CMAKE_CURRENT_SOURCE_DIR}/wallet_tests.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/wallet_tx_removal_tests.cpp ) target_link_libraries(test_firo firo_wallet) diff --git a/src/wallet/test/wallet_tx_removal_tests.cpp b/src/wallet/test/wallet_tx_removal_tests.cpp new file mode 100644 index 0000000000..438cf1c76e --- /dev/null +++ b/src/wallet/test/wallet_tx_removal_tests.cpp @@ -0,0 +1,277 @@ +// Copyright (c) 2026 The Firo developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include "key.h" +#include "random.h" +#include "script/standard.h" +#include "validation.h" +#include "wallet/test/wallet_test_fixture.h" +#include "wallet/wallet.h" +#include "wallet/walletdb.h" + +#include + +#include +#include +#include + +struct WalletTxRemovalTestingSetup : public WalletTestingSetup +{ + WalletTxRemovalTestingSetup() : WalletTestingSetup(CBaseChainParams::REGTEST) + { + ownedScript = GetScriptForDestination(pwalletMain->vchDefaultKey.GetID()); + + CKey externalKey; + externalKey.MakeNewKey(true); + externalScript = GetScriptForDestination(externalKey.GetPubKey().GetID()); + } + + uint256 AddWalletTx(const std::vector& inputs, CAmount value, bool owned, size_t outputCount = 1) + { + CMutableTransaction tx; + tx.nLockTime = nextLockTime++; + if (inputs.empty()) { + tx.vin.emplace_back(COutPoint(GetRandHash(), 0)); + } else { + for (const COutPoint& input : inputs) { + tx.vin.emplace_back(input); + } + } + for (size_t i = 0; i < outputCount; ++i) { + tx.vout.emplace_back(value, owned ? ownedScript : externalScript); + } + + CWalletTx wtx(pwalletMain, MakeTransactionRef(std::move(tx))); + wtx.SetMerkleBranch(chainActive.Tip(), 0); + const uint256 hash = wtx.GetHash(); + BOOST_REQUIRE(pwalletMain->AddToWallet(wtx)); + return hash; + } + + void RemoveWalletTx(const uint256& hash, bool zap) + { + if (!zap) { + BOOST_REQUIRE(pwalletMain->EraseFromWallet(hash)); + return; + } + + std::vector requested{hash}; + std::vector removed; + BOOST_REQUIRE_EQUAL(pwalletMain->ZapSelectTx(requested, removed), DB_LOAD_OK); + BOOST_REQUIRE_EQUAL(removed.size(), 1U); + BOOST_CHECK(removed.front() == hash); + } + + static bool IsAvailable(CWallet& wallet, const COutPoint& outpoint) + { + std::vector coins; + wallet.AvailableCoins(coins, true, nullptr, false, false); + for (const COutput& coin : coins) { + if (coin.tx->GetHash() == outpoint.hash && coin.i >= 0 && static_cast(coin.i) == outpoint.n) { + return true; + } + } + return false; + } + + bool IsAvailable(const COutPoint& outpoint) const + { + return IsAvailable(*pwalletMain, outpoint); + } + + static bool HasWalletUTXO(const CWallet& wallet, const COutPoint& outpoint) + { + return wallet.setWalletUTXO.count(outpoint); + } + + static void CheckOrderedIndex(const CWallet& wallet) + { + std::map references; + size_t transactionEntries = 0; + for (const auto& item : wallet.wtxOrdered) { + if (item.second.first) { + ++references[item.second.first]; + ++transactionEntries; + } + } + + BOOST_CHECK_EQUAL(transactionEntries, wallet.mapWallet.size()); + for (const auto& item : wallet.mapWallet) { + BOOST_CHECK_EQUAL(references[&item.second], 1U); + } + } + + void CheckRemoval(bool zap) + { + uint256 deletedParent; + uint256 survivingChild; + uint256 grandparent; + uint256 competingParent; + uint256 historyReplacement; + uint256 historyTx; + uint256 onlyParent; + uint256 secondOnlyParent; + uint256 onlySpender; + std::set deleted; + CAmount expectedBalance; + + { + LOCK2(cs_main, pwalletMain->cs_wallet); + + onlyParent = AddWalletTx({}, 11 * COIN, true); + secondOnlyParent = AddWalletTx({}, 13 * COIN, true); + onlySpender = AddWalletTx({COutPoint(onlyParent, 0), COutPoint(secondOnlyParent, 0)}, 10 * COIN, false); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(onlyParent).GetAvailableCredit(), 0); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(secondOnlyParent).GetAvailableCredit(), 0); + BOOST_CHECK(pwalletMain->HasWalletSpend(onlyParent)); + BOOST_CHECK(pwalletMain->HasWalletSpend(secondOnlyParent)); + BOOST_CHECK(!pwalletMain->setWalletUTXO.count(COutPoint(onlyParent, 0))); + BOOST_CHECK(!pwalletMain->setWalletUTXO.count(COutPoint(secondOnlyParent, 0))); + } + + RemoveWalletTx(onlySpender, zap); + deleted.insert(onlySpender); + + { + LOCK2(cs_main, pwalletMain->cs_wallet); + + BOOST_CHECK(!pwalletMain->HasWalletSpend(onlyParent)); + BOOST_CHECK(!pwalletMain->HasWalletSpend(secondOnlyParent)); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(onlyParent).GetAvailableCredit(), 11 * COIN); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(secondOnlyParent).GetAvailableCredit(), 13 * COIN); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(onlyParent, 0))); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(secondOnlyParent, 0))); + BOOST_CHECK(IsAvailable(COutPoint(onlyParent, 0))); + BOOST_CHECK(IsAvailable(COutPoint(secondOnlyParent, 0))); + + competingParent = AddWalletTx({}, 20 * COIN, true); + const uint256 firstSpender = AddWalletTx({COutPoint(competingParent, 0)}, 19 * COIN, false); + const uint256 secondSpender = AddWalletTx({COutPoint(competingParent, 0)}, 18 * COIN, false); + BOOST_CHECK(pwalletMain->GetConflicts(firstSpender) == (std::set{firstSpender, secondSpender})); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(competingParent).GetAvailableCredit(), 0); + + RemoveWalletTx(firstSpender, zap); + deleted.insert(firstSpender); + BOOST_CHECK(pwalletMain->HasWalletSpend(competingParent)); + BOOST_CHECK(pwalletMain->IsSpent(competingParent, 0)); + BOOST_CHECK(pwalletMain->GetConflicts(secondSpender).empty()); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(competingParent).GetAvailableCredit(), 0); + BOOST_CHECK(!pwalletMain->setWalletUTXO.count(COutPoint(competingParent, 0))); + + RemoveWalletTx(secondSpender, zap); + deleted.insert(secondSpender); + BOOST_CHECK(!pwalletMain->HasWalletSpend(competingParent)); + BOOST_CHECK(!pwalletMain->IsSpent(competingParent, 0)); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(competingParent).GetAvailableCredit(), 20 * COIN); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(competingParent, 0))); + BOOST_CHECK(IsAvailable(COutPoint(competingParent, 0))); + + grandparent = AddWalletTx({}, 30 * COIN, true); + deletedParent = AddWalletTx({COutPoint(grandparent, 0)}, 29 * COIN, true); + survivingChild = AddWalletTx({COutPoint(deletedParent, 0)}, 28 * COIN, false); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(grandparent).GetAvailableCredit(), 0); + + RemoveWalletTx(deletedParent, zap); + deleted.insert(deletedParent); + BOOST_CHECK(!pwalletMain->HasWalletSpend(grandparent)); + BOOST_CHECK(pwalletMain->HasWalletSpend(deletedParent)); + BOOST_CHECK(pwalletMain->mapWallet.count(survivingChild)); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(grandparent).GetAvailableCredit(), 30 * COIN); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(grandparent, 0))); + BOOST_CHECK(IsAvailable(COutPoint(grandparent, 0))); + + historyTx = AddWalletTx({}, 5 * COIN, true, 2); + const CWalletTx* const historyPtr = &pwalletMain->mapWallet.at(historyTx); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(historyTx, 0))); + BOOST_CHECK(pwalletMain->setWalletUTXO.count(COutPoint(historyTx, 1))); + RemoveWalletTx(historyTx, zap); + deleted.insert(historyTx); + BOOST_CHECK(!pwalletMain->setWalletUTXO.count(COutPoint(historyTx, 0))); + BOOST_CHECK(!pwalletMain->setWalletUTXO.count(COutPoint(historyTx, 1))); + for (const auto& item : pwalletMain->wtxOrdered) { + BOOST_CHECK(item.second.first != historyPtr); + } + + historyReplacement = AddWalletTx({}, 7 * COIN, true); + CheckOrderedIndex(*pwalletMain); + expectedBalance = pwalletMain->GetBalance(); + BOOST_CHECK_EQUAL(expectedBalance, 81 * COIN); + } + + CWallet reloaded(pwalletMain->strWalletFile); + bool firstRun = false; + BOOST_REQUIRE_EQUAL(reloaded.LoadWallet(firstRun), DB_LOAD_OK); + { + LOCK2(cs_main, reloaded.cs_wallet); + for (const uint256& hash : deleted) { + BOOST_CHECK(!reloaded.mapWallet.count(hash)); + } + BOOST_CHECK(reloaded.mapWallet.count(survivingChild)); + BOOST_CHECK(reloaded.mapWallet.count(historyReplacement)); + BOOST_CHECK(reloaded.HasWalletSpend(deletedParent)); + BOOST_CHECK(!reloaded.HasWalletSpend(grandparent)); + BOOST_CHECK(!reloaded.HasWalletSpend(competingParent)); + BOOST_CHECK(HasWalletUTXO(reloaded, COutPoint(onlyParent, 0))); + BOOST_CHECK(HasWalletUTXO(reloaded, COutPoint(secondOnlyParent, 0))); + BOOST_CHECK(HasWalletUTXO(reloaded, COutPoint(competingParent, 0))); + BOOST_CHECK(HasWalletUTXO(reloaded, COutPoint(grandparent, 0))); + BOOST_CHECK(!HasWalletUTXO(reloaded, COutPoint(deletedParent, 0))); + BOOST_CHECK(!HasWalletUTXO(reloaded, COutPoint(historyTx, 0))); + BOOST_CHECK(!HasWalletUTXO(reloaded, COutPoint(historyTx, 1))); + BOOST_CHECK(IsAvailable(reloaded, COutPoint(onlyParent, 0))); + BOOST_CHECK(IsAvailable(reloaded, COutPoint(secondOnlyParent, 0))); + BOOST_CHECK(IsAvailable(reloaded, COutPoint(competingParent, 0))); + BOOST_CHECK(IsAvailable(reloaded, COutPoint(grandparent, 0))); + BOOST_CHECK_EQUAL(reloaded.GetBalance(), expectedBalance); + CheckOrderedIndex(reloaded); + } + } + + unsigned int nextLockTime{1}; + CScript ownedScript; + CScript externalScript; +}; + +BOOST_FIXTURE_TEST_SUITE(wallet_tx_removal_tests, WalletTxRemovalTestingSetup) + +BOOST_AUTO_TEST_CASE(erase_from_wallet_cleans_indexes) +{ + CheckRemoval(false); +} + +BOOST_AUTO_TEST_CASE(zap_select_tx_cleans_indexes) +{ + CheckRemoval(true); +} + +BOOST_AUTO_TEST_CASE(zap_select_tx_db_failure_keeps_memory) +{ + LOCK2(cs_main, pwalletMain->cs_wallet); + + const uint256 hash = AddWalletTx({}, 5 * COIN, true); + const uint256 secondHash = AddWalletTx({}, 7 * COIN, true); + const COutPoint output(hash, 0); + const COutPoint secondOutput(secondHash, 0); + + CWalletDB walletdb(pwalletMain->strWalletFile, "r+"); + BOOST_REQUIRE(walletdb.TxnBegin()); + std::vector requested{hash, secondHash}; + const uint256 sentinel = GetRandHash(); + std::vector removed{sentinel}; + const DBErrors result = walletdb.ZapSelectTx(pwalletMain, requested, removed); + BOOST_REQUIRE(walletdb.TxnAbort()); + + BOOST_CHECK_EQUAL(result, DB_CORRUPT); + BOOST_REQUIRE_EQUAL(removed.size(), 1U); + BOOST_CHECK(removed.front() == sentinel); + BOOST_CHECK(pwalletMain->mapWallet.count(hash)); + BOOST_CHECK(pwalletMain->mapWallet.count(secondHash)); + BOOST_CHECK(HasWalletUTXO(*pwalletMain, output)); + BOOST_CHECK(HasWalletUTXO(*pwalletMain, secondOutput)); + BOOST_CHECK(!pwalletMain->HasWalletSpend(hash)); + BOOST_CHECK_EQUAL(pwalletMain->mapWallet.at(hash).GetAvailableCredit(), 5 * COIN); + CheckOrderedIndex(*pwalletMain); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 704ec75121..5f0f164245 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -830,6 +830,50 @@ void CWallet::AddToSpends(const uint256& wtxid) } } +void CWallet::RemoveFromWallet(const uint256& hash) +{ + AssertLockHeld(cs_main); + AssertLockHeld(cs_wallet); + + auto walletIt = mapWallet.find(hash); + if (walletIt == mapWallet.end()) + return; + + CWalletTx& wtx = walletIt->second; + + for (auto it = wtxOrdered.begin(); it != wtxOrdered.end();) { + if (it->second.first == &wtx) + it = wtxOrdered.erase(it); + else + ++it; + } + + for (auto it = mapTxSpends.begin(); it != mapTxSpends.end();) { + if (it->second == hash) + it = mapTxSpends.erase(it); + else + ++it; + } + + if (!wtx.IsCoinBase() && !wtx.tx->HasNoRegularInputs()) { + for (const CTxIn& txin : wtx.tx->vin) { + auto parentIt = mapWallet.find(txin.prevout.hash); + if (parentIt == mapWallet.end() || txin.prevout.n >= parentIt->second.tx->vout.size()) + continue; + + if (IsMine(parentIt->second.tx->vout[txin.prevout.n], *parentIt->second.tx) && + !IsSpent(txin.prevout.hash, txin.prevout.n)) { + setWalletUTXO.insert(txin.prevout); + } + } + } + + for (size_t i = 0; i < wtx.tx->vout.size(); ++i) + setWalletUTXO.erase(COutPoint(hash, i)); + + mapWallet.erase(walletIt); +} + bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) { if (IsCrypted()) @@ -3899,11 +3943,15 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CCon bool CWallet::EraseFromWallet(uint256 hash) { if (!fFileBacked) return false; - { - LOCK(cs_wallet); - if (mapWallet.erase(hash)) - CWalletDB(strWalletFile).EraseTx(hash); - } + + LOCK2(cs_main, cs_wallet); + if (!mapWallet.count(hash)) + return true; + if (!CWalletDB(strWalletFile).EraseTx(hash)) + return false; + + RemoveFromWallet(hash); + MarkDirty(); return true; } @@ -4213,7 +4261,20 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector deleted; + DBErrors nZapSelectTxRet; + { + LOCK2(cs_main, cs_wallet); + nZapSelectTxRet = CWalletDB(strWalletFile,"cr+").ZapSelectTx(this, vHashIn, deleted); + if (nZapSelectTxRet == DB_LOAD_OK) { + for (const uint256& hash : deleted) + RemoveFromWallet(hash); + if (!deleted.empty()) + MarkDirty(); + } + } + if (nZapSelectTxRet == DB_NEED_REWRITE) { if (CDB::Rewrite(strWalletFile, "\x04pool")) @@ -4230,7 +4291,7 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector UnlockWallet; extern boost::signals2::signal UnlockWallet; +struct WalletTxRemovalTestingSetup; + /** * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances, * and provides the ability to create new transactions. @@ -656,6 +658,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface { private: friend class CSparkWallet; + friend struct WalletTxRemovalTestingSetup; static std::atomic fFlushThreadRunning; @@ -692,6 +695,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface TxSpends mapTxSpends; void AddToSpends(const COutPoint& outpoint, const uint256& wtxid); void AddToSpends(const uint256& wtxid); + void RemoveFromWallet(const uint256& hash); std::set setWalletUTXO; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 39fa0b9cf2..1844f01f9a 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -883,8 +883,8 @@ DBErrors CWalletDB::ZapSelectTx(CWallet* pwallet, std::vector& vTxHashI std::sort(vTxHash.begin(), vTxHash.end()); std::sort(vTxHashIn.begin(), vTxHashIn.end()); - // erase each matching wallet TX - bool delerror = false; + // Find each matching wallet TX before changing the database. + std::vector matches; std::vector::iterator it = vTxHashIn.begin(); BOOST_FOREACH (uint256 hash, vTxHash) { while (it < vTxHashIn.end() && (*it) < hash) { @@ -894,18 +894,32 @@ DBErrors CWalletDB::ZapSelectTx(CWallet* pwallet, std::vector& vTxHashI break; } else if ((*it) == hash) { - pwallet->mapWallet.erase(hash); - if(!EraseTx(hash)) { - LogPrint("db", "Transaction was found for deletion but returned database error: %s\n", hash.GetHex()); - delerror = true; - } - vTxHashOut.push_back(hash); + matches.push_back(hash); + } + } + + if (matches.empty()) + return DB_LOAD_OK; + + if (!TxnBegin()) { + LogPrint("db", "Failed to begin transaction while deleting wallet transactions\n"); + return DB_CORRUPT; + } + + for (const uint256& hash : matches) { + if (!EraseTx(hash)) { + LogPrint("db", "Transaction was found for deletion but returned database error: %s\n", hash.GetHex()); + TxnAbort(); + return DB_CORRUPT; } } - if (delerror) { + if (!TxnCommit()) { + LogPrint("db", "Failed to commit deletion of wallet transactions\n"); return DB_CORRUPT; } + + vTxHashOut.insert(vTxHashOut.end(), matches.begin(), matches.end()); return DB_LOAD_OK; }