Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from Dec 12, 2018

Conversation

Projects
None yet
8 participants
@sipa
Copy link
Member

commented Nov 3, 2018

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 sipa force-pushed the sipa:201811_descriptcache branch Nov 3, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

Concept ACK

@meshcollider meshcollider added the Wallet label Nov 3, 2018

@sipa sipa force-pushed the sipa:201811_descriptcache branch 4 times, most recently Nov 3, 2018

@Sjors
Copy link
Member

left a comment

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?

Show resolved Hide resolved src/script/descriptor.cpp
Show resolved Hide resolved src/script/descriptor.cpp
Show resolved Hide resolved src/test/descriptor_tests.cpp
Show resolved Hide resolved src/script/descriptor.cpp
@sipa

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2018

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 sipa force-pushed the sipa:201811_descriptcache branch Nov 3, 2018

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

@sipa sipa force-pushed the sipa:201811_descriptcache branch Nov 5, 2018

@Sjors

Sjors approved these changes Nov 5, 2018

Copy link
Member

left a comment

utACK 94677cc

Show resolved Hide resolved src/script/sign.h Outdated
Show resolved Hide resolved src/script/descriptor.cpp Outdated
Show resolved Hide resolved src/script/descriptor.cpp Outdated
@ryanofsky
Copy link
Contributor

left a comment

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

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

@sipa sipa force-pushed the sipa:201811_descriptcache branch Nov 5, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Nice. Concept ACK.

*
* pos: the position at which to expand the descriptor. If IsRange() is false, this is ignored.
* cache: vector from which cached expansion data will be read.
* output_script: the expanded scriptPubKeys will be put here.

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 7, 2018

Member

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

Show resolved Hide resolved src/test/descriptor_tests.cpp
@ryanofsky
Copy link
Contributor

left a comment

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

@meshcollider
Copy link
Member

left a comment

utACK 729b818

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

const FlatSigningProvider& key_provider = (flags & HARDENED) ? keys_priv : keys_pub;

// Evaluate the descriptor selected by `t` in poisition `i`.

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 16, 2018

Member

nit: poisition -> position

Show resolved Hide resolved src/script/descriptor.cpp
MultisigDescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers) : m_threshold(threshold), m_providers(std::move(providers)) {}

bool IsRange() const override
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

This comment has been minimized.

Copy link
@Empact

Empact Nov 22, 2018

Member

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.

{
key = m_pubkey;
if (key) *key = m_pubkey;

This comment has been minimized.

Copy link
@Empact

Empact Nov 22, 2018

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 27, 2018

Contributor

re: #14646 (comment)

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 added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 27, 2018

Merge bitcoin#14820: test: Fix descriptor_tests not checking ToString…
… output of public descriptors

c77f092 Fix descriptor_tests not checking ToString output of public descriptors (Russell Yanofsky)

Pull request description:

  This fixes a minor test bug introduced in bitcoin#13697 that I noticed while reviewing bitcoin#14646

Tree-SHA512: efed91200cdff5f86ba5de3461ac00759d285e2905f6cb24cea15d3e23e0581ce5fc14b24a40db093f7ebd662ee1ee2cf67f8798bac1903a78298eda08909cfb
@ryanofsky
Copy link
Contributor

left a comment

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

{
key = m_pubkey;
if (key) *key = m_pubkey;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 27, 2018

Contributor

re: #14646 (comment)

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.

@sipa sipa force-pushed the sipa:201811_descriptcache branch to 2687950 Nov 28, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Nov 28, 2018

@ryanofsky
Copy link
Contributor

left a comment

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

@meshcollider
Copy link
Member

left a comment

utACK 2687950

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

{
CScript m_script;
//! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size of Multisig).

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 11, 2018

Member

nit: of -> for

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

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 11, 2018

Member

nit: remove -

@Sjors

Sjors approved these changes Dec 11, 2018

Copy link
Member

left a comment

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:

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

entries.emplace_back();
if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second)) return false;
if (!p->GetPubKey(pos, arg, cache_read ? nullptr : &entries.back().first, entries.back().second)) return false;

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 11, 2018

Member

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()?

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

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 11, 2018

Member

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

@meshcollider

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

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 added a commit to meshcollider/bitcoin that referenced this pull request Dec 12, 2018

Merge bitcoin#14646: Add expansion cache functions to descriptors (un…
…used for now)

2687950 Add comments to descriptor tests (Pieter Wuille)
82df4c6 Add descriptor expansion cache (Pieter Wuille)
1eda33a [refactor] Combine the ToString and ToPrivateString implementations (Pieter Wuille)
24d3a7b [refactor] Use DescriptorImpl internally, permitting access to new methods (Pieter Wuille)
6be0fb4 [refactor] Add a base DescriptorImpl with most common logic (Pieter Wuille)

Pull request description:

  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.

Tree-SHA512: f531a0a82ec1eecc30b78ba8a31724d1249826b028cc3543ad32372e1aedd537f137ab03dbffc222c5df444d5865ecd5cec754c1ae1d4989b6e9baeaffade32a

@meshcollider meshcollider merged commit 2687950 into bitcoin:master Dec 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meshcollider meshcollider removed this from Blockers in High-priority for review Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.