test(7.15): consolidated Python harness, bindings, and device coverage - #197
BitHighlander wants to merge 511 commits into
Conversation
BitHighlander
changed the title
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.
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.
|
Addressing the four suppressed comments ("Previously missed") from the round-2 Copilot review. They were not posted as inline threads, so the responses are collected here. Pushed as 1.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 68 changed files in this pull request and generated 1 comment.
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 (6)
Previously missed (4) — in code that hasn't changed since the last review.
.github/workflows/ci.yml:100
- Pin the firmware revision used by this release CI instead of building the mutable
alphabranch. The current green run can therefore validate post-RC18 behavior rather than the RC18 dependency graph; for example, it passes the new Ironwood acceptance test even though the linked RC18 protocol contract marks Ironwood fields as schema-only and requires rejection. A lateralphapush can also change this PR's result without any Python commit.
- name: Checkout firmware
uses: actions/checkout@v4
with:
repository: BitHighlander/keepkey-firmware
ref: alpha
path: keepkey-firmware
tests/test_msg_zcash_sign_pczt_device.py:305
- This asserts Ironwood succeeds on firmware 7.15, but the linked device-protocol #112 RC18 contract explicitly says
shielded_pool/ironwood_digestare schema-only and RC18 rejects a non-Orchard pool. That also conflicts with this PR's stated Orchard-only RC18 contract/report. Remove this from RC18 coverage or gate it to the later firmware version that actually supports Ironwood; RC18 should test refusal instead.
tests/test_msg_ethereum_signing_guards.py:59 - These assertions only prove that the device returned a correctly shaped signature. The malformed-RLP regression described by this test can still return 32-byte
r/svalues, so the test would pass while signing the wrong preimage. Reconstruct the EIP-1559 digest with an empty priority-fee field and recover/verify the signer against the device address.
This issue also appears on line 131 of the same file.
keepkeylib/clearsign_abi.py:77
- The encoder accepts non-existent Solidity integer widths such as
uint7,uint0, orint264and can emit plausible 32-byte calldata for them. Since this module promises canonical Solidity ABI encoding, validate both signed and unsigned widths as multiples of 8 in the 8..256 range before encoding.
This issue also appears on line 78 of the same file.
elif typ.startswith('uint'):
digits = typ[4:]
bits = int(digits) if digits else 256
n = int(val)
tests/test_msg_ethereum_signing_guards.py:135
- This does not test either property in its name/docstring: any 32-byte
r/spasses even if only the first calldata chunk was hashed, and no DebugLink screen is inspected to ensure the Sablier clear-sign summary was suppressed. Recover the signer against a digest containing the fulldataand record/assert the fallback raw-data review screen.
keepkeylib/clearsign_abi.py:82 - Static type validation is also permissive here: arbitrary truthy values are silently rewritten to ABI
true, andbytes0/bytes33are accepted (the latter emits more than one word and shifts every following argument). Require an actual boolean and restrict fixed bytes tobytes1throughbytes32.
elif typ == 'bool':
out += (1 if val else 0).to_bytes(32, 'big')
elif typ.startswith('bytes') and typ != 'bytes' and not typ.endswith('[]'):
n = int(typ[5:])
b = val if isinstance(val, (bytes, bytearray)) else bytes.fromhex(
|
Round-3 review: the one inline comment is answered in its own thread. The 6 suppressed comments have no threads, so they are answered here. Pushed as
|
test(solana): gate plain text at its 7.15 capability
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One critical transport hang and multiple moderate correctness, compatibility, coverage, dependency, and CI issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (6)
build_calldata("foo(uint)", ...)hashes the alias spelling verbatim, but ABI selectors are…
| data = self.socket.recv(64) | ||
| try: | ||
| data = self.socket.recv(64) | ||
| except socket.timeout: |
| [submodule "device-protocol"] | ||
| path = device-protocol | ||
| url = https://github.com/keepkey/device-protocol.git | ||
| url = https://github.com/BitHighlander/device-protocol.git |
| seen.add(key) | ||
| chosen.append(r) | ||
|
|
||
| for r in sorted(records, key=address_of): |
| if not mnemonic or mnemonic[-1] != words: | ||
| mnemonic.append(words) |
| tx.append(1) # 1 instruction | ||
| tx.extend(bytes([1])) # program index -> SYSTEM_PROGRAM | ||
| tx.append(1) # 1 account index | ||
| tx.append(3) # index 3: BEYOND the static table -> external |
Canonical multi-release companion
This is the single canonical python-keepkey branch used by firmware 7.14.x and 7.15. It does not encode a 7.15-only fork: behavior is selected by the device protocol, firmware version, product variant, and explicit partial-stack capability declarations.
Immutable dependencies and targets
3e1689d3ec6ffb68c8aefbbb091e0fca2b78cd4b49d537ce953b7524599bfc87f56b7a50a51a13a1dd9c85dc747cf965fb0e7bf49615dc9e7568ee6589a64f717e1690bb31adb3e4c38e23640357333c4125e1c7409b1cb7b08ba595bc408e3128fc24ca9b0583a41b758af7fb4c08280ab2a2bd8bb5d503The head differs from its Docker-tested parent only by two immutable workflow refs. Both regular and bitcoin-only hosted 7.15 jobs now target the exact Docker-validated firmware runtime head. The bitcoin-only matrix consumes each row its own immutable
firmware_ref.Release behavior
The branch carries generated bindings, chain clients, offline fixtures, ERC-7730/Solana helpers, full and bitcoin-only matrices, exact OLED assertions, owned-emulator storage reboot gates, and release-policy controls. Runtime clear-sign providers remain AdvancedMode-only and session-scoped; persistent providers and the KeepKey trusted root are not part of the 7.15 contract. Older firmware retains safe behavior through version and feature gating.
Docker evidence
Full release policy, image manifests, skip disposition, and human audit instructions are in firmware PR BitHighlander/keepkey-firmware#843.
Merge gates
Merging this host dependency does not authorize firmware merge, hardware testing, or release.