Add expansion cache functions to descriptors (unused for now) #14646
Conversation
022c93d to
3a853cb
Compare
|
Concept ACK |
f9d9483 to
0bfff69
Compare
There was a problem hiding this 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?
|
@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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
0bfff69 to
582685e
Compare
582685e to
94677cc
Compare
94677cc to
ab67eb2
Compare
| { | ||
| key = m_pubkey; | ||
| if (key) *key = m_pubkey; |
There was a problem hiding this comment.
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.
729b818 to
2687950
Compare
|
Rebased. |
There was a problem hiding this 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). |
| @@ -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. | |||
There was a problem hiding this 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...HEADWhere 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Can we rename this to m_sub_descriptor_arg? (I found that a replace-all reduced my headache)
|
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 |
2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost) 2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost) Pull request description: I found the name `m_script_arg` to be confusing while reviewing #14646 (comment). @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. ACKs for top commit: laanwj: ACK 2e68ffa Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost) 2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost) Pull request description: I found the name `m_script_arg` to be confusing while reviewing bitcoin#14646 (comment). @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. ACKs for top commit: laanwj: ACK 2e68ffa Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
Summary: This is a partial backport of Core [[bitcoin/bitcoin#14646 | PR14646]] : bitcoin/bitcoin@6be0fb4 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6194
…thods Summary: This is a partial backport of Core [[bitcoin/bitcoin#14646 | PR14646]] : bitcoin/bitcoin@24d3a7b Depends on D6194 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6195
Summary: This is a partial backport of Core [[bitcoin/bitcoin#14646 | PR14646]] : bitcoin/bitcoin@1eda33a Depends on D6195 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6196
Summary: This is a partial backport of Core [[bitcoin/bitcoin#14646 | PR14646]] : bitcoin/bitcoin@82df4c6 Depends on D6196 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6197
Summary: This is a partial backport of Core [[bitcoin/bitcoin#14646 | PR14646]] : bitcoin/bitcoin@2687950 Depends on D6197 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6198
369e5f5 [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost) 44ca2f4 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost) Pull request description: I found the name `m_script_arg` to be confusing while reviewing bitcoin/bitcoin#14646 (comment). @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. ACKs for top commit: laanwj: ACK 369e5f5 Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
Summary: 2e68ffaf205866e4cea71f64e79bbfb89e17280a [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost) 2290269759ad10cc2e35958c7b0a63f3a7608621 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost) Pull request description: I found the name `m_script_arg` to be confusing while reviewing bitcoin/bitcoin#14646 (comment). @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. ACKs for top commit: laanwj: ACK 2e68ffaf205866e4cea71f64e79bbfb89e17280a --- Backport of Core [[bitcoin/bitcoin#14934 | PR14934]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7806
… 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
… 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
… 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
This patch modifies the internal
Descriptorclass to optionally construct and use an "expansion cache". Such a cache is a byte array that encodes all information necessary to expand aDescriptora 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.