feat(taproot): BIP-340 signer, build wiring and official test vectors - #327
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.
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
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.
First of three steps toward Taproot. Adds nothing to the signing path — this lands the crypto primitive and proves it against the spec, so the risky part is reviewable on its own and the ROM cost is known before committing to the rest.
Taproot is further along in this tree than the epic doc assumed
developtaprootcolumn incoins.deftruefor Bitcoin and Testnetsegwit_addr.c:184selects it for witness version > 0PAYTOTAPROOT)transaction.c:410, size accounted at:986SPENDTAPROOT=5,PAYTOTAPROOT=6,taproot=28)device-protocoltransaction.c:188— explicitreturn 0signing.cscript_type checks never mention itcrypto/schnorr.cis Bitcoin ABC's BCH variant, and is not even in the buildSo this is four things, not a greenfield.
This PR
deps/crypto/trezor-firmwareat the BIP-340 implementation (feat(crypto): BIP-340 Schnorr signatures for Taproot keepkey/trezor-firmware#5)bip340.cintotrezorcryptounittests/crypto/bip340.cpp— all 19 official BIP-340 vectorsWhy hand-rolled instead of vendoring secp256k1-zkp
Trezor implements BIP-340 via vendored
secp256k1-zkp+zkp_bip340.c— 20–40KB. The 7.15 line is 4–10KB from the ROM wall, so it does not fit. Building on thebn_*andscalar_multiplyprimitives already in the tree costs, measured witharm-none-eabi-gcc -Os -mcpu=cortex-m3 -mthumb:Cost in shipped images from this PR is 0.
cmake/caches/device.cmakebuilds with-ffunction-sections -fdata-sections -Wl,--gc-sections, and nothing references bip340 yet, so it garbage-collects out of every image. The 1274 bytes land in step 3, when the signing path references it — the ROM wall is not in play here.The primitives are the ones already signing ECDSA with the same secrets, so this composes trusted code rather than introducing new field arithmetic. See the crypto PR for the structural diff against
schnorr.c(it is smaller than the file it is derived from — the Jacobi symbol helper goes away).Testing
unittests/crypto/bip340.cpp, 8/8 green locally against the real gtest harness:aux_rand, so this is an equality check, not a round-triphas_even_y(R)false, negated message, negated s,sG - ePinfinite withx(inf)as both 0 and 1,sig[0:32]not an x-coordinate,sig[0:32]= field size,sig[32:64]= curve order0andn) rejected, with the output buffer pre-filled0xFFso the zero-on-failure assertion proves somethingZeroSTakesTheSpecPathpins the absence of ans == 0guard:s == 0is in range per BIP-340 and must reject on the x-coordinate comparison after computingR = -eP, not bail out earlyVectors are copied verbatim from
bitcoin/bipsand were diffed against the CSV mechanically rather than by eye — which caught a dropped nibble and a short message literal.Next
SPENDTAPROOTincompute_address— removes thereturn 0attransaction.c:188, unblocks GetAddress forbc1p. Notesegwit_addr_encode()only length-checks the witness program for v0 (segwit_addr.c:182), so a wrong-length taproot key would encode into a plausible-lookingbc1p— pass exactly 32 bytes.SPENDTAPROOTsigning path + confirm UX — this is the one that needs Gate-3 OLED proof and an emulator smoke. Budget for latency: BIP-340 signing is 4 EC scalar multiplications because the self-verify roughly doubles it.Merge order
Depends on keepkey/trezor-firmware#5. That merges first, then the submodule pin here gets re-pointed at its merge commit. As it stands the pin points at the branch head (reachable from
origin/feature/bip340, so CI can fetch it).