wallet, descriptor: Fix MuSig private key completeness checks on `importdescriptors` #35493

w0xlt commented at 6:49 AM on June 9, 2026: contributor

importdescriptors currently checks whether all private keys are present by expanding the descriptor and verifying that every expanded origin pubkey has a private key.

This is wrong for MuSig descriptors because expansion includes the synthetic aggregate pubkey. There is no individual private key for that aggregate pubkey, so importing a fully private MuSig descriptor such as rawtr(musig(A_priv,B_priv)) incorrectly returns:

Not all private keys provided. Some wallet functionality may return unexpected errors

This PR fixes the issue by making descriptor private-key completeness account for MuSig participant keys, and by having importdescriptors use Descriptor::HavePrivateKeys() instead of duplicating its own manual completeness check.

The functional test covers both cases:

rawtr(musig(A_priv,B_priv)) imports without warnings.
rawtr(musig(A_priv,B_pub)) still warns that not all private keys were provided.

DrahtBot commented at 6:49 AM on June 9, 2026: 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/35493.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	rkrux, theStack
Approach ACK	b-l-u-e
Stale ACK	pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#34861 (wallet: Add importdescriptors interface by polespinasa)

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.

sedited requested review from theStack on Jun 9, 2026

pablomartin4btc commented at 5:42 PM on June 9, 2026: member

Approach ACK.

I verified the regression by running the functional test without the fix, where the fully-private rawtr(musig(...)) import incorrectly warns that not all private keys were provided. With this PR, the fully-private MuSig descriptor imports without warnings, while the partial-private case still warns as expected.

The approach makes sense to me: as the previous origin/pubkey counting heuristic does not correctly model MuSig descriptors where the aggregate key is derived from the participant keys. The added functional test covers both the regression and the expected warning case.

b-l-u-e commented at 7:18 AM on June 10, 2026: contributor

Approach ACK. The fix looks okay I manually tested importdescriptors for rawtr(musig(...)/0/*): full participant privkeys import without warning; partial import still warns. maybe add an importdescriptors case to cover aggregate-derivation imports.

--- a/test/functional/wallet_musig.py
+++ b/test/functional/wallet_musig.py
@@ -217,6 +217,26 @@ class WalletMuSigTest(BitcoinTestFramework):
         assert_equal(res[1]["success"], True)
         assert_equal(res[1]["warnings"], [missing_keys_warning])
 
+        # Check musig participant completeness for aggregate-key derivation descriptors.
+        # Full private participant set should not warn, partial should.
+        res = wallet.importdescriptors([
+            {
+                "desc": descsum_create(f"rawtr(musig({keys[0][0]},{keys[1][0]})/0/*)"),
+                "timestamp": "now",
+                "range": [0, 1],
+            },
+            {
+                "desc": descsum_create(f"rawtr(musig({keys[0][0]},{keys[1][1]})/1/*)"),
+                "timestamp": "now",
+                "range": [0, 1],
+            },
+        ])
+
+        assert_equal(res[0]["success"], True)
+        assert "warnings" not in res[0]
+        assert_equal(res[1]["success"], True)
+        assert_equal(res[1]["warnings"], [missing_keys_warning])
+

rkrux commented at 10:55 AM on June 12, 2026: contributor

Good catch, Concept ACK 9d75efa.

theStack commented at 2:16 PM on June 12, 2026: contributor

Concept ACK

in test/functional/wallet_musig.py:197 in 9d75efa10f

 193 | @@ -194,6 +194,29 @@ def test_failure_case_3(self, comment, pat):
 194 |          assert "musig2_pubnonces" in dec["inputs"][0]
 195 |          assert "musig2_partial_sigs" not in dec["inputs"][0]
 196 |  
 197 | +    def test_importdescriptors_private_key_warnings(self):

rkrux commented at 1:35 PM on June 15, 2026:

This test is more suitable for wallet_importdescriptors.py file. Even the missing_keys_warning is hardcoded there few times that could be extracted out. https://github.com/bitcoin/bitcoin/blob/6921f5df011abb8df9b2e7234613ca8a369492ab/test/functional/wallet_importdescriptors.py#L682-L707

This file wallet_musig.py is more suitable for testing the musig signing flow, the file name unfortunately falls short of its intent.

w0xlt commented at 4:25 AM on June 18, 2026:

Done. Thanks.

in test/functional/wallet_musig.py:199 in 9d75efa10f

 193 | @@ -194,6 +194,29 @@ def test_failure_case_3(self, comment, pat):
 194 |          assert "musig2_pubnonces" in dec["inputs"][0]
 195 |          assert "musig2_partial_sigs" not in dec["inputs"][0]
 196 |  
 197 | +    def test_importdescriptors_private_key_warnings(self):
 198 | +        self.log.info("Testing importdescriptors MuSig private key warnings")
 199 | +        _, keys = self.create_wallets_and_keys_from_pattern("rawtr(musig($0,$1))")

rkrux commented at 1:38 PM on June 15, 2026:

The creation of multiple wallets to extract the keys is not needed for this test.

Related to moving this test, there are already few hardcoded xprvs and xpubs in wallet_importdescriptors.py that are sufficient for this test, keeping the test light.

https://github.com/bitcoin/bitcoin/blob/6921f5df011abb8df9b2e7234613ca8a369492ab/test/functional/wallet_importdescriptors.py#L580-L589

w0xlt commented at 4:25 AM on June 18, 2026:

Done. Thanks.

rkrux changes_requested

rkrux commented at 1:39 PM on June 15, 2026: contributor

Code review 9d75efa10f09428697f76c6693149214319d6a24

w0xlt force-pushed on Jun 18, 2026

DrahtBot added the label CI failed on Jun 18, 2026

w0xlt commented at 4:25 AM on June 18, 2026: contributor

@b-l-u-e Thanks. Done.

w0xlt commented at 4:26 AM on June 18, 2026: contributor

Rebased. CI error is unrelated.

DrahtBot removed the label CI failed on Jun 19, 2026

DrahtBot added the label Needs rebase on Jun 26, 2026

in test/functional/wallet_importdescriptors.py:220 in 86dbbead1c

 215 | +                "desc": descsum_create(f"rawtr(musig({xprv1}/84h/0h/0h,{acc_xprv2})/0/*)"),
 216 | +                "timestamp": "now",
 217 | +                "range": [0, 1],
 218 | +            },
 219 | +            {
 220 | +                "desc": descsum_create(f"rawtr(musig({xprv1}/84h/0h/0h,{acc_xpub2})/1/*)"),

rkrux commented at 7:57 AM on June 27, 2026:

84h/0h/0h

It would need a rebase post merging of PR #35543. While doing that, let's also avoid hardcoding this derivation path and instead use the variable derivation_path here.

w0xlt commented at 11:15 PM on July 7, 2026:

Done. Thanks.

w0xlt force-pushed on Jul 7, 2026

DrahtBot removed the label Needs rebase on Jul 8, 2026

pablomartin4btc commented at 6:10 AM on July 9, 2026: member

ACK 0fd7b53e9c7afa693e7e9cceb5dff933530688d1

Since my last review the test was moved to wallet_importdescriptors.py and simplified, the hardcoded warning string extracted to a constant, the derivation path de-hardcoded to use the existing derivation_path variable, and rebased after #35543. Also addresses @b-l-u-e's suggestion — the test now covers both participant-level and aggregate-level derivation cases.

DrahtBot requested review from rkrux on Jul 9, 2026

willcl-ark added the label Wallet on Jul 9, 2026

willcl-ark added the label Descriptors on Jul 9, 2026

in src/test/descriptor_tests.cpp:1348 in b768584674

1344 | @@ -1343,6 +1345,20 @@ BOOST_AUTO_TEST_CASE(descriptor_older_warnings)
1345 |      }
1346 |  }
1347 |  
1348 | +BOOST_AUTO_TEST_CASE(descriptor_musig_have_private_keys)

achow101 commented at 9:34 PM on July 9, 2026:

In b7685846743605e9c542adc814c40cdd7ee19a85 "descriptors: require complete MuSig private keys"

Instead of adding a new specific unit test just for this case, we should be updating DoCheck so that it performs the proper checks for all of the descriptors being passed in. Any new test descriptors should be added to descriptor_test with appropriate flags set so that the correct things are being checked.

w0xlt commented at 7:49 AM on July 10, 2026:

Done. Thanks.

in src/script/descriptor.cpp:238 in b768584674

 233 | @@ -234,6 +234,14 @@ struct PubkeyProvider
 234 |      /** Derive a private key, if private data is available in arg and put it into out. */
 235 |      virtual void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
 236 |  
 237 | +    /** Whether private data for this provider is available in arg. */
 238 | +    virtual bool HavePrivateKeys(int pos, const SigningProvider& arg) const

achow101 commented at 9:39 PM on July 9, 2026:

In b7685846743605e9c542adc814c40cdd7ee19a85 "descriptors: require complete MuSig private keys"

Having pos here is pointless, it has no bearing on whether we do or don't have private keys for this provider. pos is only necessary in GetPrivKey to get a specific private key, but this function only cares about whether we can get private keys at all, especially because all callers pass 0 anyways.

w0xlt commented at 7:49 AM on July 10, 2026:

Done. Thanks.

descriptors: require complete MuSig private keys cd8d01bf47

wallet: check descriptor private key completeness on import 5e62fbf09c

test: check MuSig import private key warnings

Co-authored-by: b-l-u-e <winnie.gitau282@gmail.com>

0390338692

w0xlt force-pushed on Jul 10, 2026

w0xlt commented at 7:50 AM on July 10, 2026: contributor

@b-l-u-e I added you as a co-author of the test commit. I had forgotten to do so earlier.

b-l-u-e commented at 8:35 AM on July 10, 2026: contributor

@b-l-u-e I added you as a co-author of the test commit. I had forgotten to do so earlier.

It’s all good in the hood 😊