Skip to content

refactor: pass CTxMemPool and CFeeRate in-param objects by const reference #23076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jonatack wants to merge 1 commit into from
Closed

refactor: pass CTxMemPool and CFeeRate in-param objects by const reference #23076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/policy/rbf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
}

std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
CTxMemPool& pool,
const CTxMemPool& pool,
Copy link
Member

@maflcko maflcko Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

& doesn't create a copy of CTxMemPool, so I think the pull title is wrong.

Also, I wonder if this is something that can be enforced with clang-tidy, if we want it. Maybe https://clang.llvm.org/extra/clang-tidy/checks/readability/non-const-parameter.html ?

Copy link
Member Author

@jonatack jonatack Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Marco, updated the title and commit message and testing your clang-tidy idea.

Copy link
Member Author

@jonatack jonatack Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the tidy check as it proposed making a pointer to const value that needs to be mutable.

Copy link
Member

@maflcko maflcko Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't know if there is a tidy check for this. But when the reason for the changes in this pull are "consistency" #23076 (comment) , we should enforce this "consistency" to avoid incremental fixups to existing code.

Same for the CFeeRate change.

Copy link
Member Author

@jonatack jonatack Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many changes seem to be based on copying or grepping the existing code. For example, if CFeeRate in-params are all passed in the same fashion rather than, say, 2/3 by const ref and 1/3 by value, ISTM that alone may help to maintain consistency.

Copy link
Member

@maflcko maflcko Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you prevent a change from introducing this tomorrow? And then a follow-up pull to that change in tomorrow+n days?

Copy link
Member Author

@jonatack jonatack Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed consistency from the pull description summary (I think there is ample remaining rationale).

Copy link
Member

@maflcko maflcko Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment was meant generally, I only filled in a random rationale as an example.

When the reason for the changes in this pull are "$rationale", we should ideally enforce this "$rationale" to avoid incremental fixups to existing or future code.

Copy link
Member Author

@jonatack jonatack Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you prevent a change from introducing this tomorrow? And then a follow-up pull to that change in tomorrow+n days?

This is true for many changes that are merged.

There was plenty of rationale provided in the PR description and it seems pointless to continue to spend more time here.

const CTxMemPool::setEntries& iters_conflicting,
CTxMemPool::setEntries& all_conflicts)
{
Expand Down Expand Up @@ -131,7 +131,7 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
}

std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting,
CFeeRate replacement_feerate,
const CFeeRate& replacement_feerate,
const uint256& txid)
{
for (const auto& mi : iters_conflicting) {
Expand Down Expand Up @@ -159,7 +159,7 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
std::optional<std::string> 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
Expand Down
7 changes: 4 additions & 3 deletions src/policy/rbf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool,
std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, const CTxMemPool& pool,
const CTxMemPool::setEntries& iters_conflicting,
CTxMemPool::setEntries& all_conflicts)
EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
Expand Down Expand Up @@ -88,7 +88,8 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
* @returns error message if fees insufficient, otherwise std::nullopt.
*/
std::optional<std::string> 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
Expand All @@ -103,7 +104,7 @@ std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& i
std::optional<std::string> 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
2 changes: 1 addition & 1 deletion src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ struct PackageMempoolAcceptResult
std::map<const uint256, const MempoolAcceptResult>&& 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<const uint256, const MempoolAcceptResult>&& results)
: m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {}

Expand Down
7 changes: 4 additions & 3 deletions src/wallet/coinselection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CFeeRate> 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<CFeeRate>& feerate = std::nullopt)
: outpoint{outpoint},
txout{txout},
depth{depth},
Expand Down Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/coinselector_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/fuzz/coinselection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

namespace wallet {

static void AddCoin(const CAmount& value, int n_input, int n_input_bytes, int locktime, std::vector<COutput>& coins, CFeeRate fee_rate)
static void AddCoin(const CAmount& value, int n_input, int n_input_bytes, int locktime, std::vector<COutput>& coins, const CFeeRate& fee_rate)
{
CMutableTransaction tx;
tx.vout.resize(n_input + 1);
Expand Down