Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic taproot BIP32 derivation & keyspend support #33

Merged

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Aug 10, 2022

  • Adds basic support for the BIP-371 Taproot psbt field PSBT_IN_TAP_BIP32_DERIVATION (0x16) to properly parse bip32 derivations in Taproot psbts.
  • Only supports the root ("internal") key; does nothing if the psbt is expecting to be signed by a key within a leaf.
  • Also adds PSBT_IN_TAP_INTERNAL_KEY (0x17), though I believe it won't be needed until keys within leaves are supported (internal key will match the PSBT_IN_TAP_BIP32_DERIVATION pubkey when num_leaf_hashes is zero).
  • Adds the matching PSBT_OUT_TAP_INTERNAL_KEY (0x05) and PSBT_OUT_TAP_BIP32_DERIVATION (0x07) fields to outputs.
  • Stores the data in InputScope.taproot_bip32_derivations which is a tuple: ([leaf_hashes], DerivationPath). Same for OutputScope.taproot_bip32_derivations.
  • Stores the internal key in InputScope.taproot_internal_key and OutputScope.taproot_internal_key.
  • Signing is backwards-compatible with "legacy" Taproot implementation for psbts that do not have the PSBT_IN_TAP_BIP32_DERIVATION field (i.e. they populate 0x06/InputScope.bip32_derivations).
  • Updates Taproot psbt signing to expect X-only pubkeys.
  • Updates test cases to test that taproot_bip32_derivations were populated and adds fixes so that the X-only pubkeys expectation passes.

I will also try to make a PR to update Specter Desktop to output the 0x16 and 0x17 BIP-371 fields. Because of the backwards-compatibility here, a Specter-DIY release can come first and then update Specter Desktop with these fields.


TODO:
Update test_tapoot.TaprootTest.test_sign_verify to use a psbt that includes the PSBT_IN_TAP_BIP32_DERIVATION field rather than the "legacy" psbt approach.

TODO:
test_psbtview.PSBTViewTest is failing on test_sign:

FAIL: test_sign (tests.test_psbtview.PSBTTest)
Test if we can sign psbtview and get the same as from signing psbt
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kdmukai/dev/embit/tests/tests/test_psbtview.py", line 113, in test_sign
    self.assertEqual(ser.getvalue(), psbt.serialize())
AssertionError: b'psb[844 chars]0\x00"\x02\x02\xa9\x81\xb3?~\xb9*\n\xcf77P\x92[890 chars]\x00' != b'psb[844 chars]0\x00\x01\x01\x1f\x80\x96\x98\x00\x00\x00\x00\[2246 chars]\x00'

This failure pre-dates these changes so something was either already broken or something is off with my local setup.

@kdmukai
Copy link
Contributor Author

kdmukai commented Aug 11, 2022

FYI - still making improvements and tweaks so this PR should be considered DRAFT (but I don't seem to have permissions to revert the PR status to DRAFT).

@stepansnigirev stepansnigirev marked this pull request as draft August 11, 2022 14:46
@stepansnigirev
Copy link
Contributor

Converted to draft

Copy link
Contributor

@stepansnigirev stepansnigirev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PSBT parsing should not fail if the library can't sign - maybe it's not used for signing at all. You can raise when signing, but I'd better ignore inputs we don't know how to sign.
For example, when you try to sign PSBT transaction with a wrong key it doesn't raise an error, it just returns 0 as a number of added signatures. I think for tap leafs we should do the same.

src/embit/psbt.py Outdated Show resolved Hide resolved
src/embit/psbt.py Outdated Show resolved Hide resolved
tests/tests/test_psbtview.py Outdated Show resolved Hide resolved
@stepansnigirev stepansnigirev marked this pull request as ready for review October 9, 2022 16:19
@stepansnigirev stepansnigirev merged commit d8c638e into diybitcoinhardware:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants