feat: descriptor wallets uses one extra spk for mobile derivation path for CJ #6834
Conversation
knst
added
RPC
knst
changed the title
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughReplaces boolean internal/external flags with a new enum InternalKey { External, Internal, CoinJoin } across wallet, DB, scriptpubkeyman, RPC, and tests. Wallet gains m_coinjoin_spk_managers and updates APIs (GetScriptPubKeyMan, AddActiveScriptPubKeyMan, LoadActiveScriptPubKeyMan, GetScriptPubKeyMan by id). Wallet DB adds DBKeys::ACTIVECOINJOINSPK and scan state for coinjoin SPKs; WalletBatch::WriteActiveScriptPubKeyMan now takes InternalKey. Descriptor generation and descriptor listing use InternalKey (coin type 9 for CoinJoin); RPC help text and multiple functional tests adjusted for CoinJoin-related descriptors/keypool counts. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/wallet/wallet.h (1)
1048-1061: Deactivate API can’t target CoinJoin — add enum overloadDeactivateScriptPubKeyMan(uint256 id, bool internal) can’t address CoinJoin. Introduce an InternalKey overload and keep the bool as a wrapper for ABI/source compatibility.
Apply in header:
- void DeactivateScriptPubKeyMan(uint256 id, bool internal); + void DeactivateScriptPubKeyMan(uint256 id, InternalKey internal); + // Back-compat wrapper + void DeactivateScriptPubKeyMan(uint256 id, bool internal) { + DeactivateScriptPubKeyMan(id, internal ? InternalKey::Internal : InternalKey::External); + }Follow up by updating the implementation to null m_coinjoin_spk_managers when InternalKey::CoinJoin is passed.
src/wallet/walletdb.cpp (1)
254-258: EraseActiveScriptPubKeyMan still bool — can’t erase CoinJoinThis API cannot erase ACTIVECOINJOINSPK entries, risking stale DB state. Add an InternalKey overload and keep the bool version as a wrapper.
Proposed addition (implementation here; header change needed in walletdb.h):
bool WalletBatch::EraseActiveScriptPubKeyMan(bool internal) { const std::string key{internal ? DBKeys::ACTIVEINTERNALSPK : DBKeys::ACTIVEEXTERNALSPK}; return EraseIC(key); } + +bool WalletBatch::EraseActiveScriptPubKeyMan(InternalKey which) +{ + switch (which) { + case InternalKey::Internal: return EraseIC(DBKeys::ACTIVEINTERNALSPK); + case InternalKey::External: return EraseIC(DBKeys::ACTIVEEXTERNALSPK); + case InternalKey::CoinJoin: return EraseIC(DBKeys::ACTIVECOINJOINSPK); + } + return false; // unreachable +}Update callers to use the enum where CoinJoin is involved.
src/wallet/wallet.cpp (1)
3848-3861: Refactor DeactivateScriptPubKeyMan to use InternalKey enum and extend wallet DB API
- Change
CWallet::DeactivateScriptPubKeyMan(uint256 id, bool internal)toDeactivateScriptPubKeyMan(uint256 id, InternalKey which), updating the log, callingbatch.EraseActiveScriptPubKeyMan(which), and null-out the matching pointer for Internal, External, and CoinJoin.- Add a compatibility wrapper
void DeactivateScriptPubKeyMan(uint256 id, bool internal)that forwards to the enum overload.- In WalletBatch (walletdb.h/.cpp), add
bool EraseActiveScriptPubKeyMan(InternalKey)to mirrorWriteActiveScriptPubKeyMan(including the ACTIVECOINJOINSPK key).- Update all call sites (e.g. rpc/backup.cpp) to pass
InternalKeyrather than a bool.
🧹 Nitpick comments (16)
src/wallet/scriptpubkeyman.h (2)
149-154: Enum introduction looks good; add a short contract comment and avoid ambiguous naming.Document intended semantics (which SPKMs map to each value; scan/active defaults). Also consider renaming the parameter names at call sites from “internal” to “branch” to avoid confusion with InternalKey::Internal.
Apply:
enum class InternalKey { External, Internal, CoinJoin, }; +// Note: +// - External: standard receiving path +// - Internal: change path +// - CoinJoin: mobile CJ path (coin type 9), see SetupDescriptorGeneration()
584-585: Rename parameterinternaltobranchin SetupDescriptorGeneration
Inscriptpubkeyman.h:584andscriptpubkeyman.cpp:2084, change the last parameter fromInternalKey internaltoInternalKey branch.test/functional/wallet_descriptor.py (1)
44-45: Make the internal keypool expectation resilient to future SPKM additions.Derive expectation from the external pool size instead of hardcoding 200.
Apply:
-assert_equal(wallet_info['keypoolsize'], 100) -assert_equal(wallet_info['keypoolsize_hd_internal'], 200) +assert_equal(wallet_info['keypoolsize'], 100) +# Expect 2 internal pools (change + CJ) each with -keypool size +expected_hd_internal = 2 * wallet_info['keypoolsize'] +assert_equal(wallet_info['keypoolsize_hd_internal'], expected_hd_internal)src/wallet/rpc/wallet.cpp (1)
171-172: Help text nit: capitalize CoinJoin and clarify phrasing.Use “CoinJoin” consistently and clarify “for change outputs and mobile CoinJoin”.
Apply:
-{RPCResult::Type::NUM, "keypoolsize_hd_internal", /* optional */ true, "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)"}, +{RPCResult::Type::NUM, "keypoolsize_hd_internal", /* optional */ true, "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)"},test/functional/wallet_mnemonicbits.py (2)
55-57: Update outdated comments to match new counts.Comments still say “3 descriptors total ... Two other should be active”, but the code asserts 3 mnemonic descriptors and total 4.
Apply:
- # 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: + # - 1 inactive imported private key for coinbase (no mnemonic) + # - 3 active mnemonic-backed descriptors (external, internal, coinjoin)
24-27: Minor wording fix in log message.“rise an error” → “raise an error”.
Apply:
- self.log.info("Invalid -mnemonicbits should rise an error") + self.log.info("Invalid -mnemonicbits should raise an error")test/functional/wallet_listdescriptors.py (1)
49-51: Good: counts reflect the added CoinJoin descriptor. Add a presence check for the CJ path.Assert that one active descriptor uses coin type 9 (CJ), to avoid false positives if a third descriptor appears for another reason.
Apply:
assert_equal("w3", result['wallet_name']) assert_equal(3, len(result['descriptors'])) assert_equal(3, len([d for d in result['descriptors'] if d['active']])) assert_equal(1, len([d for d in result['descriptors'] if d['internal']])) +assert any("/9'/" in d['desc'] and d['active'] for d in result['descriptors'])test/functional/tool_wallet.py (1)
269-270: Future-proof the expected keypool size (avoid hardcoded 3000).Derive it from the number of SPK managers to reduce churn if defaults change again.
- out = "Topping up keypool...\n" + self.get_expected_info_output(name="foo", keypool=3000) + num_spk = 3 if self.options.descriptors else 2 # external, internal, (+ coinjoin) + out = "Topping up keypool...\n" + self.get_expected_info_output(name="foo", keypool=1000 * num_spk)Confirm stdout uses "..." and not a Unicode ellipsis "…", otherwise this exact-match assertion will fail on some locales.
test/functional/wallet_createwallet.py (1)
155-159: Simplify keys calc and align the comment with CJ-internal counting.
keys = 1 if self.options.descriptors else 1is redundant. Also clarify that CJ contributes to internal pool size.- keys = 1 if self.options.descriptors else 1 - cj_keys = 1 if self.options.descriptors else 0 + keys = 1 + cj_keys = 1 if self.options.descriptors else 0 assert_equal(walletinfo['keypoolsize'], keys) - # hd_internals are not refilled by default for descriptor wallets atm - assert_equal(walletinfo['keypoolsize_hd_internal'], keys + cj_keys) + # For descriptor wallets, hd_internal includes CJ addresses (extra branch) + assert_equal(walletinfo['keypoolsize_hd_internal'], keys + cj_keys)test/functional/wallet_keypool_hd.py (2)
89-91: Express intent: CJ doubles internal count; avoid magic “+ 6”.Using multiplication makes the rationale obvious and eases future edits.
- # dash has only 1 type of output addresses + cj addresses - assert_equal(wi['keypoolsize_hd_internal'], 6 + 6) + # dash has 1 change branch + 1 CJ branch => doubles internal keys + assert_equal(wi['keypoolsize_hd_internal'], 2 * 6)
156-158: Same here: prefer 2 * N over N + N.- # dash has only 1 type of output addresses + cj addresses - assert_equal(wi['keypoolsize_hd_internal'], 100 + 100) + # dash has 1 change branch + 1 CJ branch + assert_equal(wi['keypoolsize_hd_internal'], 2 * 100)src/wallet/scriptpubkeyman.cpp (1)
2084-2136: Avoid magic number 9 for CJ “purpose” and document/verify the mobile path.Hardcoding
9obscures intent and may drift from Android wallet conventions. Name it and add a brief rationale. Also, please confirm that CJ should use the “/0” branch (external) here.-bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, const SecureString& secure_mnemonic, const SecureString& secure_mnemonic_passphrase, InternalKey internal) +bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, const SecureString& secure_mnemonic, const SecureString& secure_mnemonic_passphrase, InternalKey internal) { @@ - // Build descriptor string - std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, internal == InternalKey::CoinJoin ? 9 : 44, Params().ExtCoinType()); + // Build descriptor string + // NOTE: kCJPurpose follows Android wallet’s CoinJoin derivation scheme. + static constexpr int kCJPurpose = 9; // TODO: consider moving to chainparams if network-specific + std::string desc_prefix = strprintf("pkh(%s/%d'/%d'", xpub, + internal == InternalKey::CoinJoin ? kCJPurpose : 44, Params().ExtCoinType()); @@ - std::string internal_path = (internal == InternalKey::Internal) ? "/1" : "/0"; + // External (/0) for receive, Internal (/1) for change; CJ currently uses /0 — verify against Android wallet. + std::string internal_path = (internal == InternalKey::Internal) ? "/1" : "/0";Kindly confirm:
- CJ purpose/indexing matches the Android wallet (purpose 9, account 0’, branch /0).
- Whether CJ should live under /0 or /1 in our descriptors.
src/wallet/rpc/backup.cpp (2)
1979-1986: RPC schema: new “coinjoin” fieldLGTM. Field is optional and described. Consider clarifying “False if this descriptor is not CoinJoin” to avoid conflating with “receiving” terminology.
2038-2042: Duplicate condition blocks — collapse to oneSame condition used twice for internal and coinjoin flags. Minor cleanup to avoid drift.
Apply:
- if (active && type != std::nullopt) { - spk.pushKV("internal", wallet->GetScriptPubKeyMan(InternalKey::Internal) == desc_spk_man); - } - if (active && type != std::nullopt) { - spk.pushKV("coinjoin", wallet->GetScriptPubKeyMan(InternalKey::CoinJoin) == desc_spk_man); - } + if (active && type != std::nullopt) { + spk.pushKV("internal", wallet->GetScriptPubKeyMan(InternalKey::Internal) == desc_spk_man); + spk.pushKV("coinjoin", wallet->GetScriptPubKeyMan(InternalKey::CoinJoin) == desc_spk_man); + }src/wallet/wallet.cpp (2)
3810-3842: Improve logging: print enum category instead of booleanCurrent log shows “internal = true/false” which is misleading for CoinJoin. Prefer readable category names.
Apply this diff:
- WalletLogPrintf("Setting spkMan to active: id = %s, type = %s, internal = %s\n", id.ToString(), FormatOutputType(OutputType::LEGACY), internal == InternalKey::Internal ? "true" : "false"); + WalletLogPrintf( + "Setting spkMan to active: id = %s, type = %s, category = %s\n", + id.ToString(), + FormatOutputType(OutputType::LEGACY), + internal == InternalKey::Internal ? "internal" : + (internal == InternalKey::External ? "external" : "coinjoin") + );
1474-1474: Tiny dedup: helper to map bool→InternalKeyThe bool→enum ternary appears in multiple places. Optional: factor a small helper for consistency.
Example:
static constexpr InternalKey ToInternalKey(bool internal) { return internal ? InternalKey::Internal : InternalKey::External; }Then replace the ternaries with ToInternalKey(...).
Also applies to: 2354-2354, 2451-2451, 3851-3851
📜 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 (14)
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(8 hunks)src/wallet/wallet.h(3 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(3 hunks)test/functional/tool_wallet.py(1 hunks)test/functional/wallet_createwallet.py(1 hunks)test/functional/wallet_descriptor.py(1 hunks)test/functional/wallet_keypool_hd.py(2 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)
🧰 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_listdescriptors.pytest/functional/wallet_createwallet.pytest/functional/wallet_keypool_hd.pytest/functional/wallet_descriptor.pytest/functional/tool_wallet.pytest/functional/wallet_mnemonicbits.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/walletdb.hsrc/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/walletdb.cppsrc/wallet/wallet.cppsrc/wallet/wallet.h
🧠 Learnings (1)
📚 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.cppsrc/wallet/rpc/backup.cpp
🧬 Code graph analysis (9)
test/functional/wallet_listdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_createwallet.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_keypool_hd.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_descriptor.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_mnemonicbits.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/wallet/walletdb.h (2)
src/wallet/scriptpubkeyman.h (1)
InternalKey(149-225)src/wallet/walletdb.cpp (2)
WriteActiveScriptPubKeyMan(234-252)WriteActiveScriptPubKeyMan(234-234)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (2)
SetupDescriptorGeneration(2084-2136)SetupDescriptorGeneration(2084-2084)
src/wallet/rpc/backup.cpp (1)
src/wallet/scriptpubkeyman.h (1)
wallet(27-240)
src/wallet/wallet.h (2)
src/wallet/scriptpubkeyman.h (2)
ScriptPubKeyMan(163-220)ScriptPubKeyMan(171-171)src/wallet/wallet.cpp (10)
GetScriptPubKeyMan(3646-3658)GetScriptPubKeyMan(3646-3646)GetScriptPubKeyMan(3671-3680)GetScriptPubKeyMan(3671-3671)GetScriptPubKeyMan(3682-3688)GetScriptPubKeyMan(3682-3682)AddActiveScriptPubKeyMan(3801-3808)AddActiveScriptPubKeyMan(3801-3801)LoadActiveScriptPubKeyMan(3810-3846)LoadActiveScriptPubKeyMan(3810-3810)
🪛 GitHub Actions: Check Merge Fast-Forward Only
src/wallet/wallet.cpp
[error] 1-1: Merge conflict detected during automatic merge (git merge --ff-only base_branch). Resolve conflicts and commit.
⏰ 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). (9)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (18)
src/wallet/scriptpubkeyman.h (1)
1-630: Potential path/role mismatch for CoinJoin.In scriptpubkeyman.cpp, CoinJoin uses coin type 9 but still selects “/0” (external) branch; tests treat CJ keys as “internal” stock (keypoolsize_hd_internal). Confirm this is intentional (external derivation path, but counted as internal pool), and that listdescriptors “active” status is consistent across tests.
src/wallet/wallet.h (4)
395-396: Add CoinJoin SPK manager pointer — ensure full lifecycle coverageGood addition. Please confirm it's wired into:
- GetActiveScriptPubKeyMans()/GetAllScriptPubKeyMans()
- ConnectScriptPubKeyManNotifiers()
- postInit/Unload paths (clear/reset)
986-987: API switch to InternalKey is clear and type-safeLGTM. This removes the bool ambiguity between external/internal.
1041-1042: Activation API updated to InternalKeyLGTM. Matches wallet.cpp implementation.
1046-1047: Loader API updated to InternalKeyLGTM. Matches wallet.cpp, including tri-state handling.
src/wallet/rpc/backup.cpp (1)
1786-1787: Use of InternalKey in activation is correctLGTM. The active path now cleanly distinguishes external vs internal.
src/wallet/walletdb.cpp (5)
37-38: DB key for ACTIVECOINJOINSPKLGTM. Key name follows existing naming.
234-252: WriteActiveScriptPubKeyMan switched to InternalKeyLGTM. switch/assert pattern is fine.
351-352: Scan state: track m_active_coinjoin_spksLGTM. Mirrors internal/external maps.
898-905: Activation on load for all three categoriesLGTM. Once the read condition is fixed, CoinJoin SPKs will activate on load.
634-646: Fix load condition to include ACTIVECOINJOINSPKIn the
else ifat line 633 ofsrc/wallet/walletdb.cpp, replace the duplicatedACTIVEEXTERNALSPKwithACTIVECOINJOINSPKso CoinJoin ScriptPubKeyMans are correctly loaded:- } else if (strType == DBKeys::ACTIVEEXTERNALSPK || strType == DBKeys::ACTIVEINTERNALSPK || strType == DBKeys::ACTIVEEXTERNALSPK) { + } else if (strType == DBKeys::ACTIVEEXTERNALSPK || strType == DBKeys::ACTIVEINTERNALSPK || strType == DBKeys::ACTIVECOINJOINSPK) { uint256 id; ssValue >> id; bool internal = strType == DBKeys::ACTIVEINTERNALSPK; bool coinjoin = strType == DBKeys::ACTIVECOINJOINSPK; auto& spk_mans = internal ? wss.m_active_internal_spks : (coinjoin ? wss.m_active_coinjoin_spks : wss.m_active_external_spks);src/wallet/wallet.cpp (7)
1474-1476: Enum-based SPKMan selection in CanGetAddresses — looks goodClean switch-over from bool to InternalKey at callsite. No issues.
2354-2358: GetNewDestination uses External SPKMan explicitly — OKAppropriate to keep “receive” on External only.
2451-2467: ReserveDestination enum mapping — OKCorrectly maps fInternalIn to Internal/External.
3628-3634: Active SPK managers now include CoinJoin — OKIteration over {Internal, External, CoinJoin} is clear and correct.
3646-3658: GetScriptPubKeyMan(InternalKey) — OKSwitch is exhaustive, no default (good for compiler warnings). Returns expected pointers for all three categories.
3782-3799: Create and activate 3 descriptor SPKMs — OKWallet lock/encryption checks are correct; SetupDescriptorGeneration receives the enum to drive derivation. Activation via AddActiveScriptPubKeyMan for each category is correct.
3801-3809: AddActiveScriptPubKeyMan persists enum — OKDB write uses enum; immediate in-memory activation via LoadActiveScriptPubKeyMan is fine.
src/wallet/walletdb.h
Outdated
| enum class InternalKey; | ||
| /** |
There was a problem hiding this comment.
Forward-declared scoped enum used by value — specify underlying type or include the definition.
enum class InternalKey; is incomplete here and is used by value in a function declaration below; this can fail to compile unless the underlying type is known. Add : int (matches the default for scoped enums) or include the defining header.
-enum class InternalKey;
+enum class InternalKey : int;📝 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.
| enum class InternalKey; | |
| /** | |
| enum class InternalKey : int; | |
| /** |
🤖 Prompt for AI Agents
In src/wallet/walletdb.h around lines 34-35, the forward declaration "enum class
InternalKey;" is insufficient because the scoped enum is used by value later;
either change the forward declaration to specify the underlying type (e.g.,
"enum class InternalKey : int;") so the size is known, or replace the forward
declaration by including the header that defines InternalKey; update the line
accordingly and ensure any include guards/imports remain correct.
| bool WriteActiveScriptPubKeyMan(const uint256& id, InternalKey internal); | ||
| bool EraseActiveScriptPubKeyMan(bool internal); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API inconsistency: EraseActiveScriptPubKeyMan still takes bool; cannot target CoinJoin.
Write-path now supports External/Internal/CoinJoin, but erase-path can only address a boolean (internal/external). This risks leaving stale ACTIVECOINJOINSPK entries.
- bool WriteActiveScriptPubKeyMan(const uint256& id, InternalKey internal);
- bool EraseActiveScriptPubKeyMan(bool internal);
+ bool WriteActiveScriptPubKeyMan(const uint256& id, InternalKey internal);
+ bool EraseActiveScriptPubKeyMan(InternalKey internal);Outside this file (walletdb.cpp), mirror the switch used in WriteActiveScriptPubKeyMan:
bool WalletBatch::EraseActiveScriptPubKeyMan(InternalKey internal)
{
std::string key;
switch (internal) {
case InternalKey::Internal: key = DBKeys::ACTIVEINTERNALSPK; break;
case InternalKey::External: key = DBKeys::ACTIVEEXTERNALSPK; break;
case InternalKey::CoinJoin: key = DBKeys::ACTIVECOINJOINSPK; break;
}
assert(!key.empty());
return EraseIC(key);
}🏁 Script executed:
#!/bin/bash
# Find erase callsites and confirm no assumptions on bool survive.
rg -n "EraseActiveScriptPubKeyMan\\(" -g src/wallet
rg -n "ACTIVECOINJOINSPK" -g src/walletLength of output: 390
🏁 Script executed:
#!/bin/bash
# Search for EraseActiveScriptPubKeyMan definitions and callsites across the repo
rg -n "EraseActiveScriptPubKeyMan\(" -C3 .
# Search for WriteActiveScriptPubKeyMan definitions and callsites across the repo
rg -n "WriteActiveScriptPubKeyMan\(" -C3 .
# Search for the CoinJoin key usage
rg -n "ACTIVECOINJOINSPK" -C3 .Length of output: 5259
Refactor EraseActiveScriptPubKeyMan to use InternalKey
Signature in src/wallet/walletdb.h (line 234) and definition in src/wallet/walletdb.cpp (line 254) must accept InternalKey internal instead of bool internal; replicate the switch(internal) from WriteActiveScriptPubKeyMan to handle InternalKey::CoinJoin (DBKeys::ACTIVECOINJOINSPK) as well as External/Internal. Update the callsite in src/wallet/wallet.cpp (around line 3855) to pass the enum value rather than a bool, ensuring proper removal of coinjoin entries.
🤖 Prompt for AI Agents
In src/wallet/walletdb.h around lines 233-235, change the prototype of
EraseActiveScriptPubKeyMan to accept InternalKey internal (not bool) so it
matches the implementation; in src/wallet/walletdb.cpp around line 254, update
the function to switch(on internal) like WriteActiveScriptPubKeyMan and handle
InternalKey::CoinJoin by erasing DBKeys::ACTIVECOINJOINSPK in addition to
External/Internal keys; then update the callsite in src/wallet/wallet.cpp near
line 3855 to pass the appropriate InternalKey enum value instead of a bool so
CoinJoin entries are removed correctly (also ensure any needed includes or
forward declarations for InternalKey are present).
| assert_equal(desc['active'], "/9'/1" not in desc['desc']) | ||
|
|
There was a problem hiding this comment.
Inconsistent “active” expectation vs listdescriptors test; CJ should be consistently active or consistently inactive.
listdescriptors.py expects 3 active descriptors for default wallets. This assertion makes CJ inactive when its descriptor contains “/9'/1”. Align with one behavior. If CJ is intended to be active (recommended for visibility), assert active unconditionally for mnemonic-backed descriptors.
Apply:
- assert_equal(desc['active'], "/9'/1" not in desc['desc'])
+ assert desc['active']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_equal(desc['active'], "/9'/1" not in desc['desc']) | |
| assert desc['active'] |
🤖 Prompt for AI Agents
In test/functional/wallet_mnemonicbits.py around lines 50-51, the assertion
toggles CJ active-state based on the presence of "/9'/1" in the descriptor which
conflicts with listdescriptors expectations; change the assertion to make
mnemonic-backed CJ descriptors always active (e.g., replace the conditional
check with an unconditional assertion that desc['active'] is True) so CJ is
consistently treated as active for mnemonic-backed wallets.
There was a problem hiding this comment.
This assertion makes CJ inactive when its descriptor contains “/9'/1”.
CJ descriptor is always inactive at this moment; derivation path "9'/1" belong to CJ descriptors only, regular internal / external descriptors have different derivation path.
There was a problem hiding this comment.
@knst, thank you for the clarification! That makes perfect sense - CJ descriptors with the "9'/1" derivation path are intentionally inactive, while regular internal/external descriptors remain active. The conditional assertion correctly reflects this design.
I appreciate you explaining that the derivation path "9'/1" is exclusively used for CJ descriptors, which helps clarify why the test logic distinguishes between active and inactive descriptors based on this path.
(\ /)
( ._. )
o_(")(") *learning noted*
✏️ Learnings added
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
knst
force-pushed
the
feat-cj-mobile-derivation
branch
from
9d999e5 to
2abdc78
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)
src/wallet/wallet.cpp (1)
3861-3877: Support CoinJoin in DeactivateScriptPubKeyMan (bool API is now inconsistent).This function still uses a bool and cannot deactivate CJ SPK managers. Update to InternalKey to keep API consistent and allow CJ deactivation.
-// TODO: probably need to support InternalKey here -void CWallet::DeactivateScriptPubKeyMan(uint256 id, bool internal) +void CWallet::DeactivateScriptPubKeyMan(uint256 id, InternalKey internal) { - auto spk_man = GetScriptPubKeyMan(internal ? InternalKey::Internal : InternalKey::External); + auto spk_man = GetScriptPubKeyMan(internal); if (spk_man != nullptr && spk_man->GetID() == id) { - WalletLogPrintf("Deactivate spkMan: id = %s, type = %s, internal = %s\n", id.ToString(), FormatOutputType(OutputType::LEGACY), internal ? "true" : "false"); + WalletLogPrintf("Deactivate spkMan: id = %s, type = %s, keyspace = %d\n", id.ToString(), FormatOutputType(OutputType::LEGACY), int(internal)); WalletBatch batch(GetDatabase()); - if (!batch.EraseActiveScriptPubKeyMan(internal)) { + if (!batch.EraseActiveScriptPubKeyMan(internal)) { throw std::runtime_error(std::string(__func__) + ": erasing active ScriptPubKeyMan id failed"); } - - auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; - spk_mans = nullptr; + switch (internal) { + case InternalKey::Internal: m_internal_spk_managers = nullptr; break; + case InternalKey::External: m_external_spk_managers = nullptr; break; + case InternalKey::CoinJoin: m_coinjoin_spk_managers = nullptr; break; + } } NotifyCanGetAddressesChanged(); }
♻️ Duplicate comments (1)
test/functional/wallet_mnemonicbits.py (1)
50-51: Make mnemonic-backed descriptors always active (align with listdescriptors expectations).This conditional deactivates CJ descriptors and conflicts with listdescriptors.py which expects 3 active descriptors by default.
- assert_equal(desc['active'], "/9'/1" not in desc['desc']) + assert desc['active']
🧹 Nitpick comments (3)
test/functional/wallet_listdescriptors.py (1)
49-51: Also assert CJ presence to prevent silent regressions.Optional: verify exactly one coinjoin descriptor is reported active.
assert_equal(3, len(result['descriptors'])) assert_equal(3, len([d for d in result['descriptors'] if d['active']])) assert_equal(1, len([d for d in result['descriptors'] if d['internal']])) + # Exactly one CoinJoin descriptor should be present + assert_equal(1, len([d for d in result['descriptors'] if d.get('coinjoin')]))test/functional/wallet_createwallet.py (1)
155-159: Comment tweak to reflect CJ-internal keypool.Minor clarity: hd_internal now includes CJ for descriptor wallets.
- # hd_internals are not refilled by default for descriptor wallets atm + # hd_internals (including CoinJoin) are not refilled by default for descriptor wallets atm assert_equal(walletinfo['keypoolsize_hd_internal'], keys + cj_keys)src/wallet/wallet.cpp (1)
3823-3855: Avoid potential pointer aliasing; optionally connect notifiers when (re)activating.Not a blocker, but consider centralizing the pointer assignment in a small helper to prevent accidental aliasing and call notifier connections for newly activated managers.
- auto spk_man = m_spk_managers.at(id).get(); + auto spk_man = m_spk_managers.at(id).get(); switch (internal) { case InternalKey::Internal: m_internal_spk_managers = spk_man; break; case InternalKey::External: m_external_spk_managers = spk_man; break; case InternalKey::CoinJoin: m_coinjoin_spk_managers = spk_man; break; } + // Optionally ensure signals are connected if activation happens post-load. + // spk_man->NotifyWatchonlyChanged.connect(NotifyWatchonlyChanged); + // spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged);
📜 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 (14)
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(8 hunks)src/wallet/wallet.h(3 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(3 hunks)test/functional/tool_wallet.py(1 hunks)test/functional/wallet_createwallet.py(1 hunks)test/functional/wallet_descriptor.py(1 hunks)test/functional/wallet_keypool_hd.py(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 (10)
- test/functional/tool_wallet.py
- test/functional/wallet_descriptor.py
- src/wallet/scriptpubkeyman.h
- src/wallet/walletdb.h
- src/wallet/rpc/backup.cpp
- test/functional/wallet_keypool_hd.py
- src/wallet/scriptpubkeyman.cpp
- src/wallet/wallet.h
- src/wallet/rpc/wallet.cpp
- src/wallet/walletdb.cpp
🧰 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.pytest/functional/wallet_createwallet.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
🧬 Code graph analysis (4)
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)
test/functional/wallet_createwallet.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/wallet/wallet.cpp (1)
src/qt/walletmodel.cpp (2)
NotifyCanGetAddressesChanged(492-496)NotifyCanGetAddressesChanged(492-492)
⏰ 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_nowallet / 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-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (7)
test/functional/wallet_mnemonicbits.py (1)
55-55: LGTM: counts updated for External + Internal + CoinJoin + coinbase key.Three mnemonic-backed descriptors and one imported coinbase key are expected now.
Also applies to: 57-57
src/wallet/wallet.cpp (6)
1476-1485: LGTM: CanGetAddresses routing via InternalKey.Uses the correct SPK manager per Internal/External.
2354-2369: LGTM: New receiving addresses correctly use External SPK manager.
2448-2468: LGTM: ReserveDestination maps to Internal/External correctly.
3642-3648: LGTM: Active SPK managers now include CoinJoin.
3659-3671: LGTM: Enum-based SPK manager selector.No default is fine to let the compiler warn on new enum values.
3795-3812: LGTM: Create and activate three descriptor managers (Ext/Internal/CJ).Handles encryption and generation paths correctly.
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 show as "spending", but 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 (internal, external, mobile CJ funds).
Amount of descriptors created by default for a new wallet on regtest is increased from 3 (internal, external, cbtx private key) to 4 (internal, external, mobile CJ funds, cbtx private key).
Checklist: