psbt: preserve sighash type when merging inputs #36076

thomasbuilds commented at 10:33 AM on August 25, 2026: contributor

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.

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.

15803d84eb

DrahtBot added the label PSBT on Aug 25, 2026

DrahtBot commented at 10:33 AM on August 25, 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/36076.

Reviews

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

Type	Reviewers
ACK	jpk68, vicjuma, winterrdog
Concept ACK	Sjors

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:

#36114 (wallet: harden external signer psbt processing, revamp mock by Sjors)
#36113 (psbt: fix rendering for invalid long sighash type field by Sjors)
#35984 (sign: skip signing SIGHASH_SINGLE inputs with no corresponding output by furszy)

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.

jpk68 commented at 2:33 PM on August 25, 2026: contributor

ACK 15803d84ebb6871d507dd93065f3572a7291271e

winterrdog commented at 11:02 AM on August 26, 2026: contributor

Concept ACK

sighash_type specifies which parts of the transaction a signature for the input commits to, so it makes sense to retain it when merging if one PSBT has it and the other does not

in src/psbt.cpp:467 in 15803d84eb

 463 | @@ -464,6 +464,7 @@ bool PSBTInput::Merge(const PSBTInput& input)
 464 |      for (const auto& [agg_key_lh, psigs] : input.m_musig2_partial_sigs) {
 465 |          m_musig2_partial_sigs[agg_key_lh].insert(psigs.begin(), psigs.end());
 466 |      }
 467 | +    if (sighash_type == std::nullopt && input.sighash_type != std::nullopt) sighash_type = input.sighash_type;

vicjuma commented at 12:31 PM on August 26, 2026:

I think it correctly complements the sighash_type check in line 701 especially after the sighash_type change from int to std::optional<int> type in #31622

in test/functional/rpc_psbt.py:577 in 15803d84eb

 572 | +        assert_equal(node.decodepsbt(signed)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY")
 573 | +        updated = wallet.walletprocesspsbt(psbt=psbt, sign=False)["psbt"]
 574 | +        assert "sighash" not in node.decodepsbt(updated)["inputs"][0]
 575 | +
 576 | +        finalized = []
 577 | +        for psbts in [[signed, updated], [updated, signed]]:

vicjuma commented at 12:32 PM on August 26, 2026:

The above change works perfectly well independent of the order.

for psbts in [[updated, signed], [signed, updated]]:
    combined = node.combinepsbt(psbts)
    self.log.info(f"combined sighash: {node.decodepsbt(combined)['inputs'][0].get('sighash')}")

Before: 2026-08-26T12:24:19.694909Z TestFramework (INFO): combined sighash: None After: 2026-08-26T12:29:47.516225Z TestFramework (INFO): combined sighash: ALL|ANYONECANPAY

vicjuma commented at 12:34 PM on August 26, 2026: contributor

ACK 15803d84ebb6871d507dd93065f3572a7291271e

DrahtBot requested review from winterrdog on Aug 26, 2026

winterrdog commented at 7:18 PM on August 26, 2026: contributor

tACK 15803d84ebb6871d507dd93065f3572a7291271e

successfully built and tested on this toolchain: FreeBSD 15.0/clang++-19/x86_64

i commented out the new sighash_type preservation line and ran the functional test, which failed with None == ALL|ANYONECANPAY, confirming that the field is not retained when it is absent from the first PSBT but present in the second

<details> <summary>diff to comment the new preservation line </summary>

diff --git a/src/psbt.cpp b/src/psbt.cpp
index 18e2d0a7f8..d4d6c31418 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -464,7 +464,7 @@ bool PSBTInput::Merge(const PSBTInput& input)
     for (const auto& [agg_key_lh, psigs] : input.m_musig2_partial_sigs) {
         m_musig2_partial_sigs[agg_key_lh].insert(psigs.begin(), psigs.end());
     }
-    if (sighash_type == std::nullopt && input.sighash_type != std::nullopt) sighash_type = input.sighash_type;
+    // if (sighash_type == std::nullopt && input.sighash_type != std::nullopt) sighash_type = input.sighash_type;
     if (sequence == std::nullopt && input.sequence != std::nullopt) sequence = input.sequence;
     if (time_locktime == std::nullopt && input.time_locktime != std::nullopt) time_locktime = input.time_locktime;
     if (height_locktime == std::nullopt && input.height_locktime != std::nullopt) height_locktime = input.height_locktime;

</details>

<details> <summary>test output log i got after i applied the diff </summary>

Traceback (most recent call last):
  File "/home/winterrdog/Documents/btc/btc-core/my-btc-fork/test/functional/test_framework/test_framework.py", line 145, in main
    self.run_test()
  File "/home/winterrdog/Documents/btc/btc-core/my-btc-fork/build/test/functional/rpc_psbt.py", line 1650, in run_test
    self.test_combinepsbt_sighash_type()
  File "/home/winterrdog/Documents/btc/btc-core/my-btc-fork/build/test/functional/rpc_psbt.py", line 579, in test_combinepsbt_sighash_type
    assert_equal(node.decodepsbt(combined)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY")
  File "/home/winterrdog/Documents/btc/btc-core/my-btc-fork/test/functional/test_framework/util.py", line 94, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(None == ALL|ANYONECANPAY)

</details>

after a git restore to remove the local change, all tests passed successfully!

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

Concept ACK