test: Remove unused coin_selection param

[yancyribbens]

The param min_viable_change is never used in any of the subsequent tests and can be removed.

[achow101]

I don't think it is unused. There are a number of calls to SelectCoins in the following tests and that function uses min_viable_change.

Note that the tests are not exhaustive. If tests do not fail due to removal of code, it is not necessarily that the code was unnecessary or unused, but often that the test was deficient.

[yancyribbens]

@achow101 yes that's true, however this is part of bnb_search_test which is testing SelectCoinsBnB.

It's confusing that the tests use SelectCoins to call SelectCoinsBnB right now.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26111 (refactor: Simplify bnb coin_selection test params by yancyribbens)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[S3RK]

NACK. I added min_viable_change and it's definitely used. You can check it with git grep min_viable_change

P.S. min_viable_change is not related to BnB algorithm. it's a parameter for the transaction building.

[yancyribbens]

The test suit should fail then if it is used, however the tests all pass. Under what circumstance would min_viable_change affect the BnB algorithm? Maybe there's a test or assertion that can be added if min_viable_change setting is important to the selection process.

[aureleoules]

It's confusing that the tests use SelectCoins to call SelectCoinsBnB right now.

The coins supplied to the selection algorithm are of round value and match exactly the target, which is the reason why BnB should be used in SelectCoins.

Since BnB does not create change outputs, wouldn't it make sense to remove min_viable_change from the test?

Suggestion

diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 264c0aa59..4a5309a4f 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -345,6 +345,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);
         const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(result10);
+        BOOST_CHECK_EQUAL(result10->GetAlgo(), SelectionAlgorithm::BNB);
     }
     {
         std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
@@ -368,6 +369,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         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));
+        BOOST_CHECK_EQUAL(result11->GetAlgo(), SelectionAlgorithm::BNB);
         available_coins.Clear();
 
         // more coins should be selected when effective fee < long term fee
@@ -383,6 +385,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         add_coin(1 * CENT, 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));
+        BOOST_CHECK_EQUAL(result12->GetAlgo(), SelectionAlgorithm::BNB);
         available_coins.Clear();
 
         // pre selected coin should be selected even if disadvantageous
@@ -404,6 +407,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         available_coins.coins[OutputType::BECH32].erase(++available_coins.coins[OutputType::BECH32].begin());
         const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result13));
+        BOOST_CHECK_EQUAL(result13->GetAlgo(), SelectionAlgorithm::BNB);
     }
 }

[yancyribbens]

@aureleoules

The coins supplied to the selection algorithm are of round value and match exactly the target, which is the reason why BnB should be used in SelectCoins

It seems to me it would be cleaner to just test BnB directly instead of indirectly testing via SelectCoins, however, that seems like a much larger refactor.

Since BnB does not create change outputs, wouldn't it make sense to remove min_viable_change from the test?

I think we are driving at the same thing. BnB does not use min_viable_change nor does it accept it as an argument, which is why I removed it in this PR. I think we could certainly add the assertion you suggested, although that seems like a separate change IMO from this PR.

[S3RK]

min_viable_change is used by ComputeAndSetWaste within SelectCoins. It just so happen that the tests pass with the current default value min_viable_change=0.
I think there are a few ways to improve the test:

1. Update BnB upper bound to use min_viable_change #26466
2. Switch to use SelectCoinsBnB instead of SelectCoins
3. initialisemin_viable_change with 0 explicitly

However, I'm not sure whether option 3 is a good option in light of #26466

[yancyribbens]

SelectCoinsBnB uses cost_of_change, nont min_viable_change to call ComputeAndSetWaste:

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.cpp#L159

As I mentioned above, I do think it's better to have the tests bnb_search_test test SelectCoinsBnB instead of testing SelectCoins (option 2). In the meantime, since SelectCoinsBnB doesn't use min_viable_change we can remove that param.

[achow101]

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.