cryptoadvance / specter-diy

DIY airgapped hardware wallet that uses QR codes for communication with the host
MIT License
441 stars 73 forks source link

Cooperating with Blockchain Common's LetheKit team on QR standards, etc. #57

Closed ChristopherA closed 3 years ago

ChristopherA commented 4 years ago

Blockchain Commons would be interested in working with you to define some better cross-wallet standards for QR-based airgap exchange of master keys, child xprvs & xpubs, signing requests, and new wallet requests, for use with current and future versions of LetheKit as some of our other wallet projects such as our multisig iOS wallet FullyNoded-2, etc.

You also may be interested in the C-implementations of some our cryptographic libraries, in particular for two-level shamir restoration of keys compatible with SLIP-39.

Other than issues in our repos, what is the best channel for having further discussions? We currently use a private Signal group, and plan to move current thoughts into a currently very out-of-date AirgappedSigning repo.

See companion issue https://github.com/BlockchainCommons/AirgappedSigning/issues/2

cc: @fonta1n3 @ksedgwic @wolfmcnally @howech @kanzure @msgilligan @bucko13 @dhruvbansal @unchained-capital @devrandom

-- Christopher Allen

kanzure commented 4 years ago

While we're at it, we might as well talk about animated QR codes.

stepansnigirev commented 4 years ago

@ChristopherA we will be happy to collaborate :)

I am in the signal group, but I would prefer to use github issues for these kinds of discussions for a few reasons:

Regarding the QR codes, we are currently using base64 encoded PSBT for transactions, and to import wallets we use a QR code with <name>&<descriptor> format. Here is a brief description of the format we use: https://github.com/cryptoadvance/specter-diy/blob/master/docs/communication.md

It's not ideal, and we would like to switch to a standard if there is one. We do certain optimizations to make PSBT smaller - throwing away fields that wallet can construct itself, using custom fields to send a derivation path and wallet id instead of derivations for every cosigner, etc. But we work with standard PSBT as well.

One thing that I would like to change - encoding. Base64 encoding doesn't make sense for QR codes as it forces QR code library to use byte representation, so raw bytes or bech32 would be better. AFAIK bech32 is even more efficient than raw bytes in QR codes. I know that Electrum is using base43, the most space-efficient encoding for QR codes, but I believe that it is a terrible choice for a standard because it doesn't allow decoding/encoding in chunks, so MCU-based devices will have to waste a lot of memory for this conversion. On the other hand bech32 is in every bitcoin library, so reusing it would make sense. Generally, I would suggest supporting both bech32 and base64 and detect which one is used after scanning. It's easy to detect PSBT because it has fixed magic bytes in the beginning. We can also introduce standard magic bytes for wallet import, address verification and other QR-commands.

For animated QR codes @gorazdko is working on that and we use a homebrew format where we just put a prefix with current and total index: p2of4 <data> (https://github.com/cryptoadvance/specter-diy/issues/47)

We would be interested in some kind of standard for animated QR codes, and probably there are even industrial standards out there, but I didn't do any research on the topic.

Fonta1n3 commented 4 years ago

FWIW it sounds like Specter and FullyNoded-2 are working in a very similar way, and are possibly compatible out of the box by utilizing descriptors and psbt's. As soon as I finish the initial mainnet release for FullyNoded-2 I can play around with Specter and am sure it would not take much effort on our end to get them working with each other.

ChristopherA commented 4 years ago

A key problem for using bech32 for things larger than 40 bytes (320 bits) is that @pwuille optimized it for that size as that is the size of a segwit transaction. If you go to much over that size you loose its error correction ability. I've been trying to persuade him to use his simulation to create a bech64 or bech128. He said it isn't that hard to come up with a not quite perfect but "good enough" solution for larger sizes.

So bech32 is a great encoding scheme for 32 byte keys plus some metadata up to 40 bytes, but I think only ok for 64 byte XPRVs (it isn't that much larger than 40 bytes) but begins to be less useful at 80 bytes. Not sure where the limits on the larger sizes are though. At some point it can't error-correct at all.

stepansnigirev commented 4 years ago

QR codes have error correction by themselves, so error correction is not required on the encoding side. hrp can be used to encode command or data type (like tx signing, address verification, xpub etc) and maybe even frame number for animated QR codes. 4-byte checksum at the end can be dropped if it doesn't add any value. The main motivation for bech32 is to reuse encoding algorithms that are in the software anyways, keep it encodable in a stream and be more space-efficient than base64 or json. bech32 is used for larger data already - lightning invoices for example.

ChristopherA commented 4 years ago

I asked about this on Twitter and got an answer from @sipa: https://twitter.com/christophera/status/1250642189366919168?s=21

He offered to give some new values for a bech128 if we can give him target error rates https://twitter.com/christophera/status/1250669486199300096?s=21

I know QR code have own EC, but I like bech32 now because it is voice readable as well.

RCasatta commented 4 years ago

Hello, I am working on offline PSBT signer for CLI and android: https://github.com/RCasatta/firma

Regarding QR codes for PSBT my vote are for:

Regarding copy and pasting I am fine with the base64 encoding, mostly because it's what we have already

Fonta1n3 commented 4 years ago

Hello, I am working on offline PSBT signer for CLI and android: https://github.com/RCasatta/firma

Regarding QR codes for PSBT my vote are for:

  • binary representation which I think it's the most efficient way, I don't get how the base43 encoding of electrum could be more efficient.

  • qr code structured append to merge multiple qrs.

  • I prefer a list of qr codes instead of animated one for 2 reasons: list of qr codes are easier to print, they better give an idea of how big is the PSBT (let's say some attacker want to snick an attack inside the QRs)

Regarding copy and pasting I am fine with the base64 encoding, mostly because it's what we have already

I definitely agree the list of QR’s is better, user should have to tap/click/swipe through each one otherwise it can get messy.

gorazdko commented 4 years ago

list of qr codes are easier to print

but also harder to scan and reconstruct if individual parts dont have a header as is the case with structured append. I would be nervous if my wallet throws errors on my funds because a scanner accidentally scans adjacent Qrs.

RCasatta commented 4 years ago

but also harder to scan and reconstruct if individual parts dont have a header as is the case with structured append. I would be nervous if my wallet throws errors on my funds because a scanner accidentally scans adjacent Qrs.

QR code structured append has headers and also some parity check https://github.com/RCasatta/firma/blob/master/lib/src/common/qr.rs#L157 some examples https://segno.readthedocs.io/en/stable/structured-append.html

ChristopherA commented 4 years ago

Wow, another QR standards esoterica I’ve never heard of before: ECC2 00 Structured Append. Max 16 QR codes. http://www.keepautomation.com/tips/data_matrix/ecc_200_data_matrix_features.html

I wonder if this is already supported in any of the native QR code readers like iOS?

— Christopher Allen

RCasatta commented 4 years ago

I wonder if this is already supported in any of the native QR code readers like iOS?

Cannot speak for iOS, on Android I am using https://github.com/journeyapps/zxing-android-embedded which does not support it, however it let's you access scanned raw bytes which contains structured append headers and data.

stepansnigirev commented 4 years ago

Storage space of the QR codes in different modes is tricky. Binary is very efficient, but other modes are good as well if used properly. https://en.wikipedia.org/wiki/QR_code#Storage

L-40 QR code can store:

So the difference between these modes and binary mode is:

What I don't like about binary mode:

Structure append is a nice feature, but I am not sure if it is widely supported in standard libraries and scanners. If I understand correctly setting structure append bytes and scanning these codes require library support and can't be done by analyzing or changing the text stored in the QR code. So I am expecting many webdevs will ignore this feature just because they use a simple library with api like qrcode("text").

So my suggestion would be to develop a base32 (maybe bech32) data structure that can have a flag for sequential QR code and <current offset><total len> fields in it. We could encode the command type in hrp part so it's clear from hrp if it is a transaction, xpub, wallet descriptor, text message to sign or whatever.

RCasatta commented 4 years ago

Thanks for your points. Didn't know about industrial scanner.

I think you shouldn't use build-in readers for multiple qrs anyway, I would avoid to use it and manually append bech32 strings.

gwillen commented 4 years ago

I've done some work on PSBT, and I've said a few times that I distrust animated QR codes for precisely the reason @RCasatta gave earlier -- they make it too hard to verify what's going on, and make sure nothing is happening behind your back. I prefer multiple QR codes for this reason.

I never knew there was a standard for structured append. I would love to see a survey of how widely supported they are by tools and libraries. If support is thin, then it does seem better to handle sequential encoding with a higher-level protocol (but I agree that it's important to include enough information in the code to reconstruct out-of-order and detect missing pieces -- <current offset><total len> seems like a good way to encode that, as long as there's also a checksum over the whole decoded message at the end. It's important that the checksum be over the WHOLE final message, not just each of the parts, to prevent accidentally or maliciously mixing up pieces of different messaages undetectably.)

Now that I understand what @stepansnigirev is saying about base43, it does seem like absolutely the right approach to use here. It is in some sense just a matter of bookkeeping, whether one feeds binary data directly to the QR library, or encodes it into base43 first and then lets the QR library pack that into binary, at roughly the same final efficiency. In light of that equivalence, the intermediate state being text seems like a big win, for the reasons discussed in his comment. Perhaps bech43 is what we need. :-)

One thing I think could be an issue that I haven't seen discussed yet: different readers have different maximum QR code density they can reasonably scan, generally well below the maximum of the format. Do people generally just pick a maximum density that is widely compatible, and use that for everything?

gorazdko commented 4 years ago

This is an example of a PSBT animation. Parts of the PSBT are animated and below the animation is a text of the complete transaction.

qr_animation

A hw wallet can scan this animation and reconstruct it automatically. The user then can/should verify/compare it with the text. Verifying individual chunks is not more verifiable it just takes longer.

I think an animation and a list are the same thing, or at least they are not incompatible.

ChristopherA commented 4 years ago

@stepansnigirev It might be good to transfer this issue to https://github.com/BlockchainCommons/AirgappedSigning/ but I believe it requires someone with admin privs.

Instructions for moving issues is at https://help.github.com/en/github/managing-your-work-on-github/transferring-an-issue-to-another-repository

-- Christopher Allen

ChristopherA commented 4 years ago

@gorazdko wrote:

This is an example of a PSBT animation.

Interestingly, when I read this with Peter Denton's @Fonta1n3 iOS QR Vault, which I believe uses the iOS default QR API, it never sees anything but the 4th frame. Never the 1st, 2nd or 3rd. So there is something interesting going on when iOS sees a rotating QR.

-- Christopher Allen

Fonta1n3 commented 4 years ago

@stepansnigirev

Can you point me to the file in your codebase where you manipulate the psbt to remove the extra fields to reduce the QR size of the signed psbt? Curious how to properly achieve this. I am able to manipulate the json as returned by decodepsbt but converting it back to base64 that Bitcoin Core understands is not working.

gorazdko commented 4 years ago

So there is something interesting going on when iOS sees a rotating QR

i dont know what could be wrong, the gif works with my android and with specter hw. People do seem to animate QRs on iOS with ~10 FPS with much larger frames and even more with fancy algos such as fountain code:

https://divan.dev/posts/animatedqr/

stepansnigirev commented 4 years ago

Can you point me to the file in your codebase where you manipulate the psbt to remove the extra fields to reduce the QR size of the signed psbt?

https://github.com/cryptoadvance/specter-diy/blob/master/src/main.py#L187-L199

I manipulate a parsed PSBT transaction here. You probably also have some kind of parser, so just remove all these fields from input and output scopes :)

ChristopherA commented 4 years ago

Other discussion about animated QR codes here: https://github.com/cryptoadvance/specter-diy/pull/55

stepansnigirev commented 4 years ago

@stepansnigirev It might be good to transfer this issue to https://github.com/BlockchainCommons/AirgappedSigning/ but I believe it requires someone with admin privs.

Instructions for moving issues is at https://help.github.com/en/github/managing-your-work-on-github/transferring-an-issue-to-another-repository

-- Christopher Allen

I failed at transferring :( It only allows me to move issues within the organization.

You can only transfer issues between repositories owned by the same user or organization account.

I suggest starting a new issue in Blockchain Commons and put here a reference to that one. We can also summarize key points we discussed here in the new issue.

ChristopherA commented 4 years ago

So that we don’t unnecessarily clog up the inbox maintainers of this repo, I’ve created a new issue to continue discussions at https://github.com/BlockchainCommons/AirgappedSigning/issues/4

— Christopher Allen

stepansnigirev commented 3 years ago

Happy to see standards! We will work on integration of these standards to Specter.