backport: batch 2 of migratewallet RPC

[knst]

Depends on #7275

Issue being fixed or feature implemented

Further improvements of migratewallet RPC to migrate legacy wallets to descriptor wallets.

What was done?

Scope to be defined

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• backport: Merge bitcoin# 28035, 26957 #7258 - backport: Merge bitcoin# 28035, 26957

Please coordinate with the authors of these PRs to avoid merge conflicts.

[thepastaclaw]

Review Gate

Commit: 8b96a2bf

• ✅ Debounce: 849m ago (need 30m)

• ⏳ CI checks: build failure: Lint / Run linters

• ✅ CodeRabbit review: comment found

• ✅ Off-peak hours: off-peak (10:45 PM PT Thursday)

• Run review now (check to override)

[coderabbitai]

Walkthrough

This PR introduces a migratewallet RPC command and supporting infrastructure to migrate legacy BDB-based wallets to descriptor SQLite-based wallets. The implementation includes: descriptor API updates allowing selective private string generation, enumeration of legacy scripts and key-derived descriptors, database migration from BDB to SQLite format, orchestration of backup/restore flows, optional creation of watch-only and solvables wallets, and comprehensive functional tests covering basic wallets, multisig, encrypted wallets, and edge cases. Supporting changes enable copying of wallet transactions and add a wallets directory path property to the test framework.

Sequence Diagram

sequenceDiagram
    participant RPC as RPC Layer<br/>(migratewallet)
    participant Wallet as CWallet
    participant LegacySPKM as LegacyScriptPubKeyMan
    participant DescSPKM as DescriptorScriptPubKeyMan
    participant DB as WalletDatabase
    participant Context as WalletContext

    RPC->>Wallet: MigrateLegacyToDescriptor(wallet_name, passphrase)
    activate Wallet
    
    Wallet->>Wallet: Backup legacy wallet
    Wallet->>LegacySPKM: GetScriptPubKeys()
    activate LegacySPKM
    LegacySPKM-->>Wallet: unordered_set<CScript>
    deactivate LegacySPKM
    
    Wallet->>LegacySPKM: MigrateToDescriptor()
    activate LegacySPKM
    LegacySPKM->>LegacySPKM: Enumerate keys, HD chains,<br/>multisig-in-P2SH
    LegacySPKM->>LegacySPKM: Create combo() descriptors<br/>for loose/HD keys
    LegacySPKM->>LegacySPKM: InferDescriptor for<br/>remaining scripts
    LegacySPKM-->>Wallet: MigrationData
    deactivate LegacySPKM
    
    Wallet->>DB: MigrateToSQLite()
    activate DB
    DB->>DB: Read all BDB records
    DB->>DB: Create SQLite database
    DB->>DB: Write records in transaction
    DB-->>Wallet: success
    deactivate DB
    
    Wallet->>Wallet: ApplyMigrationData(data)
    activate Wallet
    Wallet->>DescSPKM: Add descriptors & keys
    activate DescSPKM
    DescSPKM->>DescSPKM: Setup descriptor handlers
    DescSPKM-->>Wallet: ready
    deactivate DescSPKM
    Wallet->>LegacySPKM: DeleteRecords()
    Wallet->>DB: EraseRecords(LEGACY_TYPES)
    deactivate Wallet
    
    Wallet->>Context: Optionally create<br/>watchonly/solvables wallets
    
    Wallet-->>RPC: MigrationResult{wallet_name,<br/>watchonly_wallet,<br/>solvables_wallet,<br/>backup_path}
    deactivate Wallet

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat: mnemonic support for descriptor wallets #6570: Overlapping modifications to descriptor wallet mnemonic handling and shared APIs (DescriptorScriptPubKeyMan::AddDescriptorKey, SetupDescriptorScriptPubKeyMans, walletdb changes) across descriptor setup logic.

Suggested reviewers

• UdjinM6
• PastaPastaPasta
• kwvg

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title 'backport: batch 2 of migratewallet RPC' clearly describes the main change: a batch of improvements to the migratewallet RPC feature. The title is specific and relates directly to the changeset, which implements wallet migration from legacy to descriptor format.
Description check	✅ Passed	The PR description mentions 'Further improvements of migratewallet RPC to migrate legacy wallets to descriptor wallets' and states 'Depends on PR #7275', which is related to the changeset. While the description lacks detail and leaves some sections blank, it is not unrelated or off-topic to the changes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

src/wallet/wallet.h (1)

1072-1073: Pass the mnemonic passphrase by const SecureString&.

Both overloads currently copy SecureString, which needlessly duplicates secret material in memory. Change const SecureString mnemonic_passphrase to const SecureString& mnemonic_passphrase for both function declarations.

♻️ Proposed fix

-    void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key, const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString& mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/wallet.h` around lines 1072 - 1073, Change the two declarations of
SetupDescriptorScriptPubKeyMans to take the mnemonic passphrase by reference
instead of by value: replace the parameter type `const SecureString
mnemonic_passphrase` with `const SecureString& mnemonic_passphrase` in both
overloads (the functions declared as `void SetupDescriptorScriptPubKeyMans(const
CExtKey& master_key, const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);` and `void
SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);`) so they become
`const SecureString&` for the passphrase.

src/wallet/scriptpubkeyman.cpp (1)

1930-1940: Consider adding a nullptr check after Parse for defensive programming.

While the descriptor string constructed from a valid key should always parse successfully, the HD chain path (line 2018-2021) includes explicit nullptr checking. Adding consistency here would guard against unexpected failures.

🔧 Optional: Add nullptr check for consistency

         // Construct the combo descriptor
         std::string desc_str = "combo(" + origin_str + HexStr(key.GetPubKey()) + ")";
         FlatSigningProvider keys;
         std::string error;
         std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false);
+        if (!desc) {
+            throw std::runtime_error(std::string(__func__) + ": failed to parse combo descriptor: " + error);
+        }
         WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/scriptpubkeyman.cpp` around lines 1930 - 1940, Add a nullptr check
after calling Parse on desc_str to mirror the defensive checks used elsewhere:
verify that the returned std::unique_ptr<Descriptor> desc is non-null before
constructing WalletDescriptor and creating DescriptorScriptPubKeyMan; if desc is
null, surface the parse error (from the existing error string) and handle it
consistently (e.g., log and return or throw) instead of proceeding to call
WalletDescriptor, DescriptorScriptPubKeyMan, AddDescriptorKey, and TopUp.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@doc/managing-wallets.md`:
- Around line 125-144: Confirm whether documentation updates were intended for
this backport: if the backport explicitly requested doc changes, add a note to
the PR description referencing that approval and keep the edits in the doc
(references: "Migrating Legacy Wallets to Descriptor Wallets", migratewallet,
restorewallet); otherwise revert the changes to doc/managing-wallets.md (lines
introducing migratewallet documentation) and move them to a separate
documentation-only follow-up PR so this backport contains only code changes.

In `@src/wallet/wallet.cpp`:
- Around line 4629-4630: UpdateWalletSetting is being called before helper
wallets (e.g. *_watchonly and *_solvables) are fully committed, causing
load_on_startup entries to remain if a later step fails and the wallet
directories are rolled back; move the UpdateWalletSetting calls (the ones that
set load_on_startup=true) so they execute only after the wallet commit/migration
has completed successfully (i.e., after all filesystem operations and rollback
points are passed) and do the same change for the other instance noted (the
second call around the other occurrence), ensuring settings are updated only on
successful commit of the new wallet directories.
- Around line 4794-4796: The backup copy operation in the wallet
migration/rollback code can attempt to copy a file onto itself for direct-file
wallets (where backup_path and temp_backup_location resolve to the same path),
causing copy_file to fail; update the logic around the uses of
temp_backup_location and backup_path (the copy_file calls around the code that
uses GetWalletDir(), backup_filename, and the rollback path) to guard against
this by checking if fs::equivalent(backup_path, temp_backup_location) or
backup_path == temp_backup_location and skipping the copy (or generating a
unique temp name) when they refer to the same file; apply the same guard to the
later copy_file occurrence(s) referenced near the other block (around the code
at the second copy_file use).
- Around line 4040-4041: The exception currently echoes the full seed phrase
when CMnemonic::Check(mnemonic) fails; remove the sensitive mnemonic from the
error text and throw a generic message instead (e.g., change the throw in the
CMnemonic::Check failure branch to use only __func__ and a non-sensitive string
like ": invalid mnemonic" without including the mnemonic variable or any
backticks). Ensure the change is applied where the throw is raised (the block
that calls CMnemonic::Check and constructs std::runtime_error).

In `@src/wallet/walletdb.cpp`:
- Around line 1182-1184: In EraseRecords(), the return value of
m_batch->Erase(key_data) is ignored causing EraseRecords() to always succeed;
update the loop that checks types.count(type) > 0 to capture the boolean result
of m_batch->Erase(key_data) and if it returns false, immediately return false
(or set an overall failure flag and return false at the end) so EraseRecords()
propagates delete failures back to the caller; ensure you reference the
EraseRecords() method and the m_batch->Erase(...) call when making this change.

In `@test/functional/wallet_migration.py`:
- Around line 141-143: The test currently mutates the dict returned by
basic0.getaddressinfo(new_addr) instead of checking it; replace the assignment
on basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1" with a real
assertion: call info = basic0.getaddressinfo(new_addr) and assert
info["hdkeypath"] == "m/44'/1'/0'/0/1" (while keeping the existing assert not
new_addr == addr), so the test actually verifies the derived path returned for
new_addr.

---

Nitpick comments:
In `@src/wallet/scriptpubkeyman.cpp`:
- Around line 1930-1940: Add a nullptr check after calling Parse on desc_str to
mirror the defensive checks used elsewhere: verify that the returned
std::unique_ptr<Descriptor> desc is non-null before constructing
WalletDescriptor and creating DescriptorScriptPubKeyMan; if desc is null,
surface the parse error (from the existing error string) and handle it
consistently (e.g., log and return or throw) instead of proceeding to call
WalletDescriptor, DescriptorScriptPubKeyMan, AddDescriptorKey, and TopUp.

In `@src/wallet/wallet.h`:
- Around line 1072-1073: Change the two declarations of
SetupDescriptorScriptPubKeyMans to take the mnemonic passphrase by reference
instead of by value: replace the parameter type `const SecureString
mnemonic_passphrase` with `const SecureString& mnemonic_passphrase` in both
overloads (the functions declared as `void SetupDescriptorScriptPubKeyMans(const
CExtKey& master_key, const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);` and `void
SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);`) so they become
`const SecureString&` for the passphrase.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: be43b772-e7a7-47b1-af64-43f1656f21c8

📥 Commits

Reviewing files that changed from the base of the PR and between bcc0092 and 8b96a2b.

📒 Files selected for processing (18)

• doc/managing-wallets.md
• doc/release-notes-19602.md
• src/script/descriptor.cpp
• src/wallet/rpc/wallet.cpp
• src/wallet/scriptpubkeyman.cpp
• src/wallet/scriptpubkeyman.h
• src/wallet/test/ismine_tests.cpp
• src/wallet/transaction.cpp
• src/wallet/transaction.h
• src/wallet/wallet.cpp
• src/wallet/wallet.h
• src/wallet/walletdb.cpp
• src/wallet/walletdb.h
• src/wallet/walletutil.h
• test/functional/test_framework/test_node.py
• test/functional/test_runner.py
• test/functional/tool_wallet.py
• test/functional/wallet_migration.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine the lines in question
if [ -f "doc/managing-wallets.md" ]; then
  echo "File exists. Checking lines 125-144:"; 
  sed -n '125,144p' doc/managing-wallets.md
else
  echo "File not found. Listing doc directory:";
  ls -la doc/ 2>/dev/null | head -20;
fi

Repository: dashpay/dash

Length of output: 1637

---

🏁 Script executed:

# Check git history for recent changes to this file to understand context
git log --oneline -10 -- doc/managing-wallets.md 2>/dev/null || echo "No git history available"

Repository: dashpay/dash

Length of output: 183

---

🏁 Script executed:

# Search for any references to 'migratewallet' to understand scope of changes
rg "migratewallet" doc/ 2>/dev/null | head -20

Repository: dashpay/dash

Length of output: 441

---

Confirm whether documentation changes were explicitly requested for this backport.

Per the coding guidelines, changes to doc/** should be avoided unless specifically prompted. This PR modifies doc/managing-wallets.md (lines 125-144) with documentation for the migratewallet feature. If this backport was requested to include documentation updates, that should be noted; otherwise, these changes should be split into a separate documentation follow-up.

🧰 Tools

🪛 LanguageTool

[style] ~133-~133: Consider removing “of” to be more concise
Context: ...solvables. _watchonlycontains all of the watchonly scripts._solvables` c...

(ALL_OF_THE)

---

[style] ~136-~136: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ding P2SH scripts. Given that there is an extremely large number of possible configurations for t...

(EN_WEAK_ADJECTIVE)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@doc/managing-wallets.md` around lines 125 - 144, Confirm whether
documentation updates were intended for this backport: if the backport
explicitly requested doc changes, add a note to the PR description referencing
that approval and keep the edits in the doc (references: "Migrating Legacy
Wallets to Descriptor Wallets", migratewallet, restorewallet); otherwise revert
the changes to doc/managing-wallets.md (lines introducing migratewallet
documentation) and move them to a separate documentation-only follow-up PR so
this backport contains only code changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't echo the mnemonic in the exception.

If validation fails here, the full seed phrase is copied into the error text and can leak into logs or RPC output. Keep the message generic.

Suggested fix

     const SecureString mnemonic = mnemonic_arg.empty() ? CMnemonic::Generate(m_args.GetIntArg("-mnemonicbits", CHDChain::DEFAULT_MNEMONIC_BITS)) : mnemonic_arg;
     if (!CMnemonic::Check(mnemonic)) {
-        throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(mnemonic) + "`");
+        throw std::runtime_error(std::string(__func__) + ": invalid mnemonic");
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (!CMnemonic::Check(mnemonic)) {
	throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(mnemonic) + "`");
	if (!CMnemonic::Check(mnemonic)) {
	throw std::runtime_error(std::string(__func__) + ": invalid mnemonic");
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/wallet.cpp` around lines 4040 - 4041, The exception currently
echoes the full seed phrase when CMnemonic::Check(mnemonic) fails; remove the
sensitive mnemonic from the error text and throw a generic message instead
(e.g., change the throw in the CMnemonic::Check failure branch to use only
__func__ and a non-sensitive string like ": invalid mnemonic" without including
the mnemonic variable or any backticks). Ensure the change is applied where the
throw is raised (the block that calls CMnemonic::Check and constructs
std::runtime_error).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Delay startup-setting writes until the helper wallets are fully committed.

*_watchonly and *_solvables are added to load_on_startup before migration finishes. If a later step fails, rollback deletes those wallet dirs but never removes the new settings, so the next startup will try to autoload wallets that no longer exist.

Also applies to: 4667-4668

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/wallet.cpp` around lines 4629 - 4630, UpdateWalletSetting is being
called before helper wallets (e.g. *_watchonly and *_solvables) are fully
committed, causing load_on_startup entries to remain if a later step fails and
the wallet directories are rolled back; move the UpdateWalletSetting calls (the
ones that set load_on_startup=true) so they execute only after the wallet
commit/migration has completed successfully (i.e., after all filesystem
operations and rollback points are passed) and do the same change for the other
instance noted (the second call around the other occurrence), ensuring settings
are updated only on successful commit of the new wallet directories.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Several failure paths leave an already-loaded wallet unexpectedly unloaded.

After the original wallet is removed from context, failures in MakeWalletDatabase, CWallet::Create, or MigrateToSQLite return immediately without restoring the previous loaded state. A failed migratewallet should not silently unload the caller's wallet.

Also applies to: 4702-4710, 4761-4764

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard the backup copy for direct-file wallets.

For plain-file wallets under wallets/, backup_path and temp_backup_location resolve to the same file. Both copy_file(..., copy_options::none) calls then fail during rollback, which breaks recovery հենց for the direct-file migration path this PR supports.

Suggested fix

         // Migration failed, cleanup
         // Copy the backup to the actual wallet dir
         fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
-        fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
+        if (temp_backup_location != backup_path) {
+            fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
+        }
@@
         // Move the backup to the wallet dir
-        fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
-        fs::remove(temp_backup_location);
+        if (temp_backup_location != backup_path) {
+            fs::copy_file(temp_backup_location, backup_path, fs::copy_options::overwrite_existing);
+            fs::remove(temp_backup_location);
+        }

Also applies to: 4837-4839

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/wallet.cpp` around lines 4794 - 4796, The backup copy operation in
the wallet migration/rollback code can attempt to copy a file onto itself for
direct-file wallets (where backup_path and temp_backup_location resolve to the
same path), causing copy_file to fail; update the logic around the uses of
temp_backup_location and backup_path (the copy_file calls around the code that
uses GetWalletDir(), backup_filename, and the rollback path) to guard against
this by checking if fs::equivalent(backup_path, temp_backup_location) or
backup_path == temp_backup_location and skipping the copy (or generating a
unique temp name) when they refer to the same file; apply the same guard to the
later copy_file occurrence(s) referenced near the other block (around the code
at the second copy_file use).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Propagate delete failures from EraseRecords().

Line 1183 ignores the return value from m_batch->Erase(key_data). If one delete fails, this method still returns true, so migration can report success while legacy records remain in the wallet DB.

🩹 Proposed fix

-        if (types.count(type) > 0) {
-            m_batch->Erase(key_data);
-        }
+        if (types.count(type) > 0) {
+            if (!m_batch->Erase(key_data)) {
+                m_batch->CloseCursor();
+                return false;
+            }
+            m_database.IncrementUpdateCounter();
+        }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (types.count(type) > 0) {
	m_batch->Erase(key_data);
	}
	if (types.count(type) > 0) {
	if (!m_batch->Erase(key_data)) {
	m_batch->CloseCursor();
	return false;
	}
	m_database.IncrementUpdateCounter();
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/wallet/walletdb.cpp` around lines 1182 - 1184, In EraseRecords(), the
return value of m_batch->Erase(key_data) is ignored causing EraseRecords() to
always succeed; update the loop that checks types.count(type) > 0 to capture the
boolean result of m_batch->Erase(key_data) and if it returns false, immediately
return false (or set an overall failure flag and return false at the end) so
EraseRecords() propagates delete failures back to the caller; ensure you
reference the EraseRecords() method and the m_batch->Erase(...) call when making
this change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

This check never verifies the new derivation path.

Line 143 mutates the dict returned by getaddressinfo() instead of asserting on it, so this regression test passes even if migration hands back the wrong next index.

Suggested fix

         self.log.info("Test for re-using old addresses after migration")
         new_addr = basic0.getnewaddress()
         assert not new_addr == addr
-        basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1"
+        assert_equal(basic0.getaddressinfo(new_addr)["hdkeypath"], "m/44'/1'/0'/0/1")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	new_addr = basic0.getnewaddress()
	assert not new_addr == addr
	basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1"
	new_addr = basic0.getnewaddress()
	assert not new_addr == addr
	assert_equal(basic0.getaddressinfo(new_addr)["hdkeypath"], "m/44'/1'/0'/0/1")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/functional/wallet_migration.py` around lines 141 - 143, The test
currently mutates the dict returned by basic0.getaddressinfo(new_addr) instead
of checking it; replace the assignment on
basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1" with a real
assertion: call info = basic0.getaddressinfo(new_addr) and assert
info["hdkeypath"] == "m/44'/1'/0'/0/1" (while keeping the existing assert not
new_addr == addr), so the test actually verifies the derived path returned for
new_addr.