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

Taproot Miniscript #498

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

Taproot Miniscript #498

wants to merge 28 commits into from

Conversation

odudex
Copy link
Member

@odudex odudex commented Dec 19, 2024

What is this PR for?

After having P2WSH Miniscript compatibility added to develop branch this PR adds compatibility for Taproot Miniscripts, first tests done targeting Liana as coordinator.

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Docs update
  • Other

Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 80.54054% with 36 lines in your changes missing coverage. Please review.

Project coverage is 94.16%. Comparing base (824e8ee) to head (965dbd7).

Files with missing lines Patch % Lines
src/krux/psbt.py 78.37% 16 Missing ⚠️
src/krux/pages/wallet_settings.py 54.16% 11 Missing ⚠️
src/krux/pages/login.py 33.33% 6 Missing ⚠️
src/krux/wallet.py 91.42% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #498      +/-   ##
===========================================
- Coverage    94.37%   94.16%   -0.21%     
===========================================
  Files           74       74              
  Lines         8104     8213     +109     
===========================================
+ Hits          7648     7734      +86     
- Misses         456      479      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odudex
Copy link
Member Author

odudex commented Dec 26, 2024

At this point I'm prioritizing SD card signing for miniscript, as Liana does not support QR codes.
I'm rethinking SD card flow, also having multisig in mind. On current commit I won't trim any data of PSBT, this way, in a multi-device setup you can:
Create PSBT on PC -> Sign in device 1 -> Sign in device 2 -> Broadcast on PC.
(Without needing to load device 1 signatures on PC prior to sign with device 2)

@jdlcdl
Copy link
Collaborator

jdlcdl commented Dec 29, 2024

Recording thoughts after testing this at commit d467330:

Without loading a mnemonic, using tools / descriptor addresses, to load a wallet descriptor from liana

  • WORKS: to load and find correct addresses for miniscript-wsh as well as miniscript-tr
  • TODO: however it shows "TR Miniscript" in both cases, instead of showing WSH when that's the case. (fixed in e4d6530)
    ... if doing the same when a mnemonic is loaded:
  • still works to find correct addresses just like above, still needs a correction on the title to reflect WSH when applicable (also fixed),
  • TODO: perhaps a mismatch check needs to be added, so that when krux is expecting miniscript WSH and instead loads a TR descriptor, a warning is displayed. (also fixed) Also, primary and heir keys may have different paths, so relaxing how strict that check is needs fine-tuning. (fixed: informs and is tolerant)

When signing a PSBT from liana w/ wallet descriptor:

  • WORKS: to raise a policy-mismatch if wrong wallet descriptor is loaded,
    and without a wallet descriptor loaded:
  • WORKS: to show policy "p2tr" and list of key-origin fingerprints, for a psbt that is miniscript-p2tr,
  • WORKS: to show policy "p2wsh" w/o list of keys but could be improved to include key-origin fingerprints (also improved) -- for a psbt that is miniscript-wsh,
    and overall:
  • WORKS: to display what is being spent, whether it's change/self-transfer and what the fees are
  • appears to create valid signatures, however I had to finalizepsbt and sendrawtransaction on a signet node because liana didn't show me the "broadcast" button.... but odudex has explained how to get around this for next time (save first, then sign, then update, then can broadcast)

Learned from odudex that we cannot count on specific derivation paths, different wallets (nunchuk's m/87'/0'/0' tap multisig) may use their own preferred paths and krux may need to support custom derivation path to create / recover for different wallet softwares. (resolved in bbb1d4d)

Also noted that liana conveniently uses the bech32 checksum, which is the suffix on the wallet descriptor, as a wallet identifier, and this is very nice... perhaps something for krux to adopt, to display when loading a descriptor.

Also todo: review/tests for embit changes, tests for krux too.

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