refactor pre-selected coin code #13057

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:selectedcoins changing 2 files +46 −28
  1. instagibbs commented at 1:47 pm on April 23, 2018: member

    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.

  2. fanquake added the label Refactoring on Apr 23, 2018
  3. fanquake added the label Wallet on Apr 23, 2018
  4. in src/wallet/wallet.cpp:2570 in 4aaa5ba437 outdated 
    2522@@ -2537,6 +2523,11 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2523             return false; // TODO: Allow non-wallet inputs
2524     }
2525 
2526+    if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
2527+    {
2528+        return (nValueRet >= nTargetValue);
    laanwj commented at 3:33 pm on April 23, 2018:
    What about setCoinsRet?
    Empact commented at 4:09 pm on April 23, 2018:
    nValueRet is never updated, so this will always return false.
    laanwj commented at 5:47 pm on April 23, 2018:
    So it will always fail when coin_control.fAllowOtherInputs is not set? I don’t think that’s supposed to be the behavior.
  5. Empact commented at 7:32 pm on April 23, 2018: member
    NACK 4aaa5ba - this code isn’t redundant, the out args are used by caller CWallet::CreateTransaction
  6. Empact commented at 8:36 pm on April 23, 2018: member
    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?
  7. instagibbs commented at 8:44 pm on April 23, 2018: member

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

    On Mon, Apr 23, 2018, 4:38 PM Ben Woosley notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/13057#issuecomment-383714412, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC061sK4esuliPqtBCPdCuCC-mFlJ3ks5trjuzgaJpZM4Tf7aM .

  8. laanwj commented at 5:57 am on April 24, 2018: member

    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.

  9. instagibbs force-pushed on Apr 24, 2018
  10. instagibbs force-pushed on Apr 24, 2018
  11. instagibbs renamed this:
    remove redundant pre-selected coin code
    refactor pre-selected coin code
    on Apr 24, 2018
  12. instagibbs force-pushed on Apr 24, 2018
  13. instagibbs commented at 2:56 pm on April 24, 2018: member
    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
  14. in src/wallet/test/coinselector_tests.cpp:259 in 358c298ff8 outdated 
    254+    BOOST_CHECK(!coin_selection_params_bnb.use_bnb);
255+    std::vector<COutPoint> coin_list;
256+    coin_control2.ListSelected(coin_list);
257+    BOOST_CHECK(setCoinsRet.size() == coin_list.size());
258+    // "over-selected" since these were mandatory
259+    BOOST_CHECK(nValueRet == (5+6+11) * CENT);
    Empact commented at 7:42 am on April 26, 2018:
    BOOST_CHECK_EQUAL here and above?
  15. in src/wallet/wallet.cpp:2515 in 358c298ff8 outdated 
    2550+
2551     // remove preset inputs from vCoins
2552-    for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();)
2553-    {
2554-        if (setPresetCoins.count(CInputCoin(it->tx->tx, it->i)))
2555+    for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();) {
    Empact commented at 8:09 am on April 26, 2018:
    nit: this could be a const_iterator
    instagibbs commented at 1:48 pm on April 26, 2018:
    no, it’s deleting entries?
    Empact commented at 2:49 pm on April 26, 2018:
    The vector is deleting entries, the COutputs themselves are unmodified.
    instagibbs commented at 3:15 pm on April 26, 2018:
    ah, I assume const_iterator was const for everything, including the underlying vector. TIL
  16. in src/wallet/wallet.cpp:2563 in 358c298ff8 outdated 
    2570-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2571-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, nMaxChainLength/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2572-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2573-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2574-        (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));
2575+    CAmount amount_to_select = nTargetValue - nValueFromPresetInputs;
    Empact commented at 8:09 am on April 26, 2018:
    nit: const
  17. instagibbs force-pushed on Apr 26, 2018
  18. instagibbs force-pushed on Apr 26, 2018
  19. instagibbs force-pushed on Apr 26, 2018
  20. DrahtBot added the label Needs rebase on Jun 11, 2018
  21. MarcoFalke removed the label Needs rebase on Jun 11, 2018
  22. DrahtBot added the label Needs rebase on Jun 11, 2018
  23. instagibbs force-pushed on Jun 11, 2018
  24. DrahtBot removed the label Needs rebase on Jun 11, 2018
  25. instagibbs force-pushed on Jun 11, 2018
  26. instagibbs force-pushed on Jun 11, 2018
  27. instagibbs force-pushed on Jun 11, 2018
  28. instagibbs commented at 8:15 pm on June 11, 2018: member
    rebased, and did a tiny bit more refactoring for more readability.
  29. Empact commented at 0:10 am on June 12, 2018: member
    You have some whitespace inconsistencies, clang-format would be good.
  30. in src/wallet/wallet.cpp:2586 in c25c442db1 outdated 
    2593-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2594-        (m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2595-        (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)) ||
2596-        (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)) ||
2597-        (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));
2598+    const CAmount amount_to_select = nTargetValue - nValueFromPresetInputs;
    Empact commented at 0:12 am on June 12, 2018:
    This loses the early return if nTargetValue <= nValueFromPresetInputs
    Empact commented at 0:29 am on June 12, 2018:
    Ah, thanks, in that case how about moving this assignment up to tie the two together?
    instagibbs commented at 0:45 am on June 12, 2018:
    done
  31. in src/wallet/wallet.cpp:2597 in c25c442db1 outdated 
    2604+             res = SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2605+            SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2606+            SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2607+            SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
2608+            (!fRejectLongChains && SelectCoinsMinConf(amount_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
2609+    }
    Empact commented at 0:14 am on June 12, 2018:
    nit: I like not having repeated tests of m_spend_zero_conf_change, but I would keep it as a single assignment.
    instagibbs commented at 0:20 am on June 12, 2018:
    done
  32. instagibbs force-pushed on Jun 12, 2018
  33. instagibbs force-pushed on Jun 12, 2018
  34. instagibbs force-pushed on Jun 12, 2018
  35. DrahtBot added the label Needs rebase on Jul 24, 2018
  36. refactor pre-selected coin code, add basic unit test case 4f0111ecd6
  37. instagibbs force-pushed on Jul 25, 2018
  38. instagibbs commented at 2:42 pm on July 25, 2018: member
    rebased
  39. DrahtBot removed the label Needs rebase on Jul 25, 2018
  40. DrahtBot commented at 10:59 pm on March 13, 2019: member

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

    Conflicts

    No conflicts as of last run.

  41. DrahtBot commented at 7:11 pm on April 28, 2019: member
  42. DrahtBot closed this on Apr 28, 2019
  43. DrahtBot reopened this on Apr 28, 2019
  44. Empact commented at 11:57 am on April 29, 2019: member
    utACK https://github.com/bitcoin/bitcoin/pull/13057/commits/4f0111ecd6a9521da68b7597c73ffd7aa8bef98d but this would be easier to review if it focused around the fix e.g.: https://github.com/Empact/bitcoin/commit/3bcd12d21f9198541a74422d186cf7cd2cefad49
  45. instagibbs closed this on Jul 8, 2019
  46. laanwj commented at 6:54 pm on July 8, 2019: member
    Any specific reason for closing this?
  47. instagibbs commented at 7:02 pm on July 8, 2019: member
    No particular reason. Just something I didn’t feel like pursuing review on.
  48. MarcoFalke locked this on Dec 16, 2021


instagibbs laanwj Empact DrahtBot

Labels
Refactoring Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me