From c7c64db41e1718584aa2f30ff27f60ab0966de62 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jan 2022 19:10:39 -0500 Subject: [PATCH 01/13] wallet: cleanup COutput constructor --- src/wallet/spend.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 4453fb27625..90aab71cf76 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -49,8 +49,15 @@ public: bool fSafe; COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int nDepthIn, bool fSpendableIn, bool fSolvableIn, bool fSafeIn, bool use_max_sig_in = false) + : tx(&wtx), + i(iIn), + nDepth(nDepthIn), + nInputBytes(-1), + fSpendable(fSpendableIn), + fSolvable(fSolvableIn), + use_max_sig(use_max_sig_in), + fSafe(fSafeIn) { - tx = &wtx; i = iIn; nDepth = nDepthIn; fSpendable = fSpendableIn; fSolvable = fSolvableIn; fSafe = fSafeIn; nInputBytes = -1; use_max_sig = use_max_sig_in; // If known and signable by the given wallet, compute nInputBytes // Failure will keep this value -1 if (fSpendable) { From 10379f007fd2c18f4cd24d0a0783d6d929f45556 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 16 Mar 2022 14:38:34 -0400 Subject: [PATCH 02/13] scripted-diff: Rename COutput member variables Update the member variables to match the new style -BEGIN VERIFY SCRIPT- sed -i 's/fSpendableIn/spendable/' $(git grep -l "fSpendableIn") sed -i 's/fSpendable/spendable/' $(git grep -l "fSpendable") sed -i 's/fSolvableIn/solvable/' $(git grep -l "fSolvableIn") sed -i 's/fSolvable/solvable/' $(git grep -l "fSolvable") sed -i 's/fSafeIn/safe/' $(git grep -l "fSafeIn") sed -i 's/fSafe/safe/' $(git grep -l "fSafe") sed -i 's/nInputBytes/input_bytes/' $(git grep -l "nInputBytes") sed -i 's/nDepthIn/depth/' $(git grep -l "nDepthIn" src/wallet src/bench) sed -i 's/nDepth/depth/' src/wallet/spend.h sed -i 's/\.nDepth/.depth/' $(git grep -l "\.nDepth" src/wallet/) sed -i 's/nDepth, FormatMoney/depth, FormatMoney/' src/wallet/spend.cpp -END VERIFY SCRIPT- --- src/bench/coin_selection.cpp | 2 +- src/wallet/interfaces.cpp | 2 +- src/wallet/rpc/coins.cpp | 12 +++++------ src/wallet/spend.cpp | 16 +++++++------- src/wallet/spend.h | 30 +++++++++++++------------- src/wallet/test/coinselector_tests.cpp | 6 +++--- 6 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 609c592d20e..c97ead84f6f 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); + coins.emplace_back(wallet, *wtx, 0 /* iIn */, 6 * 24 /* depth */, true /* spendable */, true /* solvable */, true /* safe */); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 9083c304b2c..2bc8e48943a 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -420,7 +420,7 @@ public: auto& group = result[entry.first]; for (const auto& coin : entry.second) { group.emplace_back(COutPoint(coin.tx->GetHash(), coin.i), - MakeWalletTxOut(*m_wallet, *coin.tx, coin.i, coin.nDepth)); + MakeWalletTxOut(*m_wallet, *coin.tx, coin.i, coin.depth)); } } return result; diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 035541babd4..297af389524 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -703,8 +703,8 @@ RPCHelpMan listunspent() entry.pushKV("scriptPubKey", HexStr(scriptPubKey)); entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue)); - entry.pushKV("confirmations", out.nDepth); - if (!out.nDepth) { + entry.pushKV("confirmations", out.depth); + if (!out.depth) { size_t ancestor_count, descendant_count, ancestor_size; CAmount ancestor_fees; pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); @@ -714,9 +714,9 @@ RPCHelpMan listunspent() entry.pushKV("ancestorfees", uint64_t(ancestor_fees)); } } - entry.pushKV("spendable", out.fSpendable); - entry.pushKV("solvable", out.fSolvable); - if (out.fSolvable) { + entry.pushKV("spendable", out.spendable); + entry.pushKV("solvable", out.solvable); + if (out.solvable) { std::unique_ptr provider = pwallet->GetSolvingProvider(scriptPubKey); if (provider) { auto descriptor = InferDescriptor(scriptPubKey, *provider); @@ -724,7 +724,7 @@ RPCHelpMan listunspent() } } if (avoid_reuse) entry.pushKV("reused", reused); - entry.pushKV("safe", out.fSafe); + entry.pushKV("safe", out.safe); results.push_back(entry); } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a707ef89d2d..e54661456b1 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -31,7 +31,7 @@ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out std::string COutput::ToString() const { - return strprintf("COutput(%s, %d, %d) [%s]", tx->GetHash().ToString(), i, nDepth, FormatMoney(tx->tx->vout[i].nValue)); + return strprintf("COutput(%s, %d, %d) [%s]", tx->GetHash().ToString(), i, depth, FormatMoney(tx->tx->vout[i].nValue)); } int CalculateMaximumSignedInputSize(const CTxOut& txout, const SigningProvider* provider, bool use_max_sig) @@ -218,7 +218,7 @@ CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinContr std::vector vCoins; AvailableCoins(wallet, vCoins, coinControl); for (const COutput& out : vCoins) { - if (out.fSpendable) { + if (out.spendable) { balance += out.tx->tx->vout[out.i].nValue; } } @@ -254,7 +254,7 @@ std::map> ListCoins(const CWallet& wallet) for (const COutput& coin : availableCoins) { CTxDestination address; - if ((coin.fSpendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.fSolvable)) && + if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && ExtractDestination(FindNonChangeParentOutput(wallet, *coin.tx->tx, coin.i).scriptPubKey, address)) { result[address].emplace_back(std::move(coin)); } @@ -292,7 +292,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorGetHash(), ancestors, descendants); @@ -300,7 +300,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector GroupOutputs(const CWallet& wallet, const std::vector> spk_to_groups_map; for (const auto& output : outputs) { // Skip outputs we cannot spend - if (!output.fSpendable) continue; + if (!output.spendable) continue; size_t ancestors, descendants; wallet.chain().getTransactionAncestry(output.tx->GetHash(), ancestors, descendants); @@ -345,7 +345,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorInsert(input_coin, output.nDepth, CachedTxIsFromMe(wallet, *output.tx, ISMINE_ALL), ancestors, descendants, positive_only); + group->Insert(input_coin, output.depth, CachedTxIsFromMe(wallet, *output.tx, ISMINE_ALL), ancestors, descendants, positive_only); } // Now we go through the entire map and pull out the OutputGroups @@ -421,7 +421,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) { for (const COutput& out : vCoins) { - if (!out.fSpendable) continue; + if (!out.spendable) continue; /* Set depth, from_me, ancestors, and descendants to 0 or false as these don't matter for preset inputs as no actual selection is being done. * positive_only is set to false because we want to include all preset inputs, even if they are dust. */ diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 90aab71cf76..62b28c33c88 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -27,16 +27,16 @@ public: * If > 0: the tx is on chain and has this many confirmations. * If = 0: the tx is waiting confirmation. * If < 0: a conflicting tx is on chain and has this many confirmations. */ - int nDepth; + int depth; /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */ - int nInputBytes; + int input_bytes; /** Whether we have the private keys to spend this output */ - bool fSpendable; + bool spendable; /** Whether we know how to spend this output, ignoring the lack of keys */ - bool fSolvable; + bool solvable; /** Whether to use the maximum sized, 72 byte signature when calculating the size of the input spend. This should only be set when watch-only outputs are allowed */ bool use_max_sig; @@ -46,22 +46,22 @@ public: * from outside keys and unconfirmed replacement transactions are considered * unsafe and will not be used to fund new spending transactions. */ - bool fSafe; + bool safe; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int nDepthIn, bool fSpendableIn, bool fSolvableIn, bool fSafeIn, bool use_max_sig_in = false) + COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in = false) : tx(&wtx), i(iIn), - nDepth(nDepthIn), - nInputBytes(-1), - fSpendable(fSpendableIn), - fSolvable(fSolvableIn), + depth(depth), + input_bytes(-1), + spendable(spendable), + solvable(solvable), use_max_sig(use_max_sig_in), - fSafe(fSafeIn) + safe(safe) { - // If known and signable by the given wallet, compute nInputBytes + // If known and signable by the given wallet, compute input_bytes // Failure will keep this value -1 - if (fSpendable) { - nInputBytes = GetTxSpendSize(wallet, wtx, i, use_max_sig); + if (spendable) { + input_bytes = GetTxSpendSize(wallet, wtx, i, use_max_sig); } } @@ -69,7 +69,7 @@ public: inline CInputCoin GetInputCoin() const { - return CInputCoin(tx->tx, i, nInputBytes); + return CInputCoin(tx->tx, i, input_bytes); } }; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index b9f12158caa..1a6430203f8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -157,7 +157,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, false); + static_groups.back().Insert(coin.GetInputCoin(), coin.depth, 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; } @@ -315,13 +315,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) std::vector coins; add_coin(coins, *wallet, 1); - coins.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 + coins.at(0).input_bytes = 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(!SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change)); // Test fees subtracted from output: coins.clear(); add_coin(coins, *wallet, 1 * CENT); - coins.at(0).nInputBytes = 40; + coins.at(0).input_bytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; const auto result9 = SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change); BOOST_CHECK(result9); From 46022953ee2e8113167bafd1fd48a383a578b13c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 16 Mar 2022 15:13:57 -0400 Subject: [PATCH 03/13] wallet: Remove use_max_sig default value As we change the constructor for COutput, it becomes somewhat dangerous if there are default values. --- src/bench/coin_selection.cpp | 2 +- src/wallet/spend.cpp | 2 +- src/wallet/spend.h | 2 +- src/wallet/test/coinselector_tests.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index c97ead84f6f..c9c9fa3c9cb 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, 0 /* iIn */, 6 * 24 /* depth */, true /* spendable */, true /* solvable */, true /* safe */); + coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e54661456b1..6f5d0fed86f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -275,7 +275,7 @@ std::map> ListCoins(const CWallet& wallet) CTxDestination address; if (ExtractDestination(FindNonChangeParentOutput(wallet, *it->second.tx, output.n).scriptPubKey, address)) { result[address].emplace_back( - wallet, it->second, output.n, depth, true /* spendable */, true /* solvable */, false /* safe */); + wallet, it->second, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, /*use_max_sig_in=*/ false); } } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 62b28c33c88..eb0a2ec05d8 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -48,7 +48,7 @@ public: */ bool safe; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in = false) + COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in) : tx(&wtx), i(iIn), depth(depth), diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1a6430203f8..430e202d4f4 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -98,7 +98,7 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount wtx.m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1); wtx.m_is_cache_empty = false; } - COutput output(wallet, wtx, nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */); + COutput output(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false); coins.push_back(output); } From b799814bbd53736b79495072f3c9e05989a465e8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jan 2022 20:31:08 -0500 Subject: [PATCH 04/13] wallet: Store tx time in COutput --- src/bench/coin_selection.cpp | 2 +- src/wallet/interfaces.cpp | 13 ++++++++++++- src/wallet/spend.cpp | 13 +++++++------ src/wallet/spend.h | 8 ++++++-- src/wallet/test/coinselector_tests.cpp | 3 +-- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index c9c9fa3c9cb..4cbfb05228d 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false); + coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*use_max_sig_in=*/ false); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 2bc8e48943a..56ca2f0cdb1 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -111,6 +111,17 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet, return result; } +WalletTxOut MakeWalletTxOut(const CWallet& wallet, + const COutput& output) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +{ + WalletTxOut result; + result.txout = output.tx->tx->vout[output.i]; + result.time = output.time; + result.depth_in_main_chain = output.depth; + result.is_spent = wallet.IsSpent(output.tx->GetHash(), output.i); + return result; +} + class WalletImpl : public Wallet { public: @@ -420,7 +431,7 @@ public: auto& group = result[entry.first]; for (const auto& coin : entry.second) { group.emplace_back(COutPoint(coin.tx->GetHash(), coin.i), - MakeWalletTxOut(*m_wallet, *coin.tx, coin.i, coin.depth)); + MakeWalletTxOut(*m_wallet, coin)); } } return result; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 6f5d0fed86f..887a5dae772 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -191,7 +191,7 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); + vCoins.emplace_back(wallet, wtx, i, nDepth, spendable, solvable, safeTx, wtx.GetTxTime(), /*use_max_sig_in=*/ (coinControl && coinControl->fAllowWatchOnly)); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { @@ -268,14 +268,15 @@ std::map> ListCoins(const CWallet& wallet) for (const COutPoint& output : lockedCoins) { auto it = wallet.mapWallet.find(output.hash); if (it != wallet.mapWallet.end()) { - int depth = wallet.GetTxDepthInMainChain(it->second); - if (depth >= 0 && output.n < it->second.tx->vout.size() && - wallet.IsMine(it->second.tx->vout[output.n]) == is_mine_filter + const auto& wtx = it->second; + int depth = wallet.GetTxDepthInMainChain(wtx); + if (depth >= 0 && output.n < wtx.tx->vout.size() && + wallet.IsMine(wtx.tx->vout[output.n]) == is_mine_filter ) { CTxDestination address; - if (ExtractDestination(FindNonChangeParentOutput(wallet, *it->second.tx, output.n).scriptPubKey, address)) { + if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) { result[address].emplace_back( - wallet, it->second, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, /*use_max_sig_in=*/ false); + wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), /*use_max_sig_in=*/ false); } } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index eb0a2ec05d8..86735fc998e 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -48,7 +48,10 @@ public: */ bool safe; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in) + /** The time of the transaction containing this output as determined by CWalletTx::nTimeSmart */ + int64_t time; + + COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool use_max_sig_in) : tx(&wtx), i(iIn), depth(depth), @@ -56,7 +59,8 @@ public: spendable(spendable), solvable(solvable), use_max_sig(use_max_sig_in), - safe(safe) + safe(safe), + time(time) { // If known and signable by the given wallet, compute input_bytes // Failure will keep this value -1 diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 430e202d4f4..bcfcfd186a8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -98,8 +98,7 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount wtx.m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1); wtx.m_is_cache_empty = false; } - COutput output(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false); - coins.push_back(output); + coins.emplace_back(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), /*use_max_sig_in=*/ false); } /** Check if SelectionResult a is equivalent to SelectionResult b. From d51f27d3bb0d6e3ca55bcd23ce53e4fe413a9360 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jan 2022 19:03:23 -0500 Subject: [PATCH 05/13] wallet: Store whether a COutput is from the wallet Instead of determining whether the containing transaction is from the wallet dynamically as needed, just pass it in to COutput and store it. The transaction ownership isn't going to change. --- src/bench/coin_selection.cpp | 2 +- src/wallet/spend.cpp | 10 ++++++---- src/wallet/spend.h | 8 ++++++-- src/wallet/test/coinselector_tests.cpp | 14 ++------------ 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 4cbfb05228d..d22b335a8b6 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*use_max_sig_in=*/ false); + coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true, /*use_max_sig_in=*/ false); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 887a5dae772..cc987e7e653 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -158,6 +158,8 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C continue; } + bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL); + for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { // Only consider selected coins if add_inputs is false if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) { @@ -191,7 +193,7 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - vCoins.emplace_back(wallet, wtx, i, nDepth, spendable, solvable, safeTx, wtx.GetTxTime(), /*use_max_sig_in=*/ (coinControl && coinControl->fAllowWatchOnly)); + vCoins.emplace_back(wallet, wtx, i, nDepth, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, /*use_max_sig_in=*/ (coinControl && coinControl->fAllowWatchOnly)); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { @@ -276,7 +278,7 @@ std::map> ListCoins(const CWallet& wallet) CTxDestination address; if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) { result[address].emplace_back( - wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), /*use_max_sig_in=*/ false); + wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL), /*use_max_sig_in=*/ false); } } } @@ -301,7 +303,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector GroupOutputs(const CWallet& wallet, const std::vectorInsert(input_coin, output.depth, CachedTxIsFromMe(wallet, *output.tx, ISMINE_ALL), ancestors, descendants, positive_only); + group->Insert(input_coin, output.depth, output.from_me, ancestors, descendants, positive_only); } // Now we go through the entire map and pull out the OutputGroups diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 86735fc998e..e71e90eaca0 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -51,7 +51,10 @@ public: /** The time of the transaction containing this output as determined by CWalletTx::nTimeSmart */ int64_t time; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool use_max_sig_in) + /** Whether the transaction containing this output is sent from the owning wallet */ + bool from_me; + + COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, bool use_max_sig_in) : tx(&wtx), i(iIn), depth(depth), @@ -60,7 +63,8 @@ public: solvable(solvable), use_max_sig(use_max_sig_in), safe(safe), - time(time) + time(time), + from_me(from_me) { // If known and signable by the given wallet, compute input_bytes // Failure will keep this value -1 diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index bcfcfd186a8..2293f5793cb 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -82,23 +82,13 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount assert(destination_ok); tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest); } - if (fIsFromMe) { - // IsFromMe() returns (GetDebit() > 0), and GetDebit() is 0 if vin.empty(), - // so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe() - tx.vin.resize(1); - } uint256 txid = tx.GetHash(); LOCK(wallet.cs_wallet); auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); assert(ret.second); CWalletTx& wtx = (*ret.first).second; - if (fIsFromMe) - { - wtx.m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1); - wtx.m_is_cache_empty = false; - } - coins.emplace_back(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), /*use_max_sig_in=*/ false); + coins.emplace_back(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, /*use_max_sig_in=*/ false); } /** Check if SelectionResult a is equivalent to SelectionResult b. @@ -156,7 +146,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.depth, 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); + static_groups.back().Insert(coin.GetInputCoin(), coin.depth, coin.from_me, 0, 0, false); } return static_groups; } From 0ba4d1916e26e2a5d603edcdb7625463989d25b6 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jan 2022 21:04:26 -0500 Subject: [PATCH 06/13] wallet: Provide input bytes to COutput --- src/bench/coin_selection.cpp | 2 +- src/wallet/spend.cpp | 4 ++-- src/wallet/spend.h | 20 ++++++-------------- src/wallet/test/coinselector_tests.cpp | 2 +- 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index d22b335a8b6..0610739a4d2 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true, /*use_max_sig_in=*/ false); + coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index cc987e7e653..0dc79e9c804 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -193,7 +193,7 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - vCoins.emplace_back(wallet, wtx, i, nDepth, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, /*use_max_sig_in=*/ (coinControl && coinControl->fAllowWatchOnly)); + vCoins.emplace_back(wallet, wtx, i, nDepth, GetTxSpendSize(wallet, wtx, i, /*use_max_sig=*/ (coinControl && coinControl->fAllowWatchOnly)), spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { @@ -278,7 +278,7 @@ std::map> ListCoins(const CWallet& wallet) CTxDestination address; if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) { result[address].emplace_back( - wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL), /*use_max_sig_in=*/ false); + wallet, wtx, output.n, depth, GetTxSpendSize(wallet, wtx, output.n), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)); } } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index e71e90eaca0..029560a16c0 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -11,7 +11,9 @@ #include namespace wallet { -/** Get the marginal bytes if spending the specified output from this transaction */ +/** Get the marginal bytes if spending the specified output from this transaction. + * use_max_sig indicates whether to use the maximum sized, 72 byte signature when calculating the + * size of the input spend. This should only be set when watch-only outputs are allowed */ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); class COutput @@ -38,9 +40,6 @@ public: /** Whether we know how to spend this output, ignoring the lack of keys */ bool solvable; - /** Whether to use the maximum sized, 72 byte signature when calculating the size of the input spend. This should only be set when watch-only outputs are allowed */ - bool use_max_sig; - /** * Whether this output is considered safe to spend. Unconfirmed transactions * from outside keys and unconfirmed replacement transactions are considered @@ -54,24 +53,17 @@ public: /** Whether the transaction containing this output is sent from the owning wallet */ bool from_me; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, bool use_max_sig_in) + COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) : tx(&wtx), i(iIn), depth(depth), - input_bytes(-1), + input_bytes(input_bytes), spendable(spendable), solvable(solvable), - use_max_sig(use_max_sig_in), safe(safe), time(time), from_me(from_me) - { - // If known and signable by the given wallet, compute input_bytes - // Failure will keep this value -1 - if (spendable) { - input_bytes = GetTxSpendSize(wallet, wtx, i, use_max_sig); - } - } + {} std::string ToString() const; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 2293f5793cb..ebeb50bace8 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -88,7 +88,7 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); assert(ret.second); CWalletTx& wtx = (*ret.first).second; - coins.emplace_back(wallet, wtx, nInput, nAge, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, /*use_max_sig_in=*/ false); + coins.emplace_back(wallet, wtx, nInput, nAge, GetTxSpendSize(wallet, wtx, nInput), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe); } /** Check if SelectionResult a is equivalent to SelectionResult b. From 14d04d5ad15ae56df56edee7ca9a202b52037889 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 18 Jan 2022 19:08:42 -0500 Subject: [PATCH 07/13] wallet: Replace CWalletTx in COutput with COutPoint and CTxOut Instead of having a pointer to the CWalletTx in COutput, we can just store the COutPoint and the CTxOut as those are the only things we need from the CWalletTx. Other things CWalletTx used to provide were time and fIsFromMe but these are also being stored by COutput. --- src/wallet/interfaces.cpp | 6 +++--- src/wallet/rpc/coins.cpp | 12 ++++++------ src/wallet/spend.cpp | 16 +++++++++++----- src/wallet/spend.h | 14 ++++++++------ src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 56ca2f0cdb1..087d1df2a97 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -115,10 +115,10 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet, const COutput& output) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { WalletTxOut result; - result.txout = output.tx->tx->vout[output.i]; + result.txout = output.txout; result.time = output.time; result.depth_in_main_chain = output.depth; - result.is_spent = wallet.IsSpent(output.tx->GetHash(), output.i); + result.is_spent = wallet.IsSpent(output.outpoint.hash, output.outpoint.n); return result; } @@ -430,7 +430,7 @@ public: for (const auto& entry : ListCoins(*m_wallet)) { auto& group = result[entry.first]; for (const auto& coin : entry.second) { - group.emplace_back(COutPoint(coin.tx->GetHash(), coin.i), + group.emplace_back(coin.outpoint, MakeWalletTxOut(*m_wallet, coin)); } } diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 297af389524..781ae3c6e04 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -648,16 +648,16 @@ RPCHelpMan listunspent() for (const COutput& out : vecOutputs) { CTxDestination address; - const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey; + const CScript& scriptPubKey = out.txout.scriptPubKey; bool fValidAddress = ExtractDestination(scriptPubKey, address); - bool reused = avoid_reuse && pwallet->IsSpentKey(out.tx->GetHash(), out.i); + bool reused = avoid_reuse && pwallet->IsSpentKey(out.outpoint.hash, out.outpoint.n); if (destinations.size() && (!fValidAddress || !destinations.count(address))) continue; UniValue entry(UniValue::VOBJ); - entry.pushKV("txid", out.tx->GetHash().GetHex()); - entry.pushKV("vout", out.i); + entry.pushKV("txid", out.outpoint.hash.GetHex()); + entry.pushKV("vout", (int)out.outpoint.n); if (fValidAddress) { entry.pushKV("address", EncodeDestination(address)); @@ -702,12 +702,12 @@ RPCHelpMan listunspent() } entry.pushKV("scriptPubKey", HexStr(scriptPubKey)); - entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue)); + entry.pushKV("amount", ValueFromAmount(out.txout.nValue)); entry.pushKV("confirmations", out.depth); if (!out.depth) { size_t ancestor_count, descendant_count, ancestor_size; CAmount ancestor_fees; - pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); + pwallet->chain().getTransactionAncestry(out.outpoint.hash, ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); if (ancestor_count) { entry.pushKV("ancestorcount", uint64_t(ancestor_count)); entry.pushKV("ancestorsize", uint64_t(ancestor_size)); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 0dc79e9c804..ce6fe8e4c95 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -31,7 +31,7 @@ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out std::string COutput::ToString() const { - return strprintf("COutput(%s, %d, %d) [%s]", tx->GetHash().ToString(), i, depth, FormatMoney(tx->tx->vout[i].nValue)); + return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue)); } int CalculateMaximumSignedInputSize(const CTxOut& txout, const SigningProvider* provider, bool use_max_sig) @@ -221,7 +221,7 @@ CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinContr AvailableCoins(wallet, vCoins, coinControl); for (const COutput& out : vCoins) { if (out.spendable) { - balance += out.tx->tx->vout[out.i].nValue; + balance += out.txout.nValue; } } return balance; @@ -245,6 +245,12 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const CTransactio return ptx->vout[n]; } +const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& outpoint) +{ + AssertLockHeld(wallet.cs_wallet); + return FindNonChangeParentOutput(wallet, *wallet.GetWalletTx(outpoint.hash)->tx, outpoint.n); +} + std::map> ListCoins(const CWallet& wallet) { AssertLockHeld(wallet.cs_wallet); @@ -257,7 +263,7 @@ std::map> ListCoins(const CWallet& wallet) for (const COutput& coin : availableCoins) { CTxDestination address; if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && - ExtractDestination(FindNonChangeParentOutput(wallet, *coin.tx->tx, coin.i).scriptPubKey, address)) { + ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { result[address].emplace_back(std::move(coin)); } } @@ -298,7 +304,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorGetHash(), ancestors, descendants); + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); CInputCoin input_coin = output.GetInputCoin(); // Make an OutputGroup containing just this output @@ -324,7 +330,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vectorGetHash(), ancestors, descendants); + wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants); CInputCoin input_coin = output.GetInputCoin(); CScript spk = input_coin.txout.scriptPubKey; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 029560a16c0..141ec833eee 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -19,10 +19,11 @@ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out class COutput { public: - const CWalletTx *tx; + /** The outpoint identifying this UTXO */ + COutPoint outpoint; - /** Index in tx->vout. */ - int i; + /** The output itself */ + CTxOut txout; /** * Depth in block chain. @@ -54,8 +55,8 @@ public: bool from_me; COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) - : tx(&wtx), - i(iIn), + : outpoint(COutPoint(wtx.GetHash(), iIn)), + txout(wtx.tx->vout.at(iIn)), depth(depth), input_bytes(input_bytes), spendable(spendable), @@ -69,7 +70,7 @@ public: inline CInputCoin GetInputCoin() const { - return CInputCoin(tx->tx, i, input_bytes); + return CInputCoin(outpoint, txout, input_bytes); } }; @@ -100,6 +101,7 @@ CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinContr * Find non-change parent output. */ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const CTransaction& tx, int output) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& outpoint) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** * Return list of available coins and locked coins grouped by non-change output address. diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index ebeb50bace8..393ec138c6c 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -331,7 +331,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(coins, *wallet, 2 * CENT, 6 * 24, false, 0, true); CCoinControl coin_control; coin_control.fAllowOtherInputs = true; - coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); + coin_control.Select(coins.at(0).outpoint); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); const auto result10 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c59f7e6f058..8456107ebfd 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -588,7 +588,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) for (const auto& group : list) { for (const auto& coin : group.second) { LOCK(wallet->cs_wallet); - wallet->LockCoin(COutPoint(coin.tx->GetHash(), coin.i)); + wallet->LockCoin(coin.outpoint); } } { From 42e974e15c6deba1d9395a4da9341c9ebec6e8e5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Jan 2022 16:05:16 -0500 Subject: [PATCH 08/13] wallet: Remove CWallet and CWalletTx from COutput's constructor --- src/bench/coin_selection.cpp | 2 +- src/wallet/spend.cpp | 5 +++-- src/wallet/spend.h | 6 +++--- src/wallet/test/coinselector_tests.cpp | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 0610739a4d2..8ed0f6df2b0 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench) // Create coins std::vector coins; for (const auto& wtx : wtxs) { - coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true); + coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ce6fe8e4c95..0094af7b2d8 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -192,8 +192,9 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); + int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); - vCoins.emplace_back(wallet, wtx, i, nDepth, GetTxSpendSize(wallet, wtx, i, /*use_max_sig=*/ (coinControl && coinControl->fAllowWatchOnly)), spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me); + vCoins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me); // Checks the sum amount of all UTXO's. if (nMinimumSumAmount != MAX_MONEY) { @@ -284,7 +285,7 @@ std::map> ListCoins(const CWallet& wallet) CTxDestination address; if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) { result[address].emplace_back( - wallet, wtx, output.n, depth, GetTxSpendSize(wallet, wtx, output.n), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)); + COutPoint(wtx.GetHash(), output.n), wtx.tx->vout.at(output.n), depth, GetTxSpendSize(wallet, wtx, output.n), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)); } } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 141ec833eee..42f1124b7e9 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -54,9 +54,9 @@ public: /** Whether the transaction containing this output is sent from the owning wallet */ bool from_me; - COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) - : outpoint(COutPoint(wtx.GetHash(), iIn)), - txout(wtx.tx->vout.at(iIn)), + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) + : outpoint(outpoint), + txout(txout), depth(depth), input_bytes(input_bytes), spendable(spendable), diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 393ec138c6c..fb246a3377c 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -88,7 +88,7 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{})); assert(ret.second); CWalletTx& wtx = (*ret.first).second; - coins.emplace_back(wallet, wtx, nInput, nAge, GetTxSpendSize(wallet, wtx, nInput), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe); + coins.emplace_back(COutPoint(wtx.GetHash(), nInput), wtx.tx->vout.at(nInput), nAge, GetTxSpendSize(wallet, wtx, nInput), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe); } /** Check if SelectionResult a is equivalent to SelectionResult b. From f0821230b8de2eec21a869d1edf9e2b9f502de25 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Jan 2022 16:13:02 -0500 Subject: [PATCH 09/13] moveonly: move COutput to coinselection.h --- src/wallet/coinselection.cpp | 5 ++++ src/wallet/coinselection.h | 58 ++++++++++++++++++++++++++++++++++++ src/wallet/spend.cpp | 5 ---- src/wallet/spend.h | 58 ------------------------------------ 4 files changed, 63 insertions(+), 63 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 513572da455..08c36de38a2 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -432,4 +432,9 @@ bool SelectionResult::operator<(SelectionResult other) const // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal. return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size()); } + +std::string COutput::ToString() const +{ + return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue)); +} } // namespace wallet diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 496a0269991..7f375b6169c 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -72,6 +72,64 @@ public: } }; +class COutput +{ +public: + /** The outpoint identifying this UTXO */ + COutPoint outpoint; + + /** The output itself */ + CTxOut txout; + + /** + * Depth in block chain. + * If > 0: the tx is on chain and has this many confirmations. + * If = 0: the tx is waiting confirmation. + * If < 0: a conflicting tx is on chain and has this many confirmations. */ + int depth; + + /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */ + int input_bytes; + + /** Whether we have the private keys to spend this output */ + bool spendable; + + /** Whether we know how to spend this output, ignoring the lack of keys */ + bool solvable; + + /** + * Whether this output is considered safe to spend. Unconfirmed transactions + * from outside keys and unconfirmed replacement transactions are considered + * unsafe and will not be used to fund new spending transactions. + */ + bool safe; + + /** The time of the transaction containing this output as determined by CWalletTx::nTimeSmart */ + int64_t time; + + /** Whether the transaction containing this output is sent from the owning wallet */ + bool from_me; + + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) + : outpoint(outpoint), + txout(txout), + depth(depth), + input_bytes(input_bytes), + spendable(spendable), + solvable(solvable), + safe(safe), + time(time), + from_me(from_me) + {} + + std::string ToString() const; + + inline CInputCoin GetInputCoin() const + { + return CInputCoin(outpoint, txout, input_bytes); + } +}; + /** Parameters for one iteration of Coin Selection. */ struct CoinSelectionParams { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 0094af7b2d8..356e2be80d6 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -29,11 +29,6 @@ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); } -std::string COutput::ToString() const -{ - return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue)); -} - int CalculateMaximumSignedInputSize(const CTxOut& txout, const SigningProvider* provider, bool use_max_sig) { CMutableTransaction txn; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 42f1124b7e9..e43aac52733 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -16,64 +16,6 @@ namespace wallet { * size of the input spend. This should only be set when watch-only outputs are allowed */ int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); -class COutput -{ -public: - /** The outpoint identifying this UTXO */ - COutPoint outpoint; - - /** The output itself */ - CTxOut txout; - - /** - * Depth in block chain. - * If > 0: the tx is on chain and has this many confirmations. - * If = 0: the tx is waiting confirmation. - * If < 0: a conflicting tx is on chain and has this many confirmations. */ - int depth; - - /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */ - int input_bytes; - - /** Whether we have the private keys to spend this output */ - bool spendable; - - /** Whether we know how to spend this output, ignoring the lack of keys */ - bool solvable; - - /** - * Whether this output is considered safe to spend. Unconfirmed transactions - * from outside keys and unconfirmed replacement transactions are considered - * unsafe and will not be used to fund new spending transactions. - */ - bool safe; - - /** The time of the transaction containing this output as determined by CWalletTx::nTimeSmart */ - int64_t time; - - /** Whether the transaction containing this output is sent from the owning wallet */ - bool from_me; - - COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) - : outpoint(outpoint), - txout(txout), - depth(depth), - input_bytes(input_bytes), - spendable(spendable), - solvable(solvable), - safe(safe), - time(time), - from_me(from_me) - {} - - std::string ToString() const; - - inline CInputCoin GetInputCoin() const - { - return CInputCoin(outpoint, txout, input_bytes); - } -}; - //Get the marginal bytes of spending the specified output int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, bool use_max_sig = false); int CalculateMaximumSignedInputSize(const CTxOut& txout, const SigningProvider* pwallet, bool use_max_sig = false); From 14fbb57b79c664090f6a4e60d7bdfc9759ff4307 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Jan 2022 16:19:30 -0500 Subject: [PATCH 10/13] coinselection: Add effective value and fees to COutput --- src/wallet/coinselection.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 7f375b6169c..5dbb63310c9 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -110,6 +110,15 @@ public: /** Whether the transaction containing this output is sent from the owning wallet */ bool from_me; + /** The output's value minus fees required to spend it. Initialized as the output's absolute value. */ + CAmount effective_value; + + /** The fee required to spend this output at the transaction's target feerate. */ + CAmount fee{0}; + + /** The fee required to spend this output at the consolidation feerate. */ + CAmount long_term_fee{0}; + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me) : outpoint(outpoint), txout(txout), @@ -119,7 +128,8 @@ public: solvable(solvable), safe(safe), time(time), - from_me(from_me) + from_me(from_me), + effective_value(txout.nValue) {} std::string ToString() const; From 70f31f1a81710aa59e95770de9a84bf58cbce1e8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Jan 2022 17:18:31 -0500 Subject: [PATCH 11/13] coinselection: Use COutput instead of CInputCoin Also rename setPresetCoins to preset_coins --- src/bench/coin_selection.cpp | 4 +-- src/wallet/coinselection.cpp | 36 ++++++++++----------- src/wallet/coinselection.h | 26 +++++++++++---- src/wallet/spend.cpp | 44 +++++++++++++------------- src/wallet/test/coinselector_tests.cpp | 33 +++++++------------ 5 files changed, 72 insertions(+), 71 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 8ed0f6df2b0..b332ef9c11a 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -82,9 +82,9 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector CMutableTransaction tx; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; - CInputCoin coin(MakeTransactionRef(tx), nInput); + COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true); set.emplace_back(); - set.back().Insert(coin, 0, true, 0, 0, false); + set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); } // Copied from src/wallet/test/coinselector_tests.cpp static CAmount make_hard_case(int utxos, std::vector& utxo_pool) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 08c36de38a2..7347ad1d40b 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -50,8 +50,8 @@ struct { * The Branch and Bound algorithm is described in detail in Murch's Master Thesis: * https://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf * - * @param const std::vector& utxo_pool The set of UTXOs that we are choosing from. - * These UTXOs will be sorted in descending order by effective value and the CInputCoins' + * @param const std::vector& utxo_pool The set of UTXO groups that we are choosing from. + * These UTXO groups will be sorted in descending order by effective value and the OutputGroups' * values are their effective values. * @param const CAmount& selection_target This is the value that we want to select. It is the lower * bound of the range. @@ -315,29 +315,29 @@ std::optional KnapsackSolver(std::vector& groups, ******************************************************************************/ -void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only) { +void OutputGroup::Insert(const COutput& output, 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 coin_fee = output.input_bytes < 0 ? 0 : m_effective_feerate.GetFee(output.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(); + COutput& coin = m_outputs.back(); - coin.m_fee = coin_fee; - fee += coin.m_fee; + coin.fee = coin_fee; + fee += coin.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.long_term_fee = coin.input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.input_bytes); + long_term_fee += coin.long_term_fee; coin.effective_value = ev; effective_value += coin.effective_value; - m_from_me &= from_me; - m_value += output.txout.nValue; - m_depth = std::min(m_depth, depth); + m_from_me &= coin.from_me; + m_value += coin.txout.nValue; + m_depth = std::min(m_depth, coin.depth); // ancestors here express the number of ancestors the new coin will end up having, which is // the sum, rather than the max; this will overestimate in the cases where multiple inputs // have common ancestors @@ -359,7 +359,7 @@ CAmount OutputGroup::GetSelectionAmount() const return m_subtract_fee_outputs ? m_value : effective_value; } -CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value) +CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value) { // This function should not be called with empty inputs as that would mean the selection failed assert(!inputs.empty()); @@ -367,8 +367,8 @@ CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cos // Always consider the cost of spending an input now vs in the future. CAmount waste = 0; CAmount selected_effective_value = 0; - for (const CInputCoin& coin : inputs) { - waste += coin.m_fee - coin.m_long_term_fee; + for (const COutput& coin : inputs) { + waste += coin.fee - coin.long_term_fee; selected_effective_value += use_effective_value ? coin.effective_value : coin.txout.nValue; } @@ -413,14 +413,14 @@ void SelectionResult::AddInput(const OutputGroup& group) m_use_effective = !group.m_subtract_fee_outputs; } -const std::set& SelectionResult::GetInputSet() const +const std::set& SelectionResult::GetInputSet() const { return m_selected_inputs; } -std::vector SelectionResult::GetShuffledInputVector() const +std::vector SelectionResult::GetShuffledInputVector() const { - std::vector coins(m_selected_inputs.begin(), m_selected_inputs.end()); + std::vector coins(m_selected_inputs.begin(), m_selected_inputs.end()); Shuffle(coins.begin(), coins.end(), FastRandomContext()); return coins; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 5dbb63310c9..4d39be364cb 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -138,6 +138,18 @@ public: { return CInputCoin(outpoint, txout, input_bytes); } + + bool operator<(const COutput& rhs) const { + return outpoint < rhs.outpoint; + } + + bool operator!=(const COutput& rhs) const { + return outpoint != rhs.outpoint; + } + + bool operator==(const COutput& rhs) const { + return outpoint == rhs.outpoint; + } }; /** Parameters for one iteration of Coin Selection. */ @@ -207,7 +219,7 @@ struct CoinEligibilityFilter struct OutputGroup { /** The list of UTXOs contained in this output group. */ - std::vector m_outputs; + std::vector m_outputs; /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at * least a certain number of confirmations on UTXOs received from outside wallets while trusting * our own UTXOs more. */ @@ -244,7 +256,7 @@ struct OutputGroup m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} - void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only); + void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; CAmount GetSelectionAmount() const; }; @@ -266,13 +278,13 @@ struct OutputGroup * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false). * @return The waste */ -[[nodiscard]] CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); +[[nodiscard]] CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set m_selected_inputs; + std::set m_selected_inputs; /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */ const CAmount m_target; /** Whether the input values for calculations should be the effective value (true) or normal value (false) */ @@ -298,9 +310,9 @@ public: [[nodiscard]] CAmount GetWaste() const; /** Get m_selected_inputs */ - const std::set& GetInputSet() const; - /** Get the vector of CInputCoins that will be used to fill in a CTransaction's vin */ - std::vector GetShuffledInputVector() const; + const std::set& GetInputSet() const; + /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ + std::vector GetShuffledInputVector() const; bool operator<(SelectionResult other) const; }; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 356e2be80d6..288282d3de0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -301,11 +301,10 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector GroupOutputs(const CWallet& wallet, const std::vector> spk_to_groups_map; for (const auto& output : outputs) { // Skip outputs we cannot spend @@ -327,8 +326,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector& groups = spk_to_groups_map[spk]; @@ -337,7 +335,7 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector GroupOutputs(const CWallet& wallet, const std::vectorInsert(input_coin, output.depth, output.from_me, ancestors, descendants, positive_only); + // Add the output to group + group->Insert(output, ancestors, descendants, positive_only); } // Now we go through the entire map and pull out the OutputGroups @@ -427,10 +425,10 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec { for (const COutput& out : vCoins) { if (!out.spendable) continue; - /* Set depth, from_me, ancestors, and descendants to 0 or false as these don't matter for preset inputs as no actual selection is being done. + /* Set ancestors and descendants to 0 as these don't matter for preset inputs as no actual selection is being done. * positive_only is set to false because we want to include all preset inputs, even if they are dust. */ - preset_inputs.Insert(out.GetInputCoin(), 0, false, 0, 0, false); + preset_inputs.Insert(out, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); } SelectionResult result(nTargetValue); result.AddInput(preset_inputs); @@ -439,7 +437,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec } // calculate value from preset inputs and store them - std::set setPresetCoins; + std::set preset_coins; std::vector vPresetInputs; coin_control.ListSelected(vPresetInputs); @@ -467,27 +465,29 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0); } - CInputCoin coin(outpoint, txout, input_bytes); - if (coin.m_input_bytes == -1) { + if (input_bytes == -1) { return std::nullopt; // Not solvable, can't estimate size for fee } - coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); + + /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */ + COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false); + output.effective_value = output.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(output.input_bytes); if (coin_selection_params.m_subtract_fee_outputs) { - value_to_select -= coin.txout.nValue; + value_to_select -= output.txout.nValue; } else { - value_to_select -= coin.effective_value; + value_to_select -= output.effective_value; } - setPresetCoins.insert(coin); - /* Set depth, from_me, ancestors, and descendants to 0 or false as don't matter for preset inputs as no actual selection is being done. + preset_coins.insert(outpoint); + /* Set ancestors and descendants to 0 as they don't matter for preset inputs since no actual selection is being done. * positive_only is set to false because we want to include all preset inputs, even if they are dust. */ - preset_inputs.Insert(coin, 0, false, 0, 0, false); + preset_inputs.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); } // remove preset inputs from vCoins so that Coin Selection doesn't pick them. for (std::vector::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();) { - if (setPresetCoins.count(it->GetInputCoin())) + if (preset_coins.count(it->outpoint)) it = vCoins.erase(it); else ++it; @@ -802,7 +802,7 @@ static bool CreateTransactionInternal( auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); // Shuffle selected coins and fill in final vin - std::vector selected_coins = result->GetShuffledInputVector(); + std::vector selected_coins = result->GetShuffledInputVector(); // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index fb246a3377c..bd5ed09ba43 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -28,20 +28,20 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup) // we repeat those tests this many times and only complain if all iterations of the test fail #define RANDOM_REPEATS 5 -typedef std::set CoinSet; +typedef std::set CoinSet; static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); static int nextLockTime = 0; -static void add_coin(const CAmount& nValue, int nInput, std::vector& set) +static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { CMutableTransaction tx; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; tx.nLockTime = nextLockTime++; // so all transactions get different hashes - set.emplace_back(MakeTransactionRef(tx), nInput); + set.emplace_back(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false); } static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) @@ -50,9 +50,9 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; tx.nLockTime = nextLockTime++; // so all transactions get different hashes - CInputCoin coin(MakeTransactionRef(tx), nInput); + COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false); OutputGroup group; - group.Insert(coin, 1, false, 0, 0, true); + group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ true); result.AddInput(group); } @@ -62,10 +62,10 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; tx.nLockTime = nextLockTime++; // so all transactions get different hashes - CInputCoin coin(MakeTransactionRef(tx), nInput); + COutput coin(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false); coin.effective_value = nValue - fee; - coin.m_fee = fee; - coin.m_long_term_fee = long_term_fee; + coin.fee = fee; + coin.long_term_fee = long_term_fee; set.insert(coin); } @@ -117,7 +117,7 @@ static bool EqualResult(const SelectionResult& a, const SelectionResult& b) return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); } -static CAmount make_hard_case(int utxos, std::vector& utxo_pool) +static CAmount make_hard_case(int utxos, std::vector& utxo_pool) { utxo_pool.clear(); CAmount target = 0; @@ -129,24 +129,13 @@ static CAmount make_hard_case(int utxos, std::vector& utxo_pool) return target; } -inline std::vector& GroupCoins(const std::vector& coins) -{ - static std::vector static_groups; - static_groups.clear(); - for (auto& coin : coins) { - static_groups.emplace_back(); - static_groups.back().Insert(coin, 0, true, 0, 0, false); - } - return static_groups; -} - inline std::vector& GroupCoins(const std::vector& coins) { static std::vector static_groups; static_groups.clear(); for (auto& coin : coins) { static_groups.emplace_back(); - static_groups.back().Insert(coin.GetInputCoin(), coin.depth, coin.from_me, 0, 0, false); + static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); } return static_groups; } @@ -166,7 +155,7 @@ inline std::vector& KnapsackGroupOutputs(const std::vector BOOST_AUTO_TEST_CASE(bnb_search_test) { // Setup - std::vector utxo_pool; + std::vector utxo_pool; SelectionResult expected_result(CAmount(0)); ///////////////////////// From f6c39c6adb6cbf9c87f04d3d667701905ef5c0a0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Jan 2022 17:21:36 -0500 Subject: [PATCH 12/13] coinselection: Remove CInputCoin It is no longer needed as everything it was doing is now done by COutput --- src/bench/coin_selection.cpp | 3 -- src/wallet/coinselection.h | 58 ------------------------------------ 2 files changed, 61 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index b332ef9c11a..85d8ca774aa 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -13,7 +13,6 @@ using node::NodeContext; using wallet::AttemptSelection; -using wallet::CInputCoin; using wallet::COutput; using wallet::CWallet; using wallet::CWalletTx; @@ -74,8 +73,6 @@ static void CoinSelection(benchmark::Bench& bench) }); } -typedef std::set CoinSet; - // Copied from src/wallet/test/coinselector_tests.cpp static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 4d39be364cb..ff85dd5fe2f 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -19,59 +19,6 @@ static constexpr CAmount MIN_CHANGE{COIN / 100}; static const CAmount MIN_FINAL_CHANGE = MIN_CHANGE/2; /** A UTXO under consideration for use in funding a new transaction. */ -class CInputCoin { -public: - CInputCoin(const CTransactionRef& tx, unsigned int i) - { - if (!tx) - throw std::invalid_argument("tx should not be null"); - if (i >= tx->vout.size()) - throw std::out_of_range("The output index is out of range"); - - outpoint = COutPoint(tx->GetHash(), i); - txout = tx->vout[i]; - effective_value = txout.nValue; - } - - CInputCoin(const CTransactionRef& tx, unsigned int i, int input_bytes) : CInputCoin(tx, i) - { - m_input_bytes = input_bytes; - } - - CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in) - { - outpoint = outpoint_in; - txout = txout_in; - effective_value = txout.nValue; - } - - CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in, int input_bytes) : CInputCoin(outpoint_in, txout_in) - { - m_input_bytes = input_bytes; - } - - COutPoint outpoint; - CTxOut txout; - CAmount effective_value; - CAmount m_fee{0}; - CAmount m_long_term_fee{0}; - - /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */ - int m_input_bytes{-1}; - - bool operator<(const CInputCoin& rhs) const { - return outpoint < rhs.outpoint; - } - - bool operator!=(const CInputCoin& rhs) const { - return outpoint != rhs.outpoint; - } - - bool operator==(const CInputCoin& rhs) const { - return outpoint == rhs.outpoint; - } -}; - class COutput { public: @@ -134,11 +81,6 @@ public: std::string ToString() const; - inline CInputCoin GetInputCoin() const - { - return CInputCoin(outpoint, txout, input_bytes); - } - bool operator<(const COutput& rhs) const { return outpoint < rhs.outpoint; } From 049003fe68a4183f6f20da16f58f10079d1e02df Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 23 Mar 2022 14:38:14 -0400 Subject: [PATCH 13/13] coinselection: Remove COutput operators == and != These operators are used only by the tests in std::mismatch. As std::mismatch can take a binary predicate, we can use a lambda that achieves the same instead. --- src/wallet/coinselection.h | 8 -------- src/wallet/test/coinselector_tests.cpp | 5 ++++- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index ff85dd5fe2f..43c37cbad70 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -84,14 +84,6 @@ public: bool operator<(const COutput& rhs) const { return outpoint < rhs.outpoint; } - - bool operator!=(const COutput& rhs) const { - return outpoint != rhs.outpoint; - } - - bool operator==(const COutput& rhs) const { - return outpoint == rhs.outpoint; - } }; /** Parameters for one iteration of Coin Selection. */ diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index bd5ed09ba43..51e35a86af2 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -113,7 +113,10 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) /** Check if this selection is equal to another one. Equal means same inputs (i.e same value and prevout) */ static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { - std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin()); + std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), + [](const COutput& a, const COutput& b) { + return a.outpoint == b.outpoint; + }); return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); }