Taproot in Trezor T #1891

andrewkozlik · 2021-11-03T17:51:28Z

This PR implements the main Taproot functionality (#1656) in trezor-core, and fixes some bugs that I stubled on along the way:

Implements BIP-340 key tweaking functions.
Implements GetAddress for Taproot.
Implements the BIP-341 common signature message computation and Taproot signing.
Implements support for replacement transactions with Taproot inputs.
Implements support for pre-signed external Taproot inputs.
Disables prevtx streaming if all internal inputs are Taproot.
Renames Hash143 to SigHasher and replaces preimage_hash() with two functions hash143() and hash341().
Adds warning dialog in SignMessage if a non-standard path is used, e.g. GreenAddress or Casa. This is not critical, but seems reasonable to do simply for consistency with how other messages are treated. Once we implement Taproot message signing this will probably become more important, see below.
Fixes an off-by-one bug in OP_PUSH writing.
Fixes a number of bugs in the TxWeightCalculator and adds unit tests which would have caught them. I also added support for external inputs in TxWeightCalculator. After these fixes the CoinJoin approver needs to account for the weight of the coordinator's output when considering the fair distribution of the fees, which also required a fix.

This PR does not implement any Taproot functionality in trezor-legacy. It also does not implement any of the following in trezor-core:

Generating ownership proofs for Taproot addresses.
Support for external Taproot inputs with an ownership proof.
Message signing and verification for Taproot.
Multisig support for Taproot.

One item that is open for discussion is how to behave when a Taproot path is used with ECDSA or when a non-Taproot path is used with Schnorr signatures. The current implementation will only show an "unknown path" warning dialog in these cases. Using the same key in two different algorithms is generally not recommended, at least not without deeper analysis. As far as we known there is no known attack when using the same private key in ECDSA and Schnorr [1], [2], [3]. We could be more strict and allow these scenarios only if safety checks are disabled. On the other hand I am not sure yet how Taproot message signing works in bitcoin-core. If it allows using ECDSA, then being strict in Trezor might pose a problem.

@onvej-sl please review the changes in the C code (trezor-crypto and the trezorcrypto.bip340 module) and review the rest of the code from a security perspective.
@matejcik please review the Python code overall (design, security, etc.).

[1] https://bitcoin.stackexchange.com/a/107928
[2] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-February/018384.html
[3] https://crypto.stackexchange.com/questions/54660/is-it-safe-to-use-same-private-key-in-two-or-more-ec-signature-algorithms

core/SConscript.unix

crypto/zkp_bip340.c

tests/device_tests/test_msg_getaddress_segwit_native.py

crypto/zkp_bip340.c

onvej-sl · 2021-11-09T15:50:19Z

Two high level notes:

This PR doesn't implement ownership proofs for taproot utxos. We will need them for coinjoin.
I was under the impression that we don't have to stream all previous transactions provided there is at least one taproot input (see https://satoshilabs.slack.com/archives/CL1D61PQF/p1624287272003700) which is more general than it is done in 113465e.

andrewkozlik · 2021-11-10T10:52:26Z

I was under the impression that we don't have to stream all previous transactions provided there is at least one taproot input [...]

In 21f0bf8 I expanded the corresponding code comment. I also switched the two branches of the if for better legibility, but the code is the same.

matejcik

👍

i have a bunch of style comments and a few questions I'd like clarified

plus the thing about signmessage path warnings

core/src/apps/bitcoin/sign_tx/tx_info.py

core/src/apps/bitcoin/get_ownership_id.py

core/src/apps/bitcoin/common.py

core/src/apps/bitcoin/scripts.py

core/src/apps/bitcoin/sign_message.py

core/tests/test_apps.bitcoin.segwit.bip341.p2tr.py

tests/device_tests/test_msg_getaddress_segwit_native.py

tests/device_tests/test_msg_signtx_taproot.py

core/src/apps/bitcoin/sign_tx/bitcoin.py

onvej-sl · 2021-11-10T13:53:03Z

In 21f0bf8 I expanded the corresponding code comment. I also switched the two branches of the if for better legibility, but the code is the same.

If I understand it correctly, it turned out that the conjecture written on Slack isn't true because of external inputs and replacement transactions.

andrewkozlik · 2021-11-10T16:58:12Z

If I understand it correctly, it turned out that the conjecture written on Slack isn't true because of external inputs and replacement transactions.

Not only that. Although the attack that was disclosed in June 2020 would not be feasible in it's original form, it would be feasible if in addition the attacker provided invalid information about the script types.

matejcik

LGTM, modulo sign_message which will be resolved by rebasing

…ation.

- Refactor TxWeightCalculator to count inputs and outputs itself. - Fix witness data weight by adding the weight of the witness stack item count for each input in segwit transactions and removing the weight of the nonsensical extra inputs count. - Get multisig pubkey count from multisig.nodes or multisig.pubkeys like in multisig_get_pubkeys(). - Fix size of multisig script length encoding in segwit (varint vs. OP_PUSH). - Improve comments.

After fixing the TxWeightCalculator the approver needs to account for the weight of the coordinator's output.

…() with two functions.

andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. bitcoin Bitcoin related HIGH labels Nov 3, 2021

andrewkozlik requested review from matejcik and onvej-sl November 3, 2021 17:51

andrewkozlik requested a review from prusnak as a code owner November 3, 2021 17:51

prusnak reviewed Nov 3, 2021

View reviewed changes

core/SConscript.unix Outdated Show resolved Hide resolved

crypto/zkp_bip340.c Show resolved Hide resolved

tests/device_tests/test_msg_getaddress_segwit_native.py Show resolved Hide resolved

matejcik added this to To be reviewed in Pull Requests via automation Nov 4, 2021

Pull Requests automation moved this from To be reviewed to Finalizing Nov 4, 2021

prusnak approved these changes Nov 4, 2021

View reviewed changes

prusnak mentioned this pull request Nov 4, 2021

Path warning displayed when the same transaction fails later due to strict checks #1895

Closed

matejcik moved this from Finalizing to In progress in Pull Requests Nov 4, 2021

onvej-sl reviewed Nov 5, 2021

View reviewed changes

crypto/zkp_bip340.c Show resolved Hide resolved

prusnak approved these changes Nov 10, 2021

View reviewed changes

Pull Requests automation moved this from In progress to Finalizing Nov 10, 2021

matejcik requested changes Nov 10, 2021

View reviewed changes

Pull Requests automation moved this from Finalizing to In progress Nov 10, 2021

matejcik approved these changes Nov 12, 2021

View reviewed changes

Pull Requests automation moved this from In progress to Finalizing Nov 12, 2021

onvej-sl approved these changes Nov 12, 2021

View reviewed changes

andrewkozlik and others added 6 commits November 12, 2021 18:23

feat(crypto): Add key tweaking functions to zkp_bip340.c.

50350de

test(crypto): Add unit test for BIP340 tweak.

797d761

feat(core): Add key tweaking functions to trezorcrypto.bip340.

25cbabf

fix(core): Fix bip340 module build.

b793cee

feat(core): introduce PATTERN_BIP86 to keychain

514c96f

feat(core): Implement GetAddress for taproot.

f2d2d50

andrewkozlik added 21 commits November 12, 2021 18:23

feat(core): Disable prevtx streaming for Taproot.

37c12f9

chore(core): Disable ownership proof generation for Taproot.

99b3edc

chore(core): Disable Taproot for altcoins.

50b9fa8

fix(tests): Add script_pubkey field to external inputs.

df5f693

fix(tests): Fix UI tests which fail earlier due to scriptPubKey deriv…

5aea5d3

…ation.

feat(tests): Add tests for spending Taproot UTXOs.

4830c42

feat(tests): Add fee bump device test for Taproot.

383a362

feat(tests): Add device test for external presigned Taproot input.

cdfd859

fix(core): Fix OP_PUSH writing.

a4db38f

test(core): Add multisig unit tests for TxWeightCalculator.

437a14d

feat(core): Support Taproot in TxWeightCalculator.

7145eea

feat(core): Support external inputs in TxWeightCalculator.

5215d65

fix(core): Fix CoinJoin approval check.

927bdcb

After fixing the TxWeightCalculator the approver needs to account for the weight of the coordinator's output.

feat(tests): Test fake Taproot transaction attack.

a1d80dc

feat(core): Show path warning for SignMessage.

610f977

test(core): Add unit tests for BIP-341 signature hash.

cb8deb8

test(crypto): Add BIP-341 test vectors.

dd0bc45

refactor(core): Rename Hash143 to SigHasher and replace preimage_hash…

0630f78

…() with two functions.

chore(core): Remove USE_SECP256K1_ZKP_BIP340 build option.

9e4dd12

chore(core): Convert SIGHASH_* consts to SigHashType enum.

44ea5e4

andrewkozlik force-pushed the andrewkozlik/taproot-core branch from 3fe330e to 44ea5e4 Compare November 12, 2021 17:40

andrewkozlik merged commit 221977a into master Nov 13, 2021

Pull Requests automation moved this from Finalizing to Merged Nov 13, 2021

andrewkozlik deleted the andrewkozlik/taproot-core branch November 13, 2021 12:33

prusnak added this to DONE in Taproot 🥕 Nov 14, 2021

This was referenced Nov 15, 2021

Taproot in Trezor 1 #1917

Merged

SignMessage updates trezor/connect#941

Closed

achow101 mentioned this pull request Nov 17, 2021

Taproot support bitcoin-core/HWI#501

Closed

hynek-jina added this to the Taproot 🥕 milestone Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taproot in Trezor T #1891

Taproot in Trezor T #1891

andrewkozlik commented Nov 3, 2021

onvej-sl commented Nov 9, 2021

andrewkozlik commented Nov 10, 2021

matejcik left a comment

onvej-sl commented Nov 10, 2021

andrewkozlik commented Nov 10, 2021

matejcik left a comment

Taproot in Trezor T #1891

Taproot in Trezor T #1891

Conversation

andrewkozlik commented Nov 3, 2021

onvej-sl commented Nov 9, 2021

andrewkozlik commented Nov 10, 2021

matejcik left a comment

Choose a reason for hiding this comment

onvej-sl commented Nov 10, 2021

andrewkozlik commented Nov 10, 2021

matejcik left a comment

Choose a reason for hiding this comment