descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey #31243

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:descriptor-direct-fill changing 4 files +94 βˆ’72
  1. achow101 commented at 4:56 pm on November 7, 2024: member

    Instead of having MakeScripts infer what pubkeys need to go into the output FlatSigningProvider, have each of the PubkeyProviders that have GetPubKey called fill it directly with relevant keys and origins.

    This allows for keys and origins to be added that won’t directly appear in the output, which is necessary for musig() descriptors.

    Split from #29675

  2. DrahtBot commented at 4:56 pm on November 7, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31243.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK Sjors
    Stale ACK theStack, fjahr

    If your review is incorrectly listed, please react with πŸ‘Ž to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31868 ([IBD] specialize block serialization by l0rinc)
    • #31519 (refactor: Use std::span over Span by maflcko)
    • #31244 (descriptors: MuSig2 by achow101)
    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys 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.

  3. achow101 force-pushed on Nov 7, 2024
  4. in src/script/descriptor.cpp:215 in f50470e33c outdated
    211@@ -212,7 +212,7 @@ struct PubkeyProvider
    212     virtual bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const = 0;
    213 
    214     /** Derive a private key, if private data is available in arg. */
    215-    virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
    216+    virtual void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
    


    theStack commented at 4:07 pm on November 14, 2024:
    in commit f50470e33c1bb51531f3fa08f6d29c54df304d6b: nit: could also update the comment here, e.g. add “… and put it into the passed signing provider.”

    achow101 commented at 5:03 pm on November 19, 2024:
    Done
  5. in src/script/descriptor.cpp:713 in db270fa2a3 outdated
    711+        FlatSigningProvider subprovider;
    712+        std::vector<CPubKey> pubkeys;
    713+        pubkeys.reserve(m_pubkey_args.size());
    714 
    715-        // Construct temporary data in `entries`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
    716+        // Construct temporary data in `pks`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
    


    theStack commented at 2:46 pm on November 15, 2024:

    comment nit

    0        // Construct temporary data in `pubkeys`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
    

    achow101 commented at 5:03 pm on November 19, 2024:
    Done
  6. in src/script/descriptor.h:117 in d1be907045 outdated
    110@@ -111,6 +111,9 @@ struct Descriptor {
    111     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
    112     virtual bool IsSingleType() const = 0;
    113 
    114+    /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo() */
    115+    virtual bool IsSingleKey() const = 0;
    


    theStack commented at 4:28 pm on November 17, 2024:
    nit: I wonder if this method will still be needed once the legacy wallet (and with that, the importmulti RPC call in particular) is gone? If not, could maybe add a TODO here to get rid of it in the future.

    achow101 commented at 5:03 pm on November 19, 2024:
    Added a TODO
  7. theStack approved
  8. theStack commented at 4:31 pm on November 17, 2024: contributor

    Code-review ACK f50470e33c1bb51531f3fa08f6d29c54df304d6b

    To my understanding, the second and fourth commits (db270fa2a33d7c88e58682ab8a219a2e70503c9e and f50470e33c1bb51531f3fa08f6d29c54df304d6b) are pure refactors, while the third one actually changes behaviour for Descriptor::Expand call-sites w.r.t. by returning more public key data than before (stored in the FlatSigningProvider& out parameter), so the first commit (d1be90704559da45e4d76171230ecb9f4a6d9cf8) is needed to avoid breaking the importmulti RPC call behaviour. Without that, the “Imported scripts with pubkeys should not have their pubkeys go into the keypool” sub-test in wallet_importmulti.py fails.

  9. achow101 force-pushed on Nov 19, 2024
  10. theStack approved
  11. theStack commented at 10:15 pm on November 19, 2024: contributor
    re-ACK 186bc75985a0afa96eba3c5bfea09e22fe3ec2c8 πŸ—’οΈ
  12. in src/wallet/rpc/backup.cpp:1095 in 7c614b15bc outdated
    1090@@ -1091,6 +1091,8 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
    1091         std::tie(range_start, range_end) = ParseDescriptorRange(data["range"]);
    1092     }
    1093 
    1094+    bool can_keypool = parsed_descs.at(0)->IsSingleKey();
    


    fjahr commented at 3:25 pm on December 30, 2024:

    nit: Could add a comment here as well, something like

    0    // Only single key descriptors are allowed to be imported into a legacy wallet's keypool
    1    bool can_keypool = parsed_descs.at(0)->IsSingleKey();
    
  13. in src/script/descriptor.cpp:804 in 7c614b15bc outdated
    806@@ -807,6 +807,7 @@ class AddressDescriptor final : public DescriptorImpl
    807         return OutputTypeFromDestination(m_destination);
    808     }
    809     bool IsSingleType() const final { return true; }
    


    fjahr commented at 3:26 pm on December 30, 2024:
    nit: Typo in 7c614b15bc31a7d6f7daaa48c046eb2c7cca697e description β€œor now”
  14. DrahtBot added the label Needs rebase on Jan 21, 2025
  15. fjahr commented at 1:07 pm on January 21, 2025: contributor
    Apparently I started to review and got pinged now because of the rebase message. Leaving these nits here in case you want to address them in the rebase. Will look at this again soon.
  16. achow101 force-pushed on Jan 21, 2025
  17. DrahtBot removed the label Needs rebase on Jan 21, 2025
  18. in src/script/descriptor.cpp:215 in 1bfabb4f4e outdated
    210@@ -211,8 +211,8 @@ struct PubkeyProvider
    211      */
    212     virtual bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const = 0;
    213 
    214-    /** Derive a private key, if private data is available in arg. */
    215-    virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
    216+    /** Derive a private key, if private data is available in arg and put it into out. */
    217+    virtual void GetPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
    


    Sjors commented at 1:35 pm on January 30, 2025:

    1bfabb4f4ec07c8fb0dfc2515d413b5196e01348: maybe rename to FillPrivKey?

    0/** Try to derive the private key at pos, if private data is available in arg, and fill out with it. */
    1virtual void FillPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
    

    That also distinguishes it from ConstPubkeyProvider::GetPrivKey.


    achow101 commented at 11:07 pm on January 31, 2025:
    The verb “fill” suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think “Get” is still appropriate.

    rkrux commented at 1:51 pm on March 6, 2025:
    I am inclining to agree with @achow101 here. Although I don’t like a function that is prefixed with Get returning nothing but at the moment it’s a common pattern in the codebase to Get something and update a reference passed in one of the function arguments. The confusion, as highlighted above, that might arise with prefixing with Fill makes me not to want to use that.
  19. in src/script/descriptor.cpp:190 in e743d11aae outdated
    186@@ -189,7 +187,7 @@ struct PubkeyProvider
    187      *  write_cache is the cache to write keys to (if not nullptr)
    188      *  Caches are not exclusive but this is not tested. Currently we use them exclusively
    189      */
    190-    virtual bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const = 0;
    191+    virtual std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const = 0;
    


    Sjors commented at 1:51 pm on January 30, 2025:
    e743d11aae2e459058b1af70b6d19ab90293d1f4: maybe rename this to FillPubKey. The filling seems more important, and the return value makes it clear enough it also gets it.

    rkrux commented at 12:30 pm on March 6, 2025:

    In the function doc:

    0-/** Derive a public key.
    1+/** Derive a public key and put it into `out`
    

    achow101 commented at 5:28 pm on March 6, 2025:
    Added the comment.
  20. in src/script/descriptor.cpp:247 in e743d11aae outdated
    246-        info.path.insert(info.path.begin(), m_origin.path.begin(), m_origin.path.end());
    247-        return true;
    248+        std::optional<CPubKey> pub = m_provider->GetPubKey(pos, arg, out, read_cache, write_cache);
    249+        if (!pub) return std::nullopt;
    250+        auto& [pubkey, suborigin] = out.origins[pub->GetID()];
    251+        Assert(pubkey == *pub); // All subproviders must be inserting a valid origin already
    


    Sjors commented at 2:25 pm on January 30, 2025:

    e743d11aae2e459058b1af70b6d19ab90293d1f4: I’m confused by this comment. I guess by “valid” you mean that a KeyOriginInfo field is present, but it can be a dummy?

    It seems more intuitive if we insert an origin entry ourselves here … if possible.


    achow101 commented at 11:08 pm on January 31, 2025:
    Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it’s valid and correct.

    rkrux commented at 10:34 am on March 10, 2025:
    0-// All subproviders must be inserting a valid origin already
    1+// `m_provider` must be inserting a valid origin already
    

    Using the word subproviders here removes this function’s individuality (to some extent) by bringing in context from its caller. IIUC, at any time, there would just be one subprovider (either ConstPubkeyProvider or BIP32PubkeyProvider) that is referenced by the OriginPubkeyProvider class member m_provider.

  21. Sjors commented at 2:33 pm on January 30, 2025: member

    Concept ACK

    Mostly happy with 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348, but I’m confused about the key origin handling.

  22. in src/script/descriptor.cpp:301 in 1bfabb4f4e outdated
    297@@ -298,6 +298,14 @@ class ConstPubkeyProvider final : public PubkeyProvider
    298     CPubKey m_pubkey;
    299     bool m_xonly;
    300 
    301+    std::optional<CKey> GetPrivKey(const SigningProvider& arg) const
    


    fjahr commented at 9:00 pm on February 1, 2025:
    Hm, I am not seeing why having two GetPrivKeys is better than one. The description of the commit only talks about the other one, not this one. For this one the caller still cares about the return value it seems. Can’t they be unified under the std::optional returning variant? It seems like the variant using FlatSigningProvider is just used in one place in ExpandPrivate and the whole benefit is saving us one line there.

    achow101 commented at 6:21 pm on February 10, 2025:

    The benefit is in the MuSig2 PRs where GetPrivKey can insert multiple private keys to the FlatSigningProvider, none of which are the private key for the MuSig2 aggregate pubkey itself.

    For the ConstPubkeyProvider, it’s useful to have another GetPrivKey that returns the single private key since this is behavior is used in a few places within this class’s member functions. None of the other pubkey providers need this.


    fjahr commented at 8:04 pm on March 5, 2025:
    Understood. I think I would have chosen a different name for the one in ConstPubkeyProvider, like GetConstPrivKey maybe, but completely optional.

    rkrux commented at 1:02 pm on March 6, 2025:

    The benefit is in the MuSig2 PRs where GetPrivKey can insert multiple private keys to the FlatSigningProvider, none of which are the private key for the MuSig2 aggregate pubkey itself.

    Something around this can be mentioned either in the commit message and/or the PR description.


    rkrux commented at 1:56 pm on March 6, 2025:

    like GetConstPrivKey maybe

    This is already inside ConstPubkeyProvider class, using Const again could feel repetitive. I think the function overloading (with different argument types) is differentiating enough. Though, some documentation for the new function would be helpful for the reader.


    achow101 commented at 5:25 pm on March 6, 2025:

    Something around this can be mentioned either in the commit message and/or the PR description.

    Added to the commit message.

  23. fjahr commented at 9:01 pm on February 1, 2025: contributor

    Concept ACK

    Everything besides my GetPrivKey question looks good to me. Will do a final pass once I get an answer there.

  24. in src/script/descriptor.h:114 in 482e8c60f7 outdated
    110@@ -111,6 +111,11 @@ struct Descriptor {
    111     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
    112     virtual bool IsSingleType() const = 0;
    113 
    114+    /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo()
    


    theStack commented at 4:30 pm on February 18, 2025:

    nit: missing paren

    0    /** Whether this descriptor only produces single key scripts (i.e. pk(), pkh(), wpkh(), sh() and wsh() nested of those, and combo())
    

    achow101 commented at 6:03 am on March 6, 2025:
    If I need to retouch

    achow101 commented at 5:25 pm on March 6, 2025:
    Done
  25. in src/script/descriptor.cpp:979 in 482e8c60f7 outdated
    975@@ -971,6 +976,7 @@ class ComboDescriptor final : public DescriptorImpl
    976     {
    977         return std::make_unique<ComboDescriptor>(m_pubkey_args.at(0)->Clone());
    978     }
    979+    bool IsSingleKey() const final { return true; }
    


    theStack commented at 4:31 pm on February 18, 2025:
    nit: could move this up, right below IsSingleType, for consistency with other classes

    achow101 commented at 6:03 am on March 6, 2025:
    If I need to retouch

    achow101 commented at 5:25 pm on March 6, 2025:
    Done
  26. theStack approved
  27. theStack commented at 4:37 pm on February 18, 2025: contributor
    re-ACK 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348
  28. DrahtBot requested review from Sjors on Feb 18, 2025
  29. DrahtBot requested review from fjahr on Feb 18, 2025
  30. fjahr commented at 8:04 pm on March 5, 2025: contributor
    utACK 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348
  31. in src/script/descriptor.cpp:887 in b04bce2cd9 outdated
    882@@ -880,7 +883,6 @@ class PKHDescriptor final : public DescriptorImpl
    883     std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
    884     {
    885         CKeyID id = keys[0].GetID();
    886-        out.pubkeys.emplace(id, keys[0]);
    


    rkrux commented at 11:48 am on March 6, 2025:

    Now that out is not being used in this function.

    0-std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
    1+std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
    

    rkrux commented at 12:17 pm on March 6, 2025:
    Same for other MakeScripts functions where it’s applicable - WPKHDescriptor.

    achow101 commented at 5:28 pm on March 6, 2025:
    Done
  32. in src/script/descriptor.cpp:750 in 1bfabb4f4e outdated
    747@@ -740,8 +748,7 @@ class DescriptorImpl : public Descriptor
    748     {
    749         for (const auto& p : m_pubkey_args) {
    750             CKey key;
    


    rkrux commented at 12:55 pm on March 6, 2025:
    This variable is unused now.

    achow101 commented at 5:28 pm on March 6, 2025:
    Removed
  33. in src/script/descriptor.cpp:317 in b04bce2cd9 outdated
    305@@ -305,6 +306,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
    306         CKeyID keyid = m_pubkey.GetID();
    307         std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
    308         out.origins.emplace(keyid, std::make_pair(m_pubkey, info));
    309+        out.pubkeys.emplace(keyid, m_pubkey);
    


    rkrux commented at 1:07 pm on March 6, 2025:
    I agree with filling out with pubkeys and origins together in a function.
  34. rkrux commented at 1:15 pm on March 6, 2025: contributor
    Few initial comments, I would like to spend more time on this PR to complete the review.
  35. wallet, rpc: Only allow keypool import from single key descriptors
    Instead of relying on implicit assumptions about whether pubkeys show up
    after expanding a descriptor, just explicitly allow only single
    key descriptors to import keys into a legacy wallet's keypool.
    6a31eb5dca
  36. descriptors: Have GetPubKey fill origins directly
    Instead of having ExpandHelper fill in the origins in the
    FlatSigningProvider output, have GetPubKey do it by itself. This reduces
    the extra variables needed in order to track and set origins in
    ExpandHelper.
    
    Also changes GetPubKey to return a std::optional<CPubKey> rather than
    using a bool and output parameters.
    070e746c2f
  37. achow101 force-pushed on Mar 6, 2025
  38. descriptors: Move FlatSigningProvider pubkey filling to GetPubKey
    Instead of MakeScripts inconsistently filling the output
    FlatSigningProvider with the pubkeys involved, just do it in GetPubKey.
    08d22b4a3e
  39. descriptors: Have GetPrivKey fill keys directly
    Instead of GetPrivKey returning a key and having the caller fill the
    FlatSigningProvider, have GetPrivKey take the FlatSigningProvider and
    fill it by itself. This will be necessary for descriptors such as
    musig() where there are private keys that need to be added to the
    FlatSigningProvider but do not directly appear in any resulting scripts.
    
    GetPrivKey is now changed to void as the caller no longer cares whether
    it succeeds or fails.
    72694b8d1a
  40. achow101 force-pushed on Mar 6, 2025
  41. in src/wallet/rpc/backup.cpp:1114 in 6a31eb5dca outdated
    1109@@ -1107,8 +1110,10 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
    1110             std::vector<CScript> scripts_temp;
    1111             parsed_desc->Expand(i, keys, scripts_temp, out_keys);
    1112             std::copy(scripts_temp.begin(), scripts_temp.end(), std::inserter(script_pub_keys, script_pub_keys.end()));
    1113-            for (const auto& key_pair : out_keys.pubkeys) {
    1114-                ordered_pubkeys.emplace_back(key_pair.first, desc_internal);
    1115+            if (can_keypool) {
    1116+                for (const auto& key_pair : out_keys.pubkeys) {
    


    rkrux commented at 10:32 am on March 8, 2025:

    This new check here makes the flow more robust and avoids relying on implicit assumptions of what’s going on inside the Expand function.

    Instead of relying on implicit assumptions about whether pubkeys show up after expanding a descriptor, just explicitly allow only single key descriptors to import keys into a legacy wallet’s keypool.

    The commit message 6a31eb5dca1f8eb5fd0507852b49c7452031269e, however, was difficult for me to parse. The second clause, which I understand (and made me wonder why), didn’t seem to follow the first. Only after reading this #31243#pullrequestreview-2436530611 PR comment, I understood the need for this commit that made me want to investigate the Imported scripts with pubkeys should not have their pubkeys go into the keypool subtest in wallet_importmulti functional test that fails without this new check or relying only on the out_keys.pubkeys.size() == 1 check (that I thought of trying) as a 1-of-1 multisig scenario ends up passing that test, which is not desired.

    Legacy wallet keypool should not be updated with individual keys from multisig type scripts as per my understanding from this test. Was this the contract in the legacy wallet?

    Some rephrasing of the commit message as per the above mention comment would be helpful.

  42. in src/script/descriptor.cpp:400 in 070e746c2f outdated
    394@@ -394,18 +395,17 @@ class BIP32PubkeyProvider final : public PubkeyProvider
    395     BIP32PubkeyProvider(uint32_t exp_index, const CExtPubKey& extkey, KeyPath path, DeriveType derive, bool apostrophe) : PubkeyProvider(exp_index), m_root_extkey(extkey), m_path(std::move(path)), m_derive(derive), m_apostrophe(apostrophe) {}
    396     bool IsRange() const override { return m_derive != DeriveType::NO; }
    397     size_t GetSize() const override { return 33; }
    398-    bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key_out, KeyOriginInfo& final_info_out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    399+    std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    400     {
    401         // Info of parent of the to be derived pubkey
    


    rkrux commented at 3:25 pm on March 8, 2025:
    This comment is vaguely correct now, technically ok but doesn’t gel well with the new object name, causes confusion. IMO best to update it.
  43. in src/script/descriptor.cpp:401 in 070e746c2f outdated
    394@@ -394,18 +395,17 @@ class BIP32PubkeyProvider final : public PubkeyProvider
    395     BIP32PubkeyProvider(uint32_t exp_index, const CExtPubKey& extkey, KeyPath path, DeriveType derive, bool apostrophe) : PubkeyProvider(exp_index), m_root_extkey(extkey), m_path(std::move(path)), m_derive(derive), m_apostrophe(apostrophe) {}
    396     bool IsRange() const override { return m_derive != DeriveType::NO; }
    397     size_t GetSize() const override { return 33; }
    398-    bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key_out, KeyOriginInfo& final_info_out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    399+    std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    400     {
    401         // Info of parent of the to be derived pubkey
    402-        KeyOriginInfo parent_info;
    403+        KeyOriginInfo info;
    


    rkrux commented at 3:25 pm on March 8, 2025:

    Took me some time to gather what’s going on here.

    For reviewing and future reference as well, it would be easier if this commit 070e746c2f72db2cc24b1c267b91fafbcc586736 is split into 2: Usage of just info object and the removal of parent_info, final_info_out_tmp objects with the changes related to it can be one commit. Rest of the commit changes can be part of the other.

  44. in src/script/descriptor.cpp:406 in 070e746c2f outdated
    405-        std::copy(keyid.begin(), keyid.begin() + sizeof(parent_info.fingerprint), parent_info.fingerprint);
    406-        parent_info.path = m_path;
    407+        std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint);
    408+        info.path = m_path;
    409 
    410         // Info of the derived key itself which is copied out upon successful completion
    


    rkrux commented at 3:25 pm on March 8, 2025:
    This comment seems partly outdated now because, as I see, there is no more copying happening.
  45. in src/script/descriptor.cpp:1 in 08d22b4a3e outdated


    rkrux commented at 3:25 pm on March 8, 2025:
    Overall the update count (in this file atleast) of the members of the class FlatSigningProvider& out has decreased now because there are fewer PubkeyProviders and more DescrImpls , which IMO is desirable.

    rkrux commented at 3:26 pm on March 8, 2025:
    IMO GetPrivKey commit 72694b8d1a9863771cf1095037caeafb3f1a5635 changes deserves a mention in the PR description, currently only the pubkey changes are mentioned.
  46. in src/script/descriptor.cpp:447 in 08d22b4a3e outdated
    443@@ -442,6 +444,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
    444         if (!der) return std::nullopt;
    445 
    446         out.origins.emplace(final_extkey.pubkey.GetID(), std::make_pair(final_extkey.pubkey, info));
    447+        out.pubkeys.emplace(final_extkey.pubkey.GetID(), final_extkey.pubkey);
    


    rkrux commented at 3:25 pm on March 8, 2025:
    Coming from previous commit 070e746c2f72db2cc24b1c267b91fafbcc586736 and looking at this function as a whole, there’s a lot going on. Nit: A comment here would be quite helpful for the reader letting them know that origins & pubkeys are being appended.
  47. in src/script/descriptor.cpp:302 in 070e746c2f outdated
    298@@ -298,13 +299,13 @@ class ConstPubkeyProvider final : public PubkeyProvider
    299 
    300 public:
    301     ConstPubkeyProvider(uint32_t exp_index, const CPubKey& pubkey, bool xonly) : PubkeyProvider(exp_index), m_pubkey(pubkey), m_xonly(xonly) {}
    302-    bool GetPubKey(int pos, const SigningProvider& arg, CPubKey& key, KeyOriginInfo& info, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    303+    std::optional<CPubKey> GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override
    


    rkrux commented at 10:29 am on March 10, 2025:
    0-const SigningProvider& arg
    1+const SigningProvider&
    

    arg is not used in this function, though not caused by this PR.

  48. in src/script/descriptor.cpp:708 in 070e746c2f outdated
    708-        // Construct temporary data in `entries`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
    709+        // Construct temporary data in `pubkeys`, `subscripts`, and `subprovider` to avoid producing output in case of failure.
    710         for (const auto& p : m_pubkey_args) {
    711-            entries.emplace_back();
    712-            if (!p->GetPubKey(pos, arg, entries.back().first, entries.back().second, read_cache, write_cache)) return false;
    713+            std::optional<CPubKey> pubkey = p->GetPubKey(pos, arg, subprovider, read_cache, write_cache);
    


    rkrux commented at 10:41 am on March 10, 2025:
    +1 for removing the usage of entries.back().first, entries.back().second as the output parameters to the GetPubKey(), which was not intuitive to read for me.
  49. rkrux commented at 10:47 am on March 10, 2025: contributor
    Few comments, mostly LGTM.
  50. DrahtBot added the label Needs rebase on Mar 20, 2025
  51. DrahtBot commented at 10:16 am on March 20, 2025: contributor

    πŸ™ This pull request conflicts with the target branch and needs rebase.


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: 2025-04-02 00:13 UTC

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