refactor pre-selected coin code #13057

instagibbs · 2018-04-23T13:47:33Z

This section of code loops over the same coins twice, in two different ways depending on whether other inputs are allowed, even though the net effect is just an early return in one case. One used the available coins list(pruned to just preselected coins) and the other the preselected coins directly. Instead just do it once, and return early if no other inputs are allowed.

Note that by definition coin control is in use, so "fSpendable" will always be true, so filtering for this is a NOP.

Added basic unit test case as well.

laanwj · 2018-04-23T15:33:52Z

src/wallet/wallet.cpp

@@ -2537,6 +2523,11 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
            return false; // TODO: Allow non-wallet inputs
    }

+    if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
+    {
+        return (nValueRet >= nTargetValue);


What about setCoinsRet?

nValueRet is never updated, so this will always return false.

So it will always fail when coin_control.fAllowOtherInputs is not set? I don't think that's supposed to be the behavior.

Empact · 2018-04-23T19:32:22Z

NACK 4aaa5ba - this code isn't redundant, the out args are used by caller CWallet::CreateTransaction

Empact · 2018-04-23T20:36:59Z

The fact that the test suite passes with this removal is a smell though - how about a test for CreateTransaction or SelectCoins that fails with this removal?

instagibbs · 2018-04-23T20:44:54Z

I will write tests fix this, thanks. Just not at my computer today.

…

On Mon, Apr 23, 2018, 4:38 PM Ben Woosley ***@***.***> wrote: The fact that the test suite passes with this removal is a smell though - how about a test for CreateTransaction or SelectCoins that fails with this removal? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13057 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC061sK4esuliPqtBCPdCuCC-mFlJ3ks5trjuzgaJpZM4Tf7aM> .

laanwj · 2018-04-24T05:57:16Z

The fact that the test suite passes with this removal is a smell though - how about a test for CreateTransaction or SelectCoins that fails with this removal?

This is what I was afraid of - I don't think we test the part of the wallet code that is used by the GUI coin selection very well.

instagibbs · 2018-04-24T14:56:02Z

did some additional refactoring to make flow easier to follow(early return if you captured enough value with presets), and added a basic unit test that should have failed with previous broken iteration

Empact · 2018-04-26T07:42:33Z

src/wallet/test/coinselector_tests.cpp

+    coin_control2.ListSelected(coin_list);
+    BOOST_CHECK(setCoinsRet.size() == coin_list.size());
+    // "over-selected" since these were mandatory
+    BOOST_CHECK(nValueRet == (5+6+11) * CENT);


BOOST_CHECK_EQUAL here and above?

Empact · 2018-04-26T08:09:42Z

src/wallet/wallet.cpp

-    for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();)
-    {
-        if (setPresetCoins.count(CInputCoin(it->tx->tx, it->i)))
+    for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();) {


nit: this could be a const_iterator

no, it's deleting entries?

The vector is deleting entries, the COutputs themselves are unmodified.

ah, I assume const_iterator was const for everything, including the underlying vector. TIL

Empact · 2018-04-26T08:09:51Z

src/wallet/wallet.cpp

-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
+    CAmount amount_to_select = nTargetValue - nValueFromPresetInputs;


instagibbs · 2018-06-11T20:15:49Z

rebased, and did a tiny bit more refactoring for more readability.

Empact · 2018-06-12T00:10:45Z

You have some whitespace inconsistencies, clang-format would be good.

Empact · 2018-06-12T00:12:47Z

src/wallet/wallet.cpp

-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
+    const CAmount amount_to_select = nTargetValue - nValueFromPresetInputs;


This loses the early return if nTargetValue <= nValueFromPresetInputs

should be covered here: https://github.com/bitcoin/bitcoin/pull/13057/files#diff-b2bb174788c7409b671c46ccc86034bdR2567

Ah, thanks, in that case how about moving this assignment up to tie the two together?

Empact · 2018-06-12T00:14:49Z

src/wallet/wallet.cpp

+            SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
+            SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
+            (!fRejectLongChains && SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
+    }


nit: I like not having repeated tests of m_spend_zero_conf_change, but I would keep it as a single assignment.

instagibbs · 2018-07-25T14:42:31Z

rebased

DrahtBot · 2019-03-13T22:59:05Z

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

Conflicts

No conflicts as of last run.

DrahtBot · 2019-04-28T19:11:54Z

The last travis run for this pull request was 277 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

Empact · 2019-04-29T11:57:45Z

utACK 4f0111e but this would be easier to review if it focused around the fix e.g.: Empact@3bcd12d

laanwj · 2019-07-08T18:54:46Z

Any specific reason for closing this?

instagibbs · 2019-07-08T19:02:32Z

No particular reason. Just something I didn't feel like pursuing review on.

fanquake added Refactoring Wallet labels Apr 23, 2018

laanwj reviewed Apr 23, 2018

View reviewed changes

instagibbs force-pushed the selectedcoins branch 2 times, most recently from d45a954 to ebc1934 Compare April 24, 2018 14:50

instagibbs changed the title ~~remove redundant pre-selected coin code~~ refactor pre-selected coin code Apr 24, 2018

instagibbs force-pushed the selectedcoins branch from ebc1934 to 358c298 Compare April 24, 2018 14:53

Empact reviewed Apr 26, 2018

View reviewed changes

instagibbs force-pushed the selectedcoins branch 3 times, most recently from 1928a6f to 228469b Compare April 26, 2018 19:32

DrahtBot added the Needs rebase label Jun 11, 2018

maflcko removed the Needs rebase label Jun 11, 2018

DrahtBot added the Needs rebase label Jun 11, 2018

instagibbs force-pushed the selectedcoins branch from 228469b to 04839bf Compare June 11, 2018 19:47

DrahtBot removed the Needs rebase label Jun 11, 2018

instagibbs force-pushed the selectedcoins branch 3 times, most recently from 0a2fca8 to c25c442 Compare June 11, 2018 20:06

Empact reviewed Jun 12, 2018

View reviewed changes

instagibbs force-pushed the selectedcoins branch 3 times, most recently from 92572a0 to 31a0170 Compare June 12, 2018 00:44

DrahtBot added the Needs rebase label Jul 24, 2018

refactor pre-selected coin code, add basic unit test case

4f0111e

instagibbs force-pushed the selectedcoins branch from 31a0170 to 4f0111e Compare July 25, 2018 14:40

DrahtBot removed the Needs rebase label Jul 25, 2018

DrahtBot mentioned this pull request Sep 7, 2018

Replace coin selection fallback strategy with Single Random Draw #13307

Closed

DrahtBot closed this Apr 28, 2019

DrahtBot reopened this Apr 28, 2019

instagibbs closed this Jul 8, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

refactor pre-selected coin code #13057

refactor pre-selected coin code #13057

Uh oh!

Conversation

instagibbs commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Apr 23, 2018

Uh oh!

Empact commented Apr 23, 2018

Uh oh!

instagibbs commented Apr 23, 2018 via email

Uh oh!

laanwj commented Apr 24, 2018

Uh oh!

instagibbs commented Apr 24, 2018

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!

Empact Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 commented Jun 11, 2018

Uh oh!

Empact commented Jun 12, 2018

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!

Empact Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 25, 2018

Uh oh!

DrahtBot commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Apr 28, 2019

Uh oh!

Empact commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 8, 2019

Uh oh!

instagibbs commented Jul 8, 2019

Uh oh!

Uh oh!

instagibbs commented Apr 23, 2018 •

edited

Loading

laanwj Apr 23, 2018 •

edited

Loading

Empact Apr 26, 2018 •

edited

Loading

Empact Jun 12, 2018 •

edited

Loading

DrahtBot commented Mar 13, 2019 •

edited

Loading

Empact commented Apr 29, 2019 •

edited

Loading