Coin Selection with Murch's algorithm #10637

Open
wants to merge 26 commits into
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+1,053 −535
Split
View
@@ -165,6 +165,7 @@ BITCOIN_CORE_H = \
wallet/rpcwallet.h \
wallet/wallet.h \
wallet/walletdb.h \
+ wallet/coinselection.h \
warnings.h \
zmq/zmqabstractnotifier.h \
zmq/zmqconfig.h\
@@ -243,6 +244,7 @@ libbitcoin_wallet_a_SOURCES = \
wallet/rpcwallet.cpp \
wallet/wallet.cpp \
wallet/walletdb.cpp \
+ wallet/coinselection.cpp \
$(BITCOIN_CORE_H)
# crypto primitives library
@@ -91,7 +91,8 @@ BITCOIN_TESTS += \
wallet/test/wallet_test_fixture.h \
wallet/test/accounting_tests.cpp \
wallet/test/wallet_tests.cpp \
- wallet/test/crypto_tests.cpp
+ wallet/test/crypto_tests.cpp \
+ wallet/test/coinselector_tests.cpp
endif
test_test_bitcoin_SOURCES = $(BITCOIN_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES)
@@ -49,7 +49,9 @@ static void CoinSelection(benchmark::State& state)
std::set<CInputCoin> setCoinsRet;
CAmount nValueRet;
- bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet);
+ CAmount fee_ret;
+ bool success = wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet, fee_ret, CFeeRate(0), true)
+ || wallet.SelectCoinsMinConf(1003 * COIN, 1, 6, 0, vCoins, setCoinsRet, nValueRet, fee_ret, CFeeRate(0), false);
assert(success);
assert(nValueRet == 1003 * COIN);
assert(setCoinsRet.size() == 2);
@@ -0,0 +1,262 @@
+// Copyright (c) 2012-2016 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "wallet/coinselection.h"
+#include "util.h"
+#include "utilmoneystr.h"
+
+// Descending order comparator
+struct {
+ bool operator()(CInputCoin a, CInputCoin b) const
+ {
+ return a.txout.nValue > b.txout.nValue;
+ }
+} descending;
+
+struct CompareValueOnly
+{
+ bool operator()(const CInputCoin& t1,
+ const CInputCoin& t2) const
+ {
+ return t1.txout.nValue < t2.txout.nValue;
+ }
+};
+
+bool SelectCoinsBnB(std::vector<CInputCoin>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, std::vector<CAmount>& fee_vec, CAmount& fee_ret)
+{
+ out_set.clear();
+ value_ret = 0;
+
+ if (utxo_pool.size() <=0) {
+ return false;
+ }
+
+ int depth = 0;
+ int tries = 100000;
+ std::vector<std::pair<bool, bool>> selection; // First bool: select the utxo at this index; Second bool: traversing second branch of this utxo
+ selection.assign(utxo_pool.size(), std::pair<bool, bool>(false, false));
+ bool done = false;
+ bool backtrack = false;
+
+ // Sort the utxo_pool
+ std::sort(utxo_pool.begin(), utxo_pool.end(), descending);
+
+ // Calculate remaining
+ CAmount remaining = 0;
+ for (CInputCoin utxo : utxo_pool) {
+ remaining += utxo.txout.nValue;
@Xekyo

Xekyo Jun 20, 2017

Contributor

Have you filtered utxo_pool to exclude utxo's that have a net-neg value? Otherwise you're underestimating the lookahead here. To get an accurate figure for what you may still collect downtree, you should only add utxo.txout.nValue >=0

@instagibbs

instagibbs Jun 20, 2017

Member

@gmaxwell has concerns that Core wallet is only doing semi-sane utxo handling by spending these. With exact match + sane backoff algorithm this concern may be alleviated?

@achow101

achow101 Jun 20, 2017

Contributor

Indeed, that may be a problem. I will add that in as it is still good to have additional checks here even if done elsewhere.

@gmaxwell

gmaxwell Jun 20, 2017

Member

I don't have much of a concern here about the 0/negative effective value inputs: Failing to select negative effective value inputs for an exact match won't lead to a UTXO count inflation, because changeless transactions are by definition strictly UTXO reducing.

@Xekyo

Xekyo Jun 20, 2017 edited

Contributor

@instagibbs: I'm not completely opposed to spending net-negative UTXO, my concern here is primarily that it actually may cause the lookahead to be underestimated causing valid solutions not to be found.

I realize now that the knapsack algorithm would also not select uneconomic UTXO anymore, as if it had selected enough value before it reached them it would have already returned the set, and if it actually starts exploring them, cannot add more value in the first place.

Advocatus Diaboli: Would it be that terrible though, if UTXO were only considered when they actually have a net positive value? During times of low fees, they'd be used both during BnB and knapsack, during times of high fees, they wouldn't bloat the blocks and lose their owner money.

@instagibbs

instagibbs Jun 20, 2017

Member

I am not so concerned, was making sure concerns are brought up.

@gmaxwell

gmaxwell Jun 22, 2017

Member

@Xekyo we should assume that it would be terrible unless someone can show that it will not cause another massive UTXO bloat event... but thats offtopic here, as I don't think anyone has this concern with exact matches.

@runn1ng

runn1ng Jul 1, 2017 edited

The utxos with negative effective values are filtered anyway in wallet/wallet.cpp, which is the only place (except for tests) from where SelectCoinsBnB is called.

+ }
+
+ // Depth first search to find
+ while (!done)
+ {
+ if (tries <= 0) { // Too many tries, exit
+ return false;
+ } else if (value_ret > target_value + cost_of_change) { // Selected value is out of range, go back and try other branch
+ backtrack = true;
+ } else if (value_ret >= target_value) { // Selected value is within range
+ done = true;
+ } else if (depth >= (int)utxo_pool.size()) { // Reached a leaf node, no solution here
+ backtrack = true;
+ } else if (value_ret + remaining < target_value) { // Cannot possibly reach target with amount remaining
+ if (depth == 0) { // At the first utxo, no possible selections, so exit
+ return false;
+ } else {
+ backtrack = true;
+ }
+ } else { // Continue down this branch
+ // Assert that this utxo is not negative. It should never be negative, effective value calculation should have removed it
+ assert(utxo_pool.at(depth).txout.nValue >= 0);
+
+ // Remove this utxo from the remaining utxo amount
+ remaining -= utxo_pool.at(depth).txout.nValue;
+ // Inclusion branch first (Largest First Exploration)
+ selection.at(depth).first = true;
+ value_ret += utxo_pool.at(depth).txout.nValue;
+ ++depth;
+ }
+
+ // Step back to the previous utxo and try the other branch
+ if (backtrack) {
+ backtrack = false; // Reset
+ --depth;
+
+ // Walk backwards to find the first utxo which has not has its second branch traversed
+ while (selection.at(depth).second) {
+ // Reset this utxo's selection
+ if (selection.at(depth).first) {
+ value_ret -= utxo_pool.at(depth).txout.nValue;
@runn1ng

runn1ng Jul 18, 2017 edited

This line never fires.

It never happens, that an utxo is at the same time in an exclusion branch (which is what .second does) and is also selected (what .first does). Which makes sense; you never at the same time select and not select an utxo :)

With all my simulations, this line never seems to fire (when I rewrote this to JS).

So the other line after if can also be deleted.

@runn1ng

runn1ng Jul 18, 2017

I also think that .second is not needed at all; all the information necessary is in the first and depth; the only situation where .first != !(.second) is after we backtrack here, but the information in .second is useless anyway (since we will change it anyway before we backtrack to it again).

@achow101

achow101 Jul 18, 2017

Contributor

Right. That appears to be a relic of when this randomly selected which branch to try first before I changed it to always try including first.

+ }
+ selection.at(depth).first = false;
+ selection.at(depth).second = false;
+ remaining += utxo_pool.at(depth).txout.nValue;
+
+ // Step back one
+ --depth;
+
+ if (depth < 0) { // We have walked back to the first utxo and no branch is untraversed. No solution, exit.
+ return false;
+ }
+ }
+
+ if (!done) {
@runn1ng

runn1ng Jul 18, 2017

This is never true here. done is never true when backtrack is true. (Istanbul caught that :))

@runn1ng

runn1ng Jul 18, 2017

I mean done is never true and this block always happens

@Xekyo

Xekyo Jul 18, 2017

Contributor

This block doesn't happen when backtrack is false and done is true which happens when a solution is found.

@runn1ng

runn1ng Jul 18, 2017

in that case, the while cycle terminates before that

@runn1ng

runn1ng Jul 18, 2017

In the if at the start of the while cycle, either backtrack or done is set, never both. We got here only when backtrack == true, so done cannot be true.

+ // Now traverse the second branch of the utxo we have arrived at.
+ selection.at(depth).second = true;
+
+ // These were always included first, try excluding now
+ selection.at(depth).first = false;
+ value_ret -= utxo_pool.at(depth).txout.nValue;
+ ++depth;
+ }
+ }
+ --tries;
+ }
+
+ // Set output set
+ for (unsigned int i = 0; i < selection.size(); ++i) {
+ if (selection.at(i).first) {
+ out_set.insert(utxo_pool.at(i));
+ fee_ret += fee_vec.at(i);
+ }
+ }
+
+ return true;
+}
+
+static void ApproximateBestSubset(const std::vector<CInputCoin>& vValue, const CAmount& nTotalLower, const CAmount& nTargetValue,
+ std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
+{
+ std::vector<char> vfIncluded;
+
+ vfBest.assign(vValue.size(), true);
+ nBest = nTotalLower;
+
+ FastRandomContext insecure_rand;
+
+ for (int nRep = 0; nRep < iterations && nBest != nTargetValue; nRep++)
+ {
+ vfIncluded.assign(vValue.size(), false);
+ CAmount nTotal = 0;
+ bool fReachedTarget = false;
+ for (int nPass = 0; nPass < 2 && !fReachedTarget; nPass++)
+ {
+ for (unsigned int i = 0; i < vValue.size(); i++)
+ {
+ //The solver here uses a randomized algorithm,
+ //the randomness serves no real security purpose but is just
+ //needed to prevent degenerate behavior and it is important
+ //that the rng is fast. We do not use a constant random sequence,
+ //because there may be some privacy improvement by making
+ //the selection random.
+ if (nPass == 0 ? insecure_rand.randbool() : !vfIncluded[i])
+ {
+ nTotal += vValue[i].txout.nValue;
+ vfIncluded[i] = true;
+ if (nTotal >= nTargetValue)
+ {
+ fReachedTarget = true;
+ if (nTotal < nBest)
+ {
+ nBest = nTotal;
+ vfBest = vfIncluded;
+ }
+ nTotal -= vValue[i].txout.nValue;
+ vfIncluded[i] = false;
+ }
+ }
+ }
+ }
+ }
+}
+
+bool KnapsackSolver(std::vector<CInputCoin>& utxo_pool, const CAmount& nTargetValue, std::set<CInputCoin>& out_set, CAmount& value_ret)
+{
+ out_set.clear();
+ value_ret = 0;
+
+ // List of values less than target
+ boost::optional<CInputCoin> coinLowestLarger;
+ std::vector<CInputCoin> vValue;
+ CAmount nTotalLower = 0;
+
+ random_shuffle(utxo_pool.begin(), utxo_pool.end(), GetRandInt);
+
+ for (const CInputCoin coin : utxo_pool)
+ {
+ if (coin.txout.nValue == nTargetValue)
+ {
+ out_set.insert(coin);
+ value_ret += coin.txout.nValue;
+ return true;
+ }
+ else if (coin.txout.nValue < nTargetValue + MIN_CHANGE)
+ {
+ vValue.push_back(coin);
+ nTotalLower += coin.txout.nValue;
+ }
+ else if (!coinLowestLarger || coin.txout.nValue < coinLowestLarger->txout.nValue)
+ {
+ coinLowestLarger = coin;
+ }
+ }
+
+ if (nTotalLower == nTargetValue)
+ {
+ for (const auto& input : vValue)
+ {
+ out_set.insert(input);
+ value_ret += input.txout.nValue;
+ }
+ return true;
+ }
+
+ if (nTotalLower < nTargetValue)
+ {
+ if (!coinLowestLarger)
+ return false;
+ out_set.insert(coinLowestLarger.get());
+ value_ret += coinLowestLarger->txout.nValue;
+ return true;
+ }
+
+ // Solve subset sum by stochastic approximation
+ std::sort(vValue.begin(), vValue.end(), CompareValueOnly());
+ std::reverse(vValue.begin(), vValue.end());
+ std::vector<char> vfBest;
+ CAmount nBest;
+
+ ApproximateBestSubset(vValue, nTotalLower, nTargetValue, vfBest, nBest);
+ if (nBest != nTargetValue && nTotalLower >= nTargetValue + MIN_CHANGE)
+ ApproximateBestSubset(vValue, nTotalLower, nTargetValue + MIN_CHANGE, vfBest, nBest);
+
+ // If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
+ // or the next bigger coin is closer), return the bigger coin
+ if (coinLowestLarger &&
+ ((nBest != nTargetValue && nBest < nTargetValue + MIN_CHANGE) || coinLowestLarger->txout.nValue <= nBest))
+ {
+ out_set.insert(coinLowestLarger.get());
+ value_ret += coinLowestLarger->txout.nValue;
+ }
+ else {
+ for (unsigned int i = 0; i < vValue.size(); i++)
+ if (vfBest[i])
+ {
+ out_set.insert(vValue[i]);
+ value_ret += vValue[i].txout.nValue;
+ }
+
+ if (LogAcceptCategory(BCLog::SELECTCOINS)) {
+ LogPrint(BCLog::SELECTCOINS, "SelectCoins() best subset: ");
+ for (unsigned int i = 0; i < vValue.size(); i++) {
+ if (vfBest[i]) {
+ LogPrint(BCLog::SELECTCOINS, "%s ", FormatMoney(vValue[i].txout.nValue));
+ }
+ }
+ LogPrint(BCLog::SELECTCOINS, "total %s\n", FormatMoney(nBest));
+ }
+ }
+
+ return true;
+}
+
View
@@ -0,0 +1,19 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_COINSELECTION_H
+#define BITCOIN_COINSELECTION_H
+
+#include "amount.h"
+#include "primitives/transaction.h"
+#include "random.h"
+#include "wallet/wallet.h"
+
+// rand can be nullptr, but only for testing. exclude_first should only ever be true in tests.
+bool SelectCoinsBnB(std::vector<CInputCoin>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, std::vector<CAmount>& fee_vec, CAmount& fee_ret);
+
+// Original coin selection algorithm as a fallback
+bool KnapsackSolver(std::vector<CInputCoin>& utxo_pool, const CAmount& nTargetValue, std::set<CInputCoin>& out_set, CAmount& value_ret);
+
+#endif // BITCOIN_COINSELECTION_H
View
@@ -14,33 +14,6 @@
#include "util.h"
#include "net.h"
-// Calculate the size of the transaction assuming all signatures are max size
-// Use DummySignatureCreator, which inserts 72 byte signatures everywhere.
-// TODO: re-use this in CWallet::CreateTransaction (right now
-// CreateTransaction uses the constructed dummy-signed tx to do a priority
-// calculation, but we should be able to refactor after priority is removed).
-// NOTE: this requires that all inputs must be in mapWallet (eg the tx should
-// be IsAllFromMe).
-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWallet)
-{
- CMutableTransaction txNew(tx);
- std::vector<CInputCoin> vCoins;
- // Look up the inputs. We should have already checked that this transaction
- // IsAllFromMe(ISMINE_SPENDABLE), so every input should already be in our
- // wallet, with a valid index into the vout array.
- for (auto& input : tx.vin) {
- const auto mi = pWallet->mapWallet.find(input.prevout.hash);
- assert(mi != pWallet->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
- vCoins.emplace_back(CInputCoin(&(mi->second), input.prevout.n));
- }
- if (!pWallet->DummySignTx(txNew, vCoins)) {
- // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
- // implies that we can sign for every input.
- return -1;
- }
- return GetVirtualTransactionSize(txNew);
-}
-
bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx) {
if (pWallet->HasWalletSpend(wtx.GetHash())) {
vErrors.push_back("Transaction has descendants in the wallet");
Oops, something went wrong.