Skip to content

Commit 625534d

Browse files
achow101achow101
committed
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.
1 parent c325f61 commit 625534d

File tree

6 files changed

+67
-18
lines changed

6 files changed

+67
-18
lines changed

‎src/rpc/misc.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
153153
std::string error;
154154
auto desc = Parse(request.params[0].get_str(), provider, error);
155155
if (!desc) {
156-
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor, %s", error));
156+
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
157157
}
158158

159159
UniValue result(UniValue::VOBJ);
@@ -203,7 +203,7 @@ UniValue deriveaddresses(const JSONRPCRequest& request)
203203
std::string error;
204204
auto desc = Parse(desc_str, key_provider, error, /* require_checksum = */ true);
205205
if (!desc) {
206-
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor, %s", error));
206+
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
207207
}
208208

209209
if (!desc->IsRange() && request.params.size() > 1) {

‎src/rpc/util.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,7 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
720720
std::string error;
721721
auto desc = Parse(desc_str, provider, error);
722722
if (!desc) {
723-
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor '%s', %s", desc_str, error));
723+
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
724724
}
725725
if (!desc->IsRange()) {
726726
range.first = 0;

‎src/script/descriptor.cpp

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,10 @@ NODISCARD bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath&
700700
hardened = true;
701701
}
702702
uint32_t p;
703-
if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p) || p > 0x7FFFFFFFUL) {
703+
if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p)) {
704+
error = strprintf("Key path value '%s' is not a valid uint32", std::string(elem.begin(), elem.end()).c_str());
705+
return false;
706+
} else if (p > 0x7FFFFFFFUL) {
704707
error = strprintf("Key path value %u is out of range", p);
705708
return false;
706709
}
@@ -714,17 +717,35 @@ std::unique_ptr<PubkeyProvider> ParsePubkeyInner(const Span<const char>& sp, boo
714717
{
715718
auto split = Split(sp, '/');
716719
std::string str(split[0].begin(), split[0].end());
720+
if (str.size() == 0) {
721+
error = "No key provided";
722+
return nullptr;
723+
}
717724
if (split.size() == 1) {
718725
if (IsHex(str)) {
719726
std::vector<unsigned char> data = ParseHex(str);
720727
CPubKey pubkey(data);
721-
if (pubkey.IsFullyValid() && (permit_uncompressed || pubkey.IsCompressed())) return MakeUnique<ConstPubkeyProvider>(pubkey);
728+
if (pubkey.IsFullyValid()) {
729+
if (permit_uncompressed || pubkey.IsCompressed()) {
730+
return MakeUnique<ConstPubkeyProvider>(pubkey);
731+
} else {
732+
error = "Uncompressed keys are not allowed";
733+
return nullptr;
734+
}
735+
}
736+
error = strprintf("Pubkey '%s' is invalid", str);
737+
return nullptr;
722738
}
723739
CKey key = DecodeSecret(str);
724-
if (key.IsValid() && (permit_uncompressed || key.IsCompressed())) {
725-
CPubKey pubkey = key.GetPubKey();
726-
out.keys.emplace(pubkey.GetID(), key);
727-
return MakeUnique<ConstPubkeyProvider>(pubkey);
740+
if (key.IsValid()) {
741+
if (permit_uncompressed || key.IsCompressed()) {
742+
CPubKey pubkey = key.GetPubKey();
743+
out.keys.emplace(pubkey.GetID(), key);
744+
return MakeUnique<ConstPubkeyProvider>(pubkey);
745+
} else {
746+
error = "Uncompressed keys are not allowed";
747+
return nullptr;
748+
}
728749
}
729750
}
730751
CExtKey extkey = DecodeExtKey(str);
@@ -760,7 +781,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(const Span<const char>& sp, bool per
760781
}
761782
if (origin_split.size() == 1) return ParsePubkeyInner(origin_split[0], permit_uncompressed, out, error);
762783
if (origin_split[0].size() < 1 || origin_split[0][0] != '[') {
763-
error = strprintf("Key origin expected but not found, got '%s' instead", std::string(origin_split[0].begin(), origin_split[0].end()));
784+
error = strprintf("Key origin start '[ character expected but not found, got '%c' instead", origin_split[0][0]);
764785
return nullptr;
765786
}
766787
auto slash_split = Split(origin_split[0].subspan(1), '/');
@@ -802,19 +823,22 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
802823
auto pubkey = ParsePubkey(expr, true, out, error);
803824
if (!pubkey) return nullptr;
804825
return MakeUnique<ComboDescriptor>(std::move(pubkey));
826+
} else if (ctx != ParseScriptContext::TOP && Func("combo", expr)) {
827+
error = "Cannot have combo in non-top level";
828+
return nullptr;
805829
}
806830
if (Func("multi", expr)) {
807831
auto threshold = Expr(expr);
808832
uint32_t thres;
809833
std::vector<std::unique_ptr<PubkeyProvider>> providers;
810834
if (!ParseUInt32(std::string(threshold.begin(), threshold.end()), &thres)) {
811-
error = strprintf("multi threshold %u out of range", thres);
835+
error = strprintf("Multi threshold '%s' is not valid", std::string(threshold.begin(), threshold.end()).c_str());
812836
return nullptr;
813837
}
814838
size_t script_size = 0;
815839
while (expr.size()) {
816840
if (!Const(",", expr)) {
817-
error = strprintf("multi: expected ',', got '%c'", expr[0]);
841+
error = strprintf("Multi: expected ',', got '%c'", expr[0]);
818842
return nullptr;
819843
}
820844
auto arg = Expr(expr);
@@ -823,10 +847,19 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
823847
script_size += pk->GetSize() + 1;
824848
providers.emplace_back(std::move(pk));
825849
}
826-
if (providers.size() < 1 || providers.size() > 16 || thres < 1 || thres > providers.size()) return nullptr;
850+
if (providers.size() < 1 || providers.size() > 16) {
851+
error = strprintf("Cannot have %u keys in multisig; must have between 1 and 16 keys, inclusive", providers.size());
852+
return nullptr;
853+
} else if (thres < 1) {
854+
error = strprintf("Multisig threshold cannot be %d, must be at least 1", thres);
855+
return nullptr;
856+
} else if (thres > providers.size()) {
857+
error = strprintf("Multisig threshold cannot be larger than the number of keys; threshold is %d but only %u keys specified", thres, providers.size());
858+
return nullptr;
859+
}
827860
if (ctx == ParseScriptContext::TOP) {
828861
if (providers.size() > 3) {
829-
error = strprintf("Cannot %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
862+
error = strprintf("Cannot have %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
830863
return nullptr;
831864
}
832865
}
@@ -842,16 +875,25 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
842875
auto pubkey = ParsePubkey(expr, false, out, error);
843876
if (!pubkey) return nullptr;
844877
return MakeUnique<WPKHDescriptor>(std::move(pubkey));
878+
} else if (ctx == ParseScriptContext::P2WSH && Func("wpkh", expr)) {
879+
error = "Cannot have wpkh within wsh";
880+
return nullptr;
845881
}
846882
if (ctx == ParseScriptContext::TOP && Func("sh", expr)) {
847883
auto desc = ParseScript(expr, ParseScriptContext::P2SH, out, error);
848884
if (!desc || expr.size()) return nullptr;
849885
return MakeUnique<SHDescriptor>(std::move(desc));
886+
} else if (ctx != ParseScriptContext::TOP && Func("sh", expr)) {
887+
error = "Cannot have sh in non-top level";
888+
return nullptr;
850889
}
851890
if (ctx != ParseScriptContext::P2WSH && Func("wsh", expr)) {
852891
auto desc = ParseScript(expr, ParseScriptContext::P2WSH, out, error);
853892
if (!desc || expr.size()) return nullptr;
854893
return MakeUnique<WSHDescriptor>(std::move(desc));
894+
} else if (ctx == ParseScriptContext::P2WSH && Func("wsh", expr)) {
895+
error = "Cannot have wsh within wsh";
896+
return nullptr;
855897
}
856898
if (ctx == ParseScriptContext::TOP && Func("addr", expr)) {
857899
CTxDestination dest = DecodeDestination(std::string(expr.begin(), expr.end()));
@@ -870,6 +912,13 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
870912
auto bytes = ParseHex(str);
871913
return MakeUnique<RawDescriptor>(CScript(bytes.begin(), bytes.end()));
872914
}
915+
if (ctx == ParseScriptContext::P2SH) {
916+
error = "A function is needed within P2SH";
917+
return nullptr;
918+
} else if (ctx == ParseScriptContext::P2WSH) {
919+
error = "A function is needed within P2WSH";
920+
return nullptr;
921+
}
873922
error = strprintf("%s is not a valid descriptor function", std::string(expr.begin(), expr.end()));
874923
return nullptr;
875924
}

‎src/wallet/rpcdump.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1101,7 +1101,7 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
11011101
std::string error;
11021102
auto parsed_desc = Parse(descriptor, keys, error, /* require_checksum = */ true);
11031103
if (!parsed_desc) {
1104-
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Descriptor is invalid, %s", error));
1104+
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
11051105
}
11061106

11071107
have_solving_data = parsed_desc->IsSolvable();

‎test/functional/rpc_deriveaddresses.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ def set_test_params(self):
1313
self.supports_cli = 1
1414

1515
def run_test(self):
16-
assert_raises_rpc_error(-5, "Invalid descriptor", self.nodes[0].deriveaddresses, "a")
16+
assert_raises_rpc_error(-5, "Missing checksum", self.nodes[0].deriveaddresses, "a")
1717

1818
descriptor = "wpkh(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/0)#t6wfjs64"
1919
address = "bcrt1qjqmxmkpmxt80xz4y3746zgt0q3u3ferr34acd5"
2020
assert_equal(self.nodes[0].deriveaddresses(descriptor), [address])
2121

2222
descriptor = descriptor[:-9]
23-
assert_raises_rpc_error(-5, "Invalid descriptor", self.nodes[0].deriveaddresses, descriptor)
23+
assert_raises_rpc_error(-5, "Missing checksum", self.nodes[0].deriveaddresses, descriptor)
2424

2525
descriptor_pubkey = "wpkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0)#s9ga3alw"
2626
address = "bcrt1qjqmxmkpmxt80xz4y3746zgt0q3u3ferr34acd5"

‎test/functional/wallet_importmulti.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,7 @@ def run_test(self):
552552
"keys": [key.privkey]},
553553
success=False,
554554
error_code=-5,
555-
error_message="Descriptor is invalid, Missing checksum")
555+
error_message="Missing checksum")
556556

557557
# Test importing of a P2SH-P2WPKH address via descriptor + private key
558558
key = get_key(self.nodes[0])

0 commit comments

Comments
 (0)