From f5cf04564d5a0e044b59d508d51b2d1cbcd1afd5 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 23 Sep 2021 15:49:18 +0200 Subject: [PATCH] refactor: pass CTxMemPool and CFeeRate in-params objects by const reference as they are classes / user-defined types. Rationale: - maintainability, no need to change further as these classes evolve - consistency - clarity for developers reading the code and this description - performance in some cases Excerpts from: - Scott Meyers, "Effective C++, Third Edition" (2005), Item 20, pages 86-94 - Scott Meyers, "Effective Modern C++" (2015), Item 41, pages 281-292 - Avoid passing objects of user-defined types by value. - One might conclude that all small types are good candidates for pass-by-value, even if they're user-defined. However, just because an object is small doesn't mean that calling its copy constructor is inexpensive. Many objects--most STL containers among them--contain little more than a pointer, but copying such objects entails copying everything they point to. That can be very expensive. - Even when small objects have inexpensive copy constructors, there can be performance issues. Some compilers treat built-in and user-defined types differently, even if they have the same underlying representation. When that type of thing happens, you can be better off passing such objects by reference, because compilers will certainly put pointers (the implementation of references) into registers. - Another reason why small user-defined types are not necessarily good pass-by-value candidates is that, being user-defined, their size is subject to change. A type that's small now may be bigger in the future if its internal implementation changes. Things can even change when switching to a different C++ implementation. - In general, the only types for which you can reasonably assume that pass-by-value is inexpensive are built-in types and STL iterator and function object types. For everything else, prefer pass-by-reference-to-const over pass-by-value. - Exception: possibly consider pass-by-value of user-defined types for copyable parameters that are cheap to move, if they are always copied anyway in their implementation. - When there are chains of function calls, each of which passes-by-value, the cost for the entire chain of calls accumulates and may not be something you can tolerate. With by-reference passing, chains of calls don't incur this kind of accumulated overhead. - The most practical approach may be to adopt a "guilty until proven innocent" policy for pass-by-value of user-defined types. --- src/policy/rbf.cpp | 6 +++--- src/policy/rbf.h | 7 ++++--- src/validation.h | 2 +- src/wallet/coinselection.h | 7 ++++--- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/fuzz/coinselection.cpp | 2 +- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index e25f5c7c5bdf..6cecbe11ca19 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -56,7 +56,7 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) } std::optional GetEntriesForConflicts(const CTransaction& tx, - CTxMemPool& pool, + const CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, CTxMemPool::setEntries& all_conflicts) { @@ -131,7 +131,7 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& } std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, - CFeeRate replacement_feerate, + const CFeeRate& replacement_feerate, const uint256& txid) { for (const auto& mi : iters_conflicting) { @@ -159,7 +159,7 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& i std::optional PaysForRBF(CAmount original_fees, CAmount replacement_fees, size_t replacement_vsize, - CFeeRate relay_fee, + const CFeeRate& relay_fee, const uint256& txid) { // BIP125 Rule #3: The replacement fees must be greater than or equal to fees of the diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 07f68c8fd4ec..0ff7cf163918 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -57,7 +57,7 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); * the start; any existing mempool entries will remain in the set. * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. */ -std::optional GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, +std::optional GetEntriesForConflicts(const CTransaction& tx, const CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, CTxMemPool::setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); @@ -88,7 +88,8 @@ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& * @returns error message if fees insufficient, otherwise std::nullopt. */ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, - CFeeRate replacement_feerate, const uint256& txid); + const CFeeRate& replacement_feerate, + const uint256& txid); /** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also @@ -103,7 +104,7 @@ std::optional PaysMoreThanConflicts(const CTxMemPool::setEntries& i std::optional PaysForRBF(CAmount original_fees, CAmount replacement_fees, size_t replacement_vsize, - CFeeRate relay_fee, + const CFeeRate& relay_fee, const uint256& txid); #endif // BITCOIN_POLICY_RBF_H diff --git a/src/validation.h b/src/validation.h index 9fef69799be0..e7729ab55d15 100644 --- a/src/validation.h +++ b/src/validation.h @@ -219,7 +219,7 @@ struct PackageMempoolAcceptResult std::map&& results) : m_state{state}, m_tx_results(std::move(results)) {} - explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, + explicit PackageMempoolAcceptResult(PackageValidationState state, const CFeeRate& feerate, std::map&& results) : m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 9135e48104f9..80f9233583c5 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -66,7 +66,7 @@ struct COutput { /** The fee required to spend this output at the consolidation feerate. */ CAmount long_term_fee{0}; - COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const std::optional feerate = std::nullopt) + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const std::optional& feerate = std::nullopt) : outpoint{outpoint}, txout{txout}, depth{depth}, @@ -147,8 +147,9 @@ struct CoinSelectionParams { bool m_avoid_partial_spends = false; CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size, - CAmount min_change_target, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) + CAmount min_change_target, const CFeeRate& effective_feerate, + const CFeeRate& long_term_feerate, const CFeeRate& discard_feerate, + size_t tx_noinputs_size, bool avoid_partial) : rng_fast{rng_fast}, change_output_size(change_output_size), change_spend_size(change_spend_size), diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index cd7fd3f4dd88..23a2d4e8f6d5 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -67,7 +67,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe set.insert(coin); } -static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, CFeeRate feerate = CFeeRate(0), int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false) +static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, const CFeeRate& feerate = CFeeRate(0), int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false) { CMutableTransaction tx; tx.nLockTime = nextLockTime++; // so all transactions get different hashes diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 3465f2f331cd..4f807d7fb891 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -14,7 +14,7 @@ namespace wallet { -static void AddCoin(const CAmount& value, int n_input, int n_input_bytes, int locktime, std::vector& coins, CFeeRate fee_rate) +static void AddCoin(const CAmount& value, int n_input, int n_input_bytes, int locktime, std::vector& coins, const CFeeRate& fee_rate) { CMutableTransaction tx; tx.vout.resize(n_input + 1);