diff options
author | Andrew Chow <achow101-github@achow101.com> | 2022-08-17 17:46:27 -0400 |
---|---|---|
committer | Andrew Chow <achow101-github@achow101.com> | 2022-08-17 17:57:33 -0400 |
commit | a8f69541ad53a76d5f69044ba829069d225a4af1 (patch) | |
tree | c0501ff34e5107ca7e4ef9bcc02ad3762b9cea18 /src | |
parent | a75b7796b7b37be0d313d10a2ed683534ce7e95a (diff) | |
parent | fa3f15f2dd94ae597a66037f5928fe4e90fe099d (diff) | |
download | bitcoin-a8f69541ad53a76d5f69044ba829069d225a4af1.tar.xz |
Merge bitcoin/bitcoin#25748: refactor: Avoid copies in FlatSigningProvider Merge
fa3f15f2dd94ae597a66037f5928fe4e90fe099d refactor: Avoid copies in FlatSigningProvider Merge (MacroFake)
Pull request description:
`Merge` will create several copies unconditionally:
* To initialize the args `a`, and `b`
* `ret`, which is the merge of the two args
So change the code to let the caller decide how many copies they need/want:
* `a`, and `b` must be explicitly moved or copied by the caller
* `ret` is no longer needed, as `a` can be used for it in place "for free"
ACKs for top commit:
achow101:
ACK fa3f15f2dd94ae597a66037f5928fe4e90fe099d
furszy:
looks good, ACK fa3f15f2
ryanofsky:
Code review ACK fa3f15f2dd94ae597a66037f5928fe4e90fe099d. Confirmed that all the places `std::move` was added the argument actually did seem safe to move from. Compiler enforces that temporary copies are explicitly created in non-move cases.
Tree-SHA512: 7c027ccdea1549cd9f37403344ecbb76e008adf545f6ce52996bf95e89eb7dc89af6cb31435a9289d6f2eea1c416961b2fb96348bc8a211d550728f1d99ac49c
Diffstat (limited to 'src')
-rw-r--r-- | src/script/descriptor.cpp | 2 | ||||
-rw-r--r-- | src/script/signingprovider.cpp | 22 | ||||
-rw-r--r-- | src/script/signingprovider.h | 5 | ||||
-rw-r--r-- | src/test/descriptor_tests.cpp | 2 | ||||
-rw-r--r-- | src/wallet/rpc/spend.cpp | 2 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.cpp | 10 |
6 files changed, 19 insertions, 24 deletions
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 9bcbe1ceef..8ace9c3136 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -644,7 +644,7 @@ public: assert(outscripts.size() == 1); subscripts.emplace_back(std::move(outscripts[0])); } - out = Merge(std::move(out), std::move(subprovider)); + out.Merge(std::move(subprovider)); std::vector<CPubKey> pubkeys; pubkeys.reserve(entries.size()); diff --git a/src/script/signingprovider.cpp b/src/script/signingprovider.cpp index c624a17628..a82a8b252a 100644 --- a/src/script/signingprovider.cpp +++ b/src/script/signingprovider.cpp @@ -77,20 +77,14 @@ bool FlatSigningProvider::GetTaprootBuilder(const XOnlyPubKey& output_key, Tapro return LookupHelper(tr_trees, output_key, builder); } -FlatSigningProvider Merge(const FlatSigningProvider& a, const FlatSigningProvider& b) -{ - FlatSigningProvider ret; - ret.scripts = a.scripts; - ret.scripts.insert(b.scripts.begin(), b.scripts.end()); - ret.pubkeys = a.pubkeys; - ret.pubkeys.insert(b.pubkeys.begin(), b.pubkeys.end()); - ret.keys = a.keys; - ret.keys.insert(b.keys.begin(), b.keys.end()); - ret.origins = a.origins; - ret.origins.insert(b.origins.begin(), b.origins.end()); - ret.tr_trees = a.tr_trees; - ret.tr_trees.insert(b.tr_trees.begin(), b.tr_trees.end()); - return ret; +FlatSigningProvider& FlatSigningProvider::Merge(FlatSigningProvider&& b) +{ + scripts.merge(b.scripts); + pubkeys.merge(b.pubkeys); + keys.merge(b.keys); + origins.merge(b.origins); + tr_trees.merge(b.tr_trees); + return *this; } void FillableSigningProvider::ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey) diff --git a/src/script/signingprovider.h b/src/script/signingprovider.h index 792cc903f2..2d4234ea0b 100644 --- a/src/script/signingprovider.h +++ b/src/script/signingprovider.h @@ -6,6 +6,7 @@ #ifndef BITCOIN_SCRIPT_SIGNINGPROVIDER_H #define BITCOIN_SCRIPT_SIGNINGPROVIDER_H +#include <attributes.h> #include <key.h> #include <pubkey.h> #include <script/keyorigin.h> @@ -85,9 +86,9 @@ struct FlatSigningProvider final : public SigningProvider bool GetKey(const CKeyID& keyid, CKey& key) const override; bool GetTaprootSpendData(const XOnlyPubKey& output_key, TaprootSpendData& spenddata) const override; bool GetTaprootBuilder(const XOnlyPubKey& output_key, TaprootBuilder& builder) const override; -}; -FlatSigningProvider Merge(const FlatSigningProvider& a, const FlatSigningProvider& b); + FlatSigningProvider& Merge(FlatSigningProvider&& b) LIFETIMEBOUND; +}; /** Fillable signing provider that keeps keys in an address->secret map */ class FillableSigningProvider : public SigningProvider diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 1eb4b373b4..d9503d7624 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -312,7 +312,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string& txdata.Init(spend, std::move(utxos), /*force=*/true); MutableTransactionSignatureCreator creator{spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT}; SignatureData sigdata; - BOOST_CHECK_MESSAGE(ProduceSignature(Merge(keys_priv, script_provider), creator, spks[n], sigdata), prv); + BOOST_CHECK_MESSAGE(ProduceSignature(FlatSigningProvider{keys_priv}.Merge(FlatSigningProvider{script_provider}), creator, spks[n], sigdata), prv); } /* Infer a descriptor from the generated script, and verify its solvability and that it roundtrips. */ diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 699d9062ff..8bbb862a7b 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -644,7 +644,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unable to parse descriptor '%s': %s", desc_str, error)); } desc->Expand(0, desc_out, scripts_temp, desc_out); - coinControl.m_external_provider = Merge(coinControl.m_external_provider, desc_out); + coinControl.m_external_provider.Merge(std::move(desc_out)); } } } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 7511312f70..8d40846332 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2085,7 +2085,7 @@ std::unique_ptr<FlatSigningProvider> DescriptorScriptPubKeyMan::GetSigningProvid // Fetch SigningProvider from cache to avoid re-deriving auto it = m_map_signing_providers.find(index); if (it != m_map_signing_providers.end()) { - *out_keys = Merge(*out_keys, it->second); + out_keys->Merge(FlatSigningProvider{it->second}); } else { // Get the scripts, keys, and key origins for this script std::vector<CScript> scripts_temp; @@ -2122,7 +2122,7 @@ bool DescriptorScriptPubKeyMan::SignTransaction(CMutableTransaction& tx, const s if (!coin_keys) { continue; } - *keys = Merge(*keys, *coin_keys); + keys->Merge(std::move(*coin_keys)); } return ::SignTransaction(tx, keys.get(), coins, sighash, input_errors); @@ -2183,7 +2183,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& std::unique_ptr<FlatSigningProvider> keys = std::make_unique<FlatSigningProvider>(); std::unique_ptr<FlatSigningProvider> script_keys = GetSigningProvider(script, sign); if (script_keys) { - *keys = Merge(*keys, *script_keys); + keys->Merge(std::move(*script_keys)); } else { // Maybe there are pubkeys listed that we can sign for script_keys = std::make_unique<FlatSigningProvider>(); @@ -2191,7 +2191,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& const CPubKey& pubkey = pk_pair.first; std::unique_ptr<FlatSigningProvider> pk_keys = GetSigningProvider(pubkey); if (pk_keys) { - *keys = Merge(*keys, *pk_keys); + keys->Merge(std::move(*pk_keys)); } } for (const auto& pk_pair : input.m_tap_bip32_paths) { @@ -2203,7 +2203,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& fullpubkey.Set(b, b + 33); std::unique_ptr<FlatSigningProvider> pk_keys = GetSigningProvider(fullpubkey); if (pk_keys) { - *keys = Merge(*keys, *pk_keys); + keys->Merge(std::move(*pk_keys)); } } } |