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

ReviewabilityReviewsAgreementSize
#36114 Sjors wallet: harden external signer psbt processing, revamp mockReady1Strong66 + 328 tests
  • Prevent buggy or compromised external signers and bridge software from altering transaction outputs or fees
  • Reject unsafe sighash flags and finalized inputs, merging returned PSBTs instead of replacing them wholesale
  • Allow functional tests to simulate and test malicious signer responses using an offline descriptor wallet
  • Ready for review
  • All reported concerns and suggestions have been addressed with test coverage
  • Substantive support with no open objections
  • Concept approval for hardening signer handling and improving test mocks (jeanpablojp)
  • Verified rejection of unsafe sighashes, finalized inputs, and corrupted signatures via tests (fametrano)
  • Resolved objection: finalized PSBTs could bypass unsafe sighash checks, fixed by rejecting them (jeanpablojp)
  • 66 lines added or modified outside tests
  • 328 lines added or modified in tests
  • 69 lines removed in total
  • 6 files, 8 commits
Wallet#1 of 92
P1 · fund safety
  • P1 because it protects wallet users from fund theft and output tampering by buggy or malicious signers
  • Bitcoin Core previously accepted returned PSBTs without verifying outputs, sighash types, or sequence numbers
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Hardens external signer PSBT handling in the wallet by merging the returned PSBT into the original instead of replacing it, enforcing matching PSBT versions, rejecting finalized inputs, and rejecting signatures that use unsafe sighash types (SIGHASH_NONE or SIGHASH_SINGLE). It also revamps the functional test external signer mock to use an offline descriptor wallet capable of simulating misbehaving or stripped signer responses, and enforces include-what-you-use (IWYU) on external signer code.

Problem: Bitcoin Core previously replaced its PSBT wholesale with whatever an external signer returned, trusting hardware devices or intermediate bridge software not to tamper with outputs, sequence numbers, or sighash flags. A compromised or buggy signer could finalize inputs, divert funds to fees, alter outputs, or use unsafe sighash types that allow third parties to modify outputs. Furthermore, the functional test framework's mock signer was static and could only replay hardcoded PSBTs, preventing automated testing of malicious signer behavior.

Evidence of importance:

  • Commit 2b801fc384: '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.'
  • Commit 4110fbd057: '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.'
  • Description: 'This PR absorbs the scenarios from #35358, but is better able to test them thanks to the new mock signer implementation.'

Scope: Most of the diff is in Python functional test harnesses (327 lines added across mock and functional tests); C++ changes in src/external_signer.cpp are about 60 lines.

Risk of a mistake: medium (wallet, tests). A flaw in sighash inspection or PSBT merging could cause valid hardware wallet responses to be rejected, breaking external signer transactions for users.

Changed since the previous assessment: Added IWYU enforcement for src/external_signer.cpp in CI and cleaned up headers.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • nothing: all suggestions and objections addressed, fresh Tested ACK received 2026-10-25

Agreement

Strong

substantive support, no open objection (jeanpablojp, fametrano, fametrano, fametrano)

Thread notes: Sjors added the two test cases suggested by fametrano, who confirmed with a Tested ACK after testing each failure case.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
jeanpablojp (contributor)objection, safetyFinalized PSBT responses clear input fields under BIP 174, allowing signatures with unsafe sighash types like SIGHASH_NONE to bypass FindUnsafeSighashType undetected.fixed · fix pushed · author updated the code to reject finalized PSBTs and added a test casechangedyes2026-09-08: 'I made the new mock return a finalized response and `FindUnsafeSighashType` doesn't catch it.'
Status from: 2026-09-28: Sjors: 'It now rejects finalized PSBTs, which avoids complicating `FindUnsafeSighashType`.'
jeanpablojp (contributor)question, interfaceWhether returning a v0 PSBT should have a separate error message or whether the external signer must return the matching PSBT version.answered · author replied that a constraint was added requiring matching versionsansweredyes2026-09-08: 'Worth a separate message, or does the signer have to return the version it was given?'
Status from: 2026-09-28: Sjors: 'I added a constraint that the version must match.'
jeanpablojp (contributor)suggestion, maintenanceAdd single-input test transactions for ECDSA and taproot so each FindUnsafeSighashType branch is exercised independently.fixed · fix pushed · author added a test caseansweredyes2026-09-08: 'A single-input transaction of each type would make each branch carry its own case. Worth adding?'
Status from: 2026-09-28: Sjors: 'Added a test case.'
fametrano (contributor)suggestion, maintenanceAdd tests pinning behavior for a changed v2 input sequence and a corrupted signatureaccepted · fix pushed · author added the requested test cases and reviewer confirmed they pass and fail when protections are disabledansweredyes2026-10-24: '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 incomple'
Status from: 2026-10-25: fametrano: 'The two new cases are the ones I suggested, and each fails when its protection is removed.'

Support:

  • jeanpablojp (contributor) (Concept ACK, 2026-09-08) [read]: supports hardening external signer handling and testing mock changes
  • fametrano (contributor) (Tested ACK, 2026-10-21) [tested]: tested head against combiners, confirming external signer rejects unsafe sighashes and finalized inputs
  • fametrano (contributor) (Tested ACK, 2026-10-24) [tested]: re-ran probe on rebased head and verified wallet_signer.py passes
  • fametrano (contributor) (Tested ACK, 2026-10-25) [both: wallet_signer.py, ProduceSignature, CWallet::FillPSBT]: verified the two new test cases fail when protections are removed and wallet_signer.py passes

Participants:

  • dangervslash (none, neutral, 2 statements 2026-08-28 to 2026-08-28): left non-substantive comments
  • jeanpablojp (contributor, objection, 3 statements 2026-09-08 to 2026-09-08): Concept ACK, but pointed out finalized PSBT responses bypass FindUnsafeSighashType (fixed)
  • fametrano (contributor, support, 3 statements 2026-10-21 to 2026-10-25): Tested ACK; suggested two test cases for changed sequence and corrupted signature, then verified them after author added them

Files

394 lines under test/bench/ci.

  • test/functional/wallet_signer.py +185/-55
  • test/functional/mocks/signer.py +142/-10
  • src/external_signer.cpp +62/-2
  • doc/external-signer.md +4/-0
  • ci/test/03_test_script.sh +1/-1
  • src/external_signer.h +0/-1

Card

The short text the ranking pass compares across PRs.

Bitcoin Core previously trusted external signers completely when signing transactions, replacing the wallet's PSBT wholesale with whatever the signer returned. This PR hardens external signer processing by merging the response into the original PSBT, verifying matching PSBT versions, rejecting finalized inputs, and rejecting signatures with unsafe sighash types like SIGHASH_NONE or SIGHASH_SINGLE. It also revamps the functional test mock signer into a dynamic offline descriptor wallet to test tampered and misbehaving signers, and enforces IWYU for the external signer module.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-26T01:20, confidence high, input hash 6fd2fc139c4adf48
thread read through 2026-10-25 (16 statements), head 42298793aa
labels: Wallet

Categories

Wallet

P1 · fund safety

Core previously trusted external signer responses wholesale, leaving users vulnerable to output tampering, fee diversion, or unsafe sighash flags by buggy signers or compromised intermediate bridge software. Merging PSBTs instead of replacing them and rejecting unsafe signatures protects against unauthorized output alterations and theft.

Membership: Carries the Wallet label and modifies external signer PSBT handling in src/external_signer.cpp and test/functional/wallet_signer.py.

Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 1