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.
Dare I ask if you can write a test for this?
Added a test
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.
I agree that it isn't ideal, but having the caller control whether BnB should be used is just a temporary shim to work with the current coin selection. I intend for it to go away when the replacement fallback strategy is implemented (which I have already started doing so). I don't think changing it now is worth the additional review effort required when the parameter will be removed later.
Since this is just temporary code anyway, keep the parameter const and make a copy:
auto params = coin_selection_params;
params.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.
Actually, I reverted that change. It makes this untestable.
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.
Done
utACK 3f97e7cc129876b3b392057151cfece44e747938
utACK 081bf54ee4a282eed72e7de409ad6b9ab97f2987, only change since the last review is splitting BOOST_CHECK
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());
I guess the code was like this when you started, but it seems kind of dangerous to be mutating global variables in the test suite like this, because it can cause errors to propagate from one test to another. It looks like it would be pretty straightforward to change the code in the test to not use globals. Consider making these variables local to each method (maybe in a future PR?).
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.
Agree that it's fine as is, but boy scout mentality ("leave you camp site cleaner than it was before") is helpful, especially in code bases that have a lot of existing style violations (like Bitcoin).
I'd also like to see a little cleanup here, if it's not much trouble.
It's a lot of trouble to clean this up. The diff would be quite large and out of scope for this PR IMO. I think this is partially why SelectCoins itself is never actually tested; instead all of the tests focus on SelectCoinsMinConf.
utACK
