From 5cea25ba795d6eb9ccc721d01560783ae576af34 Mon Sep 17 00:00:00 2001 From: Murch Date: Fri, 6 Oct 2023 15:32:32 -0400 Subject: wallet: skip BnB when SFFO is active Co-authored-by: furszy --- src/wallet/spend.cpp | 9 +++++--- src/wallet/test/coinselector_tests.cpp | 38 ++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 35583642a5..219ad0be0f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -693,9 +693,12 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // Maximum allowed weight int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); - if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { - results.push_back(*bnb_result); - } else append_error(bnb_result); + // SFFO frequently causes issues in the context of changeless input sets: skip BnB when SFFO is active + if (!coin_selection_params.m_subtract_fee_outputs) { + if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change, max_inputs_weight)}) { + results.push_back(*bnb_result); + } else append_error(bnb_result); + } // As Knapsack and SRD can create change, also deduce change weight. max_inputs_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9569210ba0..3b7bd914ed 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -320,7 +320,6 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size); coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee; coin_selection_params_bnb.min_viable_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size); - coin_selection_params_bnb.m_subtract_fee_outputs = true; { std::unique_ptr wallet = NewWallet(m_node); @@ -345,6 +344,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinsResult available_coins; + coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); @@ -355,7 +355,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) PreSelectedInputs selected_input; selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint}); - coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); + LOCK(wallet->cs_wallet); const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); @@ -370,12 +370,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + CAmount input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(10 * CENT, 2, expected_result); + add_coin(10 * CENT + input_fee, 2, expected_result); CCoinControl coin_control; const auto result11 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result11)); @@ -385,13 +387,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(3000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(5000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(9 * CENT, 2, expected_result); - add_coin(1 * CENT, 2, expected_result); + add_coin(9 * CENT + input_fee, 2, expected_result); + add_coin(1 * CENT + input_fee, 2, expected_result); const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result12)); available_coins.Clear(); @@ -400,13 +404,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000); coin_selection_params_bnb.m_long_term_feerate = CFeeRate(3000); - add_coin(available_coins, *wallet, 10 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 9 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); - add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + // Add selectable outputs, increasing their raw amounts by their input fee to make the effective value equal to the raw amount + input_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(/*num_bytes=*/68); // bech32 input size (default test output type) + add_coin(available_coins, *wallet, 10 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 9 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); + add_coin(available_coins, *wallet, 1 * CENT + input_fee, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); expected_result.Clear(); - add_coin(9 * CENT, 2, expected_result); - add_coin(1 * CENT, 2, expected_result); + add_coin(9 * CENT + input_fee, 2, expected_result); + add_coin(1 * CENT + input_fee, 2, expected_result); coin_control.m_allow_other_inputs = true; COutput select_coin = available_coins.All().at(1); // pre select 9 coin coin_control.Select(select_coin.outpoint); -- cgit v1.2.3 From 0c5755761c3e544547899ad096121585dffa73df Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 29 Sep 2023 09:42:04 -0300 Subject: wallet: create tx, log resulting coin selection info Useful for understanding what is going on internally when the software is running. Debug issues, and provide more accurate feedback to users. --- src/wallet/spend.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'src') diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 219ad0be0f..ece5a5c5b7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1263,6 +1263,7 @@ static util::Result CreateTransactionInternal( // accidental reuse. reservedest.KeepDestination(); + wallet.WalletLogPrintf("Coin Selection: Algorithm:%s, Waste Metric Score:%d\n", GetAlgorithmName(result.GetAlgo()), result.GetWaste()); wallet.WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", current_fee, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, feeCalc.est.pass.start, feeCalc.est.pass.end, -- cgit v1.2.3 From 05e5ff194c7722b4ebc2b9309fc0bf47b3cf1df7 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 12 Sep 2023 12:14:59 -0300 Subject: test: add coverage for BnB-SFFO restriction Verify the transaction creation process does not produce a BnB solution when SFFO is enabled. This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. Co-authored-by: Andrew Chow Co-authored-by: Murch --- src/wallet/test/coinselector_tests.cpp | 38 ++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) (limited to 'src') diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 3b7bd914ed..2b7ae888d8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -455,6 +455,44 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } } +BOOST_AUTO_TEST_CASE(bnb_sffo_restriction) +{ + // Verify the coin selection process does not produce a BnB solution when SFFO is enabled. + // This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. + std::unique_ptr wallet = NewWallet(m_node); + WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(300, uint256{})); // set a high block so internal UTXOs are selectable + + FastRandomContext rand{}; + CoinSelectionParams params{ + rand, + /*change_output_size=*/ 31, // unused value, p2wpkh output size (wallet default change type) + /*change_spend_size=*/ 68, // unused value, p2wpkh input size (high-r signature) + /*min_change_target=*/ 0, // dummy, set later + /*effective_feerate=*/ CFeeRate(3000), + /*long_term_feerate=*/ CFeeRate(1000), + /*discard_feerate=*/ CFeeRate(1000), + /*tx_noinputs_size=*/ 0, + /*avoid_partial=*/ false, + }; + params.m_subtract_fee_outputs = true; + params.m_change_fee = params.m_effective_feerate.GetFee(params.change_output_size); + params.m_cost_of_change = params.m_discard_feerate.GetFee(params.change_spend_size) + params.m_change_fee; + params.m_min_change_target = params.m_cost_of_change + 1; + // Add spendable coin at the BnB selection upper bound + CoinsResult available_coins; + add_coin(available_coins, *wallet, COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + add_coin(available_coins, *wallet, 0.5 * COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + add_coin(available_coins, *wallet, 0.5 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + // Knapsack will only find a changeless solution on an exact match to the satoshi, SRD doesn’t look for changeless + // If BnB were run, it would produce a single input solution with the best waste score + auto result = WITH_LOCK(wallet->cs_wallet, return SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, COIN, /*coin_control=*/{}, params)); + BOOST_CHECK(result.has_value()); + BOOST_CHECK_NE(result->GetAlgo(), SelectionAlgorithm::BNB); + BOOST_CHECK(result->GetInputSet().size() == 2); + // We have only considered BnB, SRD, and Knapsack. Test needs to be reevaluated if new algo is added + BOOST_CHECK(result->GetAlgo() == SelectionAlgorithm::SRD || result->GetAlgo() == SelectionAlgorithm::KNAPSACK); +} + BOOST_AUTO_TEST_CASE(knapsack_solver_test) { FastRandomContext rand{}; -- cgit v1.2.3 From 576bee88fd36e207b7288077626947a1fce0fc33 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 4 Dec 2023 11:36:20 -0300 Subject: fuzz: disable BnB when SFFO is enabled --- src/wallet/test/fuzz/coinselection.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 4caf96b18d..ade3ec3f60 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -116,7 +116,8 @@ FUZZ_TARGET(coinselection) } // Run coinselection algorithms - auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); + auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} : + 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); -- cgit v1.2.3