wallet: wallet_migration.py fails on sqlite-only build

maflcko commented at 3:35 pm on December 9, 2024: member

Steps to reproduce:

sqlite-only build (the default)
./bld-cmake/test/functional/wallet_migration.py

 02024-12-07T04:56:18.691000Z TestFramework (INFO): PRNG seed is: 7532643259753153678
 12024-12-07T04:56:18.691000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2
 22024-12-07T04:56:20.630000Z TestFramework (INFO): Test migration of a basic keys only wallet without balance
 32024-12-07T04:56:20.915000Z TestFramework (INFO): Test migration of a basic keys only wallet with a balance
 42024-12-07T04:56:26.448000Z TestFramework (INFO): Test backup file can be successfully restored
 52024-12-07T04:56:27.024000Z TestFramework (INFO): Test migration of a wallet with balance received on the seed
 62024-12-07T04:56:28.270000Z TestFramework (INFO): Test "nothing to migrate" when the user tries to migrate a loaded wallet with no legacy data
 72024-12-07T04:56:28.271000Z TestFramework (INFO): Test "nothing to migrate" when the user tries to migrate an unloaded wallet with no legacy data
 82024-12-07T04:56:28.273000Z TestFramework (INFO): Test migration of a wallet with all keys for a multisig
 92024-12-07T04:56:28.473000Z TestFramework (INFO): Test migration of a wallet that has some keys in a multisig
102024-12-07T04:56:28.798000Z TestFramework (INFO): Test migration of a wallet with watchonly imports
112024-12-07T04:56:29.107000Z TestFramework (INFO): Test migration of a pure watchonly wallet
122024-12-07T04:56:29.217000Z TestFramework (INFO): Test migration of a pure watchonly wallet with pubkeys in keypool
132024-12-07T04:56:29.326000Z TestFramework (INFO): Test migration of a wallet using old pk() coinbases
142024-12-07T04:56:29.529000Z TestFramework (INFO): Test migration of an encrypted wallet
152024-12-07T04:56:31.123000Z TestFramework (INFO): Check migratewallet errors for nonexistent wallets
162024-12-07T04:56:31.124000Z TestFramework (INFO): Test migration of a wallet that isn't loaded, specified by path
172024-12-07T04:56:31.300000Z TestFramework (INFO): Test migration of the wallet named as the empty string
182024-12-07T04:56:31.450000Z TestFramework (INFO): Test migration of a wallet that is not in a wallet directory
192024-12-07T04:56:31.584000Z TestFramework (INFO): Test migration of address book data
202024-12-07T04:56:35.062000Z TestFramework (INFO): Test migration of watch-only raw p2sh script
212024-12-07T04:56:35.346000Z TestFramework (INFO): Test migration when wallet contains conflicting transactions
222024-12-07T04:56:35.609000Z TestFramework (INFO): Test migration when wallet contains a hybrid pubkey
232024-12-07T04:56:35.829000Z TestFramework (INFO): Test that a failed migration is cleaned up
242024-12-07T04:56:35.968000Z TestFramework (ERROR): Assertion failed
25Traceback (most recent call last):
26  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 160, in try_rpc
27    fun(*args, **kwds)
28  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/coverage.py", line 50, in __call__
29    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
30                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/authproxy.py", line 146, in __call__
32    raise JSONRPCException(response['error'], status)
33test_framework.authproxy.JSONRPCException: Wallet file verification failed. Failed to open database path '/tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2/node0/regtest/wallets/failed'. Build does not support Berkeley DB database format.
34Unable to restore backup of wallet. (-4)
35During handling of the above exception, another exception occurred:
36Traceback (most recent call last):
37  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 135, in main
38    self.run_test()
39  File "/tmp/cirrus-ci-build/bitcoin-core/bld-cov/test/functional/wallet_migration.py", line 1031, in run_test
40    self.test_failed_migration_cleanup()
41  File "/tmp/cirrus-ci-build/bitcoin-core/bld-cov/test/functional/wallet_migration.py", line 895, in test_failed_migration_cleanup
42    assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
43  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
44    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
45           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
46  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 166, in try_rpc
47    raise AssertionError(
48AssertionError: Expected substring not found in error message:
49substring: 'Failed to create database'
50error message: 'Wallet file verification failed. Failed to open database path '/tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2/node0/regtest/wallets/failed'. Build does not support Berkeley DB database format.
51Unable to restore backup of wallet.'.

maflcko added the label Wallet on Dec 9, 2024

maflcko added the label CI failed on Dec 9, 2024

furszy commented at 5:01 pm on December 9, 2024: member

The error occurs during the final RestoreWallet() call, post migration error. We attempt to reload the BDB wallet in rw mode even though support for it is unavailable. Moving the process to open the wallet in read-only mode fixes the issue but it might unchain other problems; e.g. leave the read-only bdb wallet loaded, and “usable”, in the system. will cook up something to fix it.

achow101 commented at 5:11 pm on December 9, 2024: member

I’m surprised that previous revisions of #28710 or #31250 don’t seem to run into this, given that BDB is completely removed there. I suppose it’s possible that it also fixed the problem in doing the removal.

furszy commented at 8:00 pm on December 9, 2024: member

I’m surprised that previous revisions of #28710 or #31250 don’t seem to run into this, given that BDB is completely removed there. I suppose it’s possible that it also fixed the problem in doing the removal.

Just looking at it (after pushing the PR.. sorry). You fixed it in #31250, in 9beab081e977e86a462854035ad31a118a8b13ae. It works fine because the legacy wallet can no longer be loaded there, but a direct cherry-pick into master would prematurely block the reload while it’s possible when bdb is still enabled. I initially started coding the fix the same way you did but decided to keep the reload enabled until we fully disable bdb in #31250. Yet, I’m not totally convinced about the approach atm, happy to receive some feedback.

achow101 commented at 9:30 pm on December 9, 2024: member

You fixed it in #31250, in 9beab08

Ah yes!

but a direct cherry-pick into master would prematurely block the reload while it’s possible when bdb is still enabled.

Yes, but that seems trivially fixable? We already know whether the wallet being migrated was loaded, so we could just pass was_loaded to RestoreWallet to load the previously loaded wallet.

furszy commented at 4:21 pm on December 10, 2024: member

Yes, but that seems trivially fixable? We already know whether the wallet being migrated was loaded, so we could just pass was_loaded to RestoreWallet to load the previously loaded wallet.

Ha, yeah true. I was trying to avoid the RestoreWallet() double nullptr return value meaning (nullptr for errors and nullptr when was_loaded=true) but.. that is fine because we have the error field to compare results anyway. Just updated the PR with few other improvements. Thanks for the feedback!

wallet: wallet_migration.py fails on sqlite-only build #31447