From 625534d7b1417da926f1ced600855ea818d6e01e Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 2 Aug 2019 19:19:53 -0400 Subject: 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. --- src/rpc/misc.cpp | 4 +- src/rpc/util.cpp | 2 +- src/script/descriptor.cpp | 71 ++++++++++++++++++++++++++++------ src/wallet/rpcdump.cpp | 2 +- test/functional/rpc_deriveaddresses.py | 4 +- test/functional/wallet_importmulti.py | 2 +- 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 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>& 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 ParsePubkeyInner(const Span& 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 data = ParseHex(str); CPubKey pubkey(data); - if (pubkey.IsFullyValid() && (permit_uncompressed || pubkey.IsCompressed())) return MakeUnique(pubkey); + if (pubkey.IsFullyValid()) { + if (permit_uncompressed || pubkey.IsCompressed()) { + return MakeUnique(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(pubkey); + if (key.IsValid()) { + if (permit_uncompressed || key.IsCompressed()) { + CPubKey pubkey = key.GetPubKey(); + out.keys.emplace(pubkey.GetID(), key); + return MakeUnique(pubkey); + } else { + error = "Uncompressed keys are not allowed"; + return nullptr; + } } } CExtKey extkey = DecodeExtKey(str); @@ -760,7 +781,7 @@ std::unique_ptr ParsePubkey(const Span& 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 ParseScript(Span& sp, ParseScriptCon auto pubkey = ParsePubkey(expr, true, out, error); if (!pubkey) return nullptr; return MakeUnique(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> 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 ParseScript(Span& 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 ParseScript(Span& sp, ParseScriptCon auto pubkey = ParsePubkey(expr, false, out, error); if (!pubkey) return nullptr; return MakeUnique(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(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(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 ParseScript(Span& sp, ParseScriptCon auto bytes = ParseHex(str); return MakeUnique(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::mapIsSolvable(); 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]) -- cgit v1.2.3