Replace coin selection fallback strategy with Single Random Draw #13307

pull achow101 wants to merge 11 commits into bitcoin:master from achow101:srd-fallback changing 14 files +322 −754
  1. achow101 commented at 3:24 AM on May 23, 2018: member

    When BnB selection fails to find an exact match, we fall back to the original Bitcoin Core algorithm. This PR replaces that fallback with the Single Random Draw strategy. Additionally, because SRD uses effective values, the previous fee increasing loop thing is now removed and effective value is used everywhere.

    Some tests may fail spuriously because they may rely on semi-deterministic behavior from the original coin selection algorithm. I have been able to fix the ones that fail the most, but others may still have issues. Please let me know if you see any tests fail for coin selection reasons (e.g. insufficient funds, missing inputs, etc.) and I will try to fix them.

    I will be working on doing simulations this week and will post the data once I finish them.

  2. achow101 commented at 3:24 AM on May 23, 2018: member

    cc @Xekyo @instagibbs

  3. fanquake added the label Wallet on May 23, 2018
  4. achow101 force-pushed on May 23, 2018
  5. in src/wallet/coinselection.cpp:320 in 7fd7c1b93c outdated 
     315 | +        value_ret += utxo.txout.nValue;
 316 | +        out_set.insert(utxo);
 317 | +
 318 | +        // We have enough coins, stop selecting
 319 | +        if (curr_value >= target_value + not_input_fees) {
 320 | +            break;
    Empact commented at 10:30 AM on May 23, 2018:

    Could return true here, return false if the loop exhausts, to avoid duplicating the condition.

    achow101 commented at 4:56 PM on May 23, 2018:

    Done

  6. in src/wallet/wallet.cpp:2466 in a603558b10 outdated 
    2512 | @@ -2513,26 +2513,21 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
2513 |  
2514 |      // Start with BnB. If that fails, use SRD
2515 |      if (SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees)) {
2516 | -        bnb_used = true;
2517 |          return true;
2518 |      } else {
2519 | -        bnb_used = false;
2520 |          return SingleRandomDraw(nTargetValue, utxo_pool, setCoinsRet, nValueRet, not_input_fees);
    Empact commented at 10:36 AM on May 23, 2018:

    Could collapse this to boolean fallback at this point.

    achow101 commented at 4:57 PM on May 23, 2018:

    Done

  7. in src/wallet/wallet.h:856 in 35830934fd outdated 
     848 | @@ -849,8 +849,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 849 |       * completion the coin set and corresponding actual target value is
 850 |       * assembled
 851 |       */
 852 | -    bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> vCoins,
 853 | -        std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const;
 854 | +    bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibilty_filter, std::vector<COutput> vCoins,
    Empact commented at 10:56 AM on May 23, 2018:

    eligibilty_filter typo

    achow101 commented at 4:57 PM on May 23, 2018:

    Fixed

  8. achow101 force-pushed on May 23, 2018
  9. achow101 force-pushed on May 23, 2018
  10. in src/wallet/coinselection.cpp:305 in f03ae69ed5 outdated 
     297 | @@ -298,3 +298,27 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<CInputCoin>& vCoins
 298 |  
 299 |      return true;
 300 |  }
 301 | +
 302 | +/*
 303 | + * Randomly selects coins until the target value is exceeded. Uses effective values
 304 | + */
 305 | +bool SingleRandomDraw(const CAmount& target_value, std::vector<CInputCoin>& utxo_pool, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees)
    instagibbs commented at 10:20 PM on June 4, 2018:

    non_input_fees? It's not "not fees", rather "non-input". Easier for me to read.

    achow101 commented at 5:31 AM on June 5, 2018:

    Done

  11. in src/wallet/wallet.cpp:2513 in 1ed31565b3 outdated 
    2508 | @@ -2509,9 +2509,10 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
2509 |      // Calculate the fees for things that aren't inputs
2510 |      CAmount not_input_fees = coin_selection_params.effective_fee.GetFee(coin_selection_params.tx_noinputs_size);
2511 |  
2512 | -    if (coin_selection_params.use_bnb) {
2513 | +    // Start with BnB. If that fails, use SRD
2514 | +    if (SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees)) {
    instagibbs commented at 10:29 PM on June 4, 2018:

    I think this new logic means that non-BnB will be tried more often? Instead of trying all variants of BnB(6 confirms, 1 confirm, small chain, etc), we seem to be trying 6 confirms for BnB, then 6 confirms for non-BnB

    achow101 commented at 5:32 AM on June 5, 2018:

    Hmm, yes, that appears to be what is happening. I guess this depends on whether we want to select an exact match for coins with less confirmatoins, or whether we want confirmations over exact match.

    sipa commented at 6:03 PM on June 9, 2018:

    This is a good point. Perhaps a solution is to add a use_bnb argument to SelectCoinsMinConf (but not as part CoinSelectionParams), and then have a sequence of attempts with and without use_bnb in SelectCoins?

    instagibbs commented at 7:32 PM on June 14, 2018:

    I prefer the behavior in master due to privacy reasons of change-less transactions.

    achow101 commented at 1:52 AM on June 16, 2018:

    I have implemented this change.

  12. in src/wallet/wallet.cpp:2892 in f84301bac5 outdated 
    2886 | @@ -2916,6 +2887,40 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2887 |              for (const auto& coin : setCoins)
2888 |                  txNew.vin.push_back(CTxIn(coin.outpoint,CScript(),
2889 |                                            nSequence));
2890 | +
2891 | +            // Fill vout
2892 | +            bool fFirst = true;
    instagibbs commented at 10:46 PM on June 4, 2018:

    maybe rename to first_output

    achow101 commented at 5:32 AM on June 5, 2018:

    Done

  13. in src/wallet/wallet.cpp:2502 in 77f7a98050 outdated 
    2498 | @@ -2498,7 +2499,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
2499 |              continue;
2500 |  
2501 |          CInputCoin coin(output.tx->tx, output.i);
2502 | -        coin.effective_value = coin.txout.nValue - (output.nInputBytes < 0 ? 0 : coin_selection_params.effective_fee.GetFee(output.nInputBytes));
2503 | +        coin.effective_value = coin.txout.nValue - (output.nInputBytes < 0 || !use_effective ? 0 : coin_selection_params.effective_fee.GetFee(output.nInputBytes));
    instagibbs commented at 10:58 PM on June 4, 2018:

    parenthesis please

    achow101 commented at 5:32 AM on June 5, 2018:

    Done

  14. instagibbs commented at 11:02 PM on June 4, 2018: member

    "Calculate the transaction fees" <--- this commit probably needs more description, not sure what it's doing.

  15. instagibbs commented at 11:02 PM on June 4, 2018: member

    Some thoughts.

  16. achow101 force-pushed on Jun 5, 2018
  17. achow101 commented at 5:33 AM on June 5, 2018: member

    "Calculate the transaction fees" <--- this commit probably needs more description, not sure what it's doing.

    Reworded, hopefully it's better.

  18. Empact commented at 6:19 AM on June 8, 2018: member

    nit: Somewhat better to revert the merge commit 9552dfb rather than the 2 pr commits, to be explicit that the whole PR was reverted (and which one).

  19. in src/wallet/coinselection.cpp:169 in 50000b1239 outdated 
     211 | -}
 212 | -
 213 | -bool KnapsackSolver(const CAmount& nTargetValue, std::vector<CInputCoin>& vCoins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet)
 214 | +/*
 215 | + * Randomly selects coins until the target value is exceeded. Uses effective values
 216 | + */
    Empact commented at 6:19 AM on June 8, 2018:

    nit: Better to document in the header, and use doxygen comment /**

    achow101 commented at 11:17 PM on June 8, 2018:

    Done

  20. in src/wallet/test/coinselector_tests.cpp:253 in 50000b1239 outdated 
     248 | @@ -554,13 +249,11 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
 249 |          CAmount target = rand.randrange(balance - 1000) + 1000;
 250 |  
 251 |          // Perform selection
 252 | -        CoinSelectionParams coin_selection_params_knapsack(false, 34, 148, CFeeRate(0), 0);
 253 | -        CoinSelectionParams coin_selection_params_bnb(true, 34, 148, CFeeRate(0), 0);
 254 | +
 255 | +        CoinSelectionParams coin_selection_params_bnb(34, 148, CFeeRate(0), 0);
    Empact commented at 6:20 AM on June 8, 2018:

    _bnb probably no longer relevant

    achow101 commented at 11:17 PM on June 8, 2018:

    Removed here and elsewhere

  21. in src/wallet/test/coinselector_tests.cpp:217 in 50000b1239 outdated 
     213 | @@ -216,316 +214,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 214 |      }
 215 |  
 216 |      // Make sure that effective value is working in SelectCoinsMinConf when BnB is used
 217 | -    CoinSelectionParams coin_selection_params_bnb(true, 0, 0, CFeeRate(3000), 0);
 218 | +    CoinSelectionParams coin_selection_params_bnb(0, 0, CFeeRate(3000), 0);
    Empact commented at 6:21 AM on June 8, 2018:

    Same

  22. in src/wallet/wallet.cpp:2536 in 35ce099536 outdated 
    2528 | @@ -2529,14 +2529,18 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2529 |              if (!out.fSpendable)
2530 |                   continue;
2531 |              nValueRet += out.tx->tx->vout[out.i].nValue;
2532 | -            setCoinsRet.insert(CInputCoin(out.tx->tx, out.i));
2533 | +            CInputCoin coin(out.tx->tx, out.i);
2534 | +            coin.effective_value = coin.txout.nValue - (out.nInputBytes < 0 || !use_effective ? 0 : coin_selection_params.effective_fee.GetFee(out.nInputBytes));
    Empact commented at 6:29 AM on June 8, 2018:

    nit: explicit precedence would be nice here: (out.nInputBytes < 0 || !use_effective)

    achow101 commented at 11:17 PM on June 8, 2018:

    I thought I did this earlier. Must have gotten lost in a rebase. Fixed.

    Empact commented at 9:58 PM on June 14, 2018:

    Doesn't look changed fyi.

    achow101 commented at 10:22 PM on June 14, 2018:

    How about now?

  23. in src/wallet/wallet.cpp:2572 in 35ce099536 outdated 
    2572 | +            nValueFromPresetInputs += preset_it->txout.nValue;
2573 | +            effective_value_from_presets += preset_it->effective_value;
2574 |              it = vCoins.erase(it);
2575 | -        else
2576 | +        }
2577 | +        else {
    Empact commented at 6:30 AM on June 8, 2018:

    nit: whitespace

    achow101 commented at 11:17 PM on June 8, 2018:

    Done

  24. in src/wallet/coinselection.h:35 in 50000b1239 outdated 
      29 | @@ -30,9 +30,9 @@ class CInputCoin {
  30 |  
  31 |      COutPoint outpoint;
  32 |      CTxOut txout;
  33 | -    CAmount effective_value;
  34 | -    CAmount fee = 0;
  35 | -    CAmount long_term_fee = 0;
  36 | +    mutable CAmount effective_value;
  37 | +    mutable CAmount fee = 0;
  38 | +    mutable CAmount long_term_fee = 0;
    Empact commented at 6:55 AM on June 8, 2018:

    nit: Better to squash this change into the commit that calls for it

    achow101 commented at 11:17 PM on June 8, 2018:

    Done

  25. achow101 force-pushed on Jun 8, 2018
  26. achow101 commented at 11:18 PM on June 8, 2018: member

    nit: Somewhat better to revert the merge commit 9552dfb rather than the 2 pr commits, to be explicit that the whole PR was reverted (and which one).

    Done

  27. in src/wallet/coinselection.cpp:304 in aea9842729 outdated 
     297 | @@ -298,3 +298,24 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<CInputCoin>& vCoins
 298 |  
 299 |      return true;
 300 |  }
 301 | +
 302 | +bool SingleRandomDraw(const CAmount& target_value, std::vector<CInputCoin>& utxo_pool, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount non_input_fees)
 303 | +{
 304 | +    random_shuffle(utxo_pool.begin(), utxo_pool.end(), GetRandInt);
    sipa commented at 5:49 PM on June 9, 2018:

    Two comments:

    • Requesting new entropy for each element being sorted may be slow (each call to GetRandInt invokes OpenSSL for randomness) (this also applies to the code you copied, but you're going to remove that anyway).
    • It's overkill to permute all elements, as you're likely only going to use a few.

    For the first, create a FastRandomContext and use that in std::shuffle (rather than random_shuffle):

    std::shuffle(utxo_pool.begin(), utxo_pool.end(), FastRandomContext());`

    For the second, you can permute on the fly; for example:

    FastRandomContext ctx;
for (size_t i = 0; i < utxo_pool.size; ++i) {
    size_t pos = i + ctx.randrange(utxo_pool.size() - i); // randomly pick one of the remaining elements
    std::swap(utxo_pool[i], utxo_pool[pos]);
    const CInputCoin& utxo = utxo_pool[i];
    ...
}
    achow101 commented at 10:01 PM on June 9, 2018:

    Done

  28. in src/wallet/wallet.cpp:2912 in 1a89ac2d03 outdated 
    2922 | -            for (const auto& coin : setCoins)
2923 | -                txNew.vin.push_back(CTxIn(coin.outpoint,CScript(),
2924 | -                                          nSequence));
2925 | +                if (nSubtractFeeFromAmount != 0) {
2926 | +                    // If we are subtracting the fee from the amount, nChange cannot be less than 0 because nFeeRet is not involved.
2927 | +                    // We need to decrase nFeeRet by the change output fee so that we do not overpay.
    sipa commented at 6:07 PM on June 9, 2018:

    Typo: decrase

    achow101 commented at 10:01 PM on June 9, 2018:

    Fixed

  29. achow101 force-pushed on Jun 9, 2018
  30. achow101 force-pushed on Jun 9, 2018
  31. in src/wallet/coinselection.cpp:171 in 05dfc65a8a outdated 
     215 | -{
 216 | -    setCoinsRet.clear();
 217 | -    nValueRet = 0;
 218 | +    FastRandomContext random_ctx;
 219 | +    CAmount curr_value = 0;
 220 | +    for (size_t i = 0; i < utxo_pool.size; ++i) {
    sipa commented at 11:57 PM on June 9, 2018:

    Travis fails because of size (instead of size()).

    achow101 commented at 12:16 AM on June 10, 2018:

    Oops.

  32. achow101 force-pushed on Jun 10, 2018
  33. achow101 commented at 2:04 AM on June 10, 2018: member

    I'm not quite sure what is causing the travis failure.

  34. DrahtBot added the label Needs rebase on Jun 11, 2018
  35. MarcoFalke removed the label Needs rebase on Jun 11, 2018
  36. DrahtBot added the label Needs rebase on Jun 11, 2018
  37. achow101 force-pushed on Jun 11, 2018
  38. achow101 commented at 7:02 PM on June 11, 2018: member

    Rebased

  39. achow101 force-pushed on Jun 11, 2018
  40. DrahtBot removed the label Needs rebase on Jun 11, 2018
  41. in src/wallet/coinselection.cpp:187 in 4c55a09769 outdated 
     315 | +        curr_value += utxo.effective_value;
 316 | +        value_ret += utxo.txout.nValue;
 317 | +        out_set.insert(utxo);
 318 | +
 319 | +        // We have enough coins, stop selecting
 320 | +        if (curr_value >= target_value + non_input_fees) {
    Xekyo commented at 3:33 PM on June 12, 2018:

    What you're calling target_value here is not actually the value you're targeting. How about recipient_amount or payload? Or add the payload, output_fees and tx_overhead_fees into a selection_target per selection style to differentiate between change/no-change?

    achow101 commented at 7:48 PM on June 18, 2018:

    I renamed it to recipient_amount. I don't think there needs to be any differentiation between change and no change.

  42. achow101 force-pushed on Jun 12, 2018
  43. achow101 commented at 7:01 PM on June 12, 2018: member

    I fixed the test failure.

  44. achow101 force-pushed on Jun 13, 2018
  45. achow101 force-pushed on Jun 14, 2018
  46. achow101 force-pushed on Jun 16, 2018
  47. Xekyo commented at 1:04 AM on June 18, 2018: member

    There are a few commits in your PR that obsolete previous changes, I only realized that after going through them commit by commit. You may want to rebase differently to tidy that up.

  48. in src/wallet/coinselection.cpp:167 in a8bca48038 outdated 
     163 | @@ -164,137 +164,29 @@ bool SelectCoinsBnB(std::vector<CInputCoin>& utxo_pool, const CAmount& target_va
 164 |      return true;
 165 |  }
 166 |  
 167 | -static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
 168 | -                                  std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
 169 | +bool SingleRandomDraw(const CAmount& target_value, std::vector<CInputCoin>& utxo_pool, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount non_input_fees)
    Xekyo commented at 1:07 AM on June 18, 2018:

    Theoretically, SRD could also produce a result without a change. You could fold the non_input_fees into the target_value, and if you don't find a perfect match, select more until you have minChange + fee for one output more than target.

    achow101 commented at 7:29 PM on June 18, 2018:

    I don't think that this distinction matters. If SRD produces a result without change, the rest of the code will treat it as if BnB had been used, there is no difference for change or no change. I don't think it is necessary to target a min change or include the change output's fees in the target.

  49. in src/wallet/wallet.h:855 in a8bca48038 outdated 
     851 | @@ -852,7 +852,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 852 |       * assembled
 853 |       */
 854 |      bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> vCoins,
 855 | -        std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const;
 856 | +        std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool use_effecitve, bool use_bnb) const;
    Xekyo commented at 1:09 AM on June 18, 2018:

    use_effecitve has a typo.

    achow101 commented at 7:48 PM on June 18, 2018:

    Fixed

  50. in test/functional/rpc_fundrawtransaction.py:360 in a8bca48038 outdated 
     358 | +        mempool_tx = self.nodes[0].getrawmempool(True)[txId]
 359 |  
 360 | -        #compare fee
 361 | -        feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee)
 362 | +        #compare feerates
 363 | +        feeDelta = (Decimal(fundedTx['fee']) / Decimal(len(fundedTx['hex']) / 2)) - (Decimal(mempool_tx['fee']) / mempool_tx['size'])
    Xekyo commented at 1:10 AM on June 18, 2018:

    Shouldn't this be segwit sensitive? Hex length is only a valid indicator of weight for non-segwit transactions. I guess this test only has legacy transactions?

    Since you use the same calculation multiple times, you might want to make a helper function that only takes fundedTx and mempool_tx as params and returns the feeDelta.

    achow101 commented at 7:36 PM on June 18, 2018:

    The test only uses legacy addresses. I don't think it is necessary to make a helper function for this.

  51. achow101 force-pushed on Jun 18, 2018
  52. achow101 commented at 7:49 PM on June 18, 2018: member

    I squashed down a few commits, hopefully that will make the code easier to review.

  53. achow101 commented at 6:18 PM on June 20, 2018: member

    I have done some simulations of this strategy along with a few other strategies of different effective value filtering. Results here: https://gist.github.com/achow101/242470486265d3f21adab08f65b9102c

  54. instagibbs commented at 6:25 PM on June 20, 2018: member

    Takeaway from the simulations is that filtering out negative effective values seemingly does near-best in utxo size and the best in fees overall. Latter point is unexpected.

    On Wed, Jun 20, 2018 at 2:20 PM Andrew Chow notifications@github.com wrote:

    I have done some simulations of this strategy along with a few other strategies of different effective value filtering. Results here: https://gist.github.com/achow101/242470486265d3f21adab08f65b9102c

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13307#issuecomment-398847615, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC019Y7wp_GYM3LgpqSHwBROiLZvTFks5t-pJ4gaJpZM4UJteu .

  55. achow101 commented at 6:29 PM on June 20, 2018: member

    The main issue here is the minimum change. SRD currently does not target a minimum change which can result in very small, near dust, change outputs. So the question is really what we should target as min change.

  56. DrahtBot added the label Needs rebase on Jun 30, 2018
  57. achow101 commented at 7:44 PM on July 3, 2018: member

    Rebased

  58. achow101 force-pushed on Jul 3, 2018
  59. sipa commented at 11:03 PM on July 3, 2018: member

    As suggested on https://iohk.io/blog/self-organisation-in-coin-selection/ it may be worth it to aim for change up to twice the amount we're trying to send, to avoid grinding down change too much.

    Of course, that doesn't work whenever the total (effective) amount in the wallet is less than twice the amount we're trying to send. A reasonably simple function to reconcile the two is C = P*(1 - P/B), where P is the amount we're sending, B is the total effective balance, and C is the maximum change size we're targetting. For small P/B, this will result in change close to P, but never aim for more than we have.

  60. achow101 commented at 2:32 AM on July 6, 2018: member

    Using @sipa's function, I have found though simulations that the smallest change created is much larger than what simple SRD produces. However the problem with a higher mean number of UTXOs persists and is slightly worse with this modification. But fees are still the same, and we are unlikely to be grinding things into dust. The question then is are we okay with having more UTXOs or should we attempt to consolidate as much as currently done.

    Part of the difference is due to the fact that currently we spend dust outputs, but simulations show that even this does not entirely account for the higher number of UTXOs.

  61. achow101 force-pushed on Jul 7, 2018
  62. achow101 commented at 12:38 AM on July 7, 2018: member

    Fixed the test failure.

  63. DrahtBot removed the label Needs rebase on Jul 7, 2018
  64. DrahtBot added the label Needs rebase on Jul 24, 2018
  65. Revert "Merge #12694: Actually disable BnB when there are preset inputs"
    This reverts commit 9552dfb1f6326b97ae05efb8af503eb7776a2d59, reversing
changes made to f686002a8eba820a40ac2f34a6e8f57b2b5cc54c.
    2f4ab05c5a
  66. Implement the SingleRandomDraw coin selection method
    SingleRandomDraw randomly chooses UTXOs until there is sufficient
effective value.
    65b535b862
  67. Replace the coin selection fallback with SRD
    Replaces the knapsack solver fallback with the SRD fallback. Also
changes SelectCoinsMinConf to use exclusively effective value to
work with BnB and SRD
    ffd71565b3
  68. Remove KnapsackSolver and related tests 02b288f91f
  69. Have SelectCoinsMinConf figure out when to use BnB or SRD
    Instead of the caller telling SelectCoinsMinConf when to use BnB
or the fallback, have SelectCoinsMinConf do it itself. This removes
the use_bnb field of coin_selection_params
    33737a5a70
  70. Remove the CreateTransaction loop and fee checking things
    We are now using effective value for coin selection, so there is no
need for the loop or any of the fee checking things that were done
within it. All fees ard handled by the effective value selection

Move vout filling to after coins are selected so that the transaction
fee is actually known to handle when users want to subtract the fee
from the outputs.
    c1c24d60bc
  71. Don't use effective values when we are subtracting the fee from the amount ae9a8e19f2
  72. Remove bnb_used
    bnb_used is no longer necessary since we account for fees when
calculating how much change there is
    8ecb6340d6
  73. handle preset inputs
    For preset inputs, use their effective values
    6989b9df81
  74. Split change output creation from fee calculation for change e3903f3aa5
  75. Make tests work with non-deterministic coin selection
    Fix tests that relied on non-deterministic coin selection to be able
to handle the random coin selection.
    339d977fbd
  76. achow101 force-pushed on Aug 17, 2018
  77. achow101 commented at 9:15 PM on August 17, 2018: member

    Rebased

  78. DrahtBot removed the label Needs rebase on Aug 17, 2018
  79. DrahtBot commented at 6:53 PM on September 7, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #14156 ([WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion. by lucash-dev)
    • #13558 (Drop unused GetType() from CSizeComputer by Empact)
    • #13546 (wallet: Avoid potential use of uninitialized value bnb_used in CWallet::CreateTransaction(...) by practicalswift)
    • #13419 ([tests] Speed up knapsack_solver_test by not recreating wallet 100 times. by lucash-dev)
    • #13057 (refactor pre-selected coin code by instagibbs)
    • #11652 (Add missing locks: validation.cpp + related by practicalswift)
    • #11634 (wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift)
    • #10973 (Refactor: separate wallet from node by ryanofsky)
    • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  80. DrahtBot added the label Needs rebase on Sep 11, 2018
  81. DrahtBot commented at 8:38 AM on September 11, 2018: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  82. DrahtBot commented at 7:23 PM on December 12, 2018: member

    <!--5fd3d806e98f4a0ca80977bb178665a0-->There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

  83. DrahtBot added the label Up for grabs on Dec 12, 2018
  84. DrahtBot closed this on Dec 12, 2018
  85. laanwj removed the label Needs rebase on Oct 24, 2019
  86. MarcoFalke locked this on Dec 16, 2021
  87. MarcoFalke removed the label Up for grabs on Mar 22, 2022
Contributors
achow101EmpactinstagibbssipaXekyoDrahtBot
Labels
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: 2026-05-02 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me