From 99b399aba5d27476b61b4865cc39553d03965d57 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 15:56:30 -0400 Subject: [PATCH] Move fee setting of OutputGroup to Insert OutputGroup will handle the fee and effective value computations inside of Insert. It now needs to take the effective feerate and long term feerates as arguments to its constructor. --- src/wallet/coinselection.cpp | 30 ++++++++++-------------------- src/wallet/coinselection.h | 12 ++++++++---- src/wallet/wallet.cpp | 27 ++++++++++++++------------- src/wallet/wallet.h | 2 +- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 1a45a2b313f..f4843dd0cec 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -302,6 +302,16 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) { m_outputs.push_back(output); + CInputCoin& coin = m_outputs.back(); + coin.m_fee = coin.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(coin.m_input_bytes); + fee += coin.m_fee; + + coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.m_input_bytes); + long_term_fee += coin.m_long_term_fee; + + coin.effective_value = coin.txout.nValue - coin.m_fee; + effective_value += coin.effective_value; + m_from_me &= from_me; m_value += output.txout.nValue; m_depth = std::min(m_depth, depth); @@ -312,9 +322,6 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size // descendants is the count as seen from the top ancestor, not the descendants as seen from the // coin itself; thus, this value is counted as the max, not the sum m_descendants = std::max(m_descendants, descendants); - effective_value += output.effective_value; - fee += output.m_fee; - long_term_fee += output.m_long_term_fee; } std::vector::iterator OutputGroup::Discard(const CInputCoin& output) { @@ -335,23 +342,6 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f && m_descendants <= eligibility_filter.max_descendants; } -void OutputGroup::SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate) -{ - fee = 0; - long_term_fee = 0; - effective_value = 0; - for (CInputCoin& coin : m_outputs) { - coin.m_fee = coin.m_input_bytes < 0 ? 0 : effective_feerate.GetFee(coin.m_input_bytes); - fee += coin.m_fee; - - coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes); - long_term_fee += coin.m_long_term_fee; - - coin.effective_value = coin.txout.nValue - coin.m_fee; - effective_value += coin.effective_value; - } -} - OutputGroup OutputGroup::GetPositiveOnlyGroup() { OutputGroup group(*this); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 1a0373eba18..721f1a22ebf 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -6,11 +6,10 @@ #define BITCOIN_WALLET_COINSELECTION_H #include +#include #include #include -class CFeeRate; - //! target minimum change amount static constexpr CAmount MIN_CHANGE{COIN / 100}; //! final minimum change amount after paying for fees @@ -78,15 +77,20 @@ struct OutputGroup size_t m_descendants{0}; CAmount effective_value{0}; CAmount fee{0}; + CFeeRate m_effective_feerate{0}; CAmount long_term_fee{0}; + CFeeRate m_long_term_feerate{0}; OutputGroup() {} + OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) : + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate) + {} + void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants); std::vector::iterator Discard(const CInputCoin& output); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; - //! Update the OutputGroup's fee, long_term_fee, and effective_value based on the given feerates - void SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate); OutputGroup GetPositiveOnlyGroup(); }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 674849e26ae..71db03c32b4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2381,7 +2381,14 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil temp.m_confirm_target = 1008; CFeeRate long_term_feerate = GetMinimumFeeRate(*this, temp, &feeCalc); - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + // Get the feerate for effective value. + // When subtracting the fee from the outputs, we want the effective feerate to be 0 + CFeeRate effective_feerate{0}; + if (!coin_selection_params.m_subtract_fee_outputs) { + effective_feerate = coin_selection_params.effective_fee; + } + + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate); // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); @@ -2390,13 +2397,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil for (OutputGroup& group : groups) { if (!group.EligibleForSpending(eligibility_filter)) continue; - if (coin_selection_params.m_subtract_fee_outputs) { - // Set the effective feerate to 0 as we don't want to use the effective value since the fees will be deducted from the output - group.SetFees(CFeeRate(0) /* effective_feerate */, long_term_feerate); - } else { - group.SetFees(coin_selection_params.effective_fee, long_term_feerate); - } - OutputGroup pos_group = group.GetPositiveOnlyGroup(); if (pos_group.effective_value > 0) utxo_pool.push_back(pos_group); } @@ -2405,7 +2405,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil bnb_used = true; return SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0)); // Filter by the min conf specs and add to utxo_pool for (const OutputGroup& group : groups) { @@ -4206,7 +4206,7 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const { std::vector groups; std::map gmap; std::set full_groups; @@ -4228,15 +4228,16 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu // high amount of fees. if (it->second.m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { groups.push_back(it->second); - it->second = OutputGroup{}; + it->second = OutputGroup{effective_feerate, long_term_feerate}; full_groups.insert(dst); } it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } else { - gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + auto ins = gmap.emplace(dst, OutputGroup{effective_feerate, long_term_feerate}); + ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } else { - groups.emplace_back(); + groups.emplace_back(effective_feerate, long_term_feerate); groups.back().Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 69bafd2e889..448b80c7bb8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);