From a2b071f9920c2f4893afcc43a152f593c03966bf Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 16 Jul 2023 13:41:07 -0300 Subject: [PATCH 1/4] wallet: ZapSelectTx, remove db rewrite code The function does not return DBErrors::NEED_REWRITE. --- src/wallet/wallet.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c34bf96b5e..032b1fed20 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2324,16 +2324,6 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vectorRewriteDB(); - } - } - } - if (nZapSelectTxRet != DBErrors::LOAD_OK) return nZapSelectTxRet; From 595d50a1032ad7ffa9945464c86aa57f16665e93 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 16 Jul 2023 20:16:03 -0300 Subject: [PATCH 2/4] wallet: migration, remove extra NotifyTransactionChanged call The wallet is unloaded at the beginning of the migration process, so no object is listening to the signals. --- src/wallet/wallet.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 032b1fed20..1c879fb976 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3934,10 +3934,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) error = _("Error: Not all watchonly txs could be deleted"); return false; } - // Tell the GUI of each tx - for (const uint256& txid : deleted_txids) { - NotifyTransactionChanged(txid, CT_UPDATED); - } } // Check the address book data in the same way we did for transactions From 83b762845f5804f23b63526d403b2f327fe99632 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 16 Jul 2023 13:31:38 -0300 Subject: [PATCH 3/4] wallet: batch and simplify ZapSelectTx process The goal of the function is to erase the wallet transactions that match the inputted hashes. There is no need to traverse the database, reading record by record, to then perform single entry removals for each of them. To ensure consistency and improve performance, this change-set removes all tx records within a single atomic db batch operation, as well as it cleans up code, improves error handling and simplifies the transactions removal process entirely. This optimizes the removal of watch-only transactions during the wallet migration process and the 'removeprunedfunds' RPC command. --- src/wallet/rpc/backup.cpp | 10 +-- src/wallet/test/wallet_tests.cpp | 4 +- src/wallet/wallet.cpp | 51 +++++++++---- src/wallet/wallet.h | 4 +- src/wallet/walletdb.cpp | 84 --------------------- src/wallet/walletdb.h | 2 - test/functional/wallet_importprunedfunds.py | 2 +- 7 files changed, 44 insertions(+), 113 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 3e1ec667e0..401c0b2d23 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -394,14 +394,8 @@ RPCHelpMan removeprunedfunds() uint256 hash(ParseHashV(request.params[0], "txid")); std::vector vHash; vHash.push_back(hash); - std::vector vHashOut; - - if (pwallet->ZapSelectTx(vHash, vHashOut) != DBErrors::LOAD_OK) { - throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction."); - } - - if(vHashOut.empty()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction does not exist in wallet."); + if (auto res = pwallet->ZapSelectTx(vHash); !res) { + throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); } return UniValue::VNULL; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dd43705a84..cbab4f2ee9 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -918,8 +918,8 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); - std::vector vHashIn{ block_hash }, vHashOut; - BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); + std::vector vHashIn{ block_hash }; + BOOST_CHECK(wallet->ZapSelectTx(vHashIn)); 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 1c879fb976..3f2b26ef99 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2311,12 +2311,41 @@ DBErrors CWallet::LoadWallet() return nLoadWalletRet; } -DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) +util::Result CWallet::ZapSelectTx(std::vector& txs_to_remove) { AssertLockHeld(cs_wallet); - DBErrors nZapSelectTxRet = WalletBatch(GetDatabase()).ZapSelectTx(vHashIn, vHashOut); - for (const uint256& hash : vHashOut) { - const auto& it = mapWallet.find(hash); + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) return util::Error{_("Error starting db txn for wallet transactions removal")}; + + // Check for transaction existence and remove entries from disk + using TxIterator = std::unordered_map::const_iterator; + std::vector erased_txs; + bilingual_str str_err; + for (const uint256& hash : txs_to_remove) { + auto it_wtx = mapWallet.find(hash); + if (it_wtx == mapWallet.end()) { + str_err = strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex()); + break; + } + if (!batch.EraseTx(hash)) { + str_err = strprintf(_("Failure removing transaction: %s"), hash.GetHex()); + break; + } + erased_txs.emplace_back(it_wtx); + } + + // Roll back removals in case of an error + if (!str_err.empty()) { + batch.TxnAbort(); + return util::Error{str_err}; + } + + // Dump changes to disk + if (!batch.TxnCommit()) return util::Error{_("Error committing db txn for wallet transactions removal")}; + + // Update the in-memory state and notify upper layers about the removals + for (const auto& it : erased_txs) { + const uint256 hash{it->first}; wtxOrdered.erase(it->second.m_it_wtxOrdered); for (const auto& txin : it->second.tx->vin) mapTxSpends.erase(txin.prevout); @@ -2324,12 +2353,9 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector& new_purpose) @@ -3925,13 +3951,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { - std::vector deleted_txids; - if (ZapSelectTx(txids_to_delete, deleted_txids) != DBErrors::LOAD_OK) { - error = _("Error: Could not delete watchonly transactions"); - return false; - } - if (deleted_txids != txids_to_delete) { - error = _("Error: Not all watchonly txs could be deleted"); + if (auto res = ZapSelectTx(txids_to_delete); !res) { + error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bc45010200..5eddaac6d8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -787,7 +787,9 @@ public: void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; DBErrors LoadWallet(); - DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Erases the provided transactions from the wallet. */ + util::Result ZapSelectTx(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 96999de97b..6f99da7aea 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1230,90 +1230,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTxHashes(std::vector& tx_hashes) -{ - DBErrors result = DBErrors::LOAD_OK; - - try { - int nMinVersion = 0; - if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) { - if (nMinVersion > FEATURE_LATEST) - return DBErrors::TOO_NEW; - } - - // Get cursor - std::unique_ptr cursor = m_batch->GetNewCursor(); - if (!cursor) - { - LogPrintf("Error getting wallet database cursor\n"); - return DBErrors::CORRUPT; - } - - while (true) - { - // Read next record - DataStream ssKey{}; - DataStream ssValue{}; - DatabaseCursor::Status status = cursor->Next(ssKey, ssValue); - if (status == DatabaseCursor::Status::DONE) { - break; - } else if (status == DatabaseCursor::Status::FAIL) { - LogPrintf("Error reading next record from wallet database\n"); - return DBErrors::CORRUPT; - } - - std::string strType; - ssKey >> strType; - if (strType == DBKeys::TX) { - uint256 hash; - ssKey >> hash; - tx_hashes.push_back(hash); - } - } - } catch (...) { - result = DBErrors::CORRUPT; - } - - return result; -} - -DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector& vTxHashOut) -{ - // build list of wallet TX hashes - std::vector vTxHash; - DBErrors err = FindWalletTxHashes(vTxHash); - if (err != DBErrors::LOAD_OK) { - return err; - } - - std::sort(vTxHash.begin(), vTxHash.end()); - std::sort(vTxHashIn.begin(), vTxHashIn.end()); - - // erase each matching wallet TX - bool delerror = false; - std::vector::iterator it = vTxHashIn.begin(); - for (const uint256& hash : vTxHash) { - while (it < vTxHashIn.end() && (*it) < hash) { - it++; - } - if (it == vTxHashIn.end()) { - break; - } - else if ((*it) == hash) { - if(!EraseTx(hash)) { - LogPrint(BCLog::WALLETDB, "Transaction was found for deletion but returned database error: %s\n", hash.GetHex()); - delerror = true; - } - vTxHashOut.push_back(hash); - } - } - - if (delerror) { - return DBErrors::CORRUPT; - } - return DBErrors::LOAD_OK; -} - void MaybeCompactWalletDB(WalletContext& context) { static std::atomic fOneThread(false); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index dad0b18a78..62449eb64e 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -275,8 +275,6 @@ public: bool EraseActiveScriptPubKeyMan(uint8_t type, bool internal); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTxHashes(std::vector& tx_hashes); - DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut); //! write the hdchain model (external chain child index counter) bool WriteHDChain(const CHDChain& chain); diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py index 5fe7c4b591..b3ae22cc44 100755 --- a/test/functional/wallet_importprunedfunds.py +++ b/test/functional/wallet_importprunedfunds.py @@ -120,7 +120,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework): assert_equal(address_info['ismine'], True) # Remove transactions - assert_raises_rpc_error(-8, "Transaction does not exist in wallet.", w1.removeprunedfunds, txnid1) + assert_raises_rpc_error(-4, f'Transaction {txnid1} does not belong to this wallet', w1.removeprunedfunds, txnid1) assert not [tx for tx in w1.listtransactions(include_watchonly=True) if tx['txid'] == txnid1] wwatch.removeprunedfunds(txnid2) From 9a3c5c8697659e34d0476103af942a4615818f4e Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 9 Feb 2024 11:45:05 -0300 Subject: [PATCH 4/4] scripted-diff: rename ZapSelectTx to RemoveTxs -BEGIN VERIFY SCRIPT- sed -i 's/ZapSelectTx/RemoveTxs/g' $(git grep -l 'ZapSelectTx' ./src/wallet) -END VERIFY SCRIPT- --- src/wallet/rpc/backup.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 401c0b2d23..c5340b1206 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -394,7 +394,7 @@ RPCHelpMan removeprunedfunds() uint256 hash(ParseHashV(request.params[0], "txid")); std::vector vHash; vHash.push_back(hash); - if (auto res = pwallet->ZapSelectTx(vHash); !res) { + if (auto res = pwallet->RemoveTxs(vHash); !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 cbab4f2ee9..90195cabad 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -892,7 +892,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) UnloadWallet(std::move(wallet)); } -BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) +BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) { m_args.ForceSetArg("-unsafesqlitesync", "1"); WalletContext context; @@ -919,7 +919,7 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); std::vector vHashIn{ block_hash }; - BOOST_CHECK(wallet->ZapSelectTx(vHashIn)); + BOOST_CHECK(wallet->RemoveTxs(vHashIn)); 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 3f2b26ef99..4e0fea156c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2311,7 +2311,7 @@ DBErrors CWallet::LoadWallet() return nLoadWalletRet; } -util::Result CWallet::ZapSelectTx(std::vector& txs_to_remove) +util::Result CWallet::RemoveTxs(std::vector& txs_to_remove) { AssertLockHeld(cs_wallet); WalletBatch batch(GetDatabase()); @@ -3951,7 +3951,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { - if (auto res = ZapSelectTx(txids_to_delete); !res) { + if (auto res = RemoveTxs(txids_to_delete); !res) { error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5eddaac6d8..397e526514 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -789,7 +789,7 @@ public: DBErrors LoadWallet(); /** Erases the provided transactions from the wallet. */ - util::Result ZapSelectTx(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + util::Result RemoveTxs(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose);