selfcustody / krux

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

Sign base64 PSBTs From SD card #429

Closed odudex closed 3 months ago

odudex commented 3 months ago

Description

When the SD PSBT signing was optimized, it lost the ability to work with base64 encoded PSBTs. @tadeubas caught this bug, which affects PSBTs exported by Blue Wallet as base64 text. This PR addresses the issue by altering the parsing and saving processes to correctly handle base64 data.

What is the purpose of this pull request?

tadeubas commented 3 months ago

Did a few tests that worked on my end

tadeubas commented 3 months ago

But we have to check these broken tests:

FAILED tests/pages/home_pages/test_home.py::test_sign_psbt - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/pages/home_pages/test_home.py::test_psbt_warnings - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/pages/home_pages/test_home.py::test_sign_p2tr_zeroes_fingerprint - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/test_psbt.py::test_init_singlesig_from_sdcard - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/test_psbt.py::test_init_multisig_from_sdcard - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/test_psbt.py::test_sign_singlesig_from_sdcard - ValueError: Error loading PSBT file: Invalid PSBT magic
FAILED tests/test_psbt.py::test_sign_multisig_from_sdcard - ValueError: Error loading PSBT file: Invalid PSBT magic
odudex commented 3 months ago

Yes, tomorrow I'll keep working on it. Fell free to modify/optimize too!

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.65%. Comparing base (1ce3427) to head (88c2590).

Files Patch % Lines
src/krux/pages/home_pages/home.py 82.60% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #429 +/- ## =========================================== - Coverage 94.65% 94.65% -0.01% =========================================== Files 57 57 Lines 7078 7110 +32 =========================================== + Hits 6700 6730 +30 - Misses 378 380 +2 ```

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

jdlcdl commented 3 months ago

As of fd42157,

This branch is working for me.

I noticed that to scan a psbt QR and to save signed to sdcard, the file will be binary; this makes good sense. Only when reading a base64 psbt from sdcard and writing the signed psbt back to sdcard will the format be preserved; this makes sense too. To read binary or base64 psbts from sdcard, can still sign to qr... all these possibilities are great!

IMPROVEMENT: As @odudex noted in chat, to enable ".txt" files, since sparrow defaults a base64 filename like "TxSendLabel.psbt.txt" would be an improvement, otherwise krux won't find the psbt file on sdcard. Because bip174 hints that ".psbt" is in binary format, I also prefer supporting ".txt" extensions for psbts that are base64.

jdlcdl commented 3 months ago

As of 88c2599,

Working for me, as well as with very intuitive filename extensions and ".txt" supported.

tadeubas commented 3 months ago

just for record, BW exports the .psbt file but in B64 mode, so we can't assume it is just .txt that uses this format.

odudex commented 3 months ago

As it is, it will open .psbt, .psbt.txt, and .txt files. I will save the file with its original extension unless it is .txt, in which case it will be replaced with .psbt.txt.