feat: adopt Bitcoin HWI to work with Dash Core #1
Conversation
WalkthroughThis pull request rebrands the Bitcoin Hardware Wallet Interface to a Dash-specific fork. Changes include updating documentation, reorienting the codebase away from Bitcoin to Dash, removing support for non-legacy address types (WIT, SH_WIT, TAP), adjusting BIP44 coin-type mappings for Dash mainnet, and restricting device integrations to legacy-only configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
hwilib/devices/jade.py (1)
258-258: Critical: Remove or update references to deleted address types.The code references
AddressType.WITandAddressType.SH_WITwhich have been removed from theAddressTypeenum inhwilib/common.py. This will cause anAttributeErrorat runtime if witness inputs are encountered.Since Jade is now legacy-only per the Dash fork requirements, this code should either:
- Be simplified to only handle legacy inputs, or
- Raise an appropriate error for non-legacy inputs
Apply this diff to handle witness inputs appropriately:
if multisig_input and path and scriptcode and (p2sh or witness_input): parsed = parse_multisig(scriptcode) if parsed: - addr_type = AddressType.LEGACY if not witness_input else AddressType.WIT if not p2sh else AddressType.SH_WIT + if witness_input: + raise BadArgumentError("Jade only supports legacy address types for Dash") + addr_type = AddressType.LEGACY script_variant = self._convertAddrType(addr_type, multisig=True)And similarly for change output validation at Lines 305-333:
for pubkey, origin in psbtout.hd_keypaths.items(): if origin.fingerprint == master_fp and len(origin.path) > 0: - change_addr_type = None if txout.is_p2pkh(): change_addr_type = AddressType.LEGACY - elif txout.is_witness()[0] and not txout.is_p2wsh(): - change_addr_type = AddressType.WIT # ie. p2wpkh - elif txout.is_p2sh() and is_witness(psbtout.redeem_script)[0]: - change_addr_type = AddressType.SH_WIT else: + # Non-legacy outputs not supported continueAlso applies to: 305-333
hwilib/_cli.py (1)
195-200: Update descriptor example to reflect legacy-only support.The default is correctly changed to LEGACY, but the help text on Line 197 shows an example descriptor using
wpkh(witness pubkey hash), which corresponds to the removed WIT address type. Update the example to usepkhfor legacy addresses.Apply this diff to update the example:
- group.add_argument('--desc', help='Output Descriptor. E.g. wpkh([00000000/84h/0h/0h]xpub.../0/0), where 00000000 must match --fingerprint and xpub can be obtained with getxpub. See doc/descriptors.md in Bitcoin Core') + group.add_argument('--desc', help='Output Descriptor. E.g. pkh([00000000/44h/5h/0h]xpub.../0/0), where 00000000 must match --fingerprint and xpub can be obtained with getxpub. See doc/descriptors.md in Dash Core')hwilib/devices/ledger.py (1)
253-266: Critical: Handle non-legacy address types or reject them explicitly.Lines 253-266 attempt to assign removed
AddressTypevalues (SH_WIT,WIT,TAP) which will causeAttributeError. Subsequently, accessingsigning_priority[script_addrtype]at Lines 306 and 318 will causeKeyErrorfor non-LEGACY types.Since Dash only supports legacy transactions, this code should either:
- Explicitly reject non-legacy inputs with a clear error message, or
- Be simplified to assume all inputs are legacy
Apply this diff to reject non-legacy inputs explicitly:
script_addrtype = AddressType.LEGACY if is_wit: - if p2sh: - if wit_ver == 0: - script_addrtype = AddressType.SH_WIT - else: - raise BadArgumentError("Cannot have witness v1+ in p2sh") - else: - if wit_ver == 0: - script_addrtype = AddressType.WIT - elif wit_ver == 1: - script_addrtype = AddressType.TAP - else: - continue + raise BadArgumentError("Witness inputs are not supported for Dash transactions")Also applies to: 306-306, 318-318
hwilib/commands.py (3)
346-357: Remove or guard branches for deleted AddressType members.Lines 348-355 reference
AddressType.SH_WIT,AddressType.WIT, andAddressType.TAP, but these enum members are removed incommon.py(they are commented out). Any code reaching these branches will raiseAttributeErrorat runtime.Since non-legacy address types are not supported for Dash, consider one of these approaches:
Option 1 (recommended): Remove the unsupported branches and simplify the logic.
- # [DASHIFIED] default address type is changed to legacy if addr_type is AddressType.LEGACY: return PKHDescriptor(pubkey) - elif addr_type is AddressType.SH_WIT: - return SHDescriptor(WPKHDescriptor(pubkey)) - elif addr_type is AddressType.WIT: - return WPKHDescriptor(pubkey) - elif addr_type is AddressType.TAP: - if not client.can_sign_taproot(): - raise UnavailableActionError("Device does not support Taproot") - return TRDescriptor(pubkey) else: - raise ValueError("Unknown address type") + raise ValueError(f"Unsupported address type for Dash: {addr_type}")Option 2: Add an early check to raise a clear error.
+ if addr_type not in (AddressType.LEGACY,): + raise UnavailableActionError(f"Address type {addr_type} is not supported for Dash") + # [DASHIFIED] default address type is changed to legacy if addr_type is AddressType.LEGACY: return PKHDescriptor(pubkey)
387-396: Remove references to deleted AddressType.TAP.Lines 392 and 396 reference
AddressType.TAP, which is removed from the enum incommon.py. This will causeAttributeErrorat runtime.Apply this diff to remove the Taproot-specific logic:
supports_taproot = client.can_sign_taproot() addr_types = [addr_type] if addr_all: addr_types = list(AddressType) - elif not supports_taproot and addr_type == AddressType.TAP: - raise UnavailableActionError("Device does not support Taproot") - - if not supports_taproot and AddressType.TAP in addr_types: - del addr_types[addr_types.index(AddressType.TAP)]
468-499: Remove logic for deleted AddressType members.Lines 479, 481, 494, 496, and 498 attempt to assign
AddressType.SH_WIT,AddressType.WIT, andAddressType.TAP, which are removed from the enum. This will causeAttributeErrorat runtime when processing descriptors for these address types.Since Dash only supports legacy addresses, consider either:
Option 1 (recommended): Raise an error for non-legacy descriptors.
addr_type = AddressType.LEGACY is_sh = isinstance(descriptor, SHDescriptor) is_wsh = isinstance(descriptor, WSHDescriptor) + is_tr = isinstance(descriptor, TRDescriptor) + + # Dash only supports legacy addresses + if is_sh or is_wsh or is_tr or isinstance(descriptor, WPKHDescriptor): + raise UnavailableActionError("Only legacy address types are supported for Dash") + if is_sh or is_wsh: - assert len(descriptor.subdescriptors) == 1 - descriptor = descriptor.subdescriptors[0] - if isinstance(descriptor, WSHDescriptor): - is_wsh = True - assert len(descriptor.subdescriptors) == 1 - descriptor = descriptor.subdescriptors[0] - if isinstance(descriptor, MultisigDescriptor): - if is_sh and is_wsh: - addr_type = AddressType.SH_WIT - elif not is_sh and is_wsh: - addr_type = AddressType.WIT - return {"address": client.display_multisig_address(addr_type, descriptor)} - is_wpkh = isinstance(descriptor, WPKHDescriptor) - if isinstance(descriptor, PKHDescriptor) or is_wpkh or isinstance(descriptor, TRDescriptor): + # Should not reach here after the check above + raise UnavailableActionError("Script descriptors are not supported for Dash") + + if isinstance(descriptor, PKHDescriptor): pubkey = descriptor.pubkeys[0] if pubkey.origin is None: raise BadArgumentError(f"Descriptor missing origin info: {desc}") if pubkey.origin.fingerprint != client.get_master_fingerprint(): raise BadArgumentError(f"Descriptor fingerprint does not match device: {desc}") xpub = client.get_pubkey_at_path(pubkey.origin.get_derivation_path()).to_string() if pubkey.pubkey != xpub and pubkey.pubkey != xpub_to_pub_hex(xpub) and pubkey.pubkey != xpub_to_xonly_pub_hex(xpub): raise BadArgumentError(f"Key in descriptor does not match device: {desc}") - if is_sh and is_wpkh: - addr_type = AddressType.SH_WIT - elif not is_sh and is_wpkh: - addr_type = AddressType.WIT - elif isinstance(descriptor, TRDescriptor): - addr_type = AddressType.TAP return {"address": client.display_singlesig_address(pubkey.get_full_derivation_path(0), addr_type)} raise BadArgumentError("Missing both path and descriptor")Option 2: Remove descriptor support entirely if not needed for Dash.
🧹 Nitpick comments (2)
README.md (1)
102-102: Consider formatting the fork attribution URL as a markdown link.The bare URL works but using proper markdown link formatting would be more consistent with markdown best practices.
Apply this diff to format as a proper link:
-This project is a fork of Bitcoin HWI: https://github.com/bitcoin-core/hwi +This project is a fork of [Bitcoin HWI](https://github.com/bitcoin-core/hwi).hwilib/commands.py (1)
176-185: Update documentation to reflect Dash paths.The default address type change to LEGACY is appropriate. However, the docstring still references Bitcoin-specific paths (
m/44'/0'/0'). For Dash, the coin type should be5'instead of0'per BIP 44.Apply this diff to update the docstring:
""" Get the master extended public key from a client :param client: The client to interact with - :return: A dictionary containing the public key at the ``m/44'/0'/0'`` derivation path. + :return: A dictionary containing the public key at the ``m/44'/5'/0'`` derivation path for Dash. Returned as ``{"xpub": <xpub string>}``. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(3 hunks)hwilib/_cli.py(3 hunks)hwilib/commands.py(7 hunks)hwilib/common.py(2 hunks)hwilib/devices/jade.py(1 hunks)hwilib/devices/ledger.py(1 hunks)hwilib/devices/ledger_bitcoin/client.py(1 hunks)hwilib/hwwclient.py(1 hunks)hwilib/key.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
hwilib/devices/ledger_bitcoin/client.py (1)
hwilib/devices/ledger_bitcoin/exception/errors.py (1)
NotSupportedError(17-18)
hwilib/hwwclient.py (1)
hwilib/common.py (1)
AddressType(38-59)
hwilib/key.py (1)
hwilib/common.py (2)
AddressType(38-59)Chain(13-35)
hwilib/_cli.py (1)
hwilib/common.py (3)
AddressType(38-59)argparse(31-35)argparse(55-59)
hwilib/devices/jade.py (1)
hwilib/common.py (2)
Chain(13-35)AddressType(38-59)
hwilib/devices/ledger.py (1)
hwilib/common.py (1)
AddressType(38-59)
hwilib/commands.py (2)
hwilib/hwwclient.py (1)
HardwareWalletClient(23-241)hwilib/common.py (1)
AddressType(38-59)
🪛 markdownlint-cli2 (0.18.1)
README.md
102-102: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.8)
hwilib/devices/jade.py
97-99: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
103-103: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
110-110: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (16)
hwilib/devices/ledger_bitcoin/client.py (2)
292-294: App name validation correctly updated for Dash.The validation logic properly restricts to Dash and Dash Test apps, aligning with the Dash-focused fork.
296-302: Verify legacy app detection logic for Dash apps.The legacy detection checks for
"Legacy" in app_name, but the valid Dash app names ("Dash","Dash Test","app") do not contain "Legacy". The comment references Bitcoin Legacy variants which may no longer be relevant to this context. Clarify whether Dash has legacy app variants and update the logic and comment accordingly.README.md (2)
10-10: Good addition of vendor endorsement disclaimer.This caveat is important for managing user expectations regarding hardware wallet quality and security.
34-34: Repository references correctly updated for Dash.The clone URL and documentation heading are properly updated to reflect the Dash fork.
Also applies to: 96-96
hwilib/devices/jade.py (2)
96-99: Network mapping correctly updated for Dash.Removal of SIGNET aligns with the Dash-focused configuration and the Chain enum changes in hwilib/common.py.
106-110: Address type mappings require verification of downstream code references.The review comment identifies a potential issue where
AddrtessType.WIT,AddressType.SH_WIT, andAddressType.TAPmay be removed from the enum but still referenced inlegacy_sign_tx. However, manual verification of the codebase is needed to:
- Confirm which address types were actually removed from
hwilib/common.py- Verify the specific references at lines 258, 305-310, and 325-333 in
legacy_sign_tx- Determine if those code paths are reachable or protected by conditional logic
hwilib/hwwclient.py (2)
48-49: Default address type correctly updated to LEGACY.The change from WIT to LEGACY aligns with the Dash-focused, legacy-only configuration throughout the codebase. The clarifying comment is helpful.
40-41: The message signing prefix is correct and matches Dash Core's implementation.The prefix
"DarkCoin Signed Message:\n"is the official magic string used by Dash Core for RPC signmessage/verifymessage operations, spork signatures, and ProRegTx signing. This maintains compatibility with historical DarkCoin/Dash implementations. The implementation is verified as correct.hwilib/key.py (3)
372-382: BIP44 purpose correctly restricted to LEGACY.The function appropriately raises a
ValueErrorfor non-legacy address types, consistent with the Dash fork's legacy-only approach.
399-412: Address type derivation correctly restricted to LEGACY.The function properly returns
Nonefor non-legacy BIP44 purposes, maintaining consistency withget_bip44_purpose.
385-397: BIP44 coin type correctly updated for Dash.The mainnet coin type of 5 is correct per BIP44 specifications and SLIP-0044 registration for Dash. The code implementation and docstring are accurate.
hwilib/common.py (2)
17-22: Chain enum correctly updated for Dash networks.Removal of SIGNET and TESTNET4 is appropriate as these are Bitcoin-specific test networks. The Dash-focused comments are clear.
42-46: AddressType enum correctly restricted to LEGACY for Dash.This fundamental change to support only legacy address types is appropriate for Dash, which doesn't support SegWit or Taproot. The well-documented comments clarify the rationale.
hwilib/_cli.py (1)
158-159: CLI default correctly updated to LEGACY for getmasterxpub.The default address type change aligns with the Dash-focused configuration. The comment is helpful for maintainers.
hwilib/devices/ledger.py (1)
115-123: Verify the actual state of signing_priority dictionary and downstream usage against the current codebase.The review comment identifies potential
KeyErrorandAttributeErrorissues at specific lines, but these claims require direct code inspection to confirm. Verify:
- Lines 115-123: Actual contents of the
signing_prioritydictionary- Lines 253-266: Whether
script_addrtypecan be assigned values not insigning_priority- Lines 306, 318: Actual dictionary access patterns
hwilib/common.py: Whether TAP, WIT, and SH_WIT address types are present in theAddressTypeenumhwilib/commands.py (1)
242-243: LGTM!The default address type change to LEGACY is appropriate and consistent with the Dash adaptation.
| # [DASHIFIED] default address type is changed to legacy | ||
| kp_type_group.add_argument("--addr-type", help="The address type (and default derivation path) to produce descriptors for", type=AddressType.argparse, choices=list(AddressType), default=AddressType.LEGACY) # type: ignore | ||
| kp_type_group.add_argument('--all', action='store_true', help='Generate addresses for all standard address types (default paths: ``m/{44,49,84}h/0h/0h/[0,1]/*)``') |
There was a problem hiding this comment.
Update help text to reflect legacy-only support.
The default address type is correctly changed to LEGACY, but the help text on Line 184 still references paths m/{44,49,84}h/0h/0h/[0,1]/*. Since only BIP44 (purpose 44) is now supported, the help text should be updated.
Apply this diff to update the help text:
- kp_type_group.add_argument('--all', action='store_true', help='Generate addresses for all standard address types (default paths: ``m/{44,49,84}h/0h/0h/[0,1]/*)``')
+ kp_type_group.add_argument('--all', action='store_true', help='Generate addresses for all standard address types (default path: ``m/44h/5h/0h/[0,1]/*`` for Dash mainnet)')📝 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.
| # [DASHIFIED] default address type is changed to legacy | |
| kp_type_group.add_argument("--addr-type", help="The address type (and default derivation path) to produce descriptors for", type=AddressType.argparse, choices=list(AddressType), default=AddressType.LEGACY) # type: ignore | |
| kp_type_group.add_argument('--all', action='store_true', help='Generate addresses for all standard address types (default paths: ``m/{44,49,84}h/0h/0h/[0,1]/*)``') | |
| # [DASHIFIED] default address type is changed to legacy | |
| kp_type_group.add_argument("--addr-type", help="The address type (and default derivation path) to produce descriptors for", type=AddressType.argparse, choices=list(AddressType), default=AddressType.LEGACY) # type: ignore | |
| kp_type_group.add_argument('--all', action='store_true', help='Generate addresses for all standard address types (default path: ``m/44h/5h/0h/[0,1]/*`` for Dash mainnet)') |
🤖 Prompt for AI Agents
In hwilib/_cli.py around lines 182 to 184, the help text for the --all flag
still references multiple BIP purposes (m/{44,49,84}h/0h/0h/[0,1]/*) even though
the default and supported address type is LEGACY (BIP44) only; update the help
string to reference the legacy/BIP44-only derivation path (e.g.
m/44h/0h/0h/[0,1]/*) so it correctly reflects legacy-only support.
| [](https://cirrus-ci.com/github/bitcoin-core/HWI) | ||
| [](https://hwi.readthedocs.io/en/latest/?badge=latest) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check git remote to understand repository ownership
git remote -vRepository: dashpay/HWI
Length of output: 166
🏁 Script executed:
# Search for Dash-related references in the codebase
rg -i "dash" --type md --type py --type txt -cRepository: dashpay/HWI
Length of output: 166
🏁 Script executed:
# Check the full README.md to understand context
head -20 README.mdRepository: dashpay/HWI
Length of output: 166
🏁 Script executed:
# Look for any repository metadata or setup files that indicate fork status
fd -e "setup.py" -e "pyproject.toml" -e ".github" | head -5Repository: dashpay/HWI
Length of output: 166
Update CI/CD badge URLs to point to the Dash fork.
The badges currently reference github/bitcoin-core/HWI but should reference github/dashpay/HWI to match this repository and the updated title "Dash Hardware Wallet Interface".
🤖 Prompt for AI Agents
In README.md around lines 3 to 4, the CI/CD badge image and link reference
github/bitcoin-core/HWI but should point to github/dashpay/HWI; update both
badge image URLs and their target links to use github/dashpay/HWI (and keep
existing badge parameters intact) so the badges reflect the Dash fork and
repository name.
2 participants
It tested to work with Ledger Nano S if Dash Core is built from dashpay/dash#6019
Steps to test:
./hwi.py enumerateto be sure that your device is shown in the list. Probably you need to setup udev rules and some extra python libraries; hidapi has been required to install for ledger on my workstation.src/qt/dash-qt -signer=FULL_PATH_TO_dash-hwi/hwi.pySummary by CodeRabbit
Release Notes
Documentation
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.