wallet: optimize migration process, batch db transactions by furszy · Pull Request #28574 · bitcoin/bitcoin

furszy · 2023-10-03T13:54:18Z

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

DrahtBot · 2023-10-03T13:54:20Z

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, theStack, pablomartin4btc
Stale ACK	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:

#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)

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.

pablomartin4btc

Concept ACK.

I'll also perform the benchmark as instructed clearly in the description and will review the code later.

theStack · 2023-11-09T11:57:00Z

Concept ACK

… single db txn f053024 wallet: batch external signer descriptor import (Sjors Provoost) 1f65241 wallet: descriptors setup, batch db operations (furszy) 3eb769f wallet: batch legacy spkm TopUp (furszy) 075aa44 wallet: batch descriptor spkm TopUp (furszy) bb4554c bench: add benchmark for wallet creation procedure (furszy) Pull request description: Work decoupled from #28574. Instead of performing multiple single write operations per spkm setup call, this PR batches them all within a single atomic db txn. Speeding up the process and preventing the wallet from entering an inconsistent state if any of the intermediate transactions fail (which shouldn't happen but.. if it does, it is better to not store any spkm rather than storing them partially). To compare the changes, added benchmark in the first commit. ACKs for top commit: Sjors: re-utACK f053024 achow101: ACK f053024 BrandonOdiwuor: ACK f053024 theStack: Code-review ACK f053024 Tree-SHA512: aead8548473e17d4d53e8e7039bbaf5e8bf2fe83f33b33f81cdedefe8a31b7003ceb6d5379b1bad1ca2692e909492009a21284ec8338eede078df3d19046ab5a

theStack

I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.

Might be worth to update the PR description with benchmark results from release builds, as only those are relevant for end users and the speed-up could vary significantly?

src/bench/wallet_migration.cpp

src/wallet/wallet.cpp

furszy · 2024-09-26T22:55:16Z

Updated per feedback, thanks @theStack!

I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.

Might be worth to update the PR description with benchmark results from release builds, as only those are relevant for end users and the speed-up could vary significantly?

Yeah, now that all the other PRs got merged, things changed a bit. Aside from the consistency reasons, this PR is important for people running on HDDs. See #28037 (comment), which shared a pretty nice time improvement. Going from a 30 minutes migration to one that took only 80 seconds.
Still, that was a long time ago. Will give it a new run on an HDD and update the PR description.

theStack

Code-review ACK a4e1ba722be5dbd71a3d21e5da82aee5409d22bd

Thanks for following up!

Yeah, now that all the other PRs got merged, things changed a bit. Aside from the consistency reasons, this PR is important for people running on HDDs. See #28037 (comment), which shared a pretty nice time improvement. Going from a 30 minutes migration to one that took only 80 seconds.
Still, that was a long time ago. Will give it a new run on an HDD and update the PR description.

That sounds pretty impressive (>20x?). Would be very interesting indeed to re-benchmark with this branch again (cc @willcl-ark), though as said above the changes also make sense to me if the performance improvement is not that significant.

furszy · 2024-09-30T21:03:31Z

PR description updated. Created #31000 so we can all run the benchmark on an external HDD in a more friendly manner.
Have seen a 15% speedup locally on a USB 3.2 pendrive.

pablomartin4btc

ACK a4e1ba722be5dbd71a3d21e5da82aee5409d22bd

I've ran the benchmarks locally on SSD and on an external USB 3.2 pendrive and got an improvement of ~46% (i7-1260P, 32GB RAM, Ubuntu 22.04 - using pyperf system tune to get stable results).

This PR:

./build/src/bench/bench_bitcoin -filter=WalletMigration

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    5,322,062,123.00 |                0.19 |    0.0% |      5.32 | `WalletMigration`

master

./build/src/bench/bench_bitcoin -filter=WalletMigration

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|   11,362,482,640.00 |                0.09 |    0.0% |     11.36 | `WalletMigration`

pablomartin4btc · 2024-10-09T21:11:22Z

src/wallet/scriptpubkeyman.cpp

        auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
-        desc_spk_man->AddDescriptorKey(key, key.GetPubKey());
-        desc_spk_man->TopUp();
+        WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));


I ran a bench as well calling GetPubKey() only once instead of twice as is currently and didn't gain much out of it, code-wise it could be update it for clarity and practicality anyways but I understand this is not part of the code change and moreover if this is going to be entirely replaced by a refactoring on a follow-up.

furszy · 2024-10-10T23:13:32Z

@josibake @achow101 friendly ping. It seems we are quite close here.

DrahtBot · 2024-10-20T20:46:11Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/30733818111}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Grouping all db writes into a single atomic write operation. Speeding up the flow and preventing inconsistent states.

This will be useful in the following-up commit to batch the entire wallet migration process.

So it can be used within an external db txn context.

The wallet is isolated during migration and reloaded at the end of the process. There is no benefit on connecting the signals few lines before unloading the wallet.

Useful to ensure that the in-memory state is updated only after successfully committing the data to disk.

Preparing it to be used within a broader db txn procedure.

So it can be used within an external db txn context.

Perform a single db write operation for the entire migration procedure.

Perform a single db write operation for each external wallet (watch-only and solvables) for the entire migration procedure.

furszy · 2024-10-21T13:26:24Z

Sad rebase due to tiny conflict with #30937. Ready to go.

achow101 · 2024-10-23T19:21:12Z

ACK c98fc36

theStack

re-ACK c98fc36
(as per $ git range-diff a4e1ba7...c98fc36)

pablomartin4btc

re-ACK c98fc36

(no diff since my previous review, just rebased)

DrahtBot added the Wallet label Oct 3, 2023

furszy mentioned this pull request Oct 3, 2023

rpc: Drop migratewallet experimental warning #28037

Merged

furszy force-pushed the 2023_wallet_batch_migration branch from 1bc914b to 3ea30fc Compare October 4, 2023 01:16

DrahtBot added the CI failed label Oct 4, 2023

furszy force-pushed the 2023_wallet_batch_migration branch 2 times, most recently from 8476a92 to 6a2e6e4 Compare October 5, 2023 13:24

DrahtBot removed the CI failed label Oct 5, 2023

This was referenced Oct 6, 2023

rpc: add path to gethdkey #22341

Closed

wallet: Migrate entire address book entries to watchonly and solvables too #28610

Merged

wallet: Reload watchonly and solvables wallets after migration #28609

Merged

pablomartin4btc reviewed Oct 11, 2023

View reviewed changes

achow101 mentioned this pull request Oct 20, 2023

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

furszy mentioned this pull request Nov 9, 2023

TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test #27582

Closed

This was referenced Nov 16, 2023

wallet: batch all individual spkms setup db writes in a single db txn #28894

Merged

wallet: group independent db writes on single batched db transaction #25297

Closed

DrahtBot mentioned this pull request Nov 25, 2023

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

Closed

furszy mentioned this pull request Dec 2, 2023

wallet: simplify and batch zap wallet txes process #28987

Merged

DrahtBot added CI failed and removed CI failed labels Sep 12, 2024

theStack reviewed Sep 24, 2024

View reviewed changes

src/bench/wallet_migration.cpp Outdated Show resolved Hide resolved

src/bench/wallet_migration.cpp Outdated Show resolved Hide resolved

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

theStack reviewed Sep 25, 2024

View reviewed changes

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

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

furszy force-pushed the 2023_wallet_batch_migration branch from cb14d98 to a4e1ba7 Compare September 26, 2024 22:43

theStack approved these changes Sep 27, 2024

View reviewed changes

furszy mentioned this pull request Sep 29, 2024

bench: add support for custom data directory #31000

Merged

pablomartin4btc reviewed Oct 9, 2024

View reviewed changes

DrahtBot mentioned this pull request Oct 11, 2024

util: Check bilingual_str format strings at compile time #31074

Closed

furszy added 12 commits October 21, 2024 08:29

bench: add coverage for wallet migration process

66c9936

wallet: batch MigrateToDescriptor() db transactions

f2541d0

Grouping all db writes into a single atomic write operation. Speeding up the flow and preventing inconsistent states.

wallet: decouple default descriptors creation from external signer setup

6052c78

This will be useful in the following-up commit to batch the entire wallet migration process.

wallet: introduce 'SetWalletFlagWithDB'

122d103

wallet: provide WalletBatch to 'DeleteRecords'

055c053

So it can be used within an external db txn context.

wallet: remove post-migration signals connection

91e065e

The wallet is isolated during migration and reloaded at the end of the process. There is no benefit on connecting the signals few lines before unloading the wallet.

wallet: introduce active db txn listeners

57249ff

Useful to ensure that the in-memory state is updated only after successfully committing the data to disk.

wallet: provide WalletBatch to 'RemoveTxs'

aacaaaa

Preparing it to be used within a broader db txn procedure.

wallet: refactor ApplyMigrationData to return util::Result<void>

34bf079

wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans'

9ef20e8

So it can be used within an external db txn context.

wallet: migration, consolidate main wallet db writes

7c9076a

Perform a single db write operation for the entire migration procedure.

wallet: migration, consolidate external wallets db writes

c98fc36

Perform a single db write operation for each external wallet (watch-only and solvables) for the entire migration procedure.

theStack approved these changes Oct 23, 2024

View reviewed changes

pablomartin4btc approved these changes Oct 24, 2024

View reviewed changes

DashCoreAutoGuix mentioned this pull request Aug 8, 2025

Merge bitcoin/bitcoin#28987: wallet: simplify and batch zap wallet txes process DashCoreAutoGuix/dash#1093

Open

Conversation

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

Uh oh!

Detailed Description:

Note for Testers:

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Nov 9, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented Sep 30, 2024

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

furszy commented Oct 10, 2024

Uh oh!

DrahtBot commented Oct 20, 2024

Uh oh!

furszy commented Oct 21, 2024

Uh oh!

achow101 commented Oct 23, 2024

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

furszy commented Oct 3, 2023 •

edited

Loading

DrahtBot commented Oct 3, 2023 •

edited

Loading

furszy commented Sep 26, 2024 •

edited

Loading