wallet: simplify and batch zap wallet txes process

furszy commented at 2:03 pm on December 2, 2023: member

Work decoupled from #28574. Brother of #28894.

Includes two different, yet interconnected, performance and code improvements to the zap wallet transactions process.

As the goal of the ZapSelectTx function is to erase tx records that match any of the inputted hashes. There is no need to traverse the whole database record by record. We could just check if the tx exist, and remove it directly by calling EraseTx().
Instead of performing single write operations per removed tx record, this PR batches them all within a single atomic db txn.

Moreover, these changes will enable us to consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.

DrahtBot commented at 2:03 pm on December 2, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, josibake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29403 (wallet: batch erase procedures and improve ‘EraseRecords’ performance by furszy)
#22693 (RPC/Wallet: Add “use_txids” to output of getaddressinfo by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Dec 2, 2023

in src/wallet/wallet.cpp:2314 in 559529480f outdated

2310@@ -2311,18 +2311,24 @@ DBErrors CWallet::LoadWallet()
2311     return nLoadWalletRet;
2312 }
2313 
2314-DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn)
2315+void CWallet::ClearTxns(const std::vector<uint256>& tx_hashes)

achow101 commented at 5:34 pm on January 5, 2024:

In 559529480fedd4181dab76f4452b5998d0cdadc3 “refactor: decouple wallet EraseTxns from ZapSelectTx”

The commit message calls this EraseTxns.

furszy commented at 3:26 pm on January 6, 2024:

fixed

in src/wallet/wallet.cpp:2319 in 559529480f outdated

2319-    for (const uint256& hash : vHashIn) {
2320+    for (const uint256& hash : tx_hashes) {
2321         const auto& it = mapWallet.find(hash);
2322         wtxOrdered.erase(it->second.m_it_wtxOrdered);
2323-        for (const auto& txin : it->second.tx->vin)
2324+        for (const auto& txin: it->second.tx->vin)

achow101 commented at 5:35 pm on January 5, 2024:

In 559529480fedd4181dab76f4452b5998d0cdadc3 “refactor: decouple wallet EraseTxns from ZapSelectTx”

Unnecessary whitespace change.

furszy commented at 3:26 pm on January 6, 2024:

fixed

in src/wallet/wallet.cpp:3934 in ff214259de outdated

3932@@ -3931,10 +3933,26 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
3933     watchonly_batch.reset(); // Flush
3934     // Do the removes
3935     if (txids_to_delete.size() > 0) {
3936-        if (ZapSelectTx(txids_to_delete) != DBErrors::LOAD_OK) {

achow101 commented at 5:38 pm on January 5, 2024:

In ff214259de28216a5190c358b786306985f30227 “wallet: migration, remove watch-only transactions atomically”

It seems unnecessary to be making this change here as ZapSelectTx is now atomic.

furszy commented at 3:28 pm on January 6, 2024:

In ff21425 “wallet: migration, remove watch-only transactions atomically”

It seems unnecessary to be making this change here as ZapSelectTx is now atomic.

True. This PR does not requires it. Will re-introduced it within #28574.

wallet: ZapSelectTx, remove db rewrite code

The function does not return DBErrors::NEED_REWRITE.

a2b071f992

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.

595d50a103

furszy force-pushed on Jan 6, 2024

furszy commented at 3:56 pm on January 6, 2024: member

Updated per feedback. Thanks achow. Diff.

DrahtBot added the label CI failed on Jan 6, 2024

furszy commented at 10:35 pm on January 8, 2024: member

CI failure is unrelated. Relates to #29112.

in src/wallet/walletdb.cpp:1243 in 783e7de01c outdated

1311-                LogPrint(BCLog::WALLETDB, "Transaction was found for deletion but returned database error: %s\n", hash.GetHex());
1312-                delerror = true;
1313-            }
1314-            vTxHashOut.push_back(hash);
1315+        if (!EraseTx(hash)) {
1316+            LogPrint(BCLog::WALLETDB, "%s: Failure removing transaction: %s\n", __func__, hash.GetHex());

maflcko commented at 4:33 pm on January 31, 2024:

nit: Any reason to add __func__ to this log, when it previously wasn’t there? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.

furszy commented at 5:23 pm on February 9, 2024:

Any reason to add func to this log, when it previously wasn’t there?

just an old bad habit. Removed. Thanks.

in src/wallet/wallet.cpp:2334 in d26a28508d outdated

2334-        return nZapSelectTxRet;
2335+    WalletBatch batch(GetDatabase());
2336+    if (!batch.TxnBegin()) return DBErrors::NONCRITICAL_ERROR;
2337+    DBErrors db_status = batch.ZapSelectTx(vHashIn);
2338+    // All deletions should have been added to the db txn, otherwise we abort the process.
2339+    if (db_status != DBErrors::LOAD_OK) return db_status;

achow101 commented at 8:23 pm on February 7, 2024:

In d26a28508d5322fe7c29b3642dcf3a17b0ea31f1 “wallet: batch ZapSelectTx db operations”

This should explicitly call TxnAbort before returning as otherwise we will get extraenous logs about the tx being aborted before batch is destroyed.

furszy commented at 5:23 pm on February 9, 2024:

This should explicitly call TxnAbort before returning as otherwise we will get extraenous logs about the tx being aborted before batch is destroyed.

sure. Fixed.

in src/wallet/wallet.cpp:2337 in d26a28508d outdated

2337+    DBErrors db_status = batch.ZapSelectTx(vHashIn);
2338+    // All deletions should have been added to the db txn, otherwise we abort the process.
2339+    if (db_status != DBErrors::LOAD_OK) return db_status;
2340 
2341+    // Apply db changes and remove transactions from the memory map
2342+    if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR;

achow101 commented at 8:24 pm on February 7, 2024:

In d26a28508d5322fe7c29b3642dcf3a17b0ea31f1 “wallet: batch ZapSelectTx db operations”

I’m not sure if NONCRITICAL_ERROR is really the right return code. It seems like something has gone critically wrong if TxnCommit were to fail.

furszy commented at 5:37 pm on February 9, 2024:

I’m not sure if NONCRITICAL_ERROR is really the right return code. It seems like something has gone critically wrong if TxnCommit were to fail.

The returned error code doesn’t really matter. Anything different from LOAD_OK is an error for all the function’s callers. And they behave exactly the same for all codes. Actually, the DBErrors return doesn’t make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff like the version inside anymore.

Based on this, I have reworked the PR cleaning up this aspect as well.

furszy force-pushed on Feb 9, 2024

furszy commented at 5:38 pm on February 9, 2024: member

Updated per feedback. Thanks Marko and achow.

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.

83b762845f

scripted-diff: rename ZapSelectTx to RemoveTxs

-BEGIN VERIFY SCRIPT-
sed -i 's/ZapSelectTx/RemoveTxs/g' $(git grep -l 'ZapSelectTx' ./src/wallet)
-END VERIFY SCRIPT-

9a3c5c8697

furszy force-pushed on Feb 9, 2024

achow101 commented at 6:40 pm on February 9, 2024: member

ACK 9a3c5c8697659e34d0476103af942a4615818f4e

The new code is much simpler.

DrahtBot removed the label CI failed on Feb 9, 2024

josibake commented at 2:00 pm on February 12, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/28987/commits/9a3c5c8697659e34d0476103af942a4615818f4e

looks good, also RemoveTxs is a much better name. Might be worth updating the description to mention the rename since the description ends up in the commit log.

achow101 merged this on Feb 12, 2024

achow101 closed this on Feb 12, 2024

furszy deleted the branch on Feb 12, 2024

wallet: simplify and batch zap wallet txes process #28987

Code Coverage

Reviews

Conflicts