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

tests, trezor: Assert scriptPubKey failure for Trezor in test_big_tx #580

Closed
wants to merge 1 commit into from

Conversation

grdddj
Copy link

@grdddj grdddj commented Feb 23, 2022

Soon there will be a security tightening in Trezor making sure that device will refuse to sign a transaction trying to spend inputs that do not belong to the wallet (the scriptPubKey of the input does not correspond to private keys in the wallet).

Connected PR, that will (most probably) merge these changes into master - trezor/trezor-firmware#2081.

Making appropriate changes in test_big_tx testcase, that is generating inputs that are not spendable by Trezor's private keys. (Seems that could be the same case as with coldcard?)

To still test Trezor's capability of signing big transaction, we might create inputs corresponding to slip-14 standard, which the wallet (inputted with all all all... seed) will be able to sign. That is up to discussion (we could supply the test vectors from our tests).

Trezor is rejecting to sign this transaction as its inputs do not
correspond to the wallet's private keys
@achow101
Copy link
Member

That would actually affect test_sign_tx where we do external inputs (and not with the expected way to be doing them).

@grdddj
Copy link
Author

grdddj commented Feb 23, 2022

That would actually affect test_sign_tx where we do external inputs (and not with the expected way to be doing them).

It really seems to be affecting them, some cases from test_sign_tx are currently failing in our CI, where we do run the HWI tests - https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/2123051935

There are some errors like

self.assertTrue(first_sign_res["signed"]) ... KeyError: 'signed'

which are not explicitly mentioning the scriptPubKey issue, but are probably caused by it.

I am not sure what is the best resolution, we might discuss it with @matejcik and @mmilata

@achow101
Copy link
Member

There are some errors like

self.assertTrue(first_sign_res["signed"]) ... KeyError: 'signed'

which are not explicitly mentioning the scriptPubKey issue, but are probably caused by it.

I am not sure what is the best resolution, we might discuss it with @matejcik and @mmilata

I think the only solution is to disable external inputs for trezors entirely. The supported method for external inputs requires proofs which we do not currently support, and I am not sure of the correct way to implement those here.

@matejcik
Copy link
Contributor

matejcik commented Feb 23, 2022

Trezor will also accept all EXTERNAL inputs that already have a valid signature. So you can sign anything if Trezor is the last signer. (in case of two Trezor signers, you'd indeed have to exchange ownership proofs)

@matejcik
Copy link
Contributor

I can look into using ownership proofs locally in the tests, that might not be too difficult. This is of course leaving aside a way to exchange them, but I suppose we could just stick a vendor field into the PSBT

@achow101
Copy link
Member

achow101 commented Mar 2, 2022

Superceded by #581

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

Successfully merging this pull request may close these issues.

3 participants