rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures

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

Fixes #32849 The descriptorprocesspsbt RPC was crashing with an internal bug assertion when processing PSBTs with invalid signatures (like invalid Schnorr signatures with SIGHASH_SINGLE | ANYONECANPAY flags). Instead of returning a helpful error message, it would just crash. This change replaces the CHECK_NONFATAL call with proper error handling that catches the failure and returns a JSONRPCError with a clear message to the user. Tested with Bitcoin Core v29.0.0 using the exact PSBT from the issue report. The fix now returns proper error messages instead of crashing.

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
Stale ACK	rkrux

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34193 (wallet: make migration more robust against failures by furszy)
#34176 (wallet: improve error msg when db directory is not writable by furszy)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

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… my initial approach was incorrect. 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.

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

achow101 commented at 10:17 pm on November 17, 2025: member

A test is definitely necessary here to avoid future regressions. @rkrux’s proposed test would be fine.

The commit message has a duplicate line that needs to be removed.

b-l-u-e force-pushed on Dec 7, 2025

b-l-u-e requested review from l0rinc on Dec 7, 2025

b-l-u-e requested review from rkrux on Dec 7, 2025

in test/functional/rpc_psbt.py:433 in ac99af4f80

428+        valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
429+        invalid_sig = bitflipper(valid_sig)
430+        flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
431+        flawed_psbt = flawed_psbt.to_base64()
432+
433+        result = self.nodes[0].descriptorprocesspsbt(psbt=flawed_psbt, descriptors=descs)

rkrux commented at 10:24 am on December 10, 2025:

Nit containing verbose naming and arguments.

 0diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
 1index cec937658f..ae1619d625 100755
 2--- a/test/functional/rpc_psbt.py
 3+++ b/test/functional/rpc_psbt.py
 4@@ -417,20 +417,24 @@ class PSBTTest(BitcoinTestFramework):
 5         self.generate(self.nodes[0], 6)
 6 
 7         utxos = [utxo for utxo in def_wallet.listunspent() if utxo["desc"].startswith("tr")]
 8-        psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
 9+        unsigned_psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
10         descs = [desc for desc in def_wallet.listdescriptors(True)["descriptors"] if desc["desc"].startswith("tr")]
11 
12         # unload wallet so as to not use the wallet RPC in further process PSBT calls
13         def_wallet.unloadwallet()
14 
15-        desc_psbt = self.nodes[0].descriptorprocesspsbt(psbt=psbt, descriptors=descs, finalize=True)["psbt"]
16-        flawed_psbt = PSBT.from_base64(desc_psbt)
17+        result = self.nodes[0].descriptorprocesspsbt(psbt=unsigned_psbt, descriptors=descs, finalize=True)
18+        assert_equal(result["complete"], True)
19+        assert_equal("hex" in result, True)
20+
21+        valid_sig_psbt = result["psbt"]
22+        flawed_psbt = PSBT.from_base64(valid_sig_psbt)
23         valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
24         invalid_sig = bitflipper(valid_sig)
25         flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
26-        flawed_psbt = flawed_psbt.to_base64()
27+        invalid_sig_psbt = flawed_psbt.to_base64()
28 
29-        result = self.nodes[0].descriptorprocesspsbt(psbt=flawed_psbt, descriptors=descs)
30+        result = self.nodes[0].descriptorprocesspsbt(psbt=invalid_sig_psbt, descriptors=descs, finalize=True)
31         assert_equal(result["complete"], False)
32         assert_equal("hex" in result, False)
33

rkrux approved

rkrux commented at 10:24 am on December 10, 2025: contributor

lgtm ACK ac99af4f80191caa24badcfbb5cca6811fc92cf1

Thanks for accepting the suggestion adding the functional test.

DrahtBot added the label CI failed on Dec 15, 2025

DrahtBot commented at 6:04 pm on December 15, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20010940750/job/58081580703 LLM reason (✨ experimental): Lint check failed due to trailing whitespace in the codebase.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot commented at 9:34 am on December 22, 2025: contributor

Could turn into draft while CI is red? @b-l-u-e Are you still working on this?

b-l-u-e commented at 12:36 pm on December 22, 2025: none

Could turn into draft while CI is red?

@b-l-u-e Are you still working on this?

yes still workin on it

b-l-u-e force-pushed on Dec 22, 2025

b-l-u-e requested review from rkrux on Dec 22, 2025

in test/functional/rpc_psbt.py:141 in 7e468a6fb0 outdated

137@@ -136,7 +138,7 @@ def test_utxo_conversion(self):
138 
139         # Have the offline node sign the PSBT (which will remove the non-witness UTXO)
140         signed_psbt = offline_node.walletprocesspsbt(psbt_new.to_base64())
141-        assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]
142+        assert "non_witness_utxo" not in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]

rkrux commented at 1:10 pm on December 22, 2025:

These changes seem unrelated to this fix, it’s usually preferred to avoid them but okay because it’s more readable this way and the test passes.

b-l-u-e commented at 4:19 pm on December 24, 2025:

These were auto-fixed by the linter “ruff check –fix” which flagged them as style issues (E713). I kept them since they improve readability, but I can revert to keep the PR focused only on the bug fix.

maflcko commented at 7:32 am on December 26, 2025:

i guess it could make sense to enable this globally, so i’ve done that in #34154. Mixing unrelated refactoring cleanup with a bugfix in the same commit often makes review harder.

rkrux approved

rkrux commented at 1:17 pm on December 22, 2025: contributor

code review re-ACK 7e468a6

0git range-diff ac99af4...7e468a6

Thanks for accepting the nit suggestion.

in test/functional/rpc_psbt.py:425 in 7e468a6fb0

420+        unsigned_psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
421+        descs = [desc for desc in def_wallet.listdescriptors(True)["descriptors"] if desc["desc"].startswith("tr")]
422+
423+        # unload wallet so as to not use the wallet RPC in further process PSBT calls
424+        def_wallet.unloadwallet()
425+

achow101 commented at 0:23 am on December 24, 2025:

In 7e468a6fb01b36edb6955ccb2b869ec1063a891c “rpc: Fix descriptorprocesspsbt internal bug on invalid signatures”

Extra whitespace on this line

in test/functional/rpc_psbt.py:429 in 7e468a6fb0

424+        def_wallet.unloadwallet()
425+        
426+        result = self.nodes[0].descriptorprocesspsbt(psbt=unsigned_psbt, descriptors=descs, finalize=True)
427+        assert_equal(result["complete"], True)
428+        assert_equal("hex" in result, True)
429+

achow101 commented at 0:23 am on December 24, 2025:

In 7e468a6fb01b36edb6955ccb2b869ec1063a891c “rpc: Fix descriptorprocesspsbt internal bug on invalid signatures”

Extra whitespace on this line

in src/rpc/rawtransaction.cpp:2051 in 7e468a6fb0

2047@@ -2048,10 +2048,12 @@ RPCHelpMan descriptorprocesspsbt()
2048         sighash_type,
2049         finalize);
2050 
2051-    // Check whether or not all of the inputs are now signed
2052+    // Check whether or not all of the inputs are now correctly signed

achow101 commented at 0:23 am on December 24, 2025:

In 7e468a6fb01b36edb6955ccb2b869ec1063a891c “rpc: Fix descriptorprocesspsbt internal bug on invalid signatures”

Trailing whitespace on this line

in src/rpc/rawtransaction.cpp:2056 in 7e468a6fb0

2054-    for (const auto& input : psbtx.inputs) {
2055-        complete &= PSBTInputSigned(input);
2056+    const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
2057+    for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
2058+        const bool verified = PSBTInputSignedAndVerified(psbtx, i, &txdata);
2059+        complete = complete && verified;

achow101 commented at 0:23 am on December 24, 2025:

In 7e468a6fb01b36edb6955ccb2b869ec1063a891c “rpc: Fix descriptorprocesspsbt internal bug on invalid signatures”

Trailing whitespace on this line

b-l-u-e commented at 2:29 pm on January 5, 2026:

thank you for your feedback

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

b-l-u-e requested review from rkrux on Dec 24, 2025

b-l-u-e requested review from achow101 on Dec 24, 2025

rkrux commented at 8:55 am on December 26, 2025: contributor

re-ACK 222139e

0git range-diff 7e468a6...222139e

b-l-u-e force-pushed on Dec 27, 2025

DrahtBot removed the label CI failed on Dec 31, 2025

rkrux approved

rkrux commented at 12:51 pm on January 5, 2026: contributor

re-ACK 9f55be650e6181f7d3ea898e3ccfafedecc26869

Removed the unrelated lint changes because PR #34154 is merged now.

0git range-diff 222139e...9f55be6

Please resolve the PR comments that have been addressed to make it easier to go through the PR.

b-l-u-e commented at 2:45 pm on January 5, 2026: none

re-ACK 9f55be6

Removed the unrelated lint changes because PR #34154 is merged now.
0git range-diff 222139e...9f55be6
Please resolve the PR comments that have been addressed to make it easier to go through the PR.

all the PR comments have been resolved

b-l-u-e force-pushed on Jan 5, 2026

b-l-u-e requested review from rkrux on Jan 5, 2026

b-l-u-e requested review from maflcko on Jan 5, 2026

rkrux commented at 3:11 pm on January 5, 2026: contributor

0git range-diff 9f55be6...e995c85

I don’t see any changes in the latest range-diff, why was this latest push done after this #33014#pullrequestreview-3626721928?

b-l-u-e commented at 3:16 pm on January 5, 2026: none

0git range-diff 9f55be6...e995c85
I don’t see any changes in the latest range-diff, why was this latest push done after this #33014 (review)?

the review my bad i was suppose to comment after i rebased the branch that i resolved the comments the trailing spaces was sorted in https://github.com/bitcoin/bitcoin/commit/9f55be650e6181f7d3ea898e3ccfafedecc26869

DrahtBot added the label CI failed on Jan 5, 2026

rpc: Fix descriptorprocesspsbt internal bug on invalid signatures

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

5a04a06f1f

in test/functional/rpc_psbt.py:444 in e995c85c4e outdated

432+        valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
433+        invalid_sig = bitflipper(valid_sig)
434+        flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
435+        invalid_sig_psbt = flawed_psbt.to_base64()
436+
437+        result = self.nodes[0].descriptorprocesspsbt(psbt=invalid_sig_psbt, descriptors=descs, finalize=True)

DrahtBot commented at 7:05 am on January 6, 2026:

 0                                      File "/home/admin/actions-runner/_work/_temp/build/test/functional/rpc_psbt.py", line 1287, in run_test
 1                                       self.test_psbt_with_invalid_signature()
 2                                     File "/home/admin/actions-runner/_work/_temp/build/test/functional/rpc_psbt.py", line 437, in test_psbt_with_invalid_signature
 3                                       result = self.nodes[0].descriptorprocesspsbt(psbt=invalid_sig_psbt, descriptors=descs, finalize=True)
 4                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 5                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 917, in __call__
 6                                       return self.cli.send_cli(self.command, *args, **kwargs)
 7                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 8                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 1000, in send_cli
 9                                       raise JSONRPCException(dict(code=int(code), message=message))
10                                   test_framework.authproxy.JSONRPCException: TX decode failed DataStream::read(): end of data: iostream error (-22)

b-l-u-e force-pushed on Jan 10, 2026

DrahtBot removed the label CI failed on Jan 11, 2026

rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures #33014

Code Coverage & Benchmarks

Reviews

Conflicts