psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization by tboy1337 · Pull Request #34219 · bitcoin/bitcoin

tboy1337 · 2026-01-07T12:41:30Z

The previous fix for invalid MuSig2 pubkeys (#34010) only
addressed the PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS field. However, the
PSBT_IN_MUSIG2_PUB_NONCE and PSBT_IN_MUSIG2_PARTIAL_SIG fields also
deserialize pubkeys without validation, which could lead to crashes when
invalid pubkeys are processed.

This commit adds validation to the DeserializeMuSig2ParticipantDataIdentifier
function to ensure all pubkeys in MuSig2 pubnonce and partial signature
fields are fully valid elliptic curve points.

The fix:

Validates both aggregate and participant pubkeys in MuSig2 pubnonce and
partial signature deserialization
Throws std::ios_base::failure with descriptive error messages for invalid
pubkeys
Prevents potential crashes from invalid elliptic curve points
Maintains backward compatibility for valid PSBTs

This completes the fix for issues #33999 and #34201.

DrahtBot · 2026-01-07T12:41:39Z

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/34219.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, w0xlt, darosior

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34725 (Fix a PSBT serialization roundtrip issue by darosior)

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.

maflcko · 2026-01-07T15:34:00Z

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

Also, you'll have to add tests

test/functional/rpc_psbt.py

src/psbt.h

src/test/psbt_tests.cpp

maflcko · 2026-01-08T12:23:54Z

Your added function test passes even before your changes, so this is not a regression test for the code change. Likely, it doesn't add any test coverage. All of this looks LLM generated.

If you want to add tests, that is fine, but you'll have to explain and show what coverage they are adding.

If you want to fix something, this is fine, but you'll have to add a regression test that fails before the changes, and passes after them: https://en.wikipedia.org/wiki/Regression_testing

rkrux

Concept ACK 6a1e362

Thanks for raising this PR, these checks for the aggregate and participant pubkeys in the MuSig2 pubnonce and partial signatures PSBT keys seem fine to me.
BIP Reference: https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki

In the absence of this fix, such invalid PSBTs get parsed successfully with empty participant and aggregate pubkeys that might later cause some flow (eg: signing) to crash like the earlier fuzz crashes found out.

I used the following code to generate cases that I have suggested in the inline comments.

Makeshift code to generate invalid PSBTs

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index cc443f6a7c..9d30d571ff 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -407,6 +407,31 @@ class PSBTTest(BitcoinTestFramework):
         self.log.info("PSBT parameter handling test completed successfully")
 
     def run_test(self):
+        encoded_psbts = [
+            "",
+        ]
+        old_key = None
+        value = None
+        new_key = None
+        for encoded in encoded_psbts:
+            decoded = PSBT.from_base64(encoded)
+            for k, v in decoded.i[0].map.items():
+                if type(k) == bytes and k.hex().startswith('1b'):
+                    old_key = k
+                    value = v
+
+                if old_key:
+                    hex = old_key.hex()
+                    new_hex = hex[0:2] + randbytes(33).hex() + hex[68:]
+                    new_key = bytes.fromhex(new_hex)
+                    break
+
+            del decoded.i[0].map[old_key]
+            decoded.i[0].map[new_key] = value
+            invalid_psbt = decoded.to_base64()
+            print(invalid_psbt)
+        return
+
         # Create and fund a raw tx for sending 10 BTC
         psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']

test/functional/rpc_psbt.py

src/wallet/test/psbt_wallet_tests.cpp

Add validation for pubkeys in MuSig2 pubnonce and partial signature deserialization to prevent crashes with invalid curve points. - Validate aggregate and participant pubkeys in PSBT MuSig2 fields - Add comprehensive test coverage for invalid pubkey rejection - Ensure proper error handling during PSBT deserialization

rkrux

lgtm ACK f51665b

Thanks for incorporating the suggestions, I verified the added rpc_psbt.json test cases are indeed those that I suggested in #34219 (comment).

w0xlt

ACK f51665b

darosior

utACK f51665b

fanquake · 2026-03-04T15:19:16Z

Backported to 30.x in #34689.

Add validation for pubkeys in MuSig2 pubnonce and partial signature deserialization to prevent crashes with invalid curve points. - Validate aggregate and participant pubkeys in PSBT MuSig2 fields - Add comprehensive test coverage for invalid pubkey rejection - Ensure proper error handling during PSBT deserialization Github-Pull: bitcoin#34219 Rebased-From: f51665b

darosior · 2026-03-04T21:24:52Z

For what it's worth another argument against IsFullyValid is that it needlessly slows down discovery of interesting inputs in fuzzing. Both because the check takes longer, but more importantly because it's a cryptographic check that's harder to pass.

d76ec4d fuzz: make sure PSBT serialization roundtrips (Antoine Poinsot) Pull request description: ~~Invalid public keys were accepted in Musig2 partial signatures. Because we serialize invalid keys as the empty byte string, this would lead us to creating an invalid PSBT serializations.~~ ~~This can be checked by reverting the first commit with the fix and simply running the target against the existing qa-assets corpus for the `psbt` harness.~~ This patch found the issue fixed in #34219 with a single run against the existing qa-assets corpus. It is useful to make sure there are no similar bugs, and we don't introduce roundtrip regressions outside of the specifc instance of accepting invalid public keys in Musig2 fields. *(Edited on March 4 to only contain the fuzz harness patch)* ACKs for top commit: davidgumberg: crACK d76ec4d achow101: ACK d76ec4d dergoegge: utACK d76ec4d brunoerg: code review ACK d76ec4d Tree-SHA512: ab5f8d4e6a1781ecdef825e1a0e2793a6b553f36c923a4a35cb1af4070eead9d9780f6cc9a76235aa03462e52a129d15e61f631490b43651dc4395f3f1c005f3

Add validation for pubkeys in MuSig2 pubnonce and partial signature deserialization to prevent crashes with invalid curve points. - Validate aggregate and participant pubkeys in PSBT MuSig2 fields - Add comprehensive test coverage for invalid pubkey rejection - Ensure proper error handling during PSBT deserialization Github-Pull: bitcoin#34219 Rebased-From: f51665b

49a777d doc: update release notes for v30.x (fanquake) 0f9e08f doc: update build guides pre v31 (fanquake) 597ac36 doc: Fix `fee` field in `getblock` RPC result (nervana21) 47ed306 depends: Allow building Qt packages after interruption (Hennadii Stepanov) d221d1c psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization (tboy1337) e1210ac doc: Improve dependencies.md IPC documentation (Ryan Ofsky) c17a5cd test: Add missing timeout_factor to zmq socket (MarcoFalke) 3042509 netif: fix compilation warning in QueryDefaultGatewayImpl() (MarcoFalke) 475a5b0 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke) 7220ee3 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke) Pull request description: Backports: * #34093 * #34219 * #34597 * #34690 * #34702 * #34706 * #34713 * #34789 ACKs for top commit: marcofleon: ACK 49a777d Tree-SHA512: b4ce54860b7306b22de75bb093ad574110875253e4ea3ca96a736809c8291dea1144a617c8791f36618d8e367022709ba5cf84ca0e450ef6d76394ab80f22e2f

DrahtBot added the PSBT label Jan 7, 2026

tboy1337 force-pushed the fix-musig2-validation-incomplete branch 2 times, most recently from 2733f19 to 8fde87b Compare January 7, 2026 21:22

w0xlt reviewed Jan 7, 2026

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

w0xlt reviewed Jan 7, 2026

View reviewed changes

src/psbt.h Outdated Show resolved Hide resolved

w0xlt reviewed Jan 7, 2026

View reviewed changes

src/test/psbt_tests.cpp Outdated Show resolved Hide resolved

tboy1337 force-pushed the fix-musig2-validation-incomplete branch from 2d51b97 to d8f711d Compare January 7, 2026 21:52

tboy1337 requested a review from w0xlt January 7, 2026 21:53

tboy1337 force-pushed the fix-musig2-validation-incomplete branch from 7392655 to 6a1e362 Compare January 8, 2026 18:14

rkrux reviewed Jan 9, 2026

View reviewed changes

DrahtBot added the CI failed label Jan 11, 2026

tboy1337 force-pushed the fix-musig2-validation-incomplete branch from b623fda to f51665b Compare January 12, 2026 12:51

tboy1337 requested a review from rkrux January 12, 2026 12:52

DrahtBot removed the CI failed label Jan 13, 2026

rkrux approved these changes Jan 20, 2026

View reviewed changes

w0xlt reviewed Jan 24, 2026

View reviewed changes

DrahtBot mentioned this pull request Mar 4, 2026

fuzz: assert we accept any PSBT serialization we create #34725

Merged

darosior approved these changes Mar 4, 2026

View reviewed changes

achow101 added this to the 31.0 milestone Mar 4, 2026

fanquake merged commit e09b816 into bitcoin:master Mar 4, 2026
27 checks passed

fanquake mentioned this pull request Mar 4, 2026

[30.x] Backports #34689

Merged

Conversation

tboy1337 commented Jan 7, 2026

Uh oh!

DrahtBot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko commented Jan 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 8, 2026

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Mar 4, 2026

Uh oh!

darosior commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Jan 7, 2026 •

edited

Loading

rkrux left a comment •

edited

Loading