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

Signing with Trezor is broken #38

Open
dennisreimann opened this issue Aug 18, 2021 · 18 comments
Open

Signing with Trezor is broken #38

dennisreimann opened this issue Aug 18, 2021 · 18 comments

Comments

@dennisreimann
Copy link
Member

Describe the bug

Importing a Trezor wallet using the Vault works, but signing a transaction does not.

Your BTCPay Environment (please complete the following information):

  • BTCPay Server Version: 1.2.1

  • BTCPay Server Vault app version: 2.0.1

  • Deployment Method: Dev Env

Your local environment (please complete the following information):

  • Your operating system: macOS
  • Browser: Firefox

Your hardware wallet details (please complete the following information):

  • Hardware wallet name: Trezor
  • Hardware wallet version: Trezor Bridge 2.0.27
  • Hardware wallet firmware, bootloader, microcontroller verisons (where applicable): 1.9.3 and 1.10.2 (latest)

Additional context

Original bug report by @pavlenex, maybe he can provide additional information. I confirmed the issue.

Screenshots / Video / GIf (if applicable)

1.9.3 Firmware

Upload.from.GitHub.for.iOS.MOV

1.10.2 Firmware

Upload.from.GitHub.for.iOS.MOV
@pavlenex
Copy link
Contributor

cc @NicolasDorier

@NicolasDorier
Copy link
Member

checking

@NicolasDorier
Copy link
Member

the new release of hwi doesn't see to solve any trezor related stuff... that's weird.

@mikeyyyv
Copy link

I'm having the exact same issue here. Thank you.

@NicolasDorier
Copy link
Member

@mikeyyyv trying to find out what happened.

@NicolasDorier
Copy link
Member

Does the Trezor T has this issue? Sadly I don't have a trezor 1...
Also @mikeyyyv have you tried, in the send wallet, go to advanced and check

image

@pavlenex
Copy link
Contributor

Yes issue reported on Trezor T as well @NicolasDorier.

@NicolasDorier
Copy link
Member

I lost the cable, so I need to order a new one. Will take a bit of time to look at it

@gruve-p
Copy link

gruve-p commented Aug 25, 2021

@pavlenex @mikeyyyv @dennisreimann
On Trezor devices its mandatory to include non witness UTXO otherwise signing will always fail:
On the send wallet go to advanced settings and check Always include non-witness UTXO if available

@dennisreimann
Copy link
Member Author

@gruve-p @NicolasDorier Yes, I can confirm that setting the "Always include non-witness UTXO" fixes the problem.

Can we circumvent this issue by detecting if it is a hardware wallet that needs this setting and include it later on, instead of relying on the user to know about it and dig it up from the "Advanced settings"?

Upload.from.GitHub.for.iOS.MOV

@gruve-p
Copy link

gruve-p commented Aug 25, 2021

Maybe best way is if a Trezor device is detected that the checkbox is automatically checked? Or maybe save user preferences when sending so user does not need to check it again ( and write a big text that if you are a Trezor user that you need to check that box the first time)

P.S. This issue was reported to Nicolas on 12 April 2021 (via Mattermost) while porting BTCPayServer.Vault to GRSPayServer.Vault

@dennisreimann
Copy link
Member Author

The PSBT one is about to sign gets created before we even know which wallet will be used for signing. I think we need to tackle this differently -- maybe by specifying preconditions which need to be met and which we can check once the wallet is defined. This way we could deliberately point out to the user what needs to be done or which kind of information is missing.

@NicolasDorier
Copy link
Member

@dennisreimann while we can't automatically add the previous transactions once we clicked through, I think we can still pass this information to be available in the signing screen and show error message. Bit pain in the ass though.

@piliugin-anton
Copy link

Did you tried to report the problem to Trezor team?

@NicolasDorier
Copy link
Member

@piliugin-anton they probably know already about it, but won't fix it. Ask them why their wallet requires the previous tx and not only non_witness_utxo.

@gruve-p
Copy link

gruve-p commented Nov 11, 2021

cc @prusnak @matejcik

@prusnak
Copy link

prusnak commented Nov 11, 2021

Ask them why their wallet requires the previous tx and not only non_witness_utxo.

We need previous TX to mitigate the design flaw of Bitcoin which makes it possible to steal user funds via really high transaction fee.

Good news is that the issue will be fixed with Taproot. When all inputs are Taproot then it is not required to provide previous TX to Trezor device because Taproot fixes this design flaw.

@Monique1965

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants