diff options
| -rw-r--r-- | src/wallet/rpc/backup.cpp | 10 | ||||
| -rw-r--r-- | src/wallet/test/wallet_tests.cpp | 6 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 65 | ||||
| -rw-r--r-- | src/wallet/wallet.h | 4 | ||||
| -rw-r--r-- | src/wallet/walletdb.cpp | 84 | ||||
| -rw-r--r-- | src/wallet/walletdb.h | 2 | ||||
| -rwxr-xr-x | test/functional/wallet_importprunedfunds.py | 2 |
7 files changed, 45 insertions, 128 deletions
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 99c548bf1d..873ebcadfd 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<uint256> vHash; vHash.push_back(hash); - std::vector<uint256> 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->RemoveTxs(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 65297054df..7396a43c50 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -888,7 +888,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; @@ -913,8 +913,8 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); - std::vector<uint256> vHashIn{ block_hash }, vHashOut; - BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); + std::vector<uint256> vHashIn{ block_hash }; + 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 fdf610955b..11203ca3a4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2320,12 +2320,41 @@ DBErrors CWallet::LoadWallet() return nLoadWalletRet; } -DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) +util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& 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<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()) { + 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); @@ -2333,22 +2362,9 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256 NotifyTransactionChanged(hash, CT_DELETED); } - if (nZapSelectTxRet == DBErrors::NEED_REWRITE) - { - if (GetDatabase().Rewrite("\x04pool")) - { - for (const auto& spk_man_pair : m_spk_managers) { - spk_man_pair.second->RewriteDB(); - } - } - } - - if (nZapSelectTxRet != DBErrors::LOAD_OK) - return nZapSelectTxRet; - MarkDirty(); - return DBErrors::LOAD_OK; + return {}; // all good } bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& new_purpose) @@ -4025,19 +4041,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { - std::vector<uint256> deleted_txids; - if (ZapSelectTx(txids_to_delete, deleted_txids) != DBErrors::LOAD_OK) { - error = _("Error: Could not delete watchonly transactions"); + if (auto res = RemoveTxs(txids_to_delete); !res) { + error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } - if (deleted_txids != txids_to_delete) { - 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); - } } // Pair external wallets with their corresponding db handler diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c0d10ab7fc..42ceda1071 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -790,7 +790,9 @@ public: void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; DBErrors LoadWallet(); - DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Erases the provided transactions from the wallet. */ + util::Result<void> RemoveTxs(std::vector<uint256>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f3dd5b328e..6e37bc2930 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<uint256>& 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<DatabaseCursor> 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<uint256>& vTxHashIn, std::vector<uint256>& vTxHashOut) -{ - // build list of wallet TX hashes - std::vector<uint256> 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<uint256>::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<bool> 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<uint256>& tx_hashes); - DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& 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) |