We don’t want to use BnB when there are preset inputs because there is some weirdness with making that work with using the KnapsackSolver as the fallback. Currently we say that we haven’t used bnb when there are preset inputs, but we don’t actually disable BnB. This fixes that.
I thought this was done originally. I guess it got lost in a rebase somewhere.
We don't want to use BnB when there are preset inputs because there
is some weirdness with making that work with using the KnapsackSolver
as the fallback. Currently we say that we haven't used bnb when
there are preset inputs, but we don't actually disable BnB. This fixes
that.
2522@@ -2523,6 +2523,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2523 {
2524 // For now, don't use BnB if preset inputs are selected. TODO: Enable this later
2525 bnb_used = false;
2526+ coin_selection_params.use_bnb = false;
In commit “Actually disable BnB when there are preset inputs”
Sorry to repeat my comment from the other PR, but I do think it’d be clearer to add explicit non-reference bool disable_bnb parameters to SelectCoins methods, so the CoinSelectionParams struct can remain const and not be changing on the fly. The current bug seems like evidence to me that controlling bnb through the struct is not a great idea.
Since this is just temporary code anyway, keep the parameter const and make a copy:
0auto params = coin_selection_params;
1params.use_bnb = false;
That way this diff doesn’t have to change the function signature, you don’t need to change the header file, and you can have more const. Considering all of the other work this method is doing, the overhead of the copy won’t even be measurable.
232+ add_coin(2 * CENT);
233+ CCoinControl coin_control;
234+ coin_control.fAllowOtherInputs = true;
235+ coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
236+ BOOST_CHECK(testWallet.SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
237+ BOOST_CHECK(!bnb_used && !coin_selection_params_bnb.use_bnb);
In commit “Test that BnB is not used when there are preset inputs”
Might want to split this up into two BOOST_CHECK statements, so if one condition fails, the output shows which one is failing.
72@@ -72,6 +73,7 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
73 }
74 COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
75 vCoins.push_back(output);
76+ testWallet.AddToWallet(*wtx.get());
testWallet isn’t actually used for anything in the tests except for a way to call SelectCoins and SelectCoinsMinConf. Furthermore the coins in the wallet are never actually used by either of those two functions, the only reason it is needed here so that we can pre-select some coins for the test that was just added. And that test is also temporary, so I think it is fine as is as this change does not have any effect on the other tests.
SelectCoins itself is never actually tested; instead all of the tests focus on SelectCoinsMinConf.
