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

[bugfix] DecodeQR: handle Compact SeedQR byte data that can be decoded to str #657

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Jan 3, 2025

Description

Fixes #656

The bug

The QR decoder (decode_qr.py) starts by interpreting QR data as str as it tries to figure out the QR format that was just scanned (e.g. animated psbt, address to verify, SeedQR, etc). In most cases, Compact SeedQR byte data will fail the conversion to str (raises UnicodeDecodeError; all good, we expect that) and the code will carry on to its next step where it assumes the data is still in bytes format.

But in the edge cases where the Compact SeedQR byte data CAN be converted to str (e.g. "abandon" x11 + "about" as reported in #656), the successful conversion to str then runs into errors in the Compact SeedQR decoding: the data isn't bytes, it's str at this point, and so the byte manipulation code errors out. The error is caught and the decoder reports the Compact SeedQR as invalid (even though it IS actually valid).

The fix

This PR simply checks and ensures that the data is in bytes format before the Compact SeedQR processing begins.

TODO: Longer term, the conversion to str should be eliminated altogether. But that approach was NOT taken in this PR in order to minimize the impact of this PR and minimize the amount of testing needed.



Testing

12-word Compact SeedQR that will fail with dev but succeed with this PR:

pr657_12_word

And a 24-word Compact SeedQR:

pr657_24_word


This pull request is categorized as a:

  • Bug fix

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • Yes

I have tested this PR on the following platforms/os:

@fedebuyito
Copy link
Contributor

fedebuyito commented Jan 3, 2025

Hi, @kdmukai . Well tested below and passed, thank you Keith!
image

@kdmukai
Copy link
Contributor Author

kdmukai commented Jan 3, 2025

@fedebuyito as a sanity check, can you run the new test on your dev device:

pytest tests/test_seedqr.py::test_compact_seedqr_bytes_interpretable_as_str

(see the README in tests/ if you don't have pytest or other test suite dependencies installed)

@fedebuyito
Copy link
Contributor

Good, it passed on both 3.10 and 3.12:

image
image

@kdmukai
Copy link
Contributor Author

kdmukai commented Jan 3, 2025

@fedebuyito that looks like it's running on your local dev machine. I want to see if you can run the test on your dev SeedSigner -- on the Pi Zero.

@fedebuyito
Copy link
Contributor

I am sorry Keith, I just saw that img on Pi Zero is not this commit PR version. Yes NOW!

image

@jdlcdl
Copy link

jdlcdl commented Jan 3, 2025

ACK tested, this all worked for me at cf83ebe

  • on Ubuntu Dev Desktop for automated tests
  • on RPi02w with RaspiOS manual build for automated tests
  • on RPi02w with RaspiOS manual build scanning 12w and 24w seeds (which confirmed to raise error on pi0 0.8.5-rc1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 Needs Code Review
Development

Successfully merging this pull request may close these issues.

Error reading Compact SeedQR generated by SeedSigner itself.
3 participants