From a9a0f22cf4346f2d4b44c88dec749791dd42ea89 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 18 Aug 2026 00:08:17 -0600 Subject: [PATCH] fix(confirm): announce the Cut Off screen's hold on the wire 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 --- lib/board/confirm_sm.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/board/confirm_sm.c b/lib/board/confirm_sm.c index b27a3222e..b12ab3fe7 100644 --- a/lib/board/confirm_sm.c +++ b/lib/board/confirm_sm.c @@ -405,10 +405,23 @@ static bool confirm_helper(const char* request_title, const char* request_body, !confirm_body_fits(request_body, body_width); if (truncated || render_incomplete) { - /* No second ButtonRequest is written: the host already sent one and its - * ButtonAck armed button_request_acked, which stays armed for the body - * screen below. The wire dialogue is unchanged; only the number of holds - * is not. */ + /* INVARIANT: one required hold, one ButtonRequest. + * + * This used to write no second request, on the reasoning that the host had + * already sent one and its ButtonAck left button_request_acked armed for + * the body screen below -- "the wire dialogue is unchanged; only the number + * of holds is not". That is exactly the defect. The device asked for two + * physical confirmations while announcing one, so a host that satisfied + * every request it was told about still waited forever for a response that + * needed a hold it never heard about. A human holding twice never notices; + * any automated or auto-approving host deadlocks. + * + * The request below belongs to the BODY screen, not to this warning: the + * caller's original ButtonRequest was written before confirm_helper() ran + * and is answered by the Cut Off screen, which is the first one shown. + * Clearing button_request_acked is the load-bearing half -- without it + * confirm_screen() would accept a press that arrived for the previous + * request. */ if (!confirm_screen("Cut Off", "This text is too long for the screen. Only part " "of it is shown. Hold to view it anyway.", @@ -416,6 +429,13 @@ static bool confirm_helper(const char* request_title, const char* request_body, immediate)) { return false; } + + ButtonRequest cut_off_ack; + memset(&cut_off_ack, 0, sizeof(cut_off_ack)); + cut_off_ack.has_code = true; + cut_off_ack.code = ButtonRequestType_ButtonRequest_Other; + button_request_acked = false; + msg_write(MessageType_MessageType_ButtonRequest, &cut_off_ack); } return confirm_screen(request_title, request_body, layout_notification_func,