qa: test descriptors with mixed xpubs and const pubkeys #23171

Writing unit tests for Miniscript descriptors i noticed that test/descriptor_tests's DoCheck() assumes that a descriptor would either contain only extended keys or only const pubkeys: if it detects an xpub in the descriptor it would assert the number of cached keys is equal to the number of keys in the descriptor, which does not hold if the descriptor also contains const (raw?) public keys since we only cache parent xpubs.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

tACK 6ae2d37c7112656f3fb09e5fabc016345f73aa06, looks good to me

Concept + tACK https://github.com/bitcoin/bitcoin/pull/23171/commits/6ae2d37c7112656f3fb09e5fabc016345f73aa06

tested ACK 6ae2d37.

Checking mixed pubkeys is a nice addition to the test.

cc @achow101 @sipa

While I like the idea of testing mixed ranged and const pubkeys, I don't think this is the right approach.

For starters, we have a flags field to indicate what to check for, so I think it would be better to add mixed_pubkeys as an additional flag rather than as a bool. Secondly, this doesn't really test that the right keys are there. Rather it just disables the cache checks when there are mixed ranged and const keys. IMO the correct thing to do would be to retain those checks, with modifications to allow for the mixed case when the appropriate flag is set.

Definitely agree the approach is clumsy. However this does test that we can parse descriptors with mixed const and extended pubkeys. Keeping the check on the number of xpubs cached in this case would require either adding a new getter to Descriptor or parsing the number of xpubs directly from the string, and both approaches sound overkill.

Pushed the change to use the flag instead of the boolean.

diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 68584307b2..4d5ef381d0 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -74,6 +74,18 @@ std::string UseHInsteadOfApostrophe(const std::string& desc)
     return ret;
 }
 
+// Count the number of times the string "xpub" appears in a descriptor string
+static size_t CountXpubs(const std::string& desc)
+{
+    size_t count = 0;
+    size_t p = desc.find("xpub", 0);
+    while (p != std::string::npos) {
+        count++;
+        p = desc.find("xpub", p + 1);
+    }
+    return count;
+}
+
 const std::set<std::vector<uint32_t>> ONLY_EMPTY{{}};
 
 void DoCheck(const std::string& prv, const std::string& pub, const std::string& norm_prv, const std::string& norm_pub, int flags, const std::vector<std::vector<std::string>>& scripts, const std::optional<OutputType>& type, const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY,
@@ -172,7 +184,8 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
             // Check whether keys are in the cache
             const auto& der_xpub_cache = desc_cache.GetCachedDerivedExtPubKeys();
             const auto& parent_xpub_cache = desc_cache.GetCachedParentExtPubKeys();
-            if ((flags & RANGE) && !(flags & (DERIVE_HARDENED | MIXED_PUBKEYS))) {
+            const size_t num_xpubs = CountXpubs(pub1);
+            if ((flags & RANGE) && !(flags & (DERIVE_HARDENED))) {
                 // For ranged, unhardened derivation, None of the keys in origins should appear in the cache but the cache should have parent keys
                 // But we can derive one level from each of those parent keys and find them all
                 BOOST_CHECK(der_xpub_cache.empty());
@@ -184,13 +197,22 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
                     xpub.Derive(der, i);
                     pubkeys.insert(der.pubkey);
                 }
+                int count_pks = 0;
                 for (const auto& origin_pair : script_provider_cached.origins) {
                     const CPubKey& pk = origin_pair.second.first;
-                    BOOST_CHECK(pubkeys.count(pk) > 0);
+                    count_pks += pubkeys.count(pk);
                 }
-            } else if (!(flags & MIXED_PUBKEYS) && pub1.find("xpub") != std::string::npos) {
+                if (flags & MIXED_PUBKEYS) {
+                    BOOST_CHECK_EQUAL(num_xpubs, count_pks);
+                } else {
+                    BOOST_CHECK_EQUAL(script_provider_cached.origins.size(), count_pks);
+                }
+            } else if (num_xpubs > 0) {
                 // For ranged, hardened derivation, or not ranged, but has an xpub, all of the keys should appear in the cache
-                BOOST_CHECK(der_xpub_cache.size() + parent_xpub_cache.size() == script_provider_cached.origins.size());
+                BOOST_CHECK(der_xpub_cache.size() + parent_xpub_cache.size() == num_xpubs);
+                if (!(flags & MIXED_PUBKEYS)) {
+                    BOOST_CHECK(num_xpubs == script_provider_cached.origins.size());
+                }
                 // Get all of the derived pubkeys
                 std::set<CPubKey> pubkeys;
                 for (const auto& xpub_map_pair : der_xpub_cache) {
@@ -207,9 +229,15 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
                     xpub.Derive(der, i);
                     pubkeys.insert(der.pubkey);
                 }
+                int count_pks = 0;
                 for (const auto& origin_pair : script_provider_cached.origins) {
                     const CPubKey& pk = origin_pair.second.first;
-                    BOOST_CHECK(pubkeys.count(pk) > 0);
+                    count_pks += pubkeys.count(pk);
+                }
+                if (flags & MIXED_PUBKEYS) {
+                    BOOST_CHECK_EQUAL(num_xpubs, count_pks);
+                } else {
+                    BOOST_CHECK_EQUAL(script_provider_cached.origins.size(), count_pks);
                 }
             } else if (!(flags & MIXED_PUBKEYS)) {
                 // Only const pubkeys, nothing should be cached

Well, whatever. Amended the commit with your patch.

Anything else that needs to be done there?

ACK 36012ef143917f97179d3ba6599ef36a26a9a014