fuzz: assert we accept any PSBT serialization we create

darosior commented at 9:04 pm on March 3, 2026: member

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

DrahtBot commented at 9:04 pm on March 3, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, brunoerg, davidgumberg, achow101

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

Conflicts

No conflicts as of last run.

achow101 added this to the milestone 31.0 on Mar 3, 2026

in src/psbt.h:246 in fc2245ed2c

242@@ -243,7 +243,13 @@ void DeserializeMuSig2ParticipantDataIdentifier(Stream& skey, CPubKey& agg_pub,
243 
244     skey >> std::as_writable_bytes(std::span{part_pubkey_bytes}) >> std::as_writable_bytes(std::span{agg_pubkey_bytes});
245     agg_pub.Set(agg_pubkey_bytes.begin(), agg_pubkey_bytes.end());
246+    if (!agg_pub.IsValid()) {

achow101 commented at 9:15 pm on March 3, 2026:

Other places where we check pubkey validity in PSBTs use IsFullyValid rather than IsValid.

darosior commented at 9:17 pm on March 3, 2026:

I have intentionally preferred the lighter version because as far as i understand a non-fully-valid pubkey wouldn’t lead us to create an invalid PSBT serialization.

Happy to change for the sake of consistency, let me know what you think is best.

fanquake added the label Needs backport (30.x) on Mar 3, 2026

achow101 commented at 9:29 pm on March 3, 2026: member

Can you share the fuzz test case that this fixes?

darosior commented at 9:33 pm on March 3, 2026: member

The input that triggers the crash is already present in qa-assets, and therefore should be run in CI. Here is the corresponding PSBT:

0cHNidP8BADMzMzADAXQABPwBQUADAXQHAAAAHgAAACIAAAAAAAAAAAAaBwcHBwcFLQAAAAEBAAAA5AAAAQEfACJwAAD2AAAWABTUjtMRC9YAAAQE/wFIAAEEAwH8AUMcAgF0AE5TdDhpb3NfT2FzZY9idP8AJOwB/HQD4jAAAAAAgAABAAAAAAAAUgTiAgAAAAAAAEIzA0F0AAT8AUAoAEIdIPx0/FJSUlJSUlJSUlJSUlJSUlJSKysrK0gAAQQDAfwBQxwBCwAPgwFkAAAAAl2nYnT/AQBFQm1CbQEANiPf6vb//hcD+VVUKFVNQUFBQQnuc/8AGQAAHDIDQXQABPwBQCgAQh0g/HT8UlJSUlJSUlJSUlJSdAD8AQAABP8BSAABBAMB/AFDHAMBdEUBbAAcQAJ6h/j/BHoAYmoBAQEBAQsAD4MBZAAEAAKTlGJ0/wEARUJtQgIAABAAAABCNQNBdAAE/AFAKABCHSD8dPxSUlJSUvwE/AFAZAMBdAAMfAFBQHQAAAAQgAAAAAA=

Calling decodepsbt on it highlights the issue. Here is the output:

 0{
 1  "tx": {
 2    "txid": "566e1156d5f4152134d49dc08610e5b979e3de76fe5d5794d7de6f2cfc234661",
 3    "hash": "566e1156d5f4152134d49dc08610e5b979e3de76fe5d5794d7de6f2cfc234661",
 4    "version": 53490483,
 5    "size": 51,
 6    "vsize": 51,
 7    "weight": 204,
 8    "locktime": 14942208,
 9    "vin": [
10      {
11        "txid": "0707071a000000000000000000220000001e00000007740103404101fc040074",
12        "vout": 755304199,
13        "scriptSig": {
14          "asm": "",
15          "hex": ""
16        },
17        "sequence": 16842752
18      }
19    ],
20    "vout": [
21    ]
22  },
23  "global_xpubs": [
24  ],
25  "psbt_version": 0,
26  "proprietary": [
27  ],
28  "unknown": {
29  },
30  "inputs": [
31    {
32      "witness_utxo": {
33        "amount": 2704798.67781632,
34        "scriptPubKey": {
35          "asm": "0 d48ed3110bd600000404ff01480001040301fc01",
36          "desc": "addr(bc1q6j8dxygt6cqqqpqyluq5sqqpqspsrlqp9t6tx9)#pdp4h8v0",
37          "hex": "0014d48ed3110bd600000404ff01480001040301fc01",
38          "address": "bc1q6j8dxygt6cqqqpqyluq5sqqpqspsrlqp9t6tx9",
39          "type": "witness_v0_keyhash"
40        }
41      },
42      "musig2_partial_sigs": [
43        {
44          "participant_pubkey": "03017445016c001c40027a87f8ff047a00626a01010101010b000f830164000400",
45          "aggregate_pubkey": "0293946274ff010045426d420200001000000042350341740004fc01402800421d",
46          "partial_sig": "fc74fc5252525252fc04fc014064030174000c7c014140740000001080000000"
47        },
48        {
49          "participant_pubkey": "",
50          "aggregate_pubkey": "03f9555428554d4141414109ee73ff001900001c320341740004fc01402800421d",
51          "partial_sig": "fc74fc5252525252525252525252527400fc01000004ff01480001040301fc01"
52        },
53        {
54          "participant_pubkey": "020174004e537438696f735f4f6173658f6274ff0024ec01fc7403e23000000000",
55          "aggregate_pubkey": "",
56          "partial_sig": "fc74fc52525252525252525252525252525252522b2b2b2b480001040301fc01"
57        }
58      ]
59    }
60  ],
61  "outputs": [
62  ],
63  "fee": 2704798.67781632
64}

achow101 commented at 10:02 pm on March 3, 2026: member

ACK fc2245ed2c29cf2cf12249435034aead52e8a9ce

fanquake commented at 10:58 am on March 4, 2026: member

Actually, is this a duplicate of #34219? Seems like that got review, but just stalled?

darosior commented at 3:00 pm on March 4, 2026: member

Oh i had missed it! I just pushed that as a separate PR while i was reviewing #21283.

The fix is virtually the same (as per #34725 (review) i don’t think IsFullyValid is necessary, but it’s not important). However i think the fuzz harness patch here is important. The tests in #34219 are specific to this regression, but we should be testing that more generally we never produce a serialization we do not accept.

How about merging/backporting #34219 then i rebase this PR with only the fuzz harness commit, that does not necessarily need backporting?

fanquake commented at 3:15 pm on March 4, 2026: member

How about merging/backporting #34219 then i rebase this PR with only the fuzz harness commit, that does not necessarily need backporting?

That seems fine.

fanquake removed the label Needs backport (30.x) on Mar 4, 2026

in src/test/fuzz/psbt.cpp:40 in fc2245ed2c outdated

32@@ -33,6 +33,12 @@ FUZZ_TARGET(psbt)
33     }
34     const PartiallySignedTransaction psbt = psbt_mut;
35 
36+    // A PSBT must roundtrip.
37+    PartiallySignedTransaction psbt_roundtrip;
38+    std::vector<uint8_t> bytes_roundtrip;
39+    VectorWriter{bytes_roundtrip, 0, psbt};
40+    SpanReader{bytes_roundtrip} >> psbt_roundtrip;

dergoegge commented at 3:36 pm on March 4, 2026:

This tests that a psbt will serialize and deserialize but it won’t check (unless i’m missing something) that the result is the same as before (i.e. assert(psbt == psbt_roundtrip)). I’m guessing that is intended here (because it’s not a property that holds?), but wanted to mention it just in case.

darosior commented at 4:05 pm on March 4, 2026:

Indeed. I would have liked to check that invariant, but the PSBT class has no operator==. We could have a more convoluted way of checking it, but #21283 introduces a GetUniqueID method that i’ve been using in this target there, so i think it’s better to wait for that.

darosior commented at 4:06 pm on March 4, 2026:

(Note GetUniqueID is still imperfect because it is not a comprehensive check of all the PSBT metadata, but i figured it was a reasonable balance between complexity and completeness of the check.)

dergoegge commented at 4:44 pm on March 4, 2026:

Perhaps alternatively we could do assert(serialize(psbt) == serialize(deserialize(serialize(psbt))));?

darosior commented at 4:55 pm on March 4, 2026:

Sure, done.

darosior force-pushed on Mar 4, 2026

darosior renamed this:
~~Fix a PSBT serialization roundtrip issue~~
fuzz: assert we accept any PSBT serialization we create
on Mar 4, 2026

DrahtBot added the label Fuzzing on Mar 4, 2026

darosior commented at 4:17 pm on March 4, 2026: member

Rebased after #34219 was merged, to only contain the patch to the fuzzing harness.

davidgumberg commented at 4:51 pm on March 4, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/pull/34725/commits/c383c3c03af196a62f425dee7877c94109426858

Feel free to disregard as orthogonal or insignificant, but I think it would be valuable to also add a (programmatic) test for the particular case addressed in #34219 / in this PR prior to rebase either as a unit test (e.g. https://github.com/davidgumberg/bitcoin/commit/a58ac094c16ec64bb740eed2ec547934e24eddaf) or as a functional test case (e.g. https://github.com/davidgumberg/bitcoin/commit/395ebfa8ba52585f9fb102a102fbfccb0666adde). While redundant with the cases added in #34219 to ‎test/functional/data/rpc_psbt.json, I think tables of raw PSBT’s are not trivial for a reader to review / comprehend.

DrahtBot requested review from achow101 on Mar 4, 2026

darosior force-pushed on Mar 4, 2026

DrahtBot added the label CI failed on Mar 4, 2026

fuzz: make sure PSBT serialization roundtrips

This will prevent us from creating a serialization we do not accept
going forward.

d76ec4de14

dergoegge approved

dergoegge commented at 4:58 pm on March 4, 2026: member

utACK d76ec4de148724e6b384efb0a4180722ed30db10

DrahtBot requested review from davidgumberg on Mar 4, 2026

darosior commented at 5:03 pm on March 4, 2026: member

@davidgumberg that feels really redundant, so i’m not too keen. Especially as it would still be specific to this very instance of the roundtrip bug, and not be able to catch others. The fuzz harness modification in this PR is also another test for the bug fixed in #34219, which i hope is easier for a reader to review / comprehend than the raw PSBT added there.

DrahtBot removed the label CI failed on Mar 4, 2026

brunoerg approved

brunoerg commented at 6:51 pm on March 4, 2026: contributor

code review ACK d76ec4de148724e6b384efb0a4180722ed30db10

davidgumberg commented at 7:14 pm on March 4, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/d76ec4de148724e6b384efb0a4180722ed30db10

Ran fuzzing for ~20 minutes with no issues.

Especially as it would still be specific to this very instance of the roundtrip bug, and not be able to catch others.

That’s what I intended, but I’m satisfied that checking roundtripping is very likely to catch any issue related to deserializing a key that fails CPubKey::IsValid() since ‘IsValidity’ and ‘Serializeability’ happen to be the same for pubkeys. Coverage will still be missing (I think) for pubkeys that are not points but have valid length prefixes, but I’m inclined to agree with your comment above that IsFullyValid() is overkill, so probably fine to remain untested.

achow101 commented at 9:34 pm on March 4, 2026: member

ACK d76ec4de148724e6b384efb0a4180722ed30db10

achow101 merged this on Mar 4, 2026

achow101 closed this on Mar 4, 2026

darosior deleted the branch on Mar 4, 2026

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

Reviews

Conflicts