Descriptor expansion cache clarifications #14934

1. 
   Sjors commented at 9:09 am on December 12, 2018: member

   I found the name m_script_arg to be confusing while reviewing #14646 (review). @sipa let me know if m_subdescriptor_arg is completely wrong.

   I also added an explanation of why we call GetPubKey when we don’t ask it for a public key.

2. fanquake added the label Wallet on Dec 12, 2018
3. Sjors force-pushed on Dec 12, 2018
4. 
   fanquake commented at 9:12 am on December 12, 2018: member

   Might as well fixup the other (trivial) nits.
5. Sjors force-pushed on Dec 12, 2018
6. 
   Sjors commented at 9:16 am on December 12, 2018: member

   @fanquake done, I snuck those into the second commit.
7. 
   DrahtBot commented at 10:52 am on December 12, 2018: member

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

   Conflicts

   No conflicts as of last run.

8. in src/script/descriptor.cpp:299 in acb6f9d127 outdated

   294@@ -295,6 +295,8 @@ class DescriptorImpl : public Descriptor
   295         // Construct temporary data in `entries` and `subscripts`, to avoid producing output in case of failure.
   296         for (const auto& p : m_pubkey_args) {
   297             entries.emplace_back();
   298+            // If we have a cache, GetPubKey should not actually return a public key,
   299+            // so we pass a nullprt. We're only interested in uncached origin info.
   

   ---

   ---

   

   flack commented at 11:01 am on December 12, 2018:

   Typo: nullprt

   ---

   

   Sjors commented at 11:22 am on December 12, 2018:

   Fixed
9. Sjors force-pushed on Dec 12, 2018
10. fanquake requested review from sipa on Dec 13, 2018
11. 
    laanwj commented at 12:25 pm on January 22, 2019: member

    Ping @sipa, as this is your code it’d help if you give your opinion on this.
12. 
    meshcollider commented at 7:44 pm on January 26, 2019: contributor

    m_script_arg refers to the defined SCRIPT in the descriptor doc, to make it distinct from KEY or ADDR. But subdescriptor is also a clear name so I have no strong preference either way.
13. in src/script/descriptor.cpp:298 in 5cb58b8f3d outdated

    294@@ -295,6 +295,8 @@ class DescriptorImpl : public Descriptor
    295         // Construct temporary data in `entries` and `subscripts`, to avoid producing output in case of failure.
    296         for (const auto& p : m_pubkey_args) {
    297             entries.emplace_back();
    298+            // If we have a cache, GetPubKey should not actually return a public key,
    

    ---

    ---

    

    sipa commented at 0:12 am on January 27, 2019:

    “should not” sounds like it should be prohibited from computing the pubkey.

    What about “If we have a cache, we don’t need GetPubKey to compute the public key. Pass in nullptr to signify only origin info is desired.”.

    ---

    

    Sjors commented at 3:15 pm on January 29, 2019:

    Done
14. scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg

    -BEGIN VERIFY SCRIPT-
    sed -i -e 's/m_script_arg/m_subdescriptor_arg/g' src/script/descriptor.cpp
    -END VERIFY SCRIPT-

    2290269759
15. 
    Sjors commented at 3:32 pm on January 29, 2019: member

    Rebased. @MeshCollider I added a comment referencing SCRIPT in the descriptor docs.

    By the way, I noticed that neither DescriptorImpl nor m_subdescriptor_arg show up in the docs, not even in the docs for script/descriptor.cpp. I don’t understand Doxygen well enough to know if that’s a feature (i.e. because these functions are private).

16. Sjors force-pushed on Jan 29, 2019
17. [doc] descriptor: explain GetPubKey() usage with cached public key

    Plus a few typo fixes.

    2e68ffaf20
18. Sjors force-pushed on Jan 29, 2019
19. 
    Sjors commented at 5:32 pm on February 21, 2019: member

    Miraculously this doesn’t need another rebase…
20. 
    practicalswift commented at 4:36 pm on March 15, 2019: contributor

    FWIW: I’ve verified that a disassembly of the bitcoind binary built with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).
21. 
    laanwj commented at 11:30 am on August 14, 2019: member

    ACK 2e68ffaf205866e4cea71f64e79bbfb89e17280a
22. laanwj merged this on Aug 14, 2019
23. laanwj closed this on Aug 14, 2019

    ---

24. laanwj referenced this in commit e7df1ecd17 on Aug 14, 2019
25. Sjors deleted the branch on Aug 14, 2019
26. fanquake referenced this in commit 396385657c on Aug 14, 2019
27. MarcoFalke referenced this in commit 034575e9c7 on Aug 14, 2019
28. ken2812221 referenced this in commit 474ab7ef61 on Aug 16, 2019
29. sidhujag referenced this in commit bc60719c5b on Aug 17, 2019
30. sidhujag referenced this in commit 3a3b482558 on Aug 17, 2019
31. jasonbcox referenced this in commit a893aa1d9b on Oct 7, 2020
32. Munkybooty referenced this in commit 512fbb5296 on Nov 25, 2021
33. Munkybooty referenced this in commit 0c12767ee7 on Nov 30, 2021
34. DrahtBot locked this on Dec 16, 2021

Contributors
Sjors fanquake DrahtBot flack laanwj meshcollider sipa practicalswift

Labels
Wallet