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

Reviews

See the guideline for information on the review process.

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.

Can you share the fuzz test case that this fixes?

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

cHNidP8BADMzMzADAXQABPwBQUADAXQHAAAAHgAAACIAAAAAAAAAAAAaBwcHBwcFLQAAAAEBAAAA5AAAAQEfACJwAAD2AAAWABTUjtMRC9YAAAQE/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:

{
  "tx": {
    "txid": "566e1156d5f4152134d49dc08610e5b979e3de76fe5d5794d7de6f2cfc234661",
    "hash": "566e1156d5f4152134d49dc08610e5b979e3de76fe5d5794d7de6f2cfc234661",
    "version": 53490483,
    "size": 51,
    "vsize": 51,
    "weight": 204,
    "locktime": 14942208,
    "vin": [
      {
        "txid": "0707071a000000000000000000220000001e00000007740103404101fc040074",
        "vout": 755304199,
        "scriptSig": {
          "asm": "",
          "hex": ""
        },
        "sequence": 16842752
      }
    ],
    "vout": [
    ]
  },
  "global_xpubs": [
  ],
  "psbt_version": 0,
  "proprietary": [
  ],
  "unknown": {
  },
  "inputs": [
    {
      "witness_utxo": {
        "amount": 2704798.67781632,
        "scriptPubKey": {
          "asm": "0 d48ed3110bd600000404ff01480001040301fc01",
          "desc": "addr(bc1q6j8dxygt6cqqqpqyluq5sqqpqspsrlqp9t6tx9)#pdp4h8v0",
          "hex": "0014d48ed3110bd600000404ff01480001040301fc01",
          "address": "bc1q6j8dxygt6cqqqpqyluq5sqqpqspsrlqp9t6tx9",
          "type": "witness_v0_keyhash"
        }
      },
      "musig2_partial_sigs": [
        {
          "participant_pubkey": "03017445016c001c40027a87f8ff047a00626a01010101010b000f830164000400",
          "aggregate_pubkey": "0293946274ff010045426d420200001000000042350341740004fc01402800421d",
          "partial_sig": "fc74fc5252525252fc04fc014064030174000c7c014140740000001080000000"
        },
        {
          "participant_pubkey": "",
          "aggregate_pubkey": "03f9555428554d4141414109ee73ff001900001c320341740004fc01402800421d",
          "partial_sig": "fc74fc5252525252525252525252527400fc01000004ff01480001040301fc01"
        },
        {
          "participant_pubkey": "020174004e537438696f735f4f6173658f6274ff0024ec01fc7403e23000000000",
          "aggregate_pubkey": "",
          "partial_sig": "fc74fc52525252525252525252525252525252522b2b2b2b480001040301fc01"
        }
      ]
    }
  ],
  "outputs": [
  ],
  "fee": 2704798.67781632
}

ACK fc2245e

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

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

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.

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

crACK c383c3c

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. davidgumberg@a58ac09) or as a functional test case (e.g. davidgumberg@395ebfa). 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.

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

crACK d76ec4d

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.

ACK d76ec4d