Return more specific errors about invalid descriptors #16542

achow101 · 2019-08-02T23:42:44Z

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

sipa · 2019-08-02T23:53:54Z

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

Eh, I mean, big concept ACK.

DrahtBot · 2019-08-03T02:29:19Z

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 · 2019-08-03T06:19:29Z

Concept ACK

Sjors · 2019-08-03T11:12:02Z

Concept ACK

promag

Concept ACK, nice to have a detailed error.

hugohn · 2019-08-05T19:05:05Z

Concept ACK.

Sjors

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.

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

src/script/descriptor.cpp

Sjors · 2019-08-06T11:46:23Z

test/functional/wallet_importmulti.py

nit: make descriptor errors start with lower case?

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

I've just removed the prefixes.

They're back

They're back

No??

practicalswift · 2019-08-08T08:41:43Z

src/test/descriptor_tests.cpp

Nit: expected_error could be const ref?

promag

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.

src/script/descriptor.cpp

promag · 2019-08-08T09:31:32Z

test/functional/wallet_importmulti.py

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

achow101 · 2019-08-08T20:17:13Z

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

Sjors · 2019-08-09T19:06:35Z

reACK a1b47955838f1151fc1904d0ff3bc7751b9fa7ca

jb55 · 2019-08-10T16:08:36Z

Code Review ACK a1b47955838f1151fc1904d0ff3bc7751b9fa7ca

instagibbs

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.

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

^ space after comma

instagibbs · 2019-08-13T14:08:42Z

src/script/descriptor.cpp

where would this case be hit previously?

It would fail below at the extkey decoding stuff.

instagibbs · 2019-08-13T14:09:59Z

src/script/descriptor.cpp

where would this case be hit previously?

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

instagibbs · 2019-08-13T14:16:52Z

src/script/descriptor.cpp

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.

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

Sjors · 2019-08-14T14:52:17Z

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 · 2019-08-16T22:07:25Z

Needs rebase

meshcollider · 2019-08-16T22:09:15Z

Sorry about the rebase again due to #15986

Once this is rebased I'll review and merge

…ut what failed

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.

achow101 · 2019-08-17T00:14:37Z

Rebased

Sjors

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

src/script/descriptor.cpp

Sjors · 2019-08-17T11:36:24Z

test/functional/wallet_importmulti.py

They're back

achow101 · 2019-08-17T17:07:13Z

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 · 2019-08-17T17:20:48Z

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

ACK 787c9ec

meshcollider

Code review ACK 787c9ec

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 · 2019-08-18T10:16:02Z

src/script/descriptor.cpp

    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]);


mismatched ' (also in the corresponding test case)

787c9ec Additional tests for other failure cases (Andrew Chow) 6e1ae58 Check error messages in descriptor tests (Andrew Chow) 625534d Give more errors for specific failure conditions (Andrew Chow) c325f61 Return an error from descriptor Parse that gives more information about what failed (Andrew Chow) Pull request description: Because it's nigh impossible to figure out what is wrong with a descriptor otherwise. ACKs for top commit: Sjors: ACK 787c9ec meshcollider: Code review ACK 787c9ec Tree-SHA512: 07c5deff127fd65507b96df2e4608b4f918e0cbd20b3b45b9ab5a05c2985a6efa9832079cf1e7f504305bfbb861019e93b96cc1574dd63b2ff0756b5928ece20

…ptors 787c9ec Additional tests for other failure cases (Andrew Chow) 6e1ae58 Check error messages in descriptor tests (Andrew Chow) 625534d Give more errors for specific failure conditions (Andrew Chow) c325f61 Return an error from descriptor Parse that gives more information about what failed (Andrew Chow) Pull request description: Because it's nigh impossible to figure out what is wrong with a descriptor otherwise. ACKs for top commit: Sjors: ACK 787c9ec meshcollider: Code review ACK 787c9ec Tree-SHA512: 07c5deff127fd65507b96df2e4608b4f918e0cbd20b3b45b9ab5a05c2985a6efa9832079cf1e7f504305bfbb861019e93b96cc1574dd63b2ff0756b5928ece20

… more information about what failed Summary: bitcoin/bitcoin@c325f61 --- Depends on D6641 Partial backport of Core [[bitcoin/bitcoin#16542 | PR16542]] Test Plan: ninja check ninja test_runner.py wallet_importmulti Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6642

Summary: 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. bitcoin/bitcoin@625534d --- Depends on D6642 Partial backport of Core [[bitcoin/bitcoin#16542 | PR16542]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6643

Summary: bitcoin/bitcoin@6e1ae58 --- Depends on D6643 Partial backport of Core [[bitcoin/bitcoin#16542 | PR16542]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6648

Summary: bitcoin/bitcoin@787c9ec --- Depends on D6648 Concludes backport of Core [[bitcoin/bitcoin#16542 | PR16542]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6649

fanquake added the Wallet label Aug 2, 2019

fanquake added the Descriptors label Aug 3, 2019

promag reviewed Aug 3, 2019

View reviewed changes

Sjors approved these changes Aug 6, 2019

View reviewed changes

achow101 force-pushed the descriptor-errors branch from dae2590 to 0d55f50 Compare August 6, 2019 19:38

practicalswift reviewed Aug 8, 2019

View reviewed changes

promag reviewed Aug 8, 2019

View reviewed changes

achow101 force-pushed the descriptor-errors branch from 0d55f50 to a1b4795 Compare August 8, 2019 20:17

instagibbs approved these changes Aug 13, 2019

View reviewed changes

DrahtBot added the Needs rebase label Aug 16, 2019

achow101 added 4 commits August 16, 2019 19:34

Return an error from descriptor Parse that gives more information abo…

c325f61

…ut what failed

Give more errors for specific failure conditions

625534d

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.

Check error messages in descriptor tests

6e1ae58

Additional tests for other failure cases

787c9ec

achow101 force-pushed the descriptor-errors branch from a1b4795 to 787c9ec Compare August 17, 2019 00:15

fanquake removed the Needs rebase label Aug 17, 2019

Sjors suggested changes Aug 17, 2019

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

test/functional/wallet_importmulti.py Outdated

Copy link

Member

Sjors Aug 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're back

meshcollider reviewed Aug 18, 2019

View reviewed changes

meshcollider merged commit 787c9ec into bitcoin:master Aug 18, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Return more specific errors about invalid descriptors #16542

Return more specific errors about invalid descriptors #16542

Uh oh!

Conversation

achow101 commented Aug 2, 2019

Uh oh!

sipa commented Aug 2, 2019

Uh oh!

DrahtBot commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jb55 commented Aug 3, 2019 via email

Uh oh!

Sjors commented Aug 3, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hugohn commented Aug 5, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 8, 2019

Uh oh!

Sjors commented Aug 9, 2019

Uh oh!

jb55 commented Aug 10, 2019

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Aug 14, 2019

Uh oh!

DrahtBot commented Aug 16, 2019

Uh oh!

DrahtBot commented Aug 3, 2019 •

edited

Loading