wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet #31423

furszy commented at 5:59 PM on December 4, 2024: member

Currently, the migration process creates a brand-new descriptor wallet with no connection to the user's legacy wallet when the legacy wallet lacks key material and contains only watch-only scripts. This behavior is not aligned with user expectations. If the legacy wallet contains only watch-only scripts, the migration process should only generate a watch-only wallet instead.

TODO List:

Explain that migratewallet renames the watch-only after migration, and also that the wallet will not have keys enabled.

DrahtBot commented at 11:11 AM on December 5, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31423.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, pablomartin4btc, rkrux
Concept ACK	Sjors

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:

#32818 (Add read-only mode to sqlite db and use in bitcoin-wallet by willcl-ark)
#32758 (wallet: remove dead code in legacy wallet migration by rkrux)
#32273 (wallet: Fix relative path backup during migration. by davidgumberg)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

exist -> exists [singular “backup path” requires “exists”]

<sup>drahtbot_id_4_m</sup>

DrahtBot added the label CI failed on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

DrahtBot added the label Wallet on Dec 5, 2024

DrahtBot added the label CI failed on Dec 7, 2024

in src/wallet/wallet.cpp:4123 in 37e3cab479 outdated

4118 | @@ -4112,8 +4119,10 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
4119 |          if (data.master_key.key.IsValid()) {
4120 |              SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key);
4121 |          } else {
4122 | -            // Setup with a new seed if we don't.
4123 | -            SetupOwnDescriptorScriptPubKeyMans(local_wallet_batch);
4124 | +            // Setup with a new seed, only if we are actually migrating any spendable descriptor
4125 | +            if (!empty_local_wallet) {

achow101 commented at 8:35 PM on December 9, 2024:

In 37e3cab479080db9a3ac10f9b9b5c7f6c0e07882 "wallet: migration, don't create spendable wallet from a watch-only legacy wallet"

This check can go above next to the !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS).

furszy commented at 6:20 PM on December 10, 2024:

sure, done as suggested. Also added an extra !data->master_key.key.IsValid() to the empty_local_wallet definition.

DrahtBot added the label Needs rebase on Dec 9, 2024

in test/functional/wallet_migration.py:1032 in 37e3cab479 outdated

1027 | +        res = wallet.migratewallet()
1028 | +        wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name'])
1029 | +        assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.get_bytes().hex()})'))
1030 | +        # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
1031 | +        assert_equal('bare_p2pk_watchonly', res['wallet_name'])
1032 | +        assert "bare_p2pk" not in self.nodes[0].listwallets()

achow101 commented at 8:55 PM on December 9, 2024:

In 37e3cab479080db9a3ac10f9b9b5c7f6c0e07882 "wallet: migration, don't create spendable wallet from a watch-only legacy wallet"

Should also check that the wallet does not appear in listwalletdir:

assert "bare_p2pk" not in [w["name"] for w in self.nodes[0].listwalletdir()["wallets"]]

This causes the test to fail. Examining the datadir after the test shows us that the wallet directory is still there, and the wallet.dat file is still intact and valid, although missing descriptor records.

The spendable wallet should be deleted if it is empty, although this needs to be done carefully to avoid deleting the backup file that is stored in the wallet directory.

furszy commented at 6:23 PM on December 10, 2024:

This causes the test to fail. Examining the datadir after the test shows us that the wallet directory is still there, and the wallet.dat file is still intact and valid, although missing descriptor records.

Good catch. The wallet is actually entirely empty. There's neither address book records nor any transaction. We can safely remove it 👍🏼 .

furszy force-pushed on Dec 10, 2024

DrahtBot removed the label Needs rebase on Dec 10, 2024

DrahtBot removed the label CI failed on Dec 10, 2024

achow101 added the label Needs backport (28.x) on Dec 11, 2024

in src/wallet/wallet.cpp:4545 in 8aab56772c outdated

4544 | +            res.wallet = local_wallet;
4545 | +            res.wallet_name = wallet_name;
4546 | +        } else {
4547 | +            // At this point, the main wallet is empty after migration, meaning it has no records.
4548 | +            // Therefore, we can safely remove it.
4549 | +            fs::path path_to_remove = fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path();

achow101 commented at 5:18 PM on December 11, 2024:

This also deletes the backup file.

furszy commented at 6:17 PM on December 11, 2024:

Upps, corrected. Thanks. Was interesting to know that we have no backup file existence general check. Added one now.

in test/functional/wallet_migration.py:1029 in 8aab56772c outdated

1017 | @@ -1018,6 +1018,12 @@ def test_migrate_simple_watch_only(self):
1018 |          res, _ = self.migrate_and_get_rpc("bare_p2pk")
1019 |          wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name'])
1020 |          assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})'))
1021 | +
1022 | +        # Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
1023 | +        assert_equal('bare_p2pk_watchonly', res['wallet_name'])
1024 | +        assert "bare_p2pk" not in self.master_node.listwallets()
1025 | +        assert "bare_p2pk" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]]

achow101 commented at 5:19 PM on December 11, 2024:

Should also check that the backup file is still present.

furszy commented at 6:17 PM on December 11, 2024:

Done. Thx.

furszy force-pushed on Dec 11, 2024

achow101 removed the label Needs backport (28.x) on Dec 13, 2024

in src/wallet/sqlite.h:175 in 788bb48f67 outdated

 170 | +    std::vector<fs::path> Files() override
 171 | +    {
 172 | +        std::vector<fs::path> files;
 173 | +        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
 174 | +        files.emplace_back(m_dir_path / "database");
 175 | +        files.emplace_back(m_dir_path / "db.log");

achow101 commented at 7:46 PM on December 30, 2024:

In 788bb48f6712c39995831020dc36297f88c42f71 "wallet: introduce method to return all db created files"

These are created only by BDB.

SQLite makes only wallet.dat and wallet.dat-journal.

furszy commented at 4:17 PM on January 22, 2025:

Fixed. Thanks.

Sjors commented at 12:53 PM on January 21, 2025: member

Concept ACK

Can this go in parallel with #31495 or better to wait for it?

furszy force-pushed on Jan 22, 2025

furszy commented at 4:21 PM on January 22, 2025: member

Can this go in parallel with #31495 or better to wait for it?

Can go in parallel. There shouldn't be any conflict between these two.

DrahtBot added the label Needs rebase on Feb 13, 2025

furszy force-pushed on Feb 14, 2025

DrahtBot removed the label Needs rebase on Feb 14, 2025

pablomartin4btc commented at 10:57 AM on March 4, 2025: member

Concept ACK

nit (top PR's description & last commit body - perhaps it's obvious but still):

If the legacy wallet contains only watch-only scripts, the migration process should only generate a watch-only descriptor wallet instead.

(I'll test and review soon)

in src/wallet/sqlite.cpp:120 in 33b170721d outdated

 117 |  {
 118 |      {
 119 |          LOCK(g_sqlite_mutex);
 120 |          LogPrintf("Using SQLite Version %s\n", SQLiteDatabaseVersion());
 121 | -        LogPrintf("Using wallet %s\n", m_dir_path);
 122 | +        LogPrintf("Using wallet %s\n", fs::PathToString(m_dir_path));

pablomartin4btc commented at 1:46 PM on March 18, 2025:

Any reasons why you wouldn't use std::filesystem::path::string instead (same for other instances):

        LogPrintf("Using wallet %s\n", m_dir_path.string());

davidgumberg commented at 10:14 PM on April 8, 2025:

Although I'm not convinced that I understand clearly what the problems are, a description of why std::filesystem::path::string is not used is in #22937, which introduced fs::PathToString() and fs::StringToPath() to avoid use of std::filesystem::path::string where previously boost::filesystem::path::string was used:

Commit message of https://github.com/bitcoin/bitcoin/pull/22937/commits/6544ea5035268025207d2402db2f7d90fde947a6:

Block unsafe fs::path std::string conversion calls

There is no change in behavior. This just helps prepare for the transition from boost::filesystem to std::filesystem by avoiding calls to methods which will be unsafe after the transaction to std::filesystem to due lack of a boost::filesystem::path::imbue equivalent and inability to set a predictable locale.

pablomartin4btc commented at 12:02 PM on April 9, 2025:

It seems there are inefficiencies in the string constructor used within plus it brings issues on Windows.

ryanofsky commented at 5:20 PM on April 9, 2025:

re: #31423 (review)

Yes the only reason PathToString exists is because on windows the path::string() method depends on the current locale and won't work reliably. The path::utf8string() method could be used instead but this doesn't work relliably on unix, so a wrapper is needed. The code comments try to explain this but maybe could be improved:

https://github.com/bitcoin/bitcoin/blob/bb92bb36f211b88e4c1aa031a4364795cbd24767/src/util/fs.h#L153-L162

pablomartin4btc commented at 8:51 PM on June 19, 2025:

nit: Just noticed that LogPrintf is being deprecated. Would you consider using LogInfo here instead?

in src/wallet/wallet.cpp:4096 in 33b170721d outdated

4091 | @@ -4092,6 +4092,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
4092 |          return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
4093 |      }
4094 |  
4095 | +    // When the legacy wallet has no spendable scripts, it will be replaced by the watch-only/solvable wallets.
4096 | +    bool empty_local_wallet = data.desc_spkms.empty() && !data.master_key.key.IsValid();

pablomartin4btc commented at 3:54 PM on March 18, 2025:

nit (to add a bit of clarity - I also think this CWallet::ApplyMigrationData should be refactored, at least extracting some behaviour into functions to begin with if possible):

    bool is_local_wallet_empty = data.desc_spkms.empty() && !data.master_key.key.IsValid();

pablomartin4btc commented at 5:46 PM on March 25, 2025: member

I see the changes done on test/functional/wallet_migration.py applied on master makes the test to fail, validating its purpose, however when I test this branch manually (on both bitcoin-qt and bitcoind + cli) I got a bad_function_call (returning from here) when I create a "blank" legacy wallet, import an address (watch-only) and run the migratewallet to it (both wallets are being created, but not loaded, I see the details from the wo look correct with getwalletinfo but not the "spendable/ main", which when I load it has "format": "sqlite", and "descriptors": false,).

On the other hand, in master, when I create a blank legacy wallet with the imported address, the migratewallet process creates both, the "main" wallet (called like that in the source code) and the wo one (watch-only), and if I run getnewaddress on the main wallet I got an address, when it was originally blank with no keys (before migration I could get "Error: This wallet has no available keys (code -4)"). is the case you are trying to solve here?

furszy force-pushed on Apr 1, 2025

in src/wallet/sqlite.h:109 in 57694818bb outdated

 104 | @@ -105,7 +105,7 @@ class SQLiteDatabase : public WalletDatabase
 105 |  private:
 106 |      const bool m_mock{false};
 107 |  
 108 | -    const std::string m_dir_path;
 109 | +    const fs::path m_dir_path;
 110 |  
 111 |      const std::string m_file_path;

davidgumberg commented at 10:35 PM on April 8, 2025:

nit, maybe while you are touching this anyways:

    const fs::path m_file_path;

but, this is less clear of a win since SQLiteDatabase::Filename() will have to convert it back.

(https://github.com/davidgumberg/bitcoin/commit/9f85c1f45c6f808f37ab3715578a8f213a74fe6b)

furszy commented at 12:37 AM on April 11, 2025:

Yes. I don't see any benefit of doing it now.

in src/wallet/sqlite.h:150 in a48d81047e outdated

 165 | @@ -166,6 +166,14 @@ class SQLiteDatabase : public WalletDatabase
 166 |      void IncrementUpdateCounter() override { ++nUpdateCounter; }
 167 |  
 168 |      std::string Filename() override { return m_file_path; }
 169 | +    /** Return paths to all database created files */

davidgumberg commented at 8:26 PM on April 9, 2025:

Should probably be skipped for BerkeleyDatabase, but should have a test for Files(), e.g.:

Maybe something like https://github.com/davidgumberg/bitcoin/commit/db9449b547b07c59f73e3ae0a74eb808325bac48

</summary>

#ifdef USE_SQLITE
BOOST_AUTO_TEST_CASE(sqlite_db_files)
{
    DatabaseOptions options;
    DatabaseStatus status;
    bilingual_str error;
    std::unique_ptr<SQLiteDatabase> sqldb = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error);

    auto sqlite_internal_filename(sqlite3_db_filename(sqldb->m_db, "main"));
    BOOST_CHECK(sqlite_internal_filename != nullptr);

    std::vector<fs::path> sql_paths;
    sql_paths.push_back(fs::PathFromString(sqlite3_filename_database(sqlite_internal_filename)));
    sql_paths.push_back(fs::PathFromString(sqlite3_filename_journal(sqlite_internal_filename)));
    BOOST_TEST(sql_paths == sqldb->Files(), boost::test_tools::per_element());
}
#endif

</details>

Edit: This test is actually failing on macOS, where sqlite's filepath differs from this, I haven't yet figured out if my test is wrong.

wallet/test/db_tests.cpp:163: error: in "db_tests/sqlite_db_files": check sql_paths == sqldb->Files() has failed
  - mismatch at position 0: ["/private/var/folders/qn/7t0vq3ts721cmgt0tgrtgzl80000gn/T/test_common bitcoin/db_tests/sqlite_db_files/4401d34f82818d7bcb73/sqlite/wallet.dat" == "/var/folders/qn/7t0vq3ts721cmgt0tgrtgzl80000gn/T/test_common bitcoin/db_tests/sqlite_db_files/4401d34f82818d7bcb73/sqlite/wallet.dat"] is false
  - mismatch at position 1: ["/private/var/folders/qn/7t0vq3ts721cmgt0tgrtgzl80000gn/T/test_common bitcoin/db_tests/sqlite_db_files/4401d34f82818d7bcb73/sqlite/wallet.dat-journal" == "/var/folders/qn/7t0vq3ts721cmgt0tgrtgzl80000gn/T/test_common bitcoin/db_tests/sqlite_db_files/4401d34f82818d7bcb73/sqlite/wallet.dat-journal"] is false

furszy commented at 10:55 AM on May 28, 2025:

if your implementation works, we could replace the current sqlite Files() code for your code (using sqlite3_filename_database and sqlite3_filename_journal instead of the hardcoded path).

DrahtBot added the label Needs rebase on Apr 10, 2025

in src/wallet/wallet.cpp:4326 in b111268897 outdated

4544 | @@ -4521,7 +4545,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4545 |          // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
4546 |          success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
4547 |          if (!success) {
4548 | -            success = DoMigration(*local_wallet, context, error, res);
4549 | +            success = DoMigration(*local_wallet, context, error, res, empty_local_wallet);
4550 |          } else {
4551 |              // Make sure that descriptors flag is actually set
4552 |              local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);

davidgumberg commented at 10:52 PM on April 10, 2025:

Please feel free to disregard:

        if (!local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
            success = DoMigration(*local_wallet, context, error, res, empty_local_wallet);
        } else {
            success = true;
            // Make sure that descriptors flag is actually set
            local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);

pablomartin4btc commented at 9:15 AM on April 11, 2025:

This has been resolved recently in #32149:

https://github.com/bitcoin/bitcoin/blob/a4fd565191e3184a174757f7a8fbda6afc98e50a/src/wallet/wallet.cpp#L4537-L4544

furszy force-pushed on Apr 24, 2025

DrahtBot removed the label Needs rebase on Apr 24, 2025

DrahtBot commented at 10:51 PM on April 24, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/41112959019</sub>

<details><summary>Hints</summary>

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.

</details>

DrahtBot added the label CI failed on Apr 24, 2025

furszy force-pushed on Apr 24, 2025

DrahtBot removed the label CI failed on Apr 25, 2025

maflcko commented at 7:37 AM on April 25, 2025: member

There are some typos reported, which look real: #31423 (comment).

DrahtBot added the label Needs rebase on Apr 25, 2025

refactor: remove sqlite dir path back-and-forth conversion d04f6a97ba

wallet: introduce method to return all db created files 1de423e0a0

furszy force-pushed on May 28, 2025

DrahtBot removed the label Needs rebase on May 28, 2025

wallet: refactor, dedup wallet re-loading code e86d71b749

furszy force-pushed on May 28, 2025

furszy renamed this:
~~wallet: migration, don't create spendable wallet from a watch-only legacy wallet~~
wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet
on May 28, 2025

furszy force-pushed on May 28, 2025

furszy commented at 10:46 AM on May 28, 2025: member

Rebased, fixed a typo and added an extra commit deduplicating the wallet re-loading code.

There are some typos reported, which look real: #31423 (comment).

done thanks. It would be neat to have the bot outputting the typo commit hash too (just a small idea, maybe someone else wants to implement it).

in test/functional/wallet_migration.py:125 in 6d9bf36b2c outdated

 120 | +        # (in which case the wallet name would be suffixed by the 'watchonly' term)
 121 | +        wallet_name = migrate_info['wallet_name']
 122 |          wallet = self.master_node.get_wallet_rpc(wallet_name)
 123 |          assert_equal(wallet.getwalletinfo()["descriptors"], True)
 124 |          self.assert_is_sqlite(wallet_name)
 125 | +        # Always verify the backup path exist after migration

pablomartin4btc commented at 7:35 PM on May 28, 2025:

nit: only if you to re-touch

        # Always verify the backup path exists after migration

pablomartin4btc commented at 7:43 PM on May 28, 2025: member

code review ACK 6d9bf36

Checked all pushes diffs. I like the latest/ current version with the refactoring & dedup re-loading wallets code. Since you touched that area, and there's no test for solvables wallet when there's no main/ local, perhaps you could add one?

I'll it test in a bit.

in src/wallet/wallet.cpp:4312 in 6d9bf36b2c outdated

4312 | +    // Notes:
4313 | +    // When non-empty: the local wallet becomes the main spendable wallet.
4314 | +    // When empty: The local wallet is excluded from the result, as the
4315 | +    //             user does not expect an empty spendable wallet after
4316 | +    //             migrating only watch-only scripts.
4317 | +    bool empty_local_wallet = false;

achow101 commented at 9:43 PM on May 28, 2025:

In 6d9bf36b2c061088abb0d839fee90683ff1525e1 "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"

Instead of having this output parameter, perhaps we could check if local_wallet has any SPKMs after DoMigratione.g. if (local_wallet->GetAllScriptPubKeyMans().empty()). These should be equivalent checks.

furszy commented at 5:50 AM on May 29, 2025:

done

DrahtBot requested review from Sjors on May 29, 2025

pablomartin4btc commented at 1:35 AM on May 29, 2025: member

I've tested this branch (6d9bf36) and I still got "bad_function_call", as mentioned in my previous testing, when I try to migrate a blank legacy wallet with an imported address. I've isolated the case and it's only happening on a blank wallet with private keys enabled (disabled private keys on blank wallet with imported address works fine).

Steps to reproduce the issue (first create a legacy wallet using v28):

start the node with ./releases/v28.0/bin/bitcoind -regtest -datadir=/tmp/btc -deprecatedrpc=create_bdb
on a diff terminal ./releases/v28.0/bin/bitcoin-cli -regtest -datadir=/tmp/btc createwallet "legacy_bad_call" false true "" false false
same terminal as above ./releases/v28.0/bin/bitcoin-cli -regtest -datadir=/tmp/btc -rpcwallet=legacy_bad_call importaddress bcrt1q5suud4au3ydeatsvu3afj2dky2cz66w2w5zxhm (an address from another legacy wallet)
stop the v28 node
start a node with this branch ./build/bin/bitcoind -regtest -datadir=/tmp/btc
on a diff terminal ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc migratewallet legacy_bad_call

error code: -1
error message:
bad_function_call

The steps above work fine on master:

./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc migratewallet legacy_bad_call
{
  "wallet_name": "legacy_bad_call",
  "watchonly_name": "legacy_bad_call_watchonly",
  "backup_path": "/tmp/btc/regtest/wallets/legacy_bad_call/legacy_bad_call_1748481476.legacy.bak"
}

wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet

Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.

b789907346

furszy force-pushed on May 29, 2025

furszy commented at 5:56 AM on May 29, 2025: member

I've tested this branch (6d9bf36) and I still got "bad_function_call", as mentioned in my previous #31423#pullrequestreview-2694630661, when I try to migrate a blank legacy wallet with an imported address. I've isolated the case and it's only happening on a blank wallet with private keys enabled (disabled private keys on blank wallet with imported address works fine).

Hmm, can you debug the reason? I can't replicate the behavior locally.

in src/wallet/wallet.cpp:4339 in b789907346

4333 | @@ -4313,6 +4334,15 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4334 |      // fails to reload.
4335 |      std::set<fs::path> wallet_dirs;
4336 |      if (success) {
4337 | +        Assume(!res.wallet); // We will set it here.
4338 | +        // Check if the local wallet is empty after migration
4339 | +        if (empty_local_wallet) {

achow101 commented at 11:59 PM on May 29, 2025:

In b78990734621b8fe46c68a6e7edaf1fbd2f7d351 "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"

I think empty_local_wallet is unnecessary and the condition can check local_wallet->GetAllScriptPubKeyMans().empty() directly.

furszy commented at 11:18 AM on May 30, 2025:

I think empty_local_wallet is unnecessary and the condition can check local_wallet->GetAllScriptPubKeyMans().empty() directly.

That would also delete blank legacy wallets.

achow101 commented at 11:02 PM on June 4, 2025: member

ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351

DrahtBot requested review from pablomartin4btc on Jun 4, 2025

pablomartin4btc commented at 3:14 AM on June 19, 2025: member

tACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351

I can't reproduce the bug I found earlier, even if I go back to the previous changes. I've also verified several tests in wallet_migration.py changing them to force the faulty scenario with no success.

Something I notice now is that when a legacy blank watchonly wallet being its keypool empty is migrated the process throws different output regarding the wallet_name when the legacy wallet was originally created with private_keys_enabled=True -> { "wallet_name": "wallet_name_watchonly",... or without them (private_keys_enabled=False) -> { "wallet_name": "wallet_name",... resulting in both descriptor watchonly wallets with no private keys. Not sure if this is intentional or a small inconsistency.

in src/qt/walletcontroller.cpp:471 in b789907346

 467 | @@ -468,7 +468,7 @@ void MigrateWalletActivity::migrate(const std::string& name)
 468 |          auto res{node().walletLoader().migrateWallet(name, passphrase)};
 469 |  
 470 |          if (res) {
 471 | -            m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->wallet->getWalletName())));
 472 | +            m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name)));

pablomartin4btc commented at 3:58 AM on June 19, 2025:

This change (from res->wallet->getWalletName() to name) is because the "main"/ migrated wallet could have the _watchonly prefix after migration.

nit: this is already happening for the message box, perhaps there /at the top) you can assign GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name)) to a var and use it here.

rkrux commented at 2:42 PM on July 2, 2025: contributor

LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351

I verified from MigrateToDescriptor that data.desc_spkms can contain only spendable SPKMs as the watch-only ones are filtered out in data.watch_descs when the descriptor can't be converted to a private string. Rest two cases of HDChains and Non-HDKeys push SPKMs in data.desc_spkms and they both have access to the corresponding key material. So, I believe the following condition seems sufficient to check for spendable material.

!data.desc_spkms.empty() || data.master_key.key.IsValid();

achow101 merged this on Jul 2, 2025

achow101 closed this on Jul 2, 2025

furszy deleted the branch on Jul 2, 2025

skylinesales commented at 10:23 PM on July 2, 2025: none

.

bitcoin deleted a comment on Jul 2, 2025

rkrux commented at 11:58 AM on July 3, 2025: contributor

I did the TODO mentioned in the PR description in PR #32866.