Return more specific errors about invalid descriptors

achow101 commented at 11:42 pm on August 2, 2019: member

Because it’s nigh impossible to figure out what is wrong with a descriptor otherwise.

fanquake added the label Wallet on Aug 2, 2019

sipa commented at 11:53 pm on August 2, 2019: member

Descriptors are hard to parse, they should be hard to write also.

Eh, I mean, big concept ACK.

fanquake added the label Descriptors on Aug 3, 2019

DrahtBot commented at 2:29 am on August 3, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16570 (Make descriptor tests deterministic by davereikher)
#15590 (Descriptor: add GetAddressType() and IsSegWit() by Sjors)

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.

jb55 commented at 6:19 am on August 3, 2019: member

Concept ACK

Sjors commented at 11:12 am on August 3, 2019: member

Concept ACK

promag commented at 4:57 pm on August 3, 2019: member

Concept ACK, nice to have a detailed error.

hugohn commented at 7:05 pm on August 5, 2019: contributor

Concept ACK.

in src/script/descriptor.cpp:707 in 735c1f0427 outdated

695@@ -696,14 +696,17 @@ NODISCARD bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath&
696             hardened = true;
697         }
698         uint32_t p;
699-        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p) || p > 0x7FFFFFFFUL) return false;
700+        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p) || p > 0x7FFFFFFFUL) {
701+            error = strprintf("Key path value %u is out of range", p);

Sjors commented at 11:34 am on August 6, 2019:

Nit: “not valid” instead of “out of range”? Or split the check?

achow101 commented at 7:39 pm on August 6, 2019:

I’ve split the check

Sjors commented at 11:35 am on August 17, 2019:

Now it’s no longer split.

achow101 commented at 5:08 pm on August 17, 2019:

Now it’s no longer split.

No??

in test/functional/wallet_importmulti.py:555 in 735c1f0427 outdated

551@@ -552,7 +552,7 @@ def run_test(self):
552                                "keys": [key.privkey]},
553                               success=False,
554                               error_code=-5,
555-                              error_message="Descriptor is invalid")
556+                              error_message="Descriptor is invalid, Missing checksum")

Sjors commented at 11:46 am on August 6, 2019:

nit: make descriptor errors start with lower case?

achow101 commented at 7:30 pm on August 6, 2019:

Nah.

promag commented at 9:31 am on August 8, 2019:

Agree with @Sjors, is there a reason to not do so? I was going to suggest to drop “Descriptor is invalid, " prefix.

achow101 commented at 8:17 pm on August 8, 2019:

I’ve just removed the prefixes.

Sjors commented at 11:36 am on August 17, 2019:

They’re back

achow101 commented at 5:07 pm on August 17, 2019:

They’re back

No??

Sjors approved

Sjors commented at 11:46 am on August 6, 2019: member

ACK dae2590

This introduces a fun little trick if you’re too lazy to call getdescriptorinfo to obtain the checkum: you can instead set the checksum to 00000000.

0$ bitcoin-cli importmulti '[{"desc": "sh(wpkh(03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556))#00000000", "timestamp": "now"}]'
1[
2  {
3    "success": false,
4    "error": {
5      "code": -5,
6      "message": "Descriptor is invalid, Provided checksum '00000000' does not match computed checksum 'qkrrc7je'"
7    }
8  }
9]

achow101 force-pushed on Aug 6, 2019

in src/test/descriptor_tests.cpp:16 in 0d55f500cb outdated

12@@ -13,13 +13,15 @@
13 
14 namespace {
15 
16-void CheckUnparsable(const std::string& prv, const std::string& pub)
17+void CheckUnparsable(const std::string& prv, const std::string& pub, std::string expected_error)

practicalswift commented at 8:41 am on August 8, 2019:

Nit: expected_error could be const ref?

achow101 commented at 8:17 pm on August 8, 2019:

Done

in src/script/descriptor.cpp:704 in 0d55f500cb outdated

695@@ -696,33 +696,60 @@ NODISCARD bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath&
696             hardened = true;
697         }
698         uint32_t p;
699-        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p) || p > 0x7FFFFFFFUL) return false;
700+        if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p)) {
701+            error = strprintf("Key path value '%s' is not a valid uint32", std::string(elem.begin(), elem.end()).c_str());

promag commented at 9:08 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:736 in 0d55f500cb outdated

730+                } else {
731+                    error = "Uncompressed keys are not allowed";
732+                    return nullptr;
733+                }
734+            }
735+            error = strprintf("Pubkey '%s' is invalid", str);

promag commented at 9:10 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:780 in 0d55f500cb outdated

779+        error = "Multiple ']' characters found for a single pubkey";
780+        return nullptr;
781+    }
782+    if (origin_split.size() == 1) return ParsePubkeyInner(origin_split[0], permit_uncompressed, out, error);
783+    if (origin_split[0].size() < 1 || origin_split[0][0] != '[') {
784+        error = strprintf("Key origin expected but not found, got '%s' instead", std::string(origin_split[0].begin(), origin_split[0].end()));

promag commented at 9:19 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:831 in 0d55f500cb outdated

839         auto threshold = Expr(expr);
840         uint32_t thres;
841         std::vector<std::unique_ptr<PubkeyProvider>> providers;
842-        if (!ParseUInt32(std::string(threshold.begin(), threshold.end()), &thres)) return nullptr;
843+        if (!ParseUInt32(std::string(threshold.begin(), threshold.end()), &thres)) {
844+            error = strprintf("multi threshold %u out of range", thres);

promag commented at 9:20 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:837 in 0d55f500cb outdated

846+        }
847         size_t script_size = 0;
848         while (expr.size()) {
849-            if (!Const(",", expr)) return nullptr;
850+            if (!Const(",", expr)) {
851+                error = strprintf("multi: expected ',', got '%c'", expr[0]);

promag commented at 9:21 am on August 8, 2019:

Missing test.

achow101 commented at 7:55 pm on August 8, 2019:

I don’t think this check is actually possible to hit due to how Expr parses things earlier.

in src/script/descriptor.cpp:851 in 0d55f500cb outdated

858             script_size += pk->GetSize() + 1;
859             providers.emplace_back(std::move(pk));
860         }
861-        if (providers.size() < 1 || providers.size() > 16 || thres < 1 || thres > providers.size()) return nullptr;
862+        if (providers.size() < 1 || providers.size() > 16) {
863+            error = strprintf("Cannot have %u keys in multisig; must have between 1 and 16 keys, inclusive", providers.size());

promag commented at 9:23 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:850 in 0d55f500cb outdated

861-        if (providers.size() < 1 || providers.size() > 16 || thres < 1 || thres > providers.size()) return nullptr;
862+        if (providers.size() < 1 || providers.size() > 16) {
863+            error = strprintf("Cannot have %u keys in multisig; must have between 1 and 16 keys, inclusive", providers.size());
864+            return nullptr;
865+        } else if (thres < 1) {
866+            error = strprintf("multisig threshold cannot be %d, must be at least 1", thres);

promag commented at 9:23 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:853 in 0d55f500cb outdated

864+            return nullptr;
865+        } else if (thres < 1) {
866+            error = strprintf("multisig threshold cannot be %d, must be at least 1", thres);
867+            return nullptr;
868+        } else if (thres > providers.size()) {
869+            error = strprintf("Multisig threshold cannot be larger than the number of keys; thoreshold is %d but only %u keys specified", thres, providers.size());

promag commented at 9:23 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:858 in 0d55f500cb outdated

870+            return nullptr;
871+        }
872         if (ctx == ParseScriptContext::TOP) {
873-            if (providers.size() > 3) return nullptr; // Not more than 3 pubkeys for raw multisig
874+            if (providers.size() > 3) {
875+                error = strprintf("Cannot %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());

promag commented at 9:24 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:901 in 0d55f500cb outdated

914     }
915     if (ctx == ParseScriptContext::TOP && Func("addr", expr)) {
916         CTxDestination dest = DecodeDestination(std::string(expr.begin(), expr.end()));
917-        if (!IsValidDestination(dest)) return nullptr;
918+        if (!IsValidDestination(dest)) {
919+            error = "Address is not valid";

promag commented at 9:25 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:909 in 0d55f500cb outdated

923     }
924     if (ctx == ParseScriptContext::TOP && Func("raw", expr)) {
925         std::string str(expr.begin(), expr.end());
926-        if (!IsHex(str)) return nullptr;
927+        if (!IsHex(str)) {
928+            error = "Raw script is not hex";

promag commented at 9:25 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:1008 in 0d55f500cb outdated

1004     // Checksum checks
1005     auto check_split = Split(sp, '#');
1006-    if (check_split.size() > 2) return nullptr; // Multiple '#' symbols
1007-    if (check_split.size() == 1 && require_checksum) return nullptr; // Missing checksum
1008+    if (check_split.size() > 2) {
1009+        error = "Multiple '#' symbols";

promag commented at 9:27 am on August 8, 2019:

Missing test.

in src/script/descriptor.cpp:1020 in 0d55f500cb outdated

1021+        }
1022         auto checksum = DescriptorChecksum(check_split[0]);
1023-        if (checksum.empty()) return nullptr; // Invalid characters in payload
1024-        if (!std::equal(checksum.begin(), checksum.end(), check_split[1].begin())) return nullptr; // Checksum mismatch
1025+        if (checksum.empty()) {
1026+            error = "Invalid characters in payload";

promag commented at 9:27 am on August 8, 2019:

Missing test.

promag commented at 9:34 am on August 8, 2019: member

I think I’ve bookmarked all errors without a test - not saying you should add them here, I’m happy to improve in a follow up.

achow101 force-pushed on Aug 8, 2019

achow101 commented at 8:17 pm on August 8, 2019: member

I’ve added more tests and also cleaned up a few typos in error messages.

Sjors commented at 7:06 pm on August 9, 2019: member

reACK a1b47955838f1151fc1904d0ff3bc7751b9fa7ca

jb55 commented at 4:08 pm on August 10, 2019: member

Code Review ACK a1b47955838f1151fc1904d0ff3bc7751b9fa7ca

in src/script/descriptor.cpp:746 in 441d29d900 outdated

742+            if (permit_uncompressed || key.IsCompressed()) {
743+                CPubKey pubkey = key.GetPubKey();
744+                out.keys.emplace(pubkey.GetID(), key);
745+                return MakeUnique<ConstPubkeyProvider>(pubkey);
746+            } else {
747+                error = "Uncompressed keys are not allowed";

instagibbs commented at 2:08 pm on August 13, 2019:

where would this case be hit previously?

achow101 commented at 4:59 pm on August 13, 2019:

It would fail below at the extkey decoding stuff.

in src/script/descriptor.cpp:826 in 441d29d900 outdated

818@@ -798,19 +819,22 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
819         auto pubkey = ParsePubkey(expr, true, out, error);
820         if (!pubkey) return nullptr;
821         return MakeUnique<ComboDescriptor>(std::move(pubkey));
822+    } else if (ctx != ParseScriptContext::TOP && Func("combo", expr)) {

instagibbs commented at 2:09 pm on August 13, 2019:

where would this case be hit previously?

achow101 commented at 5:00 pm on August 13, 2019:

It would just get to the end of the function and hit return nullptr.

in src/script/descriptor.cpp:915 in 441d29d900 outdated

907@@ -866,6 +908,13 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
908         auto bytes = ParseHex(str);
909         return MakeUnique<RawDescriptor>(CScript(bytes.begin(), bytes.end()));
910     }
911+    if (ctx == ParseScriptContext::P2SH) {

instagibbs commented at 2:16 pm on August 13, 2019:

nit: I’d prefer it also checks for !Func explicitly, since there are a number of combinations, and a generic error is preferable to a specific error that’s an implementation mistake.

achow101 commented at 5:44 pm on August 14, 2019:

I don’t think it can check that there is no function.

instagibbs approved

instagibbs commented at 2:27 pm on August 13, 2019: member

tACK

the errors I encountered during initial usage of descriptors are all caught here, and enough information to quickly figure out what I did wrong.

f.e.

0getdescriptorinfo "multi(1, 03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)"
1error code: -5
2error message:
3key ' 03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd' is not valid

^ space after comma

Sjors commented at 2:52 pm on August 14, 2019: member

If you need to make any changes, I suggest rebasing due to #14934 to be on the safe side. It still compiles and tests pass for me when I do that rebase locally though.

DrahtBot commented at 10:07 pm on August 16, 2019: member

DrahtBot added the label Needs rebase on Aug 16, 2019

meshcollider commented at 10:09 pm on August 16, 2019: contributor

Sorry about the rebase again due to #15986

Once this is rebased I’ll review and merge

Return an error from descriptor Parse that gives more information about what failed c325f619dd

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.

625534d7b1

Check error messages in descriptor tests 6e1ae58298

Additional tests for other failure cases 787c9ec0c3

achow101 commented at 0:14 am on August 17, 2019: member

Rebased

achow101 force-pushed on Aug 17, 2019

fanquake removed the label Needs rebase on Aug 17, 2019

Sjors changes_requested

Sjors commented at 11:36 am on August 17, 2019: member

Only the first commit changed in this rebase, but you lost a few of your recent fixes in the process.

achow101 commented at 5:07 pm on August 17, 2019: member

Only the first commit changed in this rebase, but you lost a few of your recent fixes in the process.

No??? Are you sure you’re looking at the right commits? The latest diff includes everything that was commented on earlier.

Sjors commented at 5:20 pm on August 17, 2019: member

Ah wait, I see these changes happen in 625534d7b1417da926f1ced600855ea818d6e01e.

ACK 787c9ec

in src/script/descriptor.cpp:784 in 625534d7b1 outdated

780@@ -760,7 +781,7 @@ std::unique_ptr<PubkeyProvider> ParsePubkey(const Span<const char>& sp, bool per
781     }
782     if (origin_split.size() == 1) return ParsePubkeyInner(origin_split[0], permit_uncompressed, out, error);
783     if (origin_split[0].size() < 1 || origin_split[0][0] != '[') {
784-        error = strprintf("Key origin expected but not found, got '%s' instead", std::string(origin_split[0].begin(), origin_split[0].end()));
785+        error = strprintf("Key origin start '[ character expected but not found, got '%c' instead", origin_split[0][0]);

meshcollider commented at 10:16 am on August 18, 2019:

mismatched ' (also in the corresponding test case)

meshcollider commented at 10:21 am on August 18, 2019: contributor

Code review ACK 787c9ec0c383cefb83a95433311a27f9c220dca0

This is a nice change, one tiny nit but it is pretty insignificant so I’ll merge this tomorrow if you don’t have time to fix it now

meshcollider referenced this in commit e8a6d52724 on Aug 18, 2019

meshcollider merged this on Aug 18, 2019

meshcollider closed this on Aug 18, 2019

sidhujag referenced this in commit 723d839842 on Aug 19, 2019

jasonbcox referenced this in commit 2abb29db9d on Jul 1, 2020

jasonbcox referenced this in commit a2c1a34f16 on Jul 1, 2020

jasonbcox referenced this in commit ed3d5a8dfe on Jul 1, 2020

jasonbcox referenced this in commit 828b2e9643 on Jul 1, 2020

kittywhiskers referenced this in commit bb1395558c on Dec 4, 2021

kittywhiskers referenced this in commit f581fa919d on Dec 8, 2021

kittywhiskers referenced this in commit 889391b9dc on Dec 8, 2021

kittywhiskers referenced this in commit 4bf8b707a6 on Dec 12, 2021

kittywhiskers referenced this in commit a3facde466 on Dec 12, 2021

kittywhiskers referenced this in commit 44f5cf5490 on Dec 12, 2021

kittywhiskers referenced this in commit 55e7b91def on Dec 13, 2021

kittywhiskers referenced this in commit d23e328064 on Dec 13, 2021

kittywhiskers referenced this in commit bf2801798b on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021

Return more specific errors about invalid descriptors #16542

Conflicts