devices: add taproot keypath spending support for bitbox02 #1
Conversation
hwilib/devices/bitbox02.py
Outdated
|
|
||
| if len(psbt_in.tap_internal_key) > 0: | ||
| # adding taproot keys to the keypaths to be checked | ||
| for pubkey , tup in psbt_in.tap_bip32_paths.items(): |
hwilib/devices/bitbox02.py
Outdated
| if len(psbt_in.tap_internal_key) > 0: | ||
| # adding taproot keys to the keypaths to be checked | ||
| for pubkey , tup in psbt_in.tap_bip32_paths.items(): | ||
| keypaths.update({pubkey: tup[1]}) |
There was a problem hiding this comment.
check that tup[0], the leaf hashes, is empty?
for clarity, you could unpack the tup into vars with descriptive names:
for pubkey, (leaf_hashes, key_origin_info) in ...
There was a problem hiding this comment.
This is nice! Done, thx :)
hwilib/devices/bitbox02.py
Outdated
| #taproot keypath input | ||
| psbt_in.tap_key_sig = sig | ||
| else: | ||
| # ser_sig_der() adds SIGHASH_ALL (only for ECDSA) |
There was a problem hiding this comment.
sounds like this function adds SIGHASH_ALL only for ECDA and not for Schnorr, but the whole function is not applicable to taproot, as the signature serialization is not a DER-encoding, but the raw 64 r|s bytes. i'd drop "only for ECDSA")
hwilib/devices/bitbox02.py
Outdated
| ) | ||
|
|
||
| # previous tx is needed for all non-Taproot transactions | ||
| if len(psbt_in.tap_internal_key) == 0: |
There was a problem hiding this comment.
the trouble is that we must provide the prevtx if any of the inputs is not taproot.
i think you'll need to have a pass through the inputs beforehand to figure this out.
potentially this function could be useful (but maybe easier to not use it and directly check in the PSBT 🤷 ): https://github.com/digitalbitbox/bitbox02-firmware/blob/987c4ce2c32308f12b31e69ec27bf0185662c42f/py/bitbox02/bitbox02/bitbox02/bitbox02.py#L78
There was a problem hiding this comment.
We talked about it, and decided that we could avoid that check, since it is already done by bb02 firmware
hwilib/devices/bitbox02.py
Outdated
| raise BadArgumentError( | ||
| "Previous transaction missing for input {}".format(input_index) | ||
| ) | ||
|
|
||
| found_pubkey, keypath = find_our_key(psbt_in.hd_keypaths) | ||
| keypaths = {} | ||
| for pubkey, keypath in psbt_in.hd_keypaths.items(): |
There was a problem hiding this comment.
keypath is better named key_origin_info, which contains the keypath but also the fingerprint.
hwilib/devices/bitbox02.py
Outdated
| @@ -650,7 +662,7 @@ def script_config_from_utxo( | |||
| # - https://medium.com/shiftcrypto/bitbox-app-firmware-update-6-2020-c70f733a5330 | |||
| # - https://blog.trezor.io/details-of-firmware-updates-for-trezor-one-version-1-9-1-and-trezor-model-t-version-2-3-1-1eba8f60f2dd. | |||
| # - https://github.com/zkSNACKs/WalletWasabi/pull/3822 | |||
| # The BitBox02 for now requires the prevtx, at least until Taproot activates. | |||
| # The BitBox02 requires the prevtx for all non-Taproot utxo. | |||
There was a problem hiding this comment.
that is false - it requires all prevtxs if not all of the inputs are taproot
hwilib/devices/bitbox02.py
Outdated
| # if utxo is Taproot, we don't need prevtx | ||
| if prevtx is None and len(psbt_in.tap_internal_key) == 0: | ||
| raise BadArgumentError( | ||
| "Previous transaction missing for input {}".format(input_index) | ||
| ) |
There was a problem hiding this comment.
the condition is not true - we might need prevtx anyway even if it is taproot (if other inputs are non-taproot).
I think you can just delete this whole section and let the bitbox02 handle the error
hwilib/devices/bitbox02.py
Outdated
| key_origin_infos = {} | ||
| for pubkey, key_origin_info in psbt_in.hd_keypaths.items(): | ||
| key_origin_infos.update({pubkey: key_origin_info}) |
There was a problem hiding this comment.
isn't this the same as key_origin_infos = psbt_in.hd_keypaths.copy()
There was a problem hiding this comment.
You're right! :D Fixed
hwilib/devices/bitbox02.py
Outdated
| key_origin_infos = {} | ||
| for pubkey, key_origin_info in psbt_out.hd_keypaths.items(): | ||
| key_origin_infos.update({pubkey: key_origin_info}) |
|
Also please rebase and squash the commits - the older commits still have debug stuff in them |
Beerosagos
force-pushed
the
bb02-tad-addr+sign
branch
from
2b3743e to
08c6896
Compare
Done! Thanks |
No longer needed, issue was fixed upstream.
No longer needed, taproot fields merged, psbt2 is not needed.
2720633 ci: Remove bitcoind patches (Andrew Chow) e769e49 ci: Remove coldcard ffilib patch (Andrew Chow) Pull request description: * Coldcard has fixed the ffilib issue upstream so our patch is no longer needed. * Bitcoin Core has merged Taproot PSBT fields, so our patch for it is no longer needed. * We did not need the PSBTv2 changes in Bitcoin Core for our tests, so these are removed as well. Top commit has no ACKs. Tree-SHA512: b5c98744a88479d635ad363599fc5e524f6fd121e4dd3058ba8d522a9dccdb0b10b7a603a2711709ea079f34a07a8d12db3f6680f3eba53180610218882bbede
|
@Beerosagos can add a commit here that bumps the bitbox02 dependency version? |
|
@Beerosagos i tested this with taproot, p2wpkh and multisig. Please open the PR upstream :) |
Beerosagos
force-pushed
the
bb02-tad-addr+sign
branch
2 times, most recently
from
642aa5a to
3df136b
Compare
Beerosagos
force-pushed
the
bb02-tad-addr+sign
branch
from
3df136b to
59379b3
Compare
No description provided.