Add checksum to getdescriptorinfo #15986

1. 
   sipa commented at 9:21 pm on May 8, 2019: member
2. sipa force-pushed on May 8, 2019
3. 
   gwillen commented at 9:27 pm on May 8, 2019: contributor

   So the only reason I didn’t write it this way, is that people have said they don’t want private keys to be echoed in the output.

   Personally I think this is fine – the only way to get a private key in the output, is if you include it in the input. And in fact, there has to be SOME way to get private keys echoed back, if you want to be able to get checksums for descriptors with private keys included.

   So I’d advocate for this PR the way it is, but if people aren’t comfortable with it, I’d suggest adding a flag, and not echoing descriptors with private keys unless the flag is set.

   Thanks!

   (Tagging #15740 for the record.)

4. DrahtBot added the label RPC/REST/ZMQ on May 8, 2019
5. DrahtBot added the label Tests on May 8, 2019
6. 
   meshcollider commented at 5:56 am on May 9, 2019: contributor

   utACK https://github.com/bitcoin/bitcoin/pull/15986/commits/15c482ef711a166b74dcd1fa127d213799d2a1d1
7. 
   Sjors commented at 12:32 pm on May 12, 2019: member

   Concept ACK.

   Light preference for returning only the checksum rather than echoing a private key. But I do agree with @gwillen’s point:

   the only way to get a private key in the output, is if you include it in the input.

   I find the extra with_checksum confusing. Maybe we can add an argument keep_private_keys which defaults to false. I would also prefer sticking to canonical form (but with private keys). An additional argument canonical can be true by default. I know it makes the change a bit more complicated, but it seems more robust in the long run.

8. 
   DrahtBot commented at 4:41 am on May 30, 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:

   • #16542 (Return more specific errors about invalid descriptors by achow101)

   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.

9. 
   jb55 commented at 6:15 pm on May 30, 2019: member

   I’m confused by this, getdescriptorinfo already adds a checksum if it’s missing?
10. 
    sipa commented at 6:18 pm on May 30, 2019: member

    @jb55 It also normalizes the descriptor (which among other things turns WIP into pubkey, and xprv into xpub)
11. 
    jb55 commented at 7:59 am on May 31, 2019: member

    ah ok that was the bit I was missing
12. 
    promag commented at 9:45 pm on June 12, 2019: member

    ACK 15c482ef711a166b74dcd1fa127d213799d2a1d1, only reviewed code change. Looks like returning only the checksum, like @Sjors suggested, is a simple solution to not echo back private key. A small release note could be added.

    Refactor commit best viewed with git show --color-words 7976dd65f6503082370a40ac189b925322c29918.

13. fanquake requested review from achow101 on Jul 30, 2019
14. 
    achow101 commented at 5:22 pm on July 30, 2019: member

    Code Review ACK 15c482ef711a166b74dcd1fa127d213799d2a1d1

    I would prefer if there were also a test where the descriptor were slightly modified (e.g. replacing ' with h in origin info) and that the result of getdescriptorinfo gives the correct canonical form and the modified form with the checksum.

15. achow101 approved
16. 
    Sjors commented at 6:30 pm on August 2, 2019: member

    Tested ACK 15c482e
17. 
    sipa commented at 6:52 pm on August 2, 2019: member

    I like the suggestion of instead just reporting the checksum for the unmodified descriptor, and not echoing anything back. Since this has been reviewed already, I’m happy to go either way. What do reviewers think?
18. 
    achow101 commented at 9:39 pm on August 2, 2019: member

    I like the suggestion of instead just reporting the checksum for the unmodified descriptor, and not echoing anything back. Since this has been reviewed already, I’m happy to go either way. What do reviewers think?

    I prefer this because it is unambiguous as to what the checksum belongs to and what the descriptor with with checksum will look like.

19. Factor out checksum checking from descriptor parsing 104b3a5069
20. Add unmodified-but-with-checksum to getdescriptorinfo 26d3fad109
21. sipa force-pushed on Aug 7, 2019
22. sipa renamed this:
    Add unmodified-descriptor-with-checksum to getdescriptorinfo
    Add checksum to getdescriptorinfo
    on Aug 7, 2019
23. 
    sipa commented at 0:12 am on August 7, 2019: member

    Updated to instead add a "checksum" to getdescriptorinfo that returns the checksum for the (unmodified) descriptor input.
24. in src/rpc/misc.cpp:139 in 26d3fad109

    135@@ -136,6 +136,7 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
    136             RPCResult{
    137             "{\n"
    138             "  \"descriptor\" : \"desc\",         (string) The descriptor in canonical form, without private keys\n"
    139+            "  \"checksum\" : \"chksum\",         (string) The checksum for the input descriptor\n"
    

    ---

    ---

    

    Sjors commented at 1:40 pm on August 7, 2019:

    nit: did you really mean chksum?
25. Sjors approved
26. 
    Sjors commented at 1:52 pm on August 7, 2019: member

    ACK 26d3fad1093dfc697048313be7a96c9adf723654
27. 
    achow101 commented at 2:43 pm on August 7, 2019: member

    Code Review ACK 26d3fad1093dfc697048313be7a96c9adf723654
28. 
    fanquake commented at 9:26 am on August 15, 2019: member

    Two wallet reviewer ACKs here; @meshcollider want to take a final look? I’ve compiled to check that there aren’t any silent merge conflicts etc.
29. 
    meshcollider commented at 9:22 pm on August 16, 2019: contributor

    re-Code Review ACK 26d3fad1093dfc697048313be7a96c9adf723654
30. meshcollider referenced this in commit 7a960ba775 on Aug 16, 2019
31. meshcollider merged this on Aug 16, 2019
32. meshcollider closed this on Aug 16, 2019

    ---

33. sidhujag referenced this in commit 1086701664 on Aug 17, 2019
34. luke-jr referenced this in commit 34a4e70160 on Sep 3, 2019
35. luke-jr referenced this in commit 74d91ab4c3 on Sep 3, 2019
36. deadalnix referenced this in commit 4757f3807c on Jun 18, 2020
37. deadalnix referenced this in commit 5b71c08726 on Jun 18, 2020
38. kittywhiskers referenced this in commit 03a36fd5a7 on Oct 25, 2021
39. kittywhiskers referenced this in commit 862f6c6414 on Oct 25, 2021
40. kittywhiskers referenced this in commit 9ca62f46b2 on Oct 25, 2021
41. kittywhiskers referenced this in commit 6689f7a4ed on Oct 26, 2021
42. kittywhiskers referenced this in commit 4a2764f8f1 on Oct 26, 2021
43. kittywhiskers referenced this in commit 324010f3a1 on Oct 26, 2021
44. kittywhiskers referenced this in commit 981df02bac on Oct 28, 2021
45. kittywhiskers referenced this in commit 60e2e63e85 on Oct 28, 2021
46. kittywhiskers referenced this in commit 300db0c0a8 on Oct 28, 2021
47. kittywhiskers referenced this in commit 760a6a0db0 on Oct 28, 2021
48. kittywhiskers referenced this in commit 8647ba1316 on Oct 29, 2021
49. pravblockc referenced this in commit c462ca0410 on Nov 18, 2021
50. DrahtBot locked this on Dec 16, 2021

Contributors
sipa gwillen meshcollider Sjors DrahtBot jb55 promag achow101 fanquake

Labels
Tests RPC/REST/ZMQ