"An output scope claims..." reads no better than "An output claims...".
The word is doing no work in a sentence about what the output says, and
the psbt-scope sense it borrows is not the one the reader needs there.
No behavior change. Corrects prose that had drifted from what the code
does, and addresses two local names for the same reason.
Docstrings and comments:
- The two ownership-claim exceptions state the condition each signals,
without speculating about how it arises.
- _get_seed_derivation_path drops the output-versus-input severity
split, which _verify_claimed_derivation_paths already states.
- The verified_*_derivation_paths comment says what an entry holds, and
no longer explains the None the positive statement already implies.
- parse() steps 1 and 2 name the per-key condition instead of "scopes".
- _parse_inputs gains a docstring covering the amount total and the
one-shared-policy requirement.
- The single-sig branch comment names the script types it covers.
Naming:
- _parse_multisig's `sc` parameter becomes `multisig_script`.
- A TODO marks _get_policy's local `script`, which shadows the embit
module imported at the top of the file.
The three REASON__ constants and the `reason` parameter were fixed up
front to settle a shared vocabulary before any consumer existed, on the
reasoning that one contributor owned all three prospective call sites.
No caller ever passed a reason. Routing each rejection by exception type
sends it to its own view, which leaves PSBTAddressVerificationFailedView
with two outcomes already decided by is_multisig. Later work should
introduce whatever distinctions its own copy needs rather than inherit
three unused constants.
The old name read as if the scan established ownership of the scope
itself. What it proves is narrower: the seed derives the key a scope
names at the claimed path, nothing about that key's role in the scope's
script. The new name states exactly that, and the docstrings around it
are tightened to match.
Also note on _reject_if_seed_cannot_sign that we deliberately skip
verifying that a key appears in its input's script: a key absent from
the script cannot spend the input, so nothing of this seed's is at
risk.
A psbt carries taproot keys as bare x coordinates and embit rebuilds
them assuming even parity, so the full-key comparisons in the two
zero-fingerprint fallbacks failed whenever the seed's real key derives
with odd parity. A taproot psbt from an xpub-only coordinator was then
wrongly rejected with PSBTSeedCannotSignError about half the time.
Both fallbacks now delegate to seed_owns_pubkey, which already compares
taproot keys by their x-only representation. The regression test re-keys
the taproot fixture to an odd-parity path; the existing fixture at
m/86h/1h/0h/0/0 happens to derive even and passed by luck.
Every derivation entry naming the signing seed's fingerprint is now
verified during parse() by re-deriving the key from the seed itself.
A claim that does not hold up aborts the parse before any transaction
detail is shown: a false claim on an output is classified as an attack
(fake change) and routed to a dire warning, while a false claim on an
input is reported as inconsistent data with a plain warning. A psbt
whose inputs this seed cannot sign at all is turned away to seed
selection.
The per-scope verification results are recorded in
verified_input_derivation_paths / verified_output_derivation_paths,
the ownership record later stages consume.
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.
Cover requirements.txt, tests/requirements.txt, and
requirements-raspi.txt like the l10n pins: every entry now carries the
sha256 of every published file for its release, so pip refuses any
unverified artifact.
Hash-checking mode can't verify git checkouts, so the pyzbar and
urtypes deps become commit-pinned GitHub archive tarballs. The mode
also demands pins for everything in the invocation, so pytest's
transitive deps are pinned too, and coverage is pinned as
coverage[toml]: older pips (e.g. the docker dev image's) won't accept
a plain pin as satisfying pytest-cov's extra-qualified dependency.
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.
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.
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.
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.
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.
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.
The cache costs roughly 650KB at the cap, not 600KB. Drop the claim
that the cap guards against malicious memory exhaustion: the device is
airgapped and stateless, so a psbt that bloats the cache costs the user
a power cycle, not funds.
Defaulting it to None means a call site that forgets it silently
derives without a cache. No caller omitted it; now none can.
_derive_with_cache keeps its optional cache, since that branch is the
plain embit derive() the equivalence test compares against.
Nothing outside parse() reads the cache, so it need not be an instance
attribute. As a local, "the cache does not outlive the parse" is a
property of scope rather than a teardown to maintain -- which also
retires the finally block, both initializations, and the test guarding
them.
Reading the cache dict back once the parse is over depends on the parse
leaving that dict intact, which is an implementation detail rather than
the guarantee under test.
Both psbts carried a single change output, so every derivation was a
first-time miss and the cached run only ever wrote to the cache. A
wrong value returned on a hit would have gone unnoticed. Carrying the
change output twice makes the second traversal read back what the
first one cached.
"Explain how MAX_CACHED_DERIVATIONS was chosen and test the cap" left
test_cache_does_not_change_parse_output hand-rolling its own comparison
as a tuple of field reprs, and promised to move it onto the new
assert_same_parse_result helper. Do that, and put it on patch.object
like the rest of the suite instead of the monkeypatch fixture.
Then measure what the tests around it are worth, by breaking the cache
eleven different ways and recording which tests notice. Two results
stood out.
The equivalence test is differential: it parses once with the cache and
once without and requires the two to agree, so it can only catch a
break that makes those two runs disagree. Anything in logic they share
passes it. Breaking legacy p2sh change detection and breaking taproot
change detection both went unnoticed here; TestPSBTParser is what
catches those. Running it across four wallet types and both fingerprint
states was therefore buying nothing, and all eight cases detect exactly
what two of them do. Narrow it to one single-sig and one multisig psbt,
the two routes into the cache, and drop the fingerprint-zeroing
variants: test_zero_fingerprint_fill_over_many_inputs is what actually
catches a break in the fill path.
test_derive_with_cache_matches_plain_derive caught nothing that
test_get_cosigners_identical_with_and_without_cache does not also
catch, so remove it.
Nothing covered the parse dropping its cache when it ends. Deleting
that clear leaves the whole suite green, and the cache holds keys
derived from the signing seed, so add a test that asserts the parse
filled the cache it was handed and that the parser is no longer holding
it afterward.
The cache keys on id(parent_key), which is the parent's address in
memory. Python is free to hand that address to a new object once the
original is released, so an entry outliving its parent could be matched
by an unrelated key that landed on the same address, and
_derive_with_cache would return that entry's child instead of deriving
the right one. It fails silently: a recycled address is
indistinguishable from a hit.
Nothing triggers this today, because every parent -- the signing seed's
root and the cosigner xpubs from psbt.xpubs -- is held for the whole
parse. But that is a property of the call sites rather than of the
function, and the failure would be a wrong key in the middle of change
detection.
Storing the parent alongside its derived child keeps that address
occupied for as long as the entry depending on it is alive. The entry
holds a reference to a key the parser already owns, so nothing new is
retained and nothing is copied. Costs 56 bytes per entry, so the cap now
holds the cache to about 600 KB; the comment is updated to match.
Reported by newtonick in review.
The cap was documented as a bound a real wallet would never approach.
That is backwards: a 3-of-5 multisig -- the largest common policy --
consolidating 200 inputs wants more cached levels than the cap allows.
That case is the watermark the number comes from. Record it, what the
cache costs at that size, and what going over means: the psbt still
parses correctly, it may just derive some levels more than once.
Nothing checked that last claim. Parse a multisig and a single-sig psbt
with the cache free to grow, then again with the cap low enough to fill
partway through, and require identical parser state. Assert the cap was
actually reached, so removing the cap outright fails the test rather
than passing it quietly.
Compare the two parses through a new assert_same_parse_result helper,
which checks field by field so a failure names the field that differs.
test_cache_does_not_change_parse_output still hand-rolls the same
comparison as a tuple of fields and should be moved onto the helper in
a follow-up.
Also give PSBTParser a class docstring; the cap comment had grown large
enough to read as one.
The change-detection block tested for a p2sh policy twice and built the
same script both times: a standalone if ahead of the policy chain, and
an elif inside it. For a p2sh policy neither p2wsh nor p2sh-p2wsh
matches, so control always reached the elif and reassigned an identical
script.p2sh over the first one, from the same redeem_script.
Removing the standalone if leaves a single if/elif chain covering every
policy type, and keeps the comment recording that this branch also
carries pre-segwit multisig (m/45').
Parse output is unchanged. Verified with the full test suite and by
diffing the complete parser state -- policy, amounts, destinations,
change_data, op_return_data -- across 112 scenarios: all seven wallet
script types against all seven recipient types, as spend-with-change,
full spend, self-transfer, and multi-recipient.
Parsing repeats work that only needs doing once, which is noticeable on
the Pi Zero. Three changes remove that repetition. Parse output is
unchanged.
- _parse_outputs read psbt.tx at twelve points in its loop, and PSBT.tx
rebuilds the entire Transaction on each access; read it once up front.
- _fill_missing_fingerprints derived a whole child key per input and per
output purely to read the parent fingerprint stamped on it. HDKey
exposes that same value as my_fingerprint, which derives nothing.
- Cosigner and change-output checks re-derive the same BIP32 levels for
every input and output, since only the address at the end differs. A
new _derive_with_cache memoizes each level for the duration of one
parse; its docstring covers how the cache is keyed, bounded and
discarded, and why.
Verified byte-identical against every fixture in psbt_testing_util.py on
both mainnet and testnet. Tests parse each of them with the cache active
and bypassed and compare the full parser state and the resulting PSBT
bytes.
The screenshot generator fetches the latest release tag at import time via an
unauthenticated GitHub API call. GitHub-hosted runners share outbound IPs and
the unauthenticated api.github.com limit is 60 req/hr/IP, so the call was
intermittently rate-limited (HTTP 403), returned (None, None), and fed None into
the mock_version_to_most_recent_release Version mock. That crashed rendering for
every locale with "TypeError: 'NoneType' object is not subscriptable" in the
OpeningSplash version string (and would likewise hit len(None) on the Version
settings screen).
Substitute a placeholder version name and timestamp when the fetch can't resolve
the latest release, so the "release build" screenshots still render instead of
feeding None into the mock. The real fetch is unchanged when it succeeds.
Previously a single button press captured the final image immediately,
so the entropy could come from just one frame. Now the live preview must
first collect a pool of PREVIEW_POOL_SIZE (50) distinct, non-blank frames
before the final image can be taken, and a progress bar/counter shows the
user how many have been gathered.
- Skip duplicate frames the camera re-hands us (sha256 of the frame bytes)
and reject flat single-color frames (getextrema), so only genuinely new
frames count toward the pool.
- Gate the final capture on a full pool, and rework the held-button guard
(is_maybe_still_holding) so a still-held button can't auto-capture the
instant the pool fills.
Extend tests/test_tools_screens.py and tests/test_flows_tools.py to cover
the pool-fill gating and the held-button behavior at capture time.
When the user holds the button that launches the image entropy flow, the
still-held press was being read as an ANYCLICK, causing the live preview
frames to be skipped and the final image to be captured immediately. The
same held button could then auto-accept the final image review without
the user ever seeing it.
- Live preview: ignore a held ANYCLICK until all buttons have been seen
released once, so only a fresh press captures the final frame.
- Final image review: wait for all buttons to be released before
listening for the accept/reshoot decision.
Add tests/test_tools_screens.py covering the held-button sequences at
both stages, since these interactions directly affect the security of
the generated seed.
crc32n() sized its output to the CRC value's bit length, so a CRC below 2**24
was emitted as 3 bytes instead of 4 (~1 in 256 payloads; 2 bytes below 2**16).
bytewords.decode() unconditionally strips buf[-4:] as the checksum, so a short
CRC costs the payload its last byte.
This corrupts data the device *emits*. Both UREncoder.encode() (single-part)
and UREncoder.encode_part() (fountain frames) go through Bytewords.encode(),
so roughly 0.4% of every UR2 QR frame SeedSigner displays is not spec
compliant. For a single-part UR the frame is deterministic, so an affected
wallet's xpub QR is broken on every export, permanently: measured 2 broken
exports out of 600 randomly generated single-sig wallets. A spec-compliant
reader rejects the frame on the checksum; a lax reader silently drops the
payload's final byte.
The bug is silent because the checksum comparison in decode() was commented
out. Re-enabled here, which is only possible once crc32n emits a fixed width:
otherwise `checksum` (variable length) and `body_checksum` (always buf[-4:])
could not be compared.
Note utils.int_to_bytes() already carries the fixed-width form with the
bit-length version commented out above it.
Vendored from Foundation Devices' ur-py; I have not checked whether upstream
shares this.
Nick KlockengaandGitHub
kdmukai
ruiva