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

trezor: restore support for external inputs #590

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

matejcik
Copy link
Contributor

partially reverts #581

implements unverified external inputs for Trezor, per https://github.com/trezor/trezor-firmware/blob/master/docs/common/communication/bitcoin-signing.md#external-inputs

Unit tests are passing for me locally, except big_tx in the newly added test suite, but that's bitcoincore complaining that "This wallet has no available keys (-4)". I have no idea what to do about that -- maybe something went wrong because of the way I subclass?

@matejcik matejcik force-pushed the restore-trezor-external branch from 790807c to 6054be5 Compare March 23, 2022 10:52
@matejcik
Copy link
Contributor Author

Fixed big_tx failure by extending the size of the keypool. I am still not entirely sure why it carries over between test runs but 🤷‍♀️

Also added support for forwarding signature data to Trezor if available. This enables Trezor T "verified external input" mode. Adding support for SLIP-19 ownership proofs would be simple, assuming they are either (a) added to BIP-174 or (b) SLIP-19 is changed to use a vendor prefix.

@achow101
Copy link
Member

Since this requires disabling safety checks, I'm not sure that this is something that we want to have, or would be useful to users.

@prusnak
Copy link
Collaborator

prusnak commented May 18, 2022

I thought lots of people use HWI to sign mixed environment transactions that bring external inputs into the mix, no?

@achow101
Copy link
Member

I thought lots of people use HWI to sign mixed environment transactions that bring external inputs into the mix, no?

No idea. No one's complained about the latest release which includes #581.

Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left couple of nits when it comes to versioning.

Comment on lines 61 to 62
* 1 - Since firmware 1.10.6, safety checks must be disabled.
* 2 - Since firmware 2.4.4, safety checks must be disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the correct firmware versions:

Suggested change
* 1 - Since firmware 1.10.6, safety checks must be disabled.
* 2 - Since firmware 2.4.4, safety checks must be disabled.
* 1 - Since firmware 1.11.1, safety checks must be disabled.
* 2 - Since firmware 2.5.1, safety checks must be disabled.

@@ -275,6 +275,14 @@ def get_path_transport(
raise BadArgumentError(f"Could not find device by path: {path}")


def _use_external_script_type(client) -> bool:
if client.features.model == "1" and client.version > (1, 10, 5):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better and matches more how people think:

Suggested change
if client.features.model == "1" and client.version > (1, 10, 5):
if client.features.model == "1" and client.version >= (1, 11, 1):

def _use_external_script_type(client) -> bool:
if client.features.model == "1" and client.version > (1, 10, 5):
return True
if client.features.model == "T" and client.version > (2, 4, 3):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better and matches more how people think:

Suggested change
if client.features.model == "T" and client.version > (2, 4, 3):
if client.features.model == "T" and client.version > (2, 5, 1):

@elkimek
Copy link

elkimek commented Jun 1, 2022

@achow101 I think it's the other way around. No one uses external inputs because no one implemented it as an useful tool.

Sparrow Wallet recently implemented privacy enhancing features such as Stonewall 2x (two party CoinJoin) and Stowaway (PayJoin) which people actually use on daily basis. With Trezor's external inputs it would be possible to do that from hardware wallets which in my opinion would close the gap where users needs to choose between security and privacy tradeoffs.

See sparrowwallet/sparrow#368

@achow101
Copy link
Member

Is this still relevant?

@prusnak
Copy link
Collaborator

prusnak commented Jan 17, 2024

I think so. Can you please confirm @matejcik?

@matejcik
Copy link
Contributor Author

I will need to update the PR in light of #713 but otherwise yes, this is still desired.

@matejcik matejcik force-pushed the restore-trezor-external branch from 6054be5 to 1985b0f Compare July 1, 2024 13:35
@matejcik
Copy link
Contributor Author

matejcik commented Jul 1, 2024

PR updated and pointed on top of #742

@matejcik matejcik force-pushed the restore-trezor-external branch from 1985b0f to 0159365 Compare July 1, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants