sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.
https://sparrowwallet.com/
Apache License 2.0
1.33k stars 188 forks source link

QR Scanning Legacy QR code #78

Closed newtonick closed 3 years ago

newtonick commented 3 years ago

I have a DIY signing device creates an animated QR code (signed PSBT) in the in the format UR:BYTES/1OF4/[HASH]/[URFRAGMENT1] (in this example there would be 4 QR codes animated.

No matter what I try when scanning the QR when signing a transaction. It does not work. At least there is no indication that the QR codes are being captured at all. I'm using an older MacBook Air 2012 model.

I'm able to scan the "Show QR" code when in "Use Legacy Encoding (Cobo Vault)" without any problem. But after I sign the PSBT on my DIY device, I'm not able to bring it back into sparrow via QR code scanning.

I'm not sure if this is supposed to work.

craigraw commented 3 years ago

There are 2 UR formats Sparrow supports:

  1. The legacy UR1.0 format, as documented here: https://github.com/CoboVault/Research/blob/master/papers/bcr-0005-ur.md
  2. The UR2.0 format, as documented here: https://github.com/BlockchainCommons/Research/blob/master/papers/bcr-2020-005-ur.md

It looks like you are following the UR1.0 format, although it's not clear why it's failing. There might be some clues in the log file (Help > Show Log File).

newtonick commented 3 years ago

The logs are not showing anything helpful as far as I can tell. I can scan the animated QR code without issue. Sign it on my DIY device and then take the signed PSBT to another wallet like Bluewallet and it scans it in just fine. So, I'm not sure what is up. I'll try a few more things. Maybe I need to watch a BTCSession video to make sure I'm doing it right. :-)

craigraw commented 3 years ago

Is it possible to share one of the ur fragments? Or perhaps even a video of a testnet PSBT animation?

It's unlikely you're doing anything wrong, probable that there is some incompatibility.

newtonick commented 3 years ago

I need to get testnet setup, then I can share with you.

I was able to implement UR2.0 format, but now I'm getting a different error

Screen Shot 2021-03-23 at 12 13 42 PM

2021-03-23 12:09:21,744 ERROR [JavaFX Application Thread] c.s.s.AppController [null:-1] Error scanning QR
com.sparrowwallet.sparrow.control.QRScanDialog$URException: Error parsing PSBT from UR type crypto-psbt
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.extractResultFromUR(Unknown Source)
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.changed(Unknown Source)
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.changed(Unknown Source)
    at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(Unknown Source)
    at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.fireValueChangedEvent(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.markInvalid(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.set(Unknown Source)
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.WebcamService.readQR(Unknown Source)
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.WebcamService$1.call(Unknown Source)
    at com.sparrowwallet.sparrow@1.3.0/com.sparrowwallet.sparrow.control.WebcamService$1.call(Unknown Source)
    at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Unknown Source)
    at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
    at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$6(Unknown Source)
    at java.base/java.security.AccessController.doPrivileged(Unknown Source)
    at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$7(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
    at java.base/java.lang.Thread.run(Unknown Source)
Caused by: com.sparrowwallet.drongo.psbt.PSBTParseException: Unverifiable partial signatures provided
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.parseInputEntries(Unknown Source)
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.parse(Unknown Source)
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.<init>(Unknown Source)
    ... 19 common frames omitted

Any ideas on the cause?

newtonick commented 3 years ago

I got it to work when I removed the psbt trimming I was doing. But the 40 part animated qr code was brutal with no indicator of any kind if it is working. Would love to see some kind of QR scanning indicator. With UR2.0 there is a estimated_percentage_complete() function. would be great if this percentage was visible on the window capturing the qr codes.

craigraw commented 3 years ago

Glad to hear you've upgraded to UR2.0, and it's working.

I got it to work when I removed the psbt trimming I was doing.

What did you mean by 'psbt trimming'?

would be great if this percentage was visible on the window capturing the qr codes.

Added in 11a201b

newtonick commented 3 years ago

What did you mean by 'psbt trimming'?

I don't really know what I'm doing, so go easy on me. I'm trying to figure this out and learn as I go. I am attempting to remove parts of the PSBT that are not required to sign, as to make the amount of data passed back by the QR smaller in size. So specifically, I was taking the original received PSBT and only adding to it the partial_sigs on all the inputs. I got the idea from specter desktop/specter dyi. It appears to work with Specter Desktop and BlueWallet.

Here is the logic I basically took from specter python code: https://github.com/cryptoadvance/specter-desktop/blob/10f0c332cfa5b4a93404231529ed68df3c941869/src/cryptoadvance/specter/devices/hwi/specter_diy.py#L90

Added in 11a201b

This is pretty cool, great work! Reminds me of the cylons in battlestar galactica.

craigraw commented 3 years ago

So specifically, I was taking the original received PSBT and only adding to it the partial_sigs on all the inputs.

I'm still a little unsure what trimming is being performed - by this description you're adding, not trimming. Sparrow performs a number of checks on the PSBT when it parses it (for example, testing the partial sigs match the provided xpubs) - possibly these checks are failing.

I've added logging in 9c6dbee so you'll be able to see if this is the case - look in sparrow.log when you parse the QR code with the trimmed PSBT.

newtonick commented 3 years ago

Here is the log error when parsing the qr code (testnet and sparrow version 1.4.0)

2021-05-07 13:55:12,491 ERROR [JavaFX Application Thread] c.s.s.AppController [null:-1] Error scanning QR
com.sparrowwallet.sparrow.control.QRScanDialog$URException: Error parsing PSBT from UR type crypto-psbt
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.extractResultFromUR(Unknown Source)
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.changed(Unknown Source)
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.QRScanDialog$QRResultListener.changed(Unknown Source)
    at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(Unknown Source)
    at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.fireValueChangedEvent(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.markInvalid(Unknown Source)
    at javafx.base/javafx.beans.property.ObjectPropertyBase.set(Unknown Source)
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.WebcamService.readQR(Unknown Source)
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.WebcamService$1.call(Unknown Source)
    at com.sparrowwallet.sparrow@1.4.0/com.sparrowwallet.sparrow.control.WebcamService$1.call(Unknown Source)
    at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Unknown Source)
    at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
    at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$6(Unknown Source)
    at java.base/java.security.AccessController.doPrivileged(Unknown Source)
    at javafx.graphics/javafx.concurrent.Service.lambda$executeTask$7(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
    at java.base/java.lang.Thread.run(Unknown Source)
Caused by: com.sparrowwallet.drongo.psbt.PSBTParseException: Unverifiable partial signatures provided
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.parseInputEntries(Unknown Source)
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.parse(Unknown Source)
    at com.sparrowwallet.drongo@0.9/com.sparrowwallet.drongo.psbt.PSBT.<init>(Unknown Source)
    ... 19 common frames omitted
craigraw commented 3 years ago

Ok - from the log Unverifiable partial signatures provided we see that Sparrow is trying to verify the partial signatures, but can't. To understand why you'll need to examine the details of the PSBT - there are 3 components in verifying a signature (the public key, the signature and the hash of the previous transaction/utxo, depending on what the script type is). The first two are provided as key/value of the partial signature field, the last is from the non_witness_utxo or witness_utxo.

6102bitcoin commented 3 years ago

@newtonick is is unclear what data you were trying to pass over the QR with psbt trimming.

It appears that sparrow is performing additional checks on the data that specter/blue are not?

Examples of a (testnet) PSBT that fails to scan would be useful in identifying the issue.

Outstanding Action: Identify an example of a PSBT that fails to scan. Proposed Priority: Low

eliasnaur commented 3 years ago

This bug sounds like the Sparrow side of https://github.com/cryptoadvance/specter-diy/issues/150. Given https://github.com/cryptoadvance/specter-diy/issues/150#issuecomment-849573046, it sounds like Sparrow may be able to accept DIY signatures.

I have a .psbt file signed by Specter DIY that results in "Unverifiable partial signatures" in Sparrow 1.4.1. Let me know where to send it.

craigraw commented 3 years ago

Great, you can email it to me at craig@sparrowwallet.com

eliasnaur commented 3 years ago

Done.

craigraw commented 3 years ago

In this case, the PSBT provided does not contain either the non_witness_utxo or the witness_utxo fields, which means the hash for verifying the signatures cannot be calculated.

I am looking at providing a way for the user to select to proceed with loading the PSBT even when the signatures could not be verified. However, when the UTXO data cannot be found (because the loaded PSBT could not be combined with a prior PSBT) then Sparrow will need to show an 'Invalid PSBT' error dialog regardless.

craigraw commented 3 years ago

Implemented in 600a77d. Sparrow will now allow PSBTs to be loaded that do not contain UTXO data, so long as a PSBT is already loaded in the active window that contains this data and can be combined with. Note that certain checks on the provided redeem and witness scripts have had to be weakened to warnings in the logs to accommodate this (see https://github.com/sparrowwallet/drongo/commit/42ffeb95650c56bffbd5ec8f8e8f38d91faaab3f for details).

I don't have a Specter DIY device, so I'd appreciate some testing here.

eliasnaur commented 3 years ago

EDIT: solved build problem.

eliasnaur commented 3 years ago

I can confirm that Sparrow now accepts .psbt files signed by a Specter DIY device. Thank you for the swift help.

craigraw commented 3 years ago

Great, thanks for the feedback. I'm going to close this off.