psbt: MuSig2 Fields

achow101 commented at 6:22 pm on November 7, 2024: member

Implements un/serialization of MuSig2 PSBT fields and prepares PSBT to be able to sign for MuSig2 inputs.

Split from #29675

DrahtBot commented at 6:22 pm on November 7, 2024: 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/31247.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	fjahr, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21283 (Implement BIP 370 PSBTv2 by achow101)

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 marked this as ready for review on Nov 7, 2024

DrahtBot added the label PSBT on Nov 7, 2024

in src/psbt.h:726 in 925fdfd67f outdated

717@@ -672,6 +718,80 @@ struct PSBTInput
718                     UnserializeFromVector(s, m_tap_merkle_root);
719                     break;
720                 }
721+                case PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS:
722+                {
723+                    if (!key_lookup.emplace(key).second) {
724+                        throw std::ios_base::failure("Duplicate Key, input participant pubkeys for an aggregate key already provided");
725+                    } else if (key.size() != 34) {
726+                        throw std::ios_base::failure("Input musig2 participants pubkeys aggregate key is more than 33 bytes");

theStack commented at 5:35 pm on November 11, 2024:

error message nit:

0                        throw std::ios_base::failure("Input musig2 participants pubkeys aggregate key is not 34 bytes");

(assuming we are refering to the complete size of the key here, including the compact-size type prefix; we seem to do so on other key types like PSBT_IN_TAP_SCRIPT_SIG)

achow101 commented at 9:13 pm on November 14, 2024:

If i need to retouch.

achow101 commented at 0:02 am on January 21, 2025:

Done

theStack approved

theStack commented at 6:09 pm on November 11, 2024: contributor

Code-review ACK 8da5ab60b58b808d693d251c8605d53ae54ba617

I’ve prepared a branch for adding the MuSig2 key types to the test framework and adding a functional test that verifies the decodepsbt results on these, feel free to either pull in or ignore (will just open a separate PR after this one is merged then): https://github.com/theStack/bitcoin/tree/202411-test-add_musig2_decodepsbt_test

Sjors commented at 2:21 pm on November 26, 2024: member

Maybe add https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki to description (also in the tracking issue, for easier review).

theStack commented at 0:55 am on November 29, 2024: contributor

Opened a PR for BIP-373 which aims to improve the clarity of some PSBT fields containing pubkeys, which should hopefully also make reviewing this PR a bit easier: https://github.com/bitcoin/bips/pull/1705 (I sometimes had to look at the code first to find out how the specification is really to be interpreted…)

Sdawg84 approved

in src/rpc/rawtransaction.cpp:1376 in abb8228944 outdated

1371+                musig_keys.push_back(musig_part);
1372+            }
1373+            in.pushKV("musig2_participant_pubkeys", musig_keys);
1374+        }
1375+        if (!input.m_musig2_pubnonces.empty()) {
1376+            UniValue musig_pubnonces(UniValue::UniValue::VARR);

fjahr commented at 9:25 pm on January 20, 2025:

0            UniValue musig_pubnonces(UniValue::VARR);

achow101 commented at 0:02 am on January 21, 2025:

Fixed

in src/rpc/rawtransaction.cpp:1391 in abb8228944 outdated

1386+                }
1387+            }
1388+            in.pushKV("musig2_pubnonces", musig_pubnonces);
1389+        }
1390+        if (!input.m_musig2_partial_sigs.empty()) {
1391+            UniValue musig_partial_sigs(UniValue::UniValue::VARR);

fjahr commented at 9:25 pm on January 20, 2025:

0            UniValue musig_partial_sigs(UniValue::VARR);

achow101 commented at 0:02 am on January 21, 2025:

Fixed

in src/rpc/rawtransaction.cpp:930 in abb8228944 outdated

926@@ -927,6 +927,37 @@ const RPCResult decodepsbt_inputs{
927             }},
928             {RPCResult::Type::STR_HEX, "taproot_internal_key", /*optional=*/ true, "The hex-encoded Taproot x-only internal key"},
929             {RPCResult::Type::STR_HEX, "taproot_merkle_root", /*optional=*/ true, "The hex-encoded Taproot merkle root"},
930+            {RPCResult::Type::ARR, "musig2_participant_pubkeys", /*optional*/true, "",

fjahr commented at 9:50 pm on January 20, 2025:

formatting nit: Would prefer if this was consistently /*optional=*/ true like above

achow101 commented at 0:02 am on January 21, 2025:

Fixed

fjahr commented at 11:36 pm on January 20, 2025: contributor

utACK 8da5ab60b58b808d693d251c8605d53ae54ba617

Would have been nice to have tests here but fine for me to keep it for a follow-up since this is part of a larger project anyway.

achow101 force-pushed on Jan 21, 2025

fjahr commented at 12:36 pm on January 21, 2025: contributor

re-ACK ea53b468e2650dc6e5d6d996cc899846a5ffe901

Just addressed minor review comments.

DrahtBot requested review from theStack on Jan 21, 2025

theStack approved

theStack commented at 12:56 pm on January 21, 2025: contributor

re-ACK ea53b468e2650dc6e5d6d996cc899846a5ffe901

Verified that all review comments have been adressed since my previous ACK.

Sjors commented at 3:42 pm on January 21, 2025: member

Would have been nice to have tests here but fine for me to keep it for a follow-up

#29675 introduces wallet_musig.py which uses PSBT internally. IIUC all new fields are needed for MuSig2 to work at all, so they’re tested implicitly. Additionally the test inspects musig2_participant_pubkeys and musig2_partial_sigs, though not musig2_pubnonces.

That said, I think it would be better if this PR introduced at least some basic test coverage for e.g. the new errors that are introduced, and a simple round trip. This could be combined with generating test vectors for BIP 373.

Test vectors should probably include some PSBTv2 examples, which would have to go in a followup after #21283.

in src/psbt.h:738 in 433eca2993 outdated

733+                    std::vector<unsigned char> val;
734+                    s >> val;
735+                    SpanReader s_val{val};
736+                    while (!s_val.empty()) {
737+                        std::array<unsigned char, 33> part_key_bytes;
738+                        s_val >> AsWritableBytes(Span{part_key_bytes});

Sjors commented at 4:06 pm on January 21, 2025:

433eca29933447b70feba61fa26d3e6345fb3115: if s_val has less than 33 bytes remaining, this throws?

achow101 commented at 6:27 pm on January 21, 2025:

Added a check for the size.

Sjors commented at 9:21 am on January 22, 2025:

Pfew. Separate from this PR I think it would be good if we had a DSL that defines the PSBT format and the constraints for every possible field. Maybe @ryanofsky has ideas?

achow101 force-pushed on Jan 21, 2025

achow101 commented at 6:27 pm on January 21, 2025: member

Will add decoding test vectors.

psbt: Implement un/ser of musig2 fields 615056ac63

rpc: Include MuSig2 fields in decodepsbt 5afe9cdc9c

achow101 force-pushed on Jan 21, 2025

achow101 force-pushed on Jan 22, 2025

fjahr commented at 4:37 pm on January 22, 2025: contributor

Will add decoding test vectors.

Sorry, will you add them here and we should wait with our review or will you add them later in a follow-up like @Sjors suggested?

achow101 commented at 5:14 pm on January 22, 2025: member

Sorry, will you add them here and we should wait with our review or will you add them later in a follow-up like @Sjors suggested?

Will add them to this PR.

Marking as draft for now.

achow101 marked this as a draft on Jan 22, 2025

psbt: MuSig2 Fields #31247

Code Coverage & Benchmarks

Reviews

Conflicts