fix: only activate BIP44 descriptors for external signers, fix mocked signers #7076
+11
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughIn SetupDescriptorScriptPubKeyMans for external signers, activation of newly loaded DescriptorScriptPubKeyMan is now conditional. After creating and storing a descriptor-based ScriptPubKeyMan, the code examines the descriptor string for a BIP44 path fragment constructed as "/<BIP32_PURPOSE_STANDARD>'/". Only when that bip44_purpose fragment is present does the code call AddActiveScriptPubKeyMan to activate the manager; otherwise activation is skipped. A comment explaining this gating was added. Sequence Diagram(s)sequenceDiagram
autonumber
participant Signer as External Signer / Descriptor Source
participant Wallet as Wallet::SetupDescriptorScriptPubKeyMans
participant Man as DescriptorScriptPubKeyMan (stored)
participant Active as ActiveManagers/AddActiveScriptPubKeyMan
Signer->>Wallet: supply descriptor(s)
Wallet->>Man: create & store ScriptPubKeyMan for descriptor
Wallet->>Wallet: inspect descriptor string for bip44_purpose ("/<BIP32_PURPOSE_STANDARD>'/<ExtCoinType>")
alt descriptor contains bip44_purpose
Wallet->>Active: AddActiveScriptPubKeyMan(id, internal)
Active-->>Wallet: activation confirmed
else descriptor missing bip44_purpose
Wallet-->>Man: do not activate (remain inactive)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-02T07:34:28.226ZApplied to files:
🧬 Code graph analysis (1)test/functional/mocks/signer.py (1)
🔇 Additional comments (3)
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 |
knst
reviewed
Dec 24, 2025
Collaborator
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
strange, I still have this type of failures on my localhost:
2/12 - wallet_signer.py --descriptors failed, Duration: 2 s
stdout:
2025-12-24T18:09:13.469000Z TestFramework (INFO): PRNG seed is: 3735113496137477052
2025-12-24T18:09:13.471000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_∋_🏃_20251225_010908/wallet_signer_3
2025-12-24T18:09:14.383000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/DASH/test/functional/test_framework/test_framework.py", line 163, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/DASH/test/functional/wallet_signer.py", line 62, in run_test
self.test_valid_signer()
~~~~~~~~~~~~~~~~~~~~~~^^
File "/DASH/test/functional/wallet_signer.py", line 105, in test_valid_signer
assert_equal(address_info['hdkeypath'], "m/44'/1'/0'/0/0")
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/DASH/test/functional/test_framework/util.py", line 74, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(m/84'/1'/0'/0/0 == m/44'/1'/0'/0/0)
2025-12-24T18:09:14.434000Z TestFramework (INFO): Stopping nodes
2025-12-24T18:09:14.836000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_∋_🏃_20251225_010908/wallet_signer_3
2025-12-24T18:09:14.836000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_∋_🏃_20251225_010908/wallet_signer_3/test_framework.log
2025-12-24T18:09:14.836000Z TestFramework (ERROR):
2025-12-24T18:09:14.836000Z TestFramework (ERROR): Hint: Call /DASH/test/functional/combine_logs.py '/tmp/test_runner_∋_🏃_20251225_010908/wallet_signer_3' to consolidate all logs
2025-12-24T18:09:14.836000Z TestFramework (ERROR):
2025-12-24T18:09:14.836000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2025-12-24T18:09:14.836000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2025-12-24T18:09:14.836000Z TestFramework (ERROR):
UdjinM6
changed the title
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 <[email protected]>
pkh should use purpose 44'
UdjinM6
force-pushed
the
fix_ext_wal_act_desc
branch
from
c934b4f to
0a72191
Compare
Author
|
Added 2 fixes for mocked signers and rebased |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.pymultiple timesBreaking Changes
n/a
Checklist: