fix(confirm): announce the Cut Off screen's hold on the wire - #482
Merged
Conversation
One required hold, one ButtonRequest. The Cut Off path broke that invariant. When a confirmation body does not fit, confirm_helper() shows a warning screen and then the body -- two physical holds -- while writing only the ButtonRequest the caller had already sent. The old comment stated the reasoning and, read back, states the bug: "the wire dialogue is unchanged; only the number of holds is not." So a host could satisfy every ButtonRequest it was told about and still wait forever for a response that needed a hold it never heard about. A person holding the button twice never notices. Any automated or auto-approving host deadlocks, which is what python-integration-tests has done on every run since this branch introduced the Cut Off screen: the device sent four ButtonRequests, the client acked all four, and the device sat on a fifth screen it had never announced. The fix writes a ButtonRequest for the body screen once the warning is answered, and clears button_request_acked first. Clearing it is the load-bearing half: without it confirm_screen() accepts a press that arrived for the previous request, which is how two holds collapsed into one announcement. Proven with the tests UNCHANGED. No acknowledgement was added anywhere, because there was nothing to acknowledge -- the screen did not exist on the wire. Same pinned suite, same test code, only the firmware differs: before 3 failed, 394 passed, 70 skipped in 139.51s after 1 failed, 396 passed, 70 skipped in 14.47s test_eos_signtx_updateauth and test_thorchain_sign_tx both pass. The 10x wall-clock drop is the two 60-second deadlock timeouts disappearing. This is a protocol-correctness fix, NOT the overflowing-confirmation UX redesign in #480. That PR replaces the Cut Off state with hold-to-scroll and belongs on alpha with its own CI and hardware round. This patch changes no screen and no flow: the device already demanded the second hold, and the wire simply never said so. The one remaining failure is unrelated and no longer hidden behind this bug: test_sign_with_thorchain_memo asserts an XRP memo field this firmware's protocol does not have. Filed separately. Refs #428 #466
BitHighlander
added a commit
that referenced
this pull request
Aug 18, 2026
Advance the dress-rehearsal pin from c75fbd5 to 6c4ad17, the current head of keepkey/python-keepkey#214. The additional commit re-gates the XRP THORChain memo test without weakening its assertion. The host-side memo transport gap is tracked separately as keepkey/keepkey-vault#422; firmware never receives the memo. With #482 plus this pin, the expected integration result is zero failures, with XRP memo support explicitly deferred rather than normalized as missing.
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Protocol-correctness fix. One file, +24/−4. Not a UX change.
The invariant, and how it was broken
One required hold, one
ButtonRequest.When a confirmation body does not fit,
confirm_helper()shows a "Cut Off" warning and then the body — two physical holds — while writing only theButtonRequestthe caller had already sent. The old comment stated the reasoning and, read back, states the bug:A host can therefore satisfy every request it was told about and still wait forever for a response that needs a hold it never heard about. A person holding the button twice never notices. Any automated or auto-approving host deadlocks.
That is exactly what
python-integration-testshas been doing on every run since this branch introduced the Cut Off screen. Instrumented, the device sends fourButtonRequests, the client acks all four, and the device then sits on a fifth screen it never announced:The fix
Write a
ButtonRequestfor the body screen once the warning is answered, and clearbutton_request_ackedfirst. Clearing it is the load-bearing half — without itconfirm_screen()accepts a press that arrived for the previous request, which is precisely how two holds collapsed into one announcement.Evidence: tests UNCHANGED
No acknowledgement was added anywhere, because there was nothing to acknowledge — the screen did not exist on the wire. Same pinned suite, same test code, only the firmware differs:
3 failed, 394 passed, 70 skippedin 139.51s1 failed, 396 passed, 70 skippedin 14.47stest_eos_signtx_updateauthandtest_thorchain_sign_txboth pass. The 10× wall-clock drop is the two 60-second deadlock timeouts disappearing.(Measured with
deps/python-keepkeyatc75fbd5— the consolidated #214 head. This branch still pins81e581f, so CI here will show the older baseline minus the two deadlocks; the repin is a separate PR.)Explicitly not #480
#480 replaces the Cut Off state with a hold-to-scroll state machine — a redesign of the overflowing-confirmation UX, with a new
SCROLLINGstate, timer-driven paging and altered approval gating. That belongs onalphawith its own CI, adversarial review and hardware round.This patch changes no screen and no flow. The device already demanded the second hold; the wire simply never said so. If #480 later removes the Cut Off state entirely, this becomes moot — but the current release still needs its own state machine to be internally consistent.
Not fixed here
The remaining failure is unrelated and was previously hidden behind this bug:
test_sign_with_thorchain_memoasserts an XRP memo field this firmware's protocol does not define. Filed separately — it deserves judging on its own merits.Verified
ARM cross-compile in the pinned builder image; cppcheck clean under CI's exact invocation (0 findings); clang-format clean.