From 2acad036575ec998f8bbe4f10f6206b1c8ad3d23 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 15:10:49 -0400 Subject: [PATCH 1/7] Remove OutputGroup non-default constructors --- src/bench/coin_selection.cpp | 6 ++++-- src/wallet/coinselection.h | 11 ----------- src/wallet/test/coinselector_tests.cpp | 10 ++++++++-- src/wallet/wallet.cpp | 3 ++- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 99aafd8dfc7..a5455bc7a16 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -46,7 +46,8 @@ static void CoinSelection(benchmark::Bench& bench) std::vector groups; for (const auto& wtx : wtxs) { COutput output(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); - groups.emplace_back(output.GetInputCoin(), 6, false, 0, 0); + groups.emplace_back(); + groups.back().Insert(output.GetInputCoin(), 6, false, 0, 0); } const CoinEligibilityFilter filter_standard(1, 6, 0); @@ -75,7 +76,8 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; std::unique_ptr wtx = MakeUnique(&testWallet, MakeTransactionRef(std::move(tx))); - set.emplace_back(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0); + set.emplace_back(); + set.back().Insert(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0); wtxn.emplace_back(std::move(wtx)); } // Copied from src/wallet/test/coinselector_tests.cpp diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 49c1134ec62..1a0373eba18 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -81,17 +81,6 @@ struct OutputGroup CAmount long_term_fee{0}; OutputGroup() {} - OutputGroup(std::vector&& outputs, bool from_me, CAmount value, int depth, size_t ancestors, size_t descendants) - : m_outputs(std::move(outputs)) - , m_from_me(from_me) - , m_value(value) - , m_depth(depth) - , m_ancestors(ancestors) - , m_descendants(descendants) - {} - OutputGroup(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) : OutputGroup() { - Insert(output, depth, from_me, ancestors, descendants); - } void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants); std::vector::iterator Discard(const CInputCoin& output); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index f38ccba3844..72430620f1d 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -114,7 +114,10 @@ inline std::vector& GroupCoins(const std::vector& coins { static std::vector static_groups; static_groups.clear(); - for (auto& coin : coins) static_groups.emplace_back(coin, 0, true, 0, 0); + for (auto& coin : coins) { + static_groups.emplace_back(); + static_groups.back().Insert(coin, 0, true, 0, 0); + } return static_groups; } @@ -122,7 +125,10 @@ inline std::vector& GroupCoins(const std::vector& coins) { static std::vector static_groups; static_groups.clear(); - for (auto& coin : coins) static_groups.emplace_back(coin.GetInputCoin(), coin.nDepth, coin.tx->m_amounts[CWalletTx::DEBIT].m_cached[ISMINE_SPENDABLE] && coin.tx->m_amounts[CWalletTx::DEBIT].m_value[ISMINE_SPENDABLE] == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0); + for (auto& coin : coins) { + static_groups.emplace_back(); + static_groups.back().Insert(coin.GetInputCoin(), coin.nDepth, coin.tx->m_amounts[CWalletTx::DEBIT].m_cached[ISMINE_SPENDABLE] && coin.tx->m_amounts[CWalletTx::DEBIT].m_value[ISMINE_SPENDABLE] == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0); + } return static_groups; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6f320096ebe..8d5e7546ec7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4233,7 +4233,8 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } else { - groups.emplace_back(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + groups.emplace_back(); + groups.back().Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } } From 6148a8acda5e594bb9b3b2d989056f9e03ddbdbd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 15:30:51 -0400 Subject: [PATCH 2/7] Move GroupOutputs into SelectCoinsMinConf --- src/bench/coin_selection.cpp | 12 ++-- src/wallet/test/coinselector_tests.cpp | 92 +++++++++++++------------- src/wallet/wallet.cpp | 23 ++++--- src/wallet/wallet.h | 12 +++- 4 files changed, 74 insertions(+), 65 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index a5455bc7a16..6cb45ec7a5c 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -42,21 +42,19 @@ static void CoinSelection(benchmark::Bench& bench) } addCoin(3 * COIN, wallet, wtxs); - // Create groups - std::vector groups; + // Create coins + std::vector coins; for (const auto& wtx : wtxs) { - COutput output(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); - groups.emplace_back(); - groups.back().Insert(output.GetInputCoin(), 6, false, 0, 0); + coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); } const CoinEligibilityFilter filter_standard(1, 6, 0); - const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0); + const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0, false); bench.run([&] { std::set setCoinsRet; CAmount nValueRet; bool bnb_used; - bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used); + bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used); assert(success); assert(nValueRet == 1003 * COIN); assert(setCoinsRet.size() == 2); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 72430620f1d..9af6ddd7cfc 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -35,7 +35,7 @@ static CAmount balance = 0; CoinEligibilityFilter filter_standard(1, 6, 0); CoinEligibilityFilter filter_confirmed(1, 1, 0); CoinEligibilityFilter filter_standard_extra(6, 6, 0); -CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), 0); +CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), 0, false); static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { @@ -268,22 +268,22 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // Make sure that effective value is working in SelectCoinsMinConf when BnB is used - CoinSelectionParams coin_selection_params_bnb(true, 0, 0, CFeeRate(3000), 0); + CoinSelectionParams coin_selection_params_bnb(true, 0, 0, CFeeRate(3000), 0, false); CoinSet setCoinsRet; CAmount nValueRet; bool bnb_used; empty_wallet(); add_coin(1); vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); // Test fees subtracted from output: empty_wallet(); add_coin(1 * CENT); vCoins.at(0).nInputBytes = 40; - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // Make sure that can use BnB when there are preset inputs @@ -322,24 +322,24 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) empty_wallet(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); add_coin(1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // but we can find a new 1 cent - BOOST_CHECK( testWallet.SelectCoinsMinConf( 1 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf( 1 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); add_coin(2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 3 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf( 3 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // we can make 3 cents of new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf( 3 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf( 3 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); add_coin(5*CENT); // add a mature 5 cent coin, @@ -349,33 +349,33 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard_extra, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard_extra, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK( testWallet.SelectCoinsMinConf(37 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(37 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf(38 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(38 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(34 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(34 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK( testWallet.SelectCoinsMinConf( 7 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf( 7 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK( testWallet.SelectCoinsMinConf( 8 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf( 8 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK(nValueRet == 8 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK( testWallet.SelectCoinsMinConf( 9 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf( 9 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -389,30 +389,30 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK( testWallet.SelectCoinsMinConf(71 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(!testWallet.SelectCoinsMinConf(72 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(71 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!testWallet.SelectCoinsMinConf(72 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); add_coin( 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); add_coin( 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK( testWallet.SelectCoinsMinConf(11 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(11 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -421,11 +421,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin( 2*COIN); add_coin( 3*COIN); add_coin( 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK( testWallet.SelectCoinsMinConf(95 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(95 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - BOOST_CHECK( testWallet.SelectCoinsMinConf(195 * CENT, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(195 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -440,14 +440,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: @@ -455,7 +455,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see http://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) @@ -464,7 +464,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(50000 * COIN); - BOOST_CHECK( testWallet.SelectCoinsMinConf(500000 * COIN, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(500000 * COIN, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins @@ -477,7 +477,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 7 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -487,7 +487,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 8 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 @@ -498,12 +498,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -517,7 +517,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(testWallet.SelectCoinsMinConf(2000, filter_confirmed, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(2000, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: @@ -543,8 +543,8 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int i = 0; i < RUN_TESTS; i++) { // picking 50 from 100 coins doesn't depend on the shuffle, // but does depend on randomness in the stochastic approximation code - BOOST_CHECK(testWallet.SelectCoinsMinConf(50 * COIN, filter_standard, GroupCoins(vCoins), setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(testWallet.SelectCoinsMinConf(50 * COIN, filter_standard, GroupCoins(vCoins), setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(50 * COIN, filter_standard, vCoins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(50 * COIN, filter_standard, vCoins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK(!equal_sets(setCoinsRet, setCoinsRet2)); int fails = 0; @@ -552,8 +552,8 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) { // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(testWallet.SelectCoinsMinConf(COIN, filter_standard, GroupCoins(vCoins), setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(testWallet.SelectCoinsMinConf(COIN, filter_standard, GroupCoins(vCoins), setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(COIN, filter_standard, vCoins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(COIN, filter_standard, vCoins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -575,8 +575,8 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) { // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(testWallet.SelectCoinsMinConf(90*CENT, filter_standard, GroupCoins(vCoins), setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(testWallet.SelectCoinsMinConf(90*CENT, filter_standard, GroupCoins(vCoins), setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(90*CENT, filter_standard, vCoins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(90*CENT, filter_standard, vCoins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -603,7 +603,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(1000 * COIN); add_coin(3 * COIN); - BOOST_CHECK(testWallet.SelectCoinsMinConf(1003 * COIN, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -638,13 +638,13 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CAmount target = rand.randrange(balance - 1000) + 1000; // Perform selection - CoinSelectionParams coin_selection_params_knapsack(false, 34, 148, CFeeRate(0), 0); - CoinSelectionParams coin_selection_params_bnb(true, 34, 148, CFeeRate(0), 0); + CoinSelectionParams coin_selection_params_knapsack(false, 34, 148, CFeeRate(0), 0, false); + CoinSelectionParams coin_selection_params_bnb(true, 34, 148, CFeeRate(0), 0, false); CoinSet out_set; CAmount out_value = 0; bool bnb_used = false; - BOOST_CHECK(testWallet.SelectCoinsMinConf(target, filter_standard, GroupCoins(vCoins), out_set, out_value, coin_selection_params_bnb, bnb_used) || - testWallet.SelectCoinsMinConf(target, filter_standard, GroupCoins(vCoins), out_set, out_value, coin_selection_params_knapsack, bnb_used)); + BOOST_CHECK(testWallet.SelectCoinsMinConf(target, filter_standard, vCoins, out_set, out_value, coin_selection_params_bnb, bnb_used) || + testWallet.SelectCoinsMinConf(target, filter_standard, vCoins, out_set, out_value, coin_selection_params_knapsack, bnb_used)); BOOST_CHECK_GE(out_value, target); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8d5e7546ec7..674849e26ae 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2367,7 +2367,7 @@ const CTxOut& CWallet::FindNonChangeParentOutput(const CTransaction& tx, int out return ptx->vout[n]; } -bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector groups, +bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const { setCoinsRet.clear(); @@ -2381,6 +2381,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil temp.m_confirm_target = 1008; CFeeRate long_term_feerate = GetMinimumFeeRate(*this, temp, &feeCalc); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); @@ -2403,6 +2405,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil bnb_used = true; return SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + // Filter by the min conf specs and add to utxo_pool for (const OutputGroup& group : groups) { if (!group.EligibleForSpending(eligibility_filter)) continue; @@ -2492,16 +2496,14 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // explicitly shuffling the outputs before processing Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext()); } - std::vector groups = GroupOutputs(vCoins, !coin_control.m_avoid_partial_spends, max_ancestors); - bool res = value_to_select <= 0 || - SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) || - SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) || - (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_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)) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset util::insert(setCoinsRet, setPresetCoins); @@ -2790,6 +2792,7 @@ bool CWallet::CreateTransactionInternal( std::vector vAvailableCoins; AvailableCoins(vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy + coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; // Create change script that will be used if we need change // TODO: pass in scriptChange instead of reservedest so diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 169f2669806..69bafd2e889 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -610,8 +610,16 @@ struct CoinSelectionParams size_t tx_noinputs_size = 0; //! Indicate that we are subtracting the fee from outputs bool m_subtract_fee_outputs = false; + bool m_avoid_partial_spends = false; - CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size) : use_bnb(use_bnb), change_output_size(change_output_size), change_spend_size(change_spend_size), effective_fee(effective_fee), tx_noinputs_size(tx_noinputs_size) {} + CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size, bool avoid_partial) : + use_bnb(use_bnb), + change_output_size(change_output_size), + change_spend_size(change_spend_size), + effective_fee(effective_fee), + tx_noinputs_size(tx_noinputs_size), + m_avoid_partial_spends(avoid_partial) + {} CoinSelectionParams() {} }; @@ -824,7 +832,7 @@ public: * completion the coin set and corresponding actual target value is * assembled */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector groups, + bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used) const; bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 99b399aba5d27476b61b4865cc39553d03965d57 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 15:56:30 -0400 Subject: [PATCH 3/7] Move fee setting of OutputGroup to Insert OutputGroup will handle the fee and effective value computations inside of Insert. It now needs to take the effective feerate and long term feerates as arguments to its constructor. --- src/wallet/coinselection.cpp | 30 ++++++++++-------------------- src/wallet/coinselection.h | 12 ++++++++---- src/wallet/wallet.cpp | 27 ++++++++++++++------------- src/wallet/wallet.h | 2 +- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 1a45a2b313f..f4843dd0cec 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -302,6 +302,16 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) { m_outputs.push_back(output); + CInputCoin& coin = m_outputs.back(); + coin.m_fee = coin.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(coin.m_input_bytes); + fee += coin.m_fee; + + coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.m_input_bytes); + long_term_fee += coin.m_long_term_fee; + + coin.effective_value = coin.txout.nValue - coin.m_fee; + effective_value += coin.effective_value; + m_from_me &= from_me; m_value += output.txout.nValue; m_depth = std::min(m_depth, depth); @@ -312,9 +322,6 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size // descendants is the count as seen from the top ancestor, not the descendants as seen from the // coin itself; thus, this value is counted as the max, not the sum m_descendants = std::max(m_descendants, descendants); - effective_value += output.effective_value; - fee += output.m_fee; - long_term_fee += output.m_long_term_fee; } std::vector::iterator OutputGroup::Discard(const CInputCoin& output) { @@ -335,23 +342,6 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f && m_descendants <= eligibility_filter.max_descendants; } -void OutputGroup::SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate) -{ - fee = 0; - long_term_fee = 0; - effective_value = 0; - for (CInputCoin& coin : m_outputs) { - coin.m_fee = coin.m_input_bytes < 0 ? 0 : effective_feerate.GetFee(coin.m_input_bytes); - fee += coin.m_fee; - - coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes); - long_term_fee += coin.m_long_term_fee; - - coin.effective_value = coin.txout.nValue - coin.m_fee; - effective_value += coin.effective_value; - } -} - OutputGroup OutputGroup::GetPositiveOnlyGroup() { OutputGroup group(*this); diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 1a0373eba18..721f1a22ebf 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -6,11 +6,10 @@ #define BITCOIN_WALLET_COINSELECTION_H #include +#include #include #include -class CFeeRate; - //! target minimum change amount static constexpr CAmount MIN_CHANGE{COIN / 100}; //! final minimum change amount after paying for fees @@ -78,15 +77,20 @@ struct OutputGroup size_t m_descendants{0}; CAmount effective_value{0}; CAmount fee{0}; + CFeeRate m_effective_feerate{0}; CAmount long_term_fee{0}; + CFeeRate m_long_term_feerate{0}; OutputGroup() {} + OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) : + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate) + {} + void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants); std::vector::iterator Discard(const CInputCoin& output); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; - //! Update the OutputGroup's fee, long_term_fee, and effective_value based on the given feerates - void SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate); OutputGroup GetPositiveOnlyGroup(); }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 674849e26ae..71db03c32b4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2381,7 +2381,14 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil temp.m_confirm_target = 1008; CFeeRate long_term_feerate = GetMinimumFeeRate(*this, temp, &feeCalc); - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + // Get the feerate for effective value. + // When subtracting the fee from the outputs, we want the effective feerate to be 0 + CFeeRate effective_feerate{0}; + if (!coin_selection_params.m_subtract_fee_outputs) { + effective_feerate = coin_selection_params.effective_fee; + } + + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate); // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); @@ -2390,13 +2397,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil for (OutputGroup& group : groups) { if (!group.EligibleForSpending(eligibility_filter)) continue; - if (coin_selection_params.m_subtract_fee_outputs) { - // Set the effective feerate to 0 as we don't want to use the effective value since the fees will be deducted from the output - group.SetFees(CFeeRate(0) /* effective_feerate */, long_term_feerate); - } else { - group.SetFees(coin_selection_params.effective_fee, long_term_feerate); - } - OutputGroup pos_group = group.GetPositiveOnlyGroup(); if (pos_group.effective_value > 0) utxo_pool.push_back(pos_group); } @@ -2405,7 +2405,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil bnb_used = true; return SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0)); // Filter by the min conf specs and add to utxo_pool for (const OutputGroup& group : groups) { @@ -4206,7 +4206,7 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const { std::vector groups; std::map gmap; std::set full_groups; @@ -4228,15 +4228,16 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu // high amount of fees. if (it->second.m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { groups.push_back(it->second); - it->second = OutputGroup{}; + it->second = OutputGroup{effective_feerate, long_term_feerate}; full_groups.insert(dst); } it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } else { - gmap[dst].Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + auto ins = gmap.emplace(dst, OutputGroup{effective_feerate, long_term_feerate}); + ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } else { - groups.emplace_back(); + groups.emplace_back(effective_feerate, long_term_feerate); groups.back().Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 69bafd2e889..448b80c7bb8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From d895e98b594b873f3d34c8ba63e9b55125d51b5a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 16:35:56 -0400 Subject: [PATCH 4/7] Move EligibleForSpending into GroupOutputs Instead of filtering after the OutputGroups have been made, do it as they are being made. --- src/wallet/wallet.cpp | 26 +++++++++++--------------- src/wallet/wallet.h | 2 +- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 71db03c32b4..f2c035da144 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2373,8 +2373,8 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil setCoinsRet.clear(); nValueRet = 0; - std::vector utxo_pool; if (coin_selection_params.use_bnb) { + std::vector utxo_pool; // Get long term estimate FeeCalculation feeCalc; CCoinControl temp; @@ -2388,15 +2388,13 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil effective_feerate = coin_selection_params.effective_fee; } - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate, eligibility_filter); // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); // Filter by the min conf specs and add to utxo_pool and calculate effective value for (OutputGroup& group : groups) { - if (!group.EligibleForSpending(eligibility_filter)) continue; - OutputGroup pos_group = group.GetPositiveOnlyGroup(); if (pos_group.effective_value > 0) utxo_pool.push_back(pos_group); } @@ -2405,15 +2403,10 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil bnb_used = true; return SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0)); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0), eligibility_filter); - // Filter by the min conf specs and add to utxo_pool - for (const OutputGroup& group : groups) { - if (!group.EligibleForSpending(eligibility_filter)) continue; - utxo_pool.push_back(group); - } bnb_used = false; - return KnapsackSolver(nTargetValue, utxo_pool, setCoinsRet, nValueRet); + return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet); } } @@ -4206,7 +4199,7 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter) const { std::vector groups; std::map gmap; std::set full_groups; @@ -4237,8 +4230,10 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); } } else { - groups.emplace_back(effective_feerate, long_term_feerate); - groups.back().Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + // This is for if each output gets it's own OutputGroup + OutputGroup coin(effective_feerate, long_term_feerate); + coin.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + if (coin.EligibleForSpending(filter)) groups.push_back(coin); } } } @@ -4249,7 +4244,8 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu // Make this unattractive as we want coin selection to avoid it if possible group.m_ancestors = max_ancestors - 1; } - groups.push_back(group); + // If the OutputGroup is not eligible, don't add it + if (group.EligibleForSpending(filter)) groups.push_back(group); } } return groups; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 448b80c7bb8..1c523d86b89 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 416d74fb1687ae1d47a58c153d09d9afe0b6dc60 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 31 Aug 2020 16:45:39 -0400 Subject: [PATCH 5/7] Move OutputGroup positive only filtering into Insert --- src/bench/coin_selection.cpp | 2 +- src/wallet/coinselection.cpp | 40 +++++++------------------- src/wallet/coinselection.h | 5 +--- src/wallet/test/coinselector_tests.cpp | 4 +-- src/wallet/wallet.cpp | 26 +++++++---------- src/wallet/wallet.h | 2 +- 6 files changed, 27 insertions(+), 52 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 6cb45ec7a5c..6af7e785de0 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -75,7 +75,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector tx.vout[nInput].nValue = nValue; std::unique_ptr wtx = MakeUnique(&testWallet, MakeTransactionRef(std::move(tx))); set.emplace_back(); - set.back().Insert(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0); + set.back().Insert(COutput(wtx.get(), nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0, false); wtxn.emplace_back(std::move(wtx)); } // Copied from src/wallet/test/coinselector_tests.cpp diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index f4843dd0cec..8032deb2fa2 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -300,16 +300,24 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group ******************************************************************************/ -void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) { +void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only) { + // Compute the effective value first + const CAmount coin_fee = output.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(output.m_input_bytes); + const CAmount ev = output.txout.nValue - coin_fee; + + // Filter for positive only here before adding the coin + if (positive_only && ev <= 0) return; + m_outputs.push_back(output); CInputCoin& coin = m_outputs.back(); - coin.m_fee = coin.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(coin.m_input_bytes); + + coin.m_fee = coin_fee; fee += coin.m_fee; coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.m_input_bytes); long_term_fee += coin.m_long_term_fee; - coin.effective_value = coin.txout.nValue - coin.m_fee; + coin.effective_value = ev; effective_value += coin.effective_value; m_from_me &= from_me; @@ -324,35 +332,9 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size m_descendants = std::max(m_descendants, descendants); } -std::vector::iterator OutputGroup::Discard(const CInputCoin& output) { - auto it = m_outputs.begin(); - while (it != m_outputs.end() && it->outpoint != output.outpoint) ++it; - if (it == m_outputs.end()) return it; - m_value -= output.txout.nValue; - effective_value -= output.effective_value; - fee -= output.m_fee; - long_term_fee -= output.m_long_term_fee; - return m_outputs.erase(it); -} - bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const { return m_depth >= (m_from_me ? eligibility_filter.conf_mine : eligibility_filter.conf_theirs) && m_ancestors <= eligibility_filter.max_ancestors && m_descendants <= eligibility_filter.max_descendants; } - -OutputGroup OutputGroup::GetPositiveOnlyGroup() -{ - OutputGroup group(*this); - for (auto it = group.m_outputs.begin(); it != group.m_outputs.end(); ) { - const CInputCoin& coin = *it; - // Only include outputs that are positive effective value (i.e. not dust) - if (coin.effective_value <= 0) { - it = group.Discard(coin); - } else { - ++it; - } - } - return group; -} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 721f1a22ebf..6e3b8c3915f 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -87,11 +87,8 @@ struct OutputGroup m_long_term_feerate(long_term_feerate) {} - void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants); - std::vector::iterator Discard(const CInputCoin& output); + void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; - - OutputGroup GetPositiveOnlyGroup(); }; bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9af6ddd7cfc..b5c9e62b5c9 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -116,7 +116,7 @@ inline std::vector& GroupCoins(const std::vector& coins static_groups.clear(); for (auto& coin : coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin, 0, true, 0, 0); + static_groups.back().Insert(coin, 0, true, 0, 0, false); } return static_groups; } @@ -127,7 +127,7 @@ inline std::vector& GroupCoins(const std::vector& coins) static_groups.clear(); for (auto& coin : coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin.GetInputCoin(), coin.nDepth, coin.tx->m_amounts[CWalletTx::DEBIT].m_cached[ISMINE_SPENDABLE] && coin.tx->m_amounts[CWalletTx::DEBIT].m_value[ISMINE_SPENDABLE] == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0); + static_groups.back().Insert(coin.GetInputCoin(), coin.nDepth, coin.tx->m_amounts[CWalletTx::DEBIT].m_cached[ISMINE_SPENDABLE] && coin.tx->m_amounts[CWalletTx::DEBIT].m_value[ISMINE_SPENDABLE] == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0, false); } return static_groups; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f2c035da144..df0e39b301d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2374,7 +2374,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil nValueRet = 0; if (coin_selection_params.use_bnb) { - std::vector utxo_pool; // Get long term estimate FeeCalculation feeCalc; CCoinControl temp; @@ -2388,22 +2387,17 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil effective_feerate = coin_selection_params.effective_fee; } - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate, eligibility_filter); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */); // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); - // Filter by the min conf specs and add to utxo_pool and calculate effective value - for (OutputGroup& group : groups) { - OutputGroup pos_group = group.GetPositiveOnlyGroup(); - if (pos_group.effective_value > 0) utxo_pool.push_back(pos_group); - } // Calculate the fees for things that aren't inputs CAmount not_input_fees = coin_selection_params.effective_fee.GetFee(coin_selection_params.tx_noinputs_size); bnb_used = true; - return SelectCoinsBnB(utxo_pool, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); + return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0), eligibility_filter); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); bnb_used = false; return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet); @@ -4199,7 +4193,7 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const { std::vector groups; std::map gmap; std::set full_groups; @@ -4224,16 +4218,17 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu it->second = OutputGroup{effective_feerate, long_term_feerate}; full_groups.insert(dst); } - it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); } else { auto ins = gmap.emplace(dst, OutputGroup{effective_feerate, long_term_feerate}); - ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); + ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); } } else { // This is for if each output gets it's own OutputGroup OutputGroup coin(effective_feerate, long_term_feerate); - coin.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants); - if (coin.EligibleForSpending(filter)) groups.push_back(coin); + coin.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); + if (positive_only && coin.effective_value <= 0) continue; + if (coin.m_outputs.size() > 0 && coin.EligibleForSpending(filter)) groups.push_back(coin); } } } @@ -4245,7 +4240,8 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu group.m_ancestors = max_ancestors - 1; } // If the OutputGroup is not eligible, don't add it - if (group.EligibleForSpending(filter)) groups.push_back(group); + if (positive_only && group.effective_value <= 0) continue; + if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups.push_back(group); } } return groups; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1c523d86b89..da14b0108c4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From f6b305273910db0e46798d361413a7e878cb45f7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 1 Oct 2020 13:43:17 -0400 Subject: [PATCH 6/7] Explicitly filter out partial groups when we don't want them Instead of hacking OutputGroup::m_ancestors to discourage the inclusion of partial groups via the eligibility filter, add a parameter to the eligibility filter that indicates whether we want to include the group. Then for those partial groups, don't return them in GroupOutputs if we indicate they aren't desired. --- src/wallet/coinselection.h | 2 ++ src/wallet/wallet.cpp | 16 ++++++++-------- src/wallet/wallet.h | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 6e3b8c3915f..5d9a410dce9 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -62,9 +62,11 @@ struct CoinEligibilityFilter const int conf_theirs; const uint64_t max_ancestors; const uint64_t max_descendants; + const bool m_include_partial_groups{false}; //! Include partial destination groups when avoid_reuse and there are full groups CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {} CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {} + CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {} }; struct OutputGroup diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index df0e39b301d..c173ebf9883 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2387,7 +2387,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil effective_feerate = coin_selection_params.effective_fee; } - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */); // Calculate cost of change CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); @@ -2397,7 +2397,7 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil bnb_used = true; return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, eligibility_filter.max_ancestors, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); bnb_used = false; return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet); @@ -2489,8 +2489,8 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && SelectCoinsMinConf(value_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)) || (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || - (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || + (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max(), std::numeric_limits::max(), true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); // because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset util::insert(setCoinsRet, setPresetCoins); @@ -4193,7 +4193,7 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const { +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const { std::vector groups; std::map gmap; std::set full_groups; @@ -4235,9 +4235,9 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu if (!single_coin) { for (auto& it : gmap) { auto& group = it.second; - if (full_groups.count(it.first) > 0) { - // Make this unattractive as we want coin selection to avoid it if possible - group.m_ancestors = max_ancestors - 1; + if (full_groups.count(it.first) > 0 && !filter.m_include_partial_groups) { + // Don't include partial groups if we don't want them + continue; } // If the OutputGroup is not eligible, don't add it if (positive_only && group.effective_value <= 0) continue; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index da14b0108c4..f6ce014358d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const size_t max_ancestors, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; + std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 8 Dec 2020 19:19:35 -0500 Subject: [PATCH 7/7] Rewrite OutputGroups to be clearer and to use scriptPubKeys Rewrite OutputGroups so that the logic is easier to follow and understand. There is a slight behavior change as OutputGroups will be grouped by scriptPubKey rather than CTxDestination as before. This should have no effect on users as all addresses are a CTxDestination. However by using scriptPubKeys, we can correctly group outputs which fall into the NoDestination case. But we also shouldn't have any NoDestination outputs. --- src/wallet/wallet.cpp | 114 +++++++++++++++++++++++++++--------------- src/wallet/wallet.h | 2 +- 2 files changed, 74 insertions(+), 42 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c173ebf9883..8ab9438f8c3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4193,58 +4193,90 @@ bool CWalletTx::IsImmatureCoinBase() const return GetBlocksToMaturity() > 0; } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool single_coin, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const { - std::vector groups; - std::map gmap; - std::set full_groups; +std::vector CWallet::GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const +{ + std::vector groups_out; - for (const auto& output : outputs) { - if (output.fSpendable) { - CTxDestination dst; - CInputCoin input_coin = output.GetInputCoin(); + if (separate_coins) { + // Single coin means no grouping. Each COutput gets its own OutputGroup. + for (const COutput& output : outputs) { + // Skip outputs we cannot spend + if (!output.fSpendable) continue; size_t ancestors, descendants; chain().getTransactionAncestry(output.tx->GetHash(), ancestors, descendants); - if (!single_coin && ExtractDestination(output.tx->tx->vout[output.i].scriptPubKey, dst)) { - auto it = gmap.find(dst); - if (it != gmap.end()) { - // Limit output groups to no more than OUTPUT_GROUP_MAX_ENTRIES - // number of entries, to protect against inadvertently creating - // a too-large transaction when using -avoidpartialspends to - // prevent breaking consensus or surprising users with a very - // high amount of fees. - if (it->second.m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { - groups.push_back(it->second); - it->second = OutputGroup{effective_feerate, long_term_feerate}; - full_groups.insert(dst); - } - it->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); - } else { - auto ins = gmap.emplace(dst, OutputGroup{effective_feerate, long_term_feerate}); - ins.first->second.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); - } - } else { - // This is for if each output gets it's own OutputGroup - OutputGroup coin(effective_feerate, long_term_feerate); - coin.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); - if (positive_only && coin.effective_value <= 0) continue; - if (coin.m_outputs.size() > 0 && coin.EligibleForSpending(filter)) groups.push_back(coin); - } + CInputCoin input_coin = output.GetInputCoin(); + + // Make an OutputGroup containing just this output + OutputGroup group{effective_feerate, long_term_feerate}; + group.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); + + // Check the OutputGroup's eligibility. Only add the eligible ones. + if (positive_only && group.effective_value <= 0) continue; + if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); } + return groups_out; } - if (!single_coin) { - for (auto& it : gmap) { - auto& group = it.second; - if (full_groups.count(it.first) > 0 && !filter.m_include_partial_groups) { - // Don't include partial groups if we don't want them + + // We want to combine COutputs that have the same scriptPubKey into single OutputGroups + // except when there are more than OUTPUT_GROUP_MAX_ENTRIES COutputs grouped in an OutputGroup. + // To do this, we maintain a map where the key is the scriptPubKey and the value is a vector of OutputGroups. + // For each COutput, we check if the scriptPubKey is in the map, and if it is, the COutput's CInputCoin is added + // to the last OutputGroup in the vector for the scriptPubKey. When the last OutputGroup has + // OUTPUT_GROUP_MAX_ENTRIES CInputCoins, a new OutputGroup is added to the end of the vector. + std::map> spk_to_groups_map; + for (const auto& output : outputs) { + // Skip outputs we cannot spend + if (!output.fSpendable) continue; + + size_t ancestors, descendants; + chain().getTransactionAncestry(output.tx->GetHash(), ancestors, descendants); + CInputCoin input_coin = output.GetInputCoin(); + CScript spk = input_coin.txout.scriptPubKey; + + std::vector& groups = spk_to_groups_map[spk]; + + if (groups.size() == 0) { + // No OutputGroups for this scriptPubKey yet, add one + groups.emplace_back(effective_feerate, long_term_feerate); + } + + // Get the last OutputGroup in the vector so that we can add the CInputCoin to it + // A pointer is used here so that group can be reassigned later if it is full. + OutputGroup* group = &groups.back(); + + // Check if this OutputGroup is full. We limit to OUTPUT_GROUP_MAX_ENTRIES when using -avoidpartialspends + // to avoid surprising users with very high fees. + if (group->m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { + // The last output group is full, add a new group to the vector and use that group for the insertion + groups.emplace_back(effective_feerate, long_term_feerate); + group = &groups.back(); + } + + // Add the input_coin to group + group->Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); + } + + // Now we go through the entire map and pull out the OutputGroups + for (const auto& spk_and_groups_pair: spk_to_groups_map) { + const std::vector& groups_per_spk= spk_and_groups_pair.second; + + // Go through the vector backwards. This allows for the first item we deal with being the partial group. + for (auto group_it = groups_per_spk.rbegin(); group_it != groups_per_spk.rend(); group_it++) { + const OutputGroup& group = *group_it; + + // Don't include partial groups if there are full groups too and we don't want partial groups + if (group_it == groups_per_spk.rbegin() && groups_per_spk.size() > 1 && !filter.m_include_partial_groups) { continue; } - // If the OutputGroup is not eligible, don't add it + + // Check the OutputGroup's eligibility. Only add the eligible ones. if (positive_only && group.effective_value <= 0) continue; - if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups.push_back(group); + if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group); } } - return groups; + + return groups_out; } bool CWallet::IsCrypted() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f6ce014358d..21cc888712f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -841,7 +841,7 @@ public: bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool single_coin, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; + std::vector GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);