trezorlib: support receiving piggybacked ACKs

[mmilata]

Fixes #6500, related to #6315. Current THP implementation on device always sends standalone ACKs but #6676 will opportunistically omit them.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: edfcfd19-03ee-4e38-95a0-22bfe0e5dc1b

📥 Commits

Reviewing files that changed from the base of the PR and between aa12d7c and d6f54f5.

📒 Files selected for processing (2)

• python/src/trezorlib/thp/channel.py
• python/src/trezorlib/thp/control_byte.py

🚧 Files skipped from review as they are similar to previous changes (2)

• python/src/trezorlib/thp/control_byte.py
• python/src/trezorlib/thp/channel.py

---

Walkthrough

Channel adds a _next_message buffer to hold a piggybacked ACK (an ACK control bit validated against the expected sequence bit) and defer delivery until the next _read. _read_ack now requires expected_seq_bit to be non-None, computes ack_bit_ok once, accepts standalone ACKs with empty data, warns and skips ACKs that carry non-empty data, and when piggybacking is allowed and ack_bit_ok is true, stores the message into self._next_message (asserting it was empty). _read returns and clears _next_message immediately before reading the transport. get_ack_bit() now returns None only for control bytes that are neither ACK nor data packets.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Host as Host
    participant Channel as Channel\n(thp.Channel)
    participant Device as Device/Transport

    Host->>Channel: _read()
    Channel->>Channel: if _next_message present?
    alt buffered message present
        Channel-->>Host: return buffered message (clears _next_message)
    else no buffered message
        Channel->>Device: read_from_wire()
        Device-->>Channel: message (ctrl byte with ACK bit + payload)
        Channel->>Channel: _read_ack(message) → compute ack_bit_ok
        alt standalone ACK (data is empty)
            Channel-->>Host: return ACK
        else piggybacked ACK and ack_bit_ok true
            Channel->>Channel: store message in _next_message
            Channel-->>Host: continue (next _read will return buffered message)
        else ACK invalid or carries unexpected data
            Channel->>Channel: log warning & skip
            Channel->>Device: read_from_wire() (retry)
        end
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding support for receiving piggybacked ACKs in trezorlib.
Description check	✅ Passed	The description references linked issues (#6500, #6315) and provides context about the change, but lacks development status setup details from the template.
Linked Issues check	✅ Passed	The code changes implement device→host ACK piggybacking support by modifying the Channel's _read method to buffer piggybacked ACKs and extract ACK bits from data frames, directly addressing #6500's objectives.
Out of Scope Changes check	✅ Passed	All changes are focused on THP ACK piggybacking support: buffering piggybacked ACKs in Channel and updating ACK bit extraction logic, with no extraneous modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch mmilata/python-piggybacking-receive

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

en main(all)

model	device_test	click_test	persistence_test
T2T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)	test(all) main(all)

Latest CI run: 24426627820

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

python/src/trezorlib/thp/channel.py (1)

470-470: Consider replacing assertion with explicit error handling.

The assert self._next_message is None will raise AssertionError if a previous piggybacked message wasn't consumed. While this shouldn't happen under normal operation, an explicit check with a descriptive error would be more robust and informative in production (where assertions may be disabled with -O).

♻️ Proposed defensive check

             elif self.is_ack_piggybacking_allowed and ack_bit_ok:
-                assert self._next_message is None
+                if self._next_message is not None:
+                    LOG.error("Buffered message not consumed before new piggybacked ACK")
+                    raise ProtocolError("Unexpected piggybacked ACK: previous message not consumed")
                 # process ACK bit, return message in next _read
                 self._next_message = message

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/src/trezorlib/thp/channel.py` at line 470, Replace the bare assertion
on self._next_message with an explicit runtime check that raises a clear
exception (e.g., RuntimeError or ValueError) when a previous piggybacked message
is still present; locate the assertion `assert self._next_message is None` in
the Channel handling code and change it to an if-statement that raises an error
with a descriptive message like "Unexpected leftover piggybacked message:
_next_message not consumed" (optionally include the value of self._next_message)
so the failure is informative and not skipped when Python is run with
optimizations.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/src/trezorlib/thp/channel.py`:
- Around line 466-480: The ACK handling can wrongly buffer malformed ACKs that
carry data: inside the ACK processing block (where message.is_ack(), ack_bit_ok,
is_ack_piggybacking_allowed and self._next_message are used) explicitly detect
ACKs that have non-empty data (message.is_ack() and len(message.data) != 0) and
treat them as invalid—log an error/warning and continue instead of entering the
piggybacking branch; ensure you do not assign to self._next_message for these
malformed ACKs and preserve existing checks for ack_bit_ok when deciding to
continue or return.

---

Nitpick comments:
In `@python/src/trezorlib/thp/channel.py`:
- Line 470: Replace the bare assertion on self._next_message with an explicit
runtime check that raises a clear exception (e.g., RuntimeError or ValueError)
when a previous piggybacked message is still present; locate the assertion
`assert self._next_message is None` in the Channel handling code and change it
to an if-statement that raises an error with a descriptive message like
"Unexpected leftover piggybacked message: _next_message not consumed"
(optionally include the value of self._next_message) so the failure is
informative and not skipped when Python is run with optimizations.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2ceb8610-55a1-49e3-9cf2-bc9fe8cff26b

📥 Commits

Reviewing files that changed from the base of the PR and between 7bbda7b and df1ce4d.

📒 Files selected for processing (2)

• python/src/trezorlib/thp/channel.py
• python/src/trezorlib/thp/control_byte.py

[mmilata]

Removed is_handshake because is_data is true for handshake messages.

[romanz]

LGTM, thanks!