From 83b762845f5804f23b63526d403b2f327fe99632 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 16 Jul 2023 13:31:38 -0300 Subject: [PATCH] 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 3e1ec667e0a..401c0b2d239 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 dd43705a840..cbab4f2ee98 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 1c879fb976d..3f2b26ef999 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 bc45010200d..5eddaac6d82 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 96999de97b8..6f99da7aead 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 dad0b18a78e..62449eb64ec 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 5fe7c4b5911..b3ae22cc445 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)