test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)

theStack commented at 9:35 pm on April 18, 2025: contributor

This PR is a follow-up to #31247 (see #31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the decodepsbt RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.

DrahtBot commented at 9:35 pm on April 18, 2025: 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/32305.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, 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:

#31622 (psbt: add non-default sighash types to PSBTs and unify sighash type match checking by achow101)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys 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.

DrahtBot added the label Tests on Apr 18, 2025

maflcko commented at 12:06 pm on April 21, 2025: member

(fresh CI)

maflcko closed this on Apr 21, 2025

maflcko reopened this on Apr 21, 2025

in test/functional/test_framework/psbt.py:96 in e5537de0ab outdated

91@@ -88,6 +92,9 @@ def serialize(self):
92         for k,v in self.map.items():
93             if isinstance(k, int) and 0 <= k and k <= 255:
94                 k = bytes([k])
95+            if isinstance(v, list):
96+                assert any(type(elem) is bytes for elem in v)

rkrux commented at 1:33 pm on April 21, 2025:

Since this is a new check that has been added & the current use case is of a list of only bytes (in participant pubkeys), maybe we can assert this on all instead of any to keep the checking strict to start with that can be eased in the future if needed?

theStack commented at 2:22 pm on May 7, 2025:

Good point, I agree that the all predicate makes much more sense than any here, not sure what motivated me to use the latter back then. Fixed.

in test/functional/rpc_psbt.py:1020 in 23ee07dd0f outdated

1015+
1016+        assert "musig2_participant_pubkeys" in res_input
1017+        in_participant_pks = res_input["musig2_participant_pubkeys"][0]
1018+        assert "aggregate_pubkey" in in_participant_pks
1019+        assert_equal(in_participant_pks["aggregate_pubkey"], in_fake_agg_pubkey.hex())
1020+        assert "participant_pubkeys" in in_participant_pks

rkrux commented at 1:34 pm on April 21, 2025:

Are asserts like on this line really needed? If the decoded psbt is malformed then the test would fail in the following assert anyway.

theStack commented at 2:22 pm on May 7, 2025:

Right, strictly speaking they are not needed. I added them because we seem to do this also in many other functional tests, and I think it’s nice to differentiate between “attribute is not available” and “attribute doesn’t match the expected value” more explicitly by failing in different lines. Happy to drop them though if others feel strongly.

in test/functional/rpc_psbt.py:984 in 23ee07dd0f outdated

980@@ -977,6 +981,74 @@ def test_psbt_input_keys(psbt_input, keys):
981             assert hash.hex() in res_input[preimage_key]
982             assert_equal(res_input[preimage_key][hash.hex()], preimage.hex())
983 
984+        self.log.info("Test decoding PSBT with MuSig2 per-input and per-output types")

rkrux commented at 1:35 pm on April 21, 2025:

Nit: Maybe consider putting this in a separate subtest as the current run_test is quite long already.

theStack commented at 2:22 pm on May 7, 2025:

Good idea, done.

in test/functional/rpc_psbt.py:1050 in 23ee07dd0f outdated

1045+        assert "musig2_participant_pubkeys" in res_output
1046+        out_participant_pks = res_output["musig2_participant_pubkeys"][0]
1047+        assert "aggregate_pubkey" in out_participant_pks
1048+        assert_equal(out_participant_pks["aggregate_pubkey"], out_fake_agg_pubkey.hex())
1049+        assert "participant_pubkeys" in out_participant_pks
1050+        assert_equal(out_participant_pks["participant_pubkeys"], [out_pubkey1.hex(), out_pubkey2.hex()])

rkrux commented at 1:38 pm on April 21, 2025:

Fine for now but maybe we can consider making an assert_musig2_fields() later when more musig2 specific tests are added - if needed though.

theStack commented at 2:22 pm on May 7, 2025:

Leaving that one for a follow-up.

rkrux approved

rkrux commented at 1:41 pm on April 21, 2025: contributor

tACK 23ee07dd0f81930f46d2f4910cc92cb0eab61cc3

I believe this is a value add as it sets the base for the MuSig2 functional testing. I left few minor suggestions, I would be quick to re-ack if they are implemented.

theStack force-pushed on May 7, 2025

theStack commented at 2:24 pm on May 7, 2025: contributor

Thanks for the review @rkrux, I rebased on master and addressed most of your review comments.

test: add constants for MuSig2 PSBT key types (BIP 373)

Also, support serialization of lists of byte-strings as PSBTMap values,
which will be simply concatenated without any compact-size prefixes
(neither for the individual items nor for the size of the list).

8ba245cb83

test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) 4b24186756

theStack force-pushed on May 7, 2025

theStack commented at 5:33 pm on May 7, 2025: contributor

Force-pushed again, fixing a small bug in an assertion (kudos to LLM linter for detecting this 👌 )

0    -+        assert "aggregate_pubkey" in in_pubnonce
1    ++        assert "aggregate_pubkey" in in_partialsig
2     +        assert_equal(in_partialsig["aggregate_pubkey"], in_fake_agg_pubkey.hex())

achow101 commented at 9:47 pm on May 7, 2025: member

ACK 4b241867567203b204823a4558c2aa5767acf028

DrahtBot requested review from rkrux on May 7, 2025

rkrux approved

rkrux commented at 10:01 am on May 8, 2025: contributor

re-ACK 4b24186

0git range-diff 23ee07d...4b24186

Thanks for addressing the comments.

achow101 merged this on May 14, 2025

achow101 closed this on May 14, 2025

theStack deleted the branch on May 14, 2025

test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) #32305

Code Coverage & Benchmarks

Reviews

Conflicts