Merge a0c73846aa into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

src/script/descriptor.cpp

@@ -642,13 +642,26 @@ public:
     virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const
     {
         size_t pos = 0;
+        bool has_any_priv = false;
         for (const auto& scriptarg : m_subdescriptor_args) {
             if (pos++) ret += ",";
             std::string tmp;
-            if (!scriptarg->ToStringHelper(arg, tmp, type, cache)) return false;
+            bool success = scriptarg->ToStringHelper(arg, tmp, type, cache);
+            if (type == StringType::PRIVATE) {
+                if (success) {
+                    has_any_priv = true;
+                } else {
+                    tmp = "";
+                    if (!scriptarg->ToStringHelper(arg, tmp, StringType::PUBLIC, cache)) {
+                        return false;
+                    }
+                }
+            } else if (!success) {
+                return false;
+            }
             ret += tmp;
         }
-        return true;
+        return (type == StringType::PRIVATE) ? has_any_priv : true;
     }
 
     // NOLINTNEXTLINE(misc-no-recursion)
@@ -657,6 +670,7 @@ public:
         std::string extra = ToStringExtra();
         size_t pos = extra.size() > 0 ? 1 : 0;
         std::string ret = m_name + "(" + extra;
+        bool has_any_priv = false;
         for (const auto& pubkey : m_pubkey_args) {
             if (pos++) ret += ",";
             std::string tmp;
@@ -665,7 +679,11 @@ public:
                     if (!pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
                     break;
                 case StringType::PRIVATE:
-                    if (!pubkey->ToPrivateString(*arg, tmp)) return false;
+                    if (!pubkey->ToPrivateString(*arg, tmp)) {
+                         tmp = pubkey->ToString();
+                     } else {
+                         has_any_priv = true;
+                     }
                     break;
                 case StringType::PUBLIC:
                     tmp = pubkey->ToString();
@@ -677,7 +695,21 @@ public:
             ret += tmp;
         }
         std::string subscript;
-        if (!ToStringSubScriptHelper(arg, subscript, type, cache)) return false;
+        bool is_subscript_successful = ToStringSubScriptHelper(arg, subscript, type, cache);
+
+        if (type == StringType::PRIVATE) {
+            if (!has_any_priv) {
+                if (subscript.empty()) {
+                    return false;
+                }
+                if (!is_subscript_successful) {
+                    return false;
+                }
+            }
+        } else if (!is_subscript_successful) {
+            return false;
+        }
+
         if (pos && subscript.size()) ret += ',';
         out = std::move(ret) + std::move(subscript) + ")";
         return true;
@@ -1186,6 +1218,7 @@ protected:
     bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const override
     {
         if (m_depths.empty()) return true;
+        bool has_any_subscript_key = false;
         std::vector<bool> path;
         for (size_t pos = 0; pos < m_depths.size(); ++pos) {
             if (pos) ret += ',';
@@ -1194,7 +1227,19 @@ protected:
                 path.push_back(false);
             }
             std::string tmp;
-            if (!m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache)) return false;
+            bool is_subdescriptor_successful = m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache);
+            if (type == StringType::PRIVATE) {
+                if (is_subdescriptor_successful) {
+                    has_any_subscript_key = true;
+                } else {
+                    tmp = "";
+                    if (!m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, StringType::PUBLIC, cache)) {
+                        return false;
+                    }
+                }
+            } else if (!is_subdescriptor_successful) {
+                return false;
+            }
             ret += tmp;
             while (!path.empty() && path.back()) {
                 if (path.size() > 1) ret += '}';
@@ -1202,7 +1247,7 @@ protected:
             }
             if (!path.empty()) path.back() = true;
         }
-        return true;
+        return (type == StringType::PRIVATE) ? has_any_subscript_key : true;
     }
 public:
     TRDescriptor(std::unique_ptr<PubkeyProvider> internal_key, std::vector<std::unique_ptr<DescriptorImpl>> descs, std::vector<int> depths) :
@@ -1296,7 +1341,9 @@ public:
     {
         std::string ret;
         if (m_private) {
-            if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {};
+            if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) {
+                ret = m_pubkeys[key]->ToString();
+            }
         } else {
             ret = m_pubkeys[key]->ToString();
         }

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)

test/functional/wallet_listdescriptors.py

@@ -29,6 +29,26 @@ class ListDescriptorsTest(BitcoinTestFramework):
     def init_wallet(self, *, node):
         return
 
+    def test_listdescriptors_with_partial_privkeys(self):
+        self.nodes[0].createwallet(wallet_name="partialkeys_descs", blank=True)
+        wallet = self.nodes[0].get_wallet_rpc("partialkeys_descs")
+
+        self.log.info("Test listdescriptors(private=true) with descriptors containing partial private keys")
+        descs = [
+            descsum_create("wsh(multi(2,tprv8ZgxMBicQKsPdzuc344mDaeUk5zseMcRK9Hst8xodskNu3YbQG5NxLa2X17PUU5yXQhptiBE7F5W5cgEmsfQg4Y21Y18w4DJhLxSb8CurDf,tpubD6NzVbkrYhZ4YiCvExLvH4yh1k3jFGf5irm6TsrArY8GYdEhYVdztQTBtTirmRc6XfSJpH9tayUdnngaJZKDaa2zbqEY29DfcGZW8iRVGUY))"),
+            descsum_create("wsh(thresh(2,pk(tprv8ZgxMBicQKsPdzuc344mDaeUk5zseMcRK9Hst8xodskNu3YbQG5NxLa2X17PUU5yXQhptiBE7F5W5cgEmsfQg4Y21Y18w4DJhLxSb8CurDf),s:pk(tpubD6NzVbkrYhZ4YiCvExLvH4yh1k3jFGf5irm6TsrArY8GYdEhYVdztQTBtTirmRc6XfSJpH9tayUdnngaJZKDaa2zbqEY29DfcGZW8iRVGUY),sln:older(2)))"),
+            descsum_create("tr(03d1d1110030000000000120000000000000000000000000001370010912cd08cc,pk(cNKAo2ZRsaWKcP481cEfj3astPyBrfq56JBtLeRhHUvTSuk2z4MR))"),
+            descsum_create("tr(cNKAo2ZRsaWKcP481cEfj3astPyBrfq56JBtLeRhHUvTSuk2z4MR,pk(03d1d1110030000000000120000000000000000000000000001370010912cd08cc))")
+        ]
+        importdescriptors_request = list(map(lambda desc: {"desc": desc, "timestamp": "now"}, descs))
+        res = wallet.importdescriptors(importdescriptors_request)
+        for imported_desc in res:
+            assert_equal(imported_desc["success"], True)
+        res = wallet.listdescriptors(private=True)
+        for listed_desc in res["descriptors"]:
+            # descriptors are not returned in the order they were present in the import request
+            assert_equal(listed_desc["desc"], next(desc for desc in descs if listed_desc["desc"] == desc))
+
     def run_test(self):
         node = self.nodes[0]
         assert_raises_rpc_error(-18, 'No wallet is loaded.', node.listdescriptors)
@@ -125,6 +145,7 @@ class ListDescriptorsTest(BitcoinTestFramework):
             ]
         }
         assert_equal(expected, wallet.listdescriptors())
+        self.test_listdescriptors_with_partial_privkeys()
 
 
 if __name__ == '__main__':