test(7.15): consolidated Python harness, bindings, and device coverage - #197
test(7.15): consolidated Python harness, bindings, and device coverage#197BitHighlander wants to merge 66 commits into
Conversation
The previous pin carried a storage-version gate that asserted STORAGE_VERSION == 20 and the literal "case StorageVersion_18:". Both are true on the 7.16 passkeys branch and both are FALSE here, where STORAGE_VERSION is 17 and nothing is burned -- so develop was pinning a test suite guaranteed to fail against its own firmware. 006142da70e4 derives the ladder, the burned set and LAST_SHIPPED from the tree under test instead. Verified green on both lines from one file: 10 passed / 5 skipped here, 15 passed on the 7.16 tree. It also carries the integration-CI repair, which matters for a release branch: that job had been ending "cancelled" at exactly 30 minutes with zero assertions run, behind a check that reported success. It now finishes in 2m46s with 636 passed, 32 skipped, 0 failed, against an emulator built from current firmware rather than a five-month-old published image. Still the head of the open upstream PR keepkey/python-keepkey#197, which was fast-forwarded to this commit first, so the pin stays resolvable for an upstream reviewer.
The previous pin carried a storage-version gate that asserted STORAGE_VERSION == 20 and the literal "case StorageVersion_18:". Both are true on the 7.16 passkeys branch and both are FALSE here, where STORAGE_VERSION is 17 and nothing is burned -- so develop was pinning a test suite guaranteed to fail against its own firmware. 006142da70e4 derives the ladder, the burned set and LAST_SHIPPED from the tree under test instead. Verified green on both lines from one file: 10 passed / 5 skipped here, 15 passed on the 7.16 tree. It also carries the integration-CI repair, which matters for a release branch: that job had been ending "cancelled" at exactly 30 minutes with zero assertions run, behind a check that reported success. It now finishes in 2m46s with 636 passed, 32 skipped, 0 failed, against an emulator built from current firmware rather than a five-month-old published image. Still the head of the open upstream PR keepkey/python-keepkey#197, which was fast-forwarded to this commit first, so the pin stays resolvable for an upstream reviewer.
The previous pin carried a storage-version gate that asserted STORAGE_VERSION == 20 and the literal "case StorageVersion_18:". Both are true on the 7.16 passkeys branch and both are FALSE here, where STORAGE_VERSION is 17 and nothing is burned -- so develop was pinning a test suite guaranteed to fail against its own firmware. 006142da70e4 derives the ladder, the burned set and LAST_SHIPPED from the tree under test instead. Verified green on both lines from one file: 10 passed / 5 skipped here, 15 passed on the 7.16 tree. It also carries the integration-CI repair, which matters for a release branch: that job had been ending "cancelled" at exactly 30 minutes with zero assertions run, behind a check that reported success. It now finishes in 2m46s with 636 passed, 32 skipped, 0 failed, against an emulator built from current firmware rather than a five-month-old published image. Still the head of the open upstream PR keepkey/python-keepkey#197, which was fast-forwarded to this commit first, so the pin stays resolvable for an upstream reviewer.
Review guideScope: the 7.15 Python harness, protobuf bindings and device coverage. Like device-protocol, this is a dependency merge — it ships nothing to a device, it makes firmware submodule pins resolvable.
|
#216 landed as one squashed commit whose content had already reached #197 by another route, so most of the 31 conflicts are the two branches doing the same thing in different words. Resolved by hand, hunk by hunk; no --ours/--theirs. Every #216 addition is verified present in the merged tree, including the files that never conflicted (test_msg_display_disclosure.py, the eip155 vectors, the ripple/ping/tron/eth-signtx disclosure tests, and the `chain_id is not None` fix in keepkeylib/client.py -- that one matters, it is what lets an explicit chain_id=0 reach the wire to be refused). tests/test_sign_typed_data.py (1 hunk) -- #197 Same policy call on both sides; the only difference is `with open(...)` vs open/close and one extra clause of comment. Nothing asserted differs. tests/test_msg_thorchain_signtx.py (2 hunks) -- #197, and this one is a real divergence, not a restatement. #197 repointed test_sign_eth_add_liquidity at the firmware-pinned THORChain router (to=0xd37bbe57...); that line auto-merged, so it IS the transaction the merged test signs. #216 regenerated exact r/s against the OLD to=0x41e55600.... `to` is an RLP field of the legacy sighash, so #216's vectors do not describe the transaction under test and would fail. #197's structural assertions win on those grounds -- NOT because structural is preferable. They are genuinely weaker than what #216 had, and the superseded vectors are recorded in a comment at the assertion so the gap is visible and regenerable on-device. Same treatment for the memo-length hunk: both sides independently corrected 0x3b -> 0x3a (the memo is 58 bytes; the 59th was ABI padding), so the comments were combined rather than one dropped. tests/test_msg_mayachain_signtx.py (2 hunks) -- #197 Same router story (to=0xe3985e6b..., Maya router v4), so #216's exact r/s -- byte-identical to the thorchain pair, same base fixture -- are stale here too. #197 does not merely relax: it reconstructs the sighash host-side and recovers the signer, so a wrong digest, wrong calldata or wrong key fails. Taking #216 would also have re-added a stray `)` and restored @unittest.skip on test_mayachain_remove_liquidity, which #197 unskipped. Raw assertion count in this file drops 26 -> 13 because seven frozen-vector memo cases became one loop over the same seven memos and three skipped tests now actually run; memo coverage is unchanged at nine cases and none of them are skipped any more. tests/test_msg_ton_signtx.py (9 hunks) -- union Both sides added the identical AdvancedMode opt-in to nine tests; #216 also carried the four-line reason. Kept #216's comment and call on all nine. #197 additionally gated three tests #216 did not -- the reject-path tests -- and those auto-merged; annotated them, because opening the gate is what stops them passing for the wrong reason (with the gate shut the firmware refuses every TonSignTx and the raises-assertion is satisfied without the parser ever validating a field). scripts/generate-test-report.py (17 hunks) -- #197 Fifteen hunks are section-letter collisions: #216 filed Display Binding under 'S' and Display Disclosure under 'D', but 'S' is already Solana and 'D' is already BIP-85 Child Derivation, so #216's own catalog carries each letter twice. #197 assigns 'J' and 'Q' and does not collide; _audit_catalog() asserts letter and id uniqueness and would have tripped on #216's scheme. Entries are otherwise identical -- same module, same test, same prose -- so nothing #216 catalogued is lost, only relettered. Fixed one cross-reference #197's rename left dangling: J13's prose said "the control for S12", which now points at a Solana test; it means J12. The other two hunks are #197-only additions (the F, I, L, U, TD sections and MUST_RUN_MODULES) against an empty #216 side. tests/test_msg_ethereum_erc20_0x_signtx.py -- union, and git got this one wrong Not one of the 31: it merged clean and merged badly. Both sides inserted an AdvancedMode opt-in into test__sign_transformERC20 at different offsets, so the merged file called apply_policy twice in a row. Deduped to one call carrying both rationales (#216's 1480-bytes-past-the-1024-chunk mechanism, which is what atlas J1 actually claims, plus #197's no-recognised-handler note). Folded in the comment #197 left three lines above it saying "No AdvancedMode policy is set here on purpose", which contradicted its own apply_policy call and both sides' findings; kept as an explicitly superseded note rather than deleted, so the behaviour change stays legible. Verified: 0 conflict markers; every .py under tests/, scripts/ and keepkeylib/ parses; _audit_catalog() passes and the report renders 27 sections / 381 distinct tests; all 383 catalog rows resolve to a test that exists (V17-V67 against CLEARSIGN_FLOWS, which generates those methods by the same transform; K3-K11 are native gtest suites the JUnit parser handles by bare classname). Not verified: no test was executed against firmware. The one suite that runs offline, test_storage_version_gate.py, needs the firmware sources checked out above this tree and errors in setUp without them -- unchanged by this merge.
Squashed from 194 commits. Everything below is against master, which this branch already contains -- the 7.14.2 disclosure tests from #216 are merged in, not replaced. BINDINGS (keepkeylib) Protobuf regenerated for the 7.15 surface: ethereum, hive, ripple, solana, thorchain, zcash, types. New modules for the features those messages carry -- clearsign_abi, clearsign_catalog, eip712_stream, signed_metadata, hive, zcash. transport_udp gained a socket timeout. It had none, so a crashed emulator blocked in recv() until the CI job was killed, which is reported as a cancelled job rather than a failing test and throws away every result. eth/token_policy.py bounds the built-in token table. It is the largest read-only symbol in the ARM image (31KB for 1,945 mostly-2018 entries) and the vetted source it comes from is a stale snapshot -- no UNI, no AAVE, no modern stables. The policy keeps what users hold plus what coins[] requires, and takes a priority symbol only when the source gives it exactly one address, so a scam token cannot inherit a real one's label. COVERAGE (17 new suites) taproot: address derivation, signing, and on-screen verification zcash: PCZT device signing and seed fingerprint clear-signing: additive tier, signing guards, EIP-712 streaming, Solana LUT attestation, thorchain deposit session trust lifetime, hive, osmosis, bitcoin-only variant, storage gate NOTES ON TWO DELIBERATE CHOICES The thorchain and mayachain suites assert structure rather than frozen (r,s) vectors. #197 repoints those transactions at current routers, and `to` is an RLP field of the EIP-155 sighash, so the old vectors describe a different transaction. The superseded vectors are kept in comments at the assertion site so the gap stays visible and regenerable on hardware. The storage version gate skips on trees that predate it, but still FAILS if storage.c references the floor while storage.h no longer defines it -- the skip cannot swallow the regression it guards.
1f6c240 to
a08dbd9
Compare
There was a problem hiding this comment.
Pull request overview
Consolidates Python bindings, client helpers, CI, and device coverage for firmware 7.15/RC18.
Changes:
- Regenerates protocol bindings and adds Hive, Zcash, Solana, EIP-712, and clear-signing support.
- Expands device and regression coverage across multiple chains.
- Reworks CI to build current firmware and run deterministic contract tests.
Reviewed changes
Copilot reviewed 58 out of 67 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Builds firmware emulator and expands CI gates. |
.github/workflows/copilot-review.yml |
Requests reviews for non-draft PRs. |
.gitmodules |
Targets the protocol release branch. |
build_pb.sh |
Generates Hive bindings. |
keepkeylib/clearsign_abi.py |
Adds static ABI encoding helpers. |
keepkeylib/debuglink.py |
Adds dice-input debug support. |
keepkeylib/eip712_stream.py |
Implements streamed EIP-712 encoding. |
keepkeylib/eth/ethereum_tokens.py |
Applies token-table budgets. |
keepkeylib/eth/token_policy.py |
Defines token selection policy. |
keepkeylib/eth/uniswap_tokens.py |
Budgets Uniswap token entries. |
keepkeylib/hive.py |
Adds Hive client helpers. |
keepkeylib/mapping.py |
Registers clearsign and Hive messages. |
keepkeylib/messages_ripple_pb2.py |
Adds Ripple memo binding. |
keepkeylib/messages_solana_pb2.py |
Adds Solana attestation fields. |
keepkeylib/messages_thorchain_pb2.py |
Adds THORChain denomination field. |
keepkeylib/signed_metadata.py |
Extends clear-sign metadata formats. |
keepkeylib/transport_udp.py |
Adds emulator response timeouts. |
keepkeylib/zcash.py |
Adds ZIP-32 fingerprint calculation. |
tests/common.py |
Adds capability gates and screenshot cleanup. |
tests/config.py |
Adds forced UDP selection. |
tests/probe.py |
Adds import-path diagnostics. |
tests/test_message_signing_protocol_bindings.py |
Checks Solana binding compatibility. |
tests/test_msg_bip85.py |
Updates BIP-85 firmware gating. |
tests/test_msg_cosmos_signtx.py |
Replaces an unprofessional memo fixture. |
tests/test_msg_eip712_streaming.py |
Covers streamed structured EIP-712. |
tests/test_msg_ethereum_clearsign_additive.py |
Tests additive clear-sign reviews. |
tests/test_msg_ethereum_erc20_0x_signtx.py |
Documents blind-sign policy behavior. |
tests/test_msg_ethereum_erc20_uniswap_liquidity.py |
Centralizes emulator skipping. |
tests/test_msg_ethereum_signing_guards.py |
Covers Ethereum signing guards. |
tests/test_msg_ethereum_signtx.py |
Adds policy and chain-ID regressions. |
tests/test_msg_ethereum_thorchain_deposit.py |
Covers THORChain EVM deposits. |
tests/test_msg_getaddress_taproot.py |
Adds BIP-86 address vectors. |
tests/test_msg_getentropy.py |
Tests entropy budget behavior. |
tests/test_msg_osmosis_signtx.py |
Adds Osmosis signing coverage. |
tests/test_msg_recoverydevice_cipher.py |
Updates recovery-version documentation. |
tests/test_msg_resetdevice.py |
Adds dice-reset and ceremony tests. |
tests/test_msg_signtx_taproot.py |
Adds Taproot transaction coverage. |
tests/test_msg_solana_lut_attestation.py |
Tests Solana LUT attestations. |
tests/test_msg_thorchain_signtx.py |
Updates THORChain router fixtures. |
tests/test_msg_ton_signtx.py |
Enables parser-specific rejection tests. |
tests/test_msg_tron_signtx.py |
Applies blind-sign policy handling. |
tests/test_msg_zcash_display_address.py |
Updates device-derived address tests. |
tests/test_msg_zcash_seed_fingerprint.py |
Tests device fingerprint binding. |
tests/test_msg_zcash_sign_pczt.py |
Adds offline PCZT contract tests. |
tests/test_msg_zcash_sign_pczt_device.py |
Adds shielded device coverage. |
tests/test_protection_levels.py |
Clears sessions before entropy tests. |
tests/test_sign_typed_data.py |
Covers typed-data policy gates. |
tests/test_taproot_screens.py |
Adds long-address OLED coverage. |
tests/test_zcash_seed_fingerprint_helper.py |
Tests fingerprint helper vectors. |
tests/txcache/insight_bitcoin_tx_3e1fdf….json |
Adds mixed-input transaction fixture. |
tests/txcache/insight_bitcoin_tx_6e3203….json |
Adds Taproot prevout fixture. |
Files not reviewed (7)
- keepkeylib/messages_ethereum_pb2.py: Generated file
- keepkeylib/messages_hive_pb2.py: Generated file
- keepkeylib/messages_ripple_pb2.py: Generated file
- keepkeylib/messages_solana_pb2.py: Generated file
- keepkeylib/messages_thorchain_pb2.py: Generated file
- keepkeylib/messages_zcash_pb2.py: Generated file
- keepkeylib/types_pb2.py: Generated file
Suppressed comments (3)
tests/test_msg_solana_lut_attestation.py:193
- As in the first test, the baseline signing clears the RAM-only signer. The replay attempt therefore runs with no verification key and proves only the no-signer fallback, not that transaction binding rejects a replay. Reload the signer after collecting the baseline.
tests/test_msg_ethereum_thorchain_deposit.py:198 - This tuple is equivalent to catching all
Exception, so any unrelated failure satisfies the unpinned-chain regression test. Catch onlyCallExceptionand assert the blind-sign policy message to ensure the intended guard is what rejected the transaction.
tests/test_msg_thorchain_signtx.py:159 - These structural assertions no longer prove that the changed router/calldata was committed to the signature. Replace the superseded oracle with a regenerated golden signature or an independent digest/recovery check.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| path = device-protocol | ||
| url = https://github.com/keepkey/device-protocol.git | ||
| branch = master | ||
| branch = up/release-protocol |
Two correctness defects and four test-integrity ones. eip712_stream: multidimensional arrays were validated against the WRONG dimension. Solidity nests right-to-left -- in T[k][j] the outer array holds j -- so int16[2][][4] parses to [2, 0, 4] while the first list a walker meets holds 4. Levels are now consumed from the END. Single-dimension arrays are unaffected (both ends coincide), which is why every existing test passed. clearsign_abi: signed integers were encoded as unsigned. intN was rejected for every negative value and ACCEPTED at or above 2^(N-1), which the EVM reads back as negative -- calldata that does not mean what its declared type says. Split the paths; intN is now range-checked to [-2^(N-1), 2^(N-1)-1] and sign-extended. test_msg_ethereum_thorchain_deposit: assertRaises((CallException, Exception)) accepts every failure, including a fixture that fails to build, so a security gate could pass without the firmware ever refusing. Narrowed to CallException. test_msg_solana_lut_attestation: two tests compared a degraded run against a baseline without reloading the RAM-only signer that the preceding signing tore down -- so they compared two identical baseline flows and would pass even if bad signatures were accepted. The attested test above them already documents this exact trap; the other two now reload too. test_msg_thorchain_signtx: restores real verification. Both tests asserted only r/s LENGTHS, which a wrong router, wrong calldata or wrong sighash would also satisfy. They now reconstruct the legacy sighash and recover the signer, comparing it to ethereum_get_address -- the pattern already proven in the mayachain suite. Stronger than the frozen vectors this replaced, and it stays correct across router changes. The superseded 7.14.2 vectors are kept as comments. test_msg_recoverydevice_cipher: gate raised to 7.15.1 to match its docstring. .gitmodules: device-protocol tracks master again, not up/release-protocol. NOT taken: forwarding a non-rune denom through thorchain_sign_tx. The firmware this targets hardcodes "denom":"rune" in its sign-doc; only 7.15+ reads one. nanopb SKIPS unknown fields, so forwarding it to older firmware would be silently ignored and the device would sign a RUNE transfer while the host believed otherwise. The refusal is fail-closed and stays, now with the reason recorded at the guard.
The job ran the firmware's firmware-unit suite alongside this repo's tests and failed if EITHER reported non-zero. No change in this repository can affect firmware C++, and the firmware repo already runs that suite in its own CI, so the only thing it contributed was failing python-keepkey for reasons no python change caused. It is failing that way right now: this branch trims the built-in token table, which requires a matching firmware change to tokens.def. The job clones firmware master, so it cannot go green until that change reaches master -- a release away -- even though this repo's own suite passes 417/0. Also hardens the verdict. The old check was [ "$(cat test-reports/python-keepkey/status)$(cat .../firmware-unit/status)" = "00" ] which printed 'cat: ... No such file or directory' and compared an empty string whenever a container died before writing its status -- so a crashed run could not report a verdict at all. Missing status is now an explicit failure.
Forward ThorchainMsgSend.denom on firmware 7.15+, retain the fail-closed RUNE-only path on older firmware, and add offline and device-path coverage. Add regression tests for the reviewed EIP-712 array-order and signed ABI integer fixes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 68 changed files in this pull request and generated no new comments.
Files not reviewed (7)
- keepkeylib/messages_ethereum_pb2.py: Generated file
- keepkeylib/messages_hive_pb2.py: Generated file
- keepkeylib/messages_ripple_pb2.py: Generated file
- keepkeylib/messages_solana_pb2.py: Generated file
- keepkeylib/messages_thorchain_pb2.py: Generated file
- keepkeylib/messages_zcash_pb2.py: Generated file
- keepkeylib/types_pb2.py: Generated file
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
keepkeylib/client.py:2109
- The deferred transparent response is accepted without checking that it contains one signature per transparent input. A device response that omits
ZcashTransparentSignedor returns a short list therefore succeeds and can hand the caller an incomplete transaction, while Orchard signatures are correctly count-checked below. Validate this count before returning the final response.
if isinstance(resp, zcash_proto.ZcashTransparentSigned):
transparent_sigs = list(resp.signatures)
resp = self.transport.read_blocking()
tests/test_msg_osmosis_signtx.py:163
- This test still expects raw-wire
uatomto sign, contradicting the new uosmo-only invariant documented and enforced bykeepkeylib/client.py:1057-1066. Against the intended 7.15 firmware,sign_denom('uatom')should raise instead of returning a second signature. Replace this with a raw-wire rejection assertion and update the P4 report entry accordingly.
tests/test_msg_bitcoin_only_variant.py:16 - This module-level contract is now false:
setUp()unconditionally callsrequires_bitcoinOnly(), so every test skips on the regular image and none provides dual-build evidence. Describe the actual bitcoin-only-only scope instead.
tests/test_msg_bitcoin_only_variant.py:107 - This unconditional gate skips all eleven new product-boundary tests in the only Python integration job:
.github/workflows/ci.yml:120-129builds and starts only the regular emulator, with no bitcoin-only matrix/job. Consequently the advertised bitcoin-only coverage is never exercised by this repository's required CI. Add a bitcoin-only emulator integration target that runs this suite.
The comment claimed firmware enforces uosmo-only on direct OsmosisMsgAck traffic. It does not. Since 7.14.2 (firmware c9dccf68) osmosis_signTxUpdateMsgSend escapes the host-supplied denom straight into the signed Amino document, and the only remaining strcmp against "uosmo" in firmware selects the display exponent. The raw-wire test test_osmosis_send_denom_is_committed_to_the_signature is correct as written; the comment was the stale artifact. State the real reason the host check stays: this helper is not version-gated, and firmware older than 7.14.2 hardcoded uosmo in the serializer, so forwarding a non-uosmo denom there would silently sign a uosmo transfer the caller never asked for.
…-gates test(bitcoin-only): gate unsupported 7.15 handlers
# Conflicts: # scripts/generate-test-report.py
|
Canonical reconciliation update: head 7598628 now contains both the prior upstream-sync work and the audited fork 7.14.3/7.15 Python work. Fork exact-head CI is green: https://github.com/BitHighlander/python-keepkey/actions/runs/33147640829. Independent upstream push and PR CI are also fully green against regular 7.14.3, regular 7.15, and both Bitcoin-only candidates: https://github.com/keepkey/python-keepkey/actions/runs/33147869705 and https://github.com/keepkey/python-keepkey/actions/runs/33147872591; CodeQL is green. Dependency order remains unchanged: review/merge device-protocol #112, repin this PR to the resulting canonical protocol master commit, rerun this exact matrix, then approve/merge #197. @pastaghost please review after the canonical protocol repin. |
Purpose
This is the single master-targeting Python release PR for firmware 7.15 / RC18. It consolidates the original #197 work, all seven commits from #199, and the review/test commits that previously had no PR. No history was rewritten;
reconcile/upstream-syncwas advanced normally.Contents
RC18 Zcash contract
The regular/full RC18 firmware includes Orchard privacy. Only the bitcoin-only build compiles non-Bitcoin features out; there is no separate Zcash artifact.
zcash_sign_pcztimplements the exact firmware 7.15 conversation:is_spendexplicitly;Deterministic scripted-flow tests cover all-dummy shield, mixed deshield, private Orchard send, compact signature ordering, malformed requests, signature-count mismatches, and preflight rejection.
Protocol provenance
https://github.com/keepkey/device-protocol.git6d0ae670e287a75338244fe82c4bef33a920a2eeValidation
addc0242847cd7be9c402980498b321b344bef34;Review and merge gates
Do not merge this PR until the protocol review and canonical repin are complete.