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.

test: have external signer mock use a wallet: gives the mock its own offline node and wallet with private keys, while the test uses the watch-only version.
external_signer: require matching PSBT versions
external_signer: merge PSBT response instead of replacing: tests signers that drop outputs, reduce their value or change the script. Merging rejects these changes while allowing signers to omit unnecessary PSBT fields.
external_signer: reject unsafe sighash types
external_signer: reject finalized PSBT inputs: keeps the above check simple. Documents this new requirement. HWI does not finalize.

The remaining commits prepare the test helpers and enforce IWYU for src/external_signer.cpp.

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.

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.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36114
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36114.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36114#author-ralph

Reviews

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

Type	Reviewers
ACK	fametrano
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:

#36598 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36598.svg"> ([Demo] validation: remove AssumeUTXO by l0rinc)
#36543 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36543.svg"> (validation: check the most recent AssumeUTXO commitment during IBD by l0rinc)
#35671 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35671.svg"> (mining: add TxCollection to bandwidth-efficiently validate external block templates by Sjors)
#35433 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35433.svg"> (wallet: deprecate replaceable argument from transaction (and psbt) creation (and modification) RPCs by rkrux)
#33112 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33112.svg"> (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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

test/functional/mocks/signer.py: assert len(signed.i) > 1 → Prefer assert_greater_than(len(signed.i), 1).

2026-10-25 02:20:54

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

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?

test: reuse create_outpoints in wallet_signer 92cd45b6e4

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.

b94d560e1d

test: add external signer mock sign modes

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

d95f9f42e8

external_signer: require matching PSBT versions

Reject a signer response with a different PSBT version and report the expected and returned versions. Document the requirement and test a v0 response to a v2 request.

12fa81f66c

in src/external_signer.cpp:161 in 0567fa6d2e outdated

 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?

Sjors commented at 8:46 AM on September 28, 2026:

I added a constraint that the version must match.

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?

Sjors commented at 8:46 AM on September 28, 2026:

Added a test case.

Sjors force-pushed on Sep 28, 2026

Sjors commented at 8:45 AM on September 28, 2026: member

Rebased after #36076 and addressed @jeanpablojp's feedback.

Is skipping the finalized case deliberate, or worth covering?

It now rejects finalized PSBTs, which avoids complicating FindUnsafeSighashType. HWI doesn't finalize.

I simplified test setup by using create_outpoints.

Also enforced IWYU.

Sjors marked this as ready for review on Sep 28, 2026

Sjors referenced this in commit 69502ef15a on Sep 28, 2026

Sjors referenced this in commit 2c4399e54b on Sep 28, 2026

Sjors referenced this in commit 4c7cd5ed45 on Sep 28, 2026

Sjors referenced this in commit 84996c7e87 on Sep 28, 2026

DrahtBot added the label Needs rebase on Oct 13, 2026

Sjors force-pushed on Oct 20, 2026

Sjors commented at 4:18 AM on October 20, 2026: member

Trivial rebase after #36440.

DrahtBot removed the label Needs rebase on Oct 20, 2026

Sjors referenced this in commit 7c12f64e62 on Oct 20, 2026

Sjors referenced this in commit 477bc3f2d5 on Oct 20, 2026

Sjors referenced this in commit e7172c3558 on Oct 20, 2026

Sjors referenced this in commit 8d0816b005 on Oct 20, 2026

Sjors referenced this in commit 7af3b5d864 on Oct 21, 2026

fametrano commented at 8:58 PM on October 21, 2026: none

tACK 26f84e2fdb62b474fb97ced6d81cff4f08d311d3

I built the head and compared its signer path with two Combiners: BIP174's, as combinepsbt runs it, and btclib-wallet's combine. The new checks stop what a Combiner lets through.

A replaying mock returned responses built from the wallet's real request, and wallet_signer.py passes. The signer path and both Combiners reject a removed output, a reduced value and a changed script. Only the signer path rejects NONE and SINGLE signatures, a PSBT_IN_SIGHASH_TYPE of NONE, and one or all inputs finalized. ALL|ANYONECANPAY passes and gives the honest txid. A changed v2 sequence or a corrupted signature leaves the wallet's transaction as it was and returns an incomplete PSBT. I can share the probe and its output.

DrahtBot requested review from jeanpablojp on Oct 21, 2026

DrahtBot added the label Needs rebase on Oct 23, 2026

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>

4110fbd057

iwyu: Fix warnings in `src/external_signer.cpp` and treat them as errors 42298793aa

Sjors force-pushed on Oct 24, 2026

Sjors commented at 6:12 AM on October 24, 2026: member

Rebased after #36624. @fametrano I expanded the tests based on your comment. If anything is still missing, feel free to suggest additional test coverage (or publish your probe in a gist, linking to that).

DrahtBot removed the label Needs rebase on Oct 24, 2026

fametrano commented at 10:00 PM on October 24, 2026: none

@Sjors the new tests cover the cases in my tACK except two: a changed v2 input sequence and a corrupted signature. In both the wallet's transaction stays as it was and walletprocesspsbt returns an incomplete PSBT, which a test could pin.

The probe and its output: https://gist.github.com/fametrano/e808f936324f16573f74e5f0f3aadf02

I re-ran it on the new head and wallet_signer.py passes. tACK dc30c41f9b29bbae1eed172155e23ade03717849

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.

Also test a signer that changes a PSBT v2 input sequence before signing.
The transaction identifier used for merging ignores sequences, but the
merge preserves the original sequence, leaving the signatures invalid
for the wallet's transaction. This and a corrupted signature must leave
the original transaction unchanged and return an incomplete PSBT. Test
ECDSA and Schnorr signatures independently.

Suggested in: https://github.com/bitcoin/bitcoin/pull/36114#issuecomment-6390058038

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

2b801fc384

external_signer: reject finalized PSBT inputs

Require signer responses to keep signatures in non-final fields and leave finalization to Bitcoin Core. Document the requirement in the signer API.

Test final scriptSig and witness fields independently, including responses that finalize only one input.

2796180842

Sjors force-pushed on Oct 25, 2026

Sjors commented at 2:20 AM on October 25, 2026: member

Thanks, added two more cases.

fametrano commented at 11:43 AM on October 25, 2026: none

tACK 42298793aa1813d805ae49b963a91df7ba49bd26

The two new cases are the ones I suggested, and each fails when its protection is removed. Copying the signer's sequence into the merged PSBT fails the sequence case. Skipping signature verification fails the corrupted-signature case, for ECDSA and Schnorr. The wallet verifies in two places, ProduceSignature and CWallet::FillPSBT, so both had to go. wallet_signer.py passes, and my probe gives the same outcomes as in the gist.