Descriptor Rework#46
Draft
odudex wants to merge 41 commits into
Draft
Conversation
The previous approach walked each PR commit sequentially via
`git rebase --exec` on a single runner, which made N-commit PRs take
N× the runtime and failed outright when the base-branch fetch was
shallow (rebase couldn't resolve merge-base → replayed the whole repo
history). Replace it with the two-job pattern already used in liana
and bwk:
list-commits: enumerates (origin/$BASE..HEAD~] via `git log` and
emits a JSON array of {sha, name} objects on the
`commits` output.
check-commit: matrix-expanded from that JSON, one parallel job per
intermediate commit, each running the full
ci-checks.sh suite (format + tests + wave_4b build).
Per-job timeout drops from 360 min to 60 min since each runner now
handles a single commit. fail-fast: false so one bad commit doesn't
cancel the siblings.
Three constructors for the unified [Label][Item][?] settings row
shape used by Settings → Wallet:
settings_row_action (parent, label, on_click) — [>]
settings_row_dropdown(parent, label, options, sel,
on_change, help_title, help_msg) — [v][?]
settings_row_toggle (parent, label, initial,
on_change, help_title, help_msg) — [⚪][?]
Each row is one touch-target tall (theme_get_min_touch_size); the
trailing [?] glyph opens a help modal via dialog_show_info() with the
caption that previously wrapped under each toggle inline. Action rows
get [>] (LV_SYMBOL_RIGHT, no asset change). The [?] is FontAwesome
question-circle (U+F128) added to icons_24 and icons_36; icons_36 also
gains qrcode (U+F029) which addresses.c was rendering as tofu on
720×720 simulator viewports.
The helper itself has no callers yet; it's introduced ahead of
Settings → Wallet's rewrite so each subsequent commit that adds a
settings entry can use it from the start.
Adds the `perm_sign` NVS key plus getter/setter (settings_get/set_permissive_signing) that the PSBT classifier will need to gate its fp-only ownership fallback. Split out from a later wallet_settings refactor so the classifier and the intermediate commits that depend on it don't forward-reference an undefined function — every intermediate commit now builds cleanly. The toggle UI and its integration land in the subsequent refactor(wallet_settings) commit.
…le + registered-descriptors sub-page
Adds a pre-classification gate in the scan flow that prevents the
review/Confirm screen from showing when the PSBT doesn't match our
signing policy:
1. No inputs signable at all → 'Cannot sign PSBT: no inputs match
this wallet's signing policy.'
2. Some inputs not ours (mixed-input PSBT) → refuse outright unless
the new 'Partial signing' toggle is enabled in Settings →
Wallet. Default off — mixed-input PSBTs are a classic splice-
attack shape.
The new setting mirrors the existing Permissive signing toggle:
NVS key 'part_sign', get/set in main/core/settings.c/h, switch UI in
main/pages/settings/wallet_settings.c next to Permissive.
The psbt_sign_ack_fn_t callback was designed for a per-input UI prompt that was never wired in — scan.c has always passed NULL. When a permissive-signing fallback produced an input with requires_ack=true, the signer's NULL-guard skipped it and returned 0 signatures, so the review screen led straight to "Failed to sign PSBT". Permissive signing is itself the explicit opt-in — the user toggled the setting AND confirmed the review screen. The second callback ACK is redundant. Drop the parameter and sign requires_ack inputs directly.
Collapse the three boolean flags into a single psbt_ownership_t with four states: EXTERNAL, OWNED_SAFE, OWNED_UNSAFE, EXPECTED_OWNED. Today the classifier never emits OWNED_UNSAFE — that distinction lands in the next commit which adds derive→spk verification. This commit is a pure type rename: - old `owned && verified` ↔ new `OWNED_SAFE` - old `owned && requires_ack` ↔ new `EXPECTED_OWNED` - old `!owned` ↔ new `EXTERNAL` scan.c policy gate, psbt_sign, the output classifier helper, and the test assertions all map across without behavioural change. The permissive-fp-only fallback in psbt_classify_input still gates on settings_get_permissive_signing(); commit 2 lifts that gate up to the scan.c policy layer where it belongs. Also adds the raw_keypath field to output_ownership_t so the next commits can drive UI labelling and derive-verification off the path.
Adds a derive_matches_spk helper that takes a raw fp+path keypath and
checks whether deriving the key and wrapping the resulting pubkey in
any of the four standard spk shapes (p2pkh, p2sh-p2wpkh, p2wpkh, p2tr)
reproduces the target script. The classifier uses it as the
final-pass tie-breaker between OWNED_UNSAFE and EXPECTED_OWNED:
- OWNED_UNSAFE — fp matches, derive verifies the spk on a
non-standard path
- EXPECTED_OWNED — fp matches, derive cannot reach the spk (or
keypath unparseable). This is the harness state
that catches both software-wallet derivation
bugs and UTXO-swap attacks (the §4.7 scenario).
The settings_get_permissive_signing() gate inside the classifier is
gone — classification is now a pure function of the PSBT contents
plus our key. Settings only gate the signing decision in scan.c (next
commit).
Output classifier mirrors the same algorithm so that fp-claiming
outputs are categorized symmetrically.
Tests:
- Fixture D (attacker-swapped UTXO with hash-of-different-key spk):
was EXTERNAL → now EXPECTED_OWNED.
- Fixture E (fp + unknown-purpose path, spk doesn't derive there):
was EXTERNAL → now EXPECTED_OWNED.
- New Fixture F: fp + non-standard path AND derive verifies the spk
→ asserts OWNED_UNSAFE.
…olicy
Adds the second harness setting and rewrites the scan.c policy gate
to use both signing settings symmetrically across inputs and outputs.
settings.{c,h}: new NVS key `exp_own_sign` plus get/set helpers
mirroring `permissive_signing` and `partial_signing`.
wallet_settings.c: new "Expected-owned signing" toggle row beneath
"Partial signing", with caption explaining the harness intent.
scan.c policy gate: walks both inputs and outputs, classifies each
into safe/unsafe/expected-owned, and refuses with a specific dialog
when:
- any element is EXPECTED_OWNED and Expected-owned signing is off
(mentions the path; this is the strictest gate, takes precedence
over Permissive)
- any element is OWNED_UNSAFE and Permissive signing is off
- any input is EXTERNAL and Partial signing is off
The path-hint dialogs cite the offending element (input/output
index + path) so the user knows what to enable.
scan.c review screen: extends output_type_t with OWNED_UNSAFE and
EXPECTED_OWNED, draws them as their own labelled sections (cyan
"Owned (non-standard path)" and red "Expected ownership
(UNVERIFIED)") with the raw path beside the address.
The PSBT review screen previously rendered every change output with its own row + address (Change #N: <amount>, then the bech32 address). Change is verified-owned (PSBT_OWNERSHIP_OWNED_SAFE on chain=1, which already requires derive(spk) to reproduce the script), so the specific addresses don't need user review — they only add noise on small viewports and dilute the user's attention from the actual outgoing spends. Roll all CHANGE-classified outputs into a single 'Change: <total>' amount row. Outputs whose keypath claims our fingerprint on chain=1 but whose derive can't reproduce the spk classify as EXPECTED_OWNED (suspicious, harness state) — not CHANGE — and continue to render in the existing 'Expected ownership (UNVERIFIED)' warning section with the path shown.
…ccepts The Partial-signing gate previously let mixed-input PSBTs straight through to the review screen with no per-input visibility — the only indication was the count in 'Inputs(N): <total>'. With the user co-signing alongside whoever holds the external inputs' keys, the review screen needs to show *what we're co-signing* so the user can spot a malicious or unexpected counterparty. Add a 'External inputs (NOT YOURS) -- you are co-signing:' section between the Inputs row and the spend list, rendered in error_color with each external input's index, amount, and decoded address. The section is omitted when no inputs are external (i.e. fully-owned PSBT), so the standard sign flow stays unchanged. Inputs are classified once via psbt_classify_input during display build; addresses for owned inputs are not decoded since they're not shown.
… PSBT
The BBQr branch in return_from_qr_scanner_cb assumed any BBQr-decoded
payload was raw PSBT bytes — it called wally_psbt_from_bytes once and,
on failure, freed qr_content immediately, so the layer-2 detectors
(Message / base64-PSBT / Descriptor / Address / Mnemonic) never saw
the payload. A BBQr-encoded descriptor (file_type 'U') therefore
landed at "Unrecognized QR format" even though qr_parser_result()
already returned the descriptor text NUL-terminated.
Free qr_content only when the PSBT parse succeeds; otherwise leave
it for the layer-2 chain to inspect. is_descriptor_prefix matches
"wsh(" / "wpkh(" / "tr(" against the decoded text and routes to the
descriptor loader.
Triggered by §5 of the test plan (desc_wsh_2of2, 314 chars) which now
ships as 3-frame BBQr-U after gen-qr.py was taught to split long
plain payloads.
The simulator's storage stub no-op'd descriptor save/load/list, so registry_init() at boot found nothing and the §5 multisig descriptor had to be re-scanned every session. The §6+ multisig flow needs the descriptor to be there after relaunch. Implement file-backed save/load/list/delete/exists in the stub, mirroring the real firmware's contract. Files live under simulator/sim_data/descriptors/<sanitized_id>.txt — exactly the directory `just sim-reset` wipes, so the wipe target still works. Encrypted .kef path isn't implemented (sim has no eFuse) so the encrypted flag is accepted-but-ignored; the testing plan registers plaintext-only. Mnemonic storage stays no-op — the test plan loads via QR each session and there's no flow that needs mnemonic persistence right now.
…fully descriptor_id_loc_wrapper() created the registry-id textarea + eye button as direct children of lv_screen_active() — whatever screen happened to be on top when the validator's info-confirm callback fired. ui_text_input_destroy() only deletes the keyboard + input_group; the textarea + eye_btn are nulled but not lv_obj_del()'d. After the user submits a name, registry_add succeeds, the success dialog appears, and the user dismisses back to home — but the orphaned textarea + eye button stay rendered on top of the home menu, since their original parent screen is still alive. Mirror the lifecycle used by passphrase_page, store_descriptor_page, and kef_decrypt_page: own a wrapper screen, parent the text input to it, lv_obj_del() the wrapper after ui_text_input_destroy(). The textarea + eye_btn cascade-delete with the wrapper, so nothing leaks onto the screen the user returns to.
Loading the same descriptor twice quietly created two registry entries under different ids — wasteful, confusing, and a likely UX trap (which of `aa` / `bb` is the canonical entry?). Add a dedup check that runs before the user is asked for an id, so they see "Descriptor already loaded as 'X'" instead of being marched through the whole register flow only to end up with a duplicate. Storage is the source of truth: registry_storage_has_duplicate() walks STORAGE_FLASH + STORAGE_SD via storage_list_descriptors, parses each persisted plaintext descriptor, and compares BIP-380 checksums (wally_descriptor_get_checksum). Hits short-circuit on the first match and write the existing entry's id back to the caller. The in-memory registry cache isn't consulted — even if it went stale, the answer reflects what's on disk. descriptor_validate_and_load() runs the check after the fingerprint match (so non-ours descriptors don't get bothered) and shows a named error dialog directly before completing with a new VALIDATION_DUPLICATE result. descriptor_loader_show_error treats that result as "dialog already shown" and returns false, alongside SUCCESS and USER_DECLINED.
The PSBT review previously gave the user no way to tell which signing
policy each owned input was being signed under — single-sig BIP44/49/
84/86, or one of the registered multisig descriptors. For psbt_E
(§6 multisig) this was the most-missing piece: a user looking at the
review couldn't confirm "yes, this is the ms_2of2 wallet I expected"
versus an attacker swapping in a different registered descriptor.
Add a "Signing with:" section between the Inputs total and the
External-inputs warning. For each distinct policy across the owned
inputs, render one line:
Signing with:
BIP84 (Native SegWit) acct 0 ← single-sig whitelist
or
ms_2of2 ← registered descriptor id
Sourced from the input_ownership_t.claim that psbt_classify_input
already produces — CLAIM_WHITELIST gives us script_type + account,
CLAIM_REGISTRY gives us the registered entry's id. Distinct
policies are de-duplicated so a 5-input PSBT all on one policy
renders one line, not five.
UNSAFE / EXPECTED_OWNED inputs surface their raw paths in their own
warning sections below — those don't have a "policy" label per se,
so they're skipped here.
Replace the single 'Inputs(N): <total>' row + the separate 'Signing with:' header with one inline row per distinct signing policy across the owned inputs: Inputs(1): B 0.00 100 000 from ms_2of2 Inputs(1): B 0.00 100 000 from Native SegWit @ account 3 Each row's count + amount cover only the inputs from that policy, so a multi-policy PSBT (e.g. an account-0 spend co-signing with an account-3 input) gets a row each. UNSAFE / EXPECTED / External inputs keep their own warning sections — they already carry count + amount + path/address inline and don't have a clean policy label to group on. Also drop the verbose `BIP84 (Native SegWit) acct 0` form that the previous commit emitted; use the script-type name directly with `@ account <N>`: P2PKH → Legacy @ account N P2SH-P2WPKH → Nested SegWit @ account N P2WPKH → Native SegWit @ account N P2TR → Taproot @ account N The "from <policy>" suffix is appended as a fourth label on the same flex row that create_btc_value_row builds, so the row stays compact and wraps cleanly on small viewports.
The stub's descriptor save/load/list/delete/exists functions all ignored the storage_location_t parameter and used a single shared directory (simulator/sim_data/descriptors/). registry_init() at boot calls registry_init_scan() once for STORAGE_FLASH and once for STORAGE_SD; both calls hit storage_list_descriptors which returned the same file list — so every reboot doubled the in-memory entries: boot 1 (after register): 1 entry visible boot 2: scan FLASH adds it, scan SD adds it again → 2 entries remove one → file deleted, second entry orphaned (its storage_delete_descriptor returns NOT_FOUND, registry just drops the in-memory entry) Honour the loc parameter and route to per-location subdirs that mirror the real-firmware layout from main/core/storage.h: STORAGE_FLASH → simulator/sim_data/spiffs/<id>.txt STORAGE_SD → simulator/sim_data/sd/kern/descriptors/<id>.txt A small mkdir_p helper handles the multi-level SD path. Existing data under simulator/sim_data/descriptors/ becomes orphaned — `just sim-reset` (which already wipes simulator/sim_data wholesale) clears it.
The Apply button refactor left the network dropdown and passphrase callbacks as no-ops that only updated local statics, so changing network or passphrase had no effect on the live key/wallet and the home page never refreshed. Add an apply_wallet_changes() helper that re-derives key+wallet+registry, and call it from both callbacks (matching the toggle rows' immediate-apply pattern).
Resolve the PSBT's network (and block on wallet/PSBT mismatch) BEFORE classifying inputs/outputs. Classification consults the singlesig whitelist, which is keyed by coin_type — running it with a stale is_testnet falsely flagged valid testnet paths as non-standard derivation.
…ctions Multisig PSBTs with no descriptor loaded land in EXPECTED_OWNED, where the dialog only mentioned "software wallet bug or attacker-supplied script" and the expected-owned toggle. Restate the risk concisely and label the two actions: load the wallet descriptor (the multisig path) or enable 'Expected-owned signing' to override.
Loading a descriptor for the other network produced a generic "Invalid descriptor format" because descriptor_validate_and_load fell back to parsing on the other network for stage 1, but verify_xpub_and_show_info re-parsed with the wallet's network only and failed. Detect the case upfront: parse on the wallet's network; if that fails, probe the other network and return VALIDATION_NETWORK_MISMATCH so descriptor_loader_show_error can name the expected network and tell the user to switch in Settings.
registry_add_from_string fell back to the other network when the wallet's failed to parse, which let wrong-network descriptors register on boot scan and occupy slots invisible at sign time. registry_init also passed an unused is_testnet flag. Drop the fallback so only descriptors valid for the active network register. The boot-scan path (registry_init_scan) already discards parse failures silently, and the user-load path is gated upstream by VALIDATION_NETWORK_MISMATCH. On a network switch, registry_init re-scans and the visible set follows the active network.
…utton - Split top button row into config row (script dropdown + account) and nav row (prev / next / scan) so the script-type label stays legible on the 320px-wide wave_35 panel. - Remove the Register Descriptor button and its callback graph: with the descriptor rework there is no UI path to a "multisig without descriptor" state, so `needs_descriptor` was always false and the button was always hidden. - Account button now renders as `acc:N` instead of a bare digit, with a slightly wider footprint. - Rename "Wrapped SegWit" to "Nested SegWit" in dropdown options and matching comments to match BIP49 phrasing.
Both addresses and address_checker had near-identical script-type dropdown + account button + numpad overlay code (~150 lines each, with the latter using `ac_*`-prefixed twins of every variable). Pulled the picker into a single helper at pages/shared/wallet_source_picker.{c,h} that owns the dropdown widget, account button + label, numpad overlay lifecycle, the script/option maps, and the visibility logic. Callers pass a parent row, an initial state, and a `(source, account) -> ()` callback.
Mode is selected at construction so the same helper can later serve the public-key page (singlesig only) and a multisig BIP48 export flow without reshuffling the call sites.
In address_checker the picker now takes the full top row by itself (dropdown 72% / account 25%), and the `Verify` button moved to its own row below the picker since there's no horizontal space left.
No behavior change. Net diff: ~150 lines added in the helper, ~390 lines removed across the two consumers.
Replaces the hardcoded BIP84 m/84'/coin'/0' display with a live picker so users can choose any of the four singlesig script types (Native SegWit / Taproot / Legacy / Nested SegWit) and any account. The xpub QR + text under it is recomputed in place whenever the picker fires. This is the export side: the page produces xpubs intended to seed a coordinator. Registered descriptors are deliberately excluded from the dropdown (mode WALLET_PICKER_SINGLESIG, not _WITH_DESCRIPTORS) since this device only contributes one cosigner xpub to a multisig descriptor, that path will be added next as a separate multisig-export flow.
Adds a Multisig settings row (switch widget + label, matching wallet-settings styling) below the picker. When on, the picker is recreated in WALLET_PICKER_MULTISIG_BIP48 mode (P2WSH or Nested-SegWit P2SH-P2WSH) and the xpub is derived at `m/48'/{coin}'/{account}'/{2|1}'` instead of the BIP44/49/84/86 singlesig path. The account selection carries over across the toggle so the user does not have to re-enter it.
The switch is disabled when the singlesig dropdown is on Taproot or Legacy, since BIP48 only covers SegWit multisig. Always enabled in multisig mode so the user can exit.
Output is the device's contributing cosigner xpub with the matching `[fingerprint/derivation]xpub` origin string — the format coordinators (Sparrow, Specter, Caravan, Nunchuk) expect when assembling a multisig descriptor. BIP45 is intentionally not included; modern coordinators all use BIP48.
Also fixes wallet_source_picker_destroy to delete its dropdown and account button explicitly. Previously it relied on the parent being deleted by the caller — fine for page teardown, but the new toggle recreates the picker into a parent that survives, which would stack widgets on top of the old ones.
Once IDF's s_vfs_count high-water mark hits VFS_MAX_COUNT it never recovers, so /dev/video20 fails to register on a second scan after /spiffs and /sdcard are lazy-mounted. Bump the cap to 16.
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.
This draft PR contains a disruptive refactor by @pythcoiner that:
This PR is not meant to be merged soon, or at all, @pythcoiner will signal the right time. The goal is to track mergeability of new features while other development fronts remain active.