Actually disable BnB when there are preset inputs #12694

achow101 · 2018-03-15T06:45:23Z

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.

eklitzke · 2018-03-15T06:47:23Z

Dare I ask if you can write a test for this?

achow101 · 2018-03-15T15:26:22Z

Added a test

ryanofsky

utACK 3f97e7cc129876b3b392057151cfece44e747938

ryanofsky · 2018-03-15T18:56:40Z

src/wallet/wallet.cpp

@@ -2523,6 +2523,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
    {
        // For now, don't use BnB if preset inputs are selected. TODO: Enable this later
        bnb_used = false;
+        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.

Implemented @eklitzke's suggestion.

Actually, I reverted that change. It makes this untestable.

ryanofsky · 2018-03-15T18:59:33Z

src/wallet/test/coinselector_tests.cpp

+    coin_control.fAllowOtherInputs = true;
+    coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
+    BOOST_CHECK(testWallet.SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
+    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.

ryanofsky

utACK 081bf54, only change since the last review is splitting BOOST_CHECK

eklitzke · 2018-03-16T22:13:13Z

src/wallet/test/coinselector_tests.cpp

@@ -72,6 +73,7 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
    }
    COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
    vCoins.push_back(output);
+    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.

eklitzke · 2018-03-16T22:19:22Z

src/wallet/wallet.cpp

@@ -2523,6 +2523,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
    {
        // For now, don't use BnB if preset inputs are selected. TODO: Enable this later
        bnb_used = false;
+        coin_selection_params.use_bnb = false;


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.

instagibbs

utACK

instagibbs · 2018-03-21T18:57:34Z

src/wallet/test/coinselector_tests.cpp

@@ -72,6 +73,7 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
    }
    COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
    vCoins.push_back(output);
+    testWallet.AddToWallet(*wtx.get());


I'd also like to see a little cleanup here, if it's not much trouble.

081bf54 Test that BnB is not used when there are preset inputs (Andrew Chow) 6ef9982 Actually disable BnB when there are preset inputs (Andrew Chow) Pull request description: 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. Tree-SHA512: 9792c0cdd0736866bddbed20f10b8050104955dc589fba49a0bd61a582ba491c921af2cdcc2269678b7b69275dad5fcf89c71b75c28733c7bacbe52e55891b9c

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

gmaxwell · 2018-09-14T17:10:14Z

@achow101 Are you keeping a coinselection todo someplace? The ability to bnb with preset inputs (less than the demanded amount) would be nice, it shouldn't be forgotten.

achow101 · 2018-09-14T18:27:48Z

@gmaxwell I think everything that really needs to be done is done in #13307. I haven't been working on it recently, partially because SRD does not seem to do as well with regards to the number of UTXOs in the wallet.

But otherwise, I don't have a todo tracking coin selection things.

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

081bf54 Test that BnB is not used when there are preset inputs (Andrew Chow) 6ef9982 Actually disable BnB when there are preset inputs (Andrew Chow) Pull request description: 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. Tree-SHA512: 9792c0cdd0736866bddbed20f10b8050104955dc589fba49a0bd61a582ba491c921af2cdcc2269678b7b69275dad5fcf89c71b75c28733c7bacbe52e55891b9c

fanquake added the Wallet label Mar 15, 2018

ryanofsky reviewed Mar 15, 2018

View reviewed changes

Test that BnB is not used when there are preset inputs

081bf54

achow101 force-pushed the fix-preset-coins-bnb branch from 3f97e7c to 081bf54 Compare March 15, 2018 19:22

ryanofsky reviewed Mar 15, 2018

View reviewed changes

eklitzke reviewed Mar 16, 2018

View reviewed changes

fanquake requested a review from morcos March 17, 2018 23:32

achow101 force-pushed the fix-preset-coins-bnb branch 3 times, most recently from 52cba62 to 081bf54 Compare March 18, 2018 04:28

instagibbs approved these changes Mar 21, 2018

View reviewed changes

laanwj merged commit 081bf54 into bitcoin:master Mar 22, 2018

ryanofsky mentioned this pull request Mar 22, 2018

scripted-diff: Rename wallet database classes #11851

Merged

achow101 added a commit to achow101/bitcoin that referenced this pull request Jun 11, 2018

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

8759e05

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

achow101 added a commit to achow101/bitcoin that referenced this pull request Jul 3, 2018

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

4ad1cb1

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

achow101 added a commit to achow101/bitcoin that referenced this pull request Aug 17, 2018

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

2f4ab05

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

instagibbs pushed a commit to instagibbs/bitcoin that referenced this pull request Aug 14, 2019

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

6736c5a

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

instagibbs pushed a commit to instagibbs/bitcoin that referenced this pull request Aug 14, 2019

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

d06ed67

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

instagibbs pushed a commit to instagibbs/bitcoin that referenced this pull request Aug 14, 2019

Revert "Merge bitcoin#12694: Actually disable BnB when there are pres…

98e51b4

…et inputs" This reverts commit 9552dfb, reversing changes made to f686002.

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Actually disable BnB when there are preset inputs #12694

Actually disable BnB when there are preset inputs #12694

Uh oh!

Conversation

achow101 commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke commented Mar 15, 2018

Uh oh!

achow101 commented Mar 15, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Sep 14, 2018

Uh oh!

achow101 commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

achow101 commented Mar 15, 2018 •

edited

Loading

achow101 commented Sep 14, 2018 •

edited

Loading