Sync with Upstream and PSBT hardening - #409
Open
3rdIteration wants to merge 134 commits into
Open
Conversation
Growning "del" button size property (from 2 to 3) solves navigation issue ("6th dice" -> "del").
solves issue SeedSigner#555
Additional differents sizes "del" buttons
new custom_additional_keys prop on KeyboardScreen class for can to change additional button (KEY_BACKSPACE) on its childs classes
Changing KEY_BACKSPACE size on BIP39, SeedBIP85, SeedCustomDerivation, symbols_1, symbols_2 screens
Changing KEY_BACKSPACE size on ToolsDiceEntropy and ToolsCoinFlip screens
Missed import sentence for commands on last commit
The class docstring enumerated two claims while the class held six tests, and each test already documents its own claim, so state the purpose instead of maintaining a second copy. test_get_cosigners_identical_with_and_without_cache only ever wrote to the cache: its three cosigners sit below distinct parent xpubs, so all six level lookups missed and nothing was read back. Call _get_cosigners a third time against the populated cache so the comparison covers the read path, which is where a wrong key would surface. Move the two assert messages that restated their own expressions into comments that give the reason instead.
SeedSigner OS installs these dependencies as buildroot packages with their own pinned, hash-verified versions -- requirements.txt only governs dev and desktop environments. Two entries had drifted from what actually runs on devices, so dev setups were exercising different code than the hardware. The OS versions are definitive. qrcode moves from 7.3.1 to 8.0 to match buildroot's python-qrcode. The major bump is safe for us: everything helpers/qr.py touches (QRCode, ERROR_CORRECT_L, StyledPilImage, the module drawers) works unchanged under 8.0, and devices have been running it all along. urtypes moves from PyPI 1.0.1 to a commit pin of selfcustody/urtypes v0.1.0, the tag the OS builds from GitHub. This is a downgrade -- dev environments were ahead of the hardware, not behind it. PyPI only publishes 1.0.0 and 1.0.1, so matching the OS exactly requires a git pin, in the same style as the existing pyzbar entry. embit and Pillow already matched, and the pyzbar commit pin is already identical to the OS's v0.1.9-ss tag, so those are unchanged.
Like requirements.txt, this file only governs dev installs on a Pi -- shipped images get these packages from buildroot, pinned and hash-verified. Three of the four entries had drifted from the versions devices actually run, so dev Pis were exercising different code than production hardware. The OS versions are definitive. numpy moves from 1.25.2 down to 1.25.0, matching buildroot's python-numpy: dev environments were ahead of the hardware here, not behind it. RPi.GPIO moves from 0.7.0 to 0.7.1 and spidev from 3.5 to 3.6, both catching up to the buildroot pins. picamera already matched the OS's external package at 1.13 and is unchanged. None of these versions are new to the codebase -- every shipped image has been running them via buildroot all along; this just brings dev installs onto the same set.
This file is not just a dev convenience: SeedSigner OS installs it during image builds to compile the .mo translation catalogs. A version pin alone only protects against drift -- it still trusts whatever artifact PyPI serves for that version. Recording sha256 hashes makes pip verify every downloaded file against the digests audited here, so a tampered artifact fails the install instead of entering the build. Each entry's hashes cover the full release (wheel and sdist), so installs work on any host. setuptools moves from >=82.0.0 to ==84.0.0 because hash-checking mode rejects range specifiers. 84.0.0 is what the floor resolves to today, and compile_catalog was verified working under it. Neither Babel nor setuptools pulls transitive dependencies on Python >= 3.10, so these two entries are the complete set pip needs. The file header records how to refresh a hash when bumping a pin, and warns that the file now needs its own pip invocation: any hashed requirement makes pip demand hashes for everything installed alongside it. CI's combined install trips over exactly that; the next commit splits it.
pip enables hash-checking for an entire invocation the moment any requirement in it carries a hash, and then demands hashes for everything else in that invocation. The combined install line mixed requirements-l10n.txt with two unhashed files and an editable install, so it fails now that the l10n pins are hash-locked -- and requirements.txt cannot simply join hash mode, because hash-checking rejects its git-pinned entries outright. Splitting the install keeps the l10n file's hashes enforced while the other files continue to install unhashed. The comment above the split line records why it exists, so it doesn't get folded back into the combined line and break CI later.
…n_cache [performance] PSBT parsing: remove redundant BIP32 derivations and Transaction rebuilds
[chore] Align dev requirements pins with the versions SeedSigner OS ships
[security] Hash-lock the l10n requirements pins
Nothing in a psbt proves the fingerprints and derivation paths it carries, but change_data's fingerprint and derivation_path keys read like facts -- the change details view splits one into a wallet path and hands it straight to get_xpub. Renaming them to claimed_fingerprint and claimed_derivation_path, along with the locals that carry them, puts the trust boundary in the data itself rather than in the logic around it. The class docstring now states the convention behind the prefix: claimed_ for coordinator-supplied metadata, verified_ for what this device proved by re-deriving from the signing seed, and the invariant that no verified_ value comes from a claimed_ one without a derivation in between. It also records that change_data covers self-transfers, not just change-branch outputs. No logic changes; the two dict keys are the only behavioral difference.
claimed_fingerprints and claimed_derivation_paths each hold a list (one entry per cosigner), so the singular key names misread at their call sites. Also drop a commented-out debugging print.
…abulary [refactor] PSBT parsing: rename `change_data`'s unverified fields to `claimed_*`
A psbt is written by an untrusted coordinator, and several constructions
have no honest explanation: they exist so the review screens say
something the signature will not honour. PSBTParser now refuses those
outright rather than displaying them, and refuses with a reason the UI
can show.
Refusals (InvalidPSBTError, carrying a RejectCode):
UNREACHABLE_CHANGE_PATH a change output the seed can derive but whose
prefix does not match one the inputs
demonstrate, or whose branch is outside {0,1}
CHANGE_INDEX_TOO_FAR change index beyond the inputs' highest plus
the Change Gap Limit
UNSUPPORTED_SIGHASH anything but SIGHASH_ALL (SIGHASH_DEFAULT is
accepted on taproot per BIP-341)
NONZERO_OP_RETURN value attached to a provably unspendable output
NEGATIVE_FEE outputs exceeding inputs
AMOUNT_OUT_OF_RANGE any amount outside [0, MAX_MONEY]
UTXO_MISMATCH witness_utxo contradicting non_witness_utxo
INVALID_WITNESS_UTXO witness_utxo that is not a witness program
EXTRANEOUS_WITNESS_SCRIPT witness_script on a non-p2wsh input
SCRIPT_HASH_MISMATCH redeem/witness script not hashing to its spk
MIXED_INPUTS mixed input script types
MISSING_UTXO an input with no prevout at all
Change binding takes its expectation from the inputs rather than a fixed
depth: every input is a utxo the wallet already found, so the prefix they
share is evidence of where this wallet keeps its keys. An unusual but
internally consistent layout still works; a spliced path does not.
Conditions with a genuine innocent explanation stay advisory and are
surfaced on a new PSBTRiskWarningView: HIGH_FEE, DUST_OUTPUT,
FUTURE_LOCKTIME. RBF is recorded but never interrupts -- opt-in RBF is
the default in every modern coordinator, and warning on every ordinary
transaction teaches people to click past the warnings that matter.
Two crashes fixed along the way: _get_policy exploded on any scope
carrying a non-multisig redeem_script (which a legitimate wrapped-segwit
psbt does), and an unparseable psbt delivered over UR2 reached
PSBTSelectSeedView as None and died there rather than being refused at
the scan.
New setting: Change Gap Limit (Settings > Wallet), 100 by default,
adjustable to 1000/10000/Off. It is the one refusal that is a threshold
rather than an impossibility, so the refusal screen points at it.
Tests: the psbt_faker adversarial corpus (41 vectors) is vendored under
tests/data/psbt_test_suite with a per-vector expectation table; 17 are
accepted, 19 refused by the parser, 5 refused by embit. Flow tests cover
the routing, and test_psbt_refusal_screens.py measures the real layout of
every refusal message -- TextArea renders past the bottom edge instead of
raising, so an overlong reason silently draws over the button.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… half Upstream PR SeedSigner#1005 renames change_data's keys to claimed_fingerprints / claimed_derivation_paths and documents a convention: claimed_ for what the coordinator asserts, verified_ for what this device proved by re-deriving, with the invariant that no verified_ value comes from a claimed_ one without a derivation in between. It is a naming change only -- verified_ appears nowhere in upstream src/, and nothing upstream enforces the boundary. Adopting the key names now makes the pending upstream merge a textual no-op instead of a semantic conflict. Instantiating verified_ turned out to require real work rather than a rename. The change-binding rule measures a claimed change path against the prefixes the inputs demonstrate, so those prefixes have to be facts. Two things were needed: - Re-derive each input's claimed path and confirm it produces the claimed pubkey, AND confirm the prevout's script actually commits to that key. Re-deriving alone is not enough: an attacker holding the account xpub can supply a matched path/pubkey pair from elsewhere in our own tree, which verifies fine but says nothing about the utxo being spent. Only the script says which key unlocks these coins. - Distinguish "no evidence" from "no means of gathering it". An empty prefix set previously meant "skip the check", so withholding every input derivation switched change binding off. It now fails closed; None is reserved for the cases with no derivable root at all (WIF/BIP38 signing, seedless multisig pre-parse), where no evidence is expected. Three attacks now covered by TestEvidenceCannotBeForged: the self-consistent input-path lie, withholding input derivations entirely, and mistaking a cosigner's derivation for our own. Corpus disposition unchanged: 17 accepted, 19 refused by the parser, 5 refused by embit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings the fork up to date with SeedSigner/seedsigner so future syncs only
have to consider what is new after this point. Recording the merge (rather
than cherry-picking) is the part that makes that true: upstream's commits
are now ancestors of this branch.
Adopted from upstream:
- PSBTParser's derivation cache (MAX_CACHED_DERIVATIONS + _derive_with_cache),
threaded through this fork's additions. Our input verification derives per
input, so it goes through the cache too; upstream's cache-efficiency test
caught it doing 55 derivations where 5 suffice.
- The duplicate p2sh branch removed from _parse_outputs -- it was dead code,
always overwritten by an identical elif.
- claimed_fingerprints / claimed_derivation_paths naming (renamed ahead of the
merge, so that hunk resolved as a textual no-op).
- VersionView and the Version helper.
- The UR2 bytewords/crc32 fixes and their tests.
- Camera entropy frame-acquisition hardening: reject flat frames (covered
lens) and duplicates (frozen camera), require a full pool before capture is
possible, and require every button released before a capture or a review
decision can register. All of it guards frame acquisition and leaves this
fork's sha256 RNG mixing untouched.
Kept from this fork, deliberately:
- seed_num-based view API. Upstream migrated to passing Seed objects, which is
the better design and will be adopted separately; taking their seed_views.py
wholesale was not an option, as it is 2270 lines to this fork's 5302 with 43
fork-only view classes (SLIP39, encrypted QR, Passport/BitBox02/Tapsigner
backups, SeedKeeper, xpub verification).
- SLIP39 built on the official Trezor shamir-mnemonic lib.
- The image entropy mixing, and a 5-frame pool against upstream's 50: 45 KB of
downsampled greyscale versus ~11 MB of full-res RGBA, which matters on the
Luckfox Pico Zero's 64 MB.
- Dependency pins, the fork's VERSION string, and the screenshots submodule
pointer (upstream's screenshots are of upstream's UI).
Also added: the camera entropy flow now refuses to run when the hardware RNG
monitor reports unhealthy, matching the gate the password generator already
applies. The sha256 mixing means a degraded RNG cannot weaken the result; the
point is not to mint a seed while a source the design relies on is failing.
Watch out for: several upstream changes auto-merged outside conflict regions
and left the tree mixing both seed APIs (finalize_pending_seed returning a Seed,
discard_seed, and view signatures). test_view_kwargs_contract.py and
test_no_undefined_names.py are what caught them.
Full suite: 1191 passed, 135 skipped, 1 xfailed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Views took a seed_num index into controller.storage.seeds, which almost every one immediately dereferenced. Upstream (PR SeedSigner#810) moved to passing the Seed itself. The index is the brittle part: it is a position in a mutable list, so a discard or reorder while a view holds one silently retargets it at a different seed. On a signing device that is worth closing, and matching upstream's API is what keeps future merges tractable. SeedStorage.finalize_pending_seed() int index -> the Seed Controller.get_seed(seed_num) -> removed Controller.discard_seed(seed_num) -> discard_seed(seed) Views: seed_num: int + lookup -> seed: Seed Upstream's seed_views.py could not be taken wholesale -- it is 2270 lines to this fork's 5302, with 43 fork-only view classes -- so the fork's file was migrated in place. `seed_num is None`, which meant "operate on the pending seed", becomes `seed is None`. Two CI failures on the merge commit are fixed here: - The desktop job installed l10n/requirements-l10n.txt in the same pip invocation as everything else. That file is hash-locked, and one hashed requirement puts pip in --require-hashes mode for the whole invocation, which the unhashed files cannot satisfy. Upstream already split it out for the `test` job; the fork-only `desktop` job needed the same treatment. - The screenshot generator passed seed= to views that still took seed_num -- more of the same auto-merge API mixing. The migration resolves it. test_view_kwargs_contract.py now covers the screenshot generator too. It instantiates Views directly, breaks the same way, and cannot run on a dev machine without libraqm (Pillow's Windows wheels omit it), so a mismatch there reaches CI unnoticed -- which is exactly what happened. Verified by injecting a known-bad kwarg and confirming the test fails; the first version silently passed because ScreenshotConfig uses dict(...) calls rather than {...} literals. Full suite: 1191 passed, 135 skipped, 1 xfailed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3rdIteration
changed the title
Both slipped through because this machine cannot run the affected tests.
test_bip85_gpg.py monkeypatched Controller.get_seed, which the Seed-object
migration removed. It now captures the selection at the seeds list, since
the view indexes storage.seeds directly. The whole file is skipped on
win32 unconditionally -- not merely when gpg is absent -- so no amount of
local running would have caught it. The same grep that found the other
call sites did surface this one; I read `def fake_get_seed` as a local
helper rather than a patch target.
Also dropped four now-dead get_seed fakes from
test_password_generator_views.py. Those tests passed either way (the fake
Storage.seeds list satisfies the new access path), but a stub standing in
for a deleted method is misleading.
test_psbt_refusal_screens.py failed on Python 3.10 only:
AttributeError: module 'seedsigner.gui' has no attribute 'renderer'
mock resolves a dotted patch target by importing the root and walking
attributes, so seedsigner.gui.renderer has to already be imported. On 3.13
something else imports it first; on 3.10 nothing does. The fix is the
explicit module-level import, which is now load-bearing and commented as
such.
Note for anyone tempted to switch this to patch.object: that was tried and
is worse. The flow tests install their own Renderer stand-in, and
patch.object binds the class object at decoration time, which does not
survive it -- every test in the file fails when run alongside
test_flows_psbt.py, and the run then hangs. The dotted target plus the
import is the combination that satisfies both constraints.
Full suite: 1192 passed, 135 skipped, 1 xfailed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orts
tests/base.py replaces sys.modules['seedsigner.gui.renderer'] with a
MagicMock at module import time, so the moment any FlowTest file is
*collected* the real renderer module is gone. That single fact explains
every failure this fixture has produced:
- A module-level `from seedsigner.gui.renderer import Renderer` binds
the real class only if this file is imported before base.py.
components.py imports lazily at call time and therefore gets the
MagicMock, so the fixture was patching a different object than the
code under test resolved -- the TextArea measurements came back as
MagicMocks and the comparison raised TypeError.
- A dotted patch target ("seedsigner.gui.renderer.Renderer.get_instance")
cannot resolve at all once that sys.modules entry is swapped, which is
the AttributeError CI hit on 3.10:
`module 'seedsigner.gui' has no attribute 'renderer'`.
- Assigning Renderer._instance directly has the same flaw as the
module-level import: it sets the attribute on whichever class object
this module happens to hold.
Importing Renderer inside the fixture resolves whatever sys.modules
currently holds -- the same object screen.py and components.py will get,
real or mocked -- and patching get_instance on that works in either state.
tests/test_tools_screens.py already does this, for this reason.
Verified in all three orderings rather than the convenient one: the file
alone, with a FlowTest module collected alongside, and with a FlowTest
module running first.
Full suite: 1192 passed, 135 skipped, 1 xfailed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TypeError: SeedAddressVerificationSuccessView.__init__() missing 1 required positional argument: 'seed' Pre-existing, not a regression: at 6654d9c that View took `seed_num: int`, equally required, and generator.py:484 supplied no view_args there either. It only surfaced now because the SeedOptionsView mismatch used to abort the screenshot run before reaching it. The reason it survived a check that exists to catch exactly this: the contract test verified that supplied kwargs are *accepted*, never that *required* ones are *supplied*. Both raise TypeError the moment the View is instantiated, but only the first is visible from the call site. Added _required_params() and wired it into the generator check, then reverted the fix to confirm the test reports the same message CI did -- a check nobody has watched fail is not evidence of anything. Also widened the Destination check, which looked for view_args only as a keyword and so skipped every site passing it positionally as Destination(SomeView, dict(...)) -- the common form in this codebase. Same class of hole: coverage that reads as coverage without being it. Full suite: 1192 passed, 135 skipped, 1 xfailed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n read TypeError: string indices must be integers, not 'str' keyboard.py:234 Upstream's Keyboard rewrite sums each additional key's "size" to check the layout fits, so additional_keys must now be a list of key dicts. But KeyboardScreen still defaults custom_additional_keys to Keyboard.ADDITIONAL_KEYS, which is a dict keyed by code -- iterating it yields strings, and "backspace"["size"] raises. Every upstream KeyboardScreen overrides that default, so upstream never executes it and the trap is invisible there. Four screens in this fork relied on it, three of which lost the override when the merge conflicts in tools_screens.py resolved to our side: ToolsDiceEntropyEntryScreen 3x5, 6 chars KEY_BACKSPACE ToolsCoinFlipEntryScreen 1x4, 2 chars KEY_BACKSPACE_2 SeedExportXpubAccountNumberScreen 3x5, 10 chars KEY_BACKSPACE_5 SettingPBFDK2IterationsScreen 3x5, 10 chars KEY_BACKSPACE_5 The first two match upstream's own choices; the last two follow this fork's existing SeedBIP85SelectChildIndexScreen, which has the identical layout. SettingPBFDK2IterationsScreen is fork-only, so upstream will never fix it for us. Found by enumerating every KeyboardScreen subclass rather than fixing whichever failed first. Verified locally rather than through CI: WSL Ubuntu has Pillow built against raqm, so a venv created with --system-site-packages can run the screenshot generator without sudo or apt. All 22 locales pass. The same environment runs the GPG tests that skip unconditionally on win32, which is how the earlier test_bip85_gpg fix is now actually confirmed rather than assumed. Windows / 3.13: 1192 passed, 135 skipped, 1 xfailed WSL / 3.12: 1320 passed, 8 skipped Screenshot generator: 22 locales passed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SeedSlip39RegenerateSharesView.__init__ ended up with `self.seed = seed` three times: the pre-migration body bound seed_num twice around a get_seed() call, and the mechanical rewrite mapped all three lines onto the same assignment. Idempotent, so behaviour is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Walks the UI for the flows that handle secret material and had no flow coverage: SLIP-39 share regeneration and share selection, the mnemonic backup verification test (correct, wrong-then-retry, wrong-then-review, skip), and BIP-85 child derivation including the invalid-index path. Plus the final-word calculator across all three entropy sources. Beyond routing, these assert on outcomes: regenerated SLIP-39 shares still reconstruct the same secret, the picked share index reaches the view that displays it, and the calculated mnemonic passes BIP-39 checksum validation. The final-word test found a live crash. Selecting "Word selection entropy" clears the last slot via update_pending_mnemonic(None, ...), and the defensive copy added in #335 does "".join(word) unconditionally, so it raised TypeError and dropped the user on the crash screen. Upstream assigns the word directly and is unaffected. Pass None through untouched -- it has no shared wordlist string to defend against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_smartcard_hardware.py drives the card connector directly, which proves the applet and helper layer work but says nothing about the views on top of them -- whether they route correctly and hand the card the seed the user actually selected. That is precisely where the seed_num -> Seed migration could have failed silently. These walk the real UI through FlowTest against a real card, following the same lifecycle as the existing hardware tests: blank JavaCard, install applet, provision, exercise, uninstall. SeedKeeper: save a BIP39 seed and a chosen SLIP-39 share through Seeds > Backup > To SeedKeeper, asserting the card's secret count and labels, and round-tripping one mnemonic back off the card. Satochip: seed a blank card by picking a stored seed in the import view, then confirm a second attempt is refused up front. Skips cleanly with no reader or card. The readiness probe checks pyscard first and ignores Windows Hello virtual readers, which answer SCardConnect with an ATR but are not JavaCards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three SeedKeeper code paths did `from mnemonic import Mnemonic` to convert between BIP-39 entropy and words, but `mnemonic` was never listed in requirements.txt. seedsigner-os carries python-mnemonic 0.20 as a buildroot package on every smartcard board config, so devices were fine; anything installed from requirements.txt -- the desktop simulator, dev machines, CI -- raised ModuleNotFoundError. Saving a BIP-39 seed to a SeedKeeper v2 card surfaced it as an error screen reading "No module named 'mnemonic'". embit is already a hard dependency and does both conversions, so this needs no new package rather than a new requirements line. embit is English-only, which matches the project-wide English-only mnemonic policy; the wordlist argument is now validated instead of honoured, so a card secret declaring a different wordlist is refused rather than silently decoded into the wrong English words. The save path only ever wrote English. Also fixes the round-trip assertion in the new hardware flow test. A v2 Masterseed secret stores the master seed plus BIP-39 entropy, not the plaintext words, so the test now decodes that structure and rebuilds the mnemonic -- which exercises both halves of the conversion instead of grepping the blob for words that were never there. Verified on hardware: 5/5 new flow tests and 51 passed / 2 xfailed in the existing test_smartcard_hardware.py suite, with `mnemonic` uninstalled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The readiness gate called smartcard.System.readers() outside its own try
block. On a machine with no PC/SC subsystem at all -- every Linux and
Windows CI runner -- enumerating readers raises EstablishContextException
("Access denied") rather than returning an empty list, so all five tests
errored at setup instead of skipping. macOS runners passed because the
PC/SC framework is always present there and simply reports no readers,
which is why the failure looked platform-specific.
Move the whole probe inside the guard so an absent subsystem skips for the
same reason an absent card does.
Also stop interpolating the exception into the skip message directly.
pyscard's PC/SC exceptions format themselves by calling back into the
native SCardGetErrorMessage, which can itself raise -- turning a clean
skip back into a spurious error. _describe() falls back to the class name.
Verified three ways: PC/SC missing (simulated via a pytest plugin that
makes readers() raise) skips 5/5; no readers present skips 5/5; a real
card still passes 5/5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up on the corpus vectors that still signed. Five of the seven turn out not to encode the trap their README describes; two were real gaps, and looking for others turned up two more. Refusals: * PSBT_GLOBAL_VERSION 2 (TX-12). A v2 psbt has no unsigned tx -- inputs and outputs carry their own fields and TX_MODIFIABLE says what a coordinator may still change after we sign. TX-12 declares v2 while also carrying a v0 unsigned tx, so reading either half means ignoring the other. We implement v0 only, so refuse rather than guess. * Outputs with no address representation. Witness versions 2-16 are reserved for future soft forks and are currently anyone-can-spend, so value sent there is takeable by anyone who notices. This previously escaped _parse_outputs as a bare ValueError -- a crash screen rather than a decision. The user authorises what the screen shows, so a destination that cannot be shown cannot be authorised. Surfaced: * BIP-68 relative timelocks. Every relative-timelock sequence is also below the RBF ceiling, so it already tripped the RBF check and a user would be told only "replaceable" about a transaction that cannot confirm for up to a year. Detected separately and it interrupts, for the same reason a future nLockTime does. * RBF itself was recorded and then never displayed anywhere: it is INFORMATIONAL, and the risk view filters those out. It stays out of the interstitial -- 0xfffffffd is the modern default and warning on every transaction teaches people to click past warnings -- but it is now stated on the approval screen. Verified by rendering: 171px of content against a 200px button top. Also: nLockTime is only enforced when some input is non-final, so a future locktime with every sequence at 0xffffffff no longer warns. XTRAS.NEGATIVE_AMOUNT also happens to declare v2, so it is now refused for that first; test_amount_bound_still_fires_on_a_v0_psbt keeps the MAX_MONEY path covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Answers "can any of the catalogue items burn, lock or hide funds": one could. A block-height nLockTime was not checked at all, while the semantically identical timestamp form was flagged -- so an attacker wanting a long lock simply used the height encoding and the review screens said nothing. Verified: height 1,000,000 signed with zero warnings. The device has no RTC and no chain tip, so a height is an uninterpretable number on its own. Two independent sources fix that, neither of which requires a clock on the device: * resources/latest-block.json pins one (height, time) pair, refreshed by a scheduled workflow that opens a PR, and hand-editable by copying any recent block from an explorer. It turns a height into an approximate date by assuming 10-minute blocks -- comparing height to height. The approval screen now states "Locked until ~Feb 2031" for both locktime encodings, rendered to month granularity so a stale anchor is harmless. * For psbts loaded from microSD, the file's mtime is a stand-in for "roughly now", written by a machine that did have a clock. A locktime 2+ years past it raises an interrupting warning. The mtime is attacker-influenceable, so it may only ever raise a warning, never suppress one. Forging it buys back the previous behaviour -- the locktime is still displayed -- and nothing more. Every missing input (QR delivery, no anchor, implausible mtime) degrades to display-only rather than guessing; there is a test asserting a forged mtime is indistinguishable from no mtime at all. Also fixed: the json anchor loader built its path outside its own try block, so an unreadable path would have raised instead of falling back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes the last item with real money attached. The existing check is fee as a share of inputs, which misses the opposite failure: a 9%-of-inputs fee on a large consolidation stays under the relative threshold while burning a fortune, and a modest absolute fee on a tiny transaction can still be a wild rate. Verified: 0.09 BTC to fees passed unwarned. A fee is only interpretable as a rate, which needs the vsize of a transaction that has not been signed yet. estimate_vsize() derives it from each input's script type; the signature length is the only unknown and it varies by a byte or two. Anchored against published size tables: 1-in 2-out P2WPKH 140.5 vs 141, P2PKH 226.0 vs 226, P2SH-P2WPKH 165.0 vs 167 -- 0.4%, 0.0%, 1.2%. Fee rates move by orders of magnitude between quiet periods and congestion, so a fixed default would either cry wolf or never fire. The Max Fee Rate setting defaults to Auto, which tracks resources/latest-block.json; the workflow now also records the mean of the top fee rate in each of the last 10 blocks. That is deliberately an outlier-of-outliers -- a "nobody sane pays more than this" line that ordinary traffic sits well under while still moving with the mempool, floored at 20 sat/vB so a quiet reading cannot warn on normal traffic. Fixed thresholds and Off remain available. Warning, never a refusal: paying a high rate is sometimes exactly what the user intends. Corpus rates confirm the headroom: NORMAL vectors run 36-71 sat/vB against the 120 default, while TX-15.mismatch and XTRAS.HUGE_FEE are 7117 and 6406. Suite expectations pin the threshold rather than reading the shipped value, so a scheduled refresh cannot change a corpus verdict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
9 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.
Two things in one branch: PSBT validation hardening built against an adversarial test corpus, and a merge of upstream/dev so the fork is current.
PSBT validation
A PSBT is written by an untrusted coordinator, so the metadata it carries is a claim rather than a fact.
PSBTParsernow validates that claim structurally before anything is shown to the user, and refuses transactions it cannot reconcile — with a reason the UI displays rather than a crash screen.Broadly this covers amount and fee arithmetic, consistency between an input's declared prevout and the script it commits to, sighash handling, and whether an output presented as the user's own sits somewhere their wallet will actually scan.
That last check takes its expectation from the inputs rather than a fixed derivation depth: every input is a utxo the wallet already found, so the prefix they share is evidence of where this wallet keeps its keys. An unusual but internally consistent layout still works. The evidence is verified by re-derivation rather than taken on trust.
Conditions with a genuine innocent explanation stay advisory (
HIGH_FEE,DUST_OUTPUT,FUTURE_LOCKTIME) and are surfaced on a review screen. RBF is recorded but never interrupts — opt-in RBF is the default in every modern coordinator, and warning on every ordinary transaction teaches people to click past the warnings that matter.Two robustness fixes: a legitimate wrapped-segwit PSBT could abort the parse, and a PSBT that embit cannot read is now refused at the scan rather than partway through the signing flow.
New setting: Change Gap Limit (Settings > Wallet), 100 by default. It is the one refusal that is a threshold rather than an impossibility, so the refusal screen points at it.
Upstream merge
118 commits, recorded as a merge so upstream's history becomes ancestry and future syncs only consider what is new.
Adopted: the derivation cache (our per-input verification goes through it — upstream's efficiency test caught it doing 55 derivations where 5 suffice); a dead duplicate p2sh branch removal;
claimed_*naming, renamed ahead of the merge so that hunk resolved as a no-op;VersionView; UR2 bytewords/crc32 fixes; and the camera entropy frame-acquisition hardening — reject flat frames and duplicates, require a full pool before capture, require every button released before a capture or review decision registers.Kept deliberately: the
seed_numview API (upstream'sSeed-object migration is the better design and is coming separately; taking theirseed_views.pywholesale was not an option — 2270 lines to this fork's 5302, with 43 fork-only view classes); SLIP39 on the official Trezor lib; the entropy mixing, and a 5-frame pool against upstream's 50 (45 KB of downsampled greyscale vs ~11 MB of full-res RGBA — material on the Luckfox Pico Zero's 64 MB); dependency pins; the screenshots submodule pointer.Also added: the camera entropy flow refuses to run when the hardware RNG monitor reports unhealthy, matching the gate the password generator already applies. The sha256 mixing means a degraded RNG cannot weaken the result — the point is not to mint a seed while a source the design relies on is failing.
Tests
tests/data/psbt_test_suite/vendors 41 vectors from the psbt_faker adversarial corpus with a per-vector expectation table. Flow tests cover the routing;test_psbt_refusal_screens.pymeasures the real layout of every refusal message, becauseTextArearenders past the bottom edge instead of raising, so an overlong reason silently draws over the button (it was, by 51px).Full suite: 1191 passed, 135 skipped, 1 xfailed.
Worth a look during review
qrcodeto 8.0 andPillowto an older 10.3.0.test_view_kwargs_contract.pyandtest_no_undefined_names.pycaught every one.test_flows_menu_navigation.pyandtest_smartcard_hardware.pyhang on an attached PC/SC reader, and the screenshot generator needs libraqm.🤖 Generated with Claude Code