feat: create extra spk by default for mobile derivation path of CJ #6836
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds an optional boolean Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/rpc/backup.cpp (1)
1979-1986: Doc mismatch: coinjoin should not be “only for active descriptors.”CJ descriptors are intentionally inactive by default; the field should be documented as present when the type is known, regardless of activity.
- {RPCResult::Type::BOOL, "coinjoin", /*optional=*/true, "True if this descriptor is used to generate CoinJoin addresses. False if this descriptor is used to generate receiving addresses; defined only for active descriptors"}, + {RPCResult::Type::BOOL, "coinjoin", /*optional=*/true, "True if this descriptor derives CoinJoin addresses (purpose 9'); present when the descriptor output type is known."},
🧹 Nitpick comments (7)
src/wallet/scriptpubkeyman.h (1)
150-155: Good addition; please document the new key class.Add a short comment explaining semantics (External, Internal, CoinJoin) and intended activation (CJ inactive by default) to aid future readers.
+// Distinguishes descriptor categories: +// - External: receiving addresses (change=0) +// - Internal: change addresses (change=1) +// - CoinJoin: mobile CJ funds (purpose 9'), created by default but kept inactive enum class InternalKey { External, Internal, CoinJoin, };src/wallet/rpc/wallet.cpp (1)
171-172: Terminology nit: capitalize CoinJoin in help text.- "how many new keys are pre-generated for internal use (used for change outputs and mobile coinjoin, only appears if the wallet is using this feature, otherwise external keys are used)"}, + "how many new keys are pre-generated for internal use (used for change outputs and mobile CoinJoin; appears only if the wallet is using this feature, otherwise external keys are used)"},test/functional/wallet_listdescriptors.py (2)
49-51: Safe dict access in assertions; drop noisy logging.Use dict.get to avoid KeyError if schema changes, and remove debug log.
- assert_equal(3, len(result['descriptors'])) - assert_equal(2, len([d for d in result['descriptors'] if d['active']])) - self.log.info(f"result: {result['descriptors']}") + assert_equal(3, len(result['descriptors'])) + assert_equal(2, len([d for d in result['descriptors'] if d.get('active')]))
52-54: Use.getand simplify coinjoin check (RUF019).- assert_equal(1, len([d for d in result['descriptors'] if d['internal']])) - assert_equal(1, len([d for d in result['descriptors'] if 'coinjoin' in d and d['coinjoin']])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('internal')])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('coinjoin')]))test/functional/wallet_mnemonicbits.py (1)
55-58: Make counts robust; avoid reliance on schema details.Keep counts but tolerate descriptors lacking ‘internal’.
- assert_equal(mnemonic_count, 3) + assert_equal(mnemonic_count, 3) assert_equal(cb_count, 1) - assert_equal(len(descriptors), 4) + assert_equal(len(descriptors), 4)src/wallet/rpc/backup.cpp (1)
1960-2065: Longer-term: avoid string matching; carry CJ metadata.Prefer tagging CJ at creation (e.g., store a flag alongside WalletDescriptor) and expose it here, rather than parsing strings.
src/wallet/wallet.cpp (1)
3827-3836: Switch on bool is unconventional; prefer if/else for clarityMinor readability nit; avoids odd switch(true/false).
- switch (internal) { - case true: - m_internal_spk_managers = spk_man; - break; - - case false: - m_external_spk_managers = spk_man; - break; - } + if (internal) { + m_internal_spk_managers = spk_man; + } else { + m_external_spk_managers = spk_man; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(6 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cppsrc/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_listdescriptors.pytest/functional/wallet_mnemonicbits.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.203Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: 2025-09-02T07:34:28.203Z
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.203Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/rpc/backup.cpptest/functional/wallet_mnemonicbits.pysrc/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
🧬 Code graph analysis (4)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (2)
SetupDescriptorGeneration(2077-2129)SetupDescriptorGeneration(2077-2077)
src/wallet/rpc/backup.cpp (1)
src/script/descriptor.cpp (1)
nullopt(657-657)
test/functional/wallet_mnemonicbits.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🪛 Ruff (0.12.2)
test/functional/wallet_listdescriptors.py
53-53: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/wallet/scriptpubkeyman.h (1)
584-586: Signature change looks right; confirm all callers updated.Given the switch from bool to enum, ensure all invocations now pass InternalKey::{External,Internal,CoinJoin}. Compile will catch most, but please double-check wallet setup paths.
src/wallet/scriptpubkeyman.cpp (1)
2077-2079: Confirm all calls updated—no remaining boolean overloads
Grep shows only one invocation insrc/wallet/wallet.cpp:3800, passing the localinternalvariable, which is already of typeInternalKey(notbool). No literaltrue/falseor stale overloads remain.src/wallet/wallet.cpp (3)
2357-2369: Use external SPKM directly for receive addresses — OKStraightforward; no behavior change beyond comment cleanup.
3789-3806: Creating 3 SPKMs and activating only External/Internal — OKMatches goal: CJ SPKM exists for discovery but is inactive for address issuance.
3838-3844: De-aliasing active SPKM pointers when reassigning — OKPrevents both pointers referencing the same manager; good defensive state management.
src/wallet/scriptpubkeyman.cpp
Outdated
| std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, Params().ExtCoinType()); | ||
| std::string desc_suffix = "/*)"; | ||
|
|
||
| std::string internal_path = internal ? "/1" : "/0"; | ||
| std::string internal_path = (internal == InternalKey::Internal) ? "/1" : "/0"; | ||
| std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
CoinJoin derivation path likely mismatched with Android “9'/1'/0'” expectation
Code uses Params().ExtCoinType() for CJ coin type, but RPC/tests reportedly detect CJ via path “/9'/1'/0'”. On mainnet ExtCoinType() != 1 (Dash is 5), so generated CJ descriptor would be “9'/5'/0'…”, breaking visibility with Android CJ funds. Fix by forcing coin type 1 for CJ (or align detection to ExtCoinType()).
Apply:
- std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, Params().ExtCoinType());
+ const int coin_type = (internal == InternalKey::CoinJoin) ? 1 : Params().ExtCoinType();
+ std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, coin_type);Also confirm listdescriptors’ CJ detection logic matches this choice:
If Android CJ uses a different coin_type per network, wire it explicitly and update tests.
🏁 Script executed:
#!/bin/bash
rg -n "listdescriptors|coinjoin|9'/1'|9'\/1'" -C3Length of output: 200
🏁 Script executed:
#!/bin/bash
# Confirm CJ descriptor generation uses Params().ExtCoinType()
rg -n "desc_prefix" -C5 src/wallet/scriptpubkeyman.cpp
# Search for listdescriptors RPC command occurrences
rg -n "listdescriptors" -C5 .
# Inspect functional tests for CJ path detection (9'/1)
rg -n "9'/1" -C5 testLength of output: 25750
Force CJ descriptor coin type to 1
Functional tests (wallet_mnemonicbits.py:50) detect CJ descriptors inactive via the substring "/9'/1" in the descriptor. On mainnet, Params().ExtCoinType() returns 5, so CJ descriptors are currently generated with 9'/5…, breaking both the functional test detection and Android’s “9'/1'/0'” assumption. Update the code as follows:
- std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, Params().ExtCoinType());
+ const int coin_type = internal == InternalKey::CoinJoin ? 1 : Params().ExtCoinType();
+ std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, coin_type);📝 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.
| std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, Params().ExtCoinType()); | |
| std::string desc_suffix = "/*)"; | |
| std::string internal_path = internal ? "/1" : "/0"; | |
| std::string internal_path = (internal == InternalKey::Internal) ? "/1" : "/0"; | |
| std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix; | |
| const int coin_type = internal == InternalKey::CoinJoin ? 1 : Params().ExtCoinType(); | |
| std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, coin_type); | |
| std::string desc_suffix = "/*)"; | |
| std::string internal_path = (internal == InternalKey::Internal) ? "/1" : "/0"; | |
| std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix; |
🤖 Prompt for AI Agents
In src/wallet/scriptpubkeyman.cpp around lines 2102 to 2107, the CJ (CoinJoin)
descriptor uses Params().ExtCoinType() which on mainnet yields 5, causing CJ
descriptors to be generated as "9'/5" and breaking tests and Android
assumptions; change the code to force coin type to 1 when internal ==
InternalKey::CoinJoin (e.g. compute a coinType variable = (internal ==
InternalKey::CoinJoin ? 1 : Params().ExtCoinType()) and use that coinType in the
desc_prefix construction), leaving other path parts unchanged.
knst
added
RPC
knst
force-pushed
the
feat-cj-mobile-derivation-inactive-descriptor
branch
from
5f0fcf9 to
9d7be36
Compare
|
@coderabbitai help to prepare release notes for this PR See PR description and #6745 for information source UPD: it's a bit too long; see as a reference release notes for v21: https://github.com/dashpay/dash/blob/master/doc/release-notes/dash/release-notes-21.0.0.md It should be just one paragraph of text; maximum 2 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you prepare release notes for this PR. Let me first gather some additional information about the issue and PR details. Based on the information gathered and the PR changes, here are the release notes for this feature: Release Notes - Mobile CoinJoin Descriptor SupportNew Feature: Enhanced CoinJoin Compatibility Between Mobile and Desktop WalletsProblem Solved:
Changes:
API Changes:
Technical Details:
User Impact:
For Developers:
This change ensures better interoperability between different Dash wallet implementations while maintaining backward compatibility for existing wallets. 🏁 Script executed: Length of output: 2098 Perfect! Now I have the complete context. Here are the refined release notes for PR #6836: Release Notes - Mobile CoinJoin Descriptor SupportNew Feature: Enhanced CoinJoin Compatibility Between Mobile and Desktop WalletsProblem Solved:
What Changed:
API Enhancements:
User Benefits:
Technical Implementation:
Compatibility:
This enhancement significantly improves the user experience when using multiple Dash wallet applications, ensuring that CoinJoin privacy features work consistently across the entire ecosystem. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@knst, I'll prepare concise release notes following the v21 format. Let me first check the reference format you mentioned: Based on the v21 release notes format, here are the concise release notes for PR #6836: Mobile CoinJoin CompatibilityFixed an issue where CoinJoin funds mixed in Dash Android wallet were invisible when importing the mnemonic into Dash Core or other desktop wallets. New descriptor wallets now include an additional default descriptor for mobile CoinJoin funds, ensuring seamless wallet migration and complete fund visibility across different Dash wallet implementations. This is a breaking change that increases the default number of descriptors from 2 to 3 on mainnet (internal, external, mobile CoinJoin) for newly created descriptor wallets only - existing wallets are unaffected. 🏁 Script executed: Length of output: 2241 Perfect! Now I can see the format. Here are the concise release notes following the v21 style: Mobile CoinJoin CompatibilityFixed an issue where CoinJoin funds mixed in Dash Android wallet were invisible when importing the mnemonic into Dash Core or other desktop wallets. New descriptor wallets now include an additional default descriptor for mobile CoinJoin funds, ensuring seamless wallet migration and complete fund visibility across different Dash wallet implementations. This is a breaking change that increases the default number of descriptors from 2 to 3 on mainnet (internal, external, mobile CoinJoin) for newly created descriptor wallets only - existing wallets are unaffected. The 🧠 Learnings used |
knst
force-pushed
the
feat-cj-mobile-derivation-inactive-descriptor
branch
3 times, most recently
from
018a42d to
bdb92b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/release-notes-6835.md (1)
1-22: Fix wrong PR reference and capture regtest delta in breaking-change note
- The reference “(#6835)” appears incorrect for this PR; update to “(#6836)” and optionally include the original issue “(#6745)”.
- The breaking-change paragraph omits the regtest increase (3 → 4); add it for completeness.
Apply:
- - This is a breaking change that increases the default number of descriptors - from 2 to 3 on mainnet (internal, external, mobile CoinJoin) for newly created - descriptor wallets only - existing wallets are unaffected. + - Breaking change (new descriptor wallets only): + - Mainnet: default descriptors increase from 2 to 3 (internal, external, mobile CoinJoin). + - Regtest: default descriptors increase from 3 to 4 (internal, external, mobile CoinJoin, cbtx private key). + Existing wallets are unaffected. @@ -(#6835) +(#6836, #6745)
♻️ Duplicate comments (1)
test/functional/wallet_mnemonicbits.py (1)
50-51: Prefer dict.get for clarity and to satisfy Ruff RUF019Equivalent and cleaner.
- assert_equal(desc['active'], ("coinjoin" not in desc or not desc['coinjoin'])) + assert_equal(desc['active'], not desc.get('coinjoin', False))
🧹 Nitpick comments (3)
test/functional/wallet_mnemonicbits.py (1)
55-57: Update adjacent comment to match new expectations (3 mnemonics, 4 descriptors)The comment above Lines 52–54 still describes the pre-change state (“3 descriptors total … Two other should be active”). Please align it with the new assertions (mnemonic_count == 3, len(descriptors) == 4). No code change needed here.
test/functional/wallet_listdescriptors.py (1)
53-53: Use dict.get to simplify the coinjoin filterShorter and avoids an unnecessary key check.
- assert_equal(1, len([d for d in result['descriptors'] if 'coinjoin' in d and d['coinjoin']])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('coinjoin')]))src/wallet/wallet.cpp (1)
3790-3807: Minor: avoid “internal” as loop var for the InternalKey enumReduces ambiguity with the existing boolean “internal” semantics elsewhere.
- for (auto internal : {InternalKey::External, InternalKey::Internal, InternalKey::CoinJoin}) { + for (auto ikey : {InternalKey::External, InternalKey::Internal, InternalKey::CoinJoin}) { @@ - spk_manager->SetupDescriptorGeneration(master_key, mnemonic, mnemonic_passphrase, internal); + spk_manager->SetupDescriptorGeneration(master_key, mnemonic, mnemonic_passphrase, ikey); @@ - if (internal != InternalKey::CoinJoin) { - AddActiveScriptPubKeyMan(id, internal == InternalKey::Internal); + if (ikey != InternalKey::CoinJoin) { + AddActiveScriptPubKeyMan(id, ikey == InternalKey::Internal);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/release-notes-6835.md(1 hunks)src/wallet/rpc/backup.cpp(3 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(2 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/wallet/scriptpubkeyman.h
- src/wallet/scriptpubkeyman.cpp
- src/wallet/rpc/wallet.cpp
- src/wallet/rpc/backup.cpp
🧰 Additional context used
📓 Path-based instructions (3)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6835.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_mnemonicbits.pytest/functional/wallet_listdescriptors.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/wallet.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
doc/release-notes-6835.mdtest/functional/wallet_mnemonicbits.pysrc/wallet/wallet.cpp
🧬 Code graph analysis (2)
test/functional/wallet_mnemonicbits.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🪛 Ruff (0.12.2)
test/functional/wallet_listdescriptors.py
53-53: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (2)
src/wallet/wallet.cpp (2)
3790-3807: LGTM: create CoinJoin SPKM but keep it inactive by defaultCreating the CJ SPKM and not activating it (only External/Internal active) matches the intended behavior and tests relying on listdescriptors. No functional issues spotted.
3790-3807: Fetched backup.cpp context for manual verification. No critical issues found: all GetAllScriptPubKeyMans call sites perform non-mutating or safe migrations/reporting (descriptor backup, cache upgrade, signing/fill PSBT across all keyrings, logging, and rescan time determination). Address generation, keypool top-up, and notifications exclusively use GetActiveScriptPubKeyMans.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/hdchain.cpp (1)
206-206: Fix formatting to comply with Clang format check.The pipeline indicates formatting differences detected by clang-format.
Apply this diff to fix the formatting:
- return strprintf("m/%d'/%d'/%d'/%d/%d", BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), nAccountIndex, nChangeIndex, extPubKey.nChild); + return strprintf("m/%d'/%d'/%d'/%d/%d", BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), nAccountIndex, nChangeIndex, + extPubKey.nChild);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/wallet/hdchain.cpp(1 hunks)src/wallet/hdchain.h(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/hdchain.hsrc/wallet/hdchain.cppsrc/wallet/scriptpubkeyman.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/hdchain.hsrc/wallet/hdchain.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/scriptpubkeyman.cpp
🧬 Code graph analysis (2)
src/wallet/hdchain.cpp (2)
src/wallet/hdchain.h (3)
BIP32_PURPOSE_STANDARD(147-149)nAccountIndex(131-131)nChangeIndex(132-132)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/wallet/scriptpubkeyman.cpp (2)
src/wallet/hdchain.h (2)
BIP32_PURPOSE_FEATURE(145-145)BIP32_PURPOSE_STANDARD(147-149)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
🪛 GitHub Actions: Clang Diff Format Check
src/wallet/hdchain.cpp
[error] 203-208: Clang format differences detected by clang-format-diff.py. Formatting changes applied would modify src/wallet/hdchain.cpp; please run clang-format (or update CI to accept formatted changes).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (5)
src/wallet/hdchain.h (1)
144-148: LGTM!The new constants
BIP32_PURPOSE_FEATURE(9) andBIP32_PURPOSE_STANDARD(44) align with the PR objectives to support mobile CoinJoin with purpose 9 and standard derivation with purpose 44. These constants will replace hardcoded values in descriptor path construction.src/wallet/hdchain.cpp (1)
206-206: LGTM!The path format change from hardcoded
"m/44'/%d'/%d'/%d/%d"to dynamic"m/%d'/%d'/%d'/%d/%d"withBIP32_PURPOSE_STANDARDis correct. This aligns with the new constants defined inhdchain.hand supports variable purpose codes for different derivation paths.src/wallet/scriptpubkeyman.cpp (3)
2077-2077: LGTM!The function signature change from
bool internaltoInternalKey internalproperly aligns with the new three-way categorization (External, Internal, CoinJoin) introduced by this PR.
2105-2105: LGTM!The conditional logic correctly maps the
InternalKeyenum to the appropriate path component: Internal becomes "/1" and External (including CoinJoin) becomes "/0". This properly differentiates internal change addresses from external receiving addresses.
2102-2102: **Force CJ descriptor coin type to 1.**Based on the web search results, I can see that Dash uses coin type 5 for BIP44 derivation paths. However, the past review comment indicates that functional tests detect CJ descriptors via the substring "/9'/1" and Android CJ uses "9'/1'/0'". This creates a mismatch sinceParams().ExtCoinType()returns 5 on mainnet, not 1.Based on the previous review feedback and the retrieved learnings showing that CJ descriptors are detected by matching "/9'/1" patterns, the current code using
Params().ExtCoinType()would generate "9'/5'" paths on mainnet instead of the expected "9'/1'" paths. This breaks compatibility with both functional tests and Android CJ wallets.Apply this fix to force coin type 1 for CJ descriptors:
- std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, Params().ExtCoinType()); + const int coin_type = (internal == InternalKey::CoinJoin) ? 1 : Params().ExtCoinType(); + std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, coin_type);
knst
removed
the
Needs Release Notes
src/wallet/rpc/backup.cpp
Outdated
| std::string match = strprintf("/9'/%s'/0'", Params().ExtCoinType()); | ||
| bool is_cj = descriptor.size() > 5 && descriptor.find(match) != std::string::npos; |
There was a problem hiding this comment.
let's avoid these magic numbers
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
This differs a lot from the proposed plan #6745 (comment). Why?
src/wallet/scriptpubkeyman.h
Outdated
| enum class InternalKey | ||
| { | ||
| External, | ||
| Internal, | ||
| CoinJoin, | ||
| }; |
There was a problem hiding this comment.
nit: These names are confusing imo. Maybe smth like
| enum class InternalKey | |
| { | |
| External, | |
| Internal, | |
| CoinJoin, | |
| }; | |
| enum class PathDerivationType | |
| { | |
| BIP44_External, | |
| BIP44_Internal, | |
| DIP0009_CoinJoin, | |
| }; |
we discussed it on weekly calls, there are several reasons to do. Firstly, I'd like to keep compatibility between Dash Core & 3rd party wallets, include hardware one. If we will have custom derivation path for mixing, same as mobile, it means that these funds won't be easily available at any other 3rd party wallets, it creates "vendor lock" to DCG projects (android wallet, Dash Core). That's why I don't like to use for CJ on Dash Core custom mobile path same as it is done in mobile android wallet. So far as we have mnemonic in Dash Core; it is great if these mnemonic can be easily imported in any 3rd party wallet and it "just works". Though, this PR is aim to add a potentially at least 1 wallet beside Android Wallet that could let user to get access to his "lost" funds if user will mix his funds on mobile and it does. Secondly, Dash Core currently has 2 competing implementation of HD wallets (legacy wallets and descriptor wallets). Both of them them (legacy & descriptor) are backported from bitcoin*(1)(2), but legacy wallets are deprecated in bitcoin and in the latest versions you can't even create legacy wallet anymore. I wish Dash Core with mnemonic for Descriptor wallets (#6570) has been released long time ago, so we could start deprecating legacy wallets; and create descriptor wallets, but we haven't had a major releases for awhile, so, it's a bit slower that it is. (1). legacy wallets have our own implementation HD, not fully backported. |
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
This differs a lot from the proposed plan #6745 (comment). Why?
we discussed it on weekly calls, there are several reasons to do.
Firstly, I'd like to keep compatibility between Dash Core & 3rd party wallets, include hardware one. If we will have custom derivation path for mixing, same as mobile, it means that these funds won't be easily available at any other 3rd party wallets, it creates "vendor lock" to DCG projects (android wallet, Dash Core). That's why I don't like to use for CJ on Dash Core custom mobile path same as it is done in mobile android wallet.
So far as we have mnemonic in Dash Core; it is great if these mnemonic can be easily imported in any 3rd party wallet and it "just works".
Though, this PR is aim to add a potentially at least 1 wallet beside Android Wallet that could let user to get access to his "lost" funds if user will mix his funds on mobile and it does.
Secondly, Dash Core currently has 2 competing implementation of HD wallets (legacy wallets and descriptor wallets). Both of them them (legacy & descriptor) are backported from bitcoin*(1)(2), but legacy wallets are deprecated in bitcoin and in the latest versions you can't even create legacy wallet anymore.
I wish Dash Core with mnemonic for Descriptor wallets (#6570) has been released long time ago, so we could start deprecating legacy wallets; and create descriptor wallets, but we haven't had a major releases for awhile, so, it's a bit slower that it is. Due that I don't really want to add new feature to legacy wallets and complicate implementation as proposed in #6745, instead it's much easier to implement & support just adding +1 extra descriptor in descriptor wallets. The patch minimal (50 lines) but it works just fine.
(1). legacy wallets have our own implementation HD, not fully backported. (2). both descriptor & legacy wallets have our own implementation of mnemonic, it's not backported feature.
Makes sense, thanks!
import mnemonic to Dash Core. Funds (including CJ transactions) expect to visible.
I'm creating an empty descriptor wallet and using "upgradetohd" rpc to import mnemonic I use on mobile. I then do "rescanblockchain" but I see no CJ txes once its done, only "Sent to"... What steps were you using exactly to test this?
Maybe there is a bug, I will re-check. I tested some not the very final version, maybe introduced some new issue. I will re-test and check if it works. |
Probably you did everything right. Implementation has lost Steps to reproduce: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/wallet_listdescriptors.py (1)
52-53: Test assertions correctly validate the new CoinJoin descriptor.The assertions properly verify that exactly one descriptor has the
internalflag and one has thecoinjoinflag, which aligns with the expected structure (3 total descriptors: external, internal, and CoinJoin). Based on learnings, CoinJoin descriptors are intentionally inactive.For a more concise style, consider using
dict.get():- assert_equal(1, len([d for d in result['descriptors'] if 'internal' in d and d['internal']])) - assert_equal(1, len([d for d in result['descriptors'] if 'coinjoin' in d and d['coinjoin']])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('internal')])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('coinjoin')]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wallet/rpc/backup.cpp(3 hunks)test/functional/wallet_listdescriptors.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/rpc/backup.cpp
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_listdescriptors.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
🧬 Code graph analysis (1)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🪛 Ruff (0.13.1)
test/functional/wallet_listdescriptors.py
52-52: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
53-53: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (2)
test/functional/wallet_listdescriptors.py (2)
49-49: LGTM!The descriptor count increase from 2 to 3 correctly reflects the addition of the CoinJoin descriptor alongside the existing external and internal descriptors.
51-51: LGTM!The debug logging helps verify the descriptor output structure during test execution.
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
feat-cj-mobile-derivation-inactive-descriptor
branch
from
d846c53 to
6d64b87
Compare
rebased due to conflicts with bitcoin backports PR |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wallet/scriptpubkeyman.cpp (1)
2102-2108: CJ path uses wrong coin_type; must be 1 to match Android m/9'/1'/…Current code builds CJ descriptors with
9'/ExtCoinType'/…(Dash mainnet ExtCoinType=5), which will not match Android’s mobile CoinJoin pathm/9'/1'/…. Funds mixed on mobile won’t be detected.Apply:
- std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, type == PathDerivationType::DIP0009_CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, Params().ExtCoinType()); + const int coin_type = (type == PathDerivationType::DIP0009_CoinJoin) ? 1 : Params().ExtCoinType(); + std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, type == PathDerivationType::DIP0009_CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, coin_type);Also ensure listdescriptors’ CJ detection logic matches this choice.
Based on learnings
🧹 Nitpick comments (4)
test/functional/wallet_mnemonicbits.py (1)
50-50: Consider simplifying the activity assertion.The current assertion
("coinjoin" not in desc or not desc['coinjoin'])is logically equivalent tonot desc.get('coinjoin', False), which is more concise and idiomatic.Apply this diff for a cleaner assertion:
- assert_equal(desc['active'], ("coinjoin" not in desc or not desc['coinjoin'])) + assert_equal(desc['active'], not desc.get('coinjoin', False))src/wallet/rpc/backup.cpp (1)
1980-1980: Consider clarifying the field description.The description "defined only if it is True" is technically correct but slightly awkward. Consider rephrasing to match the pattern used for the
internalfield or be more explicit about the detection criteria.For example:
- {RPCResult::Type::BOOL, "coinjoin", /*optional=*/true, "True if this descriptor is used to generate CoinJoin addresses; defined only if it is True."}, + {RPCResult::Type::BOOL, "coinjoin", /*optional=*/true, "True if this descriptor uses the CoinJoin derivation path (m/9'/cointype'/4'/0'). Only present when true."},test/functional/wallet_listdescriptors.py (1)
49-53: Add a sanity check that CJ descriptor matches the Android path (9'/1')Counting is fine, but we can catch regressions like coin_type drift by asserting the path. Example:
result = node.get_wallet_rpc('w3').listdescriptors() @@ -assert_equal(3, len(result['descriptors'])) +assert_equal(3, len(result['descriptors'])) @@ self.log.info(f"result: {result['descriptors']}") cj = [d for d in result['descriptors'] if d.get('coinjoin')] assert_equal(1, len(cj)) +assert "'/9'/1'/" in cj[0]['desc'], "CJ descriptor should use m/9'/1'/… (Android mobile path)"src/wallet/scriptpubkeyman.h (1)
150-156: Enum addition looks goodNames are clear and scoped; matches prior naming suggestion. Consider a brief doc comment per value for discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
doc/release-notes-6835.md(1 hunks)src/wallet/hdchain.cpp(1 hunks)src/wallet/hdchain.h(1 hunks)src/wallet/rpc/backup.cpp(3 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(2 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/wallet.cppsrc/wallet/hdchain.cppsrc/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/hdchain.h
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6835.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_mnemonicbits.pytest/functional/wallet_listdescriptors.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/wallet.cppdoc/release-notes-6835.mdsrc/wallet/scriptpubkeyman.cppsrc/wallet/rpc/backup.cpp
🧬 Code graph analysis (6)
src/wallet/hdchain.cpp (2)
src/wallet/hdchain.h (3)
BIP32_PURPOSE_STANDARD(147-149)nAccountIndex(131-131)nChangeIndex(132-132)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/wallet/scriptpubkeyman.cpp (2)
src/wallet/hdchain.h (2)
BIP32_PURPOSE_FEATURE(145-145)BIP32_PURPOSE_STANDARD(147-149)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/wallet/rpc/backup.cpp (1)
src/wallet/hdchain.h (1)
BIP32_PURPOSE_FEATURE(145-145)
test/functional/wallet_mnemonicbits.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (2)
SetupDescriptorGeneration(2077-2131)SetupDescriptorGeneration(2077-2077)
🔇 Additional comments (9)
src/wallet/hdchain.h (1)
144-148: LGTM! Purpose constants clearly defined.The new constants for BIP32 purpose codes are well-named, correctly valued (9 for DIP9 CoinJoin, 44 for standard BIP44), and appropriately typed. The comments provide clear context for their usage.
src/wallet/rpc/wallet.cpp (1)
171-171: LGTM! Help text accurately reflects mobile CoinJoin usage.The updated help text for
keypoolsize_hd_internalcorrectly documents that the internal keypool serves both change outputs and mobile CoinJoin, improving clarity for users.doc/release-notes-6835.md (2)
21-21: Verify the PR reference number.The release notes reference "(#6835)" but the PR objectives indicate this is PR #6836. Please confirm the correct PR number.
Based on learnings
1-20: Release notes are clear and comprehensive.The release notes effectively describe:
- The mobile CoinJoin compatibility issue and fix
- The breaking change scope (new wallets only)
- The RPC updates
The content aligns well with the PR objectives and provides users with the necessary context.
src/wallet/hdchain.cpp (1)
206-207: Verify that the updated key path format is backward compatible.The key path format changed from an implicit
m/44'/%d'/%d'/%d/%dto an explicitm/%d'/%d'/%d'/%d/%dwithBIP32_PURPOSE_STANDARDas the first parameter. While this appears to be a display-only function (returns string), ensure that no code parses or relies on the previous format structure.Run the following script to check for potential parsing or format dependencies:
src/wallet/rpc/backup.cpp (2)
7-7: LGTM!The include is necessary for accessing
Params().ExtCoinType()used in the CoinJoin detection logic below.
2041-2047: Verify the CoinJoin derivation path pattern matches the mobile wallet specification.The detection pattern uses
/%d'/%s'/4'/0'withBIP32_PURPOSE_FEATURE(9) andExtCoinType(). However, the PR objectives state the mobile CoinJoin derivation path is m/9'/1'/0', not m/9'/cointype'/4'/0'.Please verify:
- The correct mobile CJ derivation path specification
- Whether the pattern should include the "4'" component
- The format of
ExtCoinType()return value (string vs numeric)Run the following script to check how CoinJoin descriptors are created and what path they use:
Based on learnings: CJ descriptors with derivation path "9'/1" are intentionally inactive, which suggests the path might not include all components in the match pattern.
src/wallet/wallet.cpp (1)
3790-3807: Tri‑descriptor setup with inactive CJ is correctLooping over External, Internal, CoinJoin and activating only the first two matches the intended behavior for new descriptor wallets (CJ inactive by default).
src/wallet/scriptpubkeyman.h (1)
585-585: API change: confirm all call sites updatedSignature switch to PathDerivationType is good; please verify no remaining uses of the old boolean form.
| # there should 3 descriptors in total | ||
| # One of them is inactive imported private key for coinbase. It has no mnemonic | ||
| # Two other should be active and have mnemonic | ||
| assert_equal(mnemonic_count, 2) | ||
| assert_equal(mnemonic_count, 3) | ||
| assert_equal(cb_count, 1) | ||
| assert_equal(len(descriptors), 3) | ||
| assert_equal(len(descriptors), 4) |
There was a problem hiding this comment.
Update comment to reflect three mnemonic descriptors.
The comment states "Two other should be active and have mnemonic" but the code now expects 3 mnemonic descriptors (line 55). The comment should be updated to match the new behavior.
Apply this diff to fix the comment:
- # there should 3 descriptors in total
- # One of them is inactive imported private key for coinbase. It has no mnemonic
- # Two other should be active and have mnemonic
+ # There should be 4 descriptors in total
+ # One is an inactive imported private key for coinbase (no mnemonic)
+ # Three others have mnemonics: two active (internal, external) and one inactive (coinjoin)
assert_equal(mnemonic_count, 3)📝 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.
| # there should 3 descriptors in total | |
| # One of them is inactive imported private key for coinbase. It has no mnemonic | |
| # Two other should be active and have mnemonic | |
| assert_equal(mnemonic_count, 2) | |
| assert_equal(mnemonic_count, 3) | |
| assert_equal(cb_count, 1) | |
| assert_equal(len(descriptors), 3) | |
| assert_equal(len(descriptors), 4) | |
| # There should be 4 descriptors in total | |
| # One is an inactive imported private key for coinbase (no mnemonic) | |
| # Three others have mnemonics: two active (internal, external) and one inactive (coinjoin) | |
| assert_equal(mnemonic_count, 3) | |
| assert_equal(cb_count, 1) | |
| assert_equal(len(descriptors), 4) |
🤖 Prompt for AI Agents
In test/functional/wallet_mnemonicbits.py around lines 52 to 57, the inline
comment incorrectly states "Two other should be active and have mnemonic" while
the assertion expects 3 mnemonic descriptors; update the comment to reflect that
there are three mnemonic descriptors (e.g., "Three other should be active and
have mnemonic") so the comment matches the assertion expectations.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
seems to be working now 👍 pls rebase to fix CI
It's already checked by find()
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
knst
force-pushed
the
feat-cj-mobile-derivation-inactive-descriptor
branch
from
6d64b87 to
b957689
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/functional/wallet_mnemonicbits.py (1)
52-57: Update comment to reflect three mnemonic descriptors.The comment states "Two other should be active and have mnemonic" but the code expects 3 mnemonic descriptors (line 55). This was flagged in a previous review and remains unaddressed.
Apply this diff to fix the comment:
- # there should 3 descriptors in total - # One of them is inactive imported private key for coinbase. It has no mnemonic - # Two other should be active and have mnemonic + # There should be 4 descriptors in total + # One is an inactive imported private key for coinbase (no mnemonic) + # Three others have mnemonics: two active (internal, external) and one inactive (coinjoin) assert_equal(mnemonic_count, 3)src/wallet/scriptpubkeyman.cpp (1)
2102-2105: Critical: CoinJoin descriptor uses wrong coin type on mainnet.Line 2102 uses
Params().ExtCoinType()for all descriptors, but CoinJoin descriptors must use coin type1to match the Android wallet's m/9'/1'/4'/0' derivation path. On mainnet,ExtCoinType()returns5, so this code generates m/9'/5'/4'/0' descriptors that won't recognize Android CJ funds.Apply this fix:
- std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, type == PathDerivationType::DIP0009_CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, Params().ExtCoinType()); + const int coin_type = (type == PathDerivationType::DIP0009_CoinJoin) ? 1 : Params().ExtCoinType(); + std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, type == PathDerivationType::DIP0009_CoinJoin ? BIP32_PURPOSE_FEATURE : BIP32_PURPOSE_STANDARD, coin_type);Based on learnings and past review comments.
src/wallet/rpc/backup.cpp (1)
2041-2047: Critical: CoinJoin detection pattern uses wrong coin type.Line 2042 constructs the CJ detection pattern using
Params().ExtCoinType(), which returns5on mainnet. This creates a pattern/9'/5'/4'/0'that won't match Android CJ descriptors (which use/9'/1'/4'/0'), causing CJ descriptors to be undetected in the RPC output.Apply this fix:
if (type != std::nullopt) { - std::string match = strprintf("/%d'/%s'/4'/0'", BIP32_PURPOSE_FEATURE, Params().ExtCoinType()); + const std::string match = strprintf("/%d'/1'/4'/0'", BIP32_PURPOSE_FEATURE); bool is_cj = descriptor.find(match) != std::string::npos; if (is_cj) { spk.pushKV("coinjoin", is_cj); } }Note: This assumes the descriptor generation bug in
scriptpubkeyman.cppline 2102 is also fixed to use coin type1for CoinJoin descriptors.Based on learnings and past review comments.
🧹 Nitpick comments (1)
test/functional/wallet_listdescriptors.py (1)
49-53: LGTM: Test expectations align with three-descriptor model.The test now correctly expects 3 descriptors total (external, internal, and CoinJoin), with 2 active. The CoinJoin descriptor is intentionally inactive, consistent with retrieved learnings. The log statement aids debugging.
Optional: Simplify the dictionary checks using
dict.get()for cleaner code:- assert_equal(1, len([d for d in result['descriptors'] if 'internal' in d and d['internal']])) - assert_equal(1, len([d for d in result['descriptors'] if 'coinjoin' in d and d['coinjoin']])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('internal', False)])) + assert_equal(1, len([d for d in result['descriptors'] if d.get('coinjoin', False)]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
doc/release-notes-6835.md(1 hunks)src/wallet/hdchain.cpp(1 hunks)src/wallet/hdchain.h(1 hunks)src/wallet/rpc/backup.cpp(3 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(2 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/wallet/hdchain.cpp
- doc/release-notes-6835.md
🧰 Additional context used
📓 Path-based instructions (2)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_mnemonicbits.pytest/functional/wallet_listdescriptors.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppsrc/wallet/hdchain.h
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
PR: dashpay/dash#6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/scriptpubkeyman.cppsrc/wallet/rpc/backup.cppsrc/wallet/wallet.cpp
🧬 Code graph analysis (5)
test/functional/wallet_mnemonicbits.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/wallet/rpc/backup.cpp (1)
descriptors(2010-2010)
src/wallet/scriptpubkeyman.cpp (2)
src/wallet/hdchain.h (2)
BIP32_PURPOSE_FEATURE(145-145)BIP32_PURPOSE_STANDARD(147-149)src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
src/wallet/rpc/backup.cpp (1)
src/wallet/hdchain.h (1)
BIP32_PURPOSE_FEATURE(145-145)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (2)
SetupDescriptorGeneration(2077-2131)SetupDescriptorGeneration(2077-2077)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🪛 Ruff (0.13.2)
test/functional/wallet_listdescriptors.py
52-52: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
53-53: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-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: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (14)
src/wallet/scriptpubkeyman.h (2)
150-156: LGTM: Clean enum design following reviewer feedback.The
PathDerivationTypeenum clearly distinguishes BIP44 standard paths from the DIP0009 CoinJoin path. The naming aligns with the past review suggestion.
585-585: LGTM: Type-safe API improvement.Replacing the
bool internalparameter with thePathDerivationTypeenum makes the API more expressive and type-safe, supporting the three-descriptor model (External, Internal, CoinJoin).test/functional/wallet_mnemonicbits.py (1)
50-50: LGTM: More robust activity check.Using the
coinjoinfield from the RPC output is more robust than string-matching the derivation path, as it avoids brittleness across different network coin types.src/wallet/rpc/wallet.cpp (1)
171-171: LGTM: Documentation clarifies internal keypool usage.The updated help text accurately documents that the internal keypool now covers both change outputs and mobile CoinJoin, reflecting the PR's mobile derivation path support.
src/wallet/hdchain.h (1)
144-148: LGTM: Well-documented BIP32 purpose constants.The constants align with BIP32/BIP44 standards and DIP0009. The naming is clear, and using
constexpr uint8_tis appropriate for BIP32 purpose codes.src/wallet/scriptpubkeyman.cpp (2)
2077-2077: LGTM: Improved type safety with PathDerivationType enum.Replacing the boolean
internalparameter with thePathDerivationTypeenum improves clarity and enables three-way distinction (external/internal/CoinJoin) instead of binary.
2107-2107: LGTM: CoinJoin descriptors correctly use external path.The ternary correctly assigns
/1only toBIP44_Internalwhile bothBIP44_ExternalandDIP0009_CoinJoinuse/0, which is appropriate since CoinJoin descriptors are not change addresses.src/wallet/rpc/backup.cpp (2)
7-7: LGTM: Include necessary for ExtCoinType() usage.The
chainparams.hinclude is required to accessParams().ExtCoinType()at line 2042.
1980-1980: LGTM: RPC schema properly documents new coinjoin field.The
coinjoinfield is correctly marked as optional and documented to appear only when true, which is appropriate for CoinJoin descriptors.src/wallet/wallet.cpp (5)
3790-3790: LGTM! Loop now covers three descriptor types.The loop correctly iterates over the three required derivation path types (External, Internal, CoinJoin) to create all default descriptors for a new descriptor wallet.
3801-3801: LGTM! SetupDescriptorGeneration correctly receives the new PathDerivationType parameter.The function call properly passes the loop's
typevariable (an enum) instead of a boolean, aligning with the updated signature inscriptpubkeyman.cpp.
3804-3806: LGTM! Activation logic correctly excludes CoinJoin descriptors.The conditional ensures that only External and Internal descriptors are activated via
AddActiveScriptPubKeyMan, while CoinJoin descriptors remain inactive by default—consistent with the PR objective and retrieved learnings.Based on learnings
3811-3818: LGTM! AddActiveScriptPubKeyMan correctly persists and activates the descriptor.The function properly writes the active ScriptPubKeyMan mapping to the database before calling
LoadActiveScriptPubKeyManto activate it in memory, with appropriate error handling.
3820-3836: LGTM! LoadActiveScriptPubKeyMan correctly activates the descriptor and clears the opposite side if needed.The function properly sets the active ScriptPubKeyMan for the specified type (internal or external) and ensures that if the same SPKM was previously active on the opposite side, it is cleared—preventing dual activation. The notification at the end ensures UI consistency.
4 participants
Issue being fixed or feature implemented
#6745
Currently, funds mixed by Coin Join by Android wallet may be invisible to other wallets, including Dash Core.
What was done?
This PR introduces 3rd default descriptor for Descriptor wallets, which makes this funds available in Dash Core.
How Has This Been Tested?
Install Android Dash wallet -> get some funds -> mix them -> import mnemonic to Dash Core. Funds (including CJ transactions) expect to visible.
Without this PR, some of these transactions are shown as "spending", and funds are not available.
Breaking Changes
Amount of descriptors created by default for a new wallet on mainnet is increased from 2 (internal, external) to 3.
Amount of descriptors created by default for a new wallet on regtest is increased from 3 (internal, external, cbtx private key) to 4.
Checklist: