From 1dd092367789749527777ac2b256e639f5706584 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Mon, 7 May 2018 20:42:49 -0400 Subject: [PATCH 1/3] refactor: Track BnB selection by index This is a performance optimization - rather than track all visited values in a bool vector, track the selected index in a vector. This results in a complexity reduction of O(utxo_size) to O(selection_size). --- src/wallet/coinselection.cpp | 46 ++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 23faad027f7..b3d7c685dd1 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -66,9 +66,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo { SelectionResult result(selection_target); CAmount curr_value = 0; - - std::vector curr_selection; // select the utxo at this index - curr_selection.reserve(utxo_pool.size()); + std::vector curr_selection; // selected utxo indexes // Calculate curr_available_value CAmount curr_available_value = 0; @@ -85,11 +83,11 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo std::sort(utxo_pool.begin(), utxo_pool.end(), descending); CAmount curr_waste = 0; - std::vector best_selection; + std::vector best_selection; CAmount best_waste = MAX_MONEY; // Depth First search loop for choosing the UTXOs - for (size_t i = 0; i < TOTAL_TRIES; ++i) { + for (size_t i = 0, utxo_pool_index = 0; i < TOTAL_TRIES; ++i, ++utxo_pool_index) { // Conditions for starting a backtrack bool backtrack = false; if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. @@ -104,7 +102,6 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo // explore any more UTXOs to avoid burning money like that. if (curr_waste <= best_waste) { best_selection = curr_selection; - best_selection.resize(utxo_pool.size()); best_waste = curr_waste; if (best_waste == 0) { break; @@ -116,36 +113,37 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo // Backtracking, moving backwards if (backtrack) { - // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. - while (!curr_selection.empty() && !curr_selection.back()) { - curr_selection.pop_back(); - curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); - } - if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched break; } + // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. + for (--utxo_pool_index; utxo_pool_index > curr_selection.back(); --utxo_pool_index) { + curr_available_value += utxo_pool.at(utxo_pool_index).GetSelectionAmount(); + } + // Output was included on previous iterations, try excluding now. - curr_selection.back() = false; - OutputGroup& utxo = utxo_pool.at(curr_selection.size() - 1); + assert(utxo_pool_index == curr_selection.back()); + OutputGroup& utxo = utxo_pool.at(utxo_pool_index); curr_value -= utxo.GetSelectionAmount(); curr_waste -= utxo.fee - utxo.long_term_fee; + curr_selection.pop_back(); } else { // Moving forwards, continuing down this branch - OutputGroup& utxo = utxo_pool.at(curr_selection.size()); + OutputGroup& utxo = utxo_pool.at(utxo_pool_index); // Remove this utxo from the curr_available_value utxo amount curr_available_value -= utxo.GetSelectionAmount(); // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded. Since the ratio of fee to // long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same. - if (!curr_selection.empty() && !curr_selection.back() && - utxo.GetSelectionAmount() == utxo_pool.at(curr_selection.size() - 1).GetSelectionAmount() && - utxo.fee == utxo_pool.at(curr_selection.size() - 1).fee) { - curr_selection.push_back(false); - } else { + if (curr_selection.empty() || + // The previous index is included and therefore not relevant for exclusion shortcut + (utxo_pool_index - 1) == curr_selection.back() || + utxo.GetSelectionAmount() != utxo_pool.at(utxo_pool_index - 1).GetSelectionAmount() || + utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee) + { // Inclusion branch first (Largest First Exploration) - curr_selection.push_back(true); + curr_selection.push_back(utxo_pool_index); curr_value += utxo.GetSelectionAmount(); curr_waste += utxo.fee - utxo.long_term_fee; } @@ -158,10 +156,8 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo } // Set output set - for (size_t i = 0; i < best_selection.size(); ++i) { - if (best_selection.at(i)) { - result.AddInput(utxo_pool.at(i)); - } + for (const size_t& i : best_selection) { + result.AddInput(utxo_pool.at(i)); } return result; From def43a4d888b4a21c082404d1b25707c481d7625 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Mon, 28 Feb 2022 12:07:57 +0000 Subject: [PATCH 2/3] refactor: Rename i to curr_try in SelectCoinsBnB Clarifies purpose and removes name collisions with other indicies. --- src/wallet/coinselection.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index b3d7c685dd1..c63804a7a94 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -87,7 +87,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo CAmount best_waste = MAX_MONEY; // Depth First search loop for choosing the UTXOs - for (size_t i = 0, utxo_pool_index = 0; i < TOTAL_TRIES; ++i, ++utxo_pool_index) { + for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) { // Conditions for starting a backtrack bool backtrack = false; if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. From 9d2005285c77f6c4148bccfa0b8b9135abfa021c Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Mon, 28 Feb 2022 12:10:16 +0000 Subject: [PATCH 3/3] doc: Revise comments and whitespace to clarify --- src/wallet/coinselection.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index c63804a7a94..2f4e5885674 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -71,7 +71,8 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo // Calculate curr_available_value CAmount curr_available_value = 0; for (const OutputGroup& utxo : utxo_pool) { - // Assert that this utxo is not negative. It should never be negative, effective value calculation should have removed it + // Assert that this utxo is not negative. It should never be negative, + // effective value calculation should have removed it assert(utxo.GetSelectionAmount() > 0); curr_available_value += utxo.GetSelectionAmount(); } @@ -90,8 +91,8 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) { // Conditions for starting a backtrack bool backtrack = false; - if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. - curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch + if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. + curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch (curr_waste > best_waste && (utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0)) { // Don't select things which we know will be more wasteful if the waste is increasing backtrack = true; } else if (curr_value >= selection_target) { // Selected value is within range @@ -111,13 +112,12 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo backtrack = true; } - // Backtracking, moving backwards - if (backtrack) { + if (backtrack) { // Backtracking, moving backwards if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched break; } - // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. + // Add omitted UTXOs back to lookahead before traversing the omission branch of last included UTXO. for (--utxo_pool_index; utxo_pool_index > curr_selection.back(); --utxo_pool_index) { curr_available_value += utxo_pool.at(utxo_pool_index).GetSelectionAmount(); } @@ -134,11 +134,11 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo // Remove this utxo from the curr_available_value utxo amount curr_available_value -= utxo.GetSelectionAmount(); - // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded. Since the ratio of fee to - // long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same. if (curr_selection.empty() || // The previous index is included and therefore not relevant for exclusion shortcut (utxo_pool_index - 1) == curr_selection.back() || + // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded. + // Since the ratio of fee to long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same. utxo.GetSelectionAmount() != utxo_pool.at(utxo_pool_index - 1).GetSelectionAmount() || utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee) {