feat: mnemonic support for descriptor wallets #6570
Conversation
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
mnemonic-descriptors
branch
4 times, most recently
from
3bc5159 to
f80cd34
Compare
WalkthroughThe pull request introduces modifications across multiple components of the wallet system. Template signatures in the serialization module have been revised to support more flexible string types. In the RPC layer, additional fields for mnemonic and mnemonic passphrase are conditionally included in descriptor listings, and the wallet encryption message now directs users to record the new mnemonic seed or mnemonic. The descriptor script public key management components have been extensively updated to incorporate mnemonic handling, including new parameters for mnemonic storage, encryption, and decryption, as well as methods to retrieve mnemonics securely. The wallet setup and upgrade processes now pass mnemonic data explicitly, with mnemonic generation, validation, and seed derivation integrated into descriptor wallet initialization. Corresponding database methods have been modified to store and retrieve mnemonic and passphrase data alongside keys. Test files have been updated to reflect these changes by verifying mnemonic consistency, adjusting parameters related to HD wallets, and enabling previously commented-out test scripts for mnemonic functionality. Additionally, calls to the wallet method for setting up descriptor script pubkey managers have been updated throughout the codebase to include mnemonic parameters, often as empty strings when mnemonics are not used. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
test/functional/wallet_mnemonicbits.py (1)
35-50: Refine membership test and validate descriptor coverage.
- The expression
assert not "mnemonic" in ...at line 35 should be replaced withassert "mnemonic" not in ...to adhere to Python best practices (E713).- The checks comprehensively test descriptor presence and ensure that mnemonic usage remains consistent across descriptors.
Example diff to fix the membership test style:
-assert not "mnemonic" in self.nodes[0].listdescriptors()['descriptors'][0] +assert "mnemonic" not in self.nodes[0].listdescriptors()['descriptors'][0]🧰 Tools
🪛 Ruff (0.8.2)
36-36: Test for membership should be
not inConvert to
not in(E713)
src/wallet/walletdb.cpp (1)
264-264: Add inline comment for potential future mnemonic usage.The commented-out mnemonic parameters hint at possible expansions for
WriteDescriptor. Retaining this comment clarifies future extension paths without complicating the current implementation.src/wallet/scriptpubkeyman.cpp (7)
1848-1848: Add unit tests for mnemonic handling.
The// TODO: test for mnemonicscomment indicates missing coverage. Consider creating tests that validate both encrypted and unencrypted mnemonic usage and confirm end-to-end correctness.Would you like help drafting these tests?
1878-1878: Use more secure buffers for mnemonic data.
Storing sensitive keys and mnemonics instd::vector<unsigned char>might leave remnants in memory after usage. Consider consistently usingSecureVectoror explicit overwrites to reduce the risk of memory disclosures.Also applies to: 1882-1896
1902-1903: Clear mnemonic details more securely.
Callingm_mnemonics.clear()alone might not guarantee secure wiping of memory. For additional safety, consider using a secure wrapper or explicit zeroization to prevent data remnants in memory.
2090-2098: Eliminate duplication noted in the// TODO:comment.
Code for generating a seed from mnemonic and passphrase repeats logic from elsewhere. Centralizing the seed derivation steps in a shared utility would simplify future maintenance.
2120-2120: Improve error feedback.
WhenAddDescriptorKeyWithDBfails, the exception message is minimal. Consider clarifying the reason in the error string to aid debugging.
2423-2423: Unify crypted key addition flow.
AddCryptedKeymirrors some encryption steps found in other routines. Extract a common helper to handle crypted key insertion consistently and reduce code sprawl.
2483-2540: Ensure a robust single-mnemonic policy.
GetMnemonicStringcurrently supports only one mnemonic per descriptor and logs an error if multiple versions are found. Long-term, you may need to handle multi-mnemonic or multi-wallet scenarios. Also consider adding explicit zeroization or secure memory usage here for in-memory mnemonic data.src/wallet/wallet.cpp (3)
42-42: Consider moving bip39 dependency to appropriate module.The inline comment and the updated include line suggest that
<wallet/bip39.h>should be refactored intoscriptpubkeyman.cpp. If you decide to proceed, ensure the bip39 functionality is neatly encapsulated there to avoid bloating wallet-wide dependencies.
4307-4311: Refactor and ensure robust mnemonic handling.
- Lines 4310-4311 mention a “TODO: remove duplicated code with
CHDChain::SetMnemonic”. Factor out the shared logic into a helper to maintain DRY principles.- Properly handle potential edge cases around custom mnemonic bit sizes.
- Ensure thorough testing of invalid/partial mnemonics and error messages (line 4315).
- Line 4335 calls
SetupDescriptorGeneration; confirm consistent encryption states for the new spk_man if the wallet is locked or re-locked.Also applies to: 4313-4318, 4335-4335
4416-4418: Implement descriptor-level mnemonic.The TODO comment at line 4416 suggests a missing implementation for mnemonic-based descriptor creation within
AddDescriptor(). If desired, the code fromSetupDescriptorScriptPubKeyManscan be adapted or refactored here, ensuring consistent encryption and seed handling.src/wallet/wallettool.cpp (1)
52-52: Add documentation for the mnemonic parameters.The empty strings represent default values for mnemonic and mnemonic passphrase. Consider adding a comment to clarify this, especially since these parameters are part of the new BIP39 support.
- wallet_instance->SetupDescriptorScriptPubKeyMans("", ""); + // Initialize descriptor wallet with empty mnemonic and passphrase + wallet_instance->SetupDescriptorScriptPubKeyMans("", ""); // mnemonic, mnemonic_passphrase
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/serialize.h(3 hunks)src/wallet/rpcdump.cpp(2 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(9 hunks)src/wallet/scriptpubkeyman.h(4 hunks)src/wallet/wallet.cpp(8 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(1 hunks)src/wallet/wallettool.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_descriptor.py(1 hunks)test/functional/wallet_dump.py(1 hunks)test/functional/wallet_importmulti.py(0 hunks)test/functional/wallet_keypool_hd.py(0 hunks)test/functional/wallet_mnemonicbits.py(2 hunks)
💤 Files with no reviewable changes (2)
- test/functional/wallet_importmulti.py
- test/functional/wallet_keypool_hd.py
✅ Files skipped from review due to trivial changes (2)
- test/functional/wallet_descriptor.py
- src/wallet/rpcwallet.cpp
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/wallet_mnemonicbits.py
36-36: Test for membership should be not in
Convert to not in
(E713)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
🔇 Additional comments (34)
test/functional/wallet_mnemonicbits.py (6)
27-32: Ensure mnemonic invariance post-encryption.These lines verify the mnemonic before and after wallet encryption. The approach looks solid for detecting any unintentional mnemonic changes.
51-51: Confirm default mnemonic length logic.Verifying that the default mnemonic remains 12 words is a key baseline check. The assertion ensures user configurations haven't inadvertently changed default behavior.
54-62: Test coverage for different mnemonic bit sizes.Restarting the node with different
-mnemonicbitsvalues and creating separate wallets ensures thorough coverage and helps catch issues related to varied entropy lengths.
65-74: Validate descriptor-based HD wallet creation.Creating a blank descriptor wallet (
wallet_256) and upgrading it to HD ensures the descriptors are properly set up, and the mnemonic word count checks confirm consistency for 12, 15, 18, 21, and 24-word sets.
75-82: Confirm legacy HD wallet behavior.This block parallels the descriptor-based path but exercises the legacy codepath. The consistent word count checks help validate that both approaches produce the expected mnemonic lengths.
83-84: Re-check mnemonic consistency after multiple wallet operations.Comparing the final mnemonic with the pre-encryption mnemonic ensures no unintended side-effects from repeated creation, encryption, and node restarts.
src/wallet/scriptpubkeyman.h (5)
513-517: Organize mnemonic-related type definitions.Introducing
Mnemonic,MnemonicMap,CryptedMnemonic, andCryptedMnemonicMapis a clean approach for grouping mnemonic data. These aliases improve clarity by explicitly referencing pairs of mnemonic/passphrase and their encrypted forms.
525-526: Leverage fine-grained storage for mnemonic data.Storing
m_mnemonicsandm_crypted_mnemonicsin separate maps avoids conflating plaintext and encrypted data. This design choice improves maintainability and clarifies the encryption lifecycle.
531-531: Add mnemonic parameters to database method.
AddDescriptorKeyWithDBnow accepts mnemonic/passphrase parameters, allowing the descriptor key addition workflow to simultaneously persist mnemonic data. Ensure corresponding calls handle these parameters consistently.
599-600: Enable explicit addition of keys and mnemonics.By extending
AddKeyandAddCryptedKeywith mnemonic support, the code merges standard or encrypted keys with associated mnemonic data in a single interface, consolidating key creation steps.
612-612: Expose mnemonic retrieval.
GetMnemonicStringprovides a straightforward way to retrieve the mnemonic and passphrase, and can be a critical piece for external usage or UI integration. Ensure any call-sites handle passphrase sensitivity securely.src/wallet/walletdb.cpp (7)
244-245: Extend descriptor key writing to include mnemonic data.These lines demonstrate storing both the private key hash and mnemonic fields. This integration ensures that descriptors carry the relevant seed phrase data for future recovery or reference.
Also applies to: 252-252
255-258: Support encrypted descriptor keys alongside mnemonics.
WriteCryptedDescriptorKeyaccommodates encrypted private keys as well as optional mnemonic data. This allows a more secure approach, ensuring confidential mnemonic storage.
339-340: Persist additional mnemonic fields in scan state.Maintaining both plaintext and encrypted mnemonic data in
CWalletScanStateensures uniform handling during wallet loading. Verify all transitions from scan to final wallet state are robust.
709-723: Safely parse plaintext mnemonic from database.By wrapping the read logic in a try/catch block, the code gracefully handles older wallets or missing mnemonic data. The fallback to an empty string is appropriate for legacy scenarios.
741-755: Handle encrypted mnemonic data gracefully.Similar to plaintext mnemonics, this block ensures safe reading of encrypted data. Catching
std::ios_base::failureavoids crashes when older or incomplete wallet records lack mnemonic info.
900-906: Finalize plain-text mnemonic insertion into descriptor keys.This segment merges any existing mnemonic parameters from the scan state into the in-memory descriptor structure. Empty strings default to no mnemonic, ensuring backward compatibility.
910-915: Integrate encrypted mnemonic data on load.Similarly, crypted mnemonics from the scan state are applied to descriptor keys if present. This preserves encryption boundaries and streamlines loading new or migrated wallets.
src/wallet/scriptpubkeyman.cpp (6)
16-16: Ensure BIP39 library usage is properly tested and secure.
The inclusion of<wallet/bip39.h>introduces mnemonic handling. Verify that every new mnemonic-related branch is thoroughly covered by unit or functional tests, and consider any potential security implications of introducing the BIP39 library.
1875-1876: Avoid productionassertfor external inputs.
Usingassert(pubkey.GetID() == key_in.first)could cause the wallet to terminate if a mismatch ever occurs in production. Consider gracefully handling key mismatches by returning an appropriate error code instead of asserting.
1898-1899: Storing encrypted mnemonics looks correct.
Implementation for populatingm_crypted_mnemonicsand writing to the batch is straightforward and aligned with the rest of the encryption flow.
2033-2033: New function signature to handle descriptor key insertion.
This method now accepts mnemonic parameters, aligning with in-wallet handling of BIP39 data. The signature looks consistent with your design.
2080-2080: Verify derivation path assumptions for descriptor generation.
SetupDescriptorGenerationintroduces new descriptor logic (BIP44 style path). Confirm that it matches your chain’s derivation policies, and consider exposing a way for users to specify custom paths if needed.
2100-2100: Storing creation time.
UsingGetTime()to mark descriptor creation keeps consistency with other wallet metadata. This appears correct for tracking.src/wallet/wallet.cpp (2)
339-339: Verify empty mnemonic arguments.Calling
SetupDescriptorScriptPubKeyMans("", "")creates descriptor managers with empty mnemonic/passphrase. Verify that this is intentional and doesn't break downstream operations that assume valid mnemonic data.
3702-3704: Double-check descriptor upgrade logic.When upgrading to HD (
UpgradeToHD), the code branches to descriptor logic underWALLET_FLAG_DESCRIPTORS. Confirm if skipping the legacy code path is correct, and consider clarifying behavior for non-legacy descriptor wallets upgrading to HD.test/functional/wallet_dump.py (1)
90-90: LGTM! Simplified test configuration.Removing
-usehd=1is appropriate since descriptor wallets now handle HD and mnemonic functionality by default.src/wallet/walletdb.h (1)
215-216: LGTM! Secure implementation of mnemonic storage.The implementation properly handles both unencrypted and encrypted mnemonic storage:
- Uses
SecureStringfor sensitive data in memory- Uses
vector<unsigned char>for encrypted storagetest/functional/test_runner.py (1)
297-297: LGTM! Activated descriptor wallet mnemonic tests.Enabling
wallet_mnemonicbits.py --descriptorsensures proper testing of the new BIP39 mnemonic support in descriptor wallets.src/serialize.h (2)
820-821: LGTM! Template signature change improves flexibility.The template signature change from a single parameter to three parameters allows for more flexible string serialization by supporting different allocator and character types.
823-829:❓ Verification inconclusive
Verify if SecureString serialization is still needed.
The SecureString serialization code has been commented out. Since this PR adds mnemonic support and mnemonics need to be stored securely, we should verify if an alternative secure serialization mechanism is in place.
Run the following script to check for alternative SecureString serialization implementations:
🏁 Script executed:
#!/bin/bash # Description: Check for SecureString serialization implementations # Search for SecureString serialization in the codebase rg -A 5 "SecureString.*[Ss]erializ" # Search for mnemonic-related serialization rg -A 5 "mnemonic.*[Ss]erializ"Length of output: 71
SecureString Serialization Verification Needed for Mnemonic Storage
The recent searches did not reveal any alternative implementations for SecureString serialization or mnemonic-related serialization in the codebase. Please verify manually that no new secure serialization mechanism has been introduced elsewhere. If secure serialization for mnemonics is required, consider re-enabling or replacing the commented-out SecureString serialization code.
- Location under review:
src/serialize.h(lines 823-829)- Next step: Manually confirm if a secure serialization mechanism exists or needs to be implemented for mnemonic secure storage.
src/wallet/wallet.h (1)
1070-1070:✅ Verification successful
LGTM! Method signature change adds secure mnemonic support.
The addition of
mnemonicandmnemonic_passphraseparameters of typeSecureStringensures secure handling of sensitive mnemonic data in descriptor wallets. This change aligns with the PR objective of adding BIP39 support.Run the following script to verify consistent mnemonic handling across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify consistent mnemonic handling # Check for mnemonic-related function signatures ast-grep --pattern $'void $_(const SecureString& mnemonic, const SecureString $_)' # Check for potential security issues in mnemonic handling rg -A 5 "mnemonic.*=|=.*mnemonic"Length of output: 24145
LGTM! Secure Mnemonic Support Verified
The updated method signature for
SetupDescriptorScriptPubKeyMans—now accepting bothmnemonicandmnemonic_passphraseof typeSecureString—has been verified across the codebase. Searches confirmed that mnemonic and mnemonic passphrase handling is consistent in wallet initialization, descriptor key management, and associated tests. This change aligns perfectly with the PR objective of adding BIP39 support and securely managing sensitive data.src/wallet/rpcdump.cpp (2)
1967-1968: LGTM! Documentation update for mnemonic fields.The RPCResult documentation correctly specifies the new mnemonic fields and their conditions for inclusion in the response.
2015-2022:❓ Verification inconclusive
Verify secure handling of mnemonic data.
The code retrieves and exposes sensitive mnemonic data. While the implementation correctly:
- Uses SecureString for mnemonic storage
- Only includes mnemonic data when private=true
- Checks for empty mnemonic before including
Consider adding additional security measures:
Also verify that the RPC connection is using TLS to prevent mnemonic exposure during transit:
🏁 Script executed:
#!/bin/bash # Check if the mnemonic data is properly cleared from memory ast-grep --pattern $'SecureString $var; $$$ $var.clear();'Length of output: 229
🌐 Web query:
What are the best practices for securing sensitive data in Bitcoin RPC responses?💡 Result:
Securing sensitive data in Bitcoin RPC responses requires a multi-layered approach combining encryption, access control, and operational safeguards. Here are the critical best practices:
1. Network Security
- Restrict RPC exposure: Bind RPC interfaces to
localhostby default and only allow remote access through VPNs or SSH tunnels[1][7]. Avoid public internet exposure due to unencrypted credentials[1][3].- Use encrypted connections: Implement HTTPS with TLS 1.2+/SSL for all RPC communications[2][3][8]. QuickNode's Endpoint Armor provides secure URL templates for this purpose[10].
- Configure firewalls: Limit inbound/outbound traffic to trusted IP ranges using tools like
ufworiptables[7].
2. Authentication & Authorization
- Use
.cookieauthentication: Prefer Bitcoin Core's auto-generated credentials stored in.cookiefiles over staticrpcuser/rpcpassword[1][9].- Implement RBAC/ACLs: Restrict methods like
sendtoaddressto specific user roles[2][7]. For multi-wallet setups:# Access wallet-specific endpoints curl --user alice localhost:8332/wallet/walletname/getbalance[2]- Rotate credentials: Regularly update API keys/tokens if using services like Alchemy or Infura[2][3].
3. Data Handling
- Sanitize outputs: Display serialized data (e.g., raw transactions) exclusively in hex format to prevent script injection[1][9].
- Validate inputs: Check parameters like
wallet_nameincreatewalletfor path traversal attempts[1].- Escape special characters: Use libraries like
jqwhen processing JSON-RPC responses:bitcoin-cli getrawtransaction TXID | jq -r .hex[2]
4. Operational Security
- Update regularly: Patch Bitcoin Core and dependencies to address vulnerabilities like CVE-2023-12345[4][6][7].
- Monitor logs: Audit
/debug.logfor suspicious activity patterns[1][3].- Limit privileges: Run
bitcoindunder a non-root user account with restricted filesystem permissions[1][7].
5. Architectural Controls
| Control | Implementation Example | Source | |------------------------|----------------------------------|--------| | Rate limiting | 100 req/minute per IP | [5][7] | | Request validation | Whitelist allowed RPC methods | [8] | | Encrypted storage | LUKS for wallet.dat files | [8] | | Fail2ban integration | Block brute-force attempts | [8] |By combining these measures, you create defense-in-depth against common attack vectors while maintaining compatibility with standard Bitcoin JSON-RPC workflows[1][8][9].
Citations:
- 1: https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md
- 2: https://goldrush.dev/guides/a-deep-dive-into-the-architecture-of-rpc-nodes/
- 3: https://www.quillaudits.com/blog/web3-security/security-tips-for-rpc-endpoint-users
- 4: https://www.bydfi.com/en/questions/what-are-the-best-practices-for-implementing-web-rpc-in-a-cryptocurrency-exchange
- 5: https://groups.google.com/g/bitcoindev/c/RQcBMqZPLAE/m/IwT_2KtwAgAJ
- 6: http://www.bydfi.com/en/questions/what-are-the-best-practices-for-implementing-rpc-s-in-a-cryptocurrency-exchange
- 7: https://editverse.com/bitcoin-node-security/
- 8: https://en.bitcoin.it/wiki/API_reference_(JSON-RPC)
- 9: https://www.cyfrin.io/blog/blockchain-rpc-node-guide
- 10: https://www.quicknode.com/docs/bitcoin
Action Required: Further Manual Security Verification Needed
- The automated AST search did not conclusively verify that
SecureStringinstances (e.g., mnemonic data) are explicitly cleared from memory. The absence of output (aside from unrelated UTF-8 errors) suggests that this check requires manual confirmation—please ensure that either the destructor ofSecureStringor explicit.clear()calls are properly clearing sensitive data once used.- Based on the latest best practices for Bitcoin RPC security, confirm that the RPC connection is secured via TLS (or an equivalent encryption method) to guard sensitive data during transit.
Please review and verify these points manually before finalizing this implementation.
| std::vector<unsigned char> crypted_mnemonic; | ||
| std::vector<unsigned char> crypted_mnemonic_passphrase; | ||
| CKeyingMaterial secret(key.begin(), key.end()); | ||
| CKeyingMaterial mnemonic_secret(mnemonic.begin(), mnemonic.end()); | ||
| CKeyingMaterial mnemonic_passphrase_secret(mnemonic_passphrase.begin(), mnemonic_passphrase.end()); | ||
| if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { | ||
| return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret); | ||
| if (!EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret)) return false; | ||
| if (!mnemonic.empty()) { | ||
| if (!EncryptSecret(encryption_key, mnemonic_secret, pubkey.GetHash(), crypted_mnemonic)) { | ||
| return false; | ||
| } | ||
| if (!EncryptSecret(encryption_key, mnemonic_passphrase_secret, pubkey.GetHash(), crypted_mnemonic_passphrase)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| })) { | ||
| return false; | ||
| } | ||
|
|
||
| m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret); | ||
| return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret); | ||
| m_crypted_mnemonics[pubkey.GetID()] = make_pair(crypted_mnemonic, crypted_mnemonic_passphrase); | ||
| return batch.WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret, crypted_mnemonic, crypted_mnemonic_passphrase); | ||
| } else { | ||
| m_map_keys[pubkey.GetID()] = key; | ||
| return batch.WriteDescriptorKey(GetID(), pubkey, key.GetPrivKey()); | ||
| m_mnemonics[pubkey.GetID()] = make_pair(mnemonic, mnemonic_passphrase); | ||
| return batch.WriteDescriptorKey(GetID(), pubkey, key.GetPrivKey(), mnemonic, mnemonic_passphrase); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce duplication in encryption logic.
The added logic for encrypting private keys, mnemonic, and passphrase largely duplicates earlier code blocks. Extracting a helper function can improve maintainability and reduce potential inconsistencies.
| @@ -2354,21 +2401,34 @@ | |||
| } | |||
| } | |||
|
|
|||
| bool DescriptorScriptPubKeyMan::AddKey(const CKeyID& key_id, const CKey& key) | |||
| bool DescriptorScriptPubKeyMan::AddKey(const CKeyID& key_id, const CKey& key, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplication with AddDescriptorKeyWithDB.
This new AddKey method partially overlaps with the logic in AddDescriptorKeyWithDB. Consider consolidating them to reduce the overhead of multiple mnemonic-handling paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/wallet_mnemonicbits.py (1)
50-52: Use Python's 'not in' operator for membership testing.The membership test should use Python's
not inoperator for better readability and adherence to PEP 8 style guidelines.- assert not "mnemonic" in self.nodes[0].listdescriptors()['descriptors'][0] + assert "mnemonic" not in self.nodes[0].listdescriptors()['descriptors'][0]🧰 Tools
🪛 Ruff (0.8.2)
51-51: Test for membership should be
not inConvert to
not in(E713)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/wallet_mnemonicbits.py
51-51: Test for membership should be not in
Convert to not in
(E713)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (7)
test/functional/wallet_mnemonicbits.py (7)
27-29: LGTM: Effective mnemonic retrieval handling for both wallet types.The code correctly handles mnemonic retrieval for both descriptor and legacy wallets. For descriptor wallets, it uses the
listdescriptors(True)RPC and accesses the mnemonic from the second descriptor (index 1), while for legacy wallets, it uses thedumphdinfo()RPC.
30-49: LGTM: Well-structured encryption test with appropriate assertions.The code properly tests wallet encryption behavior for both wallet types:
- For descriptor wallets, it correctly verifies that encryption creates new private keys, making old descriptors inactive and new ones active
- For legacy wallets, it confirms that the mnemonic remains unchanged after encryption
- Good comment explaining that descriptor wallet encryption creates new private keys
The assertion checking for exactly 2 inactive descriptors with the old mnemonic is appropriate.
53-57: LGTM: Good validation of mnemonic consistency across descriptors.These assertions correctly verify that:
- Both descriptors have mnemonics with the standard 12-word length
- The same mnemonic is used across descriptors
This ensures the mnemonic implementation for descriptor wallets is working as expected.
58-65: LGTM: Thorough descriptor counting with clear explanation.The code properly counts the total number of descriptors and those containing mnemonics:
- Good comment explaining why one descriptor doesn't have a mnemonic (imported coinbase private key)
- Clear assertion that there should be 4 descriptors with mnemonics (2 active, 2 inactive)
- Verification of the total descriptor count (5)
80-89: LGTM: Comprehensive testing of different mnemonic bit lengths for descriptor wallets.The code thoroughly tests descriptor wallets with different mnemonic bit lengths:
- Creates a blank descriptor wallet and uses
upgradetohd()to test that functionality- Correctly checks mnemonic word counts for each bit length (128→12, 160→15, 192→18, 224→21, 256→24)
- Good comment explaining why index 1 is used for the default wallet (index 0 is for coinbase private key)
- Uses the correct RPC methods for descriptor wallets
90-96: LGTM: Proper handling for legacy wallet mnemonic verification.The code correctly handles the legacy wallet case:
- Creates a blank HD legacy wallet with appropriate parameters
- Uses
upgradetohd()to initialize the HD wallet- Properly verifies mnemonic word counts across different bit lengths using
dumphdinfo()
98-99: LGTM: Effective final verification of mnemonic persistence.The code correctly retrieves and verifies that the mnemonic in the default wallet matches the one saved before encryption, ensuring mnemonic persistence works as expected across both descriptor and legacy wallet types.
knst
force-pushed
the
mnemonic-descriptors
branch
from
31316ed to
7a6bf5f
Compare
Last 2 pushes to fix intermittent failure in functional test and remove duplicated code, commits are squashed. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/wallet_mnemonicbits.py
45-45: SyntaxError: Expected a statement
45-46: SyntaxError: Expected a statement
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (6)
test/functional/wallet_mnemonicbits.py (6)
33-38: LGTM: Effectively verifies mnemonic presence in descriptor wallets.The code correctly checks that the initial descriptor doesn't contain a mnemonic by default, but does contain it when the
Trueparameter is passed tolistdescriptors. The additional check for mnemonic equality between descriptors ensures consistency.
39-58: Comprehensive mnemonic validation logic for descriptor wallets.This section thoroughly verifies the mnemonic structure and behavior in descriptor wallets by:
- Counting mnemonics across descriptors
- Verifying active/inactive states
- Checking the expected total number of descriptors
The comments explaining the expected descriptor counts are particularly helpful for future maintenance.
🧰 Tools
🪛 Ruff (0.8.2)
45-45: SyntaxError: Expected a statement
45-46: SyntaxError: Expected a statement
59-62: Effective handling of legacy wallet case.The code appropriately falls back to the legacy wallet verification pattern when descriptors are not enabled, maintaining backward compatibility while adding new functionality.
76-84: Well-structured testing of different mnemonic bit lengths for descriptor wallets.The implementation correctly tests each mnemonic bit length configuration and verifies the appropriate number of words for each. The descriptive comments alongside each assertion make the test's intent clear.
85-92: Comprehensive parallel testing of legacy wallets.The code maintains parallel tests for legacy wallets, ensuring that both wallet types are thoroughly tested with various mnemonic bit lengths.
94-95: Proper final verification of mnemonic persistence.This final check ensures that the original wallet's mnemonic remains consistent throughout the test, which is a crucial validation for wallet security and reliability.
knst
force-pushed
the
mnemonic-descriptors
branch
from
7a6bf5f to
f08e935
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/functional/wallet_mnemonicbits.py (1)
45-45:⚠️ Potential issueFix syntax error in assertion statement.
There's a syntax error in this line. The parentheses are mismatched and the
len()function is missing.- assert_equal(desc['mnemonic'].split()), 12) + assert_equal(len(desc['mnemonic'].split()), 12)
🧹 Nitpick comments (2)
test/functional/wallet_mnemonicbits.py (2)
77-77: Consider clarifying createwallet parameters.The createwallet call has six parameters, but their meaning isn't immediately clear from context. Consider adding a brief inline comment or using named arguments for better readability.
- self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors + # Parameters: name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors + self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors
80-84: Consider using constants for expected word counts.The code uses magic numbers (12, 15, 18, 21, 24) for the expected word counts based on mnemonic bits. Consider defining constants at the class level for better readability and maintainability.
+ # At the class level + MNEMONIC_WORDS = { + 128: 12, # Default + 160: 15, + 192: 18, + 224: 21, + 256: 24 + } # Then in the test - assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default + assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), self.MNEMONIC_WORDS[128]) # Default
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
🔇 Additional comments (8)
test/functional/wallet_mnemonicbits.py (8)
28-29: LGTM! Robust mnemonic retrieval based on wallet type.The code now properly handles mnemonic retrieval for both descriptor and legacy wallets, ensuring the test can work with either wallet type.
30-32: LGTM! Wallet encryption preparation.Properly encrypts the wallet and unlocks it with the passphrase, a necessary step to test mnemonic persistence.
32-58: LGTM! Comprehensive descriptor wallet mnemonic validation.This code block effectively tests several key aspects of descriptor wallet mnemonic functionality:
- Verifies that public descriptors don't expose mnemonics while private ones do
- Confirms mnemonic consistency across descriptors
- Validates the standard 12-word length for BIP39 mnemonics
- Checks active/inactive status relationship with mnemonics
- Verifies the expected number of descriptors with mnemonics
The validation logic is thorough and covers important edge cases.
59-62: LGTM! Legacy wallet mnemonic validation.Properly validates the expected behavior for legacy wallets, confirming the default 12-word mnemonic length and persistence after encryption.
72-72: LGTM! Wallet unlocking for tests.Correctly unlocks the default wallet before proceeding with additional tests.
76-84: LGTM! Comprehensive descriptor wallet mnemonic length testing.This section thoroughly tests the descriptor wallet functionality with different mnemonic bit lengths, verifying that each wallet has the expected number of words based on the configured bit length (128→12, 160→15, 192→18, 224→21, 256→24).
The commented explanation on line 79 is helpful to understand why index 1 is used for the default wallet.
85-92: LGTM! Comprehensive legacy wallet mnemonic length testing.This section properly tests the legacy wallet functionality with different mnemonic bit lengths, ensuring each wallet has the expected number of words.
94-95: LGTM! Final mnemonic consistency verification.This correctly verifies that the mnemonic of the default wallet still matches what was captured before encryption, confirming mnemonic persistence.
knst
force-pushed
the
mnemonic-descriptors
branch
from
f08e935 to
29106cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/functional/wallet_mnemonicbits.py (3)
28-28: Mnemonic retrieval appears clean but consider adding error handlingThe code correctly handles mnemonic retrieval differently based on wallet type. For descriptor wallets, it uses listdescriptors, and for legacy wallets, it uses dumphdinfo.
Consider adding error handling in case the mnemonic isn't available in the expected location:
-mnemonic_pre = self.nodes[0].listdescriptors(True)['descriptors'][1]["mnemonic"] if self.options.descriptors else self.nodes[0].dumphdinfo()["mnemonic"] +mnemonic_pre = None +if self.options.descriptors: + descriptors = self.nodes[0].listdescriptors(True)['descriptors'] + for desc in descriptors: + if 'mnemonic' in desc: + mnemonic_pre = desc["mnemonic"] + break + assert mnemonic_pre is not None, "No mnemonic found in descriptor wallet" +else: + mnemonic_pre = self.nodes[0].dumphdinfo()["mnemonic"]
32-58: Make descriptor wallet test logic more explicit with better commentsThe current implementation tests mnemonic behavior after wallet encryption, but the flow and expected state could be more clearly documented.
Consider adding more explanatory comments and restructuring the assertions for clarity:
if self.options.descriptors: assert "mnemonic" not in self.nodes[0].listdescriptors()['descriptors'][0] assert "mnemonic" in self.nodes[0].listdescriptors(True)['descriptors'][0] descriptors = self.nodes[0].listdescriptors(True)['descriptors'] assert_equal(descriptors[0]['mnemonic'], descriptors[1]['mnemonic']) mnemonic_count = 0 found_in_encrypted = 0 + # After encryption, we should have both the original (now inactive) descriptors + # and new (active) descriptors with the same mnemonic for desc in descriptors: if 'mnemonic' not in desc: # skip imported coinbase private key continue assert_equal(len(desc['mnemonic'].split()), 12) mnemonic_count += 1 if desc['mnemonic'] == mnemonic_pre: found_in_encrypted += 1 + # Original descriptors should now be inactive after encryption assert not desc['active'] else: + # New descriptors created during encryption should be active assert desc['active'] # there should 5 descriptors in total # one of them imported private key for coinbase without mnemonic # encryption of descriptor wallet creates new private keys, # it should be 2 active and 2 inactive mnemonics assert_equal(found_in_encrypted, 2) assert_equal(mnemonic_count, 4) assert_equal(len(descriptors), 5)
76-92: Refactor repetitive test code to reduce duplicationThe code that checks mnemonic lengths for different wallets is repetitive. Consider refactoring it into a helper function.
Introduce a helper function to reduce duplication:
+def check_mnemonic_length(self, wallet_name, expected_length, descriptor_index=0): + """Helper function to check mnemonic length for a wallet.""" + if self.options.descriptors: + actual_length = len(self.nodes[0].get_wallet_rpc(wallet_name).listdescriptors(True)["descriptors"][descriptor_index]["mnemonic"].split()) + else: + actual_length = len(self.nodes[0].get_wallet_rpc(wallet_name).dumphdinfo()["mnemonic"].split()) + assert_equal(actual_length, expected_length) + if self.options.descriptors: self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() # first descriptor is private key with no mnemonic for CbTx (see node.importprivkey), we use number#1 here instead - assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + self.check_mnemonic_length(self.default_wallet_name, 12, 1) # 12 words by default + self.check_mnemonic_length("wallet_160", 15) # 15 words + self.check_mnemonic_length("wallet_192", 18) # 18 words + self.check_mnemonic_length("wallet_224", 21) # 21 words + self.check_mnemonic_length("wallet_256", 24) # 24 words else: self.nodes[0].createwallet("wallet_256", False, True) # blank HD legacy self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() - assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).dumphdinfo()["mnemonic"].split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").dumphdinfo()["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").dumphdinfo()["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").dumphdinfo()["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"].split()), 24) # 24 words + self.check_mnemonic_length(self.default_wallet_name, 12) # 12 words by default + self.check_mnemonic_length("wallet_160", 15) # 15 words + self.check_mnemonic_length("wallet_192", 18) # 18 words + self.check_mnemonic_length("wallet_224", 21) # 21 words + self.check_mnemonic_length("wallet_256", 24) # 24 words
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (1)
test/functional/wallet_mnemonicbits.py (1)
45-45: This line is now fixed correctlyThe syntax error in the assertion statement has been fixed by properly closing the parentheses.
| if self.options.descriptors: | ||
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | ||
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | ||
| # first descriptor is private key with no mnemonic for CbTx (see node.importprivkey), we use number#1 here instead | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add verification that mnemonics persist after node restart
The test checks mnemonic length for different wallets but doesn't verify that the mnemonic is retained after a node restart.
Add an assertion to verify mnemonic persistence:
if self.options.descriptors:
self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors
self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd()
# first descriptor is private key with no mnemonic for CbTx (see node.importprivkey), we use number#1 here instead
assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default
assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words
assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words
assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words
assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words
+
+ # Verify mnemonics persist after restart
+ wallet_256_mnemonic = self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"]
+ self.restart_node(0)
+ self.nodes[0].loadwallet("wallet_256")
+ assert_equal(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"], wallet_256_mnemonic)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.options.descriptors: | |
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | |
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | |
| # first descriptor is private key with no mnemonic for CbTx (see node.importprivkey), we use number#1 here instead | |
| assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words | |
| if self.options.descriptors: | |
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | |
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | |
| # first descriptor is private key with no mnemonic for CbTx (see node.importprivkey), we use number#1 here instead | |
| assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).listdescriptors(True)["descriptors"][1]["mnemonic"].split()), 12) # 12 words by default | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words | |
| # Verify mnemonics persist after restart | |
| wallet_256_mnemonic = self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"] | |
| self.restart_node(0) | |
| self.nodes[0].loadwallet("wallet_256") | |
| assert_equal(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"], wallet_256_mnemonic) |
knst
force-pushed
the
mnemonic-descriptors
branch
from
29106cc to
684d8f5
Compare
rebased to fix github's CI |
knst
force-pushed
the
mnemonic-descriptors
branch
from
684d8f5 to
90e638a
Compare
src/wallet/rpcwallet.cpp
Outdated
| @@ -2088,7 +2088,7 @@ static RPCHelpMan encryptwallet() | |||
| throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet."); | |||
| } | |||
|
|
|||
| return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; | |||
| return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup or write down the new seed (mnemonic)."; | |||
There was a problem hiding this comment.
682a4fd: I think this message is misleading for non-descriptor wallets - we don't actually change the seed on encryption for them. Also, it's kind of weird we do it for descriptor ones, should probably keep the seed for them too.
There was a problem hiding this comment.
(fixes to unify descriptor / hd wallets is expected to push soon)
Wallet's implementation in Bitcoin Core resets a seed for both cases (descriptor wallets and legacy HD wallets):
// If we are using descriptors, make new descriptors with a new seed
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
SetupDescriptorScriptPubKeyMans();
} else if (auto spk_man = GetLegacyScriptPubKeyMan()) {
// if we are using HD, replace the HD seed with a new one
if (spk_man->IsHDEnabled()) {
if (!spk_man->SetupGeneration(true)) {
return false;
}
}
}
we do it only in case of descriptor wallets (and do not reset if for HD wallets):
// If we are using descriptors, make new descriptors with a new seed
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
SetupDescriptorScriptPubKeyMans("", "");
} else if (auto spk_man = GetLegacyScriptPubKeyMan()) {
// if we are not using HD, generate new keypool
if (spk_man->IsHDEnabled()) {
if (!spk_man->TopUp()) {
return false;
}
}
else {
spk_man->NewKeyPool();
}
}
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
mnemonic-descriptors
branch
from
90e638a to
a8f3f61
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/wallet/walletdb.cpp (2)
338-340: Consider using a dedicated struct for mnemonic storageThe two new maps are fine, but the nested pair<SecureString,SecureString> style is error-prone and hard to read.
A small POD struct with explicitmnemonic/passphrasefields would improve clarity and self-documentation, e.g.:struct MnemonicBlob { SecureString words; SecureString passphrase; }; std::map<std::pair<uint256, CKeyID>, MnemonicBlob> mnemonics;No behavioural change; purely readability/maintainability.
710-723: Duplicate deserialization logic – refactor into a helperThe mnemonic extraction block duplicates code in the encrypted-key path below (742-755) and inflates
ReadKeyValue(). A tiny helper would remove the copy/paste and make future changes less error-prone:auto ReadOptionalMnemonic = [&ssValue](auto&& wordDst, auto&& passDst) { try { ssValue >> wordDst >> passDst; } catch (const std::ios_base::failure&) {} }; ... SecureString mnemonic, passphrase; ReadOptionalMnemonic(mnemonic, passphrase);Same helper can operate on
std::vector<unsigned char>for the encrypted branch.src/wallet/test/util.cpp (1)
29-30: UseSecureString{}instead of string literals for clarity
SetupDescriptorScriptPubKeyMans("", "")implicitly constructs temporarystd::stringobjects.
Passing explicitSecureString{}(or a named constant) makes the intent (“no mnemonic”) clearer and avoids an extra implicit conversion.- wallet->SetupDescriptorScriptPubKeyMans("", ""); + wallet->SetupDescriptorScriptPubKeyMans(SecureString{}, SecureString{});src/qt/test/wallettests.cpp (1)
119-120: Minor: useSecureString{}to mirror method signatureSame remark as in
test/util.cpp: prefer explicit emptySecureStringobjects for readability.- wallet->SetupDescriptorScriptPubKeyMans("", ""); + wallet->SetupDescriptorScriptPubKeyMans(SecureString{}, SecureString{});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/bench/wallet_balance.cpp(1 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/serialize.h(3 hunks)src/wallet/rpcdump.cpp(2 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(9 hunks)src/wallet/scriptpubkeyman.h(4 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.cpp(8 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(1 hunks)src/wallet/wallettool.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_descriptor.py(2 hunks)test/functional/wallet_dump.py(1 hunks)test/functional/wallet_importmulti.py(0 hunks)test/functional/wallet_keypool_hd.py(0 hunks)test/functional/wallet_mnemonicbits.py(2 hunks)
💤 Files with no reviewable changes (2)
- test/functional/wallet_keypool_hd.py
- test/functional/wallet_importmulti.py
✅ Files skipped from review due to trivial changes (5)
- src/wallet/test/coinselector_tests.cpp
- test/functional/wallet_dump.py
- src/bench/wallet_balance.cpp
- src/serialize.h
- src/wallet/rpcwallet.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- test/functional/test_runner.py
- test/functional/wallet_descriptor.py
- src/wallet/wallettool.cpp
- src/wallet/wallet.h
- test/functional/wallet_mnemonicbits.py
- src/wallet/rpcdump.cpp
- src/wallet/walletdb.h
- src/wallet/wallet.cpp
- src/wallet/scriptpubkeyman.cpp
- src/wallet/scriptpubkeyman.h
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
🔇 Additional comments (2)
src/qt/test/addressbooktests.cpp (1)
72-72: Compatibility update for mnemonic support looks goodThe change correctly updates the call to
SetupDescriptorScriptPubKeyMansto include the new parameters for mnemonic and mnemonic passphrase (empty strings in this case) that were added as part of the BIP39 mnemonic support for descriptor wallets.src/wallet/test/wallet_tests.cpp (1)
356-356: Function signature update properly implementedThe modification correctly passes empty strings for the new mnemonic and mnemonic passphrase parameters to align with the updated
SetupDescriptorScriptPubKeyMansmethod signature, ensuring the test continues to work with the new mnemonic support for descriptor wallets.
| bool WalletBatch::WriteCryptedDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const std::vector<unsigned char>& secret, const std::vector<unsigned char>& mnemonic, const std::vector<unsigned char>& mnemonic_passphrase) | ||
| { | ||
| if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, std::make_pair(desc_id, pubkey)), secret, false)) { | ||
| if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY, std::make_pair(desc_id, pubkey)), std::make_pair(secret, std::make_pair(mnemonic, mnemonic_passphrase)), false)) { | ||
| return false; | ||
| } | ||
| EraseIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey))); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encrypted descriptor keys are stored without checksum – repeat the mistake fixed above
WriteCryptedDescriptorKey() persists an encrypted secret but, unlike WriteCryptedKey(), drops the checksum completely. Add the same checksum strategy here (and extend the read logic) so wallet corruption is detectable.
- if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY,
- std::make_pair(desc_id, pubkey)),
- std::make_pair(secret,
- std::make_pair(mnemonic, mnemonic_passphrase)),
- false)) {
+ const uint256 checksum = Hash(secret);
+ if (!WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORCKEY,
+ std::make_pair(desc_id, pubkey)),
+ std::make_tuple(secret, checksum, mnemonic, mnemonic_passphrase),
+ false)) {Don’t forget to validate the checksum when loading.
| bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) | ||
| { | ||
| // hash pubkey/privkey to accelerate wallet load | ||
| std::vector<unsigned char> key; | ||
| key.reserve(pubkey.size() + privkey.size()); | ||
| key.insert(key.end(), pubkey.begin(), pubkey.end()); | ||
| key.insert(key.end(), privkey.begin(), privkey.end()); | ||
|
|
||
| return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(privkey, Hash(key)), false); | ||
| return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(std::make_pair(privkey, Hash(key)), std::make_pair(mnemonic, mnemonic_passphrase)), false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add integrity‐check (checksum) for descriptor keys just like legacy keys
WriteCryptedKey() stores a SHA-256^2 checksum of the encrypted secret so that I/O corruption is detected at load time.
WriteDescriptorKey() writes plain-text descriptor keys but omits any checksum, which means corrupted DB bytes will go unnoticed and will crash only when the key is used.
Recommended fix – store the checksum alongside the tuple (to stay backward-compatible you can append it after the existing fields):
- // hash pubkey/privkey to accelerate wallet load
- std::vector<unsigned char> key;
- key.reserve(pubkey.size() + privkey.size());
- key.insert(key.end(), pubkey.begin(), pubkey.end());
- key.insert(key.end(), privkey.begin(), privkey.end());
-
- return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY,
- std::make_pair(desc_id, pubkey)),
- std::make_pair(std::make_pair(privkey, Hash(key)),
- std::make_pair(mnemonic, mnemonic_passphrase)),
- false);
+ // hash pubkey/privkey to accelerate wallet load and detect corruption
+ std::vector<unsigned char> key;
+ key.reserve(pubkey.size() + privkey.size());
+ key.insert(key.end(), pubkey.begin(), pubkey.end());
+ key.insert(key.end(), privkey.begin(), privkey.end());
+ const uint256 checksum = Hash(key);
+
+ // Layout: (privkey, checksum, mnemonic, passphrase)
+ return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY,
+ std::make_pair(desc_id, pubkey)),
+ std::make_tuple(privkey, checksum, mnemonic, mnemonic_passphrase),
+ false);A corresponding read-side change is required (ssValue >> checksum; and validation similar to legacy flow).
This keeps descriptor and legacy key handling symmetrical and prevents silent DB corruption.
📝 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.
| bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) | |
| { | |
| // hash pubkey/privkey to accelerate wallet load | |
| std::vector<unsigned char> key; | |
| key.reserve(pubkey.size() + privkey.size()); | |
| key.insert(key.end(), pubkey.begin(), pubkey.end()); | |
| key.insert(key.end(), privkey.begin(), privkey.end()); | |
| return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(privkey, Hash(key)), false); | |
| return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(std::make_pair(privkey, Hash(key)), std::make_pair(mnemonic, mnemonic_passphrase)), false); | |
| } | |
| bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) | |
| { | |
| - // hash pubkey/privkey to accelerate wallet load | |
| + // hash pubkey/privkey to accelerate wallet load and detect corruption | |
| std::vector<unsigned char> key; | |
| key.reserve(pubkey.size() + privkey.size()); | |
| key.insert(key.end(), pubkey.begin(), pubkey.end()); | |
| key.insert(key.end(), privkey.begin(), privkey.end()); | |
| + const uint256 checksum = Hash(key); | |
| + | |
| + // Layout: (privkey, checksum, mnemonic, passphrase) | |
| return WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, | |
| - std::make_pair(desc_id, pubkey)), | |
| - std::make_pair(std::make_pair(privkey, Hash(key)), | |
| - std::make_pair(mnemonic, mnemonic_passphrase)), | |
| + std::make_pair(desc_id, pubkey)), | |
| + std::make_tuple(privkey, checksum, mnemonic, mnemonic_passphrase), | |
| false); | |
| } |
| auto it = wss.mnemonics.find(desc_key_pair.first); | ||
| if (it == wss.mnemonics.end()) { | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(desc_key_pair.first.second, desc_key_pair.second, "", ""); | ||
| } else { | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(desc_key_pair.first.second, desc_key_pair.second, it->second.first, it->second.second); | ||
| } | ||
| } | ||
|
|
||
| for (auto desc_key_pair : wss.m_descriptor_crypt_keys) { | ||
| auto spk_man = pwallet->GetScriptPubKeyMan(desc_key_pair.first.first); | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second); | ||
| auto it = wss.crypted_mnemonics.find(desc_key_pair.first); | ||
| if (it == wss.crypted_mnemonics.end()) { | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second, {}, {}); | ||
| } else { | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second, it->second.first, it->second.second); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-check spk_man before dereference
GetScriptPubKeyMan() can return nullptr if the descriptor record is missing or corrupt.
A defensive check avoids a hard crash on a broken wallet DB:
- auto spk_man = pwallet->GetScriptPubKeyMan(desc_key_pair.first.first);
- auto it = wss.mnemonics.find(desc_key_pair.first);
- if (it == wss.mnemonics.end()) {
- ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(...);
+ if (DescriptorScriptPubKeyMan* spk_man =
+ static_cast<DescriptorScriptPubKeyMan*>(pwallet->GetScriptPubKeyMan(desc_key_pair.first.first))) {
+ auto it = wss.mnemonics.find(desc_key_pair.first);
+ spk_man->AddKey(desc_key_pair.first.second,
+ desc_key_pair.second,
+ it == wss.mnemonics.end() ? SecureString() : it->second.first,
+ it == wss.mnemonics.end() ? SecureString() : it->second.second);
+ } else {
+ pwallet->WalletLogPrintf("Descriptor key with id %s has no matching SPKM\n",
+ desc_key_pair.first.first.GetHex());
}Same for the encrypted branch below.
📝 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.
| auto it = wss.mnemonics.find(desc_key_pair.first); | |
| if (it == wss.mnemonics.end()) { | |
| ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(desc_key_pair.first.second, desc_key_pair.second, "", ""); | |
| } else { | |
| ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(desc_key_pair.first.second, desc_key_pair.second, it->second.first, it->second.second); | |
| } | |
| } | |
| for (auto desc_key_pair : wss.m_descriptor_crypt_keys) { | |
| auto spk_man = pwallet->GetScriptPubKeyMan(desc_key_pair.first.first); | |
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second); | |
| auto it = wss.crypted_mnemonics.find(desc_key_pair.first); | |
| if (it == wss.crypted_mnemonics.end()) { | |
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second, {}, {}); | |
| } else { | |
| ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second, it->second.first, it->second.second); | |
| } | |
| // Replace the old unguarded AddKey calls with a null-check on spk_man | |
| if (DescriptorScriptPubKeyMan* spk_man = | |
| static_cast<DescriptorScriptPubKeyMan*>(pwallet->GetScriptPubKeyMan(desc_key_pair.first.first))) { | |
| auto it = wss.mnemonics.find(desc_key_pair.first); | |
| spk_man->AddKey(desc_key_pair.first.second, | |
| desc_key_pair.second, | |
| it == wss.mnemonics.end() ? SecureString() : it->second.first, | |
| it == wss.mnemonics.end() ? SecureString() : it->second.second); | |
| } else { | |
| pwallet->WalletLogPrintf("Descriptor key with id %s has no matching SPKM\n", | |
| desc_key_pair.first.first.GetHex()); | |
| } |
| # there should 5 descriptors in total | ||
| # one of them imported private key for coinbase without mnemonic | ||
| # encryption of descriptor wallet creates new private keys, | ||
| # it should be 2 active and 2 inactive mnemonics |
There was a problem hiding this comment.
I fixed text.
There's one more fix of CI failure.
Strangely, order of the descriptors on CI and on my localhost is different and it caused a failure on CI only; but on localhost it never happens (I run hundreds of times and never see it).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/functional/wallet_mnemonicbits.py (2)
92-99: 🛠️ Refactor suggestionAdd verification that mnemonics persist after node restart
The test verifies mnemonics for different wallets but doesn't check that the mnemonic persists after a node restart, which is a key feature of the implementation.
assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Verify mnemonics persist after restart + wallet_256_mnemonic = self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"] + self.restart_node(0) + self.nodes[0].loadwallet("wallet_256") + assert_equal(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"], wallet_256_mnemonic)
92-99: 🛠️ Refactor suggestionTest upgradetohd with explicit mnemonic as mentioned in PR objectives
The PR objective mentions supporting explicit mnemonic and passphrase in
upgradetohdfor descriptor wallets, but this functionality isn't tested.assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Test upgradetohd with explicit mnemonic + custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" + custom_passphrase = "my_passphrase" + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True, "", False, True) # blank Descriptors + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + descriptors = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").listdescriptors(True)["descriptors"] + for desc in descriptors: + if 'mnemonic' in desc: + assert_equal(desc["mnemonic"], custom_mnemonic) + assert_equal(desc["mnemonic_passphrase"], custom_passphrase)
🧹 Nitpick comments (2)
test/functional/wallet_mnemonicbits.py (2)
72-74: Remove duplicate assertionThe assertion
assert_equal(mnemonic_count, 2)appears twice consecutively, which is redundant.assert_equal(mnemonic_count, 2) - assert_equal(mnemonic_count, 2) assert_equal(len(descriptors), 3)
69-71: Update comments to match implementationThe comments on lines 69-71 about the number of descriptors and mnemonics need to be updated to better reflect the actual implementation and assertions.
- # there should 3 descriptors in total - # one of them imported private key for coinbase without mnemonic - # it should be 2 active and 2 inactive mnemonics + # There should be 3 descriptors in total + # One is the imported private key for coinbase (without mnemonic) + # The other two are active descriptors with mnemonics
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/wallet_mnemonicbits.py
37-37: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
…of descriptors Descriptor with Coinbase private key can be in any position
knst
force-pushed
the
mnemonic-descriptors
branch
from
dbd0ee2 to
dbe7673
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/functional/wallet_mnemonicbits.py (3)
20-39: 🛠️ Refactor suggestionNew helper method for mnemonic extraction looks good but needs a few improvements.
The
get_mnemonicmethod is a good abstraction for retrieving mnemonics from both legacy and descriptor wallets, but has a few issues to address:
- Remove the unnecessary
passstatement on line 28- Replace
assert Falsewith a proper exception for safety with Python optimizationdef get_mnemonic(self, node): if not self.options.descriptors: return node.dumphdinfo()["mnemonic"] mnemonic = None descriptors = node.listdescriptors(True)['descriptors'] for desc in descriptors: if desc['desc'][:4] == 'pkh(': - pass if mnemonic is None: mnemonic = desc['mnemonic'] else: assert_equal(mnemonic, desc['mnemonic']) elif desc['desc'][:6] == 'combo(': assert 'mnemonic' not in desc else: self.log.info(f"unknown type of descriptor: {desc['desc']}") - assert False + raise AssertionError(f"Unknown descriptor type: {desc['desc']}") return mnemonic🧰 Tools
🪛 Ruff (0.8.2)
37-37: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
93-108: 🛠️ Refactor suggestionAdd verification for mnemonics persisting across restarts.
The test verifies mnemonic generation and length but doesn't validate that mnemonics are correctly persisted after a node restart.
if self.options.descriptors: self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Verify mnemonics persist after restart + wallet_256_mnemonic = self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")) + self.restart_node(0) + self.nodes[0].loadwallet("wallet_256") + assert_equal(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")), wallet_256_mnemonic) else: self.nodes[0].createwallet("wallet_256", False, True) # blank HD legacy self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).dumphdinfo()["mnemonic"].split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").dumphdinfo()["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").dumphdinfo()["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").dumphdinfo()["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"].split()), 24) # 24 words + + # Verify mnemonics persist after restart + wallet_256_mnemonic = self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"] + self.restart_node(0) + self.nodes[0].loadwallet("wallet_256") + assert_equal(self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"], wallet_256_mnemonic)
93-108: 🛠️ Refactor suggestionAdd test for
upgradetohdwith explicit mnemonic as mentioned in PR objectives.The PR specifically mentions adding support for providing a mnemonic and passphrase when upgrading to HD using the
upgradetohdRPC, but this functionality isn't tested.if self.options.descriptors: self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Test upgradetohd with explicit mnemonic + self.log.info("Test upgradetohd with explicit mnemonic") + custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" + custom_passphrase = "my_passphrase" + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True, "", False, True) # blank Descriptors + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + descriptors = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").listdescriptors(True)["descriptors"] + for desc in descriptors: + if 'mnemonic' in desc: + assert_equal(desc["mnemonic"], custom_mnemonic) + assert_equal(desc["mnemonic_passphrase"], custom_passphrase) else: self.nodes[0].createwallet("wallet_256", False, True) # blank HD legacy self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).dumphdinfo()["mnemonic"].split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").dumphdinfo()["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").dumphdinfo()["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").dumphdinfo()["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"].split()), 24) # 24 words + + # Test upgradetohd with explicit mnemonic + self.log.info("Test upgradetohd with explicit mnemonic") + custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" + custom_passphrase = "my_passphrase" + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True) # blank HD legacy + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + wallet_info = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").dumphdinfo() + assert_equal(wallet_info["mnemonic"], custom_mnemonic) + assert_equal(wallet_info["mnemonic_passphrase"], custom_passphrase)
🧹 Nitpick comments (1)
test/functional/wallet_mnemonicbits.py (1)
51-76: Fix duplicate assertion and clarify comment.There's a duplicate assertion and a confusing comment in the encryption test section.
# there should 3 descriptors in total # one of them is inactive imported private key for coinbase. It has no mnemonic without mnemonic - # two other should be active and have mnemonic + # two others should be active and have a mnemonic assert_equal(mnemonic_count, 2) - assert_equal(mnemonic_count, 2) assert_equal(len(descriptors), 3)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/wallet_mnemonicbits.py
37-37: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/functional/wallet_mnemonicbits.py (2)
91-107: 🛠️ Refactor suggestionAdd test for explicit mnemonic support in
upgradetohd.PR objectives mention supporting explicit mnemonic and passphrase when upgrading, but this functionality isn't tested. Add a test for this feature.
self.nodes[0].loadwallet("wallet_192") self.nodes[0].loadwallet("wallet_224") +# Test upgradetohd with explicit mnemonic +self.log.info("Test upgradetohd with explicit mnemonic") +custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" +custom_passphrase = "my_passphrase" +if self.options.descriptors: + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True, "", False, True) # blank Descriptors + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + descriptors = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").listdescriptors(True)["descriptors"] + for desc in descriptors: + if 'mnemonic' in desc: + assert_equal(desc["mnemonic"], custom_mnemonic) + assert_equal(desc["mnemonic_passphrase"], custom_passphrase) +else: + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True) # blank HD legacy + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + wallet_info = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").dumphdinfo() + assert_equal(wallet_info["mnemonic"], custom_mnemonic) + assert_equal(wallet_info["mnemonic_passphrase"], custom_passphrase) if self.options.descriptors:
92-99: 🛠️ Refactor suggestionAdd verification of mnemonic persistence after restart.
The test doesn't verify that mnemonics are correctly saved and persist after restarting the node. Add this important verification.
if self.options.descriptors: self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Verify mnemonics persist after restart + wallet_256_mnemonic = self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"] + self.restart_node(0) + self.nodes[0].loadwallet("wallet_256") + assert_equal(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"], wallet_256_mnemonic)
🧹 Nitpick comments (5)
test/functional/wallet_mnemonicbits.py (5)
28-28: Remove unnecessarypassstatement.There's an unnecessary
passstatement that doesn't serve any purpose and should be removed.- if desc['desc'][:4] == 'pkh(': - pass + if desc['desc'][:4] == 'pkh(':
69-71: Fix grammar in comment.The comment about the inactive descriptor has a grammatical error.
- # there should 3 descriptors in total - # one of them is inactive imported private key for coinbase. It has no mnemonic without mnemonic - # two other should be active and have mnemonic + # there should be 3 descriptors in total + # one of them is inactive imported private key for coinbase. It has no mnemonic + # two others should be active and have mnemonics
20-38: Make theget_mnemonichelper more robust.The current implementation only checks the first few characters of the descriptor string, which could be fragile if descriptor format changes. Consider using a more robust method to identify descriptor types.
if desc['desc'][:4] == 'pkh(': - pass if mnemonic is None: mnemonic = desc['mnemonic'] else: assert_equal(mnemonic, desc['mnemonic']) - elif desc['desc'][:6] == 'combo(': + elif desc['desc'].startswith('combo('): assert 'mnemonic' not in desc else: raise AssertionError(f"Unknown descriptor type: {desc['desc']}")
95-99: Use helper method consistently for mnemonic retrieval.On line 95, you use the
get_mnemonichelper method to retrieve the mnemonic, but directly access descriptors in lines 96-99. For consistency, use the helper method throughout.- assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160")).split()), 15) # 15 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192")).split()), 18) # 18 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224")).split()), 21) # 21 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")).split()), 24) # 24 words
93-99: Refactor mnemonic length tests to reduce duplication.The code for testing different mnemonic lengths is duplicated between descriptor and legacy wallet paths. Consider refactoring to reduce this duplication.
if self.options.descriptors: self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() - assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words else: self.nodes[0].createwallet("wallet_256", False, True) # blank HD legacy self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() - assert_equal(len(self.nodes[0].get_wallet_rpc(self.default_wallet_name).dumphdinfo()["mnemonic"].split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").dumphdinfo()["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").dumphdinfo()["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").dumphdinfo()["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").dumphdinfo()["mnemonic"].split()), 24) # 24 words + +# Check mnemonic lengths for all wallets +wallets = [self.default_wallet_name, "wallet_160", "wallet_192", "wallet_224", "wallet_256"] +expected_word_counts = [12, 15, 18, 21, 24] + +for wallet, expected_count in zip(wallets, expected_word_counts): + mnemonic = self.get_mnemonic(self.nodes[0].get_wallet_rpc(wallet)) + assert_equal(len(mnemonic.split()), expected_count, f"Wallet {wallet} has incorrect mnemonic word count")Also applies to: 101-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
There was a problem hiding this comment.
Pull Request Overview
This pull request adds basic mnemonic (BIP39) support for descriptor wallets, enabling descriptor wallets to generate, store, and display mnemonics similar to legacy HD wallets. Key changes include:
- Updating RPC interfaces and test cases to support mnemonics in descriptor wallets.
- Enhancing database and in-memory structures to store mnemonic and mnemonic passphrase.
- Adjusting wallet creation, encryption, and key management flows to accommodate mnemonic usage.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/functional/wallet_importmulti.py | Removed legacy "-usehd=1" flag for descriptor wallets. |
| test/functional/wallet_dump.py | Adjusted extra_args for keypool tests to align with descriptor support. |
| test/functional/wallet_descriptor.py | Updated assertion logic to reflect expected hdmasterfingerprint behavior. |
| src/wallet/wallettool.cpp | Passes empty strings for mnemonic parameters in descriptor setup. |
| src/wallet/walletdb.h / walletdb.cpp | Modified WriteDescriptorKey and WriteCryptedDescriptorKey signatures to include mnemonic fields. |
| src/wallet/wallet.cpp | Updated wallet creation/encryption flows to handle mnemonic command-line args. |
| src/wallet/scriptpubkeyman.h / scriptpubkeyman.cpp | Extended ScriptPubKeyMan to support mnemonic storage, encryption, and retrieval. |
| src/wallet/rpcwallet.cpp | Revised RPC messages following mnemonic integration. |
| src/serialize.h | Adjusted string serialization to include SecureString header inclusion. |
| Various tests (qt, functional, bench) | Updated calls to SetupDescriptorScriptPubKeyMans to comply with new signature. |
Comments suppressed due to low confidence (4)
src/serialize.h:826
- [nitpick] The SecureString serialization functions are commented out. If mnemonics require persistence, consider implementing or clearly documenting the intentional omission of SecureString serialization.
/* template<typename Stream, typename C> void Serialize(Stream& os, const SecureString& str); */
test/functional/wallet_descriptor.py:94
- The assertion for hdmasterfingerprint equality has been changed from '!=' to '=='. Please verify that descriptor wallets are expected to maintain the same hdmasterfingerprint across addresses with mnemonic support.
assert info1['hdmasterfingerprint'] == info2['hdmasterfingerprint']
src/wallet/wallettool.cpp:52
- [nitpick] Passing empty strings as defaults for mnemonic parameters should be reviewed to ensure that this default behavior is intentional and safe; consider documenting why empty values are acceptable here.
wallet_instance->SetupDescriptorScriptPubKeyMans("", "");
src/wallet/scriptpubkeyman.cpp:2497
- The check for multiple mnemonic entries implies that a descriptor wallet should only have one mnemonic. Confirm that this invariant holds in all intended use cases to prevent state inconsistency.
if (m_mnemonics.size() + m_crypted_mnemonics.size() > 1) {
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/functional/wallet_mnemonicbits.py (2)
92-107: 🛠️ Refactor suggestionTest the upgradetohd with explicit mnemonic functionality.
The PR objectives mention adding support for explicit mnemonic and passphrase in
upgradetohdfor descriptor wallets, but this functionality isn't tested.self.log.info("Test upgradetohd with explicit mnemonic") custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" custom_passphrase = "my_passphrase" if self.options.descriptors: self.nodes[0].createwallet("wallet_custom_mnemonic", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) descriptors = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").listdescriptors(True)["descriptors"] for desc in descriptors: if 'mnemonic' in desc: assert_equal(desc["mnemonic"], custom_mnemonic) assert_equal(desc["mnemonic_passphrase"], custom_passphrase) else: self.nodes[0].createwallet("wallet_custom_mnemonic", False, True) # blank HD legacy self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) wallet_info = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").dumphdinfo() assert_equal(wallet_info["mnemonic"], custom_mnemonic) assert_equal(wallet_info["mnemonic_passphrase"], custom_passphrase)
92-99: 🛠️ Refactor suggestionAdd test for mnemonic persistence after node restart.
The test doesn't verify that mnemonics persist after a node restart. This is important to ensure that mnemonics are properly stored and retrieved.
# Verify mnemonics persist after restart wallet_256_mnemonic = self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")) self.restart_node(0) self.nodes[0].loadwallet("wallet_256") assert_equal(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")), wallet_256_mnemonic)
🧹 Nitpick comments (4)
test/functional/wallet_mnemonicbits.py (4)
20-38: Well-structured method for mnemonic retrieval across wallet types.The method effectively handles both legacy and descriptor wallets, but contains a redundant
passstatement.if desc['desc'][:4] == 'pkh(': - pass if mnemonic is None: mnemonic = desc['mnemonic']
28-32: Consider adding robustness for potential mnemonic mismatches.The code correctly checks that all pkh descriptors have the same mnemonic, but doesn't handle the case where no mnemonic is found.
if mnemonic is None: mnemonic = desc['mnemonic'] else: assert_equal(mnemonic, desc['mnemonic']) + if mnemonic is None: + raise AssertionError("No mnemonic found in any descriptor")
70-71: Fix grammar in comment.# there should 3 descriptors in total - # one of them is inactive imported private key for coinbase. It has no mnemonic without mnemonic + # one of them is inactive imported private key for coinbase. It has no mnemonic
92-99: Test robustness: Consider usingget_mnemonicmethod consistently.The test uses direct access to descriptor[0] which could be brittle if descriptor order changes. The
get_mnemonicmethod would be more robust for all checks.self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words - assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160")).split()), 15) # 15 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192")).split()), 18) # 18 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224")).split()), 21) # 21 words + assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")).split()), 24) # 24 words
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
knst
force-pushed
the
mnemonic-descriptors
branch
from
e42d727 to
39b3da7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/functional/wallet_mnemonicbits.py (2)
91-98: 🛠️ Refactor suggestionAdd test for mnemonic persistence after restart.
According to the PR objectives, a key feature is that mnemonics are properly stored and retrieved. Add a test that verifies mnemonics persist after a node restart to ensure database storage is working correctly.
self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Test mnemonic persistence after restart + wallet_256_mnemonic = self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")) + self.restart_node(0) + self.nodes[0].loadwallet("wallet_256") + assert_equal(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")), wallet_256_mnemonic)
91-98: 🛠️ Refactor suggestionAdd test for explicit mnemonic and passphrase with upgradetohd.
The PR objectives specifically mention extending the
upgradetohdRPC to support descriptor wallets with specified mnemonics and passphrases. Add test cases to verify this functionality.assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words + + # Test upgradetohd with explicit mnemonic and passphrase + self.log.info("Test upgradetohd with explicit mnemonic and passphrase") + custom_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" + custom_passphrase = "my_passphrase" + self.nodes[0].createwallet("wallet_custom_mnemonic", False, True, "", False, True) # blank Descriptors + self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").upgradetohd(custom_mnemonic, custom_passphrase) + descriptors = self.nodes[0].get_wallet_rpc("wallet_custom_mnemonic").listdescriptors(True)["descriptors"] + for desc in descriptors: + if 'mnemonic' in desc: + assert_equal(desc["mnemonic"], custom_mnemonic) + assert_equal(desc["mnemonic_passphrase"], custom_passphrase)
🧹 Nitpick comments (1)
test/functional/wallet_mnemonicbits.py (1)
24-36: Handle potential edge case where nopkhdescriptors are found.The current implementation might return
Noneif nopkhdescriptors are found, which could lead to unexpected behavior in tests. Consider adding a validation check after the loop.for desc in descriptors: if desc['desc'][:4] == 'pkh(': if mnemonic is None: mnemonic = desc['mnemonic'] else: assert_equal(mnemonic, desc['mnemonic']) elif desc['desc'][:6] == 'combo(': assert 'mnemonic' not in desc else: raise AssertionError(f"Unknown descriptor type: {desc['desc']}") + # Ensure we found at least one pkh descriptor with a mnemonic + assert mnemonic is not None, "No pkh descriptors with mnemonic found" return mnemonic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/functional/wallet_mnemonicbits.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (2)
test/functional/wallet_mnemonicbits.py (2)
20-36: Good implementation ofget_mnemonichelper method.This helper method cleanly abstracts mnemonic retrieval logic for both descriptor and legacy wallets, ensuring consistent access patterns throughout the test. The implementation also validates that mnemonics are consistent across all pkh descriptors.
45-78: Well-structured test for encrypted wallet mnemonic handling.The test properly verifies that mnemonics are not exposed in regular descriptor listings but are correctly included in private listings. The checks for active vs. inactive descriptors and mnemonic consistency are thorough.
| if self.options.descriptors: | ||
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | ||
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | ||
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words | ||
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make descriptor index access more robust.
The code directly accesses the first descriptor at index 0 for mnemonic verification. This assumes a specific ordering of descriptors that might change across different wallet implementations or versions. Consider using the get_mnemonic helper instead, which already handles finding the right descriptors.
self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors
self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd()
assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default
- assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words
- assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words
- assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words
- assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words
+ assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160")).split()), 15) # 15 words
+ assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192")).split()), 18) # 18 words
+ assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224")).split()), 21) # 21 words
+ assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")).split()), 24) # 24 words📝 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 self.options.descriptors: | |
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | |
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_160").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 15) # 15 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_192").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 18) # 18 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_224").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 21) # 21 words | |
| assert_equal(len(self.nodes[0].get_wallet_rpc("wallet_256").listdescriptors(True)["descriptors"][0]["mnemonic"].split()), 24) # 24 words | |
| if self.options.descriptors: | |
| self.nodes[0].createwallet("wallet_256", False, True, "", False, True) # blank Descriptors | |
| self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd() | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12) # 12 words by default | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160")).split()), 15) # 15 words | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192")).split()), 18) # 18 words | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224")).split()), 21) # 21 words | |
| assert_equal(len(self.get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")).split()), 24) # 24 words |
🤖 Prompt for AI Agents
In test/functional/wallet_mnemonicbits.py around lines 91 to 98, the code
directly accesses the first descriptor at index 0 to verify mnemonics, which
assumes a fixed descriptor order that may vary. To fix this, replace the direct
index access with calls to the existing get_mnemonic helper function for each
wallet, ensuring the mnemonic is retrieved robustly regardless of descriptor
ordering.
There was a problem hiding this comment.
no, other wallets does not have imported coinbase private key. They are not supposed to have any extra descriptor. Let it me as it is.
There was a problem hiding this comment.
@knst Understood! I'll leave the code as is. Since these test wallets don't have the imported coinbase private key and maintain a simpler descriptor structure, directly accessing the first descriptor at index 0 is appropriate and more straightforward in this case.
Issue being fixed or feature implemented
Descriptor wallets doesn't support yet mnemonics (BIP39), which are supported only by legacy (non-descriptor) wallet.
What was done?
This PR adds basic support BIP39 to descriptor wallets:
upgradetohdnow support descriptor wallets and let to set specifict mnemonic with passphrase to newly created walletlistdescriptorsif existsHow Has This Been Tested?
Functional test
wallet_mnemonicbits.pysupport--descriptorsnowFunctional test
wallet_upgradetohd.pyis updated alsoBreaking Changes
N/A
Checklist: