rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures

b-l-u-e commented at 5:15 am on July 19, 2025: none

Summary

Fixes #32849 - Replace CHECK_NONFATAL with proper error handling in descriptorprocesspsbt RPC to prevent internal bug assertions when encountering invalid Schnorr signatures.

Problem

When descriptorprocesspsbt encounters a PSBT with invalid signatures (e.g., invalid Schnorr signatures with SIGHASH_SINGLE | ANYONECANPAY flags), it triggers an internal bug assertion instead of returning a user-friendly error message.

Solution

Replace the CHECK_NONFATAL(FinalizeAndExtractPSBT(psbtx_copy, mtx)) call with proper error handling that throws a JSONRPCError with a clear message.

Testing

Reproduced the bug with Bitcoin Core v29.0.0
Verified the fix returns proper error messages instead of crashing
Tested with the exact PSBT from the issue report

Changes

src/rpc/rawtransaction.cpp: Replace CHECK_NONFATAL with if-statement and JSONRPCError

DrahtBot added the label RPC/REST/ZMQ on Jul 19, 2025

DrahtBot commented at 5:15 am on July 19, 2025: 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/33014.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

achow101 commented at 10:47 pm on July 22, 2025: member

The CHECK_NONFATAL is correct. complete == true means that the PSBT is complete and can be finalized. The invariant here is that complete == true means FinalizeAndExtract must succeed.

BIP 174 states that a finalizer must only construct valid scriptSigs and witnesses. We encounter this error because we assume that if a PSBT is finalized, the scriptSigs and witnesses are valid so we are not performing the extra step of verifying the script. However, in this situation, the finalizer has misbehaved and produce invalid scriptSigs or witnesses which causes the assumption to fail.

This should be resolved by using PSBTInputSignedAndVerified instead. It may even be worth it to combine PSBTInputSigned with PSBTInputSignedAndVerified and replacing PSBTInputSigned wherever it is used. Although that may not be possible to do.

b-l-u-e force-pushed on Jul 24, 2025

b-l-u-e commented at 1:00 pm on July 24, 2025: none

@achow101…You were right, my initial approach was incorrect. I now understand that CHECK_NONFATAL is correctly protecting the invariant that a PSBT marked as complete must be finalizable.

The actual bug, as you pointed out, was that the completeness check was too lenient, only checking for the presence of a signature rather than its cryptographic validity.

After ensuring that the logic first verifies the existing signatures before declaring the PSBT complete (the approach you suggested with PSBTInputSignedAndVerified), the system now behaves as expected. The same invalid PSBT now correctly results in { “complete”: false } instead of triggering the internal bug.

maflcko commented at 1:43 pm on July 24, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

rpc: Fix descriptorprocesspsbt internal bug on invalid signatures

rpc: Fix descriptorprocesspsbt internal bug on invalid signatures

Use PSBTInputSignedAndVerified instead of PSBTInputSigned to properly
validate signatures before marking PSBT as complete.

507501ddfa

b-l-u-e force-pushed on Jul 24, 2025

achow101 requested review from achow101 on Oct 22, 2025

in src/rpc/rawtransaction.cpp:2093 in 507501ddfa

2087@@ -2088,8 +2088,9 @@ RPCHelpMan descriptorprocesspsbt()
2088 
2089     // Check whether or not all of the inputs are now signed
2090     bool complete = true;
2091-    for (const auto& input : psbtx.inputs) {
2092-        complete &= PSBTInputSigned(input);
2093+    const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
2094+    for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
2095+        complete &= PSBTInputSignedAndVerified(psbtx, i, &txdata);

rkrux commented at 1:35 pm on October 24, 2025:

It’s discouraged to use bitwise operators on booleans: https://stackoverflow.com/questions/24542/using-bitwise-operators-for-booleans-in-c

0- complete &= PSBTInputSignedAndVerified(psbtx, i, &txdata);
1+ complete = complete && PSBTInputSignedAndVerified(psbtx, i, &txdata);

l0rinc commented at 2:23 pm on October 24, 2025:

this is likely a change in behavior since && short circuits

in src/rpc/rawtransaction.cpp:2089 in 507501ddfa

2087@@ -2088,8 +2088,9 @@ RPCHelpMan descriptorprocesspsbt()
2088 
2089     // Check whether or not all of the inputs are now signed

rkrux commented at 1:36 pm on October 24, 2025:

0- // Check whether or not all of the inputs are now signed
1+ // Check whether or not all of the inputs are now correctly signed

rkrux commented at 1:39 pm on October 24, 2025: contributor

I don’t think the corresponding bug is still present after v30, see: #32849 (comment)

Also, a test case in rpc_psbt.py would be good to verify the fix.

rkrux commented at 11:50 am on October 25, 2025: contributor

Concept ACK 507501ddfa3f2c538a0817a5645b20ddde554362

I don’t think the corresponding bug is still present after v30, see: #32849 (comment)

I was able to reproduce this bug in the functional tests.

Also, a test case in rpc_psbt.py would be good to verify the fix.

Consider adding the following test case in the second commit in this PR. The following test fails without this fix and passes with it.

Edit: Added bitflipper function in util (taken from feature_taproot.py) and used it to make the signature invalid. The previous approach I used here changed the last byte to 0x42 always that could have been proven to be flaky.

 0diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
 1index 933e4cd1ad..55c5df4a95 100755
 2--- a/test/functional/rpc_psbt.py
 3+++ b/test/functional/rpc_psbt.py
 4@@ -37,6 +37,7 @@ from test_framework.psbt import (
 5     PSBT_IN_WITNESS_UTXO,
 6     PSBT_OUT_MUSIG2_PARTICIPANT_PUBKEYS,
 7     PSBT_OUT_TAP_TREE,
 8+    PSBT_IN_FINAL_SCRIPTWITNESS,
 9 )
10 from test_framework.script import CScript, OP_TRUE, SIGHASH_ALL, SIGHASH_ANYONECANPAY
11 from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE
12@@ -50,6 +51,7 @@ from test_framework.util import (
13     assert_raises_rpc_error,
14     find_vout_for_address,
15     wallet_importprivkey,
16+    bitflipper,
17 )
18 from test_framework.wallet_util import (
19     calculate_input_weight,
20@@ -369,6 +371,35 @@ class PSBTTest(BitcoinTestFramework):
21         changepos = psbtx["changepos"]
22         assert_equal(decoded_psbt["tx"]["vout"][changepos]["scriptPubKey"]["type"], expected_type)
23 
24+    def test_psbt_with_invalid_signature(self):
25+        self.log.info("Test descriptorprocesspsbt with invalid signature in signed PSBT")
26+
27+        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
28+        outputs = [{def_wallet.getnewaddress(address_type="bech32m"): 1}]
29+        def_wallet.send(outputs)
30+        self.generate(self.nodes[0], 6)
31+
32+        utxos = [utxo for utxo in def_wallet.listunspent() if utxo["desc"].startswith("tr")]
33+        psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
34+        descs = [desc for desc in def_wallet.listdescriptors(True)["descriptors"] if desc["desc"].startswith("tr")]
35+
36+        # unload wallet so as to not use the wallet RPC in further process PSBT calls
37+        def_wallet.unloadwallet()
38+
39+        desc_psbt = self.nodes[0].descriptorprocesspsbt(psbt=psbt, descriptors=descs, finalize=True)["psbt"]
40+        flawed_psbt = PSBT.from_base64(desc_psbt)
41+        valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
42+        invalid_sig = bitflipper(valid_sig)
43+        flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
44+        flawed_psbt = flawed_psbt.to_base64()
45+
46+        result = self.nodes[0].descriptorprocesspsbt(psbt=flawed_psbt, descriptors=descs)
47+        assert_equal(result["complete"], False)
48+        assert_equal("hex" in result, False)
49+
50+        # load the default wallet back
51+        self.nodes[0].loadwallet(self.default_wallet_name)
52+
53     def run_test(self):
54         # Create and fund a raw tx for sending 10 BTC
55         psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
56@@ -1211,6 +1242,7 @@ class PSBTTest(BitcoinTestFramework):
57         if not self.options.usecli:
58             self.test_sighash_mismatch()
59         self.test_sighash_adding()
60+        self.test_psbt_with_invalid_signature()
61 
62 if __name__ == '__main__':
63     PSBTTest(__file__).main()
64diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
65index bbad6ea771..b4e597ce1d 100644
66--- a/test/functional/test_framework/util.py
67+++ b/test/functional/test_framework/util.py
68@@ -715,3 +715,6 @@ def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):
69     }]
70     import_res = wallet_rpc.importdescriptors(req)
71     assert_equal(import_res[0]["success"], True)
72+
73+def bitflipper(input):
74+    assert isinstance(input, bytes)
75+    return (int.from_bytes(input, 'little') ^ (1 << random.randrange(len(input) * 8))).to_bytes(len(input), 'little')
76\ No newline at end of file