Descriptor expansion cache clarifications #14934

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2018/12/rename_m_script_arg changing 2 files +21 −17

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.
fanquake added the label Wallet on Dec 12, 2018
Sjors force-pushed on Dec 12, 2018
fanquake commented at 9:12 AM on December 12, 2018: member

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

@fanquake done, I snuck those into the second commit.
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.

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

Sjors force-pushed on Dec 12, 2018
fanquake requested review from sipa on Dec 13, 2018
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.
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.

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 12: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

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

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).
Sjors force-pushed on Jan 29, 2019
[doc] descriptor: explain GetPubKey() usage with cached public key
Plus a few typo fixes.
2e68ffaf20
Sjors force-pushed on Jan 29, 2019
Sjors commented at 5:32 PM on February 21, 2019: member

Miraculously this doesn't need another rebase...
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).
laanwj commented at 11:30 AM on August 14, 2019: member

ACK 2e68ffaf205866e4cea71f64e79bbfb89e17280a
laanwj merged this on Aug 14, 2019
laanwj closed this on Aug 14, 2019
laanwj referenced this in commit e7df1ecd17 on Aug 14, 2019
Sjors deleted the branch on Aug 14, 2019
fanquake referenced this in commit 396385657c on Aug 14, 2019
MarcoFalke referenced this in commit 034575e9c7 on Aug 14, 2019
ken2812221 referenced this in commit 474ab7ef61 on Aug 16, 2019
sidhujag referenced this in commit bc60719c5b on Aug 17, 2019
sidhujag referenced this in commit 3a3b482558 on Aug 17, 2019
jasonbcox referenced this in commit a893aa1d9b on Oct 7, 2020
Munkybooty referenced this in commit 512fbb5296 on Nov 25, 2021
Munkybooty referenced this in commit 0c12767ee7 on Nov 30, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#14646 Add expansion cache functions to descriptors (unused for now)#16609 descriptor: fix missed m_script_arg arg renaming in #14934

Descriptor expansion cache clarifications #14934

Conflicts