Merge bitcoin/bitcoin#36076: psbt: preserve sighash type when merging inputs
What the developer wrote
Authored by Ava Chow
Merge bitcoin/bitcoin#36076: psbt: preserve sighash type when merging inputs
ea785a31f783e67adc9b4f0c4a8e54acd2794904 psbt: preserve sighash type when merging inputs (Thomas)
Pull request description:
`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.
ACKs for top commit:
achow101:
ACK ea785a31f783e67adc9b4f0c4a8e54acd2794904
winterrdog:
Re-ACK ea785a31f783e67adc9b4f0c4a8e54acd2794904
vicjuma:
ACK ea785a31f783e67adc9b4f0c4a8e54acd2794904
rkrux:
lgtm ACK ea785a31f783e67adc9b4f0c4a8e54acd2794904
Tree-SHA512: 3487368509926c3dc0218dfab2e08273676504ad5ed4635e12e56c0484bda4cd94f4ba6f2df26ee4a902ce9a274546f727f8eca4a62eb0add3a700b2141eb272
What changed, and why it matters
This commit fixes a bug in Bitcoin Core's handling of Partially Signed Bitcoin Transactions (PSBTs). When two PSBTs for the same transaction were combined, the intended signature hash (sighash) type could be silently dropped depending on the order the PSBTs were provided. This could cause a valid, already-signed PSBT to become impossible to finalize, because the finalizer would fall back to a default sighash type and reject the existing signatures. The fix makes the merge behave like other optional fields: keep the local value if present, otherwise copy it from the other PSBT. A new test verifies both argument orders now produce the same, finalizable result.
No immediate action beyond applying the fix. Users who relied on combinepsbt with PSBTs carrying non-default sighash types should upgrade and, if necessary, re-combine affected PSBTs. The bug is a correctness/data-integrity issue rather than a direct theft vector.
Security signals we found
Data-loss bug in PSBT merge logic
Order-dependent behavior in combinepsbt
Sighash type enforcement bypassed during finalization
Functional test added for both combine orders
Evidence from the diff
PSBTInput::Merge in src/psbt.cpp copies optional input fields from another PSBT input when the local input lacks them, but it omitted PSBT_IN_SIGHASH_TYPE. As a result, combinepsbt could drop sighash_type if the first PSBT did not contain it. Since BIP 174 uses this field to let finalizers enforce the sighash type of existing signatures, its loss caused FinalizePSBT to default to SIGHASH_ALL (or SIGHASH_DEFAULT for Taproot) and treat existing signatures with a different sighash as invalid. The patch adds one line to merge sighash_type consistently with sequence, time_locktime, and height_locktime. A functional test in rpc_psbt.py reproduces the issue with ALL|ANYONECANPAY and confirms both combine orders now finalize and produce identical transactions.
Changed components
src/psbt.cpp: PSBTInput::MergeRPC combinepsbtRPC finalizepsbtBIP 174 PSBT finalizerInspect captured patch +30 / −0
### src/psbt.cpp
@@ -460,6 +460,7 @@ void 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 (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;
### test/functional/rpc_psbt.py
@@ -565,6 +565,34 @@ def test_decodepsbt_long_sighash_type(self):
psbt.i[0].map[PSBT_IN_SIGHASH_TYPE] = (0x101).to_bytes(4, "little")
assert_equal(node.decodepsbt(psbt.to_base64())["inputs"][0]["sighash"], "")
+ def test_combinepsbt_sighash_type(self):
+ self.log.info("Test that combining PSBTs preserves the sighash type field regardless of order")
+ node = self.nodes[0]
+ node.createwallet("combine_sighash")
+ wallet = node.get_wallet_rpc("combine_sighash")
+ def_wallet = node.get_wallet_rpc(self.default_wallet_name)
+
+ def_wallet.send([{wallet.getnewaddress(address_type="bech32"): 1}])
+ self.generate(node, 1)
+ psbt = wallet.walletcreatefundedpsbt(wallet.listunspent(), [{def_wallet.getnewaddress(): 0.5}])["psbt"]
+
+ signed = wallet.walletprocesspsbt(psbt=psbt, sighashtype="ALL|ANYONECANPAY", finalize=False)["psbt"]
+ assert_equal(node.decodepsbt(signed)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY")
+ updated = wallet.walletprocesspsbt(psbt=psbt, sign=False)["psbt"]
+ assert "sighash" not in node.decodepsbt(updated)["inputs"][0]
+
+ finalized = []
+ for psbts in [[signed, updated], [updated, signed]]:
+ combined = node.combinepsbt(psbts)
+ assert_equal(node.decodepsbt(combined)["inputs"][0].get("sighash"), "ALL|ANYONECANPAY")
+ fin_res = node.finalizepsbt(combined)
+ assert_equal(fin_res["complete"], True)
+ assert_equal(node.testmempoolaccept([fin_res["hex"]])[0]["allowed"], True)
+ finalized.append(fin_res["hex"])
+ assert_equal(finalized[0], finalized[1])
+
+ wallet.unloadwallet()
+
def assert_change_type(self, psbtx, expected_type):
"""Assert that the given PSBT has a change output with the given type."""
@@ -1628,6 +1656,7 @@ def global_xpub_key(extended_pubkey):
self.test_sighash_mismatch()
self.test_sighash_adding()
self.test_decodepsbt_long_sighash_type()
+ self.test_combinepsbt_sighash_type()
self.test_psbt_named_parameter_handling()
self.test_psbt_roundtrip()
self.test_psbt_version()Why this scored 51/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.