From cc70aa77fcb5a7f5fa4f0a3314f880bd80c3e9ca Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 3 Aug 2026 21:29:12 -0300 Subject: [PATCH 1/2] test(taproot): assert the serialized transaction, not just the signature Every signing test read the `signature` protobuf field and either discarded `serialized_tx` or checked a substring of it. `signature` and `serialized_tx` are separate nanopb fields with independent presence flags, so a firmware path that populated one and not the other passed the whole suite -- which is exactly what shipped: the taproot branch omitted has_serialized_tx and the host silently lost the 66-byte witness and the 4-byte locktime footer. The substring check in test_send_p2tr_with_change could not have caught it either: the change scriptPubKey it looked for is serialized in phase 1, well before any witness, so it survives a truncated suffix. assertCompleteSegwitTx() parses the transaction per BIP-144 and requires it to consume exactly len(raw): a segwit marker promises witness data, so a dropped witness now runs the stream off the end instead of passing unnoticed. It returns the per-input witness stacks, letting the tests assert that a key-path spend carries exactly one 64-byte element and that a legacy input still serializes its empty 0x00 witness. Each test now also pins the full serialization. Those goldens were captured from a fixed-firmware emulator run and independently rederived from the inputs and the existing EXPECTED_* witnesses; both agree. --- tests/test_msg_signtx_taproot.py | 115 ++++++++++++++++++++++++++++++- 1 file changed, 113 insertions(+), 2 deletions(-) diff --git a/tests/test_msg_signtx_taproot.py b/tests/test_msg_signtx_taproot.py index 373d6a25..7dcc3cd5 100644 --- a/tests/test_msg_signtx_taproot.py +++ b/tests/test_msg_signtx_taproot.py @@ -59,8 +59,102 @@ ) +# Full BIP-144 serializations, captured from the emulator and cross-checked +# against an independent derivation from this file's own inputs and the +# EXPECTED_* witnesses above. These pin the bytes the host would broadcast -- +# `signature` alone was populated correctly even while the witness and the +# locktime footer were being dropped on the wire. +EXPECTED_SERIALIZED_TX = ( + "0100000000010137eea6e08b6227cd775f08153e291187d0df2a23261dab50752f98" + "113903326e0000000000ffffffff01905f0100000000001976a914759d6677091e97" + "3b9e9d99f19c68fbf43e3f05f988ac0140afe221b16d648a1ad7329f976593073238" + "0cc67765bd73af7ce13b59911468512d9ee77e34af56fe1f59f98372011f7cb400ce" + "d614d808c690c5ba907fb62de900000000" +) +EXPECTED_SERIALIZED_TX_CHANGE = ( + "0100000000010137eea6e08b6227cd775f08153e291187d0df2a23261dab50752f98" + "113903326e0000000000ffffffff0250c30000000000001976a914759d6677091e97" + "3b9e9d99f19c68fbf43e3f05f988ac409c000000000000225120882d74e5d0572d5a" + "816cef0041a96b6c1de832f6f9676d9605c44d5e9a97d3dc0140e3c44408fe61256a" + "d406733f100f1ee856eb31854335efa59e60a61ea5d41ab341802f0cccb55f644042" + "a1ab390f0a406b9d3efe3996d05442b4ee43d5355eab00000000" +) +EXPECTED_SERIALIZED_TX_MIXED = ( + "01000000000102a4a9ecee1384341b77c2db4d5cc54239854f0efc5f9978f3a2a878" + "2608df1f3e0000000000ffffffffa4a9ecee1384341b77c2db4d5cc54239854f0efc" + "5f9978f3a2a8782608df1f3e010000006a47304402205aa50469308c21e9e1ba0299" + "cd235add026914e4406bcfa6d9c0403c8cc3cf580220764a5832ad1bc36ba6a21020" + "a253c2272bca5aa1643d9c41b12c318b0a38824e012103aaeb52dd7494c361049de6" + "7cc680e83ebcbbbdbeb13637d92cd845f70308af5effffffff01e022020000000000" + "1976a914759d6677091e973b9e9d99f19c68fbf43e3f05f988ac0140b596e1bbefb8" + "55af9852942797075d4f452b2d186cb17a76226892334a497a62adb9a02f7c1b4573" + "e4d48b92e2307bb0b2282c97e2c5350bb3c21619fab855a20000000000" +) + + class TestMsgSigntxTaproot(KeepKeyTest): + def assertCompleteSegwitTx(self, raw, signatures, n_in, n_out): + """Parse the serialized tx strictly; it must consume exactly len(raw). + + `signature` and `serialized_tx` are separate nanopb fields on + TxRequestSerializedType, each with its own presence flag. Asserting + only `signature` passes even when the device never transmits the + witness stack -- the host then gets a tx that declares the segwit + marker/flag, carries no witness and no locktime, and every node + rejects it. A structural parse catches that: the marker promises + witnesses, so the stream ends early and the offset check fails. + + Returns the witness stacks, one list per input. + """ + pos = [0] + + def take(n): + if len(raw) < pos[0] + n: + raise AssertionError( + "tx truncated at offset %d: wanted %d more byte(s) of %d " + "total: %s" + % (pos[0], n, len(raw), hexlify(raw).decode())) + out = raw[pos[0]:pos[0] + n] + pos[0] += n + return out + + def varint(): + first = take(1)[0] + if first < 0xfd: + return first + width = {0xfd: 2, 0xfe: 4, 0xff: 8}[first] + return int.from_bytes(take(width), "little") + + take(4) # nVersion + marker = take(2) + if marker != unhexlify("0001"): + raise AssertionError( + "missing segwit marker/flag: got %s" % hexlify(marker).decode()) + if varint() != n_in: + raise AssertionError("unexpected input count") + for _ in range(n_in): + take(32); take(4); take(varint()); take(4) # outpoint, sig, seq + if varint() != n_out: + raise AssertionError("unexpected output count") + for _ in range(n_out): + take(8); take(varint()) # value, scriptPubKey + witnesses = [[take(varint()) for _ in range(varint())] + for _ in range(n_in)] + take(4) # nLockTime footer + if pos[0] != len(raw): + raise AssertionError( + "trailing bytes: parsed %d of %d" % (pos[0], len(raw))) + + # Every BIP-340 signature the device reported must actually appear in + # the witness data it serialized. + flat = [item for stack in witnesses for item in stack] + for sig in signatures: + if len(sig) == 64 and sig not in flat: + raise AssertionError( + "schnorr signature absent from serialized_tx witnesses") + return witnesses + def test_send_p2tr(self): """Spend a P2TR input and compare the witness byte for byte. @@ -115,11 +209,15 @@ def test_send_p2tr(self): request_index=0)), proto.TxRequest(request_type=proto_types.TXFINISHED), ]) - (signatures, _) = self.client.sign_tx( + (signatures, serialized) = self.client.sign_tx( "Bitcoin", [inp1], [out1]) self.assertEqual(len(signatures), 1) self.assertEqual(hexlify(signatures[0]).decode(), EXPECTED_WITNESS) + witnesses = self.assertCompleteSegwitTx(serialized, signatures, 1, 1) + # key-path spend: exactly one stack item, the bare 64-byte signature + self.assertEqual(witnesses[0], [signatures[0]]) + self.assertEqual(hexlify(serialized).decode(), EXPECTED_SERIALIZED_TX) def test_send_p2tr_with_change(self): """P2TR change is device-derived and omitted from recipient prompts.""" @@ -150,7 +248,14 @@ def test_send_p2tr_with_change(self): self.assertEqual(hexlify(signatures[0]).decode(), EXPECTED_CHANGE_WITNESS) + # EXPECTED_CHANGE_SCRIPT is a phase-1 output byte, which the device + # transmits regardless of whether the witness ever reaches the host. + # Assert the whole transaction, not just that prefix. self.assertIn(unhexlify(EXPECTED_CHANGE_SCRIPT), serialized) + witnesses = self.assertCompleteSegwitTx(serialized, signatures, 1, 2) + self.assertEqual(witnesses[0], [signatures[0]]) + self.assertEqual(hexlify(serialized).decode(), + EXPECTED_SERIALIZED_TX_CHANGE) def test_send_mixed_p2tr_and_legacy(self): """A P2TR signature commits to the legacy input's real prevout.""" @@ -178,13 +283,19 @@ def test_send_mixed_p2tr_and_legacy(self): script_type=proto_types.PAYTOADDRESS, ) - (signatures, _) = self.client.sign_tx( + (signatures, serialized) = self.client.sign_tx( "Bitcoin", [taproot, legacy], [recipient]) self.assertEqual(len(signatures), 2) self.assertEqual(hexlify(signatures[0]).decode(), EXPECTED_MIXED_WITNESS) self.assertTrue(signatures[1]) + witnesses = self.assertCompleteSegwitTx(serialized, signatures, 2, 1) + self.assertEqual(witnesses[0], [signatures[0]]) + # the legacy input must still serialize an EMPTY witness (0x00) + self.assertEqual(witnesses[1], []) + self.assertEqual(hexlify(serialized).decode(), + EXPECTED_SERIALIZED_TX_MIXED) def test_mixed_p2tr_requires_every_input_amount(self): """Fail closed instead of signing an incomplete BIP-341 commitment.""" From 1f2eecd227f73996b1d60af3af1b2972430527ad Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 3 Aug 2026 21:31:28 -0300 Subject: [PATCH 2/2] report(taproot): fail the report when required tests only skip validate_junit() accepted 'skip' as a waiver. That is right for build-flag-gated features (bitcoin-only, zcash-privacy), where a skip genuinely means "not in this build". It is wrong for a capability the build claims to have: every taproot test opens with requires_taproot(), so if that capability regressed, all six would skip and the report would still certify a green run -- coverage it never actually obtained. MUST_RUN_MODULES lists the modules that must really execute; a skip there is now a 'skipped-but-required' failure. Verified both ways against the catalogue: taproot passing validates clean, taproot skipping produces six failures (B21-B26) where it previously reported success. B21/B22/B23 prose now states what the tests prove after the serialized-tx coverage change -- that the full transaction is parsed as BIP-144 and must consume every byte, so the witness and locktime footer are known to have reached the host, not just the signature field. --- scripts/generate-test-report.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index a80289bc..bc5657fa 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -779,17 +779,23 @@ def _arg_shown(a): 'Sign a Taproot key-path spend', 'Spends a BIP-86 P2TR input using BIP-341 SIGHASH_DEFAULT and a BIP-340 Schnorr ' 'signature. The 64-byte witness is compared byte-for-byte with an independently ' - 'computed reference value.', + 'computed reference value. The complete 153-byte transaction is then parsed as ' + 'BIP-144 and must consume every byte, proving the witness stack and the 4-byte ' + 'locktime footer actually reached the host rather than only the signature field.', ['P2TR recipient confirmation', 'Fee confirmation']), ('B22', 'test_msg_signtx_taproot', 'test_send_p2tr_with_change', 'Sign P2TR with device-derived change', 'Derives m/86\'/0\'/0\'/1/0 on-device, emits a P2TR change output, and verifies ' - 'the Schnorr witness against an independent BIP-340/341 reference.', + 'the Schnorr witness against an independent BIP-340/341 reference. The complete ' + '196-byte transaction is parsed as BIP-144 and must consume every byte, and the ' + 'change output is matched as a full value/length/script triple.', ['P2TR recipient confirmation', 'Fee confirmation']), ('B23', 'test_msg_signtx_taproot', 'test_send_mixed_p2tr_and_legacy', 'Sign mixed Taproot and legacy inputs', 'Commits the P2TR signature to both inputs, including the legacy prevout amount and ' - 'scriptPubKey, while independently verifying the resulting Schnorr witness.', + 'scriptPubKey, while independently verifying the resulting Schnorr witness. The ' + 'complete 301-byte transaction is parsed as BIP-144; the Taproot input must carry ' + 'a single 64-byte stack item and the legacy input its empty 0x00 witness.', []), ('B24', 'test_msg_signtx_taproot', 'test_mixed_p2tr_requires_every_input_amount', @@ -2199,13 +2205,29 @@ def screenshot_filter(fw_version): return ' or '.join(terms) +# Modules whose tests must actually RUN once the firmware is new enough to be +# catalogued for them -- a skip is a failure, not a waiver. +# +# The general rule below treats 'skip' as a design waiver, which is right for +# build-flag-gated features (bitcoin-only, zcash-privacy). It is wrong for a +# capability the build claims to have: every taproot test opens with +# requires_taproot(), so if that capability regressed, all six would skip and +# the report would still read green -- the report would be certifying coverage +# it never obtained. Listing a module here converts that silence into a failure. +MUST_RUN_MODULES = { + 'test_msg_signtx_taproot', + 'test_msg_getaddress_taproot', +} + + def validate_junit(fw_version, results): """Check SECTIONS tests against JUnit results. Returns (passed, failed_list). A test is considered failed if it appears in SECTIONS for this firmware version and the JUnit result is 'fail' or 'error' (not 'skip' or 'pass'). Tests with no JUnit entry are treated as missing (also a failure). - Tests that were skipped (gated by requires_message/requires_firmware) are OK. + Tests that were skipped (gated by requires_message/requires_firmware) are OK, + unless their module is in MUST_RUN_MODULES. """ active = [(l,t,mf,bg,fl,tests) for l,t,mf,bg,fl,tests in SECTIONS if ver_ge(fw_version, mf)] failures = [] @@ -2214,6 +2236,8 @@ def validate_junit(fw_version, results): status = _lookup(results, mod, meth) if status in ('fail', 'error'): failures.append((tid, mod, meth, status)) + elif status == 'skip' and mod in MUST_RUN_MODULES: + failures.append((tid, mod, meth, 'skipped-but-required')) elif not status: failures.append((tid, mod, meth, 'missing')) return (len(failures) == 0, failures)