feat(taproot): spend from P2TR inputs (BIP-341 key path) - #330
Merged
Conversation
First of three steps toward Taproot support. Adds nothing to the signing
path yet -- this lands the primitive and proves it against the spec.
- pin deps/crypto/trezor-firmware at the BIP-340 implementation
- compile bip340.c into trezorcrypto
- unittests/crypto/bip340.cpp: all 19 official BIP-340 vectors
Byte-exact signatures for the 8 vectors with secret keys, correct
rejection of all 10 must-fail cases (pubkey off-curve, pubkey >= field
size, has_even_y(R) false, sG - eP infinite with x(inf) as both 0 and 1,
sig[0:32] = field size, sig[32:64] = curve order), plus out-of-range
private keys and a check that the output buffer is zeroed on failure.
ROM cost measured with arm-none-eabi-gcc -Os -mcpu=cortex-m3 -mthumb:
1278 bytes of .text, no data, no bss.
Taproot is already further along in this tree than it looks: coins.def
carries taproot=true for Bitcoin and Testnet, segwit_addr.c selects
bech32m for witness versions above 0, and PAYTOTAPROOT outputs already
build and size correctly. What is missing is the input side, which the
next two steps cover:
2. BIP-86 output key tweak + SPENDTAPROOT in compute_address, which
removes the `return 0` at transaction.c:188 and unblocks GetAddress
for bc1p
3. BIP-341 sighash + the SPENDTAPROOT signing path + confirm UX
Depends on keepkey/trezor-firmware#5; the submodule pin points at that
branch and needs re-pointing at its merge commit before this lands.
The buffer-zeroing assertion in SignRejectsOutOfRangeKeys passed whether or
not bip340_sign() cleared the output, because sig started zero-initialised.
Pre-fill with 0xFF so it proves something.
Adds two cases:
- XOnlyPubkeyZeroesOnFailure, covering the matching contract now that
bip340_get_xonly_pubkey() zeroes on failure too
- ZeroSTakesTheSpecPath, pinning the ABSENCE of an s == 0 guard. s == 0
is in range per BIP-340 and must reject on the x-coordinate comparison
after computing R = -eP, not bail out early.
Bumps the crypto pin to pick up the guard removal and the restored Bitcoin
ABC copyright notice.
8/8 green.
Second of three steps. Removes the `return 0` at transaction.c:188 and
makes GetAddress return bc1p addresses for m/86' paths.
- compute_address() handles SPENDTAPROOT: tweak the x-only internal key
per BIP-86, then bech32m encode it at witness version 1
- path_mismatched() gains an m/86' branch, in BOTH copies (fsm_msg_coin.h
and coins.c, per the keep-in-sync note above them)
- three official BIP-86 vectors, driven from the published internal keys
and again end to end from the mnemonic
Two guards worth calling out, because both would have failed silently:
- taproot multisig is rejected up front. Without it the request fell
through to the p2sh branch and returned a p2sh address for a taproot
ask -- a wrong address, not an error.
- exactly 32 bytes are passed to segwit_addr_encode(), which only
length-checks the witness program for version 0 (segwit_addr.c:182).
Any other length would have encoded into a plausible-looking bc1p.
Also fixes a pre-existing bug in the PAYTOTAPROOT output gate. It tested
`!coin->has_taproot`, but has_taproot is the nanopb presence flag and every
coin in coins.def sets it -- only the `taproot` VALUE distinguishes them.
Bitcoin and Testnet have taproot=true; the other 41 coins have taproot=false
and were all passing the gate, building p2tr outputs for chains that cannot
spend them. Now tests the value.
Device build clean (MAKE_EXIT=0, no warnings, all variants). Retained cost
in firmware.keepkey.elf is 306 bytes; bip340_sign and bip340_verify still
garbage-collect out until step 3 references them.
11/11 unit tests green. Depends on keepkey/trezor-firmware#6.
Bumps deps/python-keepkey by one commit to pick up tests/test_msg_getaddress_taproot.py (BitHighlander/python-keepkey#28), which drives the emulator through the full SPENDTAPROOT GetAddress path and asserts the three official BIP-86 addresses. Verified against a locally built kkemu: 1 passed, and mutation checked so the assertions are known not to be vacuous. The test gates on firmware 7.16.0 and CMakeLists is 7.15.0, so it SKIPS until the project version bumps. Deliberate: gating at 7.15.0 would make released 7.15.0-rcN firmware without taproot fail rather than skip.
Third and last step: SPENDTAPROOT inputs are now signed.
- signing.c accumulates sha_amounts and sha_scriptpubkeys over every input.
BIP-341 commits to the amount and scriptPubKey of ALL inputs, not just
the taproot ones, which BIP-143 never required.
- signing_hash_bip341() delegates the SigMsg assembly to bip341_sighash()
in the crypto lib, so the field ordering is unit-testable against the
published vectors instead of only reviewable.
- the witness is a single 64-byte element. SIGHASH_DEFAULT omits the
trailing sighash byte; appending 0x00 would be a different signature and
would fail verification.
- signing keys go through bip340_tweak_seckey(), so the signature verifies
against the output key in the scriptPubKey rather than the internal key.
- transaction.c gains address_to_script_pubkey() and
fill_input_script_pubkey() to derive each input's scriptPubKey.
- taproot inputs are rejected on coins with taproot=false, matching the
output-side gate.
SRAM: +448 B bss, which is ABOVE the 256 B single-commit threshold that
tools/sram-budgets.json says needs explicit review -- flagging rather than
sneaking it through. It was +1072 B until the two new accumulators were
changed from Hasher to SHA256_CTX: BIP-341 fixes them to plain SHA256, while
a Hasher carries a union sized by GROESTL512_CTX and cost ~1.2 KB for no
benefit. The remainder is two SHA256 contexts, two 32-byte digests, the
tweaked key and one HDNode.
ROM: +2928 B text, 1900 B of it bip340/bip341. Nothing garbage-collects out
now that the signing path references it.
Device build clean, no warnings, all variants. Crypto validated against
BIP-341's published transaction: tweaked privkey, sigHash and the 64-byte
witness all match byte for byte.
NOT yet verified: signing on the emulator, and no OLED proof.
Emulator cross-check against an independent BIP-340/341 implementation
caught the device signing a valid signature over the wrong commitment.
BIP-143 hashes prevouts, sequences and outputs with DOUBLE sha256 --
curve->hasher_sign is HASHER_SHA2D for Bitcoin -- while BIP-341 specifies
SINGLE sha256. Reusing hash_prevouts/hash_sequence/hash_outputs was
therefore wrong in a way nothing self-consistent could detect: the
signature verified fine against the sighash the device computed, and that
sighash committed to a transaction nobody had authorised. Trezor's
reference keeps hash_prevouts143 next to hash_prevouts for this reason.
Adds the parallel single-sha256 set and points the sighash at it.
Also wires taproot through three classifiers it was missing, each of which
failed closed rather than silently:
- is_internal_input_script_type: a taproot input may carry address_n
- is_change_output_script_type: taproot CHANGE was being rejected
- the phase-1 dispatch, which routed only SPENDWITNESS/SPENDP2SHWITNESS
down the segwit path
and extracts prepare_input_node() from compile_input_script_sig(), so a
taproot input gets the same re-validation and derivation without building a
scriptSig it does not have. Skipping that would also have skipped the
guard that the host has not swapped address_n between phases.
Emulator: witness matches the independent implementation byte for byte.
Both device variants build clean, no warnings.
SRAM +856 B bss over the step-2 baseline, above the 256 B single-commit
threshold in tools/sram-budgets.json and flagged accordingly: five SHA256
contexts, five digests, the tweaked key and one HDNode. ROM +3216 B text.
Lets a host ask the device whether it can derive and spend P2TR instead of inferring it from a firmware version. Version inference breaks the moment the feature is retargeted to a different release, and forces every client to carry a version table. Also unblocks the host side: Pioneer's taproot flag currently has no way to ask whether the connected firmware can verify a bc1p on screen or spend from one, so it has to guess. Bumps device-protocol for Features.supports_taproot (field 27; 19 and 20 are gaps with no reserved markers and are not safe to reuse against historical wire data), and python-keepkey for the regenerated bindings plus the tests that now gate on the capability rather than a version.
Deterministic signing is a choice a reviewer will question. It is spec-permitted, matches this firmware's RFC6979 ECDSA, and the nonce still depends on key and message so it is never reused across transactions. Fresh randomness would only add side-channel hardening, at the cost of making signatures unreproducible and therefore untestable against a published vector.
Gate-3 OLED capture found the address verification screen silently truncating any bech32 address longer than one line. displayed bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20 actual bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr 20 characters dropped, with no indication anything was missing -- and the QR beside it encodes the full address, so the two disagreed. A user "verifying" a receive address was checking two thirds of it. NOT a taproot bug. The threshold is ~42 characters at body font, so it already affects native segwit MULTISIG (p2wsh, 62 chars) on shipping firmware; p2wpkh is 42 and fits exactly, which is why it went unnoticed. Confirmed on the emulator for both p2wsh and p2tr. Cause: the address is drawn at TOP_MARGIN_FOR_ONE_LINE + font_height + ADDRESS_TOP_MARGIN = y=46 with line height 14 on a 64px canvas, so a second line starts at 60 and draw_char_with_shift() refuses to draw it -- it checks img->height + p->y <= canvas->height and returns false, and draw_string() then stops without reporting anything. Fix, in order of preference so the smallest lever is used first: - close the inter-line padding for multi-line addresses (14 -> 10) - only if that is still short, raise the block by exactly the overflow Raising alone was tried first and is wrong: the QR is drawn after the text and overwrites the start of a raised first line, which cost the first six characters. Closing the padding alone is also not enough -- the font is 10px, so two lines from y=46 need 66. Together they land the second line at y=54..64, clear of the QR at ~41. Single-line addresses are untouched: the whole branch is gated on calc_str_line() > ONE_LINE. Verified on the emulator: both 62-char addresses now render in full and legibly, the p2pkh confirm screen is unchanged, and the existing test_msg_getaddress_segwit suite still passes. Both device variants build clean with no warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Candidate scope
This PR targets the BitHighlander fork's
developbranch and carries the complete Bitcoin Taproot key-path stack:bc1paddress derivation and trusted-display QR confirmationSIGHASH_DEFAULTFeatures.supports_taprootcapability discoveryScript-path Taproot and Taproot multisig are intentionally out of scope and remain rejected.
Canonical upstream dependencies
The firmware pins exact upstream merge commits, not fork-only feature tips:
keepkey/trezor-firmware@9c882485e6bed1dae7538ec4d43a4f9a12c428aa— feat(crypto): BIP-341 seckey tweak and key-path sighash keepkey/trezor-firmware#7keepkey/device-protocol@674777f6d4dd16e2b8c4c2df10608976375ee879— feat(features): advertise Taproot signing support keepkey/device-protocol#117keepkey/python-keepkey@9c58e677135e24e59b5d867348073d47dae61785— test(bitcoin): prove Taproot production paths in release PDF keepkey/python-keepkey#204 and feat: Solana support (GetAddress, SignTx, SignMessage) #206Nothing was published to npm or PyPI. The two Solana
token_recipient_ownerbounds inmessages-solana.optionsare codegen compatibility for fields already present in the canonical device-protocol merge; they are not an x402 release claim in this PR.Consensus and signing correctness
SIGHASH_DEFAULTdoes not append a sighash byte.The byte-exact P2TR signatures are compared with an implementation that shares no signing code with firmware. That cross-check previously caught an incorrect SHA256d BIP-341 commitment, which is now fixed.
Evidence in the canonical PDF suite
The pinned python-keepkey report catalog names each required Taproot claim:
CI must generate the report from the exact PR head and fail if pytest, the report catalog, screenshots, or PDF generation are incomplete. The GitHub Actions run for the current head is the canonical Docker/Compose, emulator, ARM, unit, sanitizer, and PDF proof; CircleCI is not a release gate.
Resource review
The Taproot signing state adds approximately 856 bytes of static SRAM over the address-only step, above the repository's 256-byte explicit-review threshold. This is intentionally called out for human review. The final ARM jobs must be green for both full and bitcoin-only variants before testing on hardware.
Release gates
750e692e8c6ada172f64cb58e3636a69bb6af0d6sha256:068150f9c92e93163a0029e9b933581f9f06bc3a359c51c98d52346cb7646133): Bitcoin 33/33, Taproot B20-B26 pass, and the complete BIP-86 address is visible across two OLED linesDo not merge to fork
developor cut an RC until all four gates are complete.