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

b-l-u-e commented at 5:15 AM on July 19, 2025: contributor

Fixes #32849 descriptorprocesspsbt crashes with an internal bug assertion when given a PSBT whose inputs looked “finalized” that is non-empty final_script_sig / witness but whose signatures did not verify like invalid Schnorr bytes with unusual sighash combinations such as SIGHASH_SINGLE | ANYONECANPAY.

Completeness was inferred with PSBTInputSigned, which only checks that final fields are present, not that they pass script verification. That let the RPC treat the PSBT as complete and call FinalizeAndExtractPSBT, where a failing invariant surfaced as CHECK_NONFATAL instead of a normal incomplete outcome.

This PR determines complete the same way as the wallet path: after ProcessPSBT, it recomputes PrecomputedTransactionData and sets complete only if every input passes PSBTInputSignedAndVerified. Invalid signatures then yield complete: false and no hex, without going through the finalize path that asserted.

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

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:

#34193 (wallet: make migration more robust against failures 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.

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: contributor

@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

- complete &= PSBTInputSignedAndVerified(psbtx, i, &txdata);
+ 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:

- // Check whether or not all of the inputs are now signed
+ // 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.

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 933e4cd1ad..55c5df4a95 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -37,6 +37,7 @@ from test_framework.psbt import (
     PSBT_IN_WITNESS_UTXO,
     PSBT_OUT_MUSIG2_PARTICIPANT_PUBKEYS,
     PSBT_OUT_TAP_TREE,
+    PSBT_IN_FINAL_SCRIPTWITNESS,
 )
 from test_framework.script import CScript, OP_TRUE, SIGHASH_ALL, SIGHASH_ANYONECANPAY
 from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE
@@ -50,6 +51,7 @@ from test_framework.util import (
     assert_raises_rpc_error,
     find_vout_for_address,
     wallet_importprivkey,
+    bitflipper,
 )
 from test_framework.wallet_util import (
     calculate_input_weight,
@@ -369,6 +371,35 @@ class PSBTTest(BitcoinTestFramework):
         changepos = psbtx["changepos"]
         assert_equal(decoded_psbt["tx"]["vout"][changepos]["scriptPubKey"]["type"], expected_type)
 
+    def test_psbt_with_invalid_signature(self):
+        self.log.info("Test descriptorprocesspsbt with invalid signature in signed PSBT")
+
+        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
+        outputs = [{def_wallet.getnewaddress(address_type="bech32m"): 1}]
+        def_wallet.send(outputs)
+        self.generate(self.nodes[0], 6)
+
+        utxos = [utxo for utxo in def_wallet.listunspent() if utxo["desc"].startswith("tr")]
+        psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
+        descs = [desc for desc in def_wallet.listdescriptors(True)["descriptors"] if desc["desc"].startswith("tr")]
+
+        # unload wallet so as to not use the wallet RPC in further process PSBT calls
+        def_wallet.unloadwallet()
+
+        desc_psbt = self.nodes[0].descriptorprocesspsbt(psbt=psbt, descriptors=descs, finalize=True)["psbt"]
+        flawed_psbt = PSBT.from_base64(desc_psbt)
+        valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
+        invalid_sig = bitflipper(valid_sig)
+        flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
+        flawed_psbt = flawed_psbt.to_base64()
+
+        result = self.nodes[0].descriptorprocesspsbt(psbt=flawed_psbt, descriptors=descs)
+        assert_equal(result["complete"], False)
+        assert_equal("hex" in result, False)
+
+        # load the default wallet back
+        self.nodes[0].loadwallet(self.default_wallet_name)
+
     def run_test(self):
         # Create and fund a raw tx for sending 10 BTC
         psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
@@ -1211,6 +1242,7 @@ class PSBTTest(BitcoinTestFramework):
         if not self.options.usecli:
             self.test_sighash_mismatch()
         self.test_sighash_adding()
+        self.test_psbt_with_invalid_signature()
 
 if __name__ == '__main__':
     PSBTTest(__file__).main()
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index bbad6ea771..b4e597ce1d 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -715,3 +715,6 @@ def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):
     }]
     import_res = wallet_rpc.importdescriptors(req)
     assert_equal(import_res[0]["success"], True)
+
+def bitflipper(input):
+    assert isinstance(input, bytes)
+    return (int.from_bytes(input, 'little') ^ (1 << random.randrange(len(input) * 8))).to_bytes(len(input), 'little')
\ 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.

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index cec937658f..ae1619d625 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -417,20 +417,24 @@ class PSBTTest(BitcoinTestFramework):
         self.generate(self.nodes[0], 6)
 
         utxos = [utxo for utxo in def_wallet.listunspent() if utxo["desc"].startswith("tr")]
-        psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
+        unsigned_psbt = def_wallet.walletcreatefundedpsbt(utxos, [{def_wallet.getnewaddress(): 0.5}])["psbt"]
         descs = [desc for desc in def_wallet.listdescriptors(True)["descriptors"] if desc["desc"].startswith("tr")]
 
         # unload wallet so as to not use the wallet RPC in further process PSBT calls
         def_wallet.unloadwallet()
 
-        desc_psbt = self.nodes[0].descriptorprocesspsbt(psbt=psbt, descriptors=descs, finalize=True)["psbt"]
-        flawed_psbt = PSBT.from_base64(desc_psbt)
+        result = self.nodes[0].descriptorprocesspsbt(psbt=unsigned_psbt, descriptors=descs, finalize=True)
+        assert_equal(result["complete"], True)
+        assert_equal("hex" in result, True)
+
+        valid_sig_psbt = result["psbt"]
+        flawed_psbt = PSBT.from_base64(valid_sig_psbt)
         valid_sig = flawed_psbt.i[0].map.get(PSBT_IN_FINAL_SCRIPTWITNESS)
         invalid_sig = bitflipper(valid_sig)
         flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_sig
-        flawed_psbt = flawed_psbt.to_base64()
+        invalid_sig_psbt = flawed_psbt.to_base64()
 
-        result = self.nodes[0].descriptorprocesspsbt(psbt=flawed_psbt, descriptors=descs)
+        result = self.nodes[0].descriptorprocesspsbt(psbt=invalid_sig_psbt, descriptors=descs, finalize=True)
         assert_equal(result["complete"], False)
         assert_equal("hex" in result, False)

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.

<details><summary>Hints</summary>

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.

</details>

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: contributor

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

git 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 12: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 12: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 12: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 12: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

git 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.

git 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: contributor

re-ACK 9f55be6

Removed the unrelated lint changes because PR #34154 is merged now.
git 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

git 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: contributor

git 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

in test/functional/rpc_psbt.py:437 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:

                                      File "/home/admin/actions-runner/_work/_temp/build/test/functional/rpc_psbt.py", line 1287, in run_test
                                       self.test_psbt_with_invalid_signature()
                                     File "/home/admin/actions-runner/_work/_temp/build/test/functional/rpc_psbt.py", line 437, in test_psbt_with_invalid_signature
                                       result = self.nodes[0].descriptorprocesspsbt(psbt=invalid_sig_psbt, descriptors=descs, finalize=True)
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 917, in __call__
                                       return self.cli.send_cli(self.command, *args, **kwargs)
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 1000, in send_cli
                                       raise JSONRPCException(dict(code=int(code), message=message))
                                   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

in test/functional/rpc_psbt.py:438 in 5a04a06f1f

 433 | +        # The witness format is [num_items: CompactSize] [item1_len: CompactSize] [item1_data] ...
 434 | +        # For taproot key-path spend [0x01] [0x40 or 0x41] [64 or 65 byte signature]
 435 | +        # We need to flip a bit only in the signature, not in the length prefixes
 436 | +        # Skip first 2 bytes (num_items=0x01, sig_len=0x40/0x41) and flip a bit in the signature
 437 | +        prefix = valid_witness[:2]
 438 | +        signature = valid_witness[2:]

achow101 commented at 11:02 PM on January 19, 2026:

This potentially includes the sighash flag, and we shouldn't flip bits in that sighash flag as that does not guarantee that signature will be invalidated. So this slice should be bounded:

        signature = valid_witness[2:66]

achow101 commented at 11:03 PM on January 19, 2026: member

ACK 5a04a06f1f782b7d19a35b44cc8fe81f2b4d4c55

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

rkrux commented at 2:09 PM on January 20, 2026: contributor

ISTM that the Windows, ucrt, test cross-built CI failure is unrelated, a restore wallet test case related to pruned blockchain in wallet_assumeutxo.py is failing.

2026-01-20T13:41:07.2385503Z 2026-01-20T13:41:04.152353Z TestFramework (INFO): Ensuring wallet can't be restored from a backup that was created before the pruneheight (pruned node)
2026-01-20T13:41:07.2386535Z 
2026-01-20T13:41:07.2386839Z 2026-01-20T13:41:06.479889Z TestFramework (ERROR): Unexpected exception
2026-01-20T13:41:07.2387306Z 
2026-01-20T13:41:07.2387503Z Traceback (most recent call last):
2026-01-20T13:41:07.2387815Z 
2026-01-20T13:41:07.2388269Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\util.py", line 166, in try_rpc
2026-01-20T13:41:07.2388901Z 
2026-01-20T13:41:07.2389049Z     fun(*args, **kwds)
2026-01-20T13:41:07.2389318Z 
2026-01-20T13:41:07.2389432Z     ~~~^^^^^^^^^^^^^^^
2026-01-20T13:41:07.2389660Z 
2026-01-20T13:41:07.2390180Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\coverage.py", line 50, in __call__
2026-01-20T13:41:07.2390808Z 
2026-01-20T13:41:07.2391156Z     return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
2026-01-20T13:41:07.2391651Z 
2026-01-20T13:41:07.2392093Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\authproxy.py", line 156, in __call__
2026-01-20T13:41:07.2392771Z 
2026-01-20T13:41:07.2392996Z     raise JSONRPCException(response['error'], status)
2026-01-20T13:41:07.2393424Z 
2026-01-20T13:41:07.2395532Z test_framework.authproxy.JSONRPCException: filesystem error: cannot remove: The process cannot access the file because it is being used by another process [D:\a\_temp\test_runner_₿_🏃_20260120_132914\wallet_assumeutxo_56\node3\regtest\w2\wallet.dat] (-1)
2026-01-20T13:41:07.2397271Z 
2026-01-20T13:41:07.2397277Z 
2026-01-20T13:41:07.2397281Z 
2026-01-20T13:41:07.2397613Z During handling of the above exception, another exception occurred:
2026-01-20T13:41:07.2398102Z 
2026-01-20T13:41:07.2398107Z 
2026-01-20T13:41:07.2398112Z 
2026-01-20T13:41:07.2398334Z Traceback (most recent call last):
2026-01-20T13:41:07.2398622Z 
2026-01-20T13:41:07.2399135Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 142, in main
2026-01-20T13:41:07.2399796Z 
2026-01-20T13:41:07.2399922Z     self.run_test()
2026-01-20T13:41:07.2400161Z 
2026-01-20T13:41:07.2400286Z     ~~~~~~~~~~~~~^^
2026-01-20T13:41:07.2400474Z 
2026-01-20T13:41:07.2400948Z   File "D:\a\bitcoin\bitcoin/test/functional/wallet_assumeutxo.py", line 267, in run_test
2026-01-20T13:41:07.2401587Z 
2026-01-20T13:41:07.2401827Z     self.test_restore_wallet_pruneheight(n3)
2026-01-20T13:41:07.2402338Z 
2026-01-20T13:41:07.2402476Z     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^
2026-01-20T13:41:07.2402806Z 
2026-01-20T13:41:07.2403384Z   File "D:\a\bitcoin\bitcoin/test/functional/wallet_assumeutxo.py", line 99, in test_restore_wallet_pruneheight
2026-01-20T13:41:07.2404151Z 
2026-01-20T13:41:07.2404691Z     assert_raises_rpc_error(-4, error_message, n3.restorewallet, "w2", "backup_w2.dat")
2026-01-20T13:41:07.2405294Z 
2026-01-20T13:41:07.2405553Z     ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2026-01-20T13:41:07.2405970Z 
2026-01-20T13:41:07.2406496Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\util.py", line 157, in assert_raises_rpc_error
2026-01-20T13:41:07.2407238Z 
2026-01-20T13:41:07.2407544Z     assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
2026-01-20T13:41:07.2408097Z 
2026-01-20T13:41:07.2408261Z            ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2026-01-20T13:41:07.2408598Z 
2026-01-20T13:41:07.2409048Z   File "D:\a\bitcoin\bitcoin\test\functional\test_framework\util.py", line 170, in try_rpc
2026-01-20T13:41:07.2409645Z 
2026-01-20T13:41:07.2410041Z     raise AssertionError("Unexpected JSONRPC error code %i" % e.error["code"])
2026-01-20T13:41:07.2410583Z 
2026-01-20T13:41:07.2410797Z AssertionError: Unexpected JSONRPC error code -1
2026-01-20T13:41:07.2411215Z 
2026-01-20T13:41:07.2411925Z 2026-01-20T13:41:06.484138Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2026-01-20T13:41:07.2412795Z 
2026-01-20T13:41:07.2413676Z 2026-01-20T13:41:06.484270Z TestFramework (WARNING): Not cleaning up dir D:\a\_temp\test_runner_₿_🏃_20260120_132914\wallet_assumeutxo_56
2026-01-20T13:41:07.2414525Z 
2026-01-20T13:41:07.2415646Z 2026-01-20T13:41:06.484375Z TestFramework (ERROR): Test failed. Test logging available at D:\a\_temp\test_runner_₿_🏃_20260120_132914\wallet_assumeutxo_56/test_framework.log
2026-01-20T13:41:07.2416737Z 

2026-01-20T13:42:33.3464929Z wallet_assumeutxo.py                                                             | ✖ Failed  | 8 s
2026-01-20T13:42:33.3465209Z 
2026-01-20T13:42:33.3465460Z ALL                                                                              | ✖ Failed  | 2893 s (accumulated) 
2026-01-20T13:42:33.3465784Z Runtime: 796 s

</details>

DrahtBot added the label CI failed on Jan 20, 2026

rkrux commented at 1:37 PM on January 22, 2026: contributor

The #34354 issue that caused the CI to fail is fixed, can we re-run the CI on this PR?

DrahtBot removed the label CI failed on Jan 22, 2026

in test/functional/rpc_psbt.py:417 in f25db06d06

 412 | +        self.log.info("Test descriptorprocesspsbt with invalid signature in signed PSBT")
 413 | +
 414 | +        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
 415 | +        outputs = [{def_wallet.getnewaddress(address_type="bech32m"): 1}]
 416 | +        def_wallet.send(outputs)
 417 | +        self.generate(self.nodes[0], 6)

rkrux commented at 11:45 AM on January 23, 2026:

I realise that generating 1 block should be enough for this case.

@@ -414,7 +414,7 @@ class PSBTTest(BitcoinTestFramework):
         def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
         outputs = [{def_wallet.getnewaddress(address_type="bech32m"): 1}]
         def_wallet.send(outputs)
-        self.generate(self.nodes[0], 6)
+        self.generate(self.nodes[0], 1)

in test/functional/rpc_psbt.py:440 in f25db06d06

 435 | +        # We need to flip a bit only in the signature, not in the length prefixes
 436 | +        # Skip first 2 bytes (num_items=0x01, sig_len=0x40/0x41) and flip a bit in the signature
 437 | +        prefix = valid_witness[:2]
 438 | +        signature = valid_witness[2:66]
 439 | +        invalid_sig = bitflipper(signature)
 440 | +        invalid_witness = prefix + invalid_sig

rkrux commented at 11:49 AM on January 23, 2026:

Thanks for adding this verbose comment, it's helpful.

Agree with bounding it to :66, this works in both the cases of the signature being 64 or 65 bytes. I don't think we should lose the remaining signature portion though while coming up with the invalid witness. So, I feel the following diff can be added:

@@ -434,10 +434,9 @@ class PSBTTest(BitcoinTestFramework):
         # For taproot key-path spend [0x01] [0x40 or 0x41] [64 or 65 byte signature]
         # We need to flip a bit only in the signature, not in the length prefixes
         # Skip first 2 bytes (num_items=0x01, sig_len=0x40/0x41) and flip a bit in the signature
-        prefix = valid_witness[:2]
-        signature = valid_witness[2:66]
-        invalid_sig = bitflipper(signature)
-        invalid_witness = prefix + invalid_sig
+        signature_bytes = valid_witness[2:66]
+        invalid_sig_bytes = bitflipper(signature_bytes)
+        invalid_witness = valid_witness[:2] + invalid_sig_bytes + valid_witness[66:]
         flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_witness
         invalid_sig_psbt = flawed_psbt.to_base64()

b-l-u-e commented at 2:10 PM on January 23, 2026:

just realized that what happens if signature is 65 bytes because if we slicing at byte 66 is when we assume the signature is 64 bytes only.. i feel like this code below might work for both 64 and 65

@@ -435,9 +435,10 @@ class PSBTTest(BitcoinTestFramework):
         # We need to flip a bit only in the signature, not in the length prefixes
         # Skip first 2 bytes (num_items=0x01, sig_len=0x40/0x41) and flip a bit in the signature
         prefix = valid_witness[:2]
-        signature = valid_witness[2:66]
+        sig_len = valid_witness[1]
+        signature = valid_witness[2:2+sig_len]
         invalid_sig = bitflipper(signature)
-        invalid_witness = prefix + invalid_sig
+        invalid_witness = prefix + invalid_sig + valid_witness[2+sig_len:]
         flawed_psbt.i[0].map[PSBT_IN_FINAL_SCRIPTWITNESS] = invalid_witness
         invalid_sig_psbt = flawed_psbt.to_base64()

rkrux commented at 3:03 PM on January 23, 2026:

The present code should work too for a sig with length 65 bytes. Only one bit needs to be flipped to make it invalid, doesn't matter if it's done in full sig (65) or partial sig (64). But yes, the above^ suggestion is more expressive and thorough, and can be implemented.

Might as well add an assert for keypath spend, we already assume it as per the code comments.

@@ -434,6 +434,7 @@ class PSBTTest(BitcoinTestFramework):
         # For taproot key-path spend [0x01] [0x40 or 0x41] [64 or 65 byte signature]
         # We need to flip a bit only in the signature, not in the length prefixes
         # Skip first 2 bytes (num_items=0x01, sig_len=0x40/0x41) and flip a bit in the signature
+        assert_equal(valid_witness[0], 1) # keypath spend
         prefix = valid_witness[:2]
         signature = valid_witness[2:66]
         invalid_sig = bitflipper(signature)
(END)

rkrux commented at 11:50 AM on January 23, 2026: contributor

+1 on fixing the error #33014 (review) highlighted by Drahbot, I didn't think of this scenario earlier.

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

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

rkrux approved

rkrux commented at 8:27 AM on January 30, 2026: contributor

ACK 2fd79d8 Thanks for following up on all the previous comments.

git range-diff 5a04a06...2fd79d8

DrahtBot requested review from achow101 on Jan 30, 2026

DrahtBot added the label Needs rebase on Apr 30, 2026

bitcoin deleted a comment on Apr 30, 2026

b-l-u-e force-pushed on May 6, 2026

DrahtBot added the label CI failed on May 6, 2026

b-l-u-e force-pushed on May 6, 2026

DrahtBot removed the label Needs rebase on May 6, 2026

b-l-u-e force-pushed on May 6, 2026

DrahtBot removed the label CI failed on May 6, 2026

in src/rpc/rawtransaction.cpp:2120 in eb6fb09419

2118 | -    for (const auto& input : psbtx.inputs) {
2119 | -        complete &= PSBTInputSigned(input);
2120 | +    std::optional<PrecomputedTransactionData> txdata_res = PrecomputePSBTData(psbtx);
2121 | +    if (!txdata_res) {
2122 | +        throw JSONRPCPSBTError(common::PSBTError::INVALID_TX);
2123 | +    }

rkrux commented at 11:10 AM on May 7, 2026:

This check already happens in ProcessPSBT call above, it need not be done again and instead can be replaced with an assert.

https://github.com/bitcoin/bitcoin/blob/3f9c55426a12a16bd11de6dec139c4df58788360/src/rpc/rawtransaction.cpp#L183-L186

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index a90614e334..6b218da66d 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -2115,9 +2115,7 @@ RPCMethod descriptorprocesspsbt()
     // Check whether or not all of the inputs are now correctly signed
     bool complete = true;
     std::optional<PrecomputedTransactionData> txdata_res = PrecomputePSBTData(psbtx);
-    if (!txdata_res) {
-        throw JSONRPCPSBTError(common::PSBTError::INVALID_TX);
-    }
+    Assert(txdata_res.has_value());
     const PrecomputedTransactionData& txdata = *txdata_res;
     for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
         const bool verified = PSBTInputSignedAndVerified(psbtx, i, &txdata);

b-l-u-e commented at 8:02 PM on May 10, 2026:

thank you for suggestion made change

b-l-u-e commented at 8:19 PM on May 10, 2026:

i faced an error

src/rpc/rawtransaction.cpp:2118:    Assert(txdata_res.has_value());
^^^
CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.

Aborting the whole process is undesirable for RPC code. So nonfatal
checks should be used over assert. See: src/util/check.h
^---- ⚠️ Failure generated from lint check 'rpc_assert' (Check that fatal assertions are not used in RPC code)!

replaced Assert(txdata_res.has_value()); with CHECK_NONFATAL(txdata_res.has_value());

rkrux changes_requested

rkrux commented at 11:12 AM on May 7, 2026: contributor

Code review at eb6fb09419399bad0f704b4ada9b7bfe738f1429

In the PR description, the following is outdated. Please update it.

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.

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

DrahtBot added the label CI failed on May 10, 2026

DrahtBot commented at 8:19 PM on May 10, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/25638412666/job/75254359655 LLM reason (✨ experimental): CI failed due to a lint error: rpc_assert detected a fatal assert in RPC code (src/rpc/rawtransaction.cpp:2118) instead of using NONFATAL/check logic.

<details><summary>Hints</summary>

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.

</details>

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

b-l-u-e requested review from rkrux on May 10, 2026

DrahtBot removed the label CI failed on May 10, 2026

in src/rpc/rawtransaction.cpp:2119 in a5437a7754

2117 |      bool complete = true;
2118 | -    for (const auto& input : psbtx.inputs) {
2119 | -        complete &= PSBTInputSigned(input);
2120 | +    std::optional<PrecomputedTransactionData> txdata_res = PrecomputePSBTData(psbtx);
2121 | +    CHECK_NONFATAL(txdata_res.has_value());
2122 | +    const PrecomputedTransactionData& txdata = *txdata_res;

maflcko commented at 7:12 AM on May 11, 2026:

nit: Those 3 lines can be a single one: const PrecomputedTransactionData txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx));

rkrux commented at 8:02 AM on May 11, 2026:

This suggestion fails to compile in my system.

/Users/rkrux/projects/bitcoin/src/rpc/rawtransaction.cpp:2117:11: error: unknown type name '_'; did you mean 'mpl_::_'?
 2117 |     const _& txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx));
      |           ^
      |           mpl_::_
/usr/local/Cellar/boost/1.90.0_1/include/boost/mpl/aux_/preprocessed/gcc/placeholders.hpp:14:19: note: 'mpl_::_' declared here
   14 | typedef arg< -1 > _;
      |                   ^
/Users/rkrux/projects/bitcoin/src/rpc/rawtransaction.cpp:2117:63: error: expected '}'
 2117 |     const _& txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx));
      |                                                               ^
/Users/rkrux/projects/bitcoin/src/rpc/rawtransaction.cpp:2117:20: note: to match this '{'
 2117 |     const _& txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx));
      |                    ^
/Users/rkrux/projects/bitcoin/src/rpc/rawtransaction.cpp:2119:32: error: no matching function for call to 'PSBTInputSignedAndVerified'
 2119 |         complete = complete && PSBTInputSignedAndVerified(psbtx, i, &txdata);
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rkrux/projects/bitcoin/src/psbt.h:1641:6: note: candidate function not viable: no known conversion from 'const mpl_::_ *' (aka 'const arg<-1> *') to 'const PrecomputedTransactionData *' for 3rd argument
 1641 | bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned int input_index, const PrecomputedTransactionData* txdata);
      |      ^                                                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make[2]: *** [src/CMakeFiles/bitcoin_node.dir/rpc/rawtransaction.cpp.o] Error 1

maflcko commented at 8:11 AM on May 11, 2026:

I used _ as a placeholder for the type PrecomputedTransactionData, because I thought it would be obvious, but I've edited my comment now. :sweat_smile:

rkrux commented at 9:43 AM on May 11, 2026:

I used _ as a placeholder for the type

Oh, I didn't know it was a thing.

maflcko commented at 6:10 AM on May 12, 2026:

Ah, also the & isn't needed

b-l-u-e commented at 7:15 AM on May 12, 2026:

nit: Those 3 lines can be a single one: const PrecomputedTransactionData txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx));

Thanks for the nit. I tried the one-liner but on CI the build fails

 2117 |     const PrecomputedTransactionData& txdata{*CHECK_NONFATAL(PrecomputePSBTData(psbtx))};
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/util/check.h:113:28: note: expanded from macro 'CHECK_NONFATAL'
  113 |     inline_check_non_fatal(condition, std::source_location::current(), #condition)
      |                            ^~~~~~~~~
1 error generated.
gmake[2]: *** [src/CMakeFiles/bitcoin_node.dir/build.make:1182: src/CMakeFiles/bitcoin_node.dir/rpc/rawtransaction.cpp.o] Error 1
gmake[2]: Leaving directory '/home/admin/actions-runner/_work/_temp/build'
gmake[1]: *** [CMakeFiles/Makefile2:580: src/CMakeFiles/bitcoin_node.dir/all] Error 2
gmake[1]: Leaving directory '/home/admin/actions-runner/_work/_temp/build'
gmake: *** [Makefile:91: all] Error 2

i splitted into two instead first store the result in txdata_opt then use *CHECK_NONFATAL(txdata_opt)

b-l-u-e force-pushed on May 12, 2026

DrahtBot added the label CI failed on May 12, 2026

DrahtBot commented at 6:16 AM on May 12, 2026: contributor

🚧 At least one of the CI tasks failed. Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/25716513750/job/75507592780 LLM reason (✨ experimental): CI failed because the build stopped on a Clang -Werror -Wdangling-gsl error in src/rpc/rawtransaction.cpp (dangling pointer to PrecomputePSBTData(psbtx)).

<details><summary>Hints</summary>

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.

</details>

rpc: Fix descriptorprocesspsbt internal bug on invalid signatures

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

Signed-off-by: b-l-u-e <8102260+blue@users.noreply.github.com>

7e19ce200b

b-l-u-e force-pushed on May 12, 2026

DrahtBot removed the label CI failed on May 12, 2026

rkrux approved

rkrux commented at 12:51 PM on May 15, 2026: contributor

lgtm re-ACK 7e19ce200b2e65770907a818b02e4ec3da9c5374

Hope this PR gets merged soon.