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

Descriptor urtype account #426

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

jdlcdl
Copy link
Collaborator

@jdlcdl jdlcdl commented Jul 23, 2024

Description

I'm not sure if this belongs, but I found this while trying to scan an xpub exported by seedsigner for sparrow, while testing/inspecting the seedsigner release candidate for v0.8.0.

Without this snippet of code which adds support for urtype Account to existing support for urtype Output, krux raises and catches the following error (while loading a wallet descriptor):

Invalid wallet: AttributeError("'dict' object has no attribute 'decode'",)

Example) The following qrcode is decoded by SeedQReader as:

wpkh([94613660/84'/1'/0']xpub6Bin9AoWthESPudSykC8r5Kd45jTz3r8HJNJ5DvumirCZmabnhL9BE67JtjcKJMLot8FHa5S2rnBe6VdcHEeXDbnNXn4x1DEDEanuXXwQ3C)#qkze980s

zpub

Conveniently, all 4 of the coordinators supported by both seedsigner/krux deal well with the xpubs exported for sparrow by seedsigner, and they're often scanned with better success, especially with a bad webcam, than the xpubs exported specifically for that coordinator. (For singlesig types, they're full descriptors with script wrapper and checksum, always with the mainnet "xpub" version bytes). In coordinators that support testnet mode... they force the keys to testnet and the resulting import is a "tpub" (this is not the case for krux, because krux respects the xpub exactly in watch-only mode, and would show a key mismatch if a wallet were loaded for testnet. I have a strong opinion that this is the most correct thing to do since krux is a signer that should favor being strict, while coordinators also have reason to favor being tolerant in order to create a usable wallet).

My reasoning for this code, is simply to verify between my only two signers... but it's more realistic that bitcoiners will be verifying descriptors from their coordinator (which works flawlessly whether setup by krux or seedsigner)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.62%. Comparing base (5fe3270) to head (9aeaefd).

Files Patch % Lines
src/krux/wallet.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #426      +/-   ##
===========================================
- Coverage    94.63%   94.62%   -0.02%     
===========================================
  Files           57       57              
  Lines         7078     7083       +5     
===========================================
+ Hits          6698     6702       +4     
- Misses         380      381       +1     

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

@odudex
Copy link
Member

odudex commented Jul 23, 2024

If the modification enables Krux to accurately load a descriptor from SS and display the correct receive and change addresses, then we could certainly implement it.

@tadeubas
Copy link
Contributor

Nice addition Jean! Plz add a simple test to cover this use case and I think it is good to merge!

@jdlcdl
Copy link
Collaborator Author

jdlcdl commented Jul 23, 2024

If the modification enables Krux to accurately load a descriptor from SS and display the correct receive and change addresses, then we could certainly implement it.

Yes, that is the intention of this pr... but it will only work for single-sig and mainnet (because seedsigner is currently exporting for mainnet, and for multisig, like krux, will only export a fraction of the cosigners at a time.)

@jdlcdl
Copy link
Collaborator Author

jdlcdl commented Jul 23, 2024

Nice addition Jean! Plz add a simple test to cover this use case and I think it is good to merge!

Thank you for reviewing. Tests added to check for UR:Output and UR:Account at the least.

Tests for UR:Bytes would also be nice (but I don't know of a coordinator or signer, yet, for that use case... even though the code does exist).

@tadeubas
Copy link
Contributor

I know Specter-DIY uses UR:Bytes somewhere, maybe for PSBT

@jdlcdl
Copy link
Collaborator Author

jdlcdl commented Jul 24, 2024

I know Specter-DIY uses UR:Bytes somewhere, maybe for PSBT

It looks like the Specter Desktop wallet-descriptors are being qr-encoded as an alphanumeric dictionary, rather than UR:Bytes. Whenever I stumble on a coordinator that is exporting UR:Bytes as their wallet backup, then I'll add tests for that.

We already have quite thorough UR:Bytes testing for PSBTs, it's in the ~/krux/tests/test_qr.py tests.

@jdlcdl jdlcdl marked this pull request as ready for review July 24, 2024 22:27
@odudex odudex merged commit 1ce3427 into selfcustody:develop Jul 30, 2024
7 checks passed
@odudex
Copy link
Member

odudex commented Jul 30, 2024

Thank you!

@jdlcdl jdlcdl deleted the descriptor_urtype_account branch August 6, 2024 12:58
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.

3 participants