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.

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:

#36440 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36440.svg"> (test: speed up functional tests by removing avoidable waits by ViniciusCestarii)
#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 -> use assert_greater_than(len(signed.i), 1) for a comparison-specific helper.

2026-09-28 08:45:51

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 c620f2b727

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.

df7a5a59d4

test: add external signer mock sign modes

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

2a597243ac

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.

6a3f7c92fc

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>

3529725f4c

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.

d335b948ee

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>

ecaf464249

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

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