From 1abbdac6777bc5396d17a6772c8176a354730997 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 29 Dec 2019 01:58:46 +0100 Subject: wallet: Prefer full destination groups in coin selection When a wallet uses avoid_reuse and has a large number of outputs in a single destination, it groups these outputs in OutputGroups that are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend as many outputs as possible from the destination while not breaking consensus due to a huge number of inputs and also not surprise the use with high fees. If there are n outputs in a destination and n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size < OUTPUT_GROUP_MAX_ENTRIES. Prior to this commit the coin selection in the case where n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be spent by the transaction is smaller than the aggregate of those of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that the coin selection decides between the different groups based on fees and mostly the smaller group will cause smaller fees. The behavior that users of the avoid_reuse flag seek is that the full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This commit implements this by pretending that the small group has a large number of ancestors (one smallet than the maximum allowed for this wallet). This dumps the small group to the bottom of the list of priorities in the coin selection algorithm. --- src/wallet/wallet.cpp | 53 ++++++++++++++++++++++++++++++++++----------------- src/wallet/wallet.h | 2 +- 2 files changed, 36 insertions(+), 19 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 519f022cc8..feb0563409 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2372,6 +2372,13 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm ++it; } + unsigned int limit_ancestor_count = 0; + unsigned int limit_descendant_count = 0; + chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); + size_t max_ancestors = (size_t)std::max(1, limit_ancestor_count); + size_t max_descendants = (size_t)std::max(1, limit_descendant_count); + bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); + // form groups from remaining coins; note that preset coins will not // automatically have their associated (same address) coins included if (coin_control.m_avoid_partial_spends && vCoins.size() > OUTPUT_GROUP_MAX_ENTRIES) { @@ -2380,14 +2387,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // explicitly shuffling the outputs before processing Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext()); } - std::vector groups = GroupOutputs(vCoins, !coin_control.m_avoid_partial_spends); - - unsigned int limit_ancestor_count; - unsigned int limit_descendant_count; - chain().getPackageLimits(limit_ancestor_count, limit_descendant_count); - size_t max_ancestors = (size_t)std::max(1, limit_ancestor_count); - size_t max_descendants = (size_t)std::max(1, limit_descendant_count); - bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); + std::vector groups = GroupOutputs(vCoins, !coin_control.m_avoid_partial_spends, max_ancestors); bool res = value_to_select <= 0 || SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) || @@ -4184,32 +4184,49 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const { std::vector groups; std::map gmap; - CTxDestination dst; + std::set full_groups; + for (const auto& output : outputs) { if (output.fSpendable) { + CTxDestination dst; CInputCoin input_coin = output.GetInputCoin(); size_t ancestors, descendants; chain().getTransactionAncestry(output.tx->GetHash(), ancestors, descendants); if (!single_coin && ExtractDestination(output.tx->tx->vout[output.i].scriptPubKey, dst)) { - // Limit output groups to no more than 10 entries, to protect - // against inadvertently creating a too-large transaction - // when using -avoidpartialspends - if (gmap[dst].m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { - groups.push_back(gmap[dst]); - gmap.erase(dst); + auto it = gmap.find(dst); + if (it != gmap.end()) { + // Limit output groups to no more than OUTPUT_GROUP_MAX_ENTRIES + // number of entries, to protect against inadvertently creating + // a too-large transaction when using -avoidpartialspends to + // prevent breaking consensus or surprising users with a very + // high amount of fees. + if (it->second.m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { + groups.push_back(it->second); + it->second = OutputGroup{}; + 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); } - gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } else { groups.emplace_back(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } } if (!single_coin) { - for (const auto& it : gmap) groups.push_back(it.second); + for (auto& it : gmap) { + auto& group = it.second; + if (full_groups.count(it.first) > 0) { + // Make this unattractive as we want coin selection to avoid it if possible + group.m_ancestors = max_ancestors - 1; + } + groups.push_back(group); + } } return groups; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 577a739d89..638c8562c9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -830,7 +830,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; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); -- cgit v1.2.3