feat: new RPC migratewallet to migrate legacy wallets to backport wallets#7275
feat: new RPC migratewallet to migrate legacy wallets to backport wallets#7275knst wants to merge 10 commits intodashpay:developfrom
Conversation
|
|
✅ Review complete (commit 4f25d93) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b60e277f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Delete the wallet directories | ||
| for (fs::path& dir : wallet_dirs) { | ||
| fs::remove_all(dir); |
There was a problem hiding this comment.
The rollback path deletes every path in wallet_dirs with remove_all, but for top-level legacy wallets (name "", which map to wallets/wallet.dat) the stored parent directory is the global wallet directory itself. If migration fails in that case, this call can recursively delete all wallets (and the temporary backup placed under walletdir) before restore runs, causing broad data loss instead of isolated cleanup.
Useful? React with 👍 / 👎.
| if (!addr_pair.second.IsChange()) { | ||
| data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
In the non-receive address-book branch, the code intends to clone entries to all migrated wallets, but the unconditional continue after writing to watchonly_wallet skips the solvable_wallet block whenever both wallets exist. Migrations that create both _watchonly and _solvables therefore lose send-label/purpose metadata in the solvables wallet.
Useful? React with 👍 / 👎.
WalkthroughThis PR adds an experimental migratewallet RPC and end-to-end support to convert legacy (non-descriptor) wallets to descriptor-based wallets. Changes include legacy script enumeration and descriptor construction in LegacyScriptPubKeyMan, new migration data structures, DB helpers to erase legacy records and migrate BDB→SQLite, wallet orchestration to create auxiliary watch-only/solvable wallets and apply migration results, RPC wiring for migratewallet, updates to descriptor serialization behavior, and extensive functional tests covering many migration scenarios. Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant WalletRPC as Wallet RPC
participant CWallet as CWallet
participant LegacySPKM as LegacyScriptPubKeyMan
participant DescSPKM as DescriptorScriptPubKeyMan
participant DB as WalletDatabase
participant AuxWallet as Auxiliary Wallets (watchonly/solvables)
Client->>WalletRPC: migratewallet(wallet_name, passphrase)
WalletRPC->>CWallet: MigrateLegacyToDescriptor(...)
CWallet->>CWallet: Create backup (*.legacy.bak)
CWallet->>LegacySPKM: GetScriptPubKeys() / MigrateToDescriptor()
LegacySPKM-->>CWallet: MigrationData (descriptors, master key, counters)
CWallet->>DescSPKM: AddDescriptor()/SetupDescriptorScriptPubKeyMans()
CWallet->>AuxWallet: Create watchonly/solvables wallets (if needed)
CWallet->>DB: MigrateToSQLite()
CWallet->>DB: EraseRecords(LEGACY_TYPES)
CWallet-->>WalletRPC: MigrationResult (wallet names, backup_path)
WalletRPC-->>Client: RPC response (wallet_name, watchonly_name?, solvables_name?, backup_path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
src/wallet/test/ismine_tests.cpp (1)
47-53: Add a watch-only case forGetScriptPubKeys().These new assertions validate non-owned and spendable scripts, but the migration logic also depends on watch-only scripts being surfaced so the
_watchonlywallet can be built correctly. A small watch-only coverage point here would make this regression check much more complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/test/ismine_tests.cpp` around lines 47 - 53, Add a watch-only case: after the non-owned check and before/after the spendable key assertions, call keystore.GetLegacyScriptPubKeyMan()->AddWatchOnly(scriptPubKey) (or the appropriate AddWatchOnly method on the LegacyScriptPubKeyMan), then assert that keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey) equals ISMINE_WATCH_ONLY and that keystore.GetLegacyScriptPubKeyMan()->GetScriptPubKeys().count(scriptPubKey) reflects the watch-only entry (== 1); use the same GetLegacyScriptPubKeyMan(), GetScriptPubKeys(), AddWatchOnly(), and IsMine() symbols to locate where to insert these checks.src/wallet/wallet.cpp (2)
4402-4403: Minor style: prefer!container.empty()overcontainer.size() > 0.Using
empty()is idiomatic C++ and can be more efficient for some container types (though not forstd::vector).Proposed fix
- if (txids_to_delete.size() > 0) { + if (!txids_to_delete.empty()) {- if (dests_to_delete.size() > 0) { + if (!dests_to_delete.empty()) {Also applies to: 4509-4509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 4402 - 4403, Replace size() > 0 checks with empty() checks: where the code tests txids_to_delete.size() > 0 (and the similar occurrence around the other instance), change it to !txids_to_delete.empty(); this is the idiomatic and slightly more efficient form—locate the conditional that declares deleted_txids and any other conditional using txids_to_delete.size() > 0 and swap to !container.empty().
4274-4306: Critical failure handling relies on process termination.The
assert()statements after DB deletion (lines 4286, 4293, 4301, 4305) will terminate the process on failure. While the comment mentions a backup exists, this is a harsh recovery path that requires manual intervention.This pattern is consistent with Bitcoin Core's approach for irrecoverable migration failures, but consider documenting in the RPC help that users should verify their backup is accessible before migration, as recovery requires manual restoration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 4274 - 4306, The code currently uses assert() after DB recreation and writes (when calling MakeDatabase / m_database, batch->TxnBegin(), batch->Write(...), and batch->TxnCommit()) which will abort the process on failure; replace these fatal asserts with graceful error handling: log a clear error via the wallet/db logger including error details, ensure any open transaction is aborted (batch->TxnAbort()), close and clean up the partially created DB (m_database->Close() and remove the new file), attempt to restore the original backup or leave instructions to the caller, and return a failure status (false) or throw a recoverable exception from the enclosing function instead of terminating; also update the RPC/help text for the migration call to instruct users to verify their wallet backup before running migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/rpc/wallet.cpp`:
- Around line 1129-1131: The RPC help examples currently show calling
migratewallet with no wallet specified, which the migratewallet handler rejects;
update the RPCExamples block (the HelpExampleCli and HelpExampleRpc calls near
the migratewallet RPC definition) to pass an explicit wallet name (and/or use
the wallet endpoint) so the example actually succeeds — e.g. change
HelpExampleCli("migratewallet", "") and HelpExampleRpc("migratewallet", "") to
include a wallet parameter like HelpExampleCli("migratewallet", "\"mywallet\"")
and HelpExampleRpc("migratewallet", "\"mywallet\"") or alternatively show using
-rpcwallet=mywallet; ensure you edit the RPCExamples surrounding the
migratewallet RPC to reflect this.
In `@src/wallet/scriptpubkeyman.cpp`:
- Around line 2416-2420: The AddDescriptorKey flow is persisting empty defaulted
mnemonic strings causing imported single-key descriptors to appear as having
mnemonics; update the logic so mnemonic material is only recorded when at least
one of the strings is non-empty: either change
DescriptorScriptPubKeyMan::AddDescriptorKey to only pass
mnemonic/mnemonic_passphrase into AddDescriptorKeyWithDB when one of them is
non-empty, or update AddDescriptorKeyWithDB to check m_mnemonics /
m_crypted_mnemonics before inserting and skip storing empty strings; reference
functions/fields: DescriptorScriptPubKeyMan::AddDescriptorKey,
AddDescriptorKeyWithDB, m_mnemonics, m_crypted_mnemonics, and GetMnemonicString
to ensure callers observe absence of mnemonics when both inputs are empty.
- Around line 1238-1241: GetKeyOrigin() returning false when metadata is missing
causes MigrateToDescriptor() to build privkeyids only from keys.origins and drop
private keys for imported solvable scripts; update MigrateToDescriptor() (the
code that collects privkeyids from keys.origins) to also iterate keys.pubkeys,
derive candidate key IDs from those pubkeys, and for each candidate call the
legacy ScriptPubKeyMan methods GetKey() and/or HaveKey() to confirm and collect
the corresponding private key ID before finalizing privkeyids so migrated
descriptor managers include all spendable private keys.
In `@src/wallet/wallet.cpp`:
- Around line 4587-4589: The error message is incorrect when creating a solvable
wallet: locate the branch where the status is compared to
DatabaseStatus::SUCCESS during solvable wallet creation (the block that sets the
variable error and returns false) and change the text from "Error: Failed to
create new watchonly wallet" to "Error: Failed to create new solvable wallet"
(keep the same error variable and return false behavior).
In `@src/wallet/wallet.h`:
- Line 384: m_database is a raw unique_ptr that MigrateToSQLite() resets while
SPK managers can concurrently obtain a raw handle via m_storage.GetDatabase()
holding only cs_KeyStore/cs_desc_man, leading to use-after-free; change
m_database from std::unique_ptr<WalletDatabase> to
std::shared_ptr<WalletDatabase> and update m_storage.GetDatabase() (and all call
sites such as where WalletBatch is constructed in scriptpubkeyman.cpp) to
return/hold a std::shared_ptr so readers take a copy and hold ownership while
MigrateToSQLite() can atomically replace m_database (assign a new shared_ptr
while still acquiring cs_wallet) without freeing the DB under readers —
alternatively, if you prefer explicit locking, add a dedicated mutex (e.g.,
m_database_mutex) and require callers of m_storage.GetDatabase() and
MigrateToSQLite() to lock it when copying/resetting the pointer.
In `@src/wallet/walletdb.cpp`:
- Around line 1151-1187: WalletBatch::EraseRecords currently ignores
m_batch->Erase return values and runs outside a DB transaction; start a DB
transaction (call m_batch->TxnBegin()) before iterating, check the boolean
result of every m_batch->Erase(key_data) and if any Erase returns false
immediately CloseCursor(), call m_batch->TxnAbort(), and return false; on
read/cursor failure also abort the txn; after the loop, call
m_batch->TxnCommit() before CloseCursor() and return true to ensure atomicity
and that the function fails on the first delete error.
In `@src/wallet/walletdb.h`:
- Around line 97-98: walletdb.h declares LEGACY_TYPES as
std::unordered_set<std::string> but doesn't include <unordered_set>, making the
header rely on transitive includes; add an explicit `#include` <unordered_set> to
walletdb.h (near other standard includes) so the type used by LEGACY_TYPES is
defined and the header becomes self-sufficient.
In `@src/wallet/walletutil.h`:
- Around line 108-128: MigrationData currently stores an unencrypted CExtKey in
master_key which leaves xprv material in ordinary memory; change MigrationData
to not retain raw CExtKey (e.g. store the seed/mnemonic in a
SecureString/SecureVector or other secure container) and update consumers, or
ensure the master_key is explicitly and immediately wiped right after
SetupDescriptorScriptPubKeyMans() consumes it (and before ApplyMigrationData()
returns). Specifically, remove or replace CExtKey master_key from the struct or
add logic in the code path that calls
SetupDescriptorScriptPubKeyMans()/ApplyMigrationData() to securely cleanse
master_key (and any ChainCode bytes) as soon as they are no longer needed so
private key material does not remain in regular memory.
In `@test/functional/tool_wallet.py`:
- Line 217: get_expected_info_output() was changed to include imported_privs
into the address-book count, but some callers (notably in test_wipe) still use
the old expected baseline; update those calls to construct expected output with
the imported_privs folded in (e.g., call
get_expected_info_output(imported_privs=1) where appropriate) or adjust the
non-descriptor path assertions in test_wipe to match the new combined count;
specifically edit the remaining get_expected_info_output callers and the
assertions in test_wipe so the expected address-book count includes
imported_privs.
In `@test/functional/wallet_migration.py`:
- Around line 112-115: The line incorrectly assigns to the hdkeypath key instead
of checking it; change the statement using
basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1" to an assertion
that the returned hdkeypath equals the expected value (e.g., assert
basic0.getaddressinfo(new_addr)["hdkeypath"] == "m/44'/1'/0'/0/1") so the test
verifies the new_addr HD path; locate this in the wallet_migration test where
new_addr is obtained from basic0.getnewaddress().
---
Nitpick comments:
In `@src/wallet/test/ismine_tests.cpp`:
- Around line 47-53: Add a watch-only case: after the non-owned check and
before/after the spendable key assertions, call
keystore.GetLegacyScriptPubKeyMan()->AddWatchOnly(scriptPubKey) (or the
appropriate AddWatchOnly method on the LegacyScriptPubKeyMan), then assert that
keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey) equals
ISMINE_WATCH_ONLY and that
keystore.GetLegacyScriptPubKeyMan()->GetScriptPubKeys().count(scriptPubKey)
reflects the watch-only entry (== 1); use the same GetLegacyScriptPubKeyMan(),
GetScriptPubKeys(), AddWatchOnly(), and IsMine() symbols to locate where to
insert these checks.
In `@src/wallet/wallet.cpp`:
- Around line 4402-4403: Replace size() > 0 checks with empty() checks: where
the code tests txids_to_delete.size() > 0 (and the similar occurrence around the
other instance), change it to !txids_to_delete.empty(); this is the idiomatic
and slightly more efficient form—locate the conditional that declares
deleted_txids and any other conditional using txids_to_delete.size() > 0 and
swap to !container.empty().
- Around line 4274-4306: The code currently uses assert() after DB recreation
and writes (when calling MakeDatabase / m_database, batch->TxnBegin(),
batch->Write(...), and batch->TxnCommit()) which will abort the process on
failure; replace these fatal asserts with graceful error handling: log a clear
error via the wallet/db logger including error details, ensure any open
transaction is aborted (batch->TxnAbort()), close and clean up the partially
created DB (m_database->Close() and remove the new file), attempt to restore the
original backup or leave instructions to the caller, and return a failure status
(false) or throw a recoverable exception from the enclosing function instead of
terminating; also update the RPC/help text for the migration call to instruct
users to verify their wallet backup before running migration.
🪄 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: 5941bf3a-8b72-403b-ad36-0eadaef68b5e
📒 Files selected for processing (15)
doc/managing-wallets.mddoc/release-notes-19602.mdsrc/script/descriptor.cppsrc/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/test/ismine_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.hsrc/wallet/walletutil.htest/functional/test_runner.pytest/functional/tool_wallet.pytest/functional/wallet_migration.py
| RPCExamples{ | ||
| HelpExampleCli("migratewallet", "") | ||
| + HelpExampleRpc("migratewallet", "") |
There was a problem hiding this comment.
Fix the RPC examples; they currently show a call that errors.
These examples omit both the wallet endpoint and wallet_name, but the handler rejects that combination on Lines 1141-1142. help migratewallet should show a form that actually succeeds, e.g. by passing wallet_name explicitly.
💡 Suggested fix
RPCExamples{
- HelpExampleCli("migratewallet", "")
- + HelpExampleRpc("migratewallet", "")
+ HelpExampleCli("migratewallet", "\"wallet_name\"")
+ + HelpExampleRpc("migratewallet", "\"wallet_name\"")
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/rpc/wallet.cpp` around lines 1129 - 1131, The RPC help examples
currently show calling migratewallet with no wallet specified, which the
migratewallet handler rejects; update the RPCExamples block (the HelpExampleCli
and HelpExampleRpc calls near the migratewallet RPC definition) to pass an
explicit wallet name (and/or use the wallet endpoint) so the example actually
succeeds — e.g. change HelpExampleCli("migratewallet", "") and
HelpExampleRpc("migratewallet", "") to include a wallet parameter like
HelpExampleCli("migratewallet", "\"mywallet\"") and
HelpExampleRpc("migratewallet", "\"mywallet\"") or alternatively show using
-rpcwallet=mywallet; ensure you edit the RPCExamples surrounding the
migratewallet RPC to reflect this.
| if (it == mapKeyMetadata.end()) { | ||
| return false; | ||
| } | ||
| meta = it->second; |
There was a problem hiding this comment.
Don't make private-key migration depend on key-origin metadata.
GetKeyOrigin() now returns false when metadata is absent, but MigrateToDescriptor() later builds privkeyids from keys.origins only. Imported multisig/solvable scripts with spendable keys but no origin metadata will therefore migrate into descriptor managers with no private keys, and signing will fail after migration. Please collect candidate IDs from keys.pubkeys too and then probe GetKey()/HaveKey() against the legacy SPKM.
🔐 Suggested fix
- std::set<CKeyID> privkeyids;
- for (const auto& key_orig_pair : keys.origins) {
- privkeyids.insert(key_orig_pair.first);
- }
+ std::set<CKeyID> privkeyids;
+ for (const auto& [keyid, _] : keys.origins) {
+ privkeyids.insert(keyid);
+ }
+ for (const auto& [keyid, _] : keys.pubkeys) {
+ if (HaveKey(keyid)) {
+ privkeyids.insert(keyid);
+ }
+ }Also applies to: 2059-2085
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/scriptpubkeyman.cpp` around lines 1238 - 1241, GetKeyOrigin()
returning false when metadata is missing causes MigrateToDescriptor() to build
privkeyids only from keys.origins and drop private keys for imported solvable
scripts; update MigrateToDescriptor() (the code that collects privkeyids from
keys.origins) to also iterate keys.pubkeys, derive candidate key IDs from those
pubkeys, and for each candidate call the legacy ScriptPubKeyMan methods GetKey()
and/or HaveKey() to confirm and collect the corresponding private key ID before
finalizing privkeyids so migrated descriptor managers include all spendable
private keys.
| void DescriptorScriptPubKeyMan::AddDescriptorKey(const CKey& key, const CPubKey &pubkey, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) | ||
| { | ||
| LOCK(cs_desc_man); | ||
| WalletBatch batch(m_storage.GetDatabase()); | ||
| if (!AddDescriptorKeyWithDB(batch, key, pubkey, "", "")) { | ||
| if (!AddDescriptorKeyWithDB(batch, key, pubkey, mnemonic, mnemonic_passphrase)) { |
There was a problem hiding this comment.
Don't persist empty mnemonics for plain imported keys.
The new defaulted mnemonic arguments now flow through even when the caller has no mnemonic. AddDescriptorKeyWithDB() treats presence in m_mnemonics / m_crypted_mnemonics as “this key has mnemonic material”, so single-key imported descriptors start returning success with empty strings from GetMnemonicString(), and multi-key descriptors hit the "multiple mnemonics" path. Only record mnemonic state when at least one of the strings is non-empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/scriptpubkeyman.cpp` around lines 2416 - 2420, The
AddDescriptorKey flow is persisting empty defaulted mnemonic strings causing
imported single-key descriptors to appear as having mnemonics; update the logic
so mnemonic material is only recorded when at least one of the strings is
non-empty: either change DescriptorScriptPubKeyMan::AddDescriptorKey to only
pass mnemonic/mnemonic_passphrase into AddDescriptorKeyWithDB when one of them
is non-empty, or update AddDescriptorKeyWithDB to check m_mnemonics /
m_crypted_mnemonics before inserting and skip storing empty strings; reference
functions/fields: DescriptorScriptPubKeyMan::AddDescriptorKey,
AddDescriptorKeyWithDB, m_mnemonics, m_crypted_mnemonics, and GetMnemonicString
to ensure callers observe absence of mnemonics when both inputs are empty.
| if (status != DatabaseStatus::SUCCESS) { | ||
| error = _("Error: Failed to create new watchonly wallet"); | ||
| return false; |
There was a problem hiding this comment.
Incorrect error message for solvable wallet creation failure.
The error message incorrectly states "Failed to create new watchonly wallet" when the solvable wallet creation fails.
Proposed fix
if (status != DatabaseStatus::SUCCESS) {
- error = _("Error: Failed to create new watchonly wallet");
+ error = _("Error: Failed to create new solvable wallet");
return false;
}📝 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.
| if (status != DatabaseStatus::SUCCESS) { | |
| error = _("Error: Failed to create new watchonly wallet"); | |
| return false; | |
| if (status != DatabaseStatus::SUCCESS) { | |
| error = _("Error: Failed to create new solvable wallet"); | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 4587 - 4589, The error message is
incorrect when creating a solvable wallet: locate the branch where the status is
compared to DatabaseStatus::SUCCESS during solvable wallet creation (the block
that sets the variable error and returns false) and change the text from "Error:
Failed to create new watchonly wallet" to "Error: Failed to create new solvable
wallet" (keep the same error variable and return false behavior).
|
|
||
| /** Internal database handle. */ | ||
| std::unique_ptr<WalletDatabase> const m_database; | ||
| std::unique_ptr<WalletDatabase> m_database; |
There was a problem hiding this comment.
Guard m_database swaps against concurrent SPKM database access.
Making m_database mutable is required for MigrateToSQLite(), but the wallet's SPK managers create WalletBatch instances through m_storage.GetDatabase() while holding only cs_KeyStore / cs_desc_man, not cs_wallet (see src/wallet/scriptpubkeyman.cpp). If migration runs on a live wallet, m_database.reset() can race with those readers and free the old database handle underneath them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.h` at line 384, m_database is a raw unique_ptr that
MigrateToSQLite() resets while SPK managers can concurrently obtain a raw handle
via m_storage.GetDatabase() holding only cs_KeyStore/cs_desc_man, leading to
use-after-free; change m_database from std::unique_ptr<WalletDatabase> to
std::shared_ptr<WalletDatabase> and update m_storage.GetDatabase() (and all call
sites such as where WalletBatch is constructed in scriptpubkeyman.cpp) to
return/hold a std::shared_ptr so readers take a copy and hold ownership while
MigrateToSQLite() can atomically replace m_database (assign a new shared_ptr
while still acquiring cs_wallet) without freeing the DB under readers —
alternatively, if you prefer explicit locking, add a dedicated mutex (e.g.,
m_database_mutex) and require callers of m_storage.GetDatabase() and
MigrateToSQLite() to lock it when copying/resetting the pointer.
| bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types) | ||
| { | ||
| // Get cursor | ||
| if (!m_batch->StartCursor()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Iterate the DB and look for any records that have the type prefixes | ||
| while (true) | ||
| { | ||
| // Read next record | ||
| CDataStream key(SER_DISK, CLIENT_VERSION); | ||
| CDataStream value(SER_DISK, CLIENT_VERSION); | ||
| bool complete; | ||
| bool ret = m_batch->ReadAtCursor(key, value, complete); | ||
| if (complete) { | ||
| break; | ||
| } | ||
| else if (!ret) | ||
| { | ||
| m_batch->CloseCursor(); | ||
| return false; | ||
| } | ||
|
|
||
| // Make a copy of key to avoid data being deleted by the following read of the type | ||
| Span<const unsigned char> key_data = MakeUCharSpan(key); | ||
|
|
||
| std::string type; | ||
| key >> type; | ||
|
|
||
| if (types.count(type) > 0) { | ||
| m_batch->Erase(key_data); | ||
| } | ||
| } | ||
| m_batch->CloseCursor(); | ||
| return true; |
There was a problem hiding this comment.
Make legacy-record cleanup atomic and fail on the first delete error.
Line 1183 ignores the return value from Erase(), so a single failed delete still reports success. Because the loop is also not wrapped in TxnBegin/TxnAbort, any later cursor/read failure can leave only part of LEGACY_TYPES removed. In this migration flow, LegacyScriptPubKeyMan::DeleteRecords() uses this return value as the success signal before the legacy manager is dropped from memory.
💡 Suggested fix
bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
{
+ if (!TxnBegin()) {
+ return false;
+ }
+
// Get cursor
if (!m_batch->StartCursor())
{
+ TxnAbort();
return false;
}
// Iterate the DB and look for any records that have the type prefixes
while (true)
@@
if (complete) {
break;
}
else if (!ret)
{
m_batch->CloseCursor();
+ TxnAbort();
return false;
}
@@
std::string type;
key >> type;
if (types.count(type) > 0) {
- m_batch->Erase(key_data);
+ if (!m_batch->Erase(key_data)) {
+ m_batch->CloseCursor();
+ TxnAbort();
+ return false;
+ }
}
}
m_batch->CloseCursor();
- return true;
+ return TxnCommit();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/walletdb.cpp` around lines 1151 - 1187, WalletBatch::EraseRecords
currently ignores m_batch->Erase return values and runs outside a DB
transaction; start a DB transaction (call m_batch->TxnBegin()) before iterating,
check the boolean result of every m_batch->Erase(key_data) and if any Erase
returns false immediately CloseCursor(), call m_batch->TxnAbort(), and return
false; on read/cursor failure also abort the txn; after the loop, call
m_batch->TxnCommit() before CloseCursor() and return true to ensure atomicity
and that the function fails on the first delete error.
| // Keys in this set pertain only to the legacy wallet (LegacyScriptPubKeyMan) and are removed during migration from legacy to descriptors. | ||
| extern const std::unordered_set<std::string> LEGACY_TYPES; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/wallet/walletdb.h | head -120Repository: dashpay/dash
Length of output: 4438
🏁 Script executed:
# Check for unordered_set includes in the directly included headers
for file in "src/wallet/db.h" "src/wallet/walletutil.h" "src/script/sign.h" "src/key.h"; do
echo "=== Checking $file ==="
grep -n "unordered_set" "$file" || echo "No unordered_set found"
doneRepository: dashpay/dash
Length of output: 283
🏁 Script executed:
# Check the first few includes in wallet/db.h to understand the transitive chain
head -50 src/wallet/db.h | grep -E "^#include"Repository: dashpay/dash
Length of output: 232
🏁 Script executed:
# Search for usage of LEGACY_TYPES to see where it's actually used
rg "LEGACY_TYPES" --type cpp --type hRepository: dashpay/dash
Length of output: 432
🏁 Script executed:
# Check if walletdb.cpp includes <unordered_set>
echo "=== walletdb.cpp includes ===" && head -30 src/wallet/walletdb.cpp | grep "include"
echo ""
echo "=== scriptpubkeyman.cpp includes ===" && head -30 src/wallet/scriptpubkeyman.cpp | grep "include"Repository: dashpay/dash
Length of output: 889
🏁 Script executed:
# Check if walletdb.cpp defines the set (need full view of includes)
head -50 src/wallet/walletdb.cpp
echo ""
echo "=== scriptpubkeyman.cpp full header check ===" && head -50 src/wallet/scriptpubkeyman.cpp | grep "include"Repository: dashpay/dash
Length of output: 1975
🏁 Script executed:
# Check if serialize.h (which is in walletdb.cpp) includes unordered_set
head -60 src/serialize.h | grep -E "unordered_set|#include"Repository: dashpay/dash
Length of output: 501
🏁 Script executed:
# Check what streams.h includes - it may be providing the transitive include
head -40 src/streams.h | grep "include"Repository: dashpay/dash
Length of output: 376
🏁 Script executed:
# Verify the transitive include chain
echo "=== wallet/db.h includes ===" && grep "^#include" src/wallet/db.h | head -20Repository: dashpay/dash
Length of output: 261
🏁 Script executed:
# Verify the full transitive include chain
echo "=== Does walletdb.h directly include serialize.h or streams.h? ===" && grep -E "serialize|streams" src/wallet/walletdb.h
echo ""
echo "=== Confirmed chain: walletdb.h includes wallet/db.h ===" && grep "wallet/db.h" src/wallet/walletdb.h
echo "wallet/db.h includes streams.h:" && grep "streams.h" src/wallet/db.h
echo "streams.h includes serialize.h:" && grep "serialize.h" src/streams.h
echo "serialize.h includes unordered_set:" && grep "unordered_set" src/serialize.hRepository: dashpay/dash
Length of output: 957
Add explicit #include <unordered_set> to ensure header self-sufficiency.
Line 98 declares std::unordered_set<std::string> LEGACY_TYPES; in the public interface, but walletdb.h does not directly include <unordered_set>. The header currently depends on a transitive include chain (wallet/db.h → streams.h → serialize.h → <unordered_set>), which is fragile and violates the principle that headers should be self-contained for the types they expose.
💡 Suggested fix
`#include` <stdint.h>
+#include <unordered_set>
`#include` <string>
`#include` <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/walletdb.h` around lines 97 - 98, walletdb.h declares LEGACY_TYPES
as std::unordered_set<std::string> but doesn't include <unordered_set>, making
the header rely on transitive includes; add an explicit `#include` <unordered_set>
to walletdb.h (near other standard includes) so the type used by LEGACY_TYPES is
defined and the header becomes self-sufficient.
| /** struct containing information needed for migrating legacy wallets to descriptor wallets */ | ||
| struct MigrationData | ||
| { | ||
| CExtKey master_key; | ||
| SecureString mnemonic; | ||
| SecureString mnemonic_passphrase; | ||
| // Highest BIP44 index the legacy wallet ever derived per chain (external/0, | ||
| // internal/1). The active pkh() descriptors created during migration must | ||
| // start handing out the next address from this index, otherwise post- | ||
| // migration getnewaddress would re-derive addresses the legacy wallet had | ||
| // already given out. Carried via MigrationData purely to advance the active | ||
| // descriptors' next_index — the inactive combo descriptors built in | ||
| // MigrateToDescriptor already cover the history themselves. | ||
| uint32_t external_chain_counter{0}; | ||
| uint32_t internal_chain_counter{0}; | ||
| std::vector<std::pair<std::string, int64_t>> watch_descs; | ||
| std::vector<std::pair<std::string, int64_t>> solvable_descs; | ||
| std::vector<std::unique_ptr<DescriptorScriptPubKeyMan>> desc_spkms; | ||
| std::shared_ptr<CWallet> watchonly_wallet{nullptr}; | ||
| std::shared_ptr<CWallet> solvable_wallet{nullptr}; | ||
| }; |
There was a problem hiding this comment.
Don't keep the master xprv in ordinary memory for the whole migration.
MigrationData now owns an unencrypted CExtKey. Unlike SecureString, CExtKey/ChainCode are not wiped on destruction (src/key.h), and CWallet::ApplyMigrationData() just consumes this object and returns. That leaves master seed material resident in regular memory longer than needed. Store a secure seed representation here, or add an explicit wipe immediately after SetupDescriptorScriptPubKeyMans() uses it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/walletutil.h` around lines 108 - 128, MigrationData currently
stores an unencrypted CExtKey in master_key which leaves xprv material in
ordinary memory; change MigrationData to not retain raw CExtKey (e.g. store the
seed/mnemonic in a SecureString/SecureVector or other secure container) and
update consumers, or ensure the master_key is explicitly and immediately wiped
right after SetupDescriptorScriptPubKeyMans() consumes it (and before
ApplyMigrationData() returns). Specifically, remove or replace CExtKey
master_key from the struct or add logic in the code path that calls
SetupDescriptorScriptPubKeyMans()/ApplyMigrationData() to securely cleanse
master_key (and any ChainCode bytes) as soon as they are no longer needed so
private key material does not remain in regular memory.
…lets 53e7ed0 doc: Release notes and other docs for migration (Andrew Chow) 9c44bfe Test migratewallet (Andrew Chow) 0b26e7c descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow) 31764c3 Add migratewallet RPC (Andrew Chow) 0bf7b38 Implement MigrateLegacyToDescriptor (Andrew Chow) e7b16f9 Implement MigrateToSQLite (Andrew Chow) 5b62f09 wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow) 22401f1 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow) 35f428f Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow) ea1ab39 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow) e664af2 Apply label to all scriptPubKeys of imported combo() (Andrew Chow) Pull request description: This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s. For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor. There are also tests. ACKs for top commit: Sjors: tACK 53e7ed0 w0xlt: reACK bitcoin@53e7ed0 Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
…data exist 6d31900 wallet: migrate wallet, exit early if no legacy data exist (furszy) Pull request description: The process first creates a backup file then return an error, without removing the recently created file, when notices that the db is already running sqlite. ACKs for top commit: john-moffett: ACK 6d31900 achow101: ACK 6d31900 ishaanam: crACK 6d31900 Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
…atchonly/solvable wallets 730e14a test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) d5f4ae7 wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: Currently `migratewallet` migrates the address book (i.e. labels and purposes) for watchonly and solvable wallets only in RAM, but doesn't persist them on disk. Fix this by adding another loop for both of the special wallet types after which writes the corresponding NAME and PURPOSE entries to the database in a single batch. Also adds a corresponding test that checks if labels were migrated correctly for a watchonly wallet. ACKs for top commit: achow101: ACK 730e14a furszy: code ACK 730e14a, left a non-blocking nit. aureleoules: ACK 730e14a Tree-SHA512: 159487e11e858924ef762e0190ccaea185bdff239e3d2280c8d63c4ac2649ec71714dc4d53dec644f03488f91c3b4bbbbf3434dad23bc0fcecb6657f353ea766
…e cleanup 5e65a21 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) 88afc73 tests: Test for migrating encrypted wallets (Andrew Chow) 86ef7b3 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault. This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached. This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue. ACKs for top commit: S3RK: reACK 5e65a21 stickies-v: ACK [5e65a21](bitcoin@5e65a21) furszy: diff ACK 5e65a21 Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
…sphrase to migratewallet 9486509 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) aaf02b5 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) 7fd125b wallet: Be able to unlock the wallet for migration (Andrew Chow) 6bdbc5f rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) dbfa345 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) Pull request description: `migratewallet` currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate. Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to `migratewallet` in order to migrate encrypted wallets. Fixes bitcoin#27048 ACKs for top commit: john-moffett: reACK 9486509 pinheadmz: ACK 9486509 furszy: ACK 9486509 Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4
…uccessfully restored 769f5b1 test: check backup from `migratewallet` can be successfully restored (brunoerg) Pull request description: `migratewallet` migrates the wallet to a descriptor one. During the process, it generates a backup file of the wallet in case of an incorrect migration. This PR adds test to check if the backup file can be successfully restored. ACKs for top commit: achow101: ACK 769f5b1 MarcoFalke: lgtm ACK 769f5b1 Tree-SHA512: 94c50b34fbd47c4d3cc34b94e9e7903bc233608c7f50f45c161669996fd5f5b7d8f9a4e6a3437b9151d66a76af833f3f1ca28e44ecb63b5a8f391f6d6be0e39f
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b60e277f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Make a backup of the DB | ||
| fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path(); | ||
| fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", wallet_name, GetTime())); |
There was a problem hiding this comment.
The backup name is built directly from wallet_name, which may contain path separators (e.g. nested wallet names or absolute paths passed to migratewallet). Because fs::path treats those separators as path components, backup_path can resolve outside the wallet directory or into non-existent subdirectories, causing BackupWallet to fail or placing the backup in an unexpected location. This makes migration unreliable for path-based wallet names.
Useful? React with 👍 / 👎.
knst
force-pushed
the
bp-descriptor-migrate
branch
from
67b60e2 to
4f25d93
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f25d93da5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| opts.require_create = true; | ||
| opts.require_format = DatabaseFormat::SQLITE; | ||
| DatabaseStatus db_status; | ||
| std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error); |
There was a problem hiding this comment.
This creates the new SQLite database from db_dir (the parent directory of the original file) instead of the original wallet path, so legacy wallets opened as files (for example -wallet=foo.dat in walletdir) are migrated to walletdir/wallet.dat rather than back to foo.dat. In that case migration can target the wrong wallet file and the subsequent reopen by the original name will fail, which can leave the migrated wallet inaccessible under the requested wallet name.
Useful? React with 👍 / 👎.
| LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); | ||
| res.wallet_name = wallet_name; |
There was a problem hiding this comment.
The success path unconditionally reports success after calling LoadWallet without checking whether the reload actually succeeded. If reload fails (for example due to path/format/open errors), migratewallet still returns a successful result even though the wallet is not loaded, hiding a failed finalization step from users and automation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/tool_wallet.py (1)
67-82:⚠️ Potential issue | 🟠 MajorDescriptor Address Book expectation appears overcounted.
The descriptor-path formula uses
imported_privs * 2, but wallet info printsm_address_book.size(), and imported private keys are added with oneSetAddressBook(...)call per key. This likely makes expected output too high in descriptor mode.Proposed fix
- ''' % (wallet_name, keypool * output_types, transactions, imported_privs * 2 + address)) + ''' % (wallet_name, keypool * output_types, transactions, imported_privs + address))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/tool_wallet.py` around lines 67 - 82, The expected address book count in get_expected_info_output is overcounting imported private keys in descriptor mode; update the final value in the multi-line return from using "imported_privs * 2 + address" to "imported_privs + address" so the formatted tuple passed to the dedented string uses the actual number of SetAddressBook calls (one per imported key) instead of doubling them; verify this change in the get_expected_info_output method and keep the rest of the descriptor-mode calculations (like keypool * output_types) unchanged.
♻️ Duplicate comments (7)
src/wallet/walletdb.h (1)
97-98:⚠️ Potential issue | 🟡 MinorAdd the missing
<unordered_set>include.
src/wallet/walletdb.hnow exposesstd::unordered_setin its public interface, but it still doesn't include<unordered_set>directly. That keeps the header dependent on transitive includes and fragile to future include cleanup.#!/bin/bash sed -n '1,20p' src/wallet/walletdb.h printf '\n--- std::unordered_set uses in src/wallet/walletdb.h ---\n' rg -n 'unordered_set|LEGACY_TYPES|EraseRecords' src/wallet/walletdb.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/walletdb.h` around lines 97 - 98, The header exposes std::unordered_set (symbol LEGACY_TYPES) but doesn't include <unordered_set>, creating a fragile transitive-include dependency; add a direct include for <unordered_set> at the top of src/wallet/walletdb.h (near the other standard includes) so the declaration of LEGACY_TYPES and any uses of std::unordered_set in this header compile reliably without depending on indirect includes.src/wallet/scriptpubkeyman.cpp (2)
2059-2085:⚠️ Potential issue | 🔴 CriticalStill dropping spendable imported keys when origin metadata is missing.
privkeyidsis still built only fromkeys.origins. Imported solvable scripts can have entries inkeys.pubkeyswithout origin metadata, so their private keys never get copied into the descriptor manager and signing breaks after migration.🔐 Suggested fix
std::set<CKeyID> privkeyids; - for (const auto& key_orig_pair : keys.origins) { - privkeyids.insert(key_orig_pair.first); + for (const auto& [keyid, _] : keys.origins) { + privkeyids.insert(keyid); + } + for (const auto& [keyid, _] : keys.pubkeys) { + if (HaveKey(keyid)) { + privkeyids.insert(keyid); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/scriptpubkeyman.cpp` around lines 2059 - 2085, privkeyids is built only from keys.origins so imported solvable scripts that only have entries in keys.pubkeys (no origin metadata) are skipped; update the collection logic (where privkeyids is filled) to also include key IDs from keys.pubkeys (in addition to keys.origins) so those private keys are looked up via GetKey and passed into DescriptorScriptPubKeyMan::AddDescriptorKey to preserve spendable imported keys after migration.
2462-2468:⚠️ Potential issue | 🟠 MajorDon't persist empty mnemonic state for plain imported descriptors.
This still inserts
m_mnemonics/m_crypted_mnemonicsand writes empty mnemonic fields even when both strings are empty. That makes plain imported keys look like they have mnemonic material and breaksGetMnemonicString()and multi-key descriptor handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/scriptpubkeyman.cpp` around lines 2462 - 2468, When storing imported descriptor keys, avoid persisting empty mnemonic state: modify the branches around m_crypted_mnemonics/m_mnemonics and the calls to WriteCryptedDescriptorKey/WriteDescriptorKey so that you only insert into m_crypted_mnemonics or m_mnemonics and call batch.WriteCryptedDescriptorKey or batch.WriteDescriptorKey when at least one of the mnemonic or mnemonic_passphrase strings is non-empty; otherwise only insert the key into m_map_crypted_keys/m_map_keys and call the batch write variant that does not include mnemonic data (or omit the mnemonic args), ensuring GetMnemonicString() and multi-key descriptor logic won't see empty mnemonic entries.src/wallet/rpc/wallet.cpp (1)
1129-1131:⚠️ Potential issue | 🟡 MinorFix the help examples; they currently invoke the error path.
These examples omit both the wallet endpoint and
wallet_name, but Lines 1141-1142 reject that call.💡 Suggested fix
RPCExamples{ - HelpExampleCli("migratewallet", "") - + HelpExampleRpc("migratewallet", "") + HelpExampleCli("migratewallet", "\"mywallet\"") + + HelpExampleRpc("migratewallet", "\"mywallet\"") },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/rpc/wallet.cpp` around lines 1129 - 1131, The RPC help examples for migratewallet (in the RPCExamples block using HelpExampleCli and HelpExampleRpc) currently omit the required wallet endpoint and wallet_name and therefore demonstrate the error path; update these HelpExampleCli/HelpExampleRpc examples to include a valid wallet endpoint and a sample wallet_name that conforms to the validation used by migratewallet so the examples show a successful call (adjust the RPCExamples strings for migratewallet accordingly).src/wallet/walletutil.h (1)
108-128:⚠️ Potential issue | 🟠 MajorDon't keep the master xprv in ordinary memory across migration.
MigrationData::master_keykeeps seed-derived private material in a plainCExtKey, so it survives until the whole migration object is destroyed. Keep only secure seed material here, or explicitly cleanse it immediately after descriptor setup finishes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/walletutil.h` around lines 108 - 128, MigrationData currently stores private key material in the plain CExtKey member master_key; change this so no seed-derived private material lives in ordinary memory for the lifetime of the struct: remove (or stop using) MigrationData::master_key and instead derive the CExtKey from the existing SecureString mnemonic locally inside the routine that needs it (e.g., inside MigrateToDescriptor), use it only briefly, then immediately cleanse it; if you must keep master_key as a field, implement immediate secure wiping (e.g., call a Clear/Zero method and overwrite the memory) in MigrationData's cleanup path (destructor or right after descriptor setup) so the raw private material is not retained.src/wallet/wallet.cpp (1)
4587-4589:⚠️ Potential issue | 🟡 MinorFix the solvables-wallet creation error text.
This branch is creating
_solvables, but the message still says"watchonly", which makes failures harder to diagnose.Proposed fix
if (status != DatabaseStatus::SUCCESS) { - error = _("Error: Failed to create new watchonly wallet"); + error = _("Error: Failed to create new solvable wallet"); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 4587 - 4589, The error message incorrectly refers to "watchonly" while this branch is creating _solvables; update the assignment to error (the variable named error) inside the failure branch where status != DatabaseStatus::SUCCESS (near the code that creates _solvables) to use a correct message such as "Error: Failed to create new solvables wallet" or otherwise mention "_solvables" so failures accurately reflect the operation that failed.src/wallet/wallet.h (1)
384-384:⚠️ Potential issue | 🔴 CriticalKeep database handles alive across
m_databaseswaps.
MigrateToSQLite()now replacesm_database, butGetDatabase()still exposes an unowned reference. Any in-flight DB user that outlives the swap can end up dereferencing freed storage. This needs either shared ownership for the database handle or a dedicated mutex guarding every copy/reset of the handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.h` at line 384, m_database is a std::unique_ptr but MigrateToSQLite() swaps it out while GetDatabase() returns an unowned pointer, risking use-after-free for in-flight users; change m_database to std::shared_ptr<WalletDatabase> (and update its declaration), make GetDatabase() return std::shared_ptr<WalletDatabase> instead of a raw/unowned pointer, and update MigrateToSQLite() and any reset/assignment code to replace the shared_ptr atomically (or under the existing mutex) so callers hold shared ownership while using the DB. Ensure all places that constructed or moved the unique_ptr now construct/assign shared_ptrs and that consumers stop assuming the DB is unowned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/scriptpubkeyman.cpp`:
- Around line 2093-2098: The loop erases elements from spks using del_it and
assigns it = spks.erase(del_it), which can advance the outer loop iterator
unpredictably when del_it ≠ it and thus skip elements; instead, avoid
reassigning the outer iterator inside the inner loop—either call
spks.erase(del_it) (discarding the returned iterator) or collect desc_spks
to_remove and perform spks.erase for each after the outer loop, keeping the
outer loop's iterator variable it unchanged; update the code paths around the
loops that reference spks, desc_spks, it, and del_it to use this approach and
retain the existing asserts (IsMine and find checks).
In `@src/wallet/wallet.cpp`:
- Around line 4035-4041: The current thrown runtime_error in the
non-external-signer branch leaks the full SecureString mnemonic by concatenating
it into the message (see the block guarded by
IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) and the use of CMnemonic::Check and
mnemonic/ mnemonic_arg); replace that detailed message with a generic error
string (e.g. std::runtime_error(std::string(__func__) + ": invalid mnemonic"))
or a specific non-secret error code, and ensure no code logs or includes the
mnemonic variable (SecureString mnemonic) in any exception, log, or RPC response
paths.
- Around line 4462-4488: The code currently exits the loop after cloning labels
into data.watchonly_wallet because of the first continue, so when both
data.watchonly_wallet and data.solvable_wallet exist the solvable wallet never
receives the shared address-book entries; remove the mutual-exclusion by
deleting the continue after the watchonly block (or refactor so both conditional
blocks run) so that the same cloning logic for addr_pair (using
addr_pair.second.GetLabel(), .purpose, .IsChange(), and writing into
m_address_book) executes for both data.watchonly_wallet and
data.solvable_wallet.
- Around line 4281-4305: Replace the fatal assert-based aborts in the SQLite
migration path with proper error returns so the caller
(MigrateToSQLite/MigrateLegacyToDescriptor) can run the recovery path:
specifically, in the block that creates the new DB and writes records replace
assert(new_db) with a check that sets an informative error message and returns
false on failure; replace the assert(began) after batch->TxnBegin() with an if
(!began) that aborts the txn if needed, closes/removes the new DB file, sets
error and returns false; inside the record write loop, instead of assert(false)
when batch->Write(...) fails, call batch->TxnAbort(), close/remove the DB, set
error and return false; likewise replace assert(committed) after TxnCommit()
with an if (!committed) that cleans up (TxnAbort if possible), closes/removes
the DB, sets error and returns false. Reference symbols: MakeDatabase,
m_database, batch, TxnBegin, Write, TxnAbort, TxnCommit, m_database->Close(),
m_database->Filename().
- Around line 4690-4695: After resetting local_wallet and calling
LoadWallet(context, wallet_name, ...), check LoadWallet's result (its return
value or the populated status/error) and treat a failure as an overall migration
failure: do not leave res.wallet_name set, propagate or set the error/status so
migratewallet returns failure, and ensure success is updated accordingly;
specifically, after LoadWallet(...) evaluate its outcome, if it failed revert
success to false, clear res.wallet_name, and propagate the error/status back to
the caller instead of returning success.
In `@test/functional/wallet_migration.py`:
- Around line 114-115: The test asserts reuse of addresses but compares
new_change to addr instead of the legacy change address; update the assertion in
test/functional/wallet_migration.py to assert that new_change is not equal to
change (i.e., replace the comparison of new_change == addr with new_change ==
change) so the test verifies the new change address differs from the legacy
change address; ensure you reference the variables new_addr, addr, new_change,
and change in the modified assertion.
---
Outside diff comments:
In `@test/functional/tool_wallet.py`:
- Around line 67-82: The expected address book count in get_expected_info_output
is overcounting imported private keys in descriptor mode; update the final value
in the multi-line return from using "imported_privs * 2 + address" to
"imported_privs + address" so the formatted tuple passed to the dedented string
uses the actual number of SetAddressBook calls (one per imported key) instead of
doubling them; verify this change in the get_expected_info_output method and
keep the rest of the descriptor-mode calculations (like keypool * output_types)
unchanged.
---
Duplicate comments:
In `@src/wallet/rpc/wallet.cpp`:
- Around line 1129-1131: The RPC help examples for migratewallet (in the
RPCExamples block using HelpExampleCli and HelpExampleRpc) currently omit the
required wallet endpoint and wallet_name and therefore demonstrate the error
path; update these HelpExampleCli/HelpExampleRpc examples to include a valid
wallet endpoint and a sample wallet_name that conforms to the validation used by
migratewallet so the examples show a successful call (adjust the RPCExamples
strings for migratewallet accordingly).
In `@src/wallet/scriptpubkeyman.cpp`:
- Around line 2059-2085: privkeyids is built only from keys.origins so imported
solvable scripts that only have entries in keys.pubkeys (no origin metadata) are
skipped; update the collection logic (where privkeyids is filled) to also
include key IDs from keys.pubkeys (in addition to keys.origins) so those private
keys are looked up via GetKey and passed into
DescriptorScriptPubKeyMan::AddDescriptorKey to preserve spendable imported keys
after migration.
- Around line 2462-2468: When storing imported descriptor keys, avoid persisting
empty mnemonic state: modify the branches around m_crypted_mnemonics/m_mnemonics
and the calls to WriteCryptedDescriptorKey/WriteDescriptorKey so that you only
insert into m_crypted_mnemonics or m_mnemonics and call
batch.WriteCryptedDescriptorKey or batch.WriteDescriptorKey when at least one of
the mnemonic or mnemonic_passphrase strings is non-empty; otherwise only insert
the key into m_map_crypted_keys/m_map_keys and call the batch write variant that
does not include mnemonic data (or omit the mnemonic args), ensuring
GetMnemonicString() and multi-key descriptor logic won't see empty mnemonic
entries.
In `@src/wallet/wallet.cpp`:
- Around line 4587-4589: The error message incorrectly refers to "watchonly"
while this branch is creating _solvables; update the assignment to error (the
variable named error) inside the failure branch where status !=
DatabaseStatus::SUCCESS (near the code that creates _solvables) to use a correct
message such as "Error: Failed to create new solvables wallet" or otherwise
mention "_solvables" so failures accurately reflect the operation that failed.
In `@src/wallet/wallet.h`:
- Line 384: m_database is a std::unique_ptr but MigrateToSQLite() swaps it out
while GetDatabase() returns an unowned pointer, risking use-after-free for
in-flight users; change m_database to std::shared_ptr<WalletDatabase> (and
update its declaration), make GetDatabase() return
std::shared_ptr<WalletDatabase> instead of a raw/unowned pointer, and update
MigrateToSQLite() and any reset/assignment code to replace the shared_ptr
atomically (or under the existing mutex) so callers hold shared ownership while
using the DB. Ensure all places that constructed or moved the unique_ptr now
construct/assign shared_ptrs and that consumers stop assuming the DB is unowned.
In `@src/wallet/walletdb.h`:
- Around line 97-98: The header exposes std::unordered_set (symbol LEGACY_TYPES)
but doesn't include <unordered_set>, creating a fragile transitive-include
dependency; add a direct include for <unordered_set> at the top of
src/wallet/walletdb.h (near the other standard includes) so the declaration of
LEGACY_TYPES and any uses of std::unordered_set in this header compile reliably
without depending on indirect includes.
In `@src/wallet/walletutil.h`:
- Around line 108-128: MigrationData currently stores private key material in
the plain CExtKey member master_key; change this so no seed-derived private
material lives in ordinary memory for the lifetime of the struct: remove (or
stop using) MigrationData::master_key and instead derive the CExtKey from the
existing SecureString mnemonic locally inside the routine that needs it (e.g.,
inside MigrateToDescriptor), use it only briefly, then immediately cleanse it;
if you must keep master_key as a field, implement immediate secure wiping (e.g.,
call a Clear/Zero method and overwrite the memory) in MigrationData's cleanup
path (destructor or right after descriptor setup) so the raw private material is
not retained.
🪄 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: d63f16ce-0308-4c64-b09b-9c0f003dc36f
📒 Files selected for processing (15)
doc/managing-wallets.mddoc/release-notes-19602.mdsrc/script/descriptor.cppsrc/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/test/ismine_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.hsrc/wallet/walletutil.htest/functional/test_runner.pytest/functional/tool_wallet.pytest/functional/wallet_migration.py
✅ Files skipped from review due to trivial changes (2)
- doc/release-notes-19602.md
- test/functional/test_runner.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/wallet/test/ismine_tests.cpp
- src/wallet/walletdb.cpp
- src/script/descriptor.cpp
| // Remove the scriptPubKeys from our current set | ||
| for (const CScript& desc_spk : desc_spks) { | ||
| auto del_it = spks.find(desc_spk); | ||
| assert(del_it != spks.end()); | ||
| assert(IsMine(desc_spk) != ISMINE_NO); | ||
| it = spks.erase(del_it); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++, for std::unordered_set, if an outer loop is iterating with auto it, and inside an inner loop I repeatedly assign it = set.erase(del_it)wheredel_itmay refer to a different element than the currentit, can this skip remaining elements? Please include the iterator rules for std::unordered_set::erase.
💡 Result:
Yes. Even though it’s valid to do it = set.erase(del_it) (as long as del_it is a valid iterator into the same container), it can absolutely cause your outer iteration to skip elements.
Reason: unordered_set::erase(pos) returns “the iterator following the last removed element” (for the single-iterator overload: the element after the erased one in iteration order). So if del_it is not the same as the current it, you are effectively “teleporting” it to after del_it, and you may never visit whatever elements were between the old it and that returned iterator (in the container’s iteration sequence, which is unspecified but still a real sequence). [1]
std::unordered_set::erase iterator rules (key points)
From the standard library rules as summarized on cppreference:
- Invalidation: “References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated.” [1]
- Return value (iterator overloads): “Iterator following the last removed element.” [1]
- Precondition:
posmust be dereferenceable (so you can’t passend()). [1] - Rehash note: Iterators can also be invalidated by a rehash (e.g., due to operations that trigger it); rehash invalidates iterators. [2]
Safe pattern
- If you’re erasing the current element:
it = s.erase(it); - If you’re erasing a different element: do not assign to
it; justs.erase(del_it);(unlessdel_it == it, in which case use the first form).
Sources: cppreference unordered_set::erase [1], iterator invalidation on rehash (e.g., insert/emplace triggering rehash) [2].
[1] (en.cppreference.com)
[2] (bloomberg.github.io)
Citations:
- 1: https://en.cppreference.com/w/cpp/container/unordered_set/erase.html
- 2: https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/group__bslstl__unorderedset.html?utm_source=openai
🏁 Script executed:
cat -n src/wallet/scriptpubkeyman.cpp | sed -n '2080,2150p'Repository: dashpay/dash
Length of output: 3608
🏁 Script executed:
cat -n src/wallet/scriptpubkeyman.cpp | sed -n '2050,2100p'Repository: dashpay/dash
Length of output: 2626
🏁 Script executed:
cat -n src/wallet/scriptpubkeyman.cpp | sed -n '2030,2055p'Repository: dashpay/dash
Length of output: 1316
🏁 Script executed:
cat -n src/wallet/scriptpubkeyman.cpp | sed -n '2041,2101p'Repository: dashpay/dash
Length of output: 3039
Iterator reuse bug skips elements in spks.
Outer loop at line 2041 doesn't increment it; instead relies on it = spks.erase(del_it) at line 2098 to manage iteration. When the inner loop erases multiple unrelated elements, reassigning it to each erase() return value causes the outer loop to skip entries in the container. Since unordered_set::erase(pos) returns the iterator after the erased element, assigning it = spks.erase(del_it) (where del_it may differ from the loop's current position) jumps it forward, bypassing elements between the old and new position in iteration order. The only check is the release-build-stripped assert at line 2141.
Suggested fix
- for (auto it = spks.begin(); it != spks.end();) {
- const CScript& spk = *it;
+ for (auto it = spks.begin(); it != spks.end();) {
+ const CScript spk = *it;
+ ++it;
// Remove the scriptPubKeys from our current set
for (const CScript& desc_spk : desc_spks) {
auto del_it = spks.find(desc_spk);
assert(del_it != spks.end());
assert(IsMine(desc_spk) != ISMINE_NO);
- it = spks.erase(del_it);
+ spks.erase(del_it);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/scriptpubkeyman.cpp` around lines 2093 - 2098, The loop erases
elements from spks using del_it and assigns it = spks.erase(del_it), which can
advance the outer loop iterator unpredictably when del_it ≠ it and thus skip
elements; instead, avoid reassigning the outer iterator inside the inner
loop—either call spks.erase(del_it) (discarding the returned iterator) or
collect desc_spks to_remove and perform spks.erase for each after the outer
loop, keeping the outer loop's iterator variable it unchanged; update the code
paths around the loops that reference spks, desc_spks, it, and del_it to use
this approach and retain the existing asserts (IsMine and find checks).
| if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { | ||
| // Make a seed | ||
| // TODO: remove duplicated code with CHDChain::SetMnemonic | ||
| 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) + "`"); | ||
| } |
There was a problem hiding this comment.
Do not leak the mnemonic in the exception message.
If validation fails here, the thrown error includes the full seed phrase and can propagate to logs or RPC responses. This should be a generic error.
Proposed 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");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 4035 - 4041, The current thrown
runtime_error in the non-external-signer branch leaks the full SecureString
mnemonic by concatenating it into the message (see the block guarded by
IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) and the use of CMnemonic::Check and
mnemonic/ mnemonic_arg); replace that detailed message with a generic error
string (e.g. std::runtime_error(std::string(__func__) + ": invalid mnemonic"))
or a specific non-secret error code, and ensure no code logs or includes the
mnemonic variable (SecureString mnemonic) in any exception, log, or RPC response
paths.
| DatabaseOptions opts; | ||
| opts.require_create = true; | ||
| opts.require_format = DatabaseFormat::SQLITE; | ||
| DatabaseStatus db_status; | ||
| std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error); | ||
| assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists. | ||
| m_database.reset(); | ||
| m_database = std::move(new_db); | ||
|
|
||
| // Write existing records into the new DB | ||
| batch = m_database->MakeBatch(); | ||
| bool began = batch->TxnBegin(); | ||
| assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. | ||
| for (const auto& [key, value] : records) { | ||
| CDataStream ss_key(key, SER_DISK, CLIENT_VERSION); | ||
| CDataStream ss_value(value, SER_DISK, CLIENT_VERSION); | ||
| if (!batch->Write(ss_key, ss_value)) { | ||
| batch->TxnAbort(); | ||
| m_database->Close(); | ||
| fs::remove(m_database->Filename()); | ||
| assert(false); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. | ||
| } | ||
| } | ||
| bool committed = batch->TxnCommit(); | ||
| assert(committed); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. |
There was a problem hiding this comment.
Return an error here instead of aborting the process.
MigrateLegacyToDescriptor() only restores the legacy backup when MigrateToSQLite() returns false. The assert() calls here turn normal filesystem / SQLite failures into process aborts after the BDB file has already been deleted, so the recovery path never runs.
Proposed fix
DatabaseStatus db_status;
std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error);
- assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists.
+ if (!new_db) {
+ error = _("Error: Unable to create the new SQLite database");
+ return false;
+ }
m_database.reset();
m_database = std::move(new_db);
// Write existing records into the new DB
batch = m_database->MakeBatch();
bool began = batch->TxnBegin();
- assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
+ if (!began) {
+ error = _("Error: Unable to begin writing to the new SQLite database");
+ return false;
+ }
for (const auto& [key, value] : records) {
CDataStream ss_key(key, SER_DISK, CLIENT_VERSION);
CDataStream ss_value(value, SER_DISK, CLIENT_VERSION);
if (!batch->Write(ss_key, ss_value)) {
batch->TxnAbort();
m_database->Close();
fs::remove(m_database->Filename());
- assert(false); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
+ error = _("Error: Unable to write all records to the new SQLite database");
+ return false;
}
}
bool committed = batch->TxnCommit();
- assert(committed); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
+ if (!committed) {
+ error = _("Error: Unable to commit the new SQLite database");
+ return false;
+ }
return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 4281 - 4305, Replace the fatal
assert-based aborts in the SQLite migration path with proper error returns so
the caller (MigrateToSQLite/MigrateLegacyToDescriptor) can run the recovery
path: specifically, in the block that creates the new DB and writes records
replace assert(new_db) with a check that sets an informative error message and
returns false on failure; replace the assert(began) after batch->TxnBegin() with
an if (!began) that aborts the txn if needed, closes/removes the new DB file,
sets error and returns false; inside the record write loop, instead of
assert(false) when batch->Write(...) fails, call batch->TxnAbort(), close/remove
the DB, set error and return false; likewise replace assert(committed) after
TxnCommit() with an if (!committed) that cleans up (TxnAbort if possible),
closes/removes the DB, sets error and returns false. Reference symbols:
MakeDatabase, m_database, batch, TxnBegin, Write, TxnAbort, TxnCommit,
m_database->Close(), m_database->Filename().
| // Labels for everything else (send) should be cloned to all | ||
| if (data.watchonly_wallet) { | ||
| LOCK(data.watchonly_wallet->cs_wallet); | ||
| // Add to the watchonly. Preserve the labels, purpose, and change-ness | ||
| std::string label = addr_pair.second.GetLabel(); | ||
| std::string purpose = addr_pair.second.purpose; | ||
| if (!purpose.empty()) { | ||
| data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose; | ||
| } | ||
| if (!addr_pair.second.IsChange()) { | ||
| data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); | ||
| } | ||
| continue; | ||
| } | ||
| if (data.solvable_wallet) { | ||
| LOCK(data.solvable_wallet->cs_wallet); | ||
| // Add to the solvable. Preserve the labels, purpose, and change-ness | ||
| std::string label = addr_pair.second.GetLabel(); | ||
| std::string purpose = addr_pair.second.purpose; | ||
| if (!purpose.empty()) { | ||
| data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose; | ||
| } | ||
| if (!addr_pair.second.IsChange()) { | ||
| data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Clone shared address-book entries into both auxiliary wallets.
The comment says non-receive entries should be copied to all migrated wallets, but the first continue prevents the second branch from running. When both _watchonly and _solvables are created, the solvables wallet never gets these shared labels.
Proposed fix
} else {
// Labels for everything else (send) should be cloned to all
if (data.watchonly_wallet) {
LOCK(data.watchonly_wallet->cs_wallet);
// Add to the watchonly. Preserve the labels, purpose, and change-ness
std::string label = addr_pair.second.GetLabel();
std::string purpose = addr_pair.second.purpose;
if (!purpose.empty()) {
data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
}
if (!addr_pair.second.IsChange()) {
data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label);
}
- continue;
}
if (data.solvable_wallet) {
LOCK(data.solvable_wallet->cs_wallet);
// Add to the solvable. Preserve the labels, purpose, and change-ness
std::string label = addr_pair.second.GetLabel();
std::string purpose = addr_pair.second.purpose;
if (!purpose.empty()) {
data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
}
if (!addr_pair.second.IsChange()) {
data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label);
}
- continue;
}
+ continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 4462 - 4488, The code currently exits the
loop after cloning labels into data.watchonly_wallet because of the first
continue, so when both data.watchonly_wallet and data.solvable_wallet exist the
solvable wallet never receives the shared address-book entries; remove the
mutual-exclusion by deleting the continue after the watchonly block (or refactor
so both conditional blocks run) so that the same cloning logic for addr_pair
(using addr_pair.second.GetLabel(), .purpose, .IsChange(), and writing into
m_address_book) executes for both data.watchonly_wallet and
data.solvable_wallet.
| if (success) { | ||
| // Migration successful, unload the wallet locally, then reload it. | ||
| assert(local_wallet.use_count() == 1); | ||
| local_wallet.reset(); | ||
| LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); | ||
| res.wallet_name = wallet_name; |
There was a problem hiding this comment.
Check the reload result before returning success.
If LoadWallet() fails here, migratewallet still reports success and sets res.wallet_name, leaving the migrated wallet unloaded with the error silently dropped.
Proposed fix
if (success) {
// Migration successful, unload the wallet locally, then reload it.
assert(local_wallet.use_count() == 1);
local_wallet.reset();
- LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
+ if (!LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings)) {
+ return util::Error{Untranslated("Wallet migration completed, but the migrated wallet could not be reloaded.") + Untranslated(" ") + error};
+ }
res.wallet_name = wallet_name;
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 4690 - 4695, After resetting local_wallet
and calling LoadWallet(context, wallet_name, ...), check LoadWallet's result
(its return value or the populated status/error) and treat a failure as an
overall migration failure: do not leave res.wallet_name set, propagate or set
the error/status so migratewallet returns failure, and ensure success is updated
accordingly; specifically, after LoadWallet(...) evaluate its outcome, if it
failed revert success to false, clear res.wallet_name, and propagate the
error/status back to the caller instead of returning success.
| assert not new_addr == addr | ||
| assert not new_change == addr |
There was a problem hiding this comment.
Compare new_change against the legacy change address.
Line 115 checks new_change against addr instead of change, so this won't catch reuse of the old change address.
Proposed fix
new_addr = basic0.getnewaddress()
new_change = basic0.getrawchangeaddress()
assert not new_addr == addr
- assert not new_change == addr
+ assert not new_change == change📝 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.
| assert not new_addr == addr | |
| assert not new_change == addr | |
| assert not new_addr == addr | |
| assert not new_change == change |
🤖 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 114 - 115, The test asserts
reuse of addresses but compares new_change to addr instead of the legacy change
address; update the assertion in test/functional/wallet_migration.py to assert
that new_change is not equal to change (i.e., replace the comparison of
new_change == addr with new_change == change) so the test verifies the new
change address differs from the legacy change address; ensure you reference the
variables new_addr, addr, new_change, and change in the modified assertion.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR backports migratewallet with well-structured Dash-specific adaptations. Several Claude findings about CoinJoin descriptor handling are false positives — legacy wallets don't use the DIP0009 (purpose 9') derivation path, so there are no CoinJoin-specific counters to advance or keys to cover with combo descriptors. The confirmed issues are minor: a copy-paste error in log/error messages for the solvable wallet, an upstream-inherited send-label logic gap, and useful observations about the backport's prerequisite workarounds.
Reviewed commit: 4f25d93
🟡 3 suggestion(s) | 💬 3 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] lines 4461-4488: Send labels not cloned to solvable wallet when watchonly wallet also exists
The comment on line 4462 says "Labels for everything else (send) should be cloned to all" but the `continue` on line 4474 exits the loop iteration after copying to the watchonly wallet, skipping the solvable wallet block entirely. When both wallets exist, send-address labels only go to watchonly. This is an upstream Bitcoin Core issue (later fixed in subsequent PRs not yet backported) and non-blocking since send labels are preserved in the main migrated wallet.
In `src/wallet/scriptpubkeyman.cpp`:
- [SUGGESTION] lines 1979-1988: Missing upstream prerequisite bitcoin#17681 — workaround is sound but limits scope
Upstream `#19602` was built on top of `#17681`'s `m_inactive_hd_chains` model. Dash lacks this, so this backport rewrites HD migration around `mapHdPubKeys` and guards with `CountAccounts() != 1` (lines 1985-1988). This is a clean workaround for stock Dash wallets (which always use a single BIP44 account), but it means wallets with multiple HD accounts (from modified builds or future features) are explicitly rejected rather than migrated. The guard is correct — failing loudly is far better than silently dropping keys.
In `test/functional/wallet_migration.py`:
- [SUGGESTION] lines 520-531: Address-book migration paths lack functional test coverage
Upstream Bitcoin Core has dedicated `test_addressbook()` coverage for address-book entry migration to watchonly/solvable wallets. This PR backports the address-book migration logic (the label-cloning code at lines 4415-4490 in wallet.cpp) but the functional test doesn't exercise scenarios where address-book entries are verified post-migration across the main, watchonly, and solvable wallets. A targeted test — e.g., adding a send-to address label before migration and asserting it appears in the correct post-migration wallet — would protect this path.
| wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n"); | ||
|
|
||
| DatabaseStatus status; | ||
| std::vector<bilingual_str> warnings; | ||
| std::string wallet_name = wallet.GetName() + "_solvables"; | ||
| data->solvable_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); | ||
| if (status != DatabaseStatus::SUCCESS) { | ||
| error = _("Error: Failed to create new watchonly wallet"); |
There was a problem hiding this comment.
💬 Nitpick: Copy-paste error: solvable wallet log and error message say 'watchonly'
Two messages in the solvable wallet creation block (lines 4581 and 4588) incorrectly say "watchonly" when they should say "solvable". Line 4581 logs "Making a new watchonly wallet containing the unwatched solvable scripts" and line 4588 sets the error to "Failed to create new watchonly wallet". Compare with the actual watchonly block at lines 4549-4558 which uses the correct wording. This is a copy-paste error inherited from upstream Bitcoin Core.
source: ['claude']
| } else { | ||
| // Labels for everything else (send) should be cloned to all | ||
| if (data.watchonly_wallet) { | ||
| LOCK(data.watchonly_wallet->cs_wallet); | ||
| // Add to the watchonly. Preserve the labels, purpose, and change-ness | ||
| std::string label = addr_pair.second.GetLabel(); | ||
| std::string purpose = addr_pair.second.purpose; | ||
| if (!purpose.empty()) { | ||
| data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose; | ||
| } | ||
| if (!addr_pair.second.IsChange()) { | ||
| data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); | ||
| } | ||
| continue; | ||
| } | ||
| if (data.solvable_wallet) { | ||
| LOCK(data.solvable_wallet->cs_wallet); | ||
| // Add to the solvable. Preserve the labels, purpose, and change-ness | ||
| std::string label = addr_pair.second.GetLabel(); | ||
| std::string purpose = addr_pair.second.purpose; | ||
| if (!purpose.empty()) { | ||
| data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose; | ||
| } | ||
| if (!addr_pair.second.IsChange()) { | ||
| data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Send labels not cloned to solvable wallet when watchonly wallet also exists
The comment on line 4462 says "Labels for everything else (send) should be cloned to all" but the continue on line 4474 exits the loop iteration after copying to the watchonly wallet, skipping the solvable wallet block entirely. When both wallets exist, send-address labels only go to watchonly. This is an upstream Bitcoin Core issue (later fixed in subsequent PRs not yet backported) and non-blocking since send labels are preserved in the main migrated wallet.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] lines 4461-4488: Send labels not cloned to solvable wallet when watchonly wallet also exists
The comment on line 4462 says "Labels for everything else (send) should be cloned to all" but the `continue` on line 4474 exits the loop iteration after copying to the watchonly wallet, skipping the solvable wallet block entirely. When both wallets exist, send-address labels only go to watchonly. This is an upstream Bitcoin Core issue (later fixed in subsequent PRs not yet backported) and non-blocking since send labels are preserved in the main migrated wallet.
| // Stock Dash legacy wallets only ever use a single BIP44 account (index 0): | ||
| // GenerateNewKey is hardcoded to account 0 at every call site and the chain | ||
| // is initialized with exactly one AddAccount(). A wallet created by a | ||
| // modified build with multiple accounts cannot be migrated by this code path | ||
| // since the descriptor wallet also only uses one account — bail out instead | ||
| // of silently dropping the higher accounts' keys. | ||
| if (hdChainDecrypted.CountAccounts() != 1) { | ||
| throw std::runtime_error(strprintf("%s: legacy HD chain has %d accounts; " | ||
| "migration only supports wallets with a single BIP44 account", | ||
| __func__, hdChainDecrypted.CountAccounts())); |
There was a problem hiding this comment.
🟡 Suggestion: Missing upstream prerequisite bitcoin#17681 — workaround is sound but limits scope
Upstream #19602 was built on top of #17681's m_inactive_hd_chains model. Dash lacks this, so this backport rewrites HD migration around mapHdPubKeys and guards with CountAccounts() != 1 (lines 1985-1988). This is a clean workaround for stock Dash wallets (which always use a single BIP44 account), but it means wallets with multiple HD accounts (from modified builds or future features) are explicitly rejected rather than migrated. The guard is correct — failing loudly is far better than silently dropping keys.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/scriptpubkeyman.cpp`:
- [SUGGESTION] lines 1979-1988: Missing upstream prerequisite bitcoin#17681 — workaround is sound but limits scope
Upstream `#19602` was built on top of `#17681`'s `m_inactive_hd_chains` model. Dash lacks this, so this backport rewrites HD migration around `mapHdPubKeys` and guards with `CountAccounts() != 1` (lines 1985-1988). This is a clean workaround for stock Dash wallets (which always use a single BIP44 account), but it means wallets with multiple HD accounts (from modified builds or future features) are explicitly rejected rather than migrated. The guard is correct — failing loudly is far better than silently dropping keys.
| auto it = mapKeyMetadata.find(keyID); | ||
| if (it != mapKeyMetadata.end()) { | ||
| meta = it->second; | ||
| if (it == mapKeyMetadata.end()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: GetKeyOrigin now returns false for keys without metadata entries
The old code fell through when mapKeyMetadata had no entry for a key, producing a fallback fingerprint from the first 4 bytes of the key ID. The new code returns false immediately. This is actually more correct — the old behavior reported misleading origin info for imported keys without metadata. All callers check the return value (line 1927 uses has_info to conditionally include origin in the descriptor string; line 2758 guards with if), so this is safe. Worth noting it may change getaddressinfo output for pre-existing imported keys that lack metadata.
source: ['claude']
| } else { | ||
| // Setup with a new seed if we don't. | ||
| SetupDescriptorScriptPubKeyMans("", ""); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Non-HD wallet migration silently generates a new seed
When data.master_key.key.IsValid() is false (pre-HD legacy wallet), line 4372 calls SetupDescriptorScriptPubKeyMans("", "") which generates a brand-new random mnemonic and seed. The comment on line 4371 documents this, but there's no WalletLogPrintf call to record it. Adding a log line like WalletLogPrintf("No HD master key found; generating new seed for descriptor wallet\n"); would help users understand what happened during migration.
source: ['claude']
| def run_test(self): | ||
| self.generate(self.nodes[0], 101) | ||
|
|
||
| # TODO: Test the actual records in the wallet for these tests too. The behavior may be correct, but the data written may not be what we actually want | ||
| self.test_basic() | ||
| self.test_multisig() | ||
| self.test_other_watchonly() | ||
| self.test_no_privkeys() | ||
| self.test_pk_coinbases() | ||
| self.test_encrypted() | ||
| self.test_unloaded() | ||
| self.test_unloaded_by_path() |
There was a problem hiding this comment.
🟡 Suggestion: Address-book migration paths lack functional test coverage
Upstream Bitcoin Core has dedicated test_addressbook() coverage for address-book entry migration to watchonly/solvable wallets. This PR backports the address-book migration logic (the label-cloning code at lines 4415-4490 in wallet.cpp) but the functional test doesn't exercise scenarios where address-book entries are verified post-migration across the main, watchonly, and solvable wallets. A targeted test — e.g., adding a send-to address label before migration and asserting it appears in the correct post-migration wallet — would protect this path.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/wallet_migration.py`:
- [SUGGESTION] lines 520-531: Address-book migration paths lack functional test coverage
Upstream Bitcoin Core has dedicated `test_addressbook()` coverage for address-book entry migration to watchonly/solvable wallets. This PR backports the address-book migration logic (the label-cloning code at lines 4415-4490 in wallet.cpp) but the functional test doesn't exercise scenarios where address-book entries are verified post-migration across the main, watchonly, and solvable wallets. A targeted test — e.g., adding a send-to address label before migration and asserting it appears in the correct post-migration wallet — would protect this path.
4 participants
Issue being fixed or feature implemented
Legacy wallets is subject to be removed in Bitcoin Core, here's bitcoin's timeline for reference: bitcoin#20160
Descriptor wallets is going to be created by default from Dash Core v24: #7204
What was done?
Backports:
migratewalletcan be successfully restored bitcoin/bitcoin#28257And more to be done in further PR
There are several key differences with Bitcoin Core:
mapKeysandmapCryptedKeysthere should be done extra lookup overmapHdPubKeysin migration codeHow Has This Been Tested?
See updates for new functional test wallet_migration.py
Also all my own legacy wallets (mainnet, testnet) has been successfully migrated to descriptor during testing of this PR, including coinjoin-status of mixed coins.
One of my wallet on testnet highlighted existing bug of mainstream. This bug with watch-only addresses has been fixed by bitcoin#28868 and this fix will be included in the next batch of backports.
Breaking Changes
N/A
Checklist: