From 2a031cb2c218e288a9784d677705a7d2bc1c2d2b Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 4 May 2023 11:56:07 -0300 Subject: [PATCH 1/9] fuzz: coinselection, add `CreateCoins` Move coins creation for a specific function. It allows us to use it in other parts of the code. --- src/wallet/test/fuzz/coinselection.cpp | 32 +++++++++++++++----------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index bc935157b1..6c372b7852 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -45,6 +45,24 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect if (valid_outputgroup) output_groups.push_back(output_group); } +static CAmount CreateCoins(FuzzedDataProvider& fuzzed_data_provider, std::vector& utxo_pool, CoinSelectionParams& coin_params, int& next_locktime) +{ + CAmount total_balance{0}; + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) + { + const int n_input{fuzzed_data_provider.ConsumeIntegralInRange(0, 10)}; + const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange(41, 10000)}; + const CAmount amount{fuzzed_data_provider.ConsumeIntegralInRange(1, MAX_MONEY)}; + if (total_balance + amount >= MAX_MONEY) { + break; + } + AddCoin(amount, n_input, n_input_bytes, ++next_locktime, utxo_pool, coin_params.m_effective_feerate); + total_balance += amount; + } + + return total_balance; +} + // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result& res) { return !util::ErrorString(res).empty(); } @@ -67,20 +85,8 @@ FUZZ_TARGET(coinselection) coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange(10, 1000); coin_params.m_change_fee = effective_fee_rate.GetFee(coin_params.change_output_size); - // Create some coins - CAmount total_balance{0}; int next_locktime{0}; - LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) - { - const int n_input{fuzzed_data_provider.ConsumeIntegralInRange(0, 10)}; - const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange(100, 10000)}; - const CAmount amount{fuzzed_data_provider.ConsumeIntegralInRange(1, MAX_MONEY)}; - if (total_balance + amount >= MAX_MONEY) { - break; - } - AddCoin(amount, n_input, n_input_bytes, ++next_locktime, utxo_pool, coin_params.m_effective_feerate); - total_balance += amount; - } + CAmount total_balance{CreateCoins(fuzzed_data_provider, utxo_pool, coin_params, next_locktime)}; std::vector group_pos; GroupCoins(fuzzed_data_provider, utxo_pool, coin_params, /*positive_only=*/true, group_pos); From 90c4e6a241eee605809ab1b4331e620b92f05933 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 4 May 2023 11:57:18 -0300 Subject: [PATCH 2/9] fuzz: coinselection, add coverage for `EligibleForSpending` --- src/wallet/test/fuzz/coinselection.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 6c372b7852..6545d9ad9e 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -93,6 +93,11 @@ FUZZ_TARGET(coinselection) std::vector group_all; GroupCoins(fuzzed_data_provider, utxo_pool, coin_params, /*positive_only=*/false, group_all); + for (const OutputGroup& group : group_all) { + const CoinEligibilityFilter filter(fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()); + (void)group.EligibleForSpending(filter); + } + // Run coinselection algorithms const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); From 808618b8a25b1d9cfc4e4f1a5b4c6fff02972396 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 4 May 2023 14:52:03 -0300 Subject: [PATCH 3/9] fuzz: coinselection, add coverage for `AddInputs` --- src/wallet/test/fuzz/coinselection.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 6545d9ad9e..1a682599fe 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -99,7 +99,7 @@ FUZZ_TARGET(coinselection) } // Run coinselection algorithms - const auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); + auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_srd) { @@ -116,6 +116,22 @@ FUZZ_TARGET(coinselection) if (total_balance >= target && subtract_fee_outputs && !HasErrorMsg(result_knapsack)) { assert(result_knapsack); } + + std::vector utxos; + std::vector> results{result_srd, result_knapsack, result_bnb}; + CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)}; + if (new_total_balance > 0) { + std::set> new_utxo_pool; + for (const auto& utxo : utxos) { + new_utxo_pool.insert(std::make_shared(utxo)); + } + for (auto& result : results) { + if (!result) continue; + const auto weight{result->GetWeight()}; + result->AddInputs(new_utxo_pool, subtract_fee_outputs); + assert(result->GetWeight() > weight); + } + } } } // namespace wallet From f0244a8614ee35caef03bc326519823972ec61b4 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 4 May 2023 15:02:09 -0300 Subject: [PATCH 4/9] fuzz: coinselection, add coverage for `GetShuffledInputVector`/`GetInputSet` --- src/wallet/test/fuzz/coinselection.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 1a682599fe..03a3e7c67a 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -100,16 +100,26 @@ FUZZ_TARGET(coinselection) // Run coinselection algorithms auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); + if (result_bnb) { + (void)result_bnb->GetShuffledInputVector(); + (void)result_bnb->GetInputSet(); + } auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_srd) { assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + (void)result_srd->GetShuffledInputVector(); + (void)result_srd->GetInputSet(); } CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); - if (result_knapsack) result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + if (result_knapsack) { + result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + (void)result_knapsack->GetShuffledInputVector(); + (void)result_knapsack->GetInputSet(); + } // If the total balance is sufficient for the target and we are not using // effective values, Knapsack should always find a solution (unless the selection exceeded the max tx weight). From 1e351e5db1ced6a32681ceea8a111148bd83e323 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 5 May 2023 12:37:27 -0300 Subject: [PATCH 5/9] fuzz: coinselection, add coverage for `Merge` --- src/wallet/test/fuzz/coinselection.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 03a3e7c67a..c435e257dd 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -63,6 +63,17 @@ static CAmount CreateCoins(FuzzedDataProvider& fuzzed_data_provider, std::vector return total_balance; } +static SelectionResult ManualSelection(std::vector& utxos, const CAmount& total_amount, const bool& subtract_fee_outputs) +{ + SelectionResult result(total_amount, SelectionAlgorithm::MANUAL); + std::set> utxo_pool; + for (const auto& utxo : utxos) { + utxo_pool.insert(std::make_shared(utxo)); + } + result.AddInputs(utxo_pool, subtract_fee_outputs); + return result; +} + // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result& res) { return !util::ErrorString(res).empty(); } @@ -142,6 +153,21 @@ FUZZ_TARGET(coinselection) assert(result->GetWeight() > weight); } } + + std::vector manual_inputs; + CAmount manual_balance{CreateCoins(fuzzed_data_provider, manual_inputs, coin_params, next_locktime)}; + if (manual_balance == 0) return; + auto manual_selection{ManualSelection(manual_inputs, manual_balance, coin_params.m_subtract_fee_outputs)}; + for (auto& result : results) { + if (!result) continue; + const CAmount old_target{result->GetTarget()}; + const std::set> input_set{result->GetInputSet()}; + const int old_weight{result->GetWeight()}; + result->Merge(manual_selection); + assert(result->GetInputSet().size() == input_set.size() + manual_inputs.size()); + assert(result->GetTarget() == old_target + manual_selection.GetTarget()); + assert(result->GetWeight() == old_weight + manual_selection.GetWeight()); + } } } // namespace wallet From 0df0438c60e27df1aced6d31a192d6f334cef2d1 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 1 Jun 2023 18:37:54 -0300 Subject: [PATCH 6/9] fuzz: coinselection, improve `ComputeAndSetWaste` Instead of using `cost_of_change` for `min_viable_change` and `change_cost`, and 0 for `change_fee`, use values from `coin_params`. The previous values don't generate any effects that is relevant for that context. --- src/wallet/test/fuzz/coinselection.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index c435e257dd..5281d19e23 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -85,6 +85,7 @@ FUZZ_TARGET(coinselection) const CFeeRate long_term_fee_rate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; const CFeeRate effective_fee_rate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; const CAmount cost_of_change{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + const CAmount min_viable_change{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; const CAmount target{fuzzed_data_provider.ConsumeIntegralInRange(1, MAX_MONEY)}; const bool subtract_fee_outputs{fuzzed_data_provider.ConsumeBool()}; @@ -93,6 +94,8 @@ FUZZ_TARGET(coinselection) coin_params.m_subtract_fee_outputs = subtract_fee_outputs; coin_params.m_long_term_feerate = long_term_fee_rate; coin_params.m_effective_feerate = effective_fee_rate; + coin_params.min_viable_change = min_viable_change; + coin_params.m_cost_of_change = cost_of_change; coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange(10, 1000); coin_params.m_change_fee = effective_fee_rate.GetFee(coin_params.change_output_size); @@ -110,7 +113,7 @@ FUZZ_TARGET(coinselection) } // Run coinselection algorithms - auto result_bnb = SelectCoinsBnB(group_pos, target, cost_of_change, MAX_STANDARD_TX_WEIGHT); + auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); if (result_bnb) { (void)result_bnb->GetShuffledInputVector(); (void)result_bnb->GetInputSet(); @@ -119,7 +122,7 @@ FUZZ_TARGET(coinselection) auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_srd) { assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER - result_srd->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + result_srd->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_srd->GetShuffledInputVector(); (void)result_srd->GetInputSet(); } @@ -127,7 +130,7 @@ FUZZ_TARGET(coinselection) CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_knapsack) { - result_knapsack->ComputeAndSetWaste(cost_of_change, cost_of_change, 0); + result_knapsack->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_knapsack->GetShuffledInputVector(); (void)result_knapsack->GetInputSet(); } From b2eb55840778515d61465acc8106b27e16af1c88 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Thu, 3 Aug 2023 16:42:05 -0300 Subject: [PATCH 7/9] fuzz: coinselection, compare `GetSelectedValue` with target The valid results should have a target below the sum of the selected inputs amounts. Also, it increases the minimum value for target to make it more realistic. --- src/wallet/test/fuzz/coinselection.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 5281d19e23..0c65835ec6 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -115,12 +115,14 @@ FUZZ_TARGET(coinselection) // Run coinselection algorithms auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); if (result_bnb) { + assert(result_bnb->GetSelectedValue() >= target); (void)result_bnb->GetShuffledInputVector(); (void)result_bnb->GetInputSet(); } auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_srd) { + assert(result_srd->GetSelectedValue() >= target); assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER result_srd->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_srd->GetShuffledInputVector(); @@ -130,6 +132,7 @@ FUZZ_TARGET(coinselection) CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); if (result_knapsack) { + assert(result_knapsack->GetSelectedValue() >= target); result_knapsack->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_knapsack->GetShuffledInputVector(); (void)result_knapsack->GetInputSet(); From 6d9b26d56ab5295dfcfe0f80a3069046a263fb2f Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 4 Aug 2023 16:22:17 -0300 Subject: [PATCH 8/9] fuzz: coinselection, BnB should never produce change --- src/wallet/test/fuzz/coinselection.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 0c65835ec6..58d47acbc6 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -115,6 +115,7 @@ FUZZ_TARGET(coinselection) // Run coinselection algorithms auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); if (result_bnb) { + assert(result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0); assert(result_bnb->GetSelectedValue() >= target); (void)result_bnb->GetShuffledInputVector(); (void)result_bnb->GetInputSet(); From bf26f978ffbe7e2fc681825de631600e24e5c93e Mon Sep 17 00:00:00 2001 From: brunoerg Date: Tue, 15 Aug 2023 21:27:20 -0300 Subject: [PATCH 9/9] fuzz: coinselection, fix `m_cost_of_change` `m_cost_of_change` must not be generated randomly independent from m_change_fee. This commit changes it to set it up according to `wallet/spend`. --- src/wallet/test/fuzz/coinselection.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 58d47acbc6..4caf96b18d 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -84,7 +84,8 @@ FUZZ_TARGET(coinselection) const CFeeRate long_term_fee_rate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; const CFeeRate effective_fee_rate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; - const CAmount cost_of_change{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; + // Discard feerate must be at least dust relay feerate + const CFeeRate discard_fee_rate{fuzzed_data_provider.ConsumeIntegralInRange(DUST_RELAY_TX_FEE, COIN)}; const CAmount min_viable_change{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; const CAmount target{fuzzed_data_provider.ConsumeIntegralInRange(1, MAX_MONEY)}; const bool subtract_fee_outputs{fuzzed_data_provider.ConsumeBool()}; @@ -95,9 +96,11 @@ FUZZ_TARGET(coinselection) coin_params.m_long_term_feerate = long_term_fee_rate; coin_params.m_effective_feerate = effective_fee_rate; coin_params.min_viable_change = min_viable_change; - coin_params.m_cost_of_change = cost_of_change; coin_params.change_output_size = fuzzed_data_provider.ConsumeIntegralInRange(10, 1000); coin_params.m_change_fee = effective_fee_rate.GetFee(coin_params.change_output_size); + coin_params.m_discard_feerate = discard_fee_rate; + coin_params.change_spend_size = fuzzed_data_provider.ConsumeIntegralInRange(41, 1000); + coin_params.m_cost_of_change = coin_params.m_change_fee + coin_params.m_discard_feerate.GetFee(coin_params.change_spend_size); int next_locktime{0}; CAmount total_balance{CreateCoins(fuzzed_data_provider, utxo_pool, coin_params, next_locktime)};