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

pull achow101 wants to merge 10 commits into bitcoin:master from achow101:norm-desc-xpub-cache changing 13 files +272 −96
  1. achow101 commented at 10:53 pm on March 1, 2021: member

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

  2. DrahtBot added the label RPC/REST/ZMQ on Mar 1, 2021
  3. DrahtBot added the label Wallet on Mar 1, 2021
  4. in src/script/descriptor.cpp:414 in c096de4098 outdated
    410@@ -410,7 +411,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
    411         }
    412         return true;
    413     }
    414-    bool ToNormalizedString(const SigningProvider& arg, std::string& out, bool priv) const override
    415+    bool ToNormalizedString(const SigningProvider& arg, std::string& out, bool priv, std::optional<DescriptorCache> cache) const override
    


    sipa commented at 3:03 am on March 2, 2021:

    Passing a copy of the entire cache seems overkill here, and especially without std::move’s you’re going to be making copies in every step in the call graph.

    The language purist version would be std::optional<std::reference_wrapper<const DescriptorCache>> to achieve something like an optional reference to a const cache. But just const DescriptorCache* works just as well…


    sipa commented at 3:06 am on March 2, 2021:
    Argh, I meant to click comment, not approve…

    achow101 commented at 6:06 pm on March 2, 2021:
    Done
  5. sipa approved
  6. DrahtBot commented at 10:11 am on March 2, 2021: 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:

    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.

  7. achow101 force-pushed on Mar 2, 2021
  8. S3RK commented at 7:55 am on March 9, 2021: member
    Started reviewing this
  9. DrahtBot added the label Needs rebase on Mar 10, 2021
  10. in src/script/descriptor.cpp:437 in 0ab914a06b outdated
    436-        CExtKey xprv;
    437-        if (!GetExtKey(arg, xprv)) return false;
    438+        // Get the path to the last hardened stup
    439         KeyOriginInfo origin;
    440         int k = 0;
    441         for (; k <= i; ++k) {
    


    S3RK commented at 8:58 am on March 10, 2021:

    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:

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

    or

    0KeyPath KeyPath::split_unhardened();
    

    achow101 commented at 6:14 pm on March 14, 2021:
    I don’t think that’s really necessary since we only do the split in this one spot.

    S3RK commented at 8:25 am on March 18, 2021:
    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.
  11. achow101 force-pushed on Mar 10, 2021
  12. DrahtBot removed the label Needs rebase on Mar 10, 2021
  13. in src/script/descriptor.h:113 in 690160aac9 outdated
    109@@ -110,7 +110,7 @@ struct Descriptor {
    110     virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;
    111 
    112     /** 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. */
    113-    virtual bool ToNormalizedString(const SigningProvider& provider, std::string& out, bool priv) const = 0;
    114+    virtual bool ToNormalizedString(const SigningProvider& provider, std::string& out, bool priv, const DescriptorCache* cache = nullptr) const = 0;
    


    S3RK commented at 7:55 am on March 11, 2021:
    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.

    achow101 commented at 6:07 pm on March 14, 2021:
    Not every caller of this will necessarily have a cache to pass.

    S3RK commented at 7:42 am on March 15, 2021:
    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.
  14. in src/wallet/scriptpubkeyman.cpp:2321 in 8c5d41f40c outdated
    2317+    // Cache the last hardened xpubs
    2318+    WalletBatch batch(m_storage.GetDatabase());
    2319+    uint256 id = GetID();
    2320+    for (const auto& lh_xpub_pair : temp_cache.GetCachedLastHardenedExtPubKeys()) {
    2321+        CExtPubKey xpub;
    2322+        if (m_wallet_descriptor.cache.GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {
    


    S3RK commented at 4:57 pm on March 14, 2021:
    I can’t see how this ever evaluates to true since we exit early if the cache exists.

    achow101 commented at 6:10 pm on March 14, 2021:

    I don’t follow.

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


    S3RK commented at 7:27 am on March 15, 2021:
    But the stored cache is always empty, no? I’m looking at the line 2303

    achow101 commented at 3:40 pm on March 15, 2021:
    Ah, I see. There shouldn’t be anything in the stored cache, however I am doing this as a belt-and-suspenders check.

    S3RK commented at 8:40 am on March 18, 2021:

    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.


    achow101 commented at 5:16 pm on March 18, 2021:

    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.

  15. in src/wallet/scriptpubkeyman.cpp:2297 in 8c5d41f40c outdated
    2308+    FlatSigningProvider provider;
    2309+    provider.keys = GetKeys();
    2310+    FlatSigningProvider out_keys;
    2311+    std::vector<CScript> scripts_temp;
    2312+    DescriptorCache temp_cache;
    2313+    if (!m_wallet_descriptor.descriptor->Expand(0, provider, scripts_temp, out_keys, &temp_cache)){
    


    S3RK commented at 5:20 pm on March 14, 2021:
    Why do we need a temp cache?

    achow101 commented at 6:12 pm on March 14, 2021:
    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.

    S3RK commented at 8:07 am on March 18, 2021:
    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?

    achow101 commented at 5:24 pm on March 18, 2021:
    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.
  16. S3RK commented at 5:31 pm on March 14, 2021: member
    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.
  17. in src/wallet/scriptpubkeyman.cpp:1830 in e2519b0f6c outdated
    1823+                if (xpub != lh_xpub_pair.second) {
    1824+                    throw std::runtime_error(std::string(__func__) + ": New cached last hardened xpub does not match already cached last hardened xpub");
    1825+                }
    1826+                continue;
    1827+            }
    1828+            if (!batch.WriteDescriptorLastHardenedCache(lh_xpub_pair.second, id, lh_xpub_pair.first)) {
    


    S3RK commented at 8:42 am on March 17, 2021:
    If this is a new wallet, at what time do we set the WALLET_FLAG_LAST_HARDENED_XPUB_CACHED flag?

    achow101 commented at 5:12 pm on March 18, 2021:
    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.
  18. S3RK commented at 8:58 am on March 18, 2021: member

    Code review ACK 1cb501721ecbd2e3a86f16f4ed9db4cf8ac6ba3f 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.

  19. DrahtBot added the label Needs rebase on Apr 20, 2021
  20. achow101 force-pushed on Apr 20, 2021
  21. DrahtBot removed the label Needs rebase on Apr 20, 2021
  22. S3RK commented at 7:04 am on April 21, 2021: member

    reACK ec22064. Just rebased on top of latest master

    My comment above is still relevant though.

  23. achow101 commented at 4:31 pm on April 21, 2021: member

    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?

  24. S3RK commented at 7:30 am on April 22, 2021: member

    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:

    1. 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.

    1. 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).
  25. achow101 commented at 7:26 pm on April 22, 2021: member

    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.

  26. achow101 force-pushed on Apr 22, 2021
  27. S3RK commented at 7:00 am on April 26, 2021: member
    utACK e668b9e. Thanks for incorporating my suggestions.
  28. meshcollider added this to the milestone 22.0 on Jun 3, 2021
  29. in src/script/descriptor.cpp:293 in e668b9e2bb outdated
    285@@ -287,9 +286,6 @@ class BIP32PubkeyProvider final : public PubkeyProvider
    286     CExtPubKey m_root_extkey;
    287     KeyPath m_path;
    288     DeriveType m_derive;
    289-    // Cache of the parent of the final derived pubkeys.
    290-    // Primarily useful for situations when no read_cache is provided
    291-    CExtPubKey m_cached_xpub;
    


    jonatack commented at 3:06 pm on June 24, 2021:

    0cc90f1e3dbccd5940b05b633260fdcd8dac5581 essentially reverts 09e25071f40 IIUC. Perhaps re-add const to the four GetPubKey() member functions in the commit and mention in the commit message that it reverts 09e25071f40

    0-    virtual bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) = 0;
    1+    virtual bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const = 0;
    

    jonatack commented at 4:25 pm on June 24, 2021:
    Apart from this, ACK up to 8fac319f6d180fa5c597ea08beb7827331e7d9ec so far (the first three commits).

    achow101 commented at 5:47 pm on June 24, 2021:
    I’ve made this commit into a pure revert with an extra comment saying why.
  30. achow101 force-pushed on Jun 24, 2021
  31. Revert "Cache parent xpub inside of BIP32PubkeyProvider"
    This reverts commit 09e25071f40c564af08a1386c39c4f2d8eb484b6.
    
    The changes made in this commit have turned out to be unnecessary and
    confusing, so it is being reverted.
    976b53b085
  32. Refactor Cache merging and writing
    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.
    0b4c8ef75c
  33. Move DescriptorCache writing to WalletBatch
    Instead of adhoc writing of the items in DescriptorCache, move it all
    into WalletBatch.
    cacc391098
  34. descriptors: Cache last hardened xpub
    Cache the last hardenex xpub in the DescriptorCache
    d87b544b83
  35. wallet: Store last hardened xpub cache 432ba9e543
  36. wallet: Upgrade existing descriptor caches
    Add functions to upgrade existing descriptor caches to support the use
    of last hardened xpub caching.
    74fede3b8b
  37. Remove priv option for ToNormalizedString 75530c93a8
  38. achow101 force-pushed on Jun 24, 2021
  39. Change DescriptorImpl::ToStringHelper to use an enum
    Instead of having multiple, possibly conflicting, bools controlling the
    flow of ToStringHelper, use an enum.
    7a26ff10c2
  40. Pass in DescriptorCache to ToNormalizedString
    Use the descriptor xpub cache in ToNormalizedString so that the wallet
    does not need to be unlocked in order to get the normalized descriptor.
    3280704886
  41. wallet, rpc: listdescriptors does not need unlocked
    With the last hardened xpub cache, we don't neeed to have the wallet be
    unlocked for listdescriptors.
    e6cf0ed92d
  42. achow101 force-pushed on Jun 24, 2021
  43. in src/script/descriptor.cpp:357 in e6cf0ed92d
    356-            parent_extkey = final_extkey = m_cached_xpub;
    357-            if (m_derive == DeriveType::UNHARDENED) der = parent_extkey.Derive(final_extkey, pos);
    358         } else if (IsHardened()) {
    359             CExtKey xprv;
    360-            if (!GetDerivedExtKey(arg, xprv)) return false;
    361+            CExtKey lh_xprv;
    


    jonatack commented at 12:56 pm on June 26, 2021:
    d87b544b 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
  44. in src/script/descriptor.cpp:1494 in e6cf0ed92d
    1489+            diff.CacheDerivedExtPubKey(derived_xpub_map_pair.first, derived_xpub_pair.first, derived_xpub_pair.second);
    1490+        }
    1491+    }
    1492+    for (const auto& lh_xpub_pair : other.GetCachedLastHardenedExtPubKeys()) {
    1493+        CExtPubKey xpub;
    1494+        if (GetCachedLastHardenedExtPubKey(lh_xpub_pair.first, xpub)) {
    


    jonatack commented at 1:07 pm on June 26, 2021:

    d87b544b could make this easier to quickly understand

    0        if (GetCachedLastHardenedExtPubKey(/* key_exp_pos */ lh_xpub_pair.first, xpub)) {
    
  45. in src/script/descriptor.cpp:1500 in e6cf0ed92d
    1495+            if (xpub != lh_xpub_pair.second) {
    1496+                throw std::runtime_error(std::string(__func__) + ": New cached last hardened xpub does not match already cached last hardened xpub");
    1497+            }
    1498+            continue;
    1499+        }
    1500+        CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second);
    


    jonatack commented at 1:10 pm on June 26, 2021:

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

    0-        CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second);
    1-        diff.CacheLastHardenedExtPubKey(lh_xpub_pair.first, lh_xpub_pair.second);
    2+        CacheLastHardenedExtPubKey(
    3+            /* key_exp_pos */ last_hardened_xpub_pair.first,
    4+            /* xpub */ last_hardened_xpub_pair.second);
    5+        diff.CacheLastHardenedExtPubKey(
    6+            /* key_exp_pos */ last_hardened_xpub_pair.first,
    7+            /* xpub */ last_hardened_xpub_pair.second);
    
  46. in src/wallet/walletdb.cpp:55 in e6cf0ed92d
    51@@ -52,6 +52,7 @@ const std::string TX{"tx"};
    52 const std::string VERSION{"version"};
    53 const std::string WALLETDESCRIPTOR{"walletdescriptor"};
    54 const std::string WALLETDESCRIPTORCACHE{"walletdescriptorcache"};
    55+const std::string WALLETDESCRIPTORLHCACHE{"walletdescriptorlhcache"};
    


    jonatack commented at 1:27 pm on June 26, 2021:

    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”

    0const std::string WALLET_DESCRIPTOR_LAST_HARDENED_CACHE{"walletdescriptorlhcache"};
    
  47. in src/script/descriptor.cpp:309 in e6cf0ed92d
    305+    bool GetDerivedExtKey(const SigningProvider& arg, CExtKey& xprv, CExtKey& last_hardened) const
    306     {
    307         if (!GetExtKey(arg, xprv)) return false;
    308         for (auto entry : m_path) {
    309             xprv.Derive(xprv, entry);
    310+            if (entry >> 31) {
    


    jonatack commented at 1:33 pm on June 26, 2021:
    d87b544 it would be nice to hoist these 31 values to a well-named constant
  48. in src/wallet/walletdb.cpp:274 in e6cf0ed92d
    269+                return false;
    270+            }
    271+        }
    272+    }
    273+    for (const auto& lh_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) {
    274+        if (!WriteDescriptorLastHardenedCache(lh_xpub_pair.second, desc_id, lh_xpub_pair.first)) {
    


    jonatack commented at 1:38 pm on June 26, 2021:

    432ba9e readability suggestion

    0-    for (const auto& lh_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) {
    1-        if (!WriteDescriptorLastHardenedCache(lh_xpub_pair.second, desc_id, lh_xpub_pair.first)) {
    2+    for (const auto& last_hardened_xpub_pair : cache.GetCachedLastHardenedExtPubKeys()) {
    3+        if (!WriteDescriptorLastHardenedCache(/* xpub */ last_hardened_xpub_pair.second, desc_id, /* key_exp_index */ lh_xpub_pair.first)) {
    
  49. in src/wallet/scriptpubkeyman.cpp:2282 in e6cf0ed92d
    2280+}
    2281+
    2282+void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
    2283+{
    2284+    LOCK(cs_desc_man);
    2285+    if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
    


    jonatack commented at 2:31 pm on June 26, 2021:
    74fede3 It seems like IsLocked() is already checked by the callers CWallet::UpgradeDescriptorCache and CWallet::Unlock IIUC, but belt-and-suspenders, I suppose.
  50. in src/script/descriptor.cpp:451 in e6cf0ed92d
    456+        // Get the fingerprint
    457+        CKeyID id = m_root_extkey.pubkey.GetID();
    458+        std::copy(id.begin(), id.begin() + 4, origin.fingerprint);
    459+
    460+        CExtPubKey xpub;
    461+        CExtKey lh_xprv;
    


    jonatack commented at 2:57 pm on June 26, 2021:
    3280704 naming style, suggest last_hardened_xprv, as this looks like “left-hand xprv”
  51. in src/script/descriptor.cpp:457 in e6cf0ed92d
    462+        // If we have the cache, just get the parent xpub
    463+        if (cache != nullptr) {
    464+            cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
    465+        }
    466+        if (!xpub.pubkey.IsValid()) {
    467+            // Cache miss, or nor cache, or need privkey
    


    jonatack commented at 2:59 pm on June 26, 2021:

    3280704

    0            // Cache miss, or no cache, or need privkey
    
  52. jonatack commented at 3:40 pm on June 26, 2021: member
    Semi ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 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.
  53. fjahr commented at 10:48 pm on June 27, 2021: member

    tACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175

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

    nit: in d87b544b834077f102724415e0fada6ee8b2def2 there is a typo in commit description: “hardenex”

  54. achow101 commented at 5:51 pm on June 29, 2021: member
    I will implement the suggestions if I have to retouch.
  55. S3RK commented at 7:23 am on June 30, 2021: member

    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

  56. fanquake commented at 8:15 am on June 30, 2021: member
  57. meshcollider commented at 9:58 pm on June 30, 2021: contributor
    Code review + functional test run ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
  58. meshcollider referenced this in commit 722776c0fd on Jun 30, 2021
  59. meshcollider commented at 9:59 pm on June 30, 2021: contributor
    Nits can be left for follow-up if needed, lets just get this in
  60. DrahtBot commented at 10:12 pm on June 30, 2021: member

    🐙 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”.

  61. DrahtBot added the label Needs rebase on Jun 30, 2021
  62. achow101 commented at 10:31 pm on June 30, 2021: member
    This was merged, but github isn’t detecting it.
  63. jonatack commented at 10:34 pm on June 30, 2021: member
    There have been several of these, I think this is the third one in as many days.
  64. achow101 closed this on Jun 30, 2021

  65. sidhujag referenced this in commit 46eee0f6de on Jul 1, 2021
  66. gwillen referenced this in commit 8a158e3e93 on Jun 1, 2022
  67. DrahtBot locked this on Aug 18, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me