rpc: Drop migratewallet experimental warning #28037
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
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.
|
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. |
Are you still working on this? If not, it would be good to leave a reply here. |
There was a problem hiding this comment.
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.
|
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. |
There was a problem hiding this comment.
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).
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. |
|
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. |
|
I've pushed a commit that mentions the time in the RPC doc. |
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 |
|
Moved to the 27.0 milestone. |
| "\nMigrate the wallet to a descriptor wallet.\n" | ||
| "A new wallet backup will need to be made.\n" | ||
| "\nThe migration process will create a backup of the wallet before migrating. This backup\n" | ||
| "file will be named <wallet name>-<timestamp>.legacy.bak and can be found in the directory\n" | ||
| "for this wallet. In the event of an incorrect migration, the backup can be restored using restorewallet." | ||
| "\nEncrypted wallets must have the passphrase provided as an argument to this call.", | ||
| "\nEncrypted wallets must have the passphrase provided as an argument to this call.\n" | ||
| "\nThis RPC may take a long time to complete. Increasing the RPC client timeout is recommended.", |
There was a problem hiding this comment.
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?
|
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? |
There was a problem hiding this comment.
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 (#28037 (comment)). 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.
There was a problem hiding this comment.
ACK f1684bb
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).
There was a problem hiding this comment.
ACK f1684bb
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.
Can you profile that so we can see where it is actually spending the most time? |
|
Not sure how to create a flame-graph, since it takes too long. But it may be eating time in |
|
@maflcko Can you provide any more details about the wallet? The output of |
|
It is just a normal testnet3 wallet: |
|
How long does it take to migrate if you just let it complete? |
|
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? |
|
Not sure. I am using the spinning storage only to test 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 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 |
There was a problem hiding this comment.
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.
|
Update: I'm cooking another set of improvements.. will try to have them for tonight. |
|
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 + 506b738. The new commit introduces a raw multi-insert statement. Locally, this shows a significant speedup but I'm on a SSD. |
|
I tried to reproduce using a wallet with ~1k tx located on a spinning disk -- although this time on regtest -- but I found will@will-ubuntu in ~/src/bitcoin on master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original migratewallet big
{
"wallet_name": "big",
"backup_path": "/media/will/PEPSI/big-wallet-900-tx-original/regtest/wallets/big/big-1707927351.legacy.bak"
}
________________________________________________________
Executed in 8.14 secs fish external
usr time 1.69 millis 226.00 micros 1.46 millis
sys time 0.45 millis 45.00 micros 0.40 millis
will@will-ubuntu in ~/src/bitcoin on master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original getwalletinfo
{
"walletname": "big",
"walletversion": 169900,
"format": "sqlite",
"balance": 13606.24414485,
"unconfirmed_balance": 0.00000000,
"immature_balance": 515.63072317,
"txcount": 952,
"keypoolsize": 4000,
"keypoolsize_hd_internal": 4000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoid_reuse": false,
"scanning": false,
"descriptors": true,
"external_signer": false,
"blank": false,
"birthtime": 0,
"lastprocessedblock": {
"hash": "16632672917b708d311b8197d7be852c9a5ce41413b04ecfc2ca72637148766b",
"height": 634
}
}The BDB wallet will@will-ubuntu in ~/src/bitcoin on master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original getwalletinfo
{
"walletname": "/home/will/bitcoin-chains/big-wallet-900-tx-original/regtest/wallets/big",
"walletversion": 169900,
"format": "bdb",
"balance": 13606.24414485,
"unconfirmed_balance": 0.00000000,
"immature_balance": 515.63072317,
"txcount": 952,
"keypoololdest": 1707925846,
"keypoolsize": 1000,
"hdseedid": "f48cadf402473b7726449f7e50c836466970299e",
"keypoolsize_hd_internal": 1000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoid_reuse": false,
"scanning": false,
"descriptors": false,
"external_signer": false,
"blank": false,
"birthtime": 1707925845,
"lastprocessedblock": {
"hash": "16632672917b708d311b8197d7be852c9a5ce41413b04ecfc2ca72637148766b",
"height": 634
}
} |
Yes, HD wallets are fine. The issue is only about pre-HD wallets. My version is |
|
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. 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. |
|
I was able to reproduce it with wallet version 60000 and 1k keys/tx I also see 30 mins... ₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 getwalletinfo
{
"walletname": "",
"walletversion": 60000,
"format": "bdb",
"balance": 14716.40625000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 78.12500000,
"txcount": 1000,
"keypoololdest": 1707941916,
"keypoolsize": 1000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoid_reuse": false,
"scanning": false,
"descriptors": false,
"external_signer": false,
"blank": false,
"birthtime": 1707941702,
"lastprocessedblock": {
"hash": "369d42451890f636c521f3d3d70241aa524eb36cfa53c4cd16248d3b9f772538",
"height": 1000
}
}
will@will-ubuntu in ~ : 🐍 3.11.6
₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 -rpcclienttimeout=9999999999 migratewallet ""
{
"wallet_name": "",
"backup_path": "/media/will/PEPSI/regtest-v60000/regtest/-1707945979.legacy.bak"
}
________________________________________________________
Executed in 32.20 mins fish external
usr time 2.44 millis 0.00 micros 2.44 millis
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 |
|
q: the wallet to be migrated, is already loaded by the time you call Because, if the wallet wasn't loaded by the newer version yet, it will run the 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. |
|
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: Ignoring the locking stuff since I didn't turn off the lock debugging, it seems like a big chunk of time is spent in Here's a flamegraph of migration of the same wallet with #26008: You can see that almost no time is spent in If I turn off debugging, migration of my test wallet is also way 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? |
|
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:
|
|
Ok, I didn't see that you already did that. 😅 lgtm ACK f1684bb |
There are more keys than transactions, and they are encrypted, the debug log says:
It should be loaded.
I think it does, at least for me, because on the SSD it was faster.
Sure, I'll try to write a functional test for this. |
|
Done in #29438 . Maybe move the discussion there, so that this can move forward and be merged? |
There was a problem hiding this comment.
ACK f1684bb
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.
|
@achow101 for context my test wallet has 8016 descriptors after migration: will@will-ubuntu in ~ : 🐍 3.11.6
₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 listdescriptors | jq '.descriptors | length'
8016I 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 :). Branch: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert. (Most of the changes are already inside #28574) |
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.