wallet: simplify and batch zap wallet txes process #28987

furszy · 2023-12-02T14:03:31Z

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 · 2023-12-02T14:03:34Z

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.

src/wallet/wallet.cpp

The function does not return DBErrors::NEED_REWRITE.

The wallet is unloaded at the beginning of the migration process, so no object is listening to the signals.

furszy

Updated per feedback. Thanks achow. Diff.

furszy

CI failure is unrelated. Relates to #29112.

src/wallet/walletdb.cpp

src/wallet/wallet.cpp

achow101 · 2024-02-07T20:24:26Z

src/wallet/wallet.cpp


+    // Apply db changes and remove transactions from the memory map
+    if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR;


In d26a285 "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.

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

Updated per feedback. Thanks Marko and achow.

src/wallet/walletdb.cpp

src/wallet/wallet.cpp

furszy · 2024-02-09T17:37:47Z

src/wallet/wallet.cpp


+    // Apply db changes and remove transactions from the memory map
+    if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR;


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.

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.

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

achow101 · 2024-02-09T18:40:07Z

ACK 9a3c5c8

The new code is much simpler.

josibake · 2024-02-12T14:00:44Z

ACK 9a3c5c8

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.

hebasto · 2024-02-13T11:10:11Z

src/wallet/wallet.cpp

-        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);


nit: The trailing space could be easily overlooked and dropped during translation.

nit: The trailing space could be easily overlooked and dropped during translation.

@hebasto, what about creating a constant to represent the whitespace, or alternatively, implementing a function that concatenates strings with the whitespace inserted between them?

c98fc36 wallet: migration, consolidate external wallets db writes (furszy) 7c9076a wallet: migration, consolidate main wallet db writes (furszy) 9ef20e8 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy) 34bf079 wallet: refactor ApplyMigrationData to return util::Result<void> (furszy) aacaaaa wallet: provide WalletBatch to 'RemoveTxs' (furszy) 57249ff wallet: introduce active db txn listeners (furszy) 91e065e wallet: remove post-migration signals connection (furszy) 055c053 wallet: provide WalletBatch to 'DeleteRecords' (furszy) 122d103 wallet: introduce 'SetWalletFlagWithDB' (furszy) 6052c78 wallet: decouple default descriptors creation from external signer setup (furszy) f2541d0 wallet: batch MigrateToDescriptor() db transactions (furszy) 66c9936 bench: add coverage for wallet migration process (furszy) Pull request description: Last step in a chain of PRs (#26836, #28894, #28987, #29403). #### Detailed Description: The current wallet migration process performs only individual db writes. Accessing disk to delete all legacy records, clone and clean each address book entry for every created wallet, create each new descriptor (with their corresponding master key, caches and key pool), and also clone and delete each transaction that requires to be transferred to a different wallet. This work consolidates all individual disk writes into two batch operations. One for the descriptors creation from the legacy data and a second one for the execution of the migration process itself. Efficiently dumping all the information to disk at once atomically at the end of each process. This represent a speed up and also a consistency improvement. During migration, we either want to succeed or fail. No other outcomes should be accepted. We should never leave a partially migrated wallet on disk and request the user to manually restore the previous wallet from a backup (at least not if we can avoid it). Since the speedup depends on the storage device, benchmark results can vary significantly. Locally, I have seen a 15% speedup on a USB 3.2 pendrive. #### Note for Testers: The first commit introduces a benchmark for the migration process. This one can be cherry-picked on top of master to compare results pre and post changes. Please note that the benchmark setup may take some time (~70 seconds here) due to the absence of a batching mechanism for the address generation process (`GetNewDestination()` calls). ACKs for top commit: achow101: ACK c98fc36 theStack: re-ACK c98fc36 pablomartin4btc: re-ACK c98fc36 Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6

DrahtBot added the Wallet label Dec 2, 2023

This was referenced Dec 2, 2023

wallet: optimize migration process, batch db transactions #28574

Merged

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Closed

achow101 reviewed Jan 5, 2024

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

furszy added 2 commits January 6, 2024 12:40

wallet: ZapSelectTx, remove db rewrite code

a2b071f

The function does not return DBErrors::NEED_REWRITE.

wallet: migration, remove extra NotifyTransactionChanged call

595d50a

The wallet is unloaded at the beginning of the migration process, so no object is listening to the signals.

furszy force-pushed the 2023_wallet_zaptx branch from ff21425 to d26a285 Compare January 6, 2024 15:53

furszy commented Jan 6, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 6, 2024

furszy commented Jan 8, 2024

View reviewed changes

furszy mentioned this pull request Jan 31, 2024

rpc: Drop migratewallet experimental warning #28037

Merged

maflcko reviewed Jan 31, 2024

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

furszy mentioned this pull request Feb 7, 2024

wallet: batch erase procedures and improve 'EraseRecords' performance #29403

Merged

achow101 reviewed Feb 7, 2024

View reviewed changes

achow101 mentioned this pull request Feb 7, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

furszy force-pushed the 2023_wallet_zaptx branch 3 times, most recently from 875dbfb to e4f8a02 Compare February 9, 2024 17:22

furszy commented Feb 9, 2024

View reviewed changes

furszy added 2 commits February 9, 2024 14:54

scripted-diff: rename ZapSelectTx to RemoveTxs

9a3c5c8

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

furszy force-pushed the 2023_wallet_zaptx branch from e4f8a02 to 9a3c5c8 Compare February 9, 2024 17:55

DrahtBot removed the CI failed label Feb 9, 2024

DrahtBot mentioned this pull request Feb 10, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

achow101 merged commit 6ff0aa0 into bitcoin:master Feb 12, 2024

furszy deleted the 2023_wallet_zaptx branch February 12, 2024 18:45

hebasto reviewed Feb 13, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: simplify and batch zap wallet txes process #28987

wallet: simplify and batch zap wallet txes process #28987

Uh oh!

furszy commented Dec 2, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Dec 2, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Uh oh!

furszy left a comment

Uh oh!

Uh oh!

Uh oh!

achow101 Feb 7, 2024

Uh oh!

furszy Feb 9, 2024

Uh oh!

furszy left a comment

Uh oh!

Uh oh!

Uh oh!

furszy Feb 9, 2024

Uh oh!

achow101 commented Feb 9, 2024

Uh oh!

josibake commented Feb 12, 2024

Uh oh!

hebasto Feb 13, 2024

Uh oh!

furszy Feb 13, 2024

Uh oh!

Uh oh!


		// Apply db changes and remove transactions from the memory map
		if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR;

wallet: simplify and batch zap wallet txes process #28987

wallet: simplify and batch zap wallet txes process #28987

Uh oh!

Conversation

furszy commented Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 9, 2024

Uh oh!

josibake commented Feb 12, 2024

Uh oh!

hebasto Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

furszy Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Dec 2, 2023 •

edited

Loading

DrahtBot commented Dec 2, 2023 •

edited

Loading