Fix presets with sighash flags in app playground
What changed, and why it matters
This commit fixes a bug in internal developer/test tooling for Ledger's Bitcoin app. The bug caused test PSBTs (Partially Signed Bitcoin Transactions) with certain SIGHASH flags to be built incorrectly, because the tool wrongly assumed the transaction had to balance. The fix makes the test generator understand that some sighash types allow the transaction to be unbalanced. This is a test/playground fix, not a change to the app that runs on the hardware wallet.
No immediate security action required for end users. This is a developer/test-tool fix. Reviewers should verify that the updated test vectors still exercise the hardware app correctly for SIGHASH_NONE/SINGLE/ANYONECANPAY scenarios.
Security signals we found
Fixes incorrect transaction balancing assumption for non-ALL sighash types in test tooling
Adds explicit validation that remainder outputs are disallowed when amounts are not fully committed
Moves sighash assignment into PSBT creation to keep metadata consistent
Evidence from the diff
The patch updates dev-tools/playground/presets.py and test_utils/txmaker.py. It adds a helper sighash_commits_to_all_amounts() that returns False for SIGHASH_NONE, SIGHASH_SINGLE, and any flag with SIGHASH_ANYONECANPAY. When any input uses such a sighash, the PSBT builder no longer requires sum(outputs) <= sum(inputs), no longer computes a remainder/fee, and requires every output amount to be explicit. It also passes input sighashes into createPsbt() instead of setting them after the fact. This corrects the construction of unbalanced test transactions used in the app playground/tests.
Changed components
dev-tools/playground/presets.pytest_utils/txmaker.pyInspect captured patch +68 / −16
diff --git a/dev-tools/playground/presets.py b/dev-tools/playground/presets.py
index b4205b6..f109730 100644
--- a/dev-tools/playground/presets.py
+++ b/dev-tools/playground/presets.py
@@ -167,6 +167,11 @@ class PsbtSpec:
small `fee` with a change remainder gives a net receive. If every output has
an explicit amount there is no free variable, so `fee` must equal
`total_in - sum(outputs)` exactly (else it's a mistake and we raise).
+
+ All of that assumes the transaction is complete, which holds only as long as
+ every input commits to all the amounts — i.e. uses SIGHASH_DEFAULT or
+ SIGHASH_ALL (or omits the field). If there are other sighash flags, every
+ output must set `amount` explicitly, and `fee` is ignored.
"""
inputs: List[PsbtInputSpec]
outputs: List[PsbtOutputSpec]
@@ -181,19 +186,42 @@ def build_psbt_from_spec(policy, spec: PsbtSpec) -> PSBT:
verbatim. `spec.fee` is always honored — via the remainder if there is one,
otherwise as a checked invariant against `total_in - sum(outputs)`. A
per-input `sighash` is copied onto the built PSBT input.
+
+ Exception: if any input sets a `sighash` that doesn't commit to all the
+ amounts, the transaction is not required to balance (see `PsbtSpec`), so every
+ output must set `amount`, and `spec.fee` is neither used nor checked — outputs
+ may exceed the inputs.
"""
- from test_utils.txmaker import createPsbt # local import (heavy)
+ # local imports (heavy module)
+ from test_utils.txmaker import createPsbt, sighash_commits_to_all_amounts
input_amounts = [i.amount for i in spec.inputs]
input_is_external = [i.external for i in spec.inputs]
+ input_sighashes = [i.sighash for i in spec.inputs]
total_in = sum(input_amounts)
+ # SIGHASH_DEFAULT / SIGHASH_ALL still fix every amount, so they keep the
+ # ordinary balanced treatment, exactly like an unset sighash.
+ unbalanced = not all(
+ sighash_commits_to_all_amounts(sighash) for sighash in input_sighashes
+ )
+
remainder_indices = [n for n, o in enumerate(spec.outputs) if o.amount is None]
if len(remainder_indices) > 1:
raise ValueError("at most one output may omit `amount` (the remainder)")
explicit_total = sum(o.amount for o in spec.outputs if o.amount is not None)
- if remainder_indices:
+ if unbalanced:
+ # No fee to spread around: such a sighash leaves the transaction open, so
+ # there is nothing for a remainder output to absorb.
+ if remainder_indices:
+ raise ValueError(
+ "every output must set `amount` when an input sets a `sighash` "
+ "that doesn't commit to all the amounts: such a transaction need "
+ "not balance, so there is no fee for a remainder output to absorb"
+ )
+ output_amounts = [o.amount for o in spec.outputs]
+ elif remainder_indices:
remainder = total_in - explicit_total - spec.fee
if remainder < 0:
raise ValueError(
@@ -220,16 +248,11 @@ def build_psbt_from_spec(policy, spec: PsbtSpec) -> PSBT:
output_amounts = [o.amount for o in spec.outputs]
output_is_change = [o.is_change for o in spec.outputs]
- psbt = createPsbt(
- policy, input_amounts, output_amounts, output_is_change, input_is_external
+ return createPsbt(
+ policy, input_amounts, output_amounts, output_is_change, input_is_external,
+ input_sighashes,
)
- for i, inp in enumerate(spec.inputs):
- if inp.sighash is not None:
- psbt.inputs[i].sighash = inp.sighash
-
- return psbt
-
# ----- wallet-policy presets -------------------------------------------------
@@ -412,8 +435,8 @@ SIGN_PSBT_SCENARIO_PRESETS: List[PolicyPreset] = [
PsbtInputSpec(50_000_000, sighash=SIGHASH_ALL | SIGHASH_ANYONECANPAY),
],
outputs=[
- PsbtOutputSpec(amount=120_000_000), # recipient
- PsbtOutputSpec(is_change=True), # change
+ PsbtOutputSpec(amount=120_000_000), # recipient
+ PsbtOutputSpec(amount=80_999_000, is_change=True), # change
],
),
),
@@ -429,8 +452,8 @@ SIGN_PSBT_SCENARIO_PRESETS: List[PolicyPreset] = [
PsbtInputSpec(50_000_000, sighash=SIGHASH_NONE),
],
outputs=[
- PsbtOutputSpec(amount=120_000_000), # recipient
- PsbtOutputSpec(is_change=True), # change
+ PsbtOutputSpec(amount=120_000_000), # recipient
+ PsbtOutputSpec(amount=80_999_000, is_change=True), # change
],
),
),
diff --git a/test_utils/txmaker.py b/test_utils/txmaker.py
index 8e08f48..8331add 100644
--- a/test_utils/txmaker.py
+++ b/test_utils/txmaker.py
@@ -397,9 +397,22 @@ def fill_inout(wallet_policy: WalletPolicy, inout: Union[PartiallySignedInput, P
derived_key_origin = KeyOriginInfo(fpr, placeholder_der_subpath)
inout.hd_keypaths[derived_pubkey.pubkey] = derived_key_origin
-def createPsbt(wallet_policy: WalletPolicy, input_amounts: List[int], output_amounts: List[int], output_is_change: List[bool], input_is_external: Optional[List[bool]] = None) -> PSBT:
+# Sighash types that commit to every input and output: SIGHASH_DEFAULT (0x00,
+# taproot only) and SIGHASH_ALL (0x01).
+_SIGHASH_TYPES_COMMITTING_TO_ALL = (0x00, 0x01)
+
+
+def sighash_commits_to_all_amounts(sighash: Optional[int]) -> bool:
+ """True if `sighash` fixes both the input and the output set, so that a
+ transaction signed with it must balance. `None` means PSBT_IN_SIGHASH_TYPE is
+ omitted, i.e. the default, which does. Everything else (NONE or SINGLE, which
+ leave outputs open, and any type with ANYONECANPAY OR-ed on top, which leaves
+ the inputs open) does not."""
+ return sighash is None or sighash in _SIGHASH_TYPES_COMMITTING_TO_ALL
+
+
+def createPsbt(wallet_policy: WalletPolicy, input_amounts: List[int], output_amounts: List[int], output_is_change: List[bool], input_is_external: Optional[List[bool]] = None, input_sighashes: Optional[List[Optional[int]]] = None) -> PSBT:
assert len(output_amounts) == len(output_is_change)
- assert sum(output_amounts) <= sum(input_amounts)
# input_is_external marks inputs the wallet policy does NOT own: they get a
# foreign prevout and no key-derivation info, so the app treats them as
@@ -408,6 +421,19 @@ def createPsbt(wallet_policy: WalletPolicy, input_amounts: List[int], output_amo
input_is_external = [False] * len(input_amounts)
assert len(input_is_external) == len(input_amounts)
+ # input_sighashes[i] is the PSBT_IN_SIGHASH_TYPE of input i; None means the
+ # field is omitted, i.e. the default sighash. Defaults to all-default.
+ if input_sighashes is None:
+ input_sighashes = [None] * len(input_amounts)
+ assert len(input_sighashes) == len(input_amounts)
+
+ # A complete transaction cannot spend more than it receives. A sighash that
+ # doesn't commit to all the amounts leaves the transaction open — under
+ # ANYONECANPAY the missing value comes from inputs another signer adds — so it
+ # need not balance, and the check doesn't apply.
+ if all(sighash_commits_to_all_amounts(sighash) for sighash in input_sighashes):
+ assert sum(output_amounts) <= sum(input_amounts)
+
vin: List[CTxIn] = [CTxIn() for _ in input_amounts]
vout: List[CTxOut] = [CTxOut() for _ in output_amounts]
@@ -491,6 +517,9 @@ def createPsbt(wallet_policy: WalletPolicy, input_amounts: List[int], output_amo
wallet_policy.descriptor_template)
for input_index, input in enumerate(psbt.inputs):
+ if input_sighashes[input_index] is not None:
+ input.sighash = input_sighashes[input_index]
+
if input_is_external[input_index]:
# External input: only the (foreign) witness UTXO, so its amount is
# known, but deliberately no derivation info => seen as external.
Why this scored 19/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.