rpc: Drop migratewallet experimental warning

achow101 commented at 3:03 pm on July 6, 2023: member

The migration process itself hasn’t fundamentally changed since it was added, so I think it’s reasonable to say that it is no longer experimental.

rpc: Drop migratewallet experimental warning 9ecff997e1

DrahtBot commented at 3:03 pm on July 6, 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	josibake, furszy, maflcko, ryanofsky, willcl-ark

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

DrahtBot added the label RPC/REST/ZMQ on Jul 6, 2023

furszy commented at 3:12 pm on July 6, 2023: member

There is a small bug in the addressbook migration code (fixed by #26836) that we might want to fix before removing the experimental warning.

Probably, I could decouple the fix and test coverage into an standalone PR so we can get more attention there.

maflcko commented at 8:40 am on July 10, 2023: member

Would it make sense to document that this is expected to take a long time on spinning storage? For me a freshly created BDB wallet takes ~5 minutes to migrate. Another one I wasn’t sure about, see #28057 (comment).

Also, there seem to be outstanding bugs with the RPC, see same thread as above.

fanquake added this to the milestone 26.0 on Jul 26, 2023

DrahtBot added the label CI failed on Sep 15, 2023

maflcko commented at 11:06 am on September 21, 2023: member

Would it make sense to document that this is expected to take a long time on spinning storage? For me a freshly created BDB wallet takes ~5 minutes to migrate.

Are you still working on this? If not, it would be good to leave a reply here.

DrahtBot removed the label CI failed on Sep 21, 2023

furszy commented at 2:24 pm on October 3, 2023: member

Would it make sense to document that this is expected to take a long time on spinning storage? For me a freshly created BDB wallet takes ~5 minutes to migrate.

Created #28574 to improve the situation. Let me know how it goes with it.

willcl-ark commented at 9:57 am on October 13, 2023: contributor

Is the plan here to review #28574 and try and get that in, if it improves things, or should we remove the warning as proposed here and add a note that it may take more time?

I’d be happy to review/test either when I know which approach we’d prefer.

furszy commented at 12:50 pm on October 13, 2023: member

Is the plan here to review #28574 and try and get that in, if it improves things, or should we remove the warning as proposed here and add a note that it may take more time?

I’d be happy to review/test either when I know which approach we’d prefer.

I would rather leave this warning for another release just so the process undergoes an entire release cycle without issues prior to dropping the “experimental” label. I don’t think that keeping it for another cycle would modify the legacy wallet removal schedule at all.

The reality is that, even though they may seem harmless, we continue encountering issues with this process.. e.g. #28609 and #28610. And I think that we should try to avoid, as much as possible, any unexpected behavior resulting from the wallet migration process as it could be quite alarming for our users.

Other than that, after reviews on #28609 and #28610, I would be happy to receive you on #28574. A process which its execution takes +5 minutes on a spinning disk hard drive seems pretty impractical for me (and could also be tagged as an issue).

achow101 commented at 3:31 pm on October 13, 2023: member

I don’t think that keeping it for another cycle would modify the legacy wallet removal schedule at all.

I’d prefer that the RPC is not marked as experimental for one release before we drop the legacy wallet entirely. We have the legacy wallet scheduled to be removed for 27.0, but if migration is still experimental for 26.0, then the schedule should be pushed back by a release.

maflcko commented at 7:53 am on October 19, 2023: member

At a minimum, I’d prefer if the doc said that this RPC could take a long time, and that bumping the client timeout is recommended. Otherwise users will call the RPC and only see the timeout error message with no way to get further feedback.

rpc: mention that migratewallet can take a while f1684bb88a

bitcoin deleted a comment on Oct 19, 2023

achow101 commented at 3:07 pm on October 19, 2023: member

I’ve pushed a commit that mentions the time in the RPC doc.

stickies-v commented at 12:19 pm on October 20, 2023: contributor

We have the legacy wallet scheduled to be removed for 27.0

According to #20160, it seems we’re only stopping creation of new wallets in 27.0, and removing BDB in 28.0? Given that there still are outstanding bugs (even if not necessarily super critical), removing the experimental label for 26.0 is unnecessarily risky in my view

achow101 removed this from the milestone 26.0 on Oct 23, 2023

achow101 added this to the milestone 27.0 on Oct 23, 2023

achow101 commented at 2:35 pm on October 23, 2023: member

Moved to the 27.0 milestone.

in src/wallet/rpc/wallet.cpp:748 in f1684bb88a

745         "\nThe migration process will create a backup of the wallet before migrating. This backup\n"
746         "file will be named <wallet name>-<timestamp>.legacy.bak and can be found in the directory\n"
747         "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet."
748-        "\nEncrypted wallets must have the passphrase provided as an argument to this call.",
749+        "\nEncrypted wallets must have the passphrase provided as an argument to this call.\n"
750+        "\nThis RPC may take a long time to complete. Increasing the RPC client timeout is recommended.",

maflcko commented at 4:00 pm on November 1, 2023:

Another problem is that (I presume) large single-keys-bag wallets may take hundred times longer to load. Not saying that this is a common use case, but I don’t think the RPC is non-experimental for users before this is fixed?

S3RK commented at 7:58 am on November 2, 2023:

Should we then include #26008 and maybe #28574 in 27.0?

DrahtBot added the label CI failed on Jan 13, 2024

murchandamus commented at 4:02 pm on January 31, 2024: contributor

Hey, I was taking a look at this PR. What is the status on this? Are we aiming to get this into 27.0? What about the two PRs, that @S3RK mentioned, they are open and in draft. Does this PR here depend on them?

furszy commented at 4:24 pm on January 31, 2024: member

Hey, I was taking a look at this PR. What is the status on this? Are we aiming to get this into 27.0? What about the two PRs, that @S3RK mentioned, they are open and in draft. Does this PR here depend on them?

#28574 is in draft because it is the parent PR. It consolidates the entire work in a single location, allowing tests and benchmarks to be performed against the end goal as well. This work was decoupled into three independent PRs: #28894 (merged), #28987, #26836. There is no specific order required for the review.

I think we should aim to get this one in for v27. The process shouldn’t take ~5 minutes to complete (https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1628507151). Since there is no feedback during the process, users might end up aborting it, thinking it has stalled, and leave their wallet in an inconsistent state, requiring manual recovery from the backup.

furszy commented at 4:32 pm on January 31, 2024: member

Also, we do have other bug fixes that should be included in v27 to deprecate this warning. See #28976, #28868, #29112, #29367. Would be nice to bring more attention to them.

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

ACK https://github.com/bitcoin/bitcoin/pull/28037/commits/f1684bb88a878eb3f54e945db39ea69b14256eef

Looks like all the known bugs have been fixed and most of the performance improvements have been merged. The only two outstanding performance improvements are #28987 and #26008, both of which seem close/RFM. Given that we are also including a warning that this process is expected to take longer, I think its safe to drop the warning even without #28987 and #26008 (obviously, better if we can also get those in with v27).

furszy commented at 9:23 pm on February 12, 2024: member

ACK https://github.com/bitcoin/bitcoin/commit/f1684bb88a878eb3f54e945db39ea69b14256eef

The only two outstanding performance improvements are #28987 and #26008

There are actually more. #28987, and also #29403, are prerequisites for further performance improvements on #28574.

That being said, while I would like to reach that goal (same as getting closer in #26008), I also think that we should move forward with this one.

maflcko commented at 9:16 am on February 13, 2024: member

Not sure about rushing this. I tested yesterday a bdb testnet3 wallet with 1000 txs on spinning storage, and I aborted the migration after 20 minutes. Happy to re-test if more pulls are merged.

I think the migration performance can be consider a bugfix, so I’d suggest to at least give this another two weeks after feature freeze (yesterday), according to #29028.

It seems plausible that the users having a bdb wallet with many transactions are also the ones more likely to still have a slow storage device. So removing the “experimental” warning may affect the users the most that require performance the most.

achow101 commented at 5:48 pm on February 13, 2024: member

I tested yesterday a bdb testnet3 wallet with 1000 txs on spinning storage, and I aborted the migration after 20 minutes.

Can you profile that so we can see where it is actually spending the most time?

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

The last batching changes might help but.. it smells like you are facing another issue apart from those. Profiling results would help diagnose the problem.

maflcko commented at 8:40 pm on February 13, 2024: member

Not sure how to create a flame-graph, since it takes too long. But it may be eating time in fdatasync in

0[#9](/bitcoin-bitcoin/9/)  0x0000559cb227c624 in wallet::SQLiteBatch::TxnCommit (this=0x7f5b24178fc0) at wallet/sqlite.cpp:668
1[#10](/bitcoin-bitcoin/10/) 0x0000559cb21cff66 in wallet::DescriptorScriptPubKeyMan::TopUp (this=0x7f5b24178970, size=<optimized out>) at wallet/scriptpubkeyman.cpp:2148
2[#11](/bitcoin-bitcoin/11/) 0x0000559cb21cb6d3 in wallet::LegacyScriptPubKeyMan::MigrateToDescriptor (this=0x7f5b24ee3900) at wallet/scriptpubkeyman.cpp:1925
3[#12](/bitcoin-bitcoin/12/) 0x0000559cb222b2f7 in wallet::CWallet::GetDescriptorsForLegacy (this=0x7f5b2517cc70, error=...) at wallet/wallet.cpp:3922
4[#13](/bitcoin-bitcoin/13/) 0x0000559cb222e8fa in wallet::DoMigration (wallet=..., context=..., error=..., res=...) at wallet/wallet.cpp:4155

achow101 commented at 8:43 pm on February 13, 2024: member

@maflcko Can you provide any more details about the wallet? The output of getwalletinfo would be helpful.

maflcko commented at 8:52 pm on February 13, 2024: member

It is just a normal testnet3 wallet:

 0{
 1  "walletname": "-1707752467.legacy.bak-1707848638.legacy.bak-1707849652.legacy.bak-1707855504.legacy.bak",
 2  "walletversion": 60000,
 3  "format": "bdb",
 4  "balance": 1.26999586,
 5  "unconfirmed_balance": 0.00000000,
 6  "immature_balance": 0.00000000,
 7  "txcount": 661,
 8  "keypoololdest": 1515426721,
 9  "keypoolsize": 999,
10  "unlocked_until": 0,
11  "paytxfee": 0.00000000,
12  "private_keys_enabled": true,
13  "avoid_reuse": false,
14  "scanning": false,
15  "descriptors": false,
16  "external_signer": false,
17  "blank": false,
18  "birthtime": 1,
19  "lastprocessedblock": {
20    "hash": "0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f",
21    "height": 2500000
22  }
23}

achow101 commented at 8:53 pm on February 13, 2024: member

How long does it take to migrate if you just let it complete?

maflcko commented at 9:09 pm on February 13, 2024: member

Ok, I can try that. I wonder if anyone else can try the HDD+BDB combo to double check. Or if really no one else has a HDD+BDB for testing, it can be declared unsupported?

furszy commented at 10:07 pm on February 13, 2024: member

Is the chain sync, reindex or any indexing procedure taking that long as well?

maflcko commented at 8:12 am on February 14, 2024: member

Not sure. I am using the spinning storage only to test migratewallet. It finished after 2 hours.

I don’t want to block this pull request, but I wanted to share what I am seeing when running the migration on a slow storage. Migration is a one-time cost, so I’ll leave it up to others whether to improve it, and by how much. However, if migratewallet is expected to take long for other wallets if they reside on slow storage, it may be good to add a note to the RPC? Something like, “Depending on the size of your wallet and the speed of your storage device, migration may take several hours. Make sure to disable the client RPC timeout. Otherwise, the migration RPC result will not make it back to you.”

Another problem is that (I presume) large single-keys-bag wallets may take hundred times longer to load after migration, even on fast storage, IIRC. If this is still the case, it seems like a higher priority to fix, because this is a cost to be paid every time the wallet is loaded.

maflcko commented at 10:25 am on February 14, 2024: member

I guess time loadwallet will be improved by #26008 (comment)

furszy commented at 12:16 pm on February 14, 2024: member

Not sure. I am using the spinning storage only to test migratewallet.

I’m asking because if other processes are equally slow, then this could be “ok” (and we could declare the hardware unsupported for core entirely…). But if only this process takes forever, then we are sure that there is an issue.

It finished after 2 hours.

We are definitely not expecting this process to take that long. The wallet you shared is really small.

Another problem is that (I presume) large single-keys-bag wallets may take hundred times longer to load after migration, even on fast storage, IIRC. If this is still the case, it seems like a higher priority to fix, because this is a cost to be paid every time the wallet is loaded. I guess time loadwallet will be improved by #26008 (comment)

It will, but #26008’s current implementation comes with higher memory costs, which is not that good if your wallet is running on a resource-constrained system. Yet, (personal view) it will be nice to tackle one issue at a time. Users need to be able to migrate their wallet in a decent amount of time before reaching the point of running a big single-key-bag wallet.

Continuing with the issue investigation, could you measure how much it takes to create a fresh wallet? It would be good to know how much it takes to top up the new key pool for the 8k default keys (the same process is executed during migration).

Also, how much available memory (ram) does your system have?.

And, just in case, try running #28574 too.

furszy commented at 2:34 pm on February 14, 2024: member

Update: I’m cooking another set of improvements.. will try to have them for tonight.

furszy commented at 3:27 pm on February 14, 2024: member

Or well, I just crafted a simple version of what I’m doing (I’m planning to create a statement builder around this work, and that will take me some more time).

Could you please try this branch: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert and share the results?

It is essentially #28574 + 506b73872b9ce232e8c6b9fefbdb0c33e04d0707. The new commit introduces a raw multi-insert statement. Locally, this shows a significant speedup but I’m on a SSD.

willcl-ark commented at 4:25 pm on February 14, 2024: contributor

I tried to reproduce using a wallet with ~1k tx located on a spinning disk – although this time on regtest – but I found migratewallet to be pleasantly fast:

 0will@will-ubuntu in ~/src/bitcoin on  master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
 1₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original migratewallet big
 2{
 3  "wallet_name": "big",
 4  "backup_path": "/media/will/PEPSI/big-wallet-900-tx-original/regtest/wallets/big/big-1707927351.legacy.bak"
 5}
 6
 7________________________________________________________
 8Executed in    8.14 secs      fish           external
 9   usr time    1.69 millis  226.00 micros    1.46 millis
10   sys time    0.45 millis   45.00 micros    0.40 millis
11
12
13will@will-ubuntu in ~/src/bitcoin on  master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
14₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original getwalletinfo
15{
16  "walletname": "big",
17  "walletversion": 169900,
18  "format": "sqlite",
19  "balance": 13606.24414485,
20  "unconfirmed_balance": 0.00000000,
21  "immature_balance": 515.63072317,
22  "txcount": 952,
23  "keypoolsize": 4000,
24  "keypoolsize_hd_internal": 4000,
25  "paytxfee": 0.00000000,
26  "private_keys_enabled": true,
27  "avoid_reuse": false,
28  "scanning": false,
29  "descriptors": true,
30  "external_signer": false,
31  "blank": false,
32  "birthtime": 0,
33  "lastprocessedblock": {
34    "hash": "16632672917b708d311b8197d7be852c9a5ce41413b04ecfc2ca72637148766b",
35    "height": 634
36  }
37}

The BDB wallet getwalletinfo (slightly smaller keypool size):

 0will@will-ubuntu in ~/src/bitcoin on  master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
 1₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original getwalletinfo
 2{
 3  "walletname": "/home/will/bitcoin-chains/big-wallet-900-tx-original/regtest/wallets/big",
 4  "walletversion": 169900,
 5  "format": "bdb",
 6  "balance": 13606.24414485,
 7  "unconfirmed_balance": 0.00000000,
 8  "immature_balance": 515.63072317,
 9  "txcount": 952,
10  "keypoololdest": 1707925846,
11  "keypoolsize": 1000,
12  "hdseedid": "f48cadf402473b7726449f7e50c836466970299e",
13  "keypoolsize_hd_internal": 1000,
14  "paytxfee": 0.00000000,
15  "private_keys_enabled": true,
16  "avoid_reuse": false,
17  "scanning": false,
18  "descriptors": false,
19  "external_signer": false,
20  "blank": false,
21  "birthtime": 1707925845,
22  "lastprocessedblock": {
23    "hash": "16632672917b708d311b8197d7be852c9a5ce41413b04ecfc2ca72637148766b",
24    "height": 634
25  }
26}

maflcko commented at 4:33 pm on February 14, 2024: member

0  "walletversion": 169900,

Yes, HD wallets are fine. The issue is only about pre-HD wallets. My version is 60000.

achow101 commented at 10:29 pm on February 14, 2024: member

I made a non-HD legacy wallet with double the number of transactions, put it on an external hdd, and migrated it. Migration took 230 seconds, which, while slow, is certainly not multiple hours.

 0$ src/bitcoin-cli -regtest -rpcwallet=/run/media/ava/F5CD-B8E0/big_nonhd_wallet getwalletinfo
 1{
 2  "walletname": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet",
 3  "walletversion": 60000,
 4  "format": "bdb",
 5  "balance": 251.59681094,
 6  "unconfirmed_balance": 49.00000000,
 7  "immature_balance": 0.07643467,
 8  "txcount": 1351,
 9  "keypoololdest": 1707936916,
10  "keypoolsize": 1000,
11  "paytxfee": 0.00000000,
12  "private_keys_enabled": true,
13  "avoid_reuse": false,
14  "scanning": false,
15  "descriptors": false,
16  "external_signer": false,
17  "blank": false,
18  "birthtime": 1707936495,
19  "lastprocessedblock": {
20    "hash": "6c96458f7db633ef353ff78fb92dae6c92539e733dae4b2cd48db814571ea299",
21    "height": 2524
22  }
23}
24$ time src/bitcoin-cli -regtest -rpcwallet=/run/media/ava/F5CD-B8E0/big_nonhd_wallet migratewallet
25{
26  "wallet_name": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet",
27  "backup_path": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet-1707949293.legacy.bak"
28}
29
30________________________________________________________
31Executed in  230.95 secs      fish           external
32   usr time    5.21 millis    0.00 micros    5.21 millis
33   sys time    0.41 millis  414.00 micros    0.00 millis

How many keys does that wallet have (it’ll be in the debug log, look for “Legacy Wallet Keys”)? This one I just made is 2333 keys.

willcl-ark commented at 10:30 pm on February 14, 2024: contributor

I was able to reproduce it with wallet version 60000 and 1k keys/tx I also see 30 mins…

 0₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 getwalletinfo
 1{
 2  "walletname": "",
 3  "walletversion": 60000,
 4  "format": "bdb",
 5  "balance": 14716.40625000,
 6  "unconfirmed_balance": 0.00000000,
 7  "immature_balance": 78.12500000,
 8  "txcount": 1000,
 9  "keypoololdest": 1707941916,
10  "keypoolsize": 1000,
11  "paytxfee": 0.00000000,
12  "private_keys_enabled": true,
13  "avoid_reuse": false,
14  "scanning": false,
15  "descriptors": false,
16  "external_signer": false,
17  "blank": false,
18  "birthtime": 1707941702,
19  "lastprocessedblock": {
20    "hash": "369d42451890f636c521f3d3d70241aa524eb36cfa53c4cd16248d3b9f772538",
21    "height": 1000
22  }
23}
24
25will@will-ubuntu in ~ : 🐍 3.11.6
26₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 -rpcclienttimeout=9999999999 migratewallet ""
27{
28  "wallet_name": "",
29  "backup_path": "/media/will/PEPSI/regtest-v60000/regtest/-1707945979.legacy.bak"
30}
31
32________________________________________________________
33Executed in   32.20 mins      fish           external
34   usr time    2.44 millis    0.00 micros    2.44 millis
35   sys time    0.37 millis  374.00 micros    0.00 millis

I was trying to get a flamegraph but something seems to have broken with my perf tool after upgrading kernels…

furszy commented at 11:59 pm on February 14, 2024: member

q: the wallet to be migrated, is already loaded by the time you call migratewallet? Or you are migrating a non-loaded wallet?

Because, if the wallet wasn’t loaded by the newer version yet, it will run the UpgradeKeyMetadata process during migration too. Which involves performing writes for 2 times the number of your key pool.

I just updated my branch batching that process too. In case someone want to test: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert. Will try to get an hdd tomorrow if this is not solved by then.

achow101 commented at 5:29 am on February 15, 2024: member

I’m not entirely convinced that migration is actually spending most of the time in disk I/O for these big wallets.

This is a flamegraph of the migration of my big nonhd wallet on master: master-migrate-big-nonhd-perf

Ignoring the locking stuff since I didn’t turn off the lock debugging, it seems like a big chunk of time is spent in IsMine after the descriptors have been created. This suggests to me that #26008 would provide a significant speedup there.

Here’s a flamegraph of migration of the same wallet with #26008: ismine-cache-migrate-big-nonhd-perf

You can see that almost no time is spent in IsMine, so I think #26008 should make migration of such wallets much faster:

0$ src/bitcoin-cli -regtest -rpcwallet=/run/media/ava/F5CD-B8E0/big_nonhd_wallet migratewallet
1{
2  "wallet_name": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet",
3  "backup_path": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet-1707974252.legacy.bak"
4}
5
6________________________________________________________
7Executed in  150.25 secs      fish           external
8   usr time    4.69 millis    0.00 micros    4.69 millis
9   sys time    0.41 millis  414.00 micros    0.00 millis

If I turn off debugging, migration of my test wallet is also way faster:

0$ src/bitcoin-cli -regtest -rpcwallet=/run/media/ava/F5CD-B8E0/big_nonhd_wallet migratewallet
1{
2  "wallet_name": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet",
3  "backup_path": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet-1707974585.legacy.bak"
4}
5
6________________________________________________________
7Executed in   82.31 secs      fish           external
8   usr time    0.00 millis    0.00 micros    0.00 millis
9   sys time    4.33 millis  407.00 micros    3.92 millis

@maflcko @willcl-ark Can you say how many keys (or descriptors after migration) the wallet has? Can you also try migrating it using #26008?

achow101 commented at 6:13 am on February 15, 2024: member

I’m any case, I didn’t think the performance issues should be a blocker for this PR. It’s clear that migration works, it may just take a while.

Missing this release will push back the timeline another release cycle, if not more.

maflcko commented at 7:52 am on February 15, 2024: member

I’m any case, I didn’t think the performance issues should be a blocker for this PR. It’s clear that migration works, it may just take a while.

Missing this release will push back the timeline another release cycle, if not more.

Yes, I agree. Sorry for not being clear.

Since it appears that at least one other person could reproduce a longer migrate time (30 minutes), why not mention that the user should take care to disable the client rpc timeout? Something along the lines of:

“Depending on the size of your wallet and the speed of your storage device, migration may take several hours. Make sure to disable the client RPC timeout. Otherwise, the migration RPC result will not make it back to you.”

maflcko commented at 7:58 am on February 15, 2024: member

Ok, I didn’t see that you already did that. :sweat_smile:

lgtm ACK f1684bb88a878eb3f54e945db39ea69b14256eef

maflcko commented at 9:25 am on February 15, 2024: member

How many keys does that wallet have (it’ll be in the debug log, look for “Legacy Wallet Keys”)? This one I just made is 2333 keys.

There are more keys than transactions, and they are encrypted, the debug log says:

0Wallet file version = 10500, last client version = 269900
1Legacy Wallet Keys: 0 plaintext, 4998 encrypted, 4998 w/ metadata, 4998 total.
2Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
3Wallet completed loading in             229ms
4setKeyPool.size() = 999
5mapWallet.size() = 661
6m_address_book.size() = 3524

q: the wallet to be migrated, is already loaded by the time you call migratewallet? Or you are migrating a non-loaded wallet?

It should be loaded.

I’m not entirely convinced that migration is actually spending most of the time in disk I/O for these big wallets.

I think it does, at least for me, because on the SSD it was faster.

@maflcko @willcl-ark Can you say how many keys (or descriptors after migration) the wallet has? Can you also try migrating it using #26008?

Sure, I’ll try to write a functional test for this.

maflcko commented at 11:25 am on February 15, 2024: member

Done in #29438 . Maybe move the discussion there, so that this can move forward and be merged?

ryanofsky approved

ryanofsky commented at 3:23 pm on February 15, 2024: contributor

Code review ACK f1684bb88a878eb3f54e945db39ea69b14256eef

“This RPC may take a long time to complete” seems like much more helpful and accurate documentation than “This call may not work as expected and may be changed in future releases” at this point.

willcl-ark approved

willcl-ark commented at 3:51 pm on February 15, 2024: contributor

ACK f1684bb88a878eb3f54e945db39ea69b14256eef

Agree that this should no longer be marked as experimental in the next release even though some migrations do appear to take a(n unexpectedly) long time.

fanquake merged this on Feb 16, 2024

fanquake closed this on Feb 16, 2024

willcl-ark commented at 11:04 am on February 19, 2024: contributor

@achow101 for context my test wallet has 8016 descriptors after migration:

0will@will-ubuntu in ~ : 🐍 3.11.6
1₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 listdescriptors | jq '.descriptors | length'
28016

I have also in the meantime tested a branch from @furszy which reduced migration time from 30 mins to 80 seconds 👀

furszy commented at 12:33 pm on February 19, 2024: member

I have also in the meantime tested a branch from @furszy which reduced migration time from 30 mins to 80 seconds 👀

That’s great! Sounds like a bug fix :). @maflcko could you also try it please?.

Branch: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert.

(Most of the changes are already inside #28574)

rpc: Drop migratewallet experimental warning #28037

Code Coverage

Reviews