test(7.15): consolidated Python harness, bindings, and device coverage#197
Open
BitHighlander wants to merge 91 commits into
Open
test(7.15): consolidated Python harness, bindings, and device coverage#197BitHighlander wants to merge 91 commits into
BitHighlander wants to merge 91 commits into
Conversation
Same firmware as the standalone UDP kkemu binary, loaded in-process via
ctypes. Lets python-keepkey exercise the firmware contract that the
keepkey-vault FFI path imposes — most importantly, the caller-driven
polling model (no daemon thread to call kkemu_poll for you).
- keepkeylib/transport_dylib.py: DylibState (process-wide singleton over
ctypes-loaded libkkemu) + DylibTransport (one per iface 0/1).
Pumps kkemu_poll on every read/write so the firmware actually makes
forward progress on caller turns.
- tests/config.py: KK_TRANSPORT=dylib KK_DYLIB=/path/to/libkkemu.dylib
routes the same fixture to the FFI transport instead of UDP.
- tests/test_dylib_confirm_flow.py: regression for the confirm-flow
contract (Initialize, WipeDevice, LoadDevice, GetAddress). Skipped
unless KK_TRANSPORT=dylib so it won't break the default UDP run.
Reproduces the keepkey-vault hang deterministically: Initialize round-
trips fine, wipe_device hangs because confirm_helper busy-loops on a
ButtonAck the dylib silently consumed but never delivered. Caught in
~10s, no electrobun / bun stack required.
Run:
cd tests && KK_TRANSPORT=dylib KK_DYLIB=.../libkkemu.dylib \
PYTHONPATH=..:../keepkeylib python3 -m pytest \
test_dylib_confirm_flow.py -v
…ntics The existing test_dylib_confirm_flow covers the caller-driven polling contract — Initialize / Wipe / LoadDevice / GetAddress — but never asks the firmware for a layout. Two changes that just landed in the firmware emulator runtime PR (BitHighlander/keepkey-firmware#217) need functional coverage that confirm-flow doesn't provide: 1. RINGBUF_CAPACITY in lib/emulator/ringbuf.h was bumped from 32 to 128. DebugLinkState's 2048-byte `layout` plus the rest of the message serializes to ~44 HID reports through the output ring; the previous capacity left effective room for 31 reports, so screenshot capture truncated mid-layout (msg_debug_write ignores emulatorSocketWrite's 0-on-full return). 2. fsm_msgDebugLinkGetState in lib/firmware/fsm_msg_debug.h now does a single display_refresh() instead of force_animation_start() + animate(). The old form overwrote static layouts with stale animation frames or no-ops depending on queue state, so screenshots captured something different from what the user was seeing. Both fixes are functionally invisible to the existing test suite. Without these tests, regressing either change ships green. This commit adds: - tests/test_dylib_screenshot.py — four tests: * test_layout_round_trip_fits_through_ring (RINGBUF_CAPACITY) * test_layout_repeated_reads_no_truncation (RINGBUF_CAPACITY) * test_layout_stable_across_idle_reads (canvas semantics) * test_layout_features_dont_corrupt_capture (iface separation) Constructs a fresh KeepKeyDebuglinkClient against the dylib singleton WITHOUT going through common.KeepKeyTest.setUp — that fixture wipes the device on every test and exercises the confirm-flow path that test_dylib_confirm_flow is itself a pending regression for. Reading a layout doesn't require any of that; we just init and ask DebugLink for the home-screen capture. - tests/config.py — explicit-transport precedence fix: Previously HID/WebUSB were always autodetected first. With a real KeepKey plugged in, KK_TRANSPORT=dylib was silently overridden — the dylib regression suite would either route to hardware or crash on hid.pyx. Now the explicit env var (KK_TRANSPORT=dylib) skips hardware enumeration entirely, the dylib path runs as requested, and the default (no env var set) falls back to the existing UDP behavior. Verified locally: cmake -DKK_EMULATOR=1 -DKK_BUILD_DYLIB=1 -DKK_DEBUG_LINK=ON \ -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -B build-emu . cmake --build build-emu --target kkemulator_dylib KK_TRANSPORT=dylib KK_DYLIB=build-emu/lib/libkkemu.dylib \ PYTHONPATH=keepkeylib:. python -m pytest tests/test_dylib_screenshot.py ======================== 4 passed in 0.36s ======================== Out of scope: SignTx + other multi-step flows that go through confirm_helper. They share the same hang as test_dylib_confirm_flow's test_load_device_with_auto_confirm — copying the pattern would just produce a second red regression for the same underlying firmware bug, not new coverage. Once the confirm-flow regression goes green, signtx expansion is a follow-up.
…ANSPORT, split confirm-flow setUp Three findings from review of PR #14: #1 (High) test_dylib_confirm_flow used common.KeepKeyTest.setUp which calls wipe_device() — the same path the file's pending regression is for. Hangs in setUp can't be classified by xfail or interrupted by pytest-timeout, so test_features_round_trip ("just Initialize") was actually wipe + Initialize. Refactored to construct KeepKeyDebuglinkClient directly in setUp (matching test_dylib_screenshot's pattern), moved wipe + load_device into the one pending test. Tried the reviewer-suggested @pytest.mark.xfail(strict=True) + @pytest.mark.timeout combo. pytest-timeout (both signal and thread methods) cannot interrupt the C-level kkemu_poll busy-loop — the hang locks up the entire test runner instead of failing the test. Switched to @unittest.skip with explicit rationale documenting exactly that, plus the promotion path: when firmware lands the confirm fix, drop the skip; if a future change makes kkemu_poll GIL-friendly, switch back to xfail+timeout. #2 (Medium) tests/config.py treated any non-empty KK_TRANSPORT as "explicit" and skipped HID/WebUSB autodetect, but only "dylib" was actually handled. A typo like KK_TRANSPORT=dyllib silently fell through to UDP with hardware disabled. Now scoped to a _KNOWN_TRANSPORTS set; unsupported values raise at config import, surfacing typos at test collection time. Verified end-to-end: `KK_TRANSPORT=dyllib pytest test_msg_signtx.py` now errors on collection with the typo'd value in the message. #3 (Medium/Low) DylibTransport.ready_to_read appended raw frame bytes to read_buffer but DylibTransport._pump_one stripped the leading '?' HID marker first. Inconsistent stripping corrupts multi-frame message reassembly: _read_headers can scan a stray '?' from one chunk into the middle of contiguous payload bytes from another, decoding the wrong message-type / length. Centralised the read+strip into a private _poll_and_stash helper shared by both ready_to_read (no sleep) and _pump_one (sleeps on miss). Now the buffer always contains continuation+payload bytes only; the leading '?' is stripped at the single point of stashing. Trailing HID padding zeros from short messages are still tolerated by _read_headers' magic-character search. Verified locally: KK_TRANSPORT=dylib KK_DYLIB=build-emu/lib/libkkemu.dylib \ pytest tests/test_dylib_screenshot.py tests/test_dylib_confirm_flow.py ================== 5 passed, 1 skipped in 0.15s ==================
Wires the ZIP-32 §6.1 seed fingerprint binding into the python-keepkey
client to mirror the firmware-side validation.
device-protocol submodule
- URL: keepkey/device-protocol -> BitHighlander/device-protocol
(zcash work pins to fork master while seed_fingerprint sits in
long-term review for upstream; revert when upstream merges.)
- pin: d0b8d80 -> 4337c452 (BitHighlander/master with PR #27 merged).
- messages_zcash_pb2.py regenerated via docker_build_pb.sh
(kktech/firmware:v8 → libprotoc 3.5.1, the canonical toolchain).
Selective regen — other pb2 files are intentionally NOT
regenerated because they currently include content from
BitHighlander/device-protocol open PRs (#18 SolanaTokenInfo,
#19 TRON clear-signing, #20 TON clear-signing, #21
EthereumTxMetadata). Until those merge, regenerating them
against current master would back out work that the existing
python-keepkey client relies on.
keepkeylib/zcash.py (new)
calculate_seed_fingerprint(seed) -> 32 bytes
Pure-Python helper. BLAKE2b-256("Zcash_HD_Seed_FP",
I2LEBSP_8(len) || seed). Matches the firmware C
implementation byte-for-byte and the keystone3-firmware
reference vector
seed = 000102...1f
fp = deff604c246710f7176dead02aa746f2fd8d5389f7072556dcb555fdbe5e3ae3
keepkeylib/client.py
zcash_display_address — add expected_seed_fingerprint kwarg
zcash_sign_pczt — add expected_seed_fingerprint kwarg
Both pass through unchanged when the kwarg is None
(backward compatible).
tests/test_msg_zcash_seed_fingerprint.py (new)
Pure-Python helper:
- reference vector (Keystone3 cross-check)
- rejects all-zero, all-0xFF, short, long
Device-backed:
- GetOrchardFVK returns non-empty seed_fingerprint
- fingerprint stable across accounts (bound to seed, not account)
- DisplayAddress: matching expected_seed_fingerprint succeeds,
response carries seed_fingerprint
- DisplayAddress: wrong expected_seed_fingerprint rejected
- DisplayAddress: omitting expected_seed_fingerprint still works
- SignPCZT: wrong expected_seed_fingerprint rejected before
any signing crypto runs
Addresses review of PR #15. Test structure Helper tests (no device) move to a dedicated module: tests/test_zcash_seed_fingerprint_helper.py This module deliberately does NOT import common, transport, or any protobuf bindings, so it runs on a stock dev box: pytest tests/test_zcash_seed_fingerprint_helper.py The previous file inherited common.KeepKeyTest, whose setUp wipes the device — pytest -k 'helper' was never actually offline. Client wrapper coverage Device-backed tests now go through the public client helpers (self.client.zcash_display_address(... expected_seed_fingerprint=...) and self.client.zcash_sign_pczt(... expected_seed_fingerprint=...)) rather than building raw protobuf messages with self.client.call(). Confirms the kwarg pass-through end-to-end. New test test_device_fingerprint_matches_python_helper: cross-checks the device-computed fingerprint against the python-keepkey helper for the same seed (all-allallall mnemonic, empty passphrase). Ties the firmware C, python-keepkey helper, and ZIP-32 §6.1 reference vector to the same byte-for-byte output.
feat(zcash): seed_fingerprint client + tests
… ≤ 7.14.0)
Pairs the device, signs a 1550-byte EIP-1559 transaction with the
all-all-all test mnemonic, and asserts that ECDSA recovery against the
canonical type-2 pre-image yields the device's own address.
Catches a firmware/ethereum.c ordering bug present in 7.x.0 .. 7.14.0
where the empty access-list byte (0xC0) — which closes the EIP-1559 RLP
body and must be the last byte fed to keccak before signing — was being
hashed inside ethereum_signing_init() right after the initial 1024-byte
data chunk, BEFORE the host had a chance to send the remaining
EthereumTxAck frames. For any tx whose data exceeded the single-chunk
threshold, the resulting pre-image was:
keccak( ...header...
|| data_len_prefix
|| data[0..1024]
|| 0xC0 (bug: should be after ALL data)
|| data[1024..end] )
The signature was mathematically valid for that mangled hash so RPCs
accepted the broadcast, but the recovered signer was a wrong-but-
deterministic address. The mempool dropped the tx because the recovered
"from" had no balance / wrong nonce. Production symptom: every Uniswap
Universal Router swap, Permit2 batch, and large multicall hung at
"Confirm in wallet."
Single-chunk transactions (<= 1024 bytes) escaped the bug only by
accident — the misplaced 0xC0 happened to land at the end anyway.
Recovery-based assertion (eth-keys, eth-utils.keccak) — works on any
seed, no golden vectors to capture, the test asserts the actual
invariant: "signature recovers to the signer." Fails on broken
firmware, passes on 7.14.1+.
CI: eth-keys added to the existing pip install line; ships a pure-Python
keccak via eth-utils so no native deps are required.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
requires_message("EthereumTxAck") sends an empty EthereumTxAck as a
discovery probe. The firmware (correctly) rejects that with
Failure_UnexpectedMessage because we're not mid-sign, which skips the
test before the actual assertion runs.
requires_firmware("7.2.1") is sufficient — EthereumTxAck has been part
of the protocol since EIP-1559 support landed in 7.2.1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eth-utils ships keccak via the eth-hash adapter, which auto-selects between pycryptodome and pysha3 at import time. Without either backend installed, importing keccak raises: ImportError: None of these hashing backends are installed: ['pycryptodome', 'pysha3']. The new EIP-1559 chunked-data regression test imports keccak from eth_utils to build the canonical type-2 pre-image, so it failed at import rather than at the recovery assertion. Adding pycryptodome to the existing pip-install line fixes it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
KeepKeyTest overrides unittest's assertEqual with a 2-arg version (common.py:104) that doesn't accept the optional msg parameter — passing one raises: TypeError: KeepKeyTest.assertEqual() takes 3 positional arguments but 4 were given Print the regression diagnostic before asserting instead. Pytest captures stdout on failure, so the divergence (expected vs recovered, canonical hash, sig values) still surfaces in the failure report. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upstreaming this test as a permanent regression guard rather than a one-shot bug catcher. Bumping requires_firmware from 7.2.1 (the version where EIP-1559 support originally landed) to 7.14.1 (the first version where the access-list ordering bug is fixed) so CI on broken builds skips this test instead of flagging a known-broken state as a new regression. The header comment already documents the affected range (7.x.0 .. 7.14.0) and the fix landing in 7.14.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gnition - Bump device-protocol submodule to 8f80bcd (adds memo field to RippleSignTx) - Update messages_ripple_pb2.py with memo field (field 7, optional string) compatible with protobuf==3.20.3 (old-format serialized_pb descriptor) - Add test_sign_with_thorchain_memo in test_msg_ripple_sign_tx.py: verifies serialized XRPL tx ends with canonical Memos array binary (F9 EA 7D <len> <memo> E1 F1), requires firmware 7.14.2 - Add test_msg_ethereum_thorchain_deposit.py: covers legacy deposit() 0x1fece7b4 selector, new depositWithExpiry() 0x44bc937b selector (requires 7.14.2, no AdvancedMode), and verifies non-THORChain addresses are still blocked without AdvancedMode
feat(7.14.2): XRP THORChain memo + EVM depositWithExpiry tests
* feat(hive): add Hive blockchain support - messages_hive_pb2.py — generated from messages-hive.proto (IDs 1600-1603) - hive.py — get_public_key / sign_tx client helpers - mapping.py — register HiveGetPublicKey, HivePublicKey, HiveSignTx, HiveSignedTx wire IDs - client.py — hive_get_public_key / hive_sign_tx methods on ProtocolMixin * feat(hive): add HiveGetPublicKeys, HiveSignAccountCreate, HiveSignAccountUpdate - messages_hive_pb2.py: regenerated from updated proto; now includes all 10 message types (HiveGetPublicKey/Keys, HivePublicKey/Keys, HiveSignTx/ed, HiveSignAccountCreate/ed, HiveSignAccountUpdate/ed). Added role field to HiveGetPublicKey. - mapping.py: register wire IDs 1604-1609 for the six new message types. - hive.py: add get_public_keys(), sign_account_create(), sign_account_update() helpers. get_public_key() gains optional role parameter. - client.py: add hive_get_public_keys(), hive_sign_account_create(), hive_sign_account_update() mixin methods with @expect decorators.
…format Regenerated using protoc from kktech/firmware:v15 (protobuf 3.17.3). The previous version used the builder API (protobuf 3.20+) which is incompatible with the 3.20.3 Python runtime pinned in CI.
Test bugs fixed (mirrors BitHighlander/keepkey-firmware alpha CI fixes): - ETH THORChain deposit: assertIn(sig_v, [27,28]) -> [37,38] (EIP-155 chain_id=1) - XRP no-memo check: b'\xf9' -> b'\xf9\xea' (0xF9 appears in DER sigs naturally) - Zcash FVK validation: skipTest until feature lands in firmware
Covers the full Hive message surface (all 5 firmware handlers) using the standard 12-word seed (mnemonic12, "alcohol ... aisle"): - HiveGetPublicKey — active-role key format + 33-byte raw - HiveGetPublicKeys — 4 distinct STM role keys; single/bulk agreement - HiveSignTx — transfer (op 2), signature recovers to active key - HiveSignAccountCreate — account_create (op 9), recovers to owner key + binds the 4 device keys and account name into the signed bytes - HiveSignAccountUpdate — account_update (op 10), recovers to owner key Account-op tests are self-validating: they recover the signer from the 65-byte device signature over SHA256(chain_id || serialized_tx) and assert it equals the device-derived key — exercising the device and validating the attestation digest (keepkey-vault docs/HIVE-ATTESTATION-DIGEST-SPEC.md). No golden vector required; recovery is an independent check. Hive was the one alpha-firmware feature with full firmware+client support and zero test coverage.
Addresses review: substring-presence was too weak — a role swap (both keys present), a creator rewrite, or an amount change could still pass. Add a cursor-based Graphene reader matching the firmware append_* layout exactly (incl. account_update's 0x01 optional-present flags, asset symbol padding, and the no-wrapper memo_key) and rewrite all three signing tests to parse and assert each field at its expected position + assert_end() for no trailing bytes: - transfer: from / to / amount / precision / symbol / memo - account_create: fee / creator / name / owner|active|posting authority slots / memo_key - account_update: account / each replacement key in its slot / memo_key Recovery assertions retained. Parser validated offline against hand-built firmware-format bytes.
test(hive): vendored SLIP-0048 multi-key + account-op device tests
KeepKeyTest overrides assertEqual(self, lhs, rhs) with no msg parameter, so the 3-arg calls raised TypeError. Verified: all 5 tests pass against the feature/hive emulator (build-emu/bin/kkemu, fw 7.15.0) — get_public_key(s), sign_tx, sign_account_create, sign_account_update, with signature recovery + full serialized_tx field-binding.
test(hive): fix assertEqual signature — all 5 hive tests green on emulator
…ests Integration-test layer for the firmware Insight clear-signing feature (keepkey-firmware feat/evm-clear-signing-alpha, PR #257). signed_metadata.py: - Fix the key_id/slot footgun: serialize_metadata defaults key_id=3, the DEBUG_LINK CI slot whose pubkey == firmware METADATA_PUBKEYS[3] (the test signer derives to slot 3, NOT slot 0). Production/Pioneer callers must pass key_id=0 explicitly. assert_test_key_matches_slot3() pins this invariant. - sign_metadata fails loud if `ecdsa` is missing (was a silent zero-signature that firmware would reject as MALFORMED, disguising the real cause). Signs the identical byte range firmware hashes (version..key_id, excl. sig+recovery). - Add pure-python keccak256 + EIP-155/EIP-1559 RLP sighash helpers so a metadata blob's tx_hash binds the REAL signing digest. Cross-checked against the device: recovering an existing erc20-approve signature over eth_sighash_legacy yields the test mnemonic's m/44'/60'/0'/0/0 address. test_msg_ethereum_clear_signing.py: - All vectors use key_id=3. - New offline (verified green here, 12/12): slot-3 pubkey assertion, key_id=3 default, keccak256 known vectors. - New device-class cases (run on the kkemu/DEBUG_LINK emulator): tx_hash binding happy path (signs + recovers correct signer), replay reject (metadata bound to tx A, sign tx B → "Metadata does not match signed transaction", no signature), AdvancedMode gate (OFF+unknown→reject, ON→sign, native ERC-20 unaffected), and cancel-clears-metadata (stale blob not reused). Offline portion verified with PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python. Device-class cases require the firmware emulator (libkkemu) + DEBUG_LINK.
… SECTIONS The feature ships in the 7.15.0 firmware tree, so gate the device tests at 7.15.0 (was 7.15.1, which left them dormant on the current build). - test setUp: requires_firmware 7.15.1 -> 7.15.0. - generate-test-report.py SECTIONS 'V' (EVM Clear-Signing) min_firmware 7.15.1 -> 7.15.0; add V9-V12 mapping the new device-class tests (full tx-hash binding happy path, replay reject, AdvancedMode gate, cancel-clears-metadata) with OLED screenshot expectations so the report-driven Phase-1 capture includes them. Verified on the containerized kkemu emulator (docker compose, CI-faithful): all 28 clear-signing tests pass; OLED screenshots captured for the verified flow (INSIGHT VERIFIED icon + decoded method/contract/args), the replay reject, and the AdvancedMode gate.
test(insight): EVM clear-signing integration tests + metadata signer
…tract gate) Covers the firmware Ethereum signing pre-image / clear-sign correctness guards (firmware PR BitHighlander/keepkey-firmware#255, merged to alpha): - type=2 without chain_id is rejected (chain_id over-declared the RLP header) - type=2 with max_fee but no max_priority_fee still signs (priority is a mandatory 0x80-encoded field; Stage 1 and Stage 2 must agree) - type=2 carrying only gas_price, and legacy carrying max_fee_per_gas, rejected - a contract clear-sign handler selector with calldata streamed beyond the initial chunk signs the full data via the generic path instead of confirming a prefix (screen-level assertion verified on-device/emulator)
…tests Three review points, all confirmed against a full integration run. 1) The Maya tests were BROKEN, not merely unverified. Running rc10 against these pins surfaced it: `TypeError: assertEqual() takes 3 positional arguments but 4 were given` — common.KeepKeyTest overrides assertEqual with no msg param (the same trap 591dba8 already fixed once). Both sites failed, so the signature verification never actually ran. Drop the msg argument, and reuse the recovery helper already proven in test_msg_ethereum_clear_signing.py (same signature, hashfunc=None) instead of the bespoke one I added. Integration now: 546 passed, 0 failed (was 544 passed, 2 failed). 2) The public contract was contradictory. client.py still documented all negative counts (including 0x80) and both dimensions as 1..64. Corrected to literals [-127,-1] with 0x80 invalid, width 1..40 (LEFT_MARGIN_WITH_ICON), height 1..64, plus the exactness rule (no straddling run, whole input consumed) and an accurate persist description. 3) The "packed 64x64 needs 512 bytes" rationale is obsolete and is removed. 64px width is no longer legal, and a packed icon at the legal maximum (40x64) is 320 bytes — it WOULD fit the 384-byte cap. RLE is the format because draw_bitmap_mono_rle() is the decoder of record and every bundled image already uses it, not because packed wouldn't fit. The test now asserts that honestly rather than encoding dead arithmetic. Offline suites: 32 passed.
Firmware now refuses non-printable Hive messages (domain separation closes the cross-chain message->transaction signature oracle), so the test that asserted a binary buffer signs is inverted to expect the SyntaxError.
… deposit uses address(0) - Solana token transfer/approve/with_metadata now assert the unchecked variants are blocked without AdvancedMode and blind-sign with it (no signed mint to clear-sign against). - THORChain deposit fixture uses address(0) for native ETH — the only native form the pinned routers accept (0xEeee sentinel would revert on-chain).
…uthorize clear-signs CreateAccount (owner+space not shown) and SetAuthority (takeover vector, None case not disclosed) are now gated behind AdvancedMode; StakeAuthorize clear-signs showing the role and new authority.
…s; solana TransferChecked + attested-symbol OLED tests; report: hive/solana/maya catalog + specificity frame picker - mayachain: the 3 skipped native tests carried thorchain's copy-pasted signature vectors (could never pass). Rewritten self-verifying: host reconstructs the amino sign-doc byte-for-byte (mirrors mayachain.c; the identical construction reproduces thorchain's green frozen vector) and verifies the signature against it + the known device pubkey. - solana: TransferChecked clear-sign test (dedicated mint screen, no AdvancedMode) + signed-token-def test (LoadClearsignSigner attestation over mint/decimals/symbol -> 'signed by alias' screen). messages_solana_pb2 regenerated (protoc 3.5.1) with SolanaTokenInfo signature/signer_key_id; device-protocol pin -> 47e19d8 (matching proto). - report SECTIONS: Hive G6-G28 (sign-message printable-whitelist oracle fix, sign-ops, transfer fences), Solana S8/S12 captions corrected to the force-opaque behavior + S13-S24, Maya M4-M6 native entries, THOR H2/H4 + EVM E20/E21 deposit happy paths get capture hints; H2 + maya memos render full-sequence (every raw-memo page). - frame picker: blank/lock frames never shown; cross-test duplicate census (masked for the scroll-arrow animation) ranks test-specific frames above shared chrome, fixing the leaked IMPORT-RECOVERY/blank frames in section C.
… gated The router pin is now (chain_id, address): an AVAX depositWithExpiry to the live-verified Avalanche router (00dc61..f1d4) must clear-sign with AdvancedMode OFF — the signature is ECDSA-recovered against the host-built EIP-155 pre-image over chainId 43114, and the native amount screen shows msg.value with the chain's ticker (AVAX). A deposit-shaped tx on a chain with no pinned router (BSC) falls to the blind-sign gate. Report catalog: E23 (AVAX clear-sign, with OLED capture) + E24 (unpinned-chain gate).
…tocol The pin (47e19d8, solana signed-token-def fields) was only reachable from a BitHighlander feature branch while .gitmodules declared BitHighlander:master — 'git submodule update --remote' would silently rewind below the proto fields the solana tests depend on. keepkey/device-protocol:up/release-protocol now fast-forwards through the pin, so declared (url, branch) and pin agree.
The emulator-build-test CI project builds firmware master (7.14.x), which still signs unchecked SPL Transfer/Approve/CreateAccount/SetAuthority without AdvancedMode. These five tests assert the 7.15.0 hardening (refusal), so they must skip on older firmware like every other 7.15-behavior test in this suite.
This was referenced Jul 19, 2026
Covers the 11 ops added to the firmware clear-sign table: transfer_to_vesting,
withdraw_vesting, limit_order_create/cancel, convert, comment_options,
transfer_to/from_savings, claim_reward_balance, delegate_vesting_shares,
account_update2.
Transactions are built by this file's own Graphene serializer (the _op_*
helpers), never from firmware-emitted bytes, so a parser bug and a serializer
bug cannot cancel out.
Beyond happy-path sign-and-recover, the negative cases pin the invariants that
actually protect the user:
- asset symbol AND precision are pinned per op: a swapped symbol hides a
~2000x value difference behind an identical-looking number, and a wrong
precision moves the decimal point relative to what the chain applies
- negative int64 amounts are refused (they would render as enormous
positive values)
- comment_options only binds immediately after a comment with a matching
author and permlink; detached it could redirect the payout of a post
published earlier that the user is not reviewing on screen
- account_update2 refuses any authority field
- beneficiaries must be strictly ascending, unique, and sum to <= 100%
- zero amounts are refused where meaningless and accepted where meaningful
(withdraw_vesting 0 stops a power-down, delegate 0 removes a delegation)
- truncated op bodies are refused rather than partially parsed
Two fixes to existing tests:
- Three assertions matched rejection strings that the firmware has since
grouped by cause ("malformed op count", "weight out of range",
"mixed active+posting auths"). The text is diagnostic, not contractual —
the protection is that the device refuses — so the assertions follow the
grouping.
- rejects_excluded_and_unknown_ops used op type 3 as its "unknown op",
mislabelled in a comment as comment_options. Op 3 is transfer_to_vesting
and is now clear-signed, so it no longer reached the unknown-op path.
Switched to 49 (recurrent_transfer), which is deliberately out of the
table.
38/38 pass against an emulator built from keepkey-firmware
feat/hive-clearsign-ops-phase3.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hived encodes HIVE as "STEEM" and HBD as "SBD" — the 2020 rebrand renamed the tokens but not their serialization. These tests wrote the display spelling, matching a firmware parser that expected the same wrong bytes, so they passed while every resulting signature was rejected on-chain as "missing required active authority". _asset_raw maps too: its callers target the precision and negative-amount checks, and leaving the display spelling there would trip the symbol check first — passing while testing nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test(hive): phase-3 clear-sign op device tests + wire-symbol fix
…enshotted The eleven ops added by firmware #315 have had behavioural coverage since the day they landed — rc15 ran 38 Hive cases, 0 skipped, limit_order_create and limit_order_cancel among them. But SECTIONS stopped at G28, and screenshot_filter() only emits tests whose screenshot hint list is non-empty, so none of them were ever selected for the Phase-1 capture run. rc15 shipped 84 Hive OLED frames covering phase-1 only. That gap matters more here than for most tests: a correct signature over bytes the user was shown something ELSE for is precisely the failure the clear-sign table exists to prevent. Behavioural green says the device parsed and signed; only a frame says it drew "Sells 1.500 HIVE" and not a swapped symbol or a shifted decimal point. Adds G29-G38. Non-empty hints (so they capture) go to the ops that render a confirm screen: both limit-order ops, the six active-tier value ops, posting- tier claim, the zero-amount stop-power-down / remove-delegation pair, and both comment_options cases. The three pure-rejection tests (precision pinning, account_update2 authority reject, truncated bodies) keep empty hints on purpose — a refused op never draws a screen, so a hint there would select a test that can only produce an empty capture. Verified: --screenshot-filter --fw-version=7.15.0 now emits 304 tests and includes all seven new screen-rendering cases.
Verified against the rc16 capture (run 29796178504): the three frames it produced were all the idle home screen (195 ink px vs ~1100 for a real confirm). Every case in the test is _assert_ops_fails, so the device refuses before rendering — the capture could only ever be empty frames dressed up as visual proof, which is the exact failure Gate-3 exists to catch. The per-beneficiary confirm screens are real and are captured by G35, whose final case signs a two-beneficiary payout (14 frames, ink 650-1250).
…capture
Osmosis had NO device tests — the confirm screens were covered only by
host-side unit tests of the formatter in isolation. That is the wrong gap to
have open in 7.15.0, because 7.15.0 changed how every Osmosis amount is
DRAWN: fsm_msg_osmosis.h rendered with atof() + "%.6f", and a float carries
~7 significant decimal digits, so a large transfer displayed rounded on the
screen the user approves —
123456789123456 uosmo shown 123456792.000000 OSMO
actual 123456789.123456 OSMO
The signature was always over the correct amount; the error lived entirely on
the display, which is the half a hardware wallet exists to get right. It now
formats through bn_format_uint64 in integer math, exact at any magnitude.
Adds test_msg_osmosis_signtx.py (6 tests) and SECTIONS group P so four of
them are captured by screenshot_filter(): the baseline send, the
beyond-float-precision amount, a sub-unit amount (500 uosmo = 0.000500 OSMO,
must not collapse), and an unknown denom (shown as raw base units — the
device does not guess a precision). The frame is the evidence; a test that
only asserts "it signed" cannot prove what was drawn.
The two without screenshot hints are behavioural rather than visual: the
amount must be committed to the digest (two sends differing only in amount
give different signatures, so the screen is bound to what is signed), and
signing must be deterministic per RFC6979.
Scope: pyk's osmosis_sign_tx implements osmosis-sdk/MsgSend only. The
delegate/undelegate/LP/swap/IBC screens share this formatter but are not
reachable until the client learns those message types.
Verified: report renders 19 sections / P1-P6 present, filter selects the four
screenshot tests.
Adding the first Osmosis device tests immediately failed with "Unsupported denomination: uosmo". The cause is that osmosis_sign_tx's MsgSend branch had never executed: 1. It whitelisted 'uatom' — the COSMOS denom. Osmosis's native denom is uosmo, so signing a plain OSMO transfer was impossible. 2. It never forwarded the denom, though OsmosisMsgSend carries one. The firmware needs it to decide whether to scale (uosmo -> OSMO) or show raw base units. 3. It assigned amount=int(...) to OsmosisMsgSend.amount, which is a STRING field — that alone would have raised for uatom too. Bug 3 proves the path never ran; bugs 1 and 2 are why it went unnoticed. No denom whitelist belongs on the client: the firmware already handles any denom, scaling uosmo and showing everything else as raw base units precisely so it never guesses a precision it does not know.
The hardcoded fixture was a cosmos1 address with the prefix swapped to osmo1, so its bech32 checksum was invalid. osmosis_signTxUpdateMsgSend bech32-decodes to_address and returns false on failure, which surfaces as the opaque 'Failed to include send message in transaction' — a device-side syntax error that reads like a firmware bug and was mine. Asking the device for its own address also makes these genuine self-sends.
It is decorated @field('address'), so .address on the result raised AttributeError. Matches how test_msg_cosmos_getaddress uses its equivalent.
…does not display Review found that removing the old denom whitelist opened a real display/signature divergence, and that my test certified it as correct. osmosis_signTxUpdateMsgSend takes only (amount, to_address) and hardcodes "denom":"uosmo" into the amino JSON it hashes; fsm_msgOsmosisMsgAck renders whatever denom arrived. So a uatom send DISPLAYS "1500000 uatom" and SIGNS 1500000 uosmo. Exploitable in the large: a big number of some worthless ibc/... token on screen, a big number of OSMO in the signature. osmosis_signTxUpdateMsgDelegate already takes a denom — MsgSend is the outlier, and this is a pre-existing firmware bug my client change made reachable. Fenced client-side until the firmware serializer accepts a denom. The denom is still forwarded, since the firmware needs it to decide whether to scale and the fence is the temporary half. test_osmosis_send_unknown_denom_shown_raw asserted only len(signature)==64, so it PASSED against that divergence and published it as intended behaviour — worse than no test. Replaced with one that asserts the refusal, and documents the check to write once the firmware is fixed: otherwise-identical uosmo and uatom transactions must produce DIFFERENT signatures. Also corrects two overstatements the same review caught: - "exact at any magnitude" was false. osmosis_formatAmount converts with an unchecked strtoull(), so it is exact only for a canonical decimal uint64. strtoull saturates past UINT64_MAX and accepts whitespace, a sign and 0x — "18446744073709551616" and "-1" both display as 18446744073.709551615 OSMO while the original string is hashed. Same divergence, different disguise; needs a firmware-side canonical-range check. - G36 claimed enforcement of the extension cap, the 1-8 count bound and per-weight range. The mapped test asserts only unsorted, duplicate and over-100%. Narrowed to what it actually proves. P4 no longer carries a screenshot hint: the refusal happens client-side, so there is no frame to capture.
BitHighlander
changed the title
BitHighlander
marked this pull request as ready for review
July 25, 2026 15:42
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.
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 three review-test commits that previously had no PR. No history was rewritten;
reconcile/upstream-syncwas fast-forwarded.Contents
RC18 security boundary
persist=Trueuntil authenticated persistent storage is available.KK_EXPECT_PERSIST_REJECTED=1only in the firmware repository’s exact-image integration job, so this Python PR remains compatible with the older currently published emulator.Dependency provenance
device-protocoluses the canonical organization URL:https://github.com/keepkey/device-protocol.gite31cddfe7f5c72c983d06a889ac7db649b9811dfup/release-protocol)Merging remains gated on #112 landing in
device-protocolmaster. After that human-reviewed merge, this PR must repin to the resulting master commit, confirm the generated bindings remain byte-identical, and rerun the validation.Validation on current head
python -m py_compile keepkeylib/*.py: passReview and merge gates