cryptoadvance / specter-diy

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

Device PSBT Doesn't Work for Multisig #88

Closed mflaxman closed 3 years ago

mflaxman commented 3 years ago

Example of what my DIY returns on signing (this is a 1-of-4 on testnet):

cHNidP8BAH0CAAAAAcHvPZ3T/4G4m+ZUJWUA7wu2WsD48adq4dakp4JoRUJTAAAAAAD/////AtMSAAAAAAAAFgAUNTpLGUQtYD0P0IfyYHSm4B9K06iIEwAAAAAAACIAIE1pVeThYKqzZZmSDwOs1LWIkyF2CjS+UMG8yJ19SMShAAAAAAAAAAA=

C6A9438C-5655-4D44-B864-4A5E22DE2CAE_1_105_c

I'm not sure if this is a DIY or Desktop issue. Desktop will scan the device QR code from DIY yet still displays 0 signatures (and no error/warning): Screen Shot 2020-10-19 at 11 17 37 AM

Standard PSBT works great though, so perhaps this is more of a UX issue (warn user that this workflow doesn't work and that they need a device PSBT).

stepansnigirev commented 3 years ago

Did you import the multisig wallet to Specter-DIY? Device PSBT only works if the wallet is imported to the hardware - we can significantly reduce the QR code size if we assume that hardware wallet already knows everything about this wallet - cosigners and scripts.

To import multisig wallet to DIY you need to go to settings of the wallet in Specter Desktop > Export to hardware - scan it with DIY and confirm adding the wallet. Then the wallet will be able to sign compressed psbt as well.

I need to make a note in the UI somewhere...

mflaxman commented 3 years ago

Ah, I tried to import first but previous got an invalid PSBT error: IMG_1137

In retrospect, I realize what I did wrong. I was trying to scan a Cobo QR code and not a Specter QR code (for background, I have a ton of testing wallets that include seedpicker imports, so I was using that one and restored the seed to my DIY but didn't change the device type in Desktop to DIY).

mflaxman commented 3 years ago

Update: by first importing the wallet as a DIY and not a Cobo that fixed the problem. Device PSBT now signs TXs successfully: https://blockstream.info/testnet/tx/87bdf4af522c39ecb490846f61ce23c39e8321dcb4abb830303bd8a968d42607

So the issue in my mind is that DIY will sign a transaction that it's a participant in but isn't a registered wallet. It should throw a warning telling you not to do that (and to register the multisig first), though I can see an argument for allowing advancing users to transact in that case anyway if they want to be #reckless.

mflaxman commented 3 years ago

I'm on v1.4.0-pre1 btw

stepansnigirev commented 3 years ago

I think there is a warning that you are spending from unknown wallet, but I think we should make it larger - like a separate confirmation screen saying that "multisig wallet is unknown, import it with the QR code" or something.

mflaxman commented 3 years ago

There's an indication that it's an unknown wallet, but no indicator on how to make it known. Remember that I was aware of this issue and trying to import it, but the mistake was that I had it designated in Specter-Desktop as an "other" (paper wallet and not a Specter-DIY): FD1C0A14-41CB-4BCF-8536-7473C1D03DA6_1_105_c

My 2 recs:

  1. Throw a useful error here (https://github.com/cryptoadvance/specter-diy/issues/88#issuecomment-712290548) when trying import a wallet in the wrong way.
  2. Inform the user to import the wallet before signing (I assumed DIY didn't have the functionality). I'm not sure if there's room to add a note about change detection (if > 1 outputs), but it might help if you tell people why they should perform this step.
stepansnigirev commented 3 years ago

It shows a warning now and still offers to sign if you are sure, but without change verification. If the wallet is imported then change verification works.

multisig_unknown