wallet: optimize migration process, batch db transactions

furszy commented at 1:54 pm on October 3, 2023: member

Last step in a chain of PRs (#26836, #28894, #28987, #29403).

The initial benchmark conducted locally showed a ~65% processing time reduction, on a SSD. Results, from the very first benchmark, can be found at the end of the description.

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

Additionally, since db transactions relevant to the key pool generation were also batched, this work also speeds up the regular wallet creation process.

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). Which, I plan to address in a separate PR, just to not continue expanding this PR.

————————————————————————

Benchmark on master (debug mode)

ns/op	op/s	err%	total	benchmark
65,093,884,958.00	0.02	0.0%	65.09	`WalletMigration`

Benchmark on this branch (debug mode)

ns/op	op/s	err%	total	benchmark
22,926,814,750.00	0.04	0.0%	22.93	`WalletMigration`

DrahtBot commented at 1:54 pm on October 3, 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
Concept ACK	pablomartin4btc, theStack
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:

#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#30328 (wallet: Remove IsMine from migration code by achow101)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
#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)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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 Oct 3, 2023

furszy force-pushed on Oct 4, 2023

DrahtBot added the label CI failed on Oct 4, 2023

furszy force-pushed on Oct 5, 2023

DrahtBot removed the label CI failed on Oct 5, 2023

pablomartin4btc commented at 2:26 am on October 11, 2023: member

Concept ACK.

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

theStack commented at 11:57 am on November 9, 2023: contributor

Concept ACK

fanquake referenced this in commit a7f4f1a09c on Dec 8, 2023

DrahtBot added the label Needs rebase on Dec 8, 2023

furszy force-pushed on Dec 8, 2023

furszy commented at 8:13 pm on December 8, 2023: member

Rebased after #28894 merge.

DrahtBot removed the label Needs rebase on Dec 8, 2023

furszy force-pushed on Dec 8, 2023

DrahtBot added the label CI failed on Dec 8, 2023

DrahtBot added the label Needs rebase on Jan 2, 2024

furszy force-pushed on Jan 3, 2024

furszy commented at 2:28 pm on January 3, 2024: member

Rebased, conflicts solved. Next PR in the line is #28987.

DrahtBot removed the label Needs rebase on Jan 3, 2024

DrahtBot added the label Needs rebase on Jan 4, 2024

furszy force-pushed on Jan 4, 2024

DrahtBot removed the label Needs rebase on Jan 4, 2024

DrahtBot added the label Needs rebase on Jan 8, 2024

furszy commented at 4:14 pm on January 21, 2024: member

Decoupled the address book cloning related commits to #26836. I am marking this PR as a draft until the preceding PRs are merged.

furszy marked this as a draft on Jan 21, 2024

achow101 referenced this in commit 835948d44b on Feb 8, 2024

achow101 referenced this in commit 6ff0aa089c on Feb 12, 2024

josibake commented at 12:14 pm on February 13, 2024: member

Concept ACK

Starting to review, if you get a chance can you rebase this so that it’s only the relevant outstanding commits?

furszy force-pushed on Feb 13, 2024

furszy marked this as ready for review on Feb 13, 2024

furszy commented at 7:49 pm on February 13, 2024: member

Starting to review, if you get a chance can you rebase this so that it’s only the relevant outstanding commits?

Done. Rebased and reworked part of it. I still need to accommodate some code and improve the commit descriptions. But, at least the changes are reviewable now.

furszy force-pushed on Feb 13, 2024

DrahtBot removed the label Needs rebase on Feb 13, 2024

in src/bench/wallet_migration.cpp:73 in ac643893c6 outdated

68+        TestUnloadWallet(std::move(wallet));
69+    }
70+
71+    int wallet_num = 0;
72+    bench.epochs(NUM_WALLETS).run([&] {
73+        std::string filename = strprintf("legacy_%d", wallet_num);

maflcko commented at 12:04 pm on February 14, 2024:

0        auto filename{PathFromString(strprintf("legacy_%d", wallet_num));

I don’t like the c_str below. Also, it doesn’t compile on Windows.

Using PathFromString should be fine to “convert” ascii to a path segment, IIUC

Same above and #28894 (review)

furszy commented at 12:42 pm on February 14, 2024:

I don’t like the c_str below. Also, it doesn’t compile on Windows.

yeah, me neither.. I just did this 5 months ago. Fixed, thanks.

furszy force-pushed on Feb 14, 2024

furszy force-pushed on Feb 15, 2024

DrahtBot removed the label CI failed on Feb 15, 2024

furszy force-pushed on Feb 16, 2024

DrahtBot added the label Needs rebase on Feb 20, 2024

furszy force-pushed on Feb 20, 2024

DrahtBot removed the label Needs rebase on Feb 20, 2024

DrahtBot added the label CI failed on Feb 26, 2024

DrahtBot commented at 7:47 pm on February 26, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/21767740453

furszy force-pushed on Feb 26, 2024

DrahtBot removed the label CI failed on Feb 26, 2024

in src/bench/wallet_migration.cpp:40 in a2866d6ce0 outdated

35+    int NUM_WALLETS = 1;
36+    // Number of imported watch only addresses
37+    int NUM_WATCH_ONLY_ADDR = 20;
38+
39+    // Setup legacy wallets
40+    for (int i = 0; i < NUM_WALLETS; ++i) {

josibake commented at 10:44 am on February 27, 2024:

in “bench: add coverage for wallet migration process” (https://github.com/bitcoin/bitcoin/pull/28574/commits/a2866d6ce0a4bf8b249ca13c19747d6a209dadab):

Why the loop setup if we are only going to do one wallet? Seems better to either run multiple epochs or simplify the benchmark and take out the loop.

furszy commented at 1:22 pm on February 27, 2024:

Why the loop setup if we are only going to do one wallet? Seems better to either run multiple epochs or simplify the benchmark and take out the loop.

The idea was to run it multiple times to decrease the noise by taking the median. But the time this test was taking originally (10-15 min) was too much. Thus why I left everything in-place for when the migration time decreases.

in src/wallet/scriptpubkeyman.cpp:1787 in 5cc89e5eba outdated

1781@@ -1782,6 +1782,12 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1782         keyid_it++;
1783     }
1784 
1785+    WalletBatch batch(m_storage.GetDatabase());
1786+    if (!batch.TxnBegin()) {
1787+        LogPrintf("Error during descriptors migration, cannot initialize db transaction\n");

josibake commented at 10:49 am on February 27, 2024:

in “wallet: batch MigrateToDescriptor() db transactions” (https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7):

What about “Error during legacy wallet migration,” instead?

furszy commented at 1:29 pm on February 27, 2024:

What about “Error during legacy wallet migration,” instead?

The idea of the error message is to point out at which part of the process failed. Saying “Error during legacy wallet migration” is too broad (the same message could arise anywhere along migration). This part of the process generates descriptors from the legacy SKPM keys and scripts.

josibake commented at 4:37 pm on February 27, 2024:

I see, maybe “Error generating descriptors for migration, cannot initialize db transaction”? Also not a big deal, its just he current message is confusing to me in that we aren’t migrating descriptors, but taking existing keys, creating descriptors and then saving them in a new wallet.

furszy commented at 4:45 pm on February 27, 2024:

Sure. Done as suggested.

in src/wallet/scriptpubkeyman.cpp:1822 in 5cc89e5eba outdated

1816@@ -1811,8 +1817,8 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1817 
1818         // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
1819         auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
1820-        desc_spk_man->AddDescriptorKey(key, key.GetPubKey());
1821-        desc_spk_man->TopUp();
1822+        WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));

josibake commented at 10:58 am on February 27, 2024:

in “wallet: batch MigrateToDescriptor() db transactions” (https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7):

key.GetPubKey() is a pretty expensive operation and we are calling it above:

https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1812

and then again here. Not a big deal compared to some of the other stuff, but I ran the bench locally with and without the suggested change and its ~1.5% speedup (assuming I’m reading the bench results correctly :sweat_smile: ):

0# before
1|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
2|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
3|    6,936,309,493.00 |                0.14 |    0.0% |32,790,301,157.00 |11,790,224,744.00 |  2.781 |3,044,271,114.00 |    0.2% |      6.94 | `WalletMigration`
4
5
6# after
7|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
8|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
9|    6,836,475,065.00 |                0.15 |    0.0% |32,795,704,156.00 |11,732,850,832.00 |  2.795 |3,044,413,213.00 |    0.2% |      6.84 | `WalletMigration`

josibake commented at 11:39 am on February 27, 2024:

Also, in mapCryptedKey, the CPubKey is already there, so you could pull it out and avoid recalculating it: https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1762

But I’m not sure that’s worth the extra effort considering it would only make a difference if a majority of the keys come from mapCryptedKey and it seems that this isn’t making much of an improvement anyways.

furszy commented at 1:45 pm on February 27, 2024:

Sounds good. We could go further and remove the GetKey() from the keyids for-loop too (GetKey() access mapKeys for unencrypted wallets and mapCryptedKeys for encrypted ones –> which we already do at the beginning of the process). Still, maybe better to leave this and your suggestion for a follow-up. Just so this PR can be scoped to the db txn batching changes.

in src/wallet/scriptpubkeyman.cpp:1934 in 5cc89e5eba outdated

1928@@ -1923,9 +1929,9 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1929                 if (!GetKey(keyid, key)) {
1930                     continue;
1931                 }
1932-                desc_spk_man->AddDescriptorKey(key, key.GetPubKey());
1933+                WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));

josibake commented at 11:51 am on February 27, 2024:

in “wallet: batch MigrateToDescriptor() db transactions” (https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7):

Could get rid of the key.GetPubKey() call here and completely get rid of the privkeyid set above:

https://github.com/bitcoin/bitcoin/pull/28574/commits/5cc89e5eba7330ea3ab64ca66e364170dcd0b4f7#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1907-R1910

since keys.origins is a pair of the keyid and CPubKey. Running the bench with with this suggestion seemed to help:

0# after
1|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
2|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
3|    6,395,421,573.00 |                0.16 |    0.0% |32,790,285,573.00 |11,895,327,005.00 |  2.757 |3,044,102,935.00 |    0.2% |      6.40 | `WalletMigration`

6.94 -> 6.84 -> 6.4

furszy commented at 1:57 pm on February 27, 2024:

Sounds good but I would rather leave the improvement for a follow-up. Mainly to scope the PR changes to the db txn batching changes.

josibake commented at 4:39 pm on February 27, 2024:

Makes sense! I misread the title as meaning this PR is both optimizing the process and batching db transactions, but its clear from the PR description that this is focused on just the batching logic.

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

Looking good! I noticed a lot of key.GetPubKey() that could be removed, left some suggestions and verified locally that they improved the bench. I guess one conceptual question is if we are doing the GetPubKey() calls to regenerate the pubkeys in case we don’t trust the data in the legacy wallet?

furszy commented at 1:58 pm on February 27, 2024: member

Thanks for the review josi!

furszy force-pushed on Feb 27, 2024

josibake commented at 4:51 pm on February 27, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/28574/commits/868e2f64a9d7de3a980e64fd7acd7323214d3765

Code reviewed and verified the bench results. Thanks for taking the suggestions!

DrahtBot requested review from theStack on Feb 27, 2024

DrahtBot requested review from pablomartin4btc on Feb 27, 2024

in src/wallet/scriptpubkeyman.cpp:2018 in cd69403317 outdated

2008@@ -2009,9 +2009,15 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
2009 }
2010 
2011 bool LegacyScriptPubKeyMan::DeleteRecords()
2012+{
2013+    return RunWithinTxn(m_storage.GetDatabase(), /*process_desc=*/"delete legacy records", [&](WalletBatch& batch){
2014+        return DeleteRecords(batch);
2015+    });
2016+}

achow101 commented at 9:03 pm on March 22, 2024:

In cd694033173ec7de2a16187432a3d7aa33ff62ca “wallet: introduce ‘DeleteRecords(batch)’ to be used within an external db txn”

After this PR, this DeleteRecords() overload is only used by the tests, so I think it can just be removed (and perhaps never introduced).

furszy commented at 3:53 am on March 24, 2024:

After this PR, this DeleteRecords() overload is only used by the tests, so I think it can just be removed (and perhaps never introduced).

I didn’t do it because, aside from the test changes, it also requires creating a new batch object and beginning + committing the db txn inside ApplyMigrationData on https://github.com/bitcoin/bitcoin/commit/cd694033173ec7de2a16187432a3d7aa33ff62ca. And these changes will need to be removed in the last commit.

in src/wallet/wallet.cpp:4275 in 868e2f64a9 outdated

4275+        if (auto res_migration = wallet.ApplyMigrationData(batch, *data); !res_migration) {
4276+            error = util::ErrorString(res_migration);
4277+            return false;
4278+        }
4279+        // Process succeed, connect the SPKM signals
4280+        wallet.ConnectScriptPubKeyManNotifiers();

achow101 commented at 9:06 pm on March 22, 2024:

In 868e2f64a9d7de3a980e64fd7acd7323214d3765 “wallet: ApplyMigrationData, group all db txs into a single atomic operation”

I don’t think it’s necessary to connect the signals anymore since the local wallet exists outside of any contexts. Although removing this should be its own commit.

If you choose to not remove this, for completeness, this should also do NotifyCanGetAddressesChanged to match the previous behavior.

furszy commented at 4:13 am on March 24, 2024:

Sure. The wallet is also unloaded few lines after this signal connection. Will remove the lines.

in src/wallet/wallet.cpp:2349 in 4b3ec698a2 outdated

2346+    // Now that txs have been deleted from disk, erase them from memory.
2347+    ClearTxs(*res);
2348+    return {};
2349+}
2350+
2351+util::Result<std::vector<CWallet::MapWalletIt>> CWallet::RemoveTxs(WalletBatch& batch, std::vector<uint256>& txs_to_remove) {

achow101 commented at 9:15 pm on March 22, 2024:

In 4b3ec698a228fc531cca946f9a572e1dbdc4b0d5 “wallet: prepare RemoveTxs to be used within a broader db txn procedure”

I don’t really like this refactor. This means that any callers of this particular overload will have to remember to do ClearTxs, whereas callers of the other one don’t. This seems like something that could result in programming mistakes in the future. Generally our overloads should do basically the same things, just with the option of providing different arguments. Not clearing the txs from mapWallet in one overload is a pretty big difference that may not be easily noticed.

I think it would actually be a bit cleaner and better to have RemoveTxs detect if a database transaction has been started already and just choose to not begin a transaction if that’s the case.

furszy commented at 4:16 am on March 24, 2024:

I think it would actually be a bit cleaner and better to have RemoveTxs detect if a database transaction has been started already and just choose to not begin a transaction if that’s the case.

The rationale behind the decoupling wasn’t related to the existing (or not) db txn. It was to prevent another programming mistake: The provided db transaction could be aborted by the calling function (deliberately or undeliberately), causing the wallet to enter an inconsistent state where the data in memory differs from the data on disk. This would happen because, in the all-in-one version of this function, the transactions are cleared from the in-memory map.

I understand that for the migration process, this isn’t really an issue because the wallet will be unloaded anyway. We could go back to the all-in-one version of this function, but for other processes, if we ever decide to abort the db txn, the wallet will need to be reloaded from disk to recover the previous state.

I’m ok with merging back these functions. We just need to be careful not to abort the db txn and continue as if nothing happened after calling this method.

furszy commented at 2:13 pm on March 26, 2024:

I understand that for the migration process, this isn’t really an issue because the wallet will be unloaded anyway. We could go back to the all-in-one version of this function, but for other processes, if we ever decide to abort the db txn, the wallet will need to be reloaded from disk to recover the previous state.

I’m ok with merging back these functions. We just need to be careful not to abort the db txn and continue as if nothing happened after calling this method.

Ended up introducing a mechanism to listen for the db txn outcomes. This way, internal procedures can register callbacks to update the in-memory state only when the data is successfully committed to disk.

furszy force-pushed on Mar 26, 2024

furszy commented at 2:15 pm on March 26, 2024: member

Updated per feedback. Thanks achow.

furszy force-pushed on Mar 26, 2024

DrahtBot added the label Needs rebase on Mar 29, 2024

furszy force-pushed on Mar 29, 2024

furszy commented at 1:59 pm on March 29, 2024: member

rebased after #29130 merge.

DrahtBot added the label CI failed on Mar 29, 2024

DrahtBot removed the label Needs rebase on Mar 29, 2024

DrahtBot removed the label CI failed on Apr 4, 2024

DrahtBot added the label CI failed on May 1, 2024

DrahtBot commented at 9:59 pm on May 1, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/23239600985

DrahtBot marked this as a draft on May 13, 2024

DrahtBot commented at 8:05 am on May 13, 2024: contributor

Turned into draft for now, due to failing CI. If this is no longer a WIP, you can move it out of draft.

furszy force-pushed on May 15, 2024

furszy marked this as ready for review on May 15, 2024

DrahtBot removed the label CI failed on May 15, 2024

DrahtBot commented at 2:30 am on June 3, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/25006227127

DrahtBot added the label CI failed on Jun 3, 2024

bench: add coverage for wallet migration process 53654d0f24

wallet: batch MigrateToDescriptor() db transactions

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

6638709739

wallet: decouple default descriptors creation from external signer setup

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

89009eeb01

wallet: introduce 'SetWalletFlagWithDB' a0370064a0

wallet: provide WalletBatch to 'DeleteRecords'

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

fae2c61038

wallet: remove post-migration signals connection

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.

05d88f791b

wallet: introduce active db txn listeners

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

da5f1b08f3

wallet: provide WalletBatch to 'RemoveTxs'

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

71e0fa363c

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

wallet: ApplyMigrationData, group all db txs into a single atomic operation 4e72821e19

furszy force-pushed on Jun 3, 2024

furszy commented at 1:37 pm on June 3, 2024: member

Rebased due a hidden conflict with #26606.

DrahtBot removed the label CI failed on Jun 3, 2024

wallet: optimize migration process, batch db transactions #28574

Detailed Description:

Note for Testers:

Code Coverage

Reviews

Conflicts