psbt: MuSig2 Fields #31247

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:musig2-psbt changing 3 files +319 −1
  1. 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

  2. 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
    ACK fjahr, theStack, rkrux

    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:

    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
    • #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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    1. Change
      “Input musig2 pubnonce key is not expected size of 67 or 99 bytes”
      to
      “Input musig2 pubnonce key is not the expected size of 67 or 99 bytes”
    2. Change
      “Input musig2 partial sig key is not expected size of 67 or 99 bytes”
      to
      “Input musig2 partial sig key is not the expected size of 67 or 99 bytes”
    3. In RPCResult descriptions, change
      “The compressed aggregate public key for which this pubnonce is for.”
      to
      “The compressed aggregate public key for which this pubnonce is.”
    4. Similarly, change
      “The compressed aggregate public key for which this partial signature is for.”
      to
      “The compressed aggregate public key for which this partial signature is.”
  3. achow101 marked this as ready for review on Nov 7, 2024
  4. DrahtBot added the label PSBT on Nov 7, 2024
  5. 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
  6. theStack approved
  7. 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

  8. 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).
  9. 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…)
  10. Sdawg84 approved
  11. 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
  12. 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
  13. 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
  14. 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.

  15. achow101 force-pushed on Jan 21, 2025
  16. fjahr commented at 12:36 pm on January 21, 2025: contributor

    re-ACK ea53b468e2650dc6e5d6d996cc899846a5ffe901

    Just addressed minor review comments.

  17. DrahtBot requested review from theStack on Jan 21, 2025
  18. theStack approved
  19. 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.

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

  21. 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?
  22. achow101 force-pushed on Jan 21, 2025
  23. achow101 commented at 6:27 pm on January 21, 2025: member
    Will add decoding test vectors.
  24. achow101 force-pushed on Jan 21, 2025
  25. achow101 force-pushed on Jan 22, 2025
  26. 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?

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

  28. achow101 marked this as a draft on Jan 22, 2025
  29. achow101 force-pushed on Feb 12, 2025
  30. achow101 marked this as ready for review on Feb 12, 2025
  31. achow101 commented at 0:44 am on February 12, 2025: member
    Updated with the test vectors from https://github.com/bitcoin/bips/pull/1764
  32. achow101 force-pushed on Feb 12, 2025
  33. murchandamus commented at 2:22 pm on February 14, 2025: contributor
    sorry, wrong window.
  34. DrahtBot requested review from theStack on Feb 14, 2025
  35. DrahtBot requested review from fjahr on Feb 14, 2025
  36. theStack approved
  37. theStack commented at 4:28 pm on February 17, 2025: contributor

    re-ACK 7064b1a45a54f4fae2cb390a444c8eca27410c88

    Didn’t check the individual test vectors in detail though; from what I can see the two corrections in the BIP (https://github.com/bitcoin/bips/pull/1769) don’t affect this PR.

  38. fjahr commented at 8:47 pm on February 17, 2025: contributor

    I compared the test vectors here to those in the BIP and I couldn’t find the “Spend of a Taproot output where a key in a script is a MuSig2 Aggregate Pubkey - With participant pubkeys only” case here.

    e.g. this Base64 String

    0cHNidP8BAFICAAAAAZqLSlB5a5YAmQ9/4R36ALxw79KWBIr8hnGa8PsfqRk3AQAAAAD9////ARjd9QUAAAAAFgAUyRI+BujX8JZsXRzQ+TMALU63V80AAAAAAAEBKwDh9QUAAAAAIlEgVr20gbTWcQP21d6oqar9NoSmp5tKLiR3mduNSxqG4fgiFcBQkpt0waBJVLeLS2A16XpeB4paDyjsltVHv+6azoA6wCMgC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9SswCEWC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9QlAbEf7apjoJVlAacwjJO1Y3Nx52E9m4reF4PUnibAbPosJoDdbiEWNGuZWTNXEHydNFnp3rqNPq9E5mNshcf4U+uQulLozQAlAbEf7apjoJVlAacwjJO1Y3Nx52E9m4reF4PUnibAbPosWAsIhyEWT6/WX4FpGG/Cv9siM8d+Yw0QvigKJMcWXAmidhF3XCwlAbEf7apjoJVlAacwjJO1Y3Nx52E9m4reF4PUnibAbPoswySagiEWUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsAFAHxGHl0hFvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5JQGxH+2qY6CVZQGnMIyTtWNzcedhPZuK3heD1J4mwGz6LH3WVZIBFyBQkpt0waBJVLeLS2A16XpeB4paDyjsltVHv+6azoA6wAEYILEf7apjoJVlAacwjJO1Y3Nx52E9m4reF4PUnibAbPosIhoDC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9RjAjRrmVkzVxB8nTRZ6d66jT6vROZjbIXH+FPrkLpS6M0AAk+v1l+BaRhvwr/bIjPHfmMNEL4oCiTHFlwJonYRd1wsAvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5AAA=
    

    Am I missing something or did this one get lost?

  39. achow101 force-pushed on Feb 17, 2025
  40. achow101 commented at 8:58 pm on February 17, 2025: member

    Am I missing something or did this one get lost?

    Looks like it got lost. Added it now.

  41. fjahr commented at 9:25 pm on February 17, 2025: contributor

    re-ACK debacd1f59641c1e9e268c20820f4c8f0fc583f7

    Confirmed all the BIP373 test vectors are present in the PR.

  42. DrahtBot requested review from theStack on Feb 17, 2025
  43. theStack approved
  44. theStack commented at 10:05 pm on February 17, 2025: contributor

    re-ACK debacd1f59641c1e9e268c20820f4c8f0fc583f7

    Only a missing valid test vector has been added since my previous ACK.

  45. rkrux commented at 3:25 pm on March 10, 2025: contributor
    I’ve started reviewing this PR and will leave a review soon in the coming days.
  46. in src/psbt.h:272 in 642ecd9653 outdated
    220@@ -217,6 +221,11 @@ struct PSBTInput
    221     XOnlyPubKey m_tap_internal_key;
    222     uint256 m_tap_merkle_root;
    223 
    224+    // MuSig2 fields
    225+    std::map<CPubKey, std::vector<CPubKey>> m_musig2_participants;
    226+    std::map<std::pair<CPubKey, uint256>, std::map<CPubKey, std::vector<uint8_t>>> m_musig2_pubnonces;
    227+    std::map<std::pair<CPubKey, uint256>, std::map<CPubKey, uint256>> m_musig2_partial_sigs;
    


    rkrux commented at 12:19 pm on March 11, 2025:

    I think these 2 can be benefited with some documentation something like below. It was only after I read the below serialisation code that I understood this type.

    0/** key is a pair of aggregate public key and leaf hash, value is a map of participant pubkey to pubnonce */
    1std::map<std::pair<CPubKey, uint256>, std::map<CPubKey, std::vector<uint8_t>>> m_musig2_pubnonces;
    2/** key is a pair of aggregate public key and leaf hash, value is a map of participant pubkey to partial sig */
    3std::map<std::pair<CPubKey, uint256>, std::map<CPubKey, uint256>> m_musig2_partial_sigs;
    

    rkrux commented at 12:30 pm on March 11, 2025:

    IIUC, these types won’t be able to avoid collision in case the same aggregate public key appears twice while spending a script in the script path spend like in the below test (pubkey[1] appears twice in the leaf script, zero_fn script can be ignored for this case). I’m assuming for the same aggregate pubkey appearing multiple times in a script, there can be different signatures (because it appears just like any single-sig) for which there could be separate pubnonces. In the current structure, only the last one read would be stored in the structure while the previous ones overridden.

    I don’t think it is a big issue though because I am unable to see any benefit of putting the same aggregate public key multiple times in a leaf script. PLMK if I am missing something and/or if this is not a valid scenario to consider.

    https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/test/functional/feature_taproot.py#L1186-L1187


    achow101 commented at 9:50 pm on April 10, 2025:
    If a key appears multiple times in the same script, then the same signature can be used in both places. Deduplication is not required.

    achow101 commented at 10:32 pm on April 10, 2025:
    Added comments.
  47. in src/psbt.h:364 in 642ecd9653 outdated
    359+            }
    360+
    361+            // Write MuSig2 pubnonces
    362+            for (const auto& [agg_key_leaf_hash, pubnonces] : m_musig2_pubnonces) {
    363+                const auto& [agg_key, leaf_hash] = agg_key_leaf_hash;
    364+                for (const auto& [pubkey, pubnonce] : pubnonces) {
    


    rkrux commented at 12:31 pm on March 11, 2025:

    Nit for verbosity here and in the partial sigs loop below.

    0-for (const auto& [pubkey, pubnonce] : pubnonces) {
    1+for (const auto& [participant_pubkey, pubnonce] : pubnonces) {
    

    achow101 commented at 10:32 pm on April 10, 2025:
    Done
  48. in src/psbt.h:736 in 642ecd9653 outdated
    731+
    732+                    std::vector<CPubKey> participants;
    733+                    std::vector<unsigned char> val;
    734+                    s >> val;
    735+                    SpanReader s_val{val};
    736+                    while (s_val.size() >= 33) {
    


    rkrux commented at 12:33 pm on March 11, 2025:

    Here and in other similar occurrences of 33.

    0-while (s_val.size() >= 33) {
    1+while (s_val.size() >= CPubKey::COMPRESSED_SIZE) {
    

    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  49. in src/psbt.h:725 in 642ecd9653 outdated
    717@@ -672,6 +718,86 @@ 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) {
    


    rkrux commented at 12:41 pm on March 11, 2025:
    Using CPubKey::COMPRESSED_SIZE + 1 here instead of 34 would make it more explicit and reader-friendly (similarly in the other key unserialization flows below) like done for PSBT_IN_PARTIAL_SIG and PSBT_IN_RIPEMD160 cases previously.

    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  50. in src/psbt.h:752 in 642ecd9653 outdated
    747+                }
    748+                case PSBT_IN_MUSIG2_PUB_NONCE:
    749+                {
    750+                    if (!key_lookup.emplace(key).second) {
    751+                        throw std::ios_base::failure("Duplicate Key, input musig2 pubnonce already provided");
    752+                    } else if (key.size() != 67 && key.size() != 99) {
    


    rkrux commented at 12:57 pm on March 11, 2025:

    Maybe?

    (2 * CPubKey::COMPRESSED_SIZE + 1) (2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1)


    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  51. in src/psbt.h:780 in 642ecd9653 outdated
    775+                }
    776+                case PSBT_IN_MUSIG2_PARTIAL_SIG:
    777+                {
    778+                    if (!key_lookup.emplace(key).second) {
    779+                        throw std::ios_base::failure("Duplicate Key, input musig2 partial sig already provided");
    780+                    } else if (key.size() != 67 && key.size() != 99) {
    


    rkrux commented at 1:01 pm on March 11, 2025:
    Since it is being reused now, maybe a constant or an enum can also be an option.
  52. in src/rpc/rawtransaction.cpp:916 in 8b46f256b7 outdated
    915@@ -916,6 +916,37 @@ const RPCResult decodepsbt_inputs{
    916             }},
    


    rkrux commented at 1:03 pm on March 11, 2025:
    I would be in favour of using compressed terminology here over plain like proposed in the BIP change here: https://github.com/bitcoin/bips/pull/1705

    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  53. in src/psbt.h:351 in 642ecd9653 outdated
    345@@ -337,6 +346,43 @@ struct PSBTInput
    346                 SerializeToVector(s, PSBT_IN_TAP_MERKLE_ROOT);
    347                 SerializeToVector(s, m_tap_merkle_root);
    348             }
    349+
    350+            // Write MuSig2 Participants
    351+            for (const auto& [agg_key, part_pubs] : m_musig2_participants) {
    


    rkrux commented at 1:11 pm on March 11, 2025:

    agg_pubkey?

    Using only key alludes to the private key though in this case there is no aggregate private key but still to be consistent with the rest of the codebase and to get rid of any doubt in the mind of the reader.


    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  54. in src/rpc/rawtransaction.cpp:1360 in 8b46f256b7 outdated
    1355+                UniValue part_keys(UniValue::VARR);
    1356+                for (const auto& pub : parts) {
    1357+                    part_keys.push_back(HexStr(pub));
    1358+                }
    1359+                musig_part.pushKV("participant_pubkeys", part_keys);
    1360+                musig_keys.push_back(musig_part);
    


    rkrux commented at 1:41 pm on March 11, 2025:

    _pubkeys instead of _keys

    In order to be consistent with the codebase


    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  55. in src/psbt.h:753 in 642ecd9653 outdated
    748+                case PSBT_IN_MUSIG2_PUB_NONCE:
    749+                {
    750+                    if (!key_lookup.emplace(key).second) {
    751+                        throw std::ios_base::failure("Duplicate Key, input musig2 pubnonce already provided");
    752+                    } else if (key.size() != 67 && key.size() != 99) {
    753+                        throw std::ios_base::failure("Input musig2 pubnonce key is not expected size");
    


    rkrux commented at 1:44 pm on March 11, 2025:
    Maybe mention the expected size in the message: ... is not expected size of either 67 or 99 bytes

    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  56. in src/psbt.h:1069 in 642ecd9653 outdated
    1069+                        throw std::ios_base::failure("Output musig2 participants pubkeys value size is not a multiple of 33");
    1070+                    }
    1071+
    1072+                    m_musig2_participants.emplace(agg_key, participants);
    1073+                    break;
    1074+                }
    


    rkrux commented at 1:52 pm on March 11, 2025:
    This block is almost same as the corresponding input block, even the m_musig2_participants is reused. Only the key type and the error message differ slightly. Can be extracted into a function and reused in both input and output block.

    achow101 commented at 10:33 pm on April 10, 2025:
    Done
  57. rkrux commented at 2:09 pm on March 11, 2025: contributor

    Partial review.

    Some of the suggested comments I have not categorised as nits so that the newer changes are consistent with the rest of the wallet codebase. However, I’m open to them not being implemented if the PR author and other reviewers believe so.

  58. DrahtBot added the label CI failed on Mar 21, 2025
  59. DrahtBot commented at 5:14 pm on March 21, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37362505493

    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.

  60. achow101 force-pushed on Apr 10, 2025
  61. DrahtBot removed the label CI failed on Apr 11, 2025
  62. DrahtBot added the label CI failed on Apr 11, 2025
  63. DrahtBot removed the label CI failed on Apr 12, 2025
  64. achow101 force-pushed on Apr 14, 2025
  65. in src/psbt.h:204 in 10cefa7f20 outdated
    196@@ -193,6 +197,30 @@ void SerializeHDKeypaths(Stream& s, const std::map<CPubKey, KeyOriginInfo>& hd_k
    197     }
    198 }
    199 
    200+// Deserialize a PSBT_{IN/OUT}_MUSIG2_PARTICIPANT_PUBKEYS field
    201+template<typename Stream>
    202+void DeserializeMuSig2ParticipantPubkeys(Stream& s, SpanReader& skey, std::map<CPubKey, std::vector<CPubKey>>& out, std::string context)
    203+{
    204+    std::array<unsigned char, 33> agg_pubkey_bytes;
    


    rkrux commented at 2:05 pm on April 17, 2025:

    I realise that my previous review didn’t mention to remove all instances clearly, though that’s what I had in mind.

    There are few instances of 33 and 34 left, I feel now that we are using CPubKey::COMPRESSED_SIZE in few places already, we might as well use it in the whole commit. I don’t see a reason to keep remaining instances of hardcoded values here.


    achow101 commented at 8:15 pm on April 17, 2025:
    Done
  66. in src/psbt.h:802 in 10cefa7f20 outdated
    797+                    CPubKey agg_pub{std::span{agg_pubkey_bytes}};
    798+                    CPubKey part_pub{std::span{part_pubkey_bytes}};
    799+
    800+                    if (!skey.empty()) {
    801+                        skey >> leaf_hash;
    802+                    }
    


    rkrux commented at 2:09 pm on April 17, 2025:

    This portion is exactly same as the one used in the nonces case, can be extracted into a DeserializeMuSig2PartialSigOrPubNonceKey function because the format of the key is same in both the cases.

    Though the duplication is not enough but the deduplicated version would enforce the similarity of the key format to the reader easily.


    achow101 commented at 8:15 pm on April 17, 2025:
    Done
  67. in src/psbt.h:789 in 10cefa7f20 outdated
    773+                        skey >> leaf_hash;
    774+                    }
    775+
    776+                    std::vector<uint8_t> pubnonce;
    777+                    s >> pubnonce;
    778+                    if (pubnonce.size() != 66) {
    


    rkrux commented at 2:14 pm on April 17, 2025:
    To get rid of 66 here, I wouldn’t use 2 * CPubKey::COMPRESSED_SIZE and instead I believe a constant for nonce size would be more suitable that could be used later in this commit (part of draft PR) as well: https://github.com/bitcoin/bitcoin/pull/29675/commits/59612a514e348c2c3a8d2fbb82ac8245b139febc

    achow101 commented at 8:12 pm on April 17, 2025:
    I will leave this as is as there is no pubnonce object to tie the size to.
  68. in src/rpc/rawtransaction.cpp:1501 in 6e50e593e2 outdated
    1496+                UniValue part_pubkeys(UniValue::VARR);
    1497+                for (const auto& pub : parts) {
    1498+                    part_pubkeys.push_back(HexStr(pub));
    1499+                }
    1500+                musig_part.pushKV("participant_pubkeys", part_pubkeys);
    1501+                musig_pubkeys.push_back(musig_part);
    


    rkrux commented at 2:19 pm on April 17, 2025:
    This portion is completely same as for the inputs one but the whole if block can be deduplicated by using passing input/output & in/out as args to common function.

    achow101 commented at 8:11 pm on April 17, 2025:
    I don’t think deduplicating this is really all that useful nor makes it easier to read.
  69. in src/rpc/rawtransaction.cpp:935 in 6e50e593e2 outdated
    930+            {RPCResult::Type::ARR, "musig2_pubnonces", /*optional=*/true, "",
    931+            {
    932+                {RPCResult::Type::OBJ, "", "",
    933+                {
    934+                    {RPCResult::Type::STR_HEX, "participant_pubkey", "The compressed public key of the participant that created this pubnonce."},
    935+                    {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is for."},
    


    rkrux commented at 2:21 pm on April 17, 2025:

    Nit here and in the musig2_partial_sigs section below.

    0- {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is for."},
    1+ {RPCResult::Type::STR_HEX, "aggregate_pubkey", "The compressed aggregate public key for which this pubnonce is."},
    

    achow101 commented at 8:11 pm on April 17, 2025:
    This suggestion is not grammatically correct and doesn’t make sense. The original is fine.

    sipa commented at 6:12 pm on April 18, 2025:

    Nitty nit: I think both the original and the suggestion are wrong.

    “The compressed aggregate public key for which this pubnonce is for.”

  70. rkrux commented at 2:46 pm on April 17, 2025: contributor

    I had verified the implementation with the spec mentioned in BIP 373 & had navigated through the data stream serialization functions used internally when I reviewed the PR last time but forgot to mention in my review - thanks for implementing the suggested changes in previous review. Though I feel there is still some duplication & hardcoded values that can be removed and I would be quick to review/ack if they, too, are addressed.

    From the last time, besides the suggested changes, there are Span -> std::span and AsWritableBytes -> std::as_writable_bytes refactors as well that I found in the range diff: git range-diff debacd1...a18efa9. I had a concern regarding the format of the data structures used in case of duplicated aggregate public keys but that was answered here: #31247 (review).

    The whole diff is the addition of the (de)serialization of the MuSig2 fields in the PSBT, introduction of the subsequently required internal data structures to hold these fields, and then addition of the said values in the response of the decodepsbt RPC all of which seem like straightforward additions IMO. I haven’t reviewed the test vectors as it was done in a previous review by someone else but I’ll take a look at those as well tomorrow.

    Code Review 8e08b335da104cab4918e1508af2d6a5ee239bfc

  71. achow101 force-pushed on Apr 17, 2025
  72. in src/psbt.h:225 in 239e0e6509 outdated
    220+
    221+    out.emplace(agg_pubkey, participants);
    222+}
    223+
    224+// Deserialize the MuSig2 participant identifiers from PSBT_MUSIG2_{PUBNONCE/PARTIAL_SIG} fields
    225+// Both fields contain the same data after the type byte - aggregate pubkey | particitpant pubkey | leaf script hash
    


    fjahr commented at 8:30 pm on April 17, 2025:
    nit: s/particitpant/participant/

    achow101 commented at 11:32 pm on April 17, 2025:
    Oops, fixed
  73. fjahr commented at 9:26 pm on April 17, 2025: contributor

    Code review ACK f385ff7aba0eab6cf00ede1b808817dfd7047399

    Typo may be fixed in the next MuSig2 PR unless you want to retouch.

  74. DrahtBot requested review from theStack on Apr 17, 2025
  75. psbt: Implement un/ser of musig2 fields ff3d460898
  76. rpc: Include MuSig2 fields in decodepsbt 26370c68d0
  77. tests: Add BIP 373 test vectors e261eb8d50
  78. achow101 force-pushed on Apr 17, 2025
  79. fjahr commented at 10:37 am on April 18, 2025: contributor

    re-ACK e261eb8d50c7192260a449e653453e63f59dbeed

    CI failure is unrelated, see #32291

  80. in src/psbt.h:780 in ff3d460898 outdated
    775+                }
    776+                case PSBT_IN_MUSIG2_PUB_NONCE:
    777+                {
    778+                    if (!key_lookup.emplace(key).second) {
    779+                        throw std::ios_base::failure("Duplicate Key, input musig2 pubnonce already provided");
    780+                    } else if (key.size() != 2 * CPubKey::COMPRESSED_SIZE + 1 && key.size() != 2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1) {
    


    theStack commented at 11:11 am on April 18, 2025:

    nit: could add parentheses for better readability (here and below for PSBT_IN_MUSIG2_PARTIAL_SIG)

    0                    } else if (key.size() != (2 * CPubKey::COMPRESSED_SIZE + 1) && key.size() != (2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1)) {
    
  81. theStack approved
  82. theStack commented at 11:11 am on April 18, 2025: contributor

    re-ACK e261eb8d50c7192260a449e653453e63f59dbeed (as per $ git range-diff debacd1f...e261eb8d)

    Verified that since my previous ACK, the changes didn’t affect the logic, and were mostly about code deduplication (for deserializing participant pubkeys), getting rid of magic numbers, variable naming (s/key/pubkey/), and adapting the terminology to the recent BIP-373 changes (plain public key -> compressed public key). LGTM.

  83. in test/functional/data/rpc_psbt.json:76 in e261eb8d50
    71+            "cHNidP8BAFICAAAAAVaG3/QAFl9OBApYVfZYCTRyybz4EIsnKl0x8YH3tP+xAQAAAAD9////ARjd9QUAAAAAFgAUyRI+BujX8JZsXRzQ+TMALU63V80AAAAAAAEBKwDh9QUAAAAAIlEgC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9QhFgtY4zeqTThSqMKTh8QkCNjPvjphOl45fgqfAaX7cQfUBQAmgN1uIRY0a5lZM1cQfJ00Weneuo0+r0TmY2yFx/hT65C6UujNAAUAWAsIhyEWT6/WX4FpGG/Cv9siM8d+Yw0QvigKJMcWXAmidhF3XCwFAMMkmoIhFvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5BQB91lWSIhoDC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9RjAjRrmVkzVxB8nTRZ6d66jT6vROZjbIXH+FPrkLpS6M0AAk+v1l+BaRhvwr/bIjPHfmMNEL4oCiTHFlwJonYRd1wsAvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5QhsCNGuZWTNXEHydNFnp3rqNPq9E5mNshcf4U+uQulLozQALWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EICUpsZ14eczATJFUh/XxNBvGhYsLx05QNsZD03tTpDcbUDt/iv4yY/yz7yRU/hbzpnWcVgDHjmN7jfoNg+hVKIIFZDGwJPr9ZfgWkYb8K/2yIzx35jDRC+KAokxxZcCaJ2EXdcLAMLWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EIDqB2XNJnnT4LStasT8eaenhGl/bQPVGbvZy5uTOugvnkDSZWTdnFQOU7s44TQUcRbYVxS2+HI6zYQ17NEtDt9rW1DGwL5MIoBkljDEEk0T4X4nVIptTHIRYNvmbCGAfETvOA2+QMLWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EIDOUnb/zG51QJR2IIeZsxcl7fjr9fZW/3HdQD9qJR/OuACPyDk8L8LdvDUDNOr/nzjc9ZoW713tfoo5/SftCnRibAAAA==",
    72+            "Input musig2 pubnonce key is not expected size"
    73+        ],
    74+        [
    75+            "cHNidP8BAFICAAAAAVaG3/QAFl9OBApYVfZYCTRyybz4EIsnKl0x8YH3tP+xAQAAAAD9////ARjd9QUAAAAAFgAUyRI+BujX8JZsXRzQ+TMALU63V80AAAAAAAEBKwDh9QUAAAAAIlEgC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9QhFgtY4zeqTThSqMKTh8QkCNjPvjphOl45fgqfAaX7cQfUBQAmgN1uIRY0a5lZM1cQfJ00Weneuo0+r0TmY2yFx/hT65C6UujNAAUAWAsIhyEWT6/WX4FpGG/Cv9siM8d+Yw0QvigKJMcWXAmidhF3XCwFAMMkmoIhFvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5BQB91lWSIhoDC1jjN6pNOFKowpOHxCQI2M++OmE6Xjl+Cp8BpftxB9RjAjRrmVkzVxB8nTRZ6d66jT6vROZjbIXH+FPrkLpS6M0AAk+v1l+BaRhvwr/bIjPHfmMNEL4oCiTHFlwJonYRd1wsAvkwigGSWMMQSTRPhfidUim1MchFg2+ZsIYB8RO84Db5Qhs0a5lZM1cQfJ00Weneuo0+r0TmY2yFx/hT65C6UujNAAMLWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EICUpsZ14eczATJFUh/XxNBvGhYsLx05QNsZD03tTpDcbUDt/iv4yY/yz7yRU/hbzpnWcVgDHjmN7jfoNg+hVKIIFZDGwJPr9ZfgWkYb8K/2yIzx35jDRC+KAokxxZcCaJ2EXdcLAMLWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EIDqB2XNJnnT4LStasT8eaenhGl/bQPVGbvZy5uTOugvnkDSZWTdnFQOU7s44TQUcRbYVxS2+HI6zYQ17NEtDt9rW1DGwL5MIoBkljDEEk0T4X4nVIptTHIRYNvmbCGAfETvOA2+QMLWOM3qk04UqjCk4fEJAjYz746YTpeOX4KnwGl+3EH1EIDOUnb/zG51QJR2IIeZsxcl7fjr9fZW/3HdQD9qJR/OuACPyDk8L8LdvDUDNOr/nzjc9ZoW713tfoo5/SftCnRibAAAA==",
    76+            "Input musig2 pubnonce key is not expected size"
    


    rkrux commented at 2:15 pm on April 18, 2025:

    Now that the error is more detailed; the test passes because assert_raises_rpc_error checks for substring. But I can pick this up in a follow up.

    0- Input musig2 pubnonce key is not expected size
    1+ Input musig2 pubnonce key is not expected size of 67 or 99 bytes
    
  84. rkrux approved
  85. rkrux commented at 2:21 pm on April 18, 2025: contributor

    tACK e261eb8d50c7192260a449e653453e63f59dbeed

    Thanks for addressing the comments. I have checked the test vectors as well, all of them from the BIP are there.

    0git range-diff 8e08b33...e261eb8
    
  86. glozow merged this on Apr 18, 2025
  87. glozow closed this on Apr 18, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-28 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me