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 12: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 12:12 AM on June 12, 2018:

    This loses the early return if nTargetValue <= nValueFromPresetInputs

    Empact commented at 12: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 12: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 12: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 12: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

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  41. DrahtBot commented at 7:11 PM on April 28, 2019: member

    <!--5d09a71f8925f3f132321140b44b946d-->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.

  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
Contributors
instagibbslaanwjEmpactDrahtBot
Labels
RefactoringWallet
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: 2026-04-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me