Add expansion cache functions to descriptors (unused for now)

sipa commented at 2:25 am on November 3, 2018: member

This patch modifies the internal Descriptor class to optionally construct and use an “expansion cache”. Such a cache is a byte array that encodes all information necessary to expand a Descriptor a second time without access to private keys, and without the need to perform expensive BIP32 derivations. For all currently defined descriptors, the cache simply contains a concatenation of all public keys used.

This is motivated by the goal of importing a descriptor into the wallet and using it as a replacement for the keypool, where it would be impossible to expand descriptors if they use hardened derivation.

sipa force-pushed on Nov 3, 2018

meshcollider commented at 2:37 am on November 3, 2018: contributor

Concept ACK

meshcollider added the label Wallet on Nov 3, 2018

sipa force-pushed on Nov 3, 2018

in src/script/descriptor.cpp:301 in 0bfff69c33 outdated

275             entries.emplace_back();
276-            if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second)) return false;
277+            if (!p->GetPubKey(pos, arg, cache_read ? nullptr : &entries.back().first, entries.back().second)) return false;
278+            if (cache_read) {
279+                // Cached expanded public key exists, use it.
280+                if (cache_read->size() == 0) return false;

Sjors commented at 8:39 am on November 3, 2018:

Add a test for this?

sipa commented at 11:52 pm on November 3, 2018:

Done.

in src/script/descriptor.cpp:305 in 0bfff69c33 outdated

279+                // Cached expanded public key exists, use it.
280+                if (cache_read->size() == 0) return false;
281+                bool compressed = ((*cache_read)[0] == 0x02 || (*cache_read)[0] == 0x03) && cache_read->size() >= 33;
282+                bool uncompressed = ((*cache_read)[0] == 0x04) && cache_read->size() >= 65;
283+                if (!(compressed || uncompressed)) return false;
284+                CPubKey pubkey(cache_read->begin(), cache_read->begin() + (compressed ? 33 : 65));

Sjors commented at 8:39 am on November 3, 2018:

n00b C++ question: what happens if cache_read is too short?

sipa commented at 9:30 am on November 3, 2018:

That would be undefined behavior.

Thankfully the line above checks that this isn’t the case :)

in src/test/descriptor_tests.cpp:97 in 0bfff69c33 outdated

87@@ -88,9 +88,18 @@ void Check(const std::string& prv, const std::string& pub, int flags, const std:
88         const auto& ref = scripts[(flags & RANGE) ? i : 0];
89         for (int t = 0; t < 2; ++t) {

Sjors commented at 8:51 am on November 3, 2018:

Can you add comments (to the existing code) what these two loops are about? E.g.

0// Is ranged descriptor, expand up to 3 results
1size_t max ...
2for (size_t i = 0; i < max; ++i)
3...
4// ???
5for (int t = 0; t < 2; ++t)

sipa commented at 11:52 pm on November 3, 2018:

Added some comments to the test.

Sjors commented at 9:07 am on November 6, 2018:

I think the comments were lost in a recent refactor :-(

sipa commented at 7:02 pm on November 7, 2018:

I added a new commit with more comments.

in src/script/descriptor.cpp:347 in 0bfff69c33 outdated

319+    {
320+        return ExpandHelper(pos, provider, nullptr, output_scripts, out, cache);
321+    }
322+
323+    bool ExpandFromCache(int pos, const std::vector<unsigned char>& cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override final
324+    {

Sjors commented at 8:56 am on November 3, 2018:

Since ExpandHelper doesn’t care, should ExpandFromCache explicitly check that it wasn’t unintentionally called with cache=nullptr? Or should ExpandFromCache be agnostic to whether or not there’s actually a (useful) cache?

sipa commented at 9:30 am on November 3, 2018:

The argument is a reference, not a pointer. Dereferencing a nullptr is UB.

Sjors commented at 10:54 am on November 3, 2018:

Ah yes, cache is a pointer for Expand and a reference for ExpandFromCache.

Sjors commented at 9:04 am on November 3, 2018: member

Concept ACK.

IIUC each time you ask the descriptor to expand a specific index, you can pass it an individual std::vector<unsigned char> cache entry. I assume those eventually get serialised into the wallet payload? Where do you envision keeping track of all these cache entries will happen?

sipa commented at 9:35 am on November 3, 2018: member

@Sjors My idea is that the wallet will contain a number of records, each of which has a descriptor, a bool to indicate whether it’s for change or not, a birthday, perhaps information about what HW device to prompt for, a gap limit, …; those are all static configuration that generally doesn’t change unless you import something. In addition there will be a wallet entry per expanded element of a record with the cached information, which also helps in identifying what the next unused address is etc.

DrahtBot commented at 3:12 pm on November 3, 2018: 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:

#14826 (Avoid expanding descriptor scriptPubKeys by promag)
#14505 (Make single parameter constructors explicit (C++11). Add explicit constructor linter. by practicalswift)

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.

sipa force-pushed on Nov 3, 2018

in src/script/descriptor.cpp:216 in 582685e75b outdated

231-class RawDescriptor final : public Descriptor
232-{
233-    CScript m_script;
234+protected:
235+    virtual std::string ToStringExtra() const { return ""; }
236+    virtual std::vector<CScript> MakeScripts(const std::vector<CPubKey>& pubkeys, FlatSigningProvider&) const { assert(false); return {}; }; //!< Invoked when no m_script is set.

practicalswift commented at 9:25 pm on November 4, 2018:

Redundant trailing ; :-)

sipa commented at 2:43 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:217 in 582685e75b outdated

232-{
233-    CScript m_script;
234+protected:
235+    virtual std::string ToStringExtra() const { return ""; }
236+    virtual std::vector<CScript> MakeScripts(const std::vector<CPubKey>& pubkeys, FlatSigningProvider&) const { assert(false); return {}; }; //!< Invoked when no m_script is set.
237+    virtual std::vector<CScript> MakeScripts(const CScript& script, const std::vector<CPubKey>& pubkeys) const { assert(false); return {}; }; //!< Invoked for each CScript produced by m_script.

practicalswift commented at 9:25 pm on November 4, 2018:

Same here :-)

sipa commented at 2:43 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:236 in 582685e75b outdated

313     {
314-        std::string ret = strprintf("multi(%i", m_threshold);
315-        for (const auto& p : m_providers) {
316-            ret += "," + p->ToString();
317+        std::string extra = ToStringExtra();
318+        size_t pos = extra.size() > 0;

practicalswift commented at 9:27 pm on November 4, 2018:

Nit: Better doing it explicitly using ternary operator?

sipa commented at 2:21 am on November 5, 2018:

I don’t know what you mean.

practicalswift commented at 8:10 am on November 5, 2018:

I mean:

0size_t pos = extra.size() > 0 ? 1 : 0;

Sjors commented at 8:36 am on November 5, 2018:

Currently if extra.size() is 0 pos is 0 because 0 > 0 is false and false is 0? I also like dumbing it down a bit with the ternary operator :-)

sipa commented at 7:56 pm on November 5, 2018:

Done.

in src/script/descriptor.cpp:269 in 582685e75b outdated

323+            if (priv) {
324+                if (!pubkey->ToPrivateString(*arg, tmp)) return false;
325+            } else {
326+                tmp = pubkey->ToString();
327+            }
328+            ret += std::move(tmp);

practicalswift commented at 9:32 pm on November 4, 2018:

This move is redundant?

sipa commented at 2:22 am on November 5, 2018:

How so?

sipa commented at 2:43 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:275 in 582685e75b outdated

339-            ret += "," + std::move(sub);
340+        if (m_script) {
341+            if (pos++) ret += ",";
342+            std::string tmp;
343+            if (!m_script->ToStringHelper(arg, tmp, priv)) return false;
344+            ret += std::move(tmp);

practicalswift commented at 9:32 pm on November 4, 2018:

Same here? The result is passed as a const reference argument and the move is hence a noop?

sipa commented at 2:22 am on November 5, 2018:

I don’t understand.

EDIT: Ah, I see; there is no operator+= for std::string that takes advantage of the rvalue, so this indeed has no function. That’s strange, as it seems that there is an operator+ that does.

sipa commented at 2:43 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:258 in 582685e75b outdated

346         out = std::move(ret) + ")";
347         return true;
348     }
349 
350-    bool Expand(int pos, const SigningProvider& arg, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override
351+    std::string ToString() const override final

practicalswift commented at 9:33 pm on November 4, 2018:

Override redundant here?

sipa commented at 2:26 am on November 5, 2018:

override is always redundant. Its function is making sure the code does what is intended, by preventing an accidental rename/change of the base class’s definition from unintentionally changing the override into a new non-virtual method.

practicalswift commented at 8:27 am on November 5, 2018:

Yes, obviously :-) But I was thinking “redundancy” in the sense that the function also was declared as final.

Rationale: Virtual functions should specify exactly one of virtual, override, or final

Sjors commented at 9:23 am on November 5, 2018:

Fun fact: this is only place in the entire codebase where a function is declared final (search \).* final[$\ ]). I like the suggestion in this guideline, though no strong opinion.

sipa commented at 7:29 pm on November 5, 2018:

@practicalswift Oh, good point - I never realized that override would be redundant with final. Fixing.

in src/script/descriptor.cpp:288 in 582685e75b outdated

353+        std::string ret;
354+        ToStringHelper(nullptr, ret, false);
355+        return ret;
356+    }
357+
358+    bool ToPrivateString(const SigningProvider& arg, std::string& out) const override final { return ToStringHelper(&arg, out, true); }

practicalswift commented at 9:33 pm on November 4, 2018:

Same here?

in src/script/descriptor.cpp:318 in 582685e75b outdated

411+            output_scripts = MakeScripts(pubkeys, out);
412+        }
413         return true;
414     }
415+
416+    bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, std::vector<unsigned char>* cache = nullptr) const override final

practicalswift commented at 9:34 pm on November 4, 2018:

Same here?

in src/script/descriptor.cpp:323 in 582685e75b outdated

416+    bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, std::vector<unsigned char>* cache = nullptr) const override final
417+    {
418+        return ExpandHelper(pos, provider, nullptr, output_scripts, out, cache);
419+    }
420+
421+    bool ExpandFromCache(int pos, const std::vector<unsigned char>& cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const override final

practicalswift commented at 9:34 pm on November 4, 2018:

And here?

Sjors commented at 9:28 am on November 5, 2018:

One comment is probably enough for something that can be changed with find & replace :-)

in src/script/descriptor.cpp:392 in 582685e75b outdated

489+class PKDescriptor final : public DescriptorImpl
490+{
491+protected:
492+    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, FlatSigningProvider&) const override { return Singleton(GetScriptForRawPubKey(keys[0])); }
493+public:
494+    PKDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Singleton(std::move(prov)), {}, "pk") {}

practicalswift commented at 9:35 pm on November 4, 2018:

Should be explicit?

sipa commented at 2:43 am on November 5, 2018:

Fixed.

practicalswift commented at 8:08 am on November 9, 2018:

Still missing explicit? :-)

in src/script/descriptor.cpp:401 in 582685e75b outdated

501 {
502-    std::unique_ptr<PubkeyProvider> m_provider;
503+protected:
504+    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, FlatSigningProvider&) const override { return Singleton(GetScriptForDestination(keys[0].GetID())); }
505+public:
506+    PKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Singleton(std::move(prov)), {}, "pkh") {}

practicalswift commented at 9:35 pm on November 4, 2018:

Explicit?

sipa commented at 2:43 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:410 in 582685e75b outdated

511+{
512+protected:
513+    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, FlatSigningProvider&) const override { return Singleton(GetScriptForDestination(WitnessV0KeyHash(keys[0].GetID()))); }
514 public:
515-    ComboDescriptor(std::unique_ptr<PubkeyProvider> provider) : m_provider(std::move(provider)) {}
516+    WPKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Singleton(std::move(prov)), {}, "wpkh") {}

practicalswift commented at 9:35 pm on November 4, 2018:

Explicit?

sipa commented at 2:44 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:452 in 582685e75b outdated

582+class SHDescriptor final : public DescriptorImpl
583+{
584+protected:
585+    std::vector<CScript> MakeScripts(const CScript& script, const std::vector<CPubKey>&) const override { return Singleton(GetScriptForDestination(CScriptID(script))); }
586+public:
587+    SHDescriptor(std::unique_ptr<DescriptorImpl> desc) : DescriptorImpl({}, std::move(desc), "sh") {}

practicalswift commented at 9:35 pm on November 4, 2018:

Explicit?

sipa commented at 2:44 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:461 in 582685e75b outdated

591+class WSHDescriptor final : public DescriptorImpl
592+{
593+protected:
594+    std::vector<CScript> MakeScripts(const CScript& script, const std::vector<CPubKey>&) const override { return Singleton(GetScriptForDestination(WitnessV0ScriptHash(script))); }
595+public:
596+    WSHDescriptor(std::unique_ptr<DescriptorImpl> desc) : DescriptorImpl({}, std::move(desc), "wsh") {}

practicalswift commented at 9:35 pm on November 4, 2018:

Explicit?

sipa commented at 2:44 am on November 5, 2018:

Fixed.

in src/script/descriptor.cpp:272 in 582685e75b outdated

336-        for (const auto& p : m_providers) {
337-            std::string sub;
338-            if (!p->ToPrivateString(arg, sub)) return false;
339-            ret += "," + std::move(sub);
340+        if (m_script) {
341+            if (pos++) ret += ",";

practicalswift commented at 9:37 pm on November 4, 2018:

pos++ is a dead store?

sipa commented at 2:44 am on November 5, 2018:

Fixed.

sipa force-pushed on Nov 5, 2018

in src/script/sign.h:28 in 94677cc393 outdated

23@@ -24,6 +24,11 @@ struct KeyOriginInfo
24 {
25     unsigned char fingerprint[4];
26     std::vector<uint32_t> path;
27+
28+    friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b)

Sjors commented at 8:57 am on November 5, 2018:

It’s already used by the test BOOST_CHECK(spks == spks_cached), but probably deserves its own test.

sipa commented at 7:30 pm on November 5, 2018:

I think this function is too trivial. It’s just a dumb datastructure and a comparison operator. What would the test do? Replicate the implementation and verify it does the same? Such tests are not useful.

Sjors approved

Sjors commented at 9:30 am on November 5, 2018: member

utACK 94677cc

in src/script/descriptor.cpp:219 in c957f5bd34 outdated

228-/** A parsed raw(H) descriptor. */
229-class RawDescriptor final : public Descriptor
230-{
231-    CScript m_script;
232+protected:
233+    virtual std::string ToStringExtra() const { return ""; }

ryanofsky commented at 5:48 pm on November 5, 2018:

Can you add description of this method: Return string containing initial arguments to descriptor function (prior to any key or script arguments)?

sipa commented at 7:57 pm on November 5, 2018:

Done.

in src/script/descriptor.cpp:208 in c957f5bd34 outdated

206-class AddressDescriptor final : public Descriptor
207+/** Base class for all Descriptor implementations. */
208+class DescriptorImpl : public Descriptor
209 {
210-    CTxDestination m_destination;
211+    const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkeys;

ryanofsky commented at 6:02 pm on November 5, 2018:

Can you add a description of this member: List of descriptor public key arguments. Length > 1 for multisig descriptors, 1 for key or key hash descriptors, 0 for raw or address descriptors?

in src/script/descriptor.cpp:209 in c957f5bd34 outdated

207+/** Base class for all Descriptor implementations. */
208+class DescriptorImpl : public Descriptor
209 {
210-    CTxDestination m_destination;
211+    const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkeys;
212+    const std::unique_ptr<Descriptor> m_script;

ryanofsky commented at 6:06 pm on November 5, 2018:

Can you add a description of this member: Single descriptor script argument. Set for sh/wsh descriptors, unset for other types of descriptors?

ryanofsky commented at 6:11 pm on November 5, 2018:

I would maybe call these members m_pubkey_args and m_script_arg to give names more meaning. This would also avoid RawDescriptor::m_script shadowing DescriptorImpl::m_script, which is kind of confusing.

sipa commented at 7:57 pm on November 5, 2018:

Done.

sipa commented at 7:57 pm on November 5, 2018:

Good point, I didn’t even realize it was shadowing.

in src/script/descriptor.cpp:215 in c957f5bd34 outdated

208+class DescriptorImpl : public Descriptor
209 {
210-    CTxDestination m_destination;
211+    const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkeys;
212+    const std::unique_ptr<Descriptor> m_script;
213+    const std::string m_name;

ryanofsky commented at 6:07 pm on November 5, 2018:

Can you add a description of this member: name of descriptor function?

sipa commented at 7:57 pm on November 5, 2018:

Done.

in src/script/descriptor.cpp:214 in c957f5bd34 outdated

229-class RawDescriptor final : public Descriptor
230-{
231-    CScript m_script;
232+protected:
233+    virtual std::string ToStringExtra() const { return ""; }
234+    virtual std::vector<CScript> MakeScripts(const std::vector<CPubKey>& pubkeys, FlatSigningProvider&) const { assert(false); return {}; } //!< Invoked when no m_script is set.

ryanofsky commented at 6:23 pm on November 5, 2018:

Can you add a more complete description of this method: Expand descriptor into corresponding scriptPubKey(s) using keys indicated by m_pubkey arguments?

ryanofsky commented at 6:38 pm on November 5, 2018:

Maybe call this method Expand. It seem inconsistent to talk about “expanding descriptors” externally and “making scripts” internally when they seem refer to same thing.

sipa commented at 7:58 pm on November 5, 2018:

Rewritten (see next suggestion) and added more comments.

I don’t think it should be called Expand, as it’s only one piece of the expansion process.

in src/script/descriptor.cpp:215 in c957f5bd34 outdated

230-{
231-    CScript m_script;
232+protected:
233+    virtual std::string ToStringExtra() const { return ""; }
234+    virtual std::vector<CScript> MakeScripts(const std::vector<CPubKey>& pubkeys, FlatSigningProvider&) const { assert(false); return {}; } //!< Invoked when no m_script is set.
235+    virtual std::vector<CScript> MakeScripts(const CScript& script, const std::vector<CPubKey>& pubkeys) const { assert(false); return {}; } //!< Invoked for each CScript produced by m_script.

ryanofsky commented at 6:50 pm on November 5, 2018:

Having these two overloaded methods and deciding to call one or other other at runtime, then asserting the right one is called and overloaded seems awkward. I think it’d be better to replace both of these with a single pure virtual method:

0virtual void ExpandImpl(
1    const std::vector<CPubKey>& pubkeys,
2    const CScript* script,
3    std::vector<CScript>& output_scripts,
4    FlatSigningProvider& out) = 0;

This would also make the new method more consistent with the existing Descriptor::Expand.

sipa commented at 7:59 pm on November 5, 2018:

That’s much better indeed, as it allows static checking that all implementations provide it.

I don’t think Expand is a good name.

ryanofsky commented at 7:02 pm on November 5, 2018: member

So for only reviewed the first (largest) commit in detail. The later commits also seem good at first glance. Will continue reviewing.

sipa force-pushed on Nov 5, 2018

in src/script/descriptor.h:59 in ab67eb24bb outdated

51+
52+    /** Expand a descriptor at a specified position using cached expansion data.
53+     *
54+     * pos: the position at which to expand the descriptor. If IsRange() is false, this is ignored.
55+     * cache: vector from which cached expansion data will be read.
56+     * output_script: the expanded scriptPubKeys will be put here.

Sjors commented at 9:06 am on November 6, 2018:

nit: output_script -> output_scripts (was already wrong in the original)

jonasschnelli commented at 2:13 am on November 7, 2018: contributor

Nice. Concept ACK.

ryanofsky approved

ryanofsky commented at 6:31 pm on November 7, 2018: member

utACK ab67eb24bb039a8bb8ff3c7363e6667bf7a5ea6e. Thanks for adding all the comments. I like the caching code, too. It’s pretty clever.

laanwj added this to the "Blockers" column in a project

in src/test/descriptor_tests.cpp:101 in 729b8187dd outdated

 99+        // When t=0, evaluate the `prv` descriptor; when t=1, evaluate the `pub` descriptor.
100         for (int t = 0; t < 2; ++t) {
101+            // When the descriptor is hardened, evaluate with access to the private keys inside.
102             const FlatSigningProvider& key_provider = (flags & HARDENED) ? keys_priv : keys_pub;
103+
104+            // Evaluate the descriptor selected by `t` in poisition `i`.

meshcollider commented at 8:07 am on November 9, 2018:

nit: poisition -> position

in src/script/descriptor.cpp:303 in ab67eb24bb outdated

292+            if (!p->GetPubKey(pos, arg, cache_read ? nullptr : &entries.back().first, entries.back().second)) return false;
293+            if (cache_read) {
294+                // Cached expanded public key exists, use it.
295+                if (cache_read->size() == 0) return false;
296+                bool compressed = ((*cache_read)[0] == 0x02 || (*cache_read)[0] == 0x03) && cache_read->size() >= 33;
297+                bool uncompressed = ((*cache_read)[0] == 0x04) && cache_read->size() >= 65;

meshcollider commented at 8:22 am on November 9, 2018:

Why >= instead of just equality? In what case would it be longer?

sipa commented at 4:11 pm on November 16, 2018:

There can be multiple public keys in one descriptor, and the cache contains all of them concatenated.

meshcollider commented at 10:05 am on November 16, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14646/commits/729b8187dd795e31a87cad9ce0ba3638cf01f502

This is a really really nice refactor of the descriptor code!

in src/script/descriptor.cpp:290 in 729b8187dd outdated

342-
343-public:
344-    MultisigDescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers) : m_threshold(threshold), m_providers(std::move(providers)) {}
345-
346-    bool IsRange() const override
347+    bool ExpandHelper(int pos, const SigningProvider& arg, Span<const unsigned char>* cache_read, std::vector<CScript>& output_scripts, FlatSigningProvider& out, std::vector<unsigned char>* cache_write) const

Empact commented at 4:51 pm on November 22, 2018:

Doesn’t seem like there is a case where both cache_read and cache_write should be supplied, right? If you make that explicit / an error condition we can rely on it elsewhere.

in src/script/descriptor.cpp:99 in 729b8187dd outdated

 93@@ -94,9 +94,9 @@ class ConstPubkeyProvider final : public PubkeyProvider
 94 
 95 public:
 96     ConstPubkeyProvider(const CPubKey& pubkey) : m_pubkey(pubkey) {}
 97-    bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info) const override
 98+    bool GetPubKey(int pos, const SigningProvider& arg, CPubKey* key, KeyOriginInfo& info) const override
 99     {
100-        key = m_pubkey;
101+        if (key) *key = m_pubkey;

Empact commented at 4:57 pm on November 22, 2018:

How about instead extracting GetPubKeyInfo and changing up the caller in ExpandHelper to only request the desired information? This looks doable if cache_read and cache_write are mutually exclusive.

ryanofsky commented at 8:31 pm on November 27, 2018:

re: #14646 (review)

How about instead extracting GetPubKeyInfo and changing up the caller in ExpandHelper to only request the desired information? This looks doable if cache_read and cache_write are mutually exclusive.

It does seem like it would be a minor simplification to have separate methods for retrieving CPubKey and KeyOriginInfo, since KeyOriginInfo isn’t needed by ExpandHelper. But that would make this PR bigger, and I also don’t see what it has to do with cache read/write becoming exclusive options.

MarcoFalke referenced this in commit 0fa3703c17 on Nov 27, 2018

DrahtBot added the label Needs rebase on Nov 27, 2018

ryanofsky approved

ryanofsky commented at 8:40 pm on November 27, 2018: member

utACK 729b8187dd795e31a87cad9ce0ba3638cf01f502. Only change since last review is adding a new commit which tweaks and adds comments to the descriptor test.

[refactor] Add a base DescriptorImpl with most common logic 6be0fb4b3f

[refactor] Use DescriptorImpl internally, permitting access to new methods 24d3a7b3a9

[refactor] Combine the ToString and ToPrivateString implementations 1eda33aabc

Add descriptor expansion cache 82df4c64ff

Add comments to descriptor tests 26879509f1

sipa force-pushed on Nov 28, 2018

sipa commented at 11:26 pm on November 28, 2018: member

Rebased.

DrahtBot removed the label Needs rebase on Nov 28, 2018

ryanofsky approved

ryanofsky commented at 9:51 pm on December 4, 2018: member

utACK 26879509f1a3b9fc0fa616164e5a88fdb344ac4d. Somewhat messy rebase after #14477, but no changes since last review.

in src/script/descriptor.cpp:208 in 6be0fb4b3f outdated

225-class RawDescriptor final : public Descriptor
226+/** Base class for all Descriptor implementations. */
227+class DescriptorImpl : public Descriptor
228 {
229-    CScript m_script;
230+    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size of Multisig).

meshcollider commented at 4:30 am on December 10, 2018:

nit: of -> for

in src/script/descriptor.h:51 in 82df4c64ff outdated

47@@ -48,8 +48,18 @@ struct Descriptor {
48      * provider: the provider to query for private keys in case of hardened derivation.
49      * output_script: the expanded scriptPubKeys will be put here.
50      * out: scripts and public keys necessary for solving the expanded scriptPubKeys will be put here (may be equal to provider).
51+     * cache: vector which will be overwritten with cache data necessary to-evaluate the descriptor at this point without access to private keys.

meshcollider commented at 12:25 pm on December 10, 2018:

nit: remove -

meshcollider commented at 10:36 am on December 11, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14646/commits/26879509f1a3b9fc0fa616164e5a88fdb344ac4d

Just two minor comment nits inline, sorry to pick :P

in src/script/descriptor.cpp:298 in 26879509f1

392+
393+        // Construct temporary data in `entries` and `subscripts`, to avoid producing output in case of failure.
394+        for (const auto& p : m_pubkey_args) {
395             entries.emplace_back();
396-            if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second)) return false;
397+            if (!p->GetPubKey(pos, arg, cache_read ? nullptr : &entries.back().first, entries.back().second)) return false;

Sjors commented at 3:39 pm on December 11, 2018:

Nit: explain that if we already have a cache, we don’t want GetPubKey to return public keys, since we’re getting them from the cache ourselves. This is counter intuitive given the name of the function, but the point is to only make GetPubKey do work if our cache is empty.

Alternatively, maybe the stuff below under if (cache_read) belongs in GetPubKey?

Maybe add an assert to GetPubKey if KeyOriginInfo& info is non empty and doesn’t match m_extkey.pubkey.GetID()?

in src/script/descriptor.cpp:213 in 26879509f1

251-    const std::string m_fn_name;
252-    std::unique_ptr<PubkeyProvider> m_provider;
253+    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size of Multisig).
254+    const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
255+    //! The sub-descriptor argument (nullptr for everything but SH and WSH).
256+    const std::unique_ptr<DescriptorImpl> m_script_arg;

Sjors commented at 4:10 pm on December 11, 2018:

Can we rename this to m_sub_descriptor_arg? (I found that a replace-all reduced my headache)

Sjors approved

Sjors commented at 4:21 pm on December 11, 2018: member

utACK 2687950

I found a git (2.20+) incantation that shows the difference between now and the last time I reviewed, but that difference is so large it’s not super useful here:

0 git range-diff `git merge-base --all HEAD 94677cc`...94677cc HEAD~5...HEAD

Where 94677cc is the last commit I reviewed and 5 is the number of commits in this PR.

Anyway, reviewed again manually :-)

meshcollider commented at 3:31 am on December 12, 2018: contributor

Because this is obviously intended to be followed-up by more work, I think we can leave the last couple nits to be addressed later

meshcollider merged this on Dec 12, 2018

meshcollider closed this on Dec 12, 2018

meshcollider removed this from the "Blockers" column in a project

laanwj referenced this in commit e7df1ecd17 on Aug 14, 2019

sidhujag referenced this in commit bc60719c5b on Aug 17, 2019

deadalnix referenced this in commit 2f8295ebf4 on May 21, 2020

deadalnix referenced this in commit 752d0621de on May 21, 2020

deadalnix referenced this in commit 0378eb56a0 on May 21, 2020

deadalnix referenced this in commit 64a380c868 on May 21, 2020

deadalnix referenced this in commit 5a76fe12d2 on May 21, 2020

ShengguangXiao referenced this in commit fdb5a5803c on Aug 28, 2020

jasonbcox referenced this in commit a893aa1d9b on Oct 7, 2020

linuxsh2 referenced this in commit d32d0e78fe on Jul 29, 2021

linuxsh2 referenced this in commit 670d090a52 on Jul 30, 2021

linuxsh2 referenced this in commit 7603e1ce8c on Aug 3, 2021

MarcoFalke locked this on Sep 8, 2021

Add expansion cache functions to descriptors (unused for now) #14646

Conflicts