Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases.
Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
2709 {
2710 const CWalletTx& wtx = it->second;
2711 // Clearly invalid input, fail
2712- if (wtx.tx->vout.size() <= outpoint.n)
2713+ if (wtx.tx->vout.size() <= outpoint.n) {
2714+ bnb_used = false;
bnb_used lines really necessary? We aren’t using knapsack solver either on SelectCoins failure
CreateTransaction loop ends up infinitely looping without them
2673@@ -2674,6 +2674,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
2674 bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const
2675 {
2676 std::vector<COutput> vCoins(vAvailableCoins);
2677+ CAmount value_select = nTargetValue;
value_to_select
259@@ -250,17 +260,24 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
260 vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail
261 BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
262
263- // Make sure that we aren't using BnB when there are preset inputs
264+ // Make sure that we are using BnB when there are preset inputs
This change might have broken wallet_bumpfee.py somewhat randomly: https://travis-ci.org/bitcoin/bitcoin/jobs/604712627?utm_medium=notification&utm_source=github_status
N.B. bumpfee uses preselected inputs to create the transaction, so it might be the case that BnB ended up “hitting” where it previously hadn’t and got rid of the change output earlier than expected in the test.
This change might have broken wallet_bumpfee.py somewhat randomly:
I’m not sure why it would fail randomly since BnB should be deterministic. I also can’t reproduce this at all..
2727+ if (coin.m_input_bytes <= 0) {
2728+ bnb_used = false;
2729+ return false; // Not solvable, can't estimate size for fee
2730+ }
2731+ coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
2732+ value_to_select -= coin.effective_value;
0- value_to_select -= coin.effective_value;
1+ if (coin_selection_params.use_bnb) {
2+ value_to_select -= coin.effective_value;
3+ } else {
4+ value_to_select -= coin.txout.nValue;
5+ }
2677@@ -2674,6 +2678,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
2678 bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const
2679 {
2680 std::vector<COutput> vCoins(vAvailableCoins);
2681+ CAmount value_to_select = nTargetValue;
2682
2683 // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
2684 if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
I find it a bit concerning that no test breaks when I remove this
That’s a sanity check where the transaction in the wallet somehow doesn’t match the coin requested. Maybe it warrants a log print statement, but that can be done later.
85+ wallet.AddToWallet(*wtx.get());
86 wtxn.emplace_back(std::move(wtx));
87 }
88+static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
89+{
90+ add_coin(testWallet, nValue, nAge, fIsFromMe, nInput);
spendable?
very light code review ACK 7e6cf109c2bc3f824f23fa58cad0bf5fe8184a9f
Do not treat my review for much, I am not experienced with coin selection and am only providing limited experience when looking at this code. But it looks ok to me and I trust Sjors and instagibbs’ reviews.