Introduce N4W1 backup/recovery flows#6799
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughAdds N4W1 as an alternative backup/recovery method across firmware, emulator, UI/layouts, management apps, and tests. CI matrices and artifact naming/handling were extended to produce and consume -n4w1 debuglink artifacts and to split device-test steps when N4W1 is enabled. Management/reset/recovery layouts introduce N4W1 read/write handlers with retry/confirmation and incremental validation logic. Test suites and input-flow helpers are parameterized with a backup_method fixture and updated to drive N4W1 debuglink read/write helper sequences in host-side test flows. Sequence Diagram(s)sequenceDiagram
participant Device
participant N4W1Tag as N4W1 Tag
participant Layout as Recovery Layout
participant UI as UI Handler
Device->>Layout: start recovery (method=N4W1)
Layout->>N4W1Tag: read("mnemonic")
alt empty or damaged
N4W1Tag-->>Layout: null or corrupted
Layout->>UI: show danger retry warning
UI-->>Layout: user confirms retry
loop retry
Layout->>N4W1Tag: read("mnemonic")
end
else valid share
N4W1Tag-->>Layout: return encoded mnemonic
Layout->>Layout: decode & incremental validate
Layout->>UI: show progress / result
UI-->>Layout: user proceeds
Layout->>Device: supply share to recovery flow
end
sequenceDiagram
participant Device
participant N4W1Tag as N4W1 Tag
participant Layout as Reset Layout
participant UI as UI Handler
Device->>Layout: start backup (method=N4W1)
loop for each share
Layout->>N4W1Tag: read("mnemonic")
alt tag contains data
N4W1Tag-->>Layout: non-empty
Layout->>UI: show danger overwrite warning
UI-->>Layout: user confirms retry
Layout->>N4W1Tag: read("mnemonic") -- wait for empty tag
else tag empty
Layout->>Layout: encode share -> blob
Layout->>N4W1Tag: write("mnemonic", blob)
N4W1Tag-->>Layout: write ack
Layout->>UI: show hold/footer & success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
4353da1 to
33e6532
Compare
romanz
force-pushed
the
romanz/2604/show-info
branch
from
099bfb0 to
14265bc
Compare
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
2 times, most recently
from
8fe2388 to
94b032e
Compare
Merged
romanz
force-pushed
the
romanz/2604/n4w1-prep
branch
2 times, most recently
from
ac4c601 to
1df8713
Compare
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
94b032e to
233d9b9
Compare
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
11 times, most recently
from
671ea4e to
9af64ff
Compare
romanz
added
the
translations
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/apps/management/reset_device/layout.py (1)
237-243: Finish the missing N4W1 intro/safety screens.
intro()is still empty andbackup()skips the warning shown by_DisplayBackup, so this path drops straight into writing secret shares to tags. I'd close those TODOs before enabling the flow outside demo builds.Do you want me to sketch a minimal version that reuses the existing backup intro/warning layouts?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/apps/management/reset_device/layout.py` around lines 237 - 243, The intro() and backup() stubs in ResetDevice.layout must present the same N4W1 intro and safety screens used elsewhere instead of being no-ops; implement intro(self, num_of_words) to render the existing backup/introduction text/UI (reuse the layout or components used by the prior backup flow), and update backup(self, iter_shares) to invoke the existing _DisplayBackup warning/confirmation sequence before proceeding to write shares (so the user must acknowledge safety). Locate and call the same helper/layout functions or components used by the current backup flow (e.g., the backup intro screen and _DisplayBackup warning) inside ResetDevice.layout.intro and ResetDevice.layout.backup, preserving the num_of_words and iter_shares parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 283-290: The current warning in choose_handler logs "Unsupported
backup method: None" for the intentional default path; change the condition so
that the warning is only emitted for explicit unsupported methods, not when
method is None. Concretely, update the conditional that checks method (in
function choose_handler) to first ensure method is not None and not
BackupMethod.Display before entering the __debug__ logging branch and calling
trezor.log.warning, so None is treated as the silent placeholder.
---
Nitpick comments:
In `@core/src/apps/management/reset_device/layout.py`:
- Around line 237-243: The intro() and backup() stubs in ResetDevice.layout must
present the same N4W1 intro and safety screens used elsewhere instead of being
no-ops; implement intro(self, num_of_words) to render the existing
backup/introduction text/UI (reuse the layout or components used by the prior
backup flow), and update backup(self, iter_shares) to invoke the existing
_DisplayBackup warning/confirmation sequence before proceeding to write shares
(so the user must acknowledge safety). Locate and call the same helper/layout
functions or components used by the current backup flow (e.g., the backup intro
screen and _DisplayBackup warning) inside ResetDevice.layout.intro and
ResetDevice.layout.backup, preserving the num_of_words and iter_shares
parameters.
🪄 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: 758731a6-ed4f-4b4c-b9f9-5b278cb2b8c9
⛔ Files ignored due to path filters (1)
core/embed/rust/src/translations/generated/translated_string.rsis excluded by!**/generated/**
📒 Files selected for processing (9)
.github/workflows/core.ymlcore/embed/rust/librust_qstr.hcore/mocks/trezortranslate_keys.pyicore/src/apps/management/recovery_device/layout.pycore/src/apps/management/reset_device/layout.pycore/translations/en.jsoncore/translations/order.jsoncore/translations/signatures.jsontests/device_tests/reset_recovery/test_recovery_slip39_basic.py
✅ Files skipped from review due to trivial changes (5)
- core/embed/rust/librust_qstr.h
- core/translations/signatures.json
- core/translations/en.json
- core/translations/order.json
- core/mocks/trezortranslate_keys.pyi
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/device_tests/reset_recovery/test_recovery_slip39_basic.py
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
1f936e1 to
c5d047b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/core.yml (1)
305-356:⚠️ Potential issue | 🟠 MajorKeep N4W1 coverage separate from the default T3W1 report.
This include row adds a second
T3W1/universal/noasanbinary withN4W1=1, but the coverage pipeline still reuses only the standardcore-emu-T3W1-universal-debuglink-noasanbuild for.isources. That mixes coverage from two different compiled variants and can skew or break the final T3W1 coverage report. Either skipupload-coverageformatrix.n4w1for now, or make the coverage artifacts/report job variant-aware (-n4w1sources + separate merge/reporting).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/core.yml around lines 305 - 356, The pipeline is mixing coverage from the N4W1 variant into the standard T3W1 report because matrix.n4w1 produces a second core-emu artifact but upload-coverage always processes the default build; either prevent uploading coverage for N4W1 by adding a condition to the upload-coverage step (if: ${{ !matrix.n4w1 }}) or make the coverage flow variant-aware by including the n4w1 suffix in the core-emu download/upload artifact names (core-emu-${{ matrix.model }}-${{ matrix.coins }}-debuglink-${{ matrix.asan }}${{ matrix.n4w1 && '-n4w1' || '' }}) and updating the upload-coverage job to merge/report per-variant artifacts instead of a single default artifact.
♻️ Duplicate comments (1)
core/src/apps/management/recovery_device/layout.py (1)
283-291:⚠️ Potential issue | 🟡 MinorDon't warn on the intentional
method=Nonepath.When
USE_N4W1is off, this still logsUnsupported backup method: Nonefor the normal default flow. Limit the warning to explicit non-Displayvalues.Proposed fix
- if method is not BackupMethod.Display and __debug__: + if method not in (None, BackupMethod.Display) and __debug__: from trezor import log log.warning(__name__, "Unsupported backup method: %s", method)Based on learnings,
method=Nonepassed tochoose_backup_method()is intentionally distinct fromBackupMethod.Display, and debug-mode unsupported-method warnings should be suppressed forNoneas well as forBackupMethod.Display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 291, The warning in choose_handler currently triggers for method==None; change the condition so the debug warning only runs when method is neither None nor BackupMethod.Display: update the if to check "method is not None and method is not BackupMethod.Display and __debug__" (keep the import of BackupMethod and return of _DisplayHandler unchanged) so intentional None flows do not log an unsupported-method warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/apps/management/reset_device/layout.py`:
- Around line 241-259: The branch in _backup_share misinterprets ShareInfo.index
as a global position; change backup to materialize iter_shares into a flattened
list (so you can compute global_index and total_shares across groups) and pass
those global values into _backup_share (or adjust _backup_share signature to
accept global_index and total_shares) so the description/button decision uses
global_index == 0 and global_index == total_shares - 1; update any callers
(e.g., slip39_advanced_show_and_confirm_shares) to rely on the flattened order
instead of group-local ShareInfo.index/num_of_shares.
In `@core/src/trezor/ui/layouts/eckhart/recovery.py`:
- Around line 224-236: The choose_method function currently indexes a two-item
tuple with the result of trezorui_api.select_word(), which can return 0..3 and
will raise IndexError for values >=2; replace the tuple lookup with an explicit
conditional mapping in choose_method: call trezorui_api.select_word(...) as
before, then if index == 0 return BackupMethod.N4W1, elif index == 1 return
BackupMethod.Display, elif index in (2, 3) return a safe default (e.g.,
BackupMethod.Display) or raise a clear ValueError for unexpected slots;
reference the choose_method function, trezorui_api.select_word, BackupMethod and
the TR.backup__type_* symbols when making the change.
---
Outside diff comments:
In @.github/workflows/core.yml:
- Around line 305-356: The pipeline is mixing coverage from the N4W1 variant
into the standard T3W1 report because matrix.n4w1 produces a second core-emu
artifact but upload-coverage always processes the default build; either prevent
uploading coverage for N4W1 by adding a condition to the upload-coverage step
(if: ${{ !matrix.n4w1 }}) or make the coverage flow variant-aware by including
the n4w1 suffix in the core-emu download/upload artifact names (core-emu-${{
matrix.model }}-${{ matrix.coins }}-debuglink-${{ matrix.asan }}${{ matrix.n4w1
&& '-n4w1' || '' }}) and updating the upload-coverage job to merge/report
per-variant artifacts instead of a single default artifact.
---
Duplicate comments:
In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 283-291: The warning in choose_handler currently triggers for
method==None; change the condition so the debug warning only runs when method is
neither None nor BackupMethod.Display: update the if to check "method is not
None and method is not BackupMethod.Display and __debug__" (keep the import of
BackupMethod and return of _DisplayHandler unchanged) so intentional None flows
do not log an unsupported-method warning.
🪄 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: 71009308-ea25-4473-9716-294aed30b1e5
⛔ Files ignored due to path filters (1)
core/embed/rust/src/translations/generated/translated_string.rsis excluded by!**/generated/**
📒 Files selected for processing (24)
.github/workflows/core.ymlcore/embed/rust/librust_qstr.hcore/mocks/trezortranslate_keys.pyicore/src/apps/management/recovery_device/layout.pycore/src/apps/management/reset_device/layout.pycore/src/trezor/ui/layouts/eckhart/recovery.pycore/translations/en.jsoncore/translations/order.jsoncore/translations/signatures.jsontests/conftest.pytests/device_tests/reset_recovery/test_recovery_bip39_dryrun.pytests/device_tests/reset_recovery/test_recovery_bip39_t2.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.pytests/device_tests/reset_recovery/test_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.pytests/device_tests/reset_recovery/test_reset_recovery_bip39.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_reset_slip39_basic.pytests/device_tests/test_repeated_backup.pytests/input_flows.pytests/input_flows_helpers.pytests/ui_tests/fixtures.json
✅ Files skipped from review due to trivial changes (6)
- tests/device_tests/reset_recovery/test_recovery_bip39_t2.py
- core/translations/signatures.json
- core/translations/en.json
- core/embed/rust/librust_qstr.h
- core/mocks/trezortranslate_keys.pyi
- core/translations/order.json
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.py
- tests/device_tests/reset_recovery/test_reset_recovery_slip39_advanced.py
- tests/device_tests/reset_recovery/test_reset_recovery_bip39.py
- tests/device_tests/reset_recovery/test_recovery_bip39_dryrun.py
- tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced.py
- tests/device_tests/reset_recovery/test_reset_slip39_basic.py
- tests/device_tests/reset_recovery/test_recovery_slip39_basic.py
- tests/input_flows_helpers.py
- tests/device_tests/test_repeated_backup.py
- tests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.py
- tests/input_flows.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/core.yml (1)
334-339: Don't silently drop UI checks on the N4W1 universal/noasan path.For this matrix entry
ACTIONS_DO_UI_TESTis still true, but the N4W1 branch switches totest_emu_multicoreinstead oftest_emu_ui_multicore. That loses the--ui=test --ui-check-missing --do-master-diffcoverage the standard path gets for the same job shape.Possible fix
- name: Run device tests (N4W1) if: ${{ matrix.n4w1 }} # TODO: test N4W1 UI fixtures as well - run: nix-shell --run "uv run make -C core ${{ env.ACTIONS_DO_UI_TEST == 'true' && 'test_emu_multicore' || 'test_emu' }}" + run: nix-shell --run "uv run make -C core ${{ env.ACTIONS_DO_UI_TEST == 'true' && 'test_emu_ui_multicore' || 'test_emu' }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/core.yml around lines 334 - 339, The N4W1 matrix branch incorrectly uses test_emu_multicore instead of the UI-aware target, which drops UI checks when ACTIONS_DO_UI_TEST is true; update the N4W1 run line to mirror the non-N4W1 conditional expression so it selects test_emu_ui_multicore when ACTIONS_DO_UI_TEST is 'true' (i.e., use the same ternary logic with ACTIONS_DO_UI_TEST -> 'test_emu_ui_multicore' || 'test_emu' instead of switching to 'test_emu_multicore'), ensuring the UI flags (--ui=test, --ui-check-missing, --do-master-diff) are preserved for N4W1 runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/core.yml:
- Around line 334-339: The N4W1 matrix branch incorrectly uses
test_emu_multicore instead of the UI-aware target, which drops UI checks when
ACTIONS_DO_UI_TEST is true; update the N4W1 run line to mirror the non-N4W1
conditional expression so it selects test_emu_ui_multicore when
ACTIONS_DO_UI_TEST is 'true' (i.e., use the same ternary logic with
ACTIONS_DO_UI_TEST -> 'test_emu_ui_multicore' || 'test_emu' instead of switching
to 'test_emu_multicore'), ensuring the UI flags (--ui=test, --ui-check-missing,
--do-master-diff) are preserved for N4W1 runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab51a135-d1d8-47a2-b5d1-53e25c5308bb
📒 Files selected for processing (1)
.github/workflows/core.yml
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
b8e841b to
53dcb05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/trezor/ui/layouts/eckhart/recovery.py (1)
224-236:⚠️ Potential issue | 🟠 MajorHandle unexpected
select_word()values explicitly.This still relies on a 2-item tuple lookup, so any unexpected index turns into a generic
IndexError. Given the UI contract here, it should fail deliberately and readably instead.Proposed fix
index = await interact( trezorui_api.select_word( title=title, description=description, words=(TR.backup__type_n4w1, TR.backup__type_wordlist, ""), ), br_name="choose_method", ) - return (BackupMethod.N4W1, BackupMethod.Display)[index] + if index == 0: + return BackupMethod.N4W1 + if index == 1: + return BackupMethod.Display + raise ValueError(f"Unexpected select_word index: {index}")Based on learnings,
trezorui_api.select_word()called fromchoose_methodwill only return 0 or 1 on Eckhart because the empty third word slot is disabled at the UI level. Unexpected return values (index >= 2) should trigger an explicit failure (e.g.,raise ValueError) rather than silently defaulting toBackupMethod.Display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/trezor/ui/layouts/eckhart/recovery.py` around lines 224 - 236, The choose_method function uses the index from trezorui_api.select_word to pick from a two-item tuple, which will raise a generic IndexError for unexpected values; change choose_method (the async function calling trezorui_api.select_word) to validate the returned index explicitly: accept 0 -> BackupMethod.N4W1, 1 -> BackupMethod.Display, and if index is any other value raise a clear ValueError (or custom error) with a descriptive message referencing the unexpected index and the select_word call so failures are deliberate and readable.core/src/apps/management/recovery_device/layout.py (1)
283-290:⚠️ Potential issue | 🟡 MinorSuppress the debug warning for
method=Nonehere as well.
Noneis the intentional default path, so this branch still logsUnsupported backup method: Noneon ordinary recovery calls whenUSE_N4W1is off.Proposed fix
- if method is not BackupMethod.Display and __debug__: + if method not in (None, BackupMethod.Display) and __debug__: from trezor import log log.warning(__name__, "Unsupported backup method: %s", method)Based on learnings,
method=Nonepassed tochoose_backup_method()is intentionally distinct fromBackupMethod.Display.Noneis reserved for a future flow where the user is prompted to choose between supported backup methods interactively. It should NOT be treated as equivalent toBackupMethod.Display, and debug-mode unsupported-method warnings should be suppressed forNone(as well as forBackupMethod.Display).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 290, The debug warning in choose_handler currently fires for method=None; update the condition so it only logs when method is neither None nor BackupMethod.Display (i.e., skip logging for the intentional default None path as well as the Display enum). Modify the branch that checks BackupMethod to require method is not None and method is not BackupMethod.Display before calling log.warning (refer to function choose_handler, symbol BackupMethod, __debug__, and log.warning).
🧹 Nitpick comments (2)
.github/workflows/core.yml (1)
55-69: Consider adding abtconlyN4W1 lane too.All new N4W1 matrix rows are
coins: universal, so CI still won’t build or exercise thebtconlyvariant. If N4W1 is expected to ship on both firmware types, this leaves a blind spot for btconly-only regressions.Based on learnings, when advertising the
Capability.N4W1capability, reviewers should ensure it is added for all firmware types (both Bitcoin-only and universal builds). Do not wrapf.capabilities.append(Capability.N4W1)withutils.BITCOIN_ONLY. The only acceptable conditional guard isutils.USE_N4W1.Also applies to: 133-138, 306-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/core.yml around lines 55 - 69, The CI matrix is missing a btconly N4W1 lane and some code conditionally guards N4W1 with utils.BITCOIN_ONLY; add a matrix row mirroring the existing N4W1 universal entry but with coins: btconly and n4w1: true (e.g., alongside the T3W1 n4w1 entry) so btconly builds are exercised, and remove any conditional guards that append Capability.N4W1 under utils.BITCOIN_ONLY — ensure Capability.N4W1 is only gated by utils.USE_N4W1; apply the same change to the other similar blocks that add N4W1 (the other matrix sections and any code paths that call f.capabilities.append(Capability.N4W1)).tests/input_flows.py (1)
2227-2242: Add an explicit assert for method validation to matchInputFlowSlip39BasicBackup.In
InputFlowSlip39BasicBackup.input_flow_eckhart, there's an explicitassert self.method in (Display, N4W1). Here, the dict lookup implicitly validates valid methods, but an invalid method would raise a crypticKeyErrorinstead of a clearAssertionError.♻️ Suggested fix for consistency
def input_flow_eckhart(self) -> BRGeneratorType: + assert self.method in ( + messages.BackupMethod.Display, + messages.BackupMethod.N4W1, + ) num_screens = { messages.BackupMethod.Display: 10, messages.BackupMethod.N4W1: 8, }[self.method]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/input_flows.py` around lines 2227 - 2242, Add an explicit assertion to validate self.method at the start of input_flow_eckhart so it mirrors InputFlowSlip39BasicBackup: assert self.method in (messages.BackupMethod.Display, messages.BackupMethod.N4W1). This makes method validation explicit and raises an AssertionError with a clear failure mode before using the dict lookup that currently raises a KeyError; place the assert immediately before the num_screens assignment in the input_flow_eckhart method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/apps/management/reset_device/layout.py`:
- Around line 237-246: The intro() method is currently a no-op and backup()
immediately writes shares, skipping the pre-export safety screens; implement
intro() to reuse the existing _DisplayBackup intro/warning flow (e.g., await
self._display_backup.intro(num_of_words) or delegate to the _DisplayBackup.intro
method) and ensure backup() calls await self.intro(num_of_words) (or awaits the
same _DisplayBackup warning) before iterating and calling _backup_share(share)
so the N4W1 path shows the same safety/acknowledgement screens as
_DisplayBackup.
---
Duplicate comments:
In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 283-290: The debug warning in choose_handler currently fires for
method=None; update the condition so it only logs when method is neither None
nor BackupMethod.Display (i.e., skip logging for the intentional default None
path as well as the Display enum). Modify the branch that checks BackupMethod to
require method is not None and method is not BackupMethod.Display before calling
log.warning (refer to function choose_handler, symbol BackupMethod, __debug__,
and log.warning).
In `@core/src/trezor/ui/layouts/eckhart/recovery.py`:
- Around line 224-236: The choose_method function uses the index from
trezorui_api.select_word to pick from a two-item tuple, which will raise a
generic IndexError for unexpected values; change choose_method (the async
function calling trezorui_api.select_word) to validate the returned index
explicitly: accept 0 -> BackupMethod.N4W1, 1 -> BackupMethod.Display, and if
index is any other value raise a clear ValueError (or custom error) with a
descriptive message referencing the unexpected index and the select_word call so
failures are deliberate and readable.
---
Nitpick comments:
In @.github/workflows/core.yml:
- Around line 55-69: The CI matrix is missing a btconly N4W1 lane and some code
conditionally guards N4W1 with utils.BITCOIN_ONLY; add a matrix row mirroring
the existing N4W1 universal entry but with coins: btconly and n4w1: true (e.g.,
alongside the T3W1 n4w1 entry) so btconly builds are exercised, and remove any
conditional guards that append Capability.N4W1 under utils.BITCOIN_ONLY — ensure
Capability.N4W1 is only gated by utils.USE_N4W1; apply the same change to the
other similar blocks that add N4W1 (the other matrix sections and any code paths
that call f.capabilities.append(Capability.N4W1)).
In `@tests/input_flows.py`:
- Around line 2227-2242: Add an explicit assertion to validate self.method at
the start of input_flow_eckhart so it mirrors InputFlowSlip39BasicBackup: assert
self.method in (messages.BackupMethod.Display, messages.BackupMethod.N4W1). This
makes method validation explicit and raises an AssertionError with a clear
failure mode before using the dict lookup that currently raises a KeyError;
place the assert immediately before the num_screens assignment in the
input_flow_eckhart method.
🪄 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: a039282f-6b92-43a3-9da0-5db463dc2c7d
📒 Files selected for processing (19)
.github/workflows/core.ymlcore/src/apps/management/recovery_device/layout.pycore/src/apps/management/reset_device/layout.pycore/src/trezor/ui/layouts/eckhart/recovery.pytests/conftest.pytests/device_tests/reset_recovery/test_recovery_bip39_dryrun.pytests/device_tests/reset_recovery/test_recovery_bip39_t2.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.pytests/device_tests/reset_recovery/test_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.pytests/device_tests/reset_recovery/test_reset_recovery_bip39.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_reset_slip39_basic.pytests/device_tests/test_repeated_backup.pytests/input_flows.pytests/input_flows_helpers.pytests/ui_tests/fixtures.json
✅ Files skipped from review due to trivial changes (3)
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.py
- tests/device_tests/reset_recovery/test_recovery_bip39_t2.py
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/conftest.py
- tests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.py
- tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py
- tests/device_tests/reset_recovery/test_reset_recovery_bip39.py
- tests/device_tests/reset_recovery/test_reset_slip39_basic.py
- tests/device_tests/reset_recovery/test_recovery_slip39_basic.py
Comment on lines
+237
to
+246
| async def intro(self, num_of_words: int | None = None) -> None: | ||
| # TODO: design/copy | ||
| pass | ||
|
|
||
| async def backup(self, iter_shares: Iterable[ShareInfo]) -> None: | ||
| # TODO: warn user about safety | ||
|
|
||
| # backup all shares | ||
| for share in iter_shares: | ||
| await self._backup_share(share) |
There was a problem hiding this comment.
Don’t skip the backup intro/safety screens on the N4W1 path.
intro() is a no-op and backup() jumps straight into writing shares, so this path misses the same pre-export safety acknowledgment that _DisplayBackup shows before exposing the seed.
Possible stopgap
class _N4W1Backup:
async def intro(self, num_of_words: int | None = None) -> None:
- # TODO: design/copy
- pass
+ from trezor.ui.layouts.reset import show_intro_backup
+
+ await show_intro_backup(num_of_words=num_of_words)
async def backup(self, iter_shares: Iterable[ShareInfo]) -> None:
- # TODO: warn user about safety
+ from trezor.ui.layouts.reset import show_warning_backup
+
+ await show_warning_backup()
# backup all shares
for share in iter_shares:
await self._backup_share(share)I can help sketch a minimal patch that reuses the existing intro/warning screens until the N4W1-specific copy lands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/apps/management/reset_device/layout.py` around lines 237 - 246, The
intro() method is currently a no-op and backup() immediately writes shares,
skipping the pre-export safety screens; implement intro() to reuse the existing
_DisplayBackup intro/warning flow (e.g., await
self._display_backup.intro(num_of_words) or delegate to the _DisplayBackup.intro
method) and ensure backup() calls await self.intro(num_of_words) (or awaits the
same _DisplayBackup warning) before iterating and calling _backup_share(share)
so the N4W1 path shows the same safety/acknowledgement screens as
_DisplayBackup.
Contributor
Author
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/src/apps/management/reset_device/layout.py (1)
237-246:⚠️ Potential issue | 🟠 MajorReuse the existing backup disclosure flow before N4W1 writes.
intro()is a no-op andbackup()starts exporting shares immediately, so the N4W1 path skips the intro/safety acknowledgement that_DisplayBackupshows before exposing mnemonic material.Possible stopgap
async def intro(self, num_of_words: int | None = None) -> None: - # TODO: design/copy - pass + from trezor.ui.layouts.reset import show_intro_backup + + await show_intro_backup(num_of_words=num_of_words) async def backup(self, iter_shares: Iterable[ShareInfo]) -> None: - # TODO: warn user about safety + from trezor.ui.layouts.reset import show_warning_backup + + await show_warning_backup() # backup all shares for share in iter_shares: await self._backup_share(share)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/apps/management/reset_device/layout.py` around lines 237 - 246, The intro() method is a no-op and backup() immediately calls _backup_share, which skips the existing _DisplayBackup disclosure/safety acknowledgement used in the N4W1 flow; update intro() to present the same disclosure/acknowledgement flow used by _DisplayBackup (or call the common method/component that shows the copy/safety notice) and modify backup() to ensure it awaits/uses intro() before iterating shares (i.e., call await self.intro(...) or invoke the shared disclosure helper before calling self._backup_share for each ShareInfo) so the safety intro is always shown prior to exposing mnemonic material.core/src/apps/management/recovery_device/layout.py (1)
283-291:⚠️ Potential issue | 🟡 MinorDon't warn on the default
method=Nonepath.In non-N4W1 builds this still logs
Unsupported backup method: Nonefor the ordinary recovery flow. Restrict the warning to explicit non-Noneunsupported values.Proposed patch
- if method is not BackupMethod.Display and __debug__: + if method not in (None, BackupMethod.Display) and __debug__: from trezor import log log.warning(__name__, "Unsupported backup method: %s", method)Based on learnings,
method=Noneis an intentional placeholder and should not trigger unsupported-method warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/apps/management/recovery_device/layout.py` around lines 283 - 291, The warning should not fire for the intentional placeholder value None; update choose_handler so the log.warning inside the __debug__ block only runs when method is not None and not BackupMethod.Display. Locate the choose_handler function (and the log.warning call that references Unsupported backup method) and change the condition to require method is not None in addition to method is not BackupMethod.Display so only explicit unsupported enum values trigger the warning; keep returning _DisplayHandler unchanged.
🧹 Nitpick comments (1)
tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py (1)
38-39: Threadbackup_methodthrough the reset leg too.The new fixture only affects the recovery half of this roundtrip.
reset(session)still uses the default display backup flow, so the N4W1 matrix here never exercises reset-time backup.Proposed patch
-def test_reset_recovery(client: Client, backup_method: BackupMethod): +def test_reset_recovery(client: Client, backup_method: BackupMethod): session = client.get_seedless_session() - mnemonics = reset(session) + mnemonics = reset(session, method=backup_method) session = client.get_session() address_before = btc.get_address(session, "Bitcoin", parse_path("m/44h/0h/0h/0/0"))-def reset(session: Session, strength: int = 128) -> list[str]: +def reset( + session: Session, + strength: int = 128, + method: BackupMethod = BackupMethod.Display, +) -> list[str]: with session.test_ctx as client: - IF = InputFlowSlip39BasicResetRecovery(session) + IF = InputFlowSlip39BasicResetRecovery(session, method) client.set_input_flow(IF.get()) device.setup( session, strength=strength, passphrase_protection=False, pin_protection=False, label="test", backup_type=BackupType.Slip39_Basic, + backup_method=method, entropy_check_count=0, _get_entropy=MOCK_GET_ENTROPY, )Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py` around lines 38 - 39, The test only applies the backup_method fixture to the recovery half; thread the backup_method through the reset leg so reset uses the same backup flow. Update the test_reset_recovery code paths that call client.get_seedless_session() and reset(session) to pass the backup_method fixture into the reset flow (for example by calling reset(session, backup_method=backup_method) or by creating the session with client.get_seedless_session(backup_method=backup_method) so both get_seedless_session and reset() use the same backup_method); do the same change for the other occurrence referenced (around line 52) so both reset and recovery phases exercise the selected backup_method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 283-291: The warning should not fire for the intentional
placeholder value None; update choose_handler so the log.warning inside the
__debug__ block only runs when method is not None and not BackupMethod.Display.
Locate the choose_handler function (and the log.warning call that references
Unsupported backup method) and change the condition to require method is not
None in addition to method is not BackupMethod.Display so only explicit
unsupported enum values trigger the warning; keep returning _DisplayHandler
unchanged.
In `@core/src/apps/management/reset_device/layout.py`:
- Around line 237-246: The intro() method is a no-op and backup() immediately
calls _backup_share, which skips the existing _DisplayBackup disclosure/safety
acknowledgement used in the N4W1 flow; update intro() to present the same
disclosure/acknowledgement flow used by _DisplayBackup (or call the common
method/component that shows the copy/safety notice) and modify backup() to
ensure it awaits/uses intro() before iterating shares (i.e., call await
self.intro(...) or invoke the shared disclosure helper before calling
self._backup_share for each ShareInfo) so the safety intro is always shown prior
to exposing mnemonic material.
---
Nitpick comments:
In `@tests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.py`:
- Around line 38-39: The test only applies the backup_method fixture to the
recovery half; thread the backup_method through the reset leg so reset uses the
same backup flow. Update the test_reset_recovery code paths that call
client.get_seedless_session() and reset(session) to pass the backup_method
fixture into the reset flow (for example by calling reset(session,
backup_method=backup_method) or by creating the session with
client.get_seedless_session(backup_method=backup_method) so both
get_seedless_session and reset() use the same backup_method); do the same change
for the other occurrence referenced (around line 52) so both reset and recovery
phases exercise the selected backup_method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e18c8a3-1132-4528-869b-a022ee1ab303
📒 Files selected for processing (19)
.github/workflows/core.ymlcore/src/apps/management/recovery_device/layout.pycore/src/apps/management/reset_device/layout.pycore/src/trezor/ui/layouts/eckhart/recovery.pytests/conftest.pytests/device_tests/reset_recovery/test_recovery_bip39_dryrun.pytests/device_tests/reset_recovery/test_recovery_bip39_t2.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.pytests/device_tests/reset_recovery/test_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.pytests/device_tests/reset_recovery/test_reset_recovery_bip39.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_advanced.pytests/device_tests/reset_recovery/test_reset_recovery_slip39_basic.pytests/device_tests/reset_recovery/test_reset_slip39_basic.pytests/device_tests/test_repeated_backup.pytests/input_flows.pytests/input_flows_helpers.pytests/ui_tests/fixtures.json
✅ Files skipped from review due to trivial changes (4)
- tests/device_tests/reset_recovery/test_recovery_bip39_t2.py
- tests/device_tests/reset_recovery/test_recovery_bip39_dryrun.py
- tests/device_tests/reset_recovery/test_reset_recovery_slip39_advanced.py
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/device_tests/reset_recovery/test_recovery_slip39_advanced_dryrun.py
- tests/device_tests/reset_recovery/test_reset_recovery_bip39.py
- tests/device_tests/reset_recovery/test_recovery_slip39_basic_dryrun.py
- tests/input_flows_helpers.py
- tests/conftest.py
- tests/input_flows.py
Merged
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
53dcb05 to
c9d57c0
Compare
Contributor
Author
|
Rebased over #6861. |
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
c9d57c0 to
563284b
Compare
mmilata
approved these changes
May 5, 2026
mmilata
left a comment
mmilata
left a comment
Member
There was a problem hiding this comment.
LGTM
Perhaps consider s/TODO/TODO(N4W1)/ so we can easily search & make sure the relevant TODOs are resolved before we enable the feature by default?
| raise RetryRead(TR.n4w1__err_empty) | ||
|
|
||
| # TODO: use protobuf? | ||
| blob = bytes(blob) |
Member
There was a problem hiding this comment.
Why is the bytes constructor needed? I assume we don't have enough information for now to resolve the TODO.
Contributor
Author
There was a problem hiding this comment.
Why is the bytes constructor needed?
memoryview doesn't have .decode() method - so I couldn't call it on blob (which is AnyBytes after c3e6e7a).
I can switch to use str(blob, "utf-8") as done in:
https://github.com/trezor/micropython/blob/e8d89a02609259ad4b5cbaf785255f36bfd3d8dc/py/objstr.c#L1896-L1910
OTOH, assert isinstance(blob, bytes) works as well, since our protobuf codec allocates bytes:
trezor-firmware/core/embed/rust/src/protobuf/decode.rs
Lines 229 to 233 in 563284b
Contributor
Author
There was a problem hiding this comment.
I assume we don't have enough information for now to resolve the TODO.
We can use https://www.notion.so/satoshilabs/Firmware-workpage-WIP-4e58d783efbb41748d599df35053a40a?source=copy_link#28aa30d2d75049bfadcc6e92ad09f2ed for encoding the mnemonic - WDYT?
Member
There was a problem hiding this comment.
memoryview doesn't have .decode() method
ah, didn't realize - blob = bytes(blob) is fine unless it's huge
We can use https://www.notion.so/... for encoding the mnemonic - WDYT?
sounds good!
Contributor
Author
There was a problem hiding this comment.
blob should be small (currently it's only the mnemonic, so at most 33 words).
Opened an issue for defining the storage format -> #6876
Contributor
Author
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/src/apps/management/recovery_device/layout.py (2)
347-360: 💤 Low valueDrop the redundant
super().__init__()call.
_N4W1Handlerdoes not inherit from any explicit base class, sosuper().__init__()on line 353 just invokesobject.__init__(). It's harmless but adds noise;_DisplayHandler.__init__does not include it either, so removing keeps the two handlers consistent.♻️ Proposed nit
def __init__( self, recovery_type: RecoveryType, slip39_state: Slip39State | None, ) -> None: - super().__init__() self.recovery_type = recovery_type # `slip39_state is None` indicates that we are (re)starting the first recovery step. self.slip39_state = slip39_state🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/apps/management/recovery_device/layout.py` around lines 347 - 360, Remove the redundant super().__init__() call inside the _N4W1Handler.__init__ method: delete the line calling super().__init__() so the constructor simply assigns self.recovery_type and self.slip39_state (keeping the load classmethod as-is which returns cls(recovery_type, load_slip39_state())). This makes _N4W1Handler consistent with _DisplayHandler by avoiding the unnecessary object.__init__ invocation.
393-418: 💤 Low valueRemove redundant prefix-checking loop.
check_wordscallsword_validity.checkfor each prefix length 1..N (lines 408-410), butcheckonly examines the last word of the partial mnemonic (vialen(partial_mnemonic) - 1), making each iteration O(1). The outer loop validates at specific index boundaries (0–3) to mirror per-word entry behavior, but since the share is read atomically rather than typed word-by-word, a single call with the full mnemonic may be sufficient. Verify whether validation errors must trigger at intermediate prefix boundaries for correctness or UX before simplifying to a single post-loop check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/apps/management/recovery_device/layout.py` around lines 393 - 418, The prefix-checking loop in check_words unnecessarily calls check(backup_type, partial_mnemonic=...) for each prefix length; simplify by calling check once with the full share_words (i.e. check(backup_type, partial_mnemonic=share_words)) and remove the for loop and per-prefix progress reports, keeping the same try/except/finally structure and progress_obj.start()/stop(); ensure you still await exc.show_error() on WordValidityResult and preserve use of backup_type and share_words for the single check call so behavior and error handling remain identical.core/src/apps/management/reset_device/layout.py (1)
281-305: 💤 Low valueConsider starting progress at
report(0)for consistency.The
report()method accepts values from 0 to 1000. Starting withreport(100)displays ~10% progress before any I/O, then jumps to 100% afterctx.write()completes. Other code in the codebase (e.g.,core/src/trezor/pin.py) initializes progress atreport(0). Aligning with this pattern and the existing TODO comment would support proper progress animation during I/O operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/apps/management/reset_device/layout.py` around lines 281 - 305, In _write_share, change the initial progress report to start at 0 instead of 100: after creating progress_obj = progress(description=TR.n4w1__writing) and progress_obj.start(), call progress_obj.report(0) to match the project's convention (see core/src/trezor/pin.py) so the progress animation covers the I/O; leave the post-write report calls (report(1000) or final values) and the progress_obj.stop() intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 347-360: Remove the redundant super().__init__() call inside the
_N4W1Handler.__init__ method: delete the line calling super().__init__() so the
constructor simply assigns self.recovery_type and self.slip39_state (keeping the
load classmethod as-is which returns cls(recovery_type, load_slip39_state())).
This makes _N4W1Handler consistent with _DisplayHandler by avoiding the
unnecessary object.__init__ invocation.
- Around line 393-418: The prefix-checking loop in check_words unnecessarily
calls check(backup_type, partial_mnemonic=...) for each prefix length; simplify
by calling check once with the full share_words (i.e. check(backup_type,
partial_mnemonic=share_words)) and remove the for loop and per-prefix progress
reports, keeping the same try/except/finally structure and
progress_obj.start()/stop(); ensure you still await exc.show_error() on
WordValidityResult and preserve use of backup_type and share_words for the
single check call so behavior and error handling remain identical.
In `@core/src/apps/management/reset_device/layout.py`:
- Around line 281-305: In _write_share, change the initial progress report to
start at 0 instead of 100: after creating progress_obj =
progress(description=TR.n4w1__writing) and progress_obj.start(), call
progress_obj.report(0) to match the project's convention (see
core/src/trezor/pin.py) so the progress animation covers the I/O; leave the
post-write report calls (report(1000) or final values) and the
progress_obj.stop() intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bbe16c9-a656-4371-984b-56f758c6e186
📒 Files selected for processing (3)
.github/workflows/core.ymlcore/src/apps/management/recovery_device/layout.pycore/src/apps/management/reset_device/layout.py
Can be manually tested by invoking: ``` $ core/emu.py -ea -c trezorctl device setup -b shamir # will run multi-share backup $ core/tools/n1w1-emu.py run 127.0.0.1:21325 /tmp/tagN # simulate tag connection and I/O ``` Enabled N4W1-based backup/recovery device tests for SLIP-39 single group scenarios. Other device & click tests will be added in subsequent PRs. [no changelog]
[no changelog]
Contributor
Author
|
Squashing before merge. |
romanz
force-pushed
the
romanz/2604/n4w1-demo
branch
from
5c7bf59 to
29e4b3e
Compare
mmilata
approved these changes
May 5, 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
2 participants
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.
Design: https://satoshilabs.slack.com/docs/T0J8V2YBY/F0AKE51PVFZ
Also add N4W1-enabled builds to the CI for integration tests.
I have used https://docs.pytest.org/en/stable/how-to/fixtures.html#parametrizing-fixtures in order to re-use some of the existing backup/recovery device tests in a separate CI job:
The rest of the test-related changes will be done in separate PRs.