selfcustody / krux

Open-source signing device firmware for Bitcoin
https://selfcustody.github.io/krux/
Other
175 stars 34 forks source link

Descriptor urtype account #426

Closed jdlcdl closed 1 month ago

jdlcdl commented 2 months ago

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?

codecov[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odudex commented 2 months ago

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 commented 2 months ago

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

jdlcdl commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

jdlcdl commented 2 months ago

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.

odudex commented 1 month ago

Thank you!