bitcoin-core / HWI

Bitcoin Hardware Wallet Interface
MIT License
494 stars 197 forks source link

Ledger app 2.0.4 fails to sign #605

Closed achow101 closed 2 years ago

achow101 commented 2 years ago

Several users are reporting signing failures when using Ledgers with Bitcoin app version 2.0.4. The error is:

{"error": "('0x6a80', 'Error in <BitcoinInsType.SIGN_PSBT: 4> command', '')", "code": -13}
achow101 commented 2 years ago

I am unable to replicate the issue with speculos on self built app version 2.0.4. Edit: Forgot to set SPECULOS_APPNAME. However I was able to replicate using a Nano S Plus with the released app binary.

Looking at the APDUs sent, it seems to choke on receiving the key for a default wallet policy.

cc @bigspider

achow101 commented 2 years ago

Found it, Ledger doesn't like h in key origin info.

DeGxx commented 2 years ago

Hello, i have the same issue. My english is not very good, i don't understand if you have found a solution or not ?

achow101 commented 2 years ago

i don't understand if you have found a solution or not ?

I am working on a fix.

NicolasDorier commented 2 years ago

I don't think it fix it completely.

I have some tests that were passing before but now, I tried to sign a PSBT in testnet and I got the same error.

Then tried to sign a PSBT in mainnet, it worked, but only for legacy addresses. For segwit I get same error.

bigspider commented 2 years ago

@NicolasDorier do you have a failing PSBT from those tests? Is the previous transaction included in the PSBT?

achow101 commented 2 years ago

@NicolasDorier Do your PSBTs have mixed input script types (e.g. a P2WPKH and a P2TR)? Or do you have SIGHASH_DEFAULT set? If so, see #608 and #609 respectively. Those are issues with the Ledger apps and not something that HWI can particularly fix. Otherwise please open a new issue.

NicolasDorier commented 2 years ago

Will open new issue with PSBTs and mnemonic. I don't have SIGHASH_DEFAULT nor P2WPKH

NicolasDorier commented 2 years ago

So for record @achow101, the error is also sent if you are using segwit without the non_witness_utxo. (didn't check if that's the case for taproot)

This is a breaking change with their old app which merely show a warning.

bigspider commented 2 years ago

@NicolasDorier the new app is completely rewritten from scratch (and with a redisigned/incompatible API). I purposely added a number of limitations on what's allowed. In most cases, I don't expect the limitations to be relevant or even noticed (and limiting the user's behaviors to sane ones is useful for a number of reasons).

I wasn't aware that there is external software that still doesn't provide the non_witness_utxo, since it's been about two years that the segwit bug is known; I think I'll manage to add back the support (and the warning) by the next release (together with SIGHASH_DEFAULT support), since the severity of the segwit bug is rather limited after all.

On taproot, the non_witness_utxo is not required.

bigspider commented 2 years ago

Update: version 2.0.6 of the app now allows signing segwitv0 transaction with no non_witness_utxo, with a warning. SIGHASH_DEFAULT still planned for the next release.

martinbarilik commented 2 years ago

confirming that version 2.1.1 fixes the above error message ( previous version 2.0.x )

ididdid commented 2 years ago

A quick note : AFAIK, it is still a confirmed compatibility issue even with latest ledger firmware + ledger btc app and some multisig wallets like Sparrow. As today, we just tested again, on september 6th : sparrow 1.6.6 with latest firmware for Ledger Nano S (2.1.0) still has this issue. Same Issue with Ledger S+. (The only 2.1.1 firmware release available is only for Ledger Blue)

bigspider commented 2 years ago

@ididdid the current (2.0.6) version of the Ledger bitcoin app requires using SIGHASH_ALL instead of SIGHASH_DEFAULT. They are essentially equivalent in meaning, so it's safe to use SIGHASH_ALL.

Sparrow correctly uses SIGHASH_DEFAULT as the default choice, but I believe it is now possible to manually choose SIGHASH_ALL, and that should work. Please let me know if that is not the case, and I will investigate.

The upcoming version of the Ledger bitcoin app (ETA 1-2 months) will correctly support SIGHASH_DEFAULT as well.

debben commented 1 year ago

I'm seeing the same on 2.0.6 of the Ledger bitcoin app and sparrow 1.7.0. I verified the PBST created used SIGHASH_ALL but I'm still seeing this error.

bigspider commented 1 year ago

I'm seeing the same on 2.0.6 of the Ledger bitcoin app and sparrow 1.7.0. I verified the PBST created used SIGHASH_ALL but I'm still seeing this error.

Thanks for reporting!

That's strange − but the new Ledger bitcoin app (version 2.1.0) is coming within the next few weeks and it handles all sighash flags, so it will hopefully address this issue once and for all.

debben commented 1 year ago

I'm seeing the same on 2.0.6 of the Ledger bitcoin app and sparrow 1.7.0. I verified the PBST created used SIGHASH_ALL but I'm still seeing this error.

False alarm. This ended up being unrelated to any issue with HWI or the bitcoin app for ledger. Turns out I had a misconfiguration in the sparrow wallet (correct XPUBs but a duplicate fingerprint for one of the signers). This was enough to get the 0x6a80 mentioned in the issue. After correcting the wallet I was able to sign again with a nano s plus and 2.0.6 bitcoin app.

bim45454545 commented 1 year ago

How did you solve this problem of a duplicate fingerprint?

debben commented 1 year ago

I had to get the fingerprints for all signing keys and configure the sparrow wallet with them. In this particular wallet I had setup to be watch-only and didn't intend to use for signing. Once I assigned the correct fingerprint to each signer, sparrow would correctly interface to the ledger bitcoin app to sign.