wallet: harden external signer psbt processing, revamp mock #36114

Sjors commented at 11:27 AM on August 28, 2026: member

The current external signer functional test mock can only echo a prepared PSBT. This gets in the way of testing more complicated scenarios, i.e. misbehaving signers and (MuSig2) multisig.

Commit test: have external signer mock use a wallet revamps the mock signer by handing it its own node and wallet. The (offline) mock wallet has the private keys, while the test uses the watch-only version.

Commit external_signer: merge PSBT response instead of replacing lets the mock signer manipulate the PSBT it returns. It can drop outputs, reduce their value and change the script. Not all manipulations are malicious, e.g. a signer may simply not echo all PSBT fields, which is also covered. All this can be compensated for (thwarted) by merging the external signer PSBT instead of discarding the original.

Commit external_signer: reject unsafe sighash types adds scenarios where devices use an unsafe sighash type.

This PR absorbs the scenarios from #35358, but is better able to test them thanks to the new mock signer implementation.

This PR does not intend to fully harden against a malicious external signer, and can't protect against a malicious HWI (equivalent) process running as the same user.

Based on:

#36076

DrahtBot added the label Wallet on Aug 28, 2026

DrahtBot commented at 11:27 AM on August 28, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36114.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36122 (BIP460: CISA for Taproot key path spends by fjahr)
#35358 (external signer: verify PSBT is reliable after signing it by brunoerg)
#33112 (wallet: relax external_signer flag constraints by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors commented at 11:30 AM on August 28, 2026: member

cc @bigspider and other hardware wallet folks, any other hardware wallet shenanigans we should check against (perhaps in a followup)?

dangervslash commented at 10:00 PM on August 28, 2026: none

Approved flame

dangervslash commented at 10:00 PM on August 28, 2026: none

MarkUp

Sjors referenced this in commit 2ced1cc1ee on Sep 3, 2026

Sjors referenced this in commit e225eb8358 on Sep 3, 2026

DrahtBot added the label Needs rebase on Sep 7, 2026

psbt: preserve sighash type when merging inputs

`PSBTInput::Merge` copies every optional input field from the other
input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So
`combinepsbt` silently drops the sighash type whenever the first PSBT
does not carry it, making the result depend on the argument order.

The field is what lets finalizers enforce the sighash type of existing
signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the
default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs),
rejects signatures made with any other type as a sighash mismatch, and
the PSBT can no longer be finalized. Combining a PSBT signed with
`ALL|ANYONECANPAY` after a merely updated copy of the same PSBT
reproduces this: `finalizepsbt` reports it as incomplete, while the
reverse order finalizes and broadcasts fine.

Merge the sighash type like the other optional fields, keeping the one
already present, and test both combine orders.

ea785a31f7

test: have external signer mock use a wallet

The external signer mock previously replayed a PSBT that the test
prepared in advance. This makes it difficult to test more complicated
scenarios like a misbehaving wallet and (MuSig2) multisig.

Instead, give the mock its own descriptor wallet. The test provides a
dedicated node for this wallet and keeps it offline, so the mock can't
cheat by e.g. inspecting the UTXO set. The mock creates the wallet on
first use and signs with walletprocesspsbt.

wallet_signer.py now funds all four descriptor types and spends them
in a single transaction, exercising every signing code path.

9078ef76ed

external_signer: merge PSBT response instead of replacing

Previously the PSBT returned by the external signer replaced the
original wholesale, trusting the signer not to modify the transaction.
Merge it instead. This rejects a response that describes a different
transaction.

Merging also supports signers that strip fields they don't need from
their response. New tests cover both scenarios.

Co-authored-by: brunoerg <brunoely.gc@gmail.com>

d5b357da01

external_signer: reject unsafe sighash types

A signature with SIGHASH_NONE or SIGHASH_SINGLE doesn't commit to all
outputs, letting anyone alter them after signing. Reject a PSBT from
an external signer that declares such a sighash type or contains
signatures made with one.

SIGHASH_ANYONECANPAY is still accepted: it only permits adding inputs,
which does not affect us.

The mock signer produces real signatures for these scenarios by
letting its wallet sign with the requested sighash type, optionally
hiding the declared sighash type field so that only the signatures
themselves reveal it.

Co-authored-by: brunoerg <brunoely.gc@gmail.com>

0567fa6d2e

Sjors force-pushed on Sep 8, 2026

Sjors commented at 10:57 AM on September 8, 2026: member

Rebased after #36113, still based on #36076.

DrahtBot removed the label Needs rebase on Sep 8, 2026

jeanpablojp commented at 11:54 AM on September 8, 2026: contributor

Concept ACK

I made the new mock return a finalized response and FindUnsafeSighashType doesn't catch it. The fields it reads are the ones PSBTInput::Serialize writes only when the input isn't finalized, and BIP 174 has a finalizer clear them, so the signatures arrive in final_script_sig and final_script_witness alone.

With SIGHASH_NONE the send comes back complete, both signatures in the transaction end in 0x02, and testmempoolaccept allows it.

The commit message says the change rejects a response containing signatures made with an unsafe sighash type, and a finalized one does. The different-transaction check still rejects a finalized response that changes an output.

Is skipping the finalized case deliberate, or worth covering?

in src/external_signer.cpp:161 in 0567fa6d2e

 157 | +        error = strprintf("Signer used an unsafe sighash type: %s", sighash_str.empty() ? "unknown" : sighash_str);
 158 | +        return false;
 159 | +    }
 160 | +
 161 | +    if (!psbtx.Merge(*signer_psbtx)) {
 162 | +        error = "Signer returned a PSBT for a different transaction";

jeanpablojp commented at 11:54 AM on September 8, 2026:

nit: Merge only returns false for two reasons, GetUniqueID and the version comparison, since neither PSBTInput::Merge nor PSBTOutput::Merge has a failure path.

I returned the same transaction as a v0 PSBT and send failed with this message. GetUniqueID is what stops it, since it zeroes nSequence only from version 2 on, and before this commit that response was accepted.

Worth a separate message, or does the signer have to return the version it was given?

in test/functional/wallet_signer.py:266 in 0567fa6d2e

 314 | +                assert_raises_rpc_error(-25, "External signer failed to sign", hww.send, outputs={dest: 1.5}, inputs=inputs, add_inputs=False)
 315 | +
 316 | +        self.log.info('The signer must not use unsafe sighash types')
 317 | +        # The first mode declares the sighash type in the PSBT, the second
 318 | +        # leaves it out, so only the signatures themselves reveal it
 319 | +        for mode in ["sighash_none", "sighash_none_hidden"]:

jeanpablojp commented at 11:54 AM on September 8, 2026:

nit: this transaction has an ECDSA and a taproot input, and FindUnsafeSighashType collects from both before it scans, so either signature alone triggers the rejection. Disabling the m_tap_key_sig collection leaves wallet_signer.py green, and disabling the partial_sigs one does too. Only removing both makes sighash_none_hidden fail.

A single-input transaction of each type would make each branch carry its own case. Worth adding?