Skip to content

fix(core): ignore unexpected messages in backup flow during setup#6650

Merged
romanz merged 4 commits intomainfrom
romanz/keep-backup
Apr 7, 2026
Merged

fix(core): ignore unexpected messages in backup flow during setup#6650
romanz merged 4 commits intomainfrom
romanz/keep-backup

Conversation

@romanz
Copy link
Copy Markdown
Contributor

@romanz romanz commented Mar 25, 2026

Similar to #6483 (backup should not fail due to unexpected messages from host), but for ResetDevice-based backup, which is being used in trezor/trezor-suite#25084.

Related to #6348.

CC: @STew790

This PR doesn't solve I/O error handling (i.e. ButtonRequest write errors due to dead host) -> should be done in #6651.

TODOs:

  • improve cancellation test coverage

Note to QA:

Please test on both v1 & THP (e.g. TS5 & TS7):

  • run trezorctl device setup -b single (single-share SLIP-39 wallet).
  • exit trezorctl with Ctrl+C when the first mnemonic words are shown.
  • it should fail with:
Please confirm action on your Trezor device.
^CFailed to end session: InProgress: Backup in progress

Aborted!
  • try to run trezorctl ping 123
    • the backup flow MUST NOT be interrupted.
    • trezorctl should fail with:
Failed to connect: UnexpectedMessageError (Expected Features but Trezor sent <Failure: {'code': <FailureType.InProgress: 19>, 'message': 'Backup in progress'}>)
  • complete the backup and the quiz - the flow should succeed even with disconnected host.
    • it is expected to show "Your Trezor is having trouble communicating with your connected device." warning after the backup succeeds.
    • it can be dismissed by calling trezorctl ping 123
    • the device should be correctly initialized.

@romanz romanz self-assigned this Mar 25, 2026
@romanz romanz added the core Trezor Core firmware. Runs on Trezor Model T and Safe models. label Mar 25, 2026
@trezor-bot trezor-bot Bot added this to Firmware Mar 25, 2026
@github-project-automation github-project-automation Bot moved this to 🔎 Needs review in Firmware Mar 25, 2026
@romanz romanz moved this from 🔎 Needs review to 🏃‍♀️ In progress in Firmware Mar 25, 2026
@coderabbitai

This comment has been minimized.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2026

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)
Translations

cs main(all)

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

de main(all)

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

es main(all)

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

fr main(all)

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

pt main(all)

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

Latest CI run: 24092783972

@romanz romanz force-pushed the romanz/keep-backup branch 3 times, most recently from 3419d74 to 111854f Compare April 1, 2026 16:31
@romanz romanz moved this from 🏃‍♀️ In progress to 🔎 Needs review in Firmware Apr 1, 2026
@romanz romanz marked this pull request as ready for review April 1, 2026 16:31
@romanz romanz requested a review from obrusvit as a code owner April 1, 2026 16:31
@romanz romanz added the translations Put this label on a PR to run tests in all languages label Apr 1, 2026
@romanz romanz marked this pull request as draft April 1, 2026 16:40
@romanz romanz marked this pull request as ready for review April 1, 2026 17:18
@romanz
Copy link
Copy Markdown
Contributor Author

romanz commented Apr 2, 2026

Rebasing to resolve a conflict.

@romanz romanz force-pushed the romanz/keep-backup branch from 09aa2c4 to a331f07 Compare April 2, 2026 09:09
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/device_tests/reset_recovery/test_reset_bip39_t2.py`:
- Around line 38-41: The FLOW_ADAPTERS entry using try_to_cancel currently skips
{"backup_device", "setup_device", "success_backup"} but misses the
Delizia-specific ButtonRequest name "confirm_setup_device", causing a Cancel to
be injected during setup; update the try_to_cancel call in FLOW_ADAPTERS to
include "confirm_setup_device" in the skipped set (alongside "backup_device",
"setup_device", and "success_backup") so cancellations are ignored for that
setup request as intended.
🪄 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: 97bee016-947c-4897-80e4-47730471e0c8

📥 Commits

Reviewing files that changed from the base of the PR and between 111854f and a331f07.

📒 Files selected for processing (7)
  • core/src/apps/management/reset_device/__init__.py
  • tests/device_tests/reset_recovery/test_reset_bip39_t2.py
  • tests/device_tests/reset_recovery/test_reset_slip39_advanced.py
  • tests/device_tests/reset_recovery/test_reset_slip39_basic.py
  • tests/device_tests/test_msg_backup_device.py
  • tests/input_flows.py
  • tests/ui_tests/fixtures.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • core/src/apps/management/reset_device/init.py
  • tests/device_tests/reset_recovery/test_reset_slip39_advanced.py
  • tests/device_tests/reset_recovery/test_reset_slip39_basic.py

Comment thread tests/device_tests/reset_recovery/test_reset_bip39_t2.py
@romanz romanz requested a review from PrisionMike April 2, 2026 10:04
@romanz romanz force-pushed the romanz/keep-backup branch from 0204e18 to 2d11926 Compare April 2, 2026 19:46
@romanz
Copy link
Copy Markdown
Contributor Author

romanz commented Apr 2, 2026

Squashed.

@romanz romanz requested a review from mmilata April 7, 2026 14:42
Copy link
Copy Markdown
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice tests

Comment thread tests/input_flows.py Outdated
return flow()


def try_to_cancel(skip_cancel: set[str] | None = None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - fixed and rebased:

1:  9d5556ffa5 ! 1:  dc41f5f3fc chore(core): move backup flow adapters to `tests.input_flows`
    @@ tests/input_flows.py: class InputFlowCancelBrightness(InputFlowBase):
     +    return flow()
     +
     +
    -+def try_to_cancel(skip_cancel: set[str] | None = None):
    ++def try_to_cancel(skip_cancel: set[str] | None = None) -> FlowAdapter:
     +    BACKUP_IN_PROGRESS = messages.Failure(
     +        code=messages.FailureType.InProgress,
     +        message="Backup in progress",
2:  f56509c58b = 2:  77db2ff1b3 fix(core): ignore unexpected messages in backup flow during setup
3:  c86a55a1f9 = 3:  5a94a97682 test(core): test backup cancellation during `ResetDevice`
4:  2d119266fa = 4:  ed22f86ce7 chore(core): update UI fixtures

romanz added 4 commits April 7, 2026 18:32
To be re-used in other tests as well.

[no changelog]
Changelog entry will be added after #6348 is fully resolved.

[no changelog]
Previously, backup cancellation was tested only during `BackupDevice`.

[no changelog]
@romanz romanz force-pushed the romanz/keep-backup branch from 2d11926 to ed22f86 Compare April 7, 2026 16:35
@romanz romanz merged commit 01731b7 into main Apr 7, 2026
165 checks passed
@romanz romanz deleted the romanz/keep-backup branch April 7, 2026 17:34
@github-project-automation github-project-automation Bot moved this from 🔎 Needs review to 🤝 Needs QA in Firmware Apr 7, 2026
@zktex zktex linked an issue Apr 16, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Trezor Core firmware. Runs on Trezor Model T and Safe models. translations Put this label on a PR to run tests in all languages

Projects

Status: 🤝 Needs QA

Development

Successfully merging this pull request may close these issues.

TS7 – Backup failure scenarios

3 participants