Skip to content

Conversation

@b-l-u-e
Copy link

@b-l-u-e b-l-u-e commented Jul 19, 2025

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
Copy link
Contributor

DrahtBot commented Jul 19, 2025

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)

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
Copy link
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 b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 5265405 to 8b4cf7d Compare July 24, 2025 12:56
@b-l-u-e
Copy link
Author

b-l-u-e commented Jul 24, 2025

@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 b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 8b4cf7d to 507501d Compare July 24, 2025 18:55
@achow101 achow101 self-requested a review October 22, 2025 15:47
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
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 b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 507501d to ac99af4 Compare December 7, 2025 21:59
@b-l-u-e b-l-u-e requested review from l0rinc and rkrux December 7, 2025 22:02
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK ac99af4

Thanks for accepting the suggestion adding the functional test.

@DrahtBot
Copy link
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.

Hints

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
Copy link
Contributor

Could turn into draft while CI is red?

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

@b-l-u-e
Copy link
Author

b-l-u-e commented Dec 22, 2025

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 b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from ac99af4 to 7e468a6 Compare December 22, 2025 12:54
@b-l-u-e b-l-u-e requested a review from rkrux December 22, 2025 12:55
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review re-ACK 7e468a6

git range-diff ac99af4...7e468a6

Thanks for accepting the nit suggestion.

@b-l-u-e b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 7e468a6 to 222139e Compare December 24, 2025 18:10
@b-l-u-e b-l-u-e requested review from achow101 and rkrux December 24, 2025 18:14
@rkrux
Copy link
Contributor

rkrux commented Dec 26, 2025

re-ACK 222139e

git range-diff 7e468a6...222139e

@b-l-u-e b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 222139e to 9f55be6 Compare December 27, 2025 17:34
Copy link
Contributor

@rkrux rkrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@b-l-u-e
Copy link
Author

b-l-u-e commented Jan 5, 2026

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 b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from 9f55be6 to e995c85 Compare January 5, 2026 14:57
@b-l-u-e b-l-u-e requested review from maflcko and rkrux January 5, 2026 15:03
@rkrux
Copy link
Contributor

rkrux commented Jan 5, 2026

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)?

@b-l-u-e
Copy link
Author

b-l-u-e commented Jan 5, 2026

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 9f55be6

Use PSBTInputSignedAndVerified instead of PSBTInputSigned to properly
validate signatures before marking PSBT as complete.
@b-l-u-e b-l-u-e force-pushed the fix-32849-descriptorprocesspsbt-internal-bug branch from e995c85 to 5a04a06 Compare January 10, 2026 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)

6 participants