fix: only activate BIP44 descriptors for external signers #7076

UdjinM6 · 2025-12-19T11:34:42Z

Issue being fixed or feature implemented

When creating a wallet with an external signer (hardware wallet), the mock signer returns multiple descriptors (BIP44, BIP49, BIP84) for both receive and internal categories. Previously, all valid descriptors were being activated by calling AddActiveScriptPubKeyMan() for each one.

This caused a database key collision since multiple descriptors tried to write to the same key (ACTIVEEXTERNALSPK or ACTIVEINTERNALSPK), resulting in non-deterministic behavior where sometimes BIP44 would be active and sometimes BIP84, depending on database flush timing.

What was done?

The fix follows the pattern from the non-external-signer branch which stores multiple descriptors but only activates certain types (External and Internal, but not CoinJoin). Now we:

Store ALL descriptors returned by the signer in m_spk_managers
Only activate descriptors matching the BIP44 path pattern /44'/{cointype}'

This ensures deterministic behavior and fixes the intermittent test failure in wallet_signer.py where the test expected m/44' but sometimes got m/84'.

Should fix intermittent test failures like https://github.com/dashpay/dash/actions/runs/20364286460/job/58521843159#step:6:1662

How Has This Been Tested?

Run wallet_signer.py multiple times

Breaking Changes

n/a

Checklist:

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

When creating a wallet with an external signer (hardware wallet), the mock signer returns multiple descriptors (BIP44, BIP49, BIP84) for both receive and internal categories. Previously, all valid descriptors were being activated by calling AddActiveScriptPubKeyMan() for each one. This caused a database key collision since multiple descriptors tried to write to the same key (ACTIVEEXTERNALSPK or ACTIVEINTERNALSPK), resulting in non-deterministic behavior where sometimes BIP44 would be active and sometimes BIP84, depending on database flush timing. The fix follows the pattern from the non-external-signer branch which stores multiple descriptors but only activates certain types (External and Internal, but not CoinJoin). Now we: - Store ALL descriptors returned by the signer in m_spk_managers - Only activate descriptors matching the BIP44 path pattern /44'/{cointype}' This ensures deterministic behavior and fixes the intermittent test failure in wallet_signer.py where the test expected m/44' but sometimes got m/84'. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2025-12-19T11:35:13Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-12-19T11:38:18Z

Walkthrough

In the SetupDescriptorScriptPubKeyMans function for external signers, descriptor activation logic is now conditional. Previously, all loaded descriptors were activated via AddActiveScriptPubKeyMan. The change filters activation to only descriptors matching a specific BIP44 path pattern constructed as "/<BIP32_PURPOSE_STANDARD>'/<ExtCoinType>". Descriptors containing this bip44_purpose pattern trigger activation; others do not. An explanatory comment documents this gating logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Areas requiring attention:

Correctness of the BIP44 path pattern matching logic and the pattern construction itself
Impact of the conditional activation on wallet initialization and descriptor handling during load
Edge cases with malformed or non-standard descriptor paths that may not contain the expected pattern
Verification that this filtering doesn't inadvertently prevent activation of legitimate descriptors in existing workflows

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: conditionally activating only BIP44 descriptors for external signers instead of activating all descriptors.
Description check	✅ Passed	The description clearly explains the problem (database key collisions from activating multiple descriptors), the fix (only activate BIP44 descriptors), and provides context about testing and references to intermittent test failures.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bcb984 and 8f54a88.

📒 Files selected for processing (1)

src/wallet/wallet.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,hpp,cc}

📄 CodeRabbit inference engine (CLAUDE.md)

Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1

Files:

src/wallet/wallet.cpp

src/wallet/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

Wallet implementation must use Berkeley DB and SQLite

Files:

src/wallet/wallet.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: knst
Repo: dashpay/dash PR: 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.

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.

📚 Learning: 2025-09-02T07:34:28.226Z

Learnt from: knst
Repo: dashpay/dash PR: 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.cpp

🧬 Code graph analysis (1)

src/wallet/wallet.cpp (1)

src/wallet/hdchain.h (1)

BIP32_PURPOSE_STANDARD (147-149)

⏰ 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_tsan-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (1)

src/wallet/wallet.cpp (1)

3929-3934: LGTM! Fix correctly addresses the database collision issue.

The selective activation of only BIP44 descriptors prevents multiple descriptors from writing to the same database keys (ACTIVEEXTERNALSPK/ACTIVEINTERNALSPK). The approach is consistent with the non-external-signer branch which also selectively activates descriptor types (lines 3898-3900 exclude CoinJoin).

The substring pattern matching using desc_str.find(bip44_purpose) is appropriate for well-formed descriptor strings. The BIP44 path pattern "/44'/<ExtCoinType>'" is specific enough to avoid false positives in practice.

Based on learnings, selective activation of descriptors (similar to how CoinJoin descriptors remain intentionally inactive) is an established pattern in this codebase.

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

❤️ Share

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

UdjinM6 added this to the 23.1 milestone Dec 19, 2025

UdjinM6 requested review from PastaPastaPasta and knst December 19, 2025 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: only activate BIP44 descriptors for external signers #7076

fix: only activate BIP44 descriptors for external signers #7076

Uh oh!

UdjinM6 commented Dec 19, 2025

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fix: only activate BIP44 descriptors for external signers #7076

Are you sure you want to change the base?

fix: only activate BIP44 descriptors for external signers #7076

Uh oh!

Conversation

UdjinM6 commented Dec 19, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Dec 19, 2025

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Dec 19, 2025

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant