Repository navigation
feat(hwi): Add Hardware Wallet Support - #311
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #311 +/- ##
==========================================
- Coverage 61.46% 57.97% -3.50%
==========================================
Files 23 25 +2
Lines 3989 4490 +501
==========================================
+ Hits 2452 2603 +151
- Misses 1537 1887 +350
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Below are the steps one can follow to test this hardware integration using specter simulator and bitcoind in regtest mode:
cargo run --features hwi -- -n regtest hwi devices
`
Finished dev profile [unoptimized + debuginfo] target(s) in 0.17s
Running target/debug/bdk-cli -n regtest hwi devices
{
"count": 1,
"devices": [
{
"fingerprint": "2b1f6f60",
"model": "specter-simulator"
}
]
}
`
cargo run --features hwi -- -n regtest hwi xpub "m/84'/1'/1'"
`
Finished dev profile [unoptimized + debuginfo] target(s) in 0.17s
Running target/debug/bdk-cli -n regtest hwi xpub 'm/84'\''/1'\''/1'\'''
{
"external_descriptor": "wpkh([2b1f6f60/84'/1'/1']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/0/*)#3d9lanp5",
"internal_descriptor": "wpkh([2b1f6f60/84'/1'/1']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/1/*)#qeq7qx3v",
"key_expression": "[2b1f6f60/84'/1'/1']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA",
"master_fingerprint": "2b1f6f60",
"path": "m/84'/1'/1'",
"script_type": "wpkh",
"xpub": "tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA"
}
`
cargo run --features hwi,rpc -- --network regtest wallet -w spec_regtest_wallett config -f -e
"wpkh([2b1f6f60/84'/1'/1']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/0/*)#3d9lanp5" -i "wpkh([2b1f6f60/84'/1'/1']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/1/*)#qeq7qx3v" -d sqlite -c rpc -u "127.0.0.1:18443" -a username:password
`
Finished dev profile [unoptimized + debuginfo] target(s) in 0.28s
Running target/debug/bdk-cli --network regtest wallet -w spec_regtest_wallett config -f -e 'wpkh([2b1f6f60/84'\''/1'\''/1'\'']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/0/*)#3d9lanp5' -i 'wpkh([2b1f6f60/84'\''/1'\''/1'\'']tpubDD1quQtjKgfEz2bqvkcz3VqSeV3vvv72bfhC88kRh1Dh7tFhyh3yUomH3qLruGevPv9Zyek4JqHJob7aDwtAKbKrqLASyv137CQyRhDSncA/1/*)#qeq7qx3v' -d sqlite -c rpc -u '127.0.0.1:18443' -a 'username:password'
{
"message": "Wallet 'spec_regtest_wallett' initialized successfully in \"/Users/x/x/config.toml\""
}
`
cargo run --features hwi,rpc -- --network regtest wallet -w spec_regtest_wallett hwi register
`
Finished dev profile [unoptimized + debuginfo] target(s) in 0.18s
Running target/debug/bdk-cli --network regtest wallet -w spec_regtest_wallett hwi register
{
"hmac": null,
"next_step": null,
"success": true
}
`
cargo run --features hwi,rpc -- --network regtest wallet -w spec_regtest_wallett hwi address --index 0
`
Finished dev profile [unoptimized + debuginfo] target(s) in 0.20s
Running target/debug/bdk-cli --network regtest wallet -w spec_regtest_wallett hwi address --index 0
{
"address": "bcrt1qwp5fqrqfuzph8uvhwsrews5pdtfjq6adzqqety",
"change": false,
"displayed_on_device": false,
"index": 0,
"message": "Device did not display the address (Unimplemented method); showing the locally derived address"
}
`
|
vadim-anfv
left a comment
vadim-anfv
left a comment
There was a problem hiding this comment.
Concept ACK
Hardware wallet support in bdk-cli is something I have been waiting for, so thanks for taking this on. Left a few comments inline. I tested this with a Jade.
|
Concept ACK, I reviewed at 1ec353b. I tested on the Specter simulator (regtest): hwi devices → hwi xpub → wallet config → hwi register → hwi address → create_tx → hwi sign (confirmed on the simulator) → finalize_psbt → extract_psbt → broadcast. The tx reached the mempool, and the balance change matched. |
| warn!( | ||
| "Using hardware wallet {fingerprint} ({})", | ||
| device.device_kind() | ||
| ); |
There was a problem hiding this comment.
The doc comment on this function says, "The chosen device's fingerprint and model are always logged," but as per my reply to @vadim-anfv on the Jade thread, this warn! doesn't reach the user by default (I only see it with RUST_LOG=warn, and only after switching the import to log). In the sign handler (handlers/hwi.rs lines 300–301 as at commit 1ec353b) the selection happens right before sign_tx, so this line is the user's only indication of which device they are about to confirm on. The "N hardware wallets connected … using the first" warning above it has the same problem.
Since the point is to tell the user before the operation starts, would it make sense to print these two notices unconditionally instead of through the logger? There seems to be a precedent: the "WARNING: … was accessible to other users" message from config (src/utils/common.rs:403) is printed with eprintln!, so it shows without RUST_LOG.
I only tested with a single simulator. With one device connected, --fingerprint behaves as expected: a matching fingerprint works, and a non-matching one fails with "No connected device matches fingerprint …". I could not test the multi-device behavior.
There was a problem hiding this comment.
Thank you so much for the review. I have replaced tracing with log and used eprintln! to display the selected device.
Thank you so much for testing this PR. I know it is a tedious task testing this feature. I have updated the PR to cover all your feedback. |
22dd104 to
4e8d272
Compare
- add devices, register, address and sign tx hwi subcommands
- add hidapi dependencies to workflows to fix failing compilation
- move the register, address and sign commands to subwallet commands
- add --fingerprint to `hwi xpub` and `wallet <name> hwi`, select the matching device, and enumerate physical devices before simulators.
- Move the single-sig script-type logic and multipath detection out of the HWI handler and into utils/descriptors. - Add SingleSigScriptType (ValueEnum) with purpose/as_str/descriptor and from_derivation_path. - Add is_multipath_descriptor, a wrapper over the existing descriptor_path_count primitive. - Drop the contains_private_key helper; the final DescriptorPublicKey parse already rejects private keys. - Add BitBox simulator endpoint as a const.
vadim-anfv
left a comment
vadim-anfv
left a comment
There was a problem hiding this comment.
tACK cc730d8
Thanks for working through all eight of my comments. Some of the fixes are better than what I suggested, the simulator endpoint as a named constant for one.
Re-tested on a Jade with a throwaway seed:
hwi xpub --type trnow readsm/86'/0'/0'- the
warn!lines reach the user with noRUST_LOGset --fingerprintis accepted in both positions and shows up in--helphwi devicesanswers immediately instead of waiting on the BitBox simulator
Two I checked by reading rather than running: the per-device error needs an uninitialized device, and the Jade with_wallet fix is identical to the suggestion I tested earlier.
One thing left inline.
| let ext = ext_desc.to_string(); | ||
| let int = int_desc.to_string(); | ||
| let int = if int == ext { None } else { Some(int) }; | ||
| let policy = build_device_policy(&ext, int.as_deref(), network)?; |
There was a problem hiding this comment.
The comment above says single-sig wallets skip the policy, but build_device_policy(...)? runs for every wallet kind, and the result is only used when kind is Policy. When I tested at 7cbce6e on the Specter simulator (regtest):
A wpkh wallet with external /0/* and internal /5/* fails hwi address with “Cannot derive a multipath policy: …”, although no policy is needed for single-sig.
A wpkh wallet with only an external descriptor prints, by default, “Wallet has no separate internal (change) descriptor; the device policy will only cover receive addresses.”, although no policy is attached.
Would it make sense to build the policy only for the Policy kind, like this:
matches!(kind, WalletKind::Policy)
.then(|| build_device_policy(...))
.transpose()?There was a problem hiding this comment.
Moved the policy building to only applicable WalletKind.
There was a problem hiding this comment.
I verified at d34e6bf (Specter simulator, regtest, single device): the hwi address on the wpkh wallet with internal /5/* now returns the address instead of the “Cannot derive a multipath policy” error, and the external-only wallet no longer prints the receive-only warning.
| let derivation = DerivationPath::from_str(&path) | ||
| .map_err(|e| Error::Generic(format!("Invalid derivation path: {e}")))?; | ||
| let device = first_hwi_device(network, &hwi_wallet, target_fp).await?; | ||
| let xpub = device.get_extended_pubkey(&derivation).await?; |
There was a problem hiding this comment.
At 7cbce6e, On Specter simulator: bdk-cli -n bitcoin hwi xpub "m/84'/0'/0'" returns a tpub… key and no warning. The key follows my Specter device’s network setting, not -n. Should xpub compare the network of the returned key with -n and fail early, rather than emitting descriptors that the wallet config will later reject?
There was a problem hiding this comment.
Added a check for the device network against the supplied network.
There was a problem hiding this comment.
Though the network comparison is coarse because hardware devices are aware of only two kinds of network, "mainnet" and "testnet" and thankfully, "NetworkKind" provides that.
There was a problem hiding this comment.
I verified at d34e6bf: with the Specter simulator on testnet, -n bitcoin hwi xpub "m/84'/0'/0'" now fails, and -n regtest still returns a tpub.
Musab1258
left a comment
Musab1258
left a comment
There was a problem hiding this comment.
tACK d34e6bf
cargo test --features hwi,rpc passes (35 unit tests, 18 integration tests). I tested on the Specter simulator (regtest) with one device: hwi devices, hwi xpub, wallet config, register, address, create_tx, sign (the “Using hardware wallet” notice appears before the confirmation screen), finalize_psbt, extract_psbt, broadcast, --fingerprint before and after the subcommand, and the three follow-up fixes (the “Using hardware wallet” notice now shows without RUST_LOG, single-sig policy building, and the xpub network check). I reviewed the PR head, not commit by commit.
Thank you for the comprehensive review. This feature is a heavy one to test. |
Description
Adds hardware wallet support to the project using
async-hwilibrary. Supported hardware devices include Specter, Jade, Ledger, Coldcard, and BitBox02 together with their simulators.This supersedes #207, which was approved but went stale after the project restructure. The feature has been reimplemented from scratch on the current modular structure, and the design is reworked around the saved wallet config.
Commands:
The feature introduced two top-level commands (
hwi devicesandhwi xpub) and wallet subcommands (register,address, andsign).hwi xpub [<path>] [--type <wpkh|tr|sh-wpkh|pkh>]: reads a device extended public key. Outputs a ready-to-use key expression and external/internal descriptors.hwi devices: list every connected device (fingerprint + model).register: Wallet scoped operation (wallet -w <walletname> hwi register). It registers the wallet policy on the device (returns the HMAC where the device supports it). The policy comes from the saved wallet config.sign <psbt>: sign a PSBT with the device.fixes #194
Notes to the reviewers
xpubanddevicesare top-level because you need xpub to build a wallet.register,address,signlive under wallet and read descriptors from the saved config.create_txdefaults --offline_signer to true when built with hwi, so PSBTs carry the xpub data the hardware wallets need.Known limitations:
signproduces partial signatures only; finalize separately withfinalize_psbtHWI subcommand implementation
hwi xpubhwi deviceswallet … hwi registerwallet … hwi addresswallet … hwi signDevices tested:
Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md