Give more errors for specific failure conditions

Some failure conditions implicitly fail by failing some other check.
But the error messages are more helpful if they say explicitly what
actually caused the failure, so add those as failure conditions and
errors.

6 files changed, 67 insertions, 18 deletions

diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index e48ed8db75..1516007201 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -153,7 +153,7 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
     std::string error;
     auto desc = Parse(request.params[0].get_str(), provider, error);
     if (!desc) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor, %s", error));
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
 
     UniValue result(UniValue::VOBJ);
@@ -203,7 +203,7 @@ UniValue deriveaddresses(const JSONRPCRequest& request)
     std::string error;
     auto desc = Parse(desc_str, key_provider, error, /* require_checksum = */ true);
     if (!desc) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor, %s", error));
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
 
     if (!desc->IsRange() && request.params.size() > 1) {
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 52b2b94852..464537a5b0 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -720,7 +720,7 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
     std::string error;
     auto desc = Parse(desc_str, provider, error);
     if (!desc) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor '%s', %s", desc_str, error));
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
     if (!desc->IsRange()) {
         range.first = 0;
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index d9b0cfa000..b782ebbd1f 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -700,7 +700,10 @@ NODISCARD bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath&
             hardened = true;
         }
         uint32_t p;
-        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p) || p > 0x7FFFFFFFUL) {
+        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p)) {
+            error = strprintf("Key path value '%s' is not a valid uint32", std::string(elem.begin(), elem.end()).c_str());
+            return false;
+        } else if (p > 0x7FFFFFFFUL) {
             error = strprintf("Key path value %u is out of range", p);
             return false;
         }
@@ -714,17 +717,35 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(const Span<const char>& sp, boo
 {
     auto split = Split(sp, '/');
     std::string str(split[0].begin(), split[0].end());
+    if (str.size() == 0) {
+        error = "No key provided";
+        return nullptr;
+    }
     if (split.size() == 1) {
         if (IsHex(str)) {
             std::vector<unsigned char> data = ParseHex(str);
             CPubKey pubkey(data);
-            if (pubkey.IsFullyValid() && (permit_uncompressed || pubkey.IsCompressed())) return MakeUnique<ConstPubkeyProvider>(pubkey);
+            if (pubkey.IsFullyValid()) {
+                if (permit_uncompressed || pubkey.IsCompressed()) {
+                    return MakeUnique<ConstPubkeyProvider>(pubkey);
+                } else {
+                    error = "Uncompressed keys are not allowed";
+                    return nullptr;
+                }
+            }
+            error = strprintf("Pubkey '%s' is invalid", str);
+            return nullptr;
         }
         CKey key = DecodeSecret(str);
-        if (key.IsValid() && (permit_uncompressed || key.IsCompressed())) {
-            CPubKey pubkey = key.GetPubKey();
-            out.keys.emplace(pubkey.GetID(), key);
-            return MakeUnique<ConstPubkeyProvider>(pubkey);
+        if (key.IsValid()) {
+            if (permit_uncompressed || key.IsCompressed()) {
+                CPubKey pubkey = key.GetPubKey();
+                out.keys.emplace(pubkey.GetID(), key);
+                return MakeUnique<ConstPubkeyProvider>(pubkey);
+            } else {
+                error = "Uncompressed keys are not allowed";
+                return nullptr;
+            }
         }
     }
     CExtKey extkey = DecodeExtKey(str);
@@ -760,7 +781,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(const Span<const char>& sp, bool per
     }
     if (origin_split.size() == 1) return ParsePubkeyInner(origin_split[0], permit_uncompressed, out, error);
     if (origin_split[0].size() < 1 || origin_split[0][0] != '[') {
-        error = strprintf("Key origin expected but not found, got '%s' instead", std::string(origin_split[0].begin(), origin_split[0].end()));
+        error = strprintf("Key origin start '[ character expected but not found, got '%c' instead", origin_split[0][0]);
         return nullptr;
     }
     auto slash_split = Split(origin_split[0].subspan(1), '/');
@@ -802,19 +823,22 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
         auto pubkey = ParsePubkey(expr, true, out, error);
         if (!pubkey) return nullptr;
         return MakeUnique<ComboDescriptor>(std::move(pubkey));
+    } else if (ctx != ParseScriptContext::TOP && Func("combo", expr)) {
+        error = "Cannot have combo in non-top level";
+        return nullptr;
     }
     if (Func("multi", expr)) {
         auto threshold = Expr(expr);
         uint32_t thres;
         std::vector<std::unique_ptr<PubkeyProvider>> providers;
         if (!ParseUInt32(std::string(threshold.begin(), threshold.end()), &thres)) {
-            error = strprintf("multi threshold %u out of range", thres);
+            error = strprintf("Multi threshold '%s' is not valid", std::string(threshold.begin(), threshold.end()).c_str());
             return nullptr;
         }
         size_t script_size = 0;
         while (expr.size()) {
             if (!Const(",", expr)) {
-                error = strprintf("multi: expected ',', got '%c'", expr[0]);
+                error = strprintf("Multi: expected ',', got '%c'", expr[0]);
                 return nullptr;
             }
             auto arg = Expr(expr);
@@ -823,10 +847,19 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
             script_size += pk->GetSize() + 1;
             providers.emplace_back(std::move(pk));
         }
-        if (providers.size() < 1 || providers.size() > 16 || thres < 1 || thres > providers.size()) return nullptr;
+        if (providers.size() < 1 || providers.size() > 16) {
+            error = strprintf("Cannot have %u keys in multisig; must have between 1 and 16 keys, inclusive", providers.size());
+            return nullptr;
+        } else if (thres < 1) {
+            error = strprintf("Multisig threshold cannot be %d, must be at least 1", thres);
+            return nullptr;
+        } else if (thres > providers.size()) {
+            error = strprintf("Multisig threshold cannot be larger than the number of keys; threshold is %d but only %u keys specified", thres, providers.size());
+            return nullptr;
+        }
         if (ctx == ParseScriptContext::TOP) {
             if (providers.size() > 3) {
-                error = strprintf("Cannot %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
+                error = strprintf("Cannot have %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
                 return nullptr;
             }
         }
@@ -842,16 +875,25 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
         auto pubkey = ParsePubkey(expr, false, out, error);
         if (!pubkey) return nullptr;
         return MakeUnique<WPKHDescriptor>(std::move(pubkey));
+    } else if (ctx == ParseScriptContext::P2WSH && Func("wpkh", expr)) {
+        error = "Cannot have wpkh within wsh";
+        return nullptr;
     }
     if (ctx == ParseScriptContext::TOP && Func("sh", expr)) {
         auto desc = ParseScript(expr, ParseScriptContext::P2SH, out, error);
         if (!desc || expr.size()) return nullptr;
         return MakeUnique<SHDescriptor>(std::move(desc));
+    } else if (ctx != ParseScriptContext::TOP && Func("sh", expr)) {
+        error = "Cannot have sh in non-top level";
+        return nullptr;
     }
     if (ctx != ParseScriptContext::P2WSH && Func("wsh", expr)) {
         auto desc = ParseScript(expr, ParseScriptContext::P2WSH, out, error);
         if (!desc || expr.size()) return nullptr;
         return MakeUnique<WSHDescriptor>(std::move(desc));
+    } else if (ctx == ParseScriptContext::P2WSH && Func("wsh", expr)) {
+        error = "Cannot have wsh within wsh";
+        return nullptr;
     }
     if (ctx == ParseScriptContext::TOP && Func("addr", expr)) {
         CTxDestination dest = DecodeDestination(std::string(expr.begin(), expr.end()));
@@ -870,6 +912,13 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
         auto bytes = ParseHex(str);
         return MakeUnique<RawDescriptor>(CScript(bytes.begin(), bytes.end()));
     }
+    if (ctx == ParseScriptContext::P2SH) {
+        error = "A function is needed within P2SH";
+        return nullptr;
+    } else if (ctx == ParseScriptContext::P2WSH) {
+        error = "A function is needed within P2WSH";
+        return nullptr;
+    }
     error = strprintf("%s is not a valid descriptor function", std::string(expr.begin(), expr.end()));
     return nullptr;
 }
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index fad0aefabd..7707d6233b 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -1101,7 +1101,7 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
     std::string error;
     auto parsed_desc = Parse(descriptor, keys, error, /* require_checksum = */ true);
     if (!parsed_desc) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Descriptor is invalid, %s", error));
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
     }
 
     have_solving_data = parsed_desc->IsSolvable();
diff --git a/test/functional/rpc_deriveaddresses.py b/test/functional/rpc_deriveaddresses.py
index 1984694692..42128d5767 100755
--- a/test/functional/rpc_deriveaddresses.py
+++ b/test/functional/rpc_deriveaddresses.py
@@ -13,14 +13,14 @@ class DeriveaddressesTest(BitcoinTestFramework):
         self.supports_cli = 1
 
     def run_test(self):
-        assert_raises_rpc_error(-5, "Invalid descriptor", self.nodes[0].deriveaddresses, "a")
+        assert_raises_rpc_error(-5, "Missing checksum", self.nodes[0].deriveaddresses, "a")
 
         descriptor = "wpkh(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/0)#t6wfjs64"
         address = "bcrt1qjqmxmkpmxt80xz4y3746zgt0q3u3ferr34acd5"
         assert_equal(self.nodes[0].deriveaddresses(descriptor), [address])
 
         descriptor = descriptor[:-9]
-        assert_raises_rpc_error(-5, "Invalid descriptor", self.nodes[0].deriveaddresses, descriptor)
+        assert_raises_rpc_error(-5, "Missing checksum", self.nodes[0].deriveaddresses, descriptor)
 
         descriptor_pubkey = "wpkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0)#s9ga3alw"
         address = "bcrt1qjqmxmkpmxt80xz4y3746zgt0q3u3ferr34acd5"
diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py
index 4a8fcbf6d0..e4a4ab1f35 100755
--- a/test/functional/wallet_importmulti.py
+++ b/test/functional/wallet_importmulti.py
@@ -552,7 +552,7 @@ class ImportMultiTest(BitcoinTestFramework):
                                "keys": [key.privkey]},
                               success=False,
                               error_code=-5,
-                              error_message="Descriptor is invalid, Missing checksum")
+                              error_message="Missing checksum")
 
         # Test importing of a P2SH-P2WPKH address via descriptor + private key
         key = get_key(self.nodes[0])