descriptor wallet: Cache last hardened xpub and use in normalized descriptors #21329

achow101 · 2021-03-01T22:53:01Z

Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).

However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of /84'/0'/0'/0/*, the parent xpub that is cached is m/84'/0'/0'/0, and the child keys of m/84'/0'/0'/0/i (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at m/84'/0'/0'. So this PR adds another field to DescriptorCache to cache the last hardened xpub so that we can use them for normalized descriptors.

Since DescriptorCache is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that UpgradeKeyMetadata operates. It will use a new wallet flag WALLET_FLAG_LAST_HARDENED_XPUB_CACHED to indicate whether the descriptor wallet has the last hardened xpub cache.

Lastly listdescriptors will not require the wallet to be locked and getaddressinfo's parent_desc will always be output (assuming the upgrade has occurred).

src/script/descriptor.cpp

DrahtBot · 2021-03-02T10:11:42Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet, rpc: add an option to list private descriptors #21500 by S3RK
wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time #20096 by achow101

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.

S3RK · 2021-03-09T07:55:22Z

Started reviewing this

S3RK

Wow, it took more than expected to grasp what's going on. I think I still need more time, but I have a few questions/suggestions now already.

S3RK · 2021-03-10T08:58:48Z

src/script/descriptor.cpp

-        // Derive the xpub at the last hardened step
-        CExtKey xprv;
-        if (!GetExtKey(arg, xprv)) return false;
+        // Get the path to the last hardened stup
        KeyOriginInfo origin;
        int k = 0;
        for (; k <= i; ++k) {


nit: I think it would be nice to simply this function by extracting the code for splitting KeyPath. We can introduce a member function like:

std::pair<KeyPath, KeyPath> KeyPath::split_unhardened() const;

or

KeyPath KeyPath::split_unhardened();

I don't think that's really necessary since we only do the split in this one spot.

Feel free to leave it as it. But just to clarify my point. It's not about code deduplication, it's about readability and comprehension. There are 3 loops dedicated to splitting derivation path and I think they distract from the main purpose of this function.

S3RK · 2021-03-11T07:55:57Z

src/script/descriptor.h

@@ -110,7 +110,7 @@ struct Descriptor {
    virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;

    /** Convert the descriptor to a normalized string. Normalized descriptors have the xpub at the last hardened step. This fails if the provided provider does not have the private keys to derive that xpub. */
-    virtual bool ToNormalizedString(const SigningProvider& provider, std::string& out, bool priv) const = 0;
+    virtual bool ToNormalizedString(const SigningProvider& provider, std::string& out, bool priv, const DescriptorCache* cache = nullptr) const = 0;


Why not pass it as const reference? It can help to remove some of the checks and we can always relax the requirements and make it optional later if needed.

Not every caller of this will necessarily have a cache to pass.

Ok. I don't have strong feelings about it, but right now the only caller besides the unit tests is DescriptorScriptPubKeyMan. Though I can imagine other possibilities in the future I'd still go with more restrictive signature as it's easier to relax the requirements rather than the other way around.

S3RK · 2021-03-14T16:57:08Z

src/wallet/scriptpubkeyman.cpp

+    uint256 id = GetID();
+    for (const auto& lh_xpub_pair : temp_cache.GetCachedLastHardenedExtPubKeys()) {
+        CExtPubKey xpub;
+        if (m_wallet_descriptor.cache.GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {


I can't see how this ever evaluates to true since we exit early if the cache exists.

I don't follow.

This is checking to see if elements in the temp cache are in the stored cache;

But the stored cache is always empty, no? I'm looking at the line 2303

Ah, I see. There shouldn't be anything in the stored cache, however I am doing this as a belt-and-suspenders check.

I find it confusing when we check same conditions multiple times in different places because it's not clear what is the responsibility/assumptions of each class/function. It also makes it harder to modify the code. DescriptorScriptPubKeyMan and CWallet are strongly coupled anyway.

Maybe we can just remove the check on the top of the function? Even in the worst case nothing will go wrong 1) If wallet is locked then expand will fail 2) if the cache exists already we will just regenerate it.

This is largely just code duplicated from TopUp. The intention was to refactor them to avoid the duplication, but I never got around to doing that.

The check at the top of the function is important to avoid unnecessary extra computation which slows down wallet loading and unlocking.

S3RK · 2021-03-14T17:20:07Z

src/wallet/scriptpubkeyman.cpp

+    FlatSigningProvider out_keys;
+    std::vector<CScript> scripts_temp;
+    DescriptorCache temp_cache;
+    if (!m_wallet_descriptor.descriptor->Expand(0, provider, scripts_temp, out_keys, &temp_cache)){


Why do we need a temp cache?

To avoid modifying the wallet state if something goes wrong during expansion. It is possible that the cache could be modified, but something else causes expansion to fail.

I don't understand the real life scenario. From what I see Expand fails if we can't get a pub key. For example we tried to derive hardened derivation from the cache or the cache itself was inconsistent. This seems to indicate some logic error in the code itself or a corrupted state. Is my understanding correct or do I miss something?

Consider multi(2,xpub.../0/*,xpub.../0h/*). The first xpub is entirely unhardened derivation. So we will have entries added to the cache when it is expanded. When ExpandHelper moves onto the second xpub, it finds it is entirely hardened derivation. If the keys are unavailable (e.g. they're encrypted), expand now fails here. However the cache now has entries for the first xpub but no entries for the second. While this is not a dangerous state to be in, the in memory state will not be accurately reflected on disk and that is generally something that I think we should avoid. Thus the temp cache is used.

S3RK

Code review ACK 1cb5017
I don't see any logical errors in the code, it does what it supposed to do.

With that said, I don't like the direction in which DescriptorImpl::ToStringHelper and DescriptorScriptPubKeyMan::TopUp is going. The code becomes more and more complicated and there are untapped opportunities for simplification.

I believe refactoring should go together with feature implementation because of the following reasons:

just a pure refactoring PR by itself will get much less attention and therefore lower chances to get merged
this is not an urgent feature and it can wait for a refactoring to be implemented

I'm happy to propose, discuss and implement some refactoring ideas if you think it will benefit the project.

S3RK · 2021-03-17T08:42:23Z

src/wallet/scriptpubkeyman.cpp

+                }
+                continue;
+            }
+            if (!batch.WriteDescriptorLastHardenedCache(lh_xpub_pair.second, id, lh_xpub_pair.first)) {


If this is a new wallet, at what time do we set the WALLET_FLAG_LAST_HARDENED_XPUB_CACHED flag?

LoadWallet, which is called as part of wallet creation, will always call CWallet::UpgradeDescriptorCache. Since a newly created wallet is never encrypted at that stage, and has no ScriptPubKeyMans, UpgradeDescriptorCache will just set the flag.

S3RK · 2021-03-18T08:07:28Z

src/wallet/scriptpubkeyman.cpp

+    FlatSigningProvider out_keys;
+    std::vector<CScript> scripts_temp;
+    DescriptorCache temp_cache;
+    if (!m_wallet_descriptor.descriptor->Expand(0, provider, scripts_temp, out_keys, &temp_cache)){


I don't understand the real life scenario. From what I see Expand fails if we can't get a pub key. For example we tried to derive hardened derivation from the cache or the cache itself was inconsistent. This seems to indicate some logic error in the code itself or a corrupted state. Is my understanding correct or do I miss something?

S3RK · 2021-03-18T08:25:25Z

src/script/descriptor.cpp

-        // Derive the xpub at the last hardened step
-        CExtKey xprv;
-        if (!GetExtKey(arg, xprv)) return false;
+        // Get the path to the last hardened stup
        KeyOriginInfo origin;
        int k = 0;
        for (; k <= i; ++k) {


Feel free to leave it as it. But just to clarify my point. It's not about code deduplication, it's about readability and comprehension. There are 3 loops dedicated to splitting derivation path and I think they distract from the main purpose of this function.

S3RK · 2021-03-18T08:40:21Z

src/wallet/scriptpubkeyman.cpp

+    uint256 id = GetID();
+    for (const auto& lh_xpub_pair : temp_cache.GetCachedLastHardenedExtPubKeys()) {
+        CExtPubKey xpub;
+        if (m_wallet_descriptor.cache.GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {


I find it confusing when we check same conditions multiple times in different places because it's not clear what is the responsibility/assumptions of each class/function. It also makes it harder to modify the code. DescriptorScriptPubKeyMan and CWallet are strongly coupled anyway.

Maybe we can just remove the check on the top of the function? Even in the worst case nothing will go wrong 1) If wallet is locked then expand will fail 2) if the cache exists already we will just regenerate it.

S3RK · 2021-04-21T07:04:42Z

reACK ec22064. Just rebased on top of latest master

My comment above is still relevant though.

achow101 · 2021-04-21T16:31:45Z

With that said, I don't like the direction in which DescriptorImpl::ToStringHelper and DescriptorScriptPubKeyMan::TopUp is going. The code becomes more and more complicated and there are untapped opportunities for simplification.

What direction do you think it should be going in?

S3RK · 2021-04-22T07:30:02Z

What direction do you think it should be going in?

Thanks for asking. I think we need to try to simplify the code for readability and comprehension. Some rough ideas:

for ToStringHelper we can a) avoid multiple flags controlling the control flow b) shift non-general code to child classes (addr, raw, multisig).

To be more specific for this PR: I think it's better to have ToNormalizedString reimplement some logic and not call ToStringHelper at all. Both functions would be easier to reason about. We also don't need to support normalized private descriptors case, no code actually uses this.

for DescriptorScriptPubKeyMan::TopUp we can keep the code on the same/constant level of abstraction by better encapsulating cache structure. One option is to introduce something like DescriptorCache::CopyFrom(DescriptorCache) and WalletBatch::WriteCache(DescriptorCache).

achow101 · 2021-04-22T19:26:21Z

To be more specific for this PR: I think it's better to have ToNormalizedString reimplement some logic and not call ToStringHelper at all. Both functions would be easier to reason about. We also don't need to support normalized private descriptors case, no code actually uses this.

I kind of disagree. All of the To*String functions do roughly the same thing. The construction of all of the non-key related parts are all the same and it doesn't make sense to me to be duplicating that. The only parts that are different are the key things, which is why ToStringHelper has options in order to determine which key function to use. I think that instead of using bools, it would be better to use an enum, so I have implemented it that way.

for DescriptorScriptPubKeyMan::TopUp we can keep the code on the same/constant level of abstraction by better encapsulating cache structure. One option is to introduce something like DescriptorCache::CopyFrom(DescriptorCache) and WalletBatch::WriteCache(DescriptorCache).

I've added DescriptorCache::MergeAndDiff which merges another DescriptorCache with the current DescriptorCache and returns another DescriptorCache containing a copy of the new items. I've also added WalletBatch::WriteDescriptorCache which takes a DescriptorCache and writes all of the items. This cleans up TopUp and deduplicates some code.

S3RK · 2021-04-26T07:00:03Z

utACK e668b9e. Thanks for incorporating my suggestions.

src/script/descriptor.cpp

This reverts commit 09e2507. The changes made in this commit have turned out to be unnecessary and confusing, so it is being reverted.

Instead of having a large blob of cache merging code in TopUp, refactor this into DescriptorCache so that it can merge and provide a diff (another DescriptorCache containing just the items that were added). Then TopUp can just write everything that was in the diff.

Instead of adhoc writing of the items in DescriptorCache, move it all into WalletBatch.

Cache the last hardenex xpub in the DescriptorCache

Add functions to upgrade existing descriptor caches to support the use of last hardened xpub caching.

Instead of having multiple, possibly conflicting, bools controlling the flow of ToStringHelper, use an enum.

Use the descriptor xpub cache in ToNormalizedString so that the wallet does not need to be unlocked in order to get the normalized descriptor.

With the last hardened xpub cache, we don't neeed to have the wallet be unlocked for listdescriptors.

jonatack

Semi ACK e6cf0ed reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.

jonatack · 2021-06-26T12:56:01Z

src/script/descriptor.cpp

        } else if (IsHardened()) {
            CExtKey xprv;
-            if (!GetDerivedExtKey(arg, xprv)) return false;
+            CExtKey lh_xprv;


d87b544 consider naming s/lh_xprv/last_hardened_xprv/ as "lh" is a common abbreviation for left-hand and last_hardened_extkey is a localvar too

jonatack · 2021-06-26T13:07:11Z

src/script/descriptor.cpp

+    }
+    for (const auto& lh_xpub_pair : other.GetCachedLastHardenedExtPubKeys()) {
+        CExtPubKey xpub;
+        if (GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {


d87b544 could make this easier to quickly understand

Suggested change

if (GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {

if (GetCachedLastHardenedExtPubKey(/* key_exp_pos */ lh_xpub_pair.first, xpub)) {

jonatack · 2021-06-26T13:10:54Z

src/script/descriptor.cpp

+            }
+            continue;
+        }
+        CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second);


d87b544 It would be nice to make the code easier to quickly understand (using a struct instead of pairs would make the code more self-documenting but is outside the scope here)

- CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second); - diff.CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second); + CacheLastHardenedExtPubKey( + /* key_exp_pos */ last_hardened_xpub_pair.first, + /* xpub */ last_hardened_xpub_pair.second); + diff.CacheLastHardenedExtPubKey( + /* key_exp_pos */ last_hardened_xpub_pair.first, + /* xpub */ last_hardened_xpub_pair.second);

jonatack · 2021-06-26T13:27:50Z

src/wallet/walletdb.cpp

@@ -52,6 +52,7 @@ const std::string TX{"tx"};
 const std::string VERSION{"version"};
 const std::string WALLETDESCRIPTOR{"walletdescriptor"};
 const std::string WALLETDESCRIPTORCACHE{"walletdescriptorcache"};
+const std::string WALLETDESCRIPTORLHCACHE{"walletdescriptorlhcache"};


432ba9e this naming isn't very readable; s/LH/LASTHARDENED/ would be nice and per developer-notes.md, "constant names are all uppercase, and use _ to separate words"

Suggested change

const std::string WALLETDESCRIPTORLHCACHE{"walletdescriptorlhcache"};

const std::string WALLET_DESCRIPTOR_LAST_HARDENED_CACHE{"walletdescriptorlhcache"};

jonatack · 2021-06-26T13:33:06Z

src/script/descriptor.cpp

    {
        if (!GetExtKey(arg, xprv)) return false;
        for (auto entry : m_path) {
            xprv.Derive(xprv, entry);
+            if (entry >> 31) {


d87b544 it would be nice to hoist these 31 values to a well-named constant

jonatack · 2021-06-26T13:38:21Z

src/wallet/walletdb.cpp

+        }
+    }
+    for (const auto& lh_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) {
+        if (!WriteDescriptorLastHardenedCache(lh_xpub_pair.second, desc_id, lh_xpub_pair.first)) {


432ba9e readability suggestion

- for (const auto& lh_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) { - if (!WriteDescriptorLastHardenedCache(lh_xpub_pair.second, desc_id, lh_xpub_pair.first)) { + for (const auto& last_hardened_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) { + if (!WriteDescriptorLastHardenedCache(/* xpub */ last_hardened_xpub_pair.second, desc_id, /* key_exp_index */ lh_xpub_pair.first)) {

jonatack · 2021-06-26T14:31:04Z

src/wallet/scriptpubkeyman.cpp

+void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
+{
+    LOCK(cs_desc_man);
+    if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {


74fede3 It seems like IsLocked() is already checked by the callers CWallet::UpgradeDescriptorCache and CWallet::Unlock IIUC, but belt-and-suspenders, I suppose.

jonatack · 2021-06-26T14:57:43Z

src/script/descriptor.cpp

+        std::copy(id.begin(), id.begin() + 4, origin.fingerprint);
+
+        CExtPubKey xpub;
+        CExtKey lh_xprv;


3280704 naming style, suggest last_hardened_xprv, as this looks like "left-hand xprv"

jonatack · 2021-06-26T14:59:06Z

src/script/descriptor.cpp

+            cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
+        }
+        if (!xpub.pubkey.IsValid()) {
+            // Cache miss, or nor cache, or need privkey


3280704

Suggested change

// Cache miss, or nor cache, or need privkey

// Cache miss, or no cache, or need privkey

fjahr · 2021-06-27T22:48:22Z

tACK e6cf0ed

Reviewed code, light manual testing with restarts to ensure persistence works (could have been covered by a test case as well).

nit: in d87b544 there is a typo in commit description: "hardenex"

achow101 · 2021-06-29T17:51:58Z

I will implement the suggestions if I have to retouch.

S3RK · 2021-06-30T07:23:54Z

reACK e6cf0ed

Changes from last review: 1) PubkeyProvider::GetPubKey became const 2) necessary changes for taproot descriptors

Happy to reACK again if you want to incorporate jonatack's readability suggestions

fanquake · 2021-06-30T08:15:04Z

@meshcollider

meshcollider

Code review + functional test run ACK e6cf0ed

…pub and use in normalized descriptors e6cf0ed wallet, rpc: listdescriptors does not need unlocked (Andrew Chow) 3280704 Pass in DescriptorCache to ToNormalizedString (Andrew Chow) 7a26ff1 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow) 75530c9 Remove priv option for ToNormalizedString (Andrew Chow) 74fede3 wallet: Upgrade existing descriptor caches (Andrew Chow) 432ba9e wallet: Store last hardened xpub cache (Andrew Chow) d87b544 descriptors: Cache last hardened xpub (Andrew Chow) cacc391 Move DescriptorCache writing to WalletBatch (Andrew Chow) 0b4c8ef Refactor Cache merging and writing (Andrew Chow) 976b53b Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow) Pull request description: Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool). However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors. Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache. Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred). ACKs for top commit: fjahr: tACK e6cf0ed S3RK: reACK e6cf0ed jonatack: Semi ACK e6cf0ed reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose. meshcollider: Code review + functional test run ACK e6cf0ed Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d

meshcollider · 2021-06-30T21:59:45Z

Nits can be left for follow-up if needed, lets just get this in

DrahtBot · 2021-06-30T22:12:44Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

achow101 · 2021-06-30T22:31:22Z

This was merged, but github isn't detecting it.

jonatack · 2021-06-30T22:34:40Z

There have been several of these, I think this is the third one in as many days.

…use in normalized descriptors e6cf0ed wallet, rpc: listdescriptors does not need unlocked (Andrew Chow) 3280704 Pass in DescriptorCache to ToNormalizedString (Andrew Chow) 7a26ff1 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow) 75530c9 Remove priv option for ToNormalizedString (Andrew Chow) 74fede3 wallet: Upgrade existing descriptor caches (Andrew Chow) 432ba9e wallet: Store last hardened xpub cache (Andrew Chow) d87b544 descriptors: Cache last hardened xpub (Andrew Chow) cacc391 Move DescriptorCache writing to WalletBatch (Andrew Chow) 0b4c8ef Refactor Cache merging and writing (Andrew Chow) 976b53b Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow) Pull request description: Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool). However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors. Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache. Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred). ACKs for top commit: fjahr: tACK e6cf0ed S3RK: reACK e6cf0ed jonatack: Semi ACK e6cf0ed reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose. meshcollider: Code review + functional test run ACK e6cf0ed Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d

DrahtBot added RPC/REST/ZMQ Wallet labels Mar 1, 2021

sipa approved these changes Mar 2, 2021

View reviewed changes

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

DrahtBot mentioned this pull request Mar 2, 2021

rpc: replace wallet raw pointers with references (#18592 rebased) #21331

Merged

This was referenced Mar 2, 2021

A few descriptor improvements to prepare for Taproot support #21238

Merged

wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time #20096

Closed

achow101 force-pushed the norm-desc-xpub-cache branch from c096de4 to ce42387 Compare March 2, 2021 18:06

DrahtBot mentioned this pull request Mar 4, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

DrahtBot added the Needs rebase label Mar 10, 2021

achow101 force-pushed the norm-desc-xpub-cache branch from ce42387 to 1cb5017 Compare March 10, 2021 17:44

DrahtBot removed the Needs rebase label Mar 10, 2021

S3RK reviewed Mar 14, 2021

View reviewed changes

S3RK reviewed Mar 18, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 22, 2021

wallet, rpc: add an option to list private descriptors #21500

Merged

DrahtBot added the Needs rebase label Apr 20, 2021

achow101 force-pushed the norm-desc-xpub-cache branch from 1cb5017 to ec22064 Compare April 20, 2021 16:51

DrahtBot removed the Needs rebase label Apr 20, 2021

achow101 force-pushed the norm-desc-xpub-cache branch from ec22064 to e668b9e Compare April 22, 2021 19:26

meshcollider added this to the 22.0 milestone Jun 3, 2021

Sjors mentioned this pull request Jun 16, 2021

Add support for inferring tr() descriptors #22166

Merged

jonatack reviewed Jun 24, 2021

View reviewed changes

src/script/descriptor.cpp Show resolved Hide resolved

achow101 force-pushed the norm-desc-xpub-cache branch from e668b9e to 58dbf65 Compare June 24, 2021 17:47

Revert "Cache parent xpub inside of BIP32PubkeyProvider"

976b53b

This reverts commit 09e2507. The changes made in this commit have turned out to be unnecessary and confusing, so it is being reverted.

achow101 added 6 commits June 24, 2021 13:49

Move DescriptorCache writing to WalletBatch

cacc391

Instead of adhoc writing of the items in DescriptorCache, move it all into WalletBatch.

descriptors: Cache last hardened xpub

d87b544

Cache the last hardenex xpub in the DescriptorCache

wallet: Store last hardened xpub cache

432ba9e

wallet: Upgrade existing descriptor caches

74fede3

Add functions to upgrade existing descriptor caches to support the use of last hardened xpub caching.

Remove priv option for ToNormalizedString

75530c9

achow101 force-pushed the norm-desc-xpub-cache branch from 58dbf65 to 725fdc2 Compare June 24, 2021 17:49

achow101 added 3 commits June 24, 2021 14:08

Change DescriptorImpl::ToStringHelper to use an enum

7a26ff1

Instead of having multiple, possibly conflicting, bools controlling the flow of ToStringHelper, use an enum.

Pass in DescriptorCache to ToNormalizedString

3280704

Use the descriptor xpub cache in ToNormalizedString so that the wallet does not need to be unlocked in order to get the normalized descriptor.

wallet, rpc: listdescriptors does not need unlocked

e6cf0ed

With the last hardened xpub cache, we don't neeed to have the wallet be unlocked for listdescriptors.

achow101 force-pushed the norm-desc-xpub-cache branch from 725fdc2 to e6cf0ed Compare June 24, 2021 18:09

jonatack reviewed Jun 26, 2021

View reviewed changes

meshcollider reviewed Jun 30, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jun 30, 2021

achow101 closed this Jun 30, 2021

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 722776c into merged_master (Bitcoin PR bitcoin/bitcoin#21329)

8a158e3

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

	if (GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {
	if (GetCachedLastHardenedExtPubKey(/* key_exp_pos */ lh_xpub_pair.first, xpub)) {

	const std::string WALLETDESCRIPTORLHCACHE{"walletdescriptorlhcache"};
	const std::string WALLET_DESCRIPTOR_LAST_HARDENED_CACHE{"walletdescriptorlhcache"};

	// Cache miss, or nor cache, or need privkey
	// Cache miss, or no cache, or need privkey

descriptor wallet: Cache last hardened xpub and use in normalized descriptors #21329

descriptor wallet: Cache last hardened xpub and use in normalized descriptors #21329

Conversation

achow101 commented Mar 1, 2021 • edited Loading

DrahtBot commented Mar 2, 2021 • edited Loading

Conflicts

S3RK commented Mar 9, 2021

S3RK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S3RK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S3RK commented Apr 21, 2021

achow101 commented Apr 21, 2021

S3RK commented Apr 22, 2021

achow101 commented Apr 22, 2021

S3RK commented Apr 26, 2021

jonatack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr commented Jun 27, 2021

achow101 commented Jun 29, 2021

S3RK commented Jun 30, 2021

fanquake commented Jun 30, 2021

meshcollider left a comment

Choose a reason for hiding this comment

meshcollider commented Jun 30, 2021

DrahtBot commented Jun 30, 2021

achow101 commented Jun 30, 2021

jonatack commented Jun 30, 2021

achow101 commented Mar 1, 2021 •

edited

Loading

DrahtBot commented Mar 2, 2021 •

edited

Loading

jonatack Jun 26, 2021 •

edited

Loading