From f5649db9d5e984ba7f376ccfd5b0a627f5c42402 Mon Sep 17 00:00:00 2001 From: josibake Date: Thu, 28 Jul 2022 16:19:56 +0200 Subject: [PATCH 1/7] refactor: add UNKNOWN OutputType add to enum, array and handle UNKNOWN in various case statements --- src/outputtype.cpp | 10 ++++++++-- src/outputtype.h | 2 ++ src/wallet/scriptpubkeyman.cpp | 5 +++++ src/wallet/wallet.cpp | 1 + 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/outputtype.cpp b/src/outputtype.cpp index 19366295e68..08c2ab4e309 100644 --- a/src/outputtype.cpp +++ b/src/outputtype.cpp @@ -20,6 +20,7 @@ static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy"; static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit"; static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32"; static const std::string OUTPUT_TYPE_STRING_BECH32M = "bech32m"; +static const std::string OUTPUT_TYPE_STRING_UNKNOWN = "unknown"; std::optional ParseOutputType(const std::string& type) { @@ -31,6 +32,8 @@ std::optional ParseOutputType(const std::string& type) return OutputType::BECH32; } else if (type == OUTPUT_TYPE_STRING_BECH32M) { return OutputType::BECH32M; + } else if (type == OUTPUT_TYPE_STRING_UNKNOWN) { + return OutputType::UNKNOWN; } return std::nullopt; } @@ -42,6 +45,7 @@ const std::string& FormatOutputType(OutputType type) case OutputType::P2SH_SEGWIT: return OUTPUT_TYPE_STRING_P2SH_SEGWIT; case OutputType::BECH32: return OUTPUT_TYPE_STRING_BECH32; case OutputType::BECH32M: return OUTPUT_TYPE_STRING_BECH32M; + case OutputType::UNKNOWN: return OUTPUT_TYPE_STRING_UNKNOWN; } // no default case, so the compiler can warn about missing cases assert(false); } @@ -61,7 +65,8 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type) return witdest; } } - case OutputType::BECH32M: {} // This function should never be used with BECH32M, so let it assert + case OutputType::BECH32M: + case OutputType::UNKNOWN: {} // This function should never be used with BECH32M or UNKNOWN, so let it assert } // no default case, so the compiler can warn about missing cases assert(false); } @@ -101,7 +106,8 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, return ScriptHash(witprog); } } - case OutputType::BECH32M: {} // This function should not be used for BECH32M, so let it assert + case OutputType::BECH32M: + case OutputType::UNKNOWN: {} // This function should not be used for BECH32M or UNKNOWN, so let it assert } // no default case, so the compiler can warn about missing cases assert(false); } diff --git a/src/outputtype.h b/src/outputtype.h index 6b4e6957601..be5fd62b806 100644 --- a/src/outputtype.h +++ b/src/outputtype.h @@ -19,6 +19,7 @@ enum class OutputType { P2SH_SEGWIT, BECH32, BECH32M, + UNKNOWN, }; static constexpr auto OUTPUT_TYPES = std::array{ @@ -26,6 +27,7 @@ static constexpr auto OUTPUT_TYPES = std::array{ OutputType::P2SH_SEGWIT, OutputType::BECH32, OutputType::BECH32M, + OutputType::UNKNOWN, }; std::optional ParseOutputType(const std::string& str); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1682ce2eef8..3cf289b1cd4 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1966,6 +1966,11 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ desc_prefix = "tr(" + xpub + "/86'"; break; } + case OutputType::UNKNOWN: { + // We should never have a DescriptorScriptPubKeyMan for an UNKNOWN OutputType, + // so if we get to this point something is wrong + assert(false); + } } // no default case, so the compiler can warn about missing cases assert(!desc_prefix.empty()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0a2997b3f1b..b01c03eb537 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3421,6 +3421,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() for (bool internal : {false, true}) { for (OutputType t : OUTPUT_TYPES) { + if (t == OutputType::UNKNOWN) continue; auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this)); if (IsCrypted()) { if (IsLocked()) { From 3f27a2adce12c6b0e7b43ba7c024331657bcf335 Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 10:01:25 +0200 Subject: [PATCH 2/7] refactor: add new helper methods add Shuffle, Erase, and Add to CoinsResult struct add a helper function for mapping TxoutType to OutputType Co-authored-by: furszy --- src/wallet/spend.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ src/wallet/spend.h | 4 ++++ 2 files changed, 45 insertions(+) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index b359d3e19b2..e9516d480df 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -105,6 +105,47 @@ void CoinsResult::clear() other.clear(); } +void CoinsResult::Erase(std::set& preset_coins) +{ + for (auto& it : coins) { + auto& vec = it.second; + auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);}); + if (i != vec.end()) { + vec.erase(i); + break; + } + } +} + +void CoinsResult::Shuffle(FastRandomContext& rng_fast) +{ + for (auto& it : coins) { + ::Shuffle(it.second.begin(), it.second.end(), rng_fast); + } +} + +void CoinsResult::Add(OutputType type, const COutput& out) +{ + coins[type].emplace_back(out); +} + +static OutputType GetOutputType(TxoutType type, bool is_from_p2sh) +{ + switch (type) { + case TxoutType::WITNESS_V1_TAPROOT: + return OutputType::BECH32M; + case TxoutType::WITNESS_V0_KEYHASH: + case TxoutType::WITNESS_V0_SCRIPTHASH: + if (is_from_p2sh) return OutputType::P2SH_SEGWIT; + else return OutputType::BECH32; + case TxoutType::SCRIPTHASH: + case TxoutType::PUBKEYHASH: + return OutputType::LEGACY; + default: + return OutputType::UNKNOWN; + } +} + CoinsResult AvailableCoins(const CWallet& wallet, const CCoinControl* coinControl, std::optional feerate, diff --git a/src/wallet/spend.h b/src/wallet/spend.h index fdb5113ba4a..dea5e7c65f4 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -38,6 +38,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle * the CoinsResult struct as if it was a vector */ struct CoinsResult { + std::map> coins; /** Vectors for each OutputType */ std::vector legacy; std::vector P2SH_segwit; @@ -54,6 +55,9 @@ struct CoinsResult { * i.e., methods can work with individual OutputType vectors or on the entire object */ uint64_t size() const; void clear(); + void Erase(std::set& preset_coins); + void Shuffle(FastRandomContext& rng_fast); + void Add(OutputType type, const COutput& out); /** Sum of all available coins */ CAmount total_amount{0}; From b6b50b0f2b055d81c5d4ff9e21dd88cdc9a88ccb Mon Sep 17 00:00:00 2001 From: josibake Date: Wed, 10 Aug 2022 14:35:53 +0200 Subject: [PATCH 3/7] scripted-diff: Uppercase function names Change `CoinsResult` functions to uppercase to be consistent with the style guide. -BEGIN VERIFY SCRIPT- git grep -l "available_coins" | grep -v mempool_stress.cpp | xargs sed -i "s/available_coins\.\(size\|all\|clear\)/available_coins\.\u\1/" git grep -l AvailableCoins | xargs sed -i "/AvailableCoins/ s/\(all()\|size()\|clear()\)/\u\1/" sed -i "s/\(clear()\|all()\|size()\)/\u&/g" src/wallet/spend.h sed -i "/CoinsResult::/ s/\(clear()\|all()\|size()\)/\u&/" src/wallet/spend.cpp sed -i "s/result.size/result.Size/" src/wallet/spend.cpp sed -i "s/this->size/this->Size/" src/wallet/spend.cpp -END VERIFY SCRIPT- --- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 2 +- src/wallet/spend.cpp | 16 ++-- src/wallet/spend.h | 8 +- src/wallet/test/availablecoins_tests.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 110 +++++++++++------------ src/wallet/test/wallet_tests.cpp | 4 +- 7 files changed, 72 insertions(+), 72 deletions(-) diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index d40d800f285..e962fffec58 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -638,7 +638,7 @@ RPCHelpMan listunspent() cctl.m_max_depth = nMaxDepth; cctl.m_include_unsafe_inputs = include_unsafe; LOCK(pwallet->cs_wallet); - vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).all(); + vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).All(); } LOCK(pwallet->cs_wallet); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 628bf3cc998..6c212cbc9b1 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1382,7 +1382,7 @@ RPCHelpMan sendall() total_input_value += tx->tx->vout[input.prevout.n].nValue; } } else { - for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).all()) { + for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).All()) { CHECK_NONFATAL(output.input_bytes > 0); if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue) { continue; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e9516d480df..3376b59e46b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -79,15 +79,15 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control); } -uint64_t CoinsResult::size() const +uint64_t CoinsResult::Size() const { return bech32m.size() + bech32.size() + P2SH_segwit.size() + legacy.size() + other.size(); } -std::vector CoinsResult::all() const +std::vector CoinsResult::All() const { std::vector all; - all.reserve(this->size()); + all.reserve(this->Size()); all.insert(all.end(), bech32m.begin(), bech32m.end()); all.insert(all.end(), bech32.begin(), bech32.end()); all.insert(all.end(), P2SH_segwit.begin(), P2SH_segwit.end()); @@ -96,7 +96,7 @@ std::vector CoinsResult::all() const return all; } -void CoinsResult::clear() +void CoinsResult::Clear() { bech32m.clear(); bech32.clear(); @@ -319,7 +319,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, } // Checks the maximum number of UTXO's. - if (nMaximumCount > 0 && result.size() >= nMaximumCount) { + if (nMaximumCount > 0 && result.Size() >= nMaximumCount) { return result; } } @@ -375,7 +375,7 @@ std::map> ListCoins(const CWallet& wallet) std::map> result; - for (const COutput& coin : AvailableCoinsListUnspent(wallet).all()) { + for (const COutput& coin : AvailableCoinsListUnspent(wallet).All()) { CTxDestination address; if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { @@ -515,7 +515,7 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm // over all available coins, else pick the best solution from the results if (results.size() == 0) { if (allow_mixed_output_types) { - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params)}) { + if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params)}) { return result; } } @@ -649,7 +649,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a // form groups from remaining coins; note that preset coins will not // automatically have their associated (same address) coins included - if (coin_control.m_avoid_partial_spends && available_coins.size() > OUTPUT_GROUP_MAX_ENTRIES) { + if (coin_control.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) { // Cases where we have 101+ outputs all pointing to the same destination may result in // privacy leaks as they will potentially be deterministically sorted. We solve that by // explicitly shuffling the outputs before processing diff --git a/src/wallet/spend.h b/src/wallet/spend.h index dea5e7c65f4..e11a126cdad 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -34,7 +34,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle * This struct is really just a wrapper around OutputType vectors with a convenient * method for concatenating and returning all COutputs as one vector. * - * clear(), size() methods are implemented so that one can interact with + * Clear(), Size() methods are implemented so that one can interact with * the CoinsResult struct as if it was a vector */ struct CoinsResult { @@ -49,12 +49,12 @@ struct CoinsResult { std::vector other; /** Concatenate and return all COutputs as one vector */ - std::vector all() const; + std::vector All() const; /** The following methods are provided so that CoinsResult can mimic a vector, * i.e., methods can work with individual OutputType vectors or on the entire object */ - uint64_t size() const; - void clear(); + uint64_t Size() const; + void Clear(); void Erase(std::set& preset_coins); void Shuffle(FastRandomContext& rng_fast); void Add(OutputType type, const COutput& out); diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index 33561281128..cc85b345787 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/src/wallet/test/availablecoins_tests.cpp @@ -63,7 +63,7 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) // Verify our wallet has one usable coinbase UTXO before starting // This UTXO is a P2PK, so it should show up in the Other bucket available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.size(), 1U); + BOOST_CHECK_EQUAL(available_coins.Size(), 1U); BOOST_CHECK_EQUAL(available_coins.other.size(), 1U); // We will create a self transfer for each of the OutputTypes and diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index a4a72164366..61aa6db9f47 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -308,15 +308,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinsResult available_coins; add_coin(available_coins, *wallet, 1, coin_selection_params_bnb.m_effective_feerate); - available_coins.all().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(available_coins.all()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change)); + available_coins.All().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(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change)); // Test fees subtracted from output: - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, 1 * CENT, coin_selection_params_bnb.m_effective_feerate); - available_coins.all().at(0).input_bytes = 40; + available_coins.All().at(0).input_bytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.all()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change); + const auto result9 = SelectCoinsBnB(GroupCoins(available_coins.All()), 1 * CENT, coin_selection_params_bnb.m_cost_of_change); BOOST_CHECK(result9); BOOST_CHECK_EQUAL(result9->GetSelectedValue(), 1 * CENT); } @@ -335,7 +335,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); CCoinControl coin_control; coin_control.m_allow_other_inputs = true; - coin_control.Select(available_coins.all().at(0).outpoint); + coin_control.Select(available_coins.All().at(0).outpoint); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); const auto result10 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); @@ -362,7 +362,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CCoinControl coin_control; const auto result11 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result11)); - available_coins.clear(); + available_coins.Clear(); // more coins should be selected when effective fee < long term fee coin_selection_params_bnb.m_effective_feerate = CFeeRate(3000); @@ -377,7 +377,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(1 * CENT, 2, expected_result); const auto result12 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result12)); - available_coins.clear(); + available_coins.Clear(); // pre selected coin should be selected even if disadvantageous coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000); @@ -391,7 +391,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(9 * CENT, 2, expected_result); add_coin(1 * CENT, 2, expected_result); coin_control.m_allow_other_inputs = true; - coin_control.Select(available_coins.all().at(1).outpoint); // pre select 9 coin + coin_control.Select(available_coins.All().at(1).outpoint); // pre select 9 coin const auto result13 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13)); } @@ -413,28 +413,28 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // test multiple times to allow for differences in the shuffle order for (int i = 0; i < RUN_TESTS; i++) { - available_coins.clear(); + available_coins.Clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); add_coin(available_coins, *wallet, 1*CENT, CFeeRate(0), 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 1 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1 * CENT, CENT)); // but we can find a new 1 cent - const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result1 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result1); BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); add_coin(available_coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 3 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 3 * CENT, CENT)); // we can make 3 cents of new coins - const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 3 * CENT, CENT); + const auto result2 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 3 * CENT, CENT); BOOST_CHECK(result2); BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 3 * CENT); @@ -445,44 +445,44 @@ 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(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 38 * CENT, CENT)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard_extra), 38 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard_extra), 38 * CENT, CENT)); // but we can make 37 cents if we accept new coins from ourself - const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 37 * CENT, CENT); + const auto result3 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 37 * CENT, CENT); BOOST_CHECK(result3); BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 37 * CENT); // and we can make 38 cents if we accept all new coins - const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 38 * CENT, CENT); + const auto result4 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 38 * CENT, CENT); BOOST_CHECK(result4); BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 34 * CENT, CENT); + const auto result5 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 34 * CENT, CENT); BOOST_CHECK(result5); BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(result5->GetInputSet().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 - const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 7 * CENT, CENT); + const auto result6 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 7 * CENT, CENT); BOOST_CHECK(result6); BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 7 * CENT); BOOST_CHECK_EQUAL(result6->GetInputSet().size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 8 * CENT, CENT); + const auto result7 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 8 * CENT, CENT); BOOST_CHECK(result7); BOOST_CHECK(result7->GetSelectedValue() == 8 * CENT); BOOST_CHECK_EQUAL(result7->GetInputSet().size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 9 * CENT, CENT); + const auto result8 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 9 * CENT, CENT); BOOST_CHECK(result8); BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 10 * CENT); BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U); // now clear out the wallet and start again to test choosing between subsets of smaller coins and the next biggest coin - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, 6*CENT); add_coin(available_coins, *wallet, 7*CENT); @@ -491,12 +491,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 71 * CENT, CENT); + const auto result9 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 71 * CENT, CENT); BOOST_CHECK(result9); - BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 72 * CENT, CENT)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 72 * CENT, CENT)); // 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 - const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result10 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result10); BOOST_CHECK_EQUAL(result10->GetSelectedValue(), 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(result10->GetInputSet().size(), 1U); @@ -504,7 +504,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 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 - const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result11 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result11); BOOST_CHECK_EQUAL(result11->GetSelectedValue(), 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(result11->GetInputSet().size(), 3U); @@ -512,13 +512,13 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 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 - const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 16 * CENT, CENT); + const auto result12 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 16 * CENT, CENT); BOOST_CHECK(result12); BOOST_CHECK_EQUAL(result12->GetSelectedValue(), 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(result12->GetInputSet().size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 11 * CENT, CENT); + const auto result13 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 11 * CENT, CENT); BOOST_CHECK(result13); BOOST_CHECK_EQUAL(result13->GetSelectedValue(), 11 * CENT); BOOST_CHECK_EQUAL(result13->GetInputSet().size(), 2U); @@ -528,19 +528,19 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 2*COIN); add_coin(available_coins, *wallet, 3*COIN); add_coin(available_coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 95 * CENT, CENT); + const auto result14 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 95 * CENT, CENT); BOOST_CHECK(result14); BOOST_CHECK_EQUAL(result14->GetSelectedValue(), 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(result14->GetInputSet().size(), 1U); - const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 195 * CENT, CENT); + const auto result15 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 195 * CENT, CENT); BOOST_CHECK(result15); BOOST_CHECK_EQUAL(result15->GetSelectedValue(), 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(result15->GetInputSet().size(), 1U); // empty the wallet and start again, now with fractions of a cent, to test small change avoidance - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, CENT * 1 / 10); add_coin(available_coins, *wallet, CENT * 2 / 10); add_coin(available_coins, *wallet, CENT * 3 / 10); @@ -549,7 +549,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * CENT from the 1.5 * CENT // we'll get change smaller than CENT whatever happens, so can expect CENT exactly - const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), CENT, CENT); + const auto result16 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result16); BOOST_CHECK_EQUAL(result16->GetSelectedValue(), CENT); @@ -557,7 +557,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, 1111*CENT); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result17 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result17); BOOST_CHECK_EQUAL(result17->GetSelectedValue(), 1 * CENT); // we should get the exact amount @@ -566,17 +566,17 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(available_coins, *wallet, CENT * 7 / 10); // and try again to make 1.0 * CENT - const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result18 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result18); BOOST_CHECK_EQUAL(result18->GetSelectedValue(), 1 * CENT); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) // they tried to consolidate 10 50k coins into one 500k coin, and ended up with 50k in change - available_coins.clear(); + available_coins.Clear(); for (int j = 0; j < 20; j++) add_coin(available_coins, *wallet, 50000 * COIN); - const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 500000 * COIN, CENT); + const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 500000 * COIN, CENT); BOOST_CHECK(result19); BOOST_CHECK_EQUAL(result19->GetSelectedValue(), 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(result19->GetInputSet().size(), 10U); // in ten coins @@ -585,41 +585,41 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // we need to try finding an exact subset anyway // sometimes it will fail, and so we use the next biggest coin: - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, CENT * 5 / 10); add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 7 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 1 * CENT, CENT); + const auto result20 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 1 * CENT, CENT); BOOST_CHECK(result20); BOOST_CHECK_EQUAL(result20->GetSelectedValue(), 1111 * CENT); // we get the bigger coin BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U); // but sometimes it's possible, and we use an exact subset (0.4 + 0.6 = 1.0) - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, CENT * 4 / 10); add_coin(available_coins, *wallet, CENT * 6 / 10); add_coin(available_coins, *wallet, CENT * 8 / 10); add_coin(available_coins, *wallet, 1111 * CENT); - const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), CENT, CENT); + const auto result21 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT, CENT); BOOST_CHECK(result21); BOOST_CHECK_EQUAL(result21->GetSelectedValue(), CENT); // we should get the exact amount BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6 // test avoiding small change - available_coins.clear(); + available_coins.Clear(); add_coin(available_coins, *wallet, CENT * 5 / 100); add_coin(available_coins, *wallet, CENT * 1); add_coin(available_coins, *wallet, CENT * 100); // trying to make 100.01 from these three coins - const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), CENT * 10001 / 100, CENT); + const auto result22 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 10001 / 100, CENT); BOOST_CHECK(result22); BOOST_CHECK_EQUAL(result22->GetSelectedValue(), CENT * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(result22->GetInputSet().size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), CENT * 9990 / 100, CENT); + const auto result23 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), CENT * 9990 / 100, CENT); BOOST_CHECK(result23); BOOST_CHECK_EQUAL(result23->GetSelectedValue(), 101 * CENT); BOOST_CHECK_EQUAL(result23->GetInputSet().size(), 2U); @@ -627,14 +627,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // test with many inputs for (CAmount amt=1500; amt < COIN; amt*=10) { - available_coins.clear(); + available_coins.Clear(); // Create 676 inputs (= (old MAX_STANDARD_TX_SIZE == 100000) / 148 bytes per input) for (uint16_t j = 0; j < 676; j++) add_coin(available_coins, *wallet, amt); // 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++) { - const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_confirmed), 2000, CENT); + const auto result24 = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_confirmed), 2000, CENT); BOOST_CHECK(result24); if (amt - 2000 < CENT) { @@ -653,7 +653,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // test randomness { - available_coins.clear(); + available_coins.Clear(); for (int i2 = 0; i2 < 100; i2++) add_coin(available_coins, *wallet, COIN); @@ -661,9 +661,9 @@ 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 - const auto result25 = KnapsackSolver(GroupCoins(available_coins.all()), 50 * COIN, CENT); + const auto result25 = KnapsackSolver(GroupCoins(available_coins.All()), 50 * COIN, CENT); BOOST_CHECK(result25); - const auto result26 = KnapsackSolver(GroupCoins(available_coins.all()), 50 * COIN, CENT); + const auto result26 = KnapsackSolver(GroupCoins(available_coins.All()), 50 * COIN, CENT); BOOST_CHECK(result26); BOOST_CHECK(!EqualResult(*result25, *result26)); @@ -674,9 +674,9 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // When choosing 1 from 100 identical coins, 1% of the time, this test will choose the same coin twice // which will cause it to fail. // To avoid that issue, run the test RANDOM_REPEATS times and only complain if all of them fail - const auto result27 = KnapsackSolver(GroupCoins(available_coins.all()), COIN, CENT); + const auto result27 = KnapsackSolver(GroupCoins(available_coins.All()), COIN, CENT); BOOST_CHECK(result27); - const auto result28 = KnapsackSolver(GroupCoins(available_coins.all()), COIN, CENT); + const auto result28 = KnapsackSolver(GroupCoins(available_coins.All()), COIN, CENT); BOOST_CHECK(result28); if (EqualResult(*result27, *result28)) fails++; @@ -697,9 +697,9 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - const auto result29 = KnapsackSolver(GroupCoins(available_coins.all()), 90 * CENT, CENT); + const auto result29 = KnapsackSolver(GroupCoins(available_coins.All()), 90 * CENT, CENT); BOOST_CHECK(result29); - const auto result30 = KnapsackSolver(GroupCoins(available_coins.all()), 90 * CENT, CENT); + const auto result30 = KnapsackSolver(GroupCoins(available_coins.All()), 90 * CENT, CENT); BOOST_CHECK(result30); if (EqualResult(*result29, *result30)) fails++; @@ -725,7 +725,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(available_coins, *wallet, 1000 * COIN); add_coin(available_coins, *wallet, 3 * COIN); - const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins.all(), *wallet, filter_standard), 1003 * COIN, CENT, rand); + const auto result = KnapsackSolver(KnapsackGroupOutputs(available_coins.All(), *wallet, filter_standard), 1003 * COIN, CENT, rand); BOOST_CHECK(result); BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN); BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 199925b4b18..30209304b41 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -591,7 +591,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) // Lock both coins. Confirm number of available coins drops to 0. { LOCK(wallet->cs_wallet); - BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).size(), 2U); + BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).Size(), 2U); } for (const auto& group : list) { for (const auto& coin : group.second) { @@ -601,7 +601,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) } { LOCK(wallet->cs_wallet); - BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).size(), 0U); + BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).Size(), 0U); } // Confirm ListCoins still returns same result as before, despite coins // being locked. From db09aec9378c5e8cc49c866fa50bfcb6c567d66c Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 10:35:50 +0200 Subject: [PATCH 4/7] wallet: switch to new shuffle, erase, push_back switch to new methods, remove old code. this also updates the Size, All, and Clear methods to now use the coins map. this commit is not strictly a refactor because previously coin selection was never run over the UNKNOWN type until the last step when being run over all. now that we are iterating over each, it is run over UNKNOWN but this is expected to be empty most of the time. Co-authored-by: furszy --- src/bench/coin_selection.cpp | 2 +- src/wallet/spend.cpp | 95 ++++++++---------------- src/wallet/spend.h | 14 +--- src/wallet/test/availablecoins_tests.cpp | 10 +-- src/wallet/test/coinselector_tests.cpp | 2 +- 5 files changed, 39 insertions(+), 84 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index a80ec3703c8..6ada28115e3 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -59,7 +59,7 @@ static void CoinSelection(benchmark::Bench& bench) wallet::CoinsResult available_coins; for (const auto& wtx : wtxs) { const auto txout = wtx->tx->vout.at(0); - available_coins.bech32.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0); + available_coins.coins[OutputType::BECH32].emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0); } const CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 3376b59e46b..d161393995a 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -79,30 +79,27 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control); } -uint64_t CoinsResult::Size() const +size_t CoinsResult::Size() const { - return bech32m.size() + bech32.size() + P2SH_segwit.size() + legacy.size() + other.size(); + size_t size{0}; + for (const auto& it : coins) { + size += it.second.size(); + } + return size; } std::vector CoinsResult::All() const { std::vector all; - all.reserve(this->Size()); - all.insert(all.end(), bech32m.begin(), bech32m.end()); - all.insert(all.end(), bech32.begin(), bech32.end()); - all.insert(all.end(), P2SH_segwit.begin(), P2SH_segwit.end()); - all.insert(all.end(), legacy.begin(), legacy.end()); - all.insert(all.end(), other.begin(), other.end()); + all.reserve(coins.size()); + for (const auto& it : coins) { + all.insert(all.end(), it.second.begin(), it.second.end()); + } return all; } -void CoinsResult::Clear() -{ - bech32m.clear(); - bech32.clear(); - P2SH_segwit.clear(); - legacy.clear(); - other.clear(); +void CoinsResult::Clear() { + coins.clear(); } void CoinsResult::Erase(std::set& preset_coins) @@ -263,51 +260,32 @@ CoinsResult AvailableCoins(const CWallet& wallet, // Filter by spendable outputs only if (!spendable && only_spendable) continue; - // When parsing a scriptPubKey, Solver returns the parsed pubkeys or hashes (depending on the script) - // We don't need those here, so we are leaving them in return_values_unused - std::vector> return_values_unused; - TxoutType type; - bool is_from_p2sh{false}; - // If the Output is P2SH and spendable, we want to know if it is // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine // this from the redeemScript. If the Output is not spendable, it will be classified // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript + CScript script; + bool is_from_p2sh{false}; if (output.scriptPubKey.IsPayToScriptHash() && solvable) { - CScript redeemScript; CTxDestination destination; if (!ExtractDestination(output.scriptPubKey, destination)) continue; const CScriptID& hash = CScriptID(std::get(destination)); - if (!provider->GetCScript(hash, redeemScript)) + if (!provider->GetCScript(hash, script)) continue; - type = Solver(redeemScript, return_values_unused); is_from_p2sh = true; } else { - type = Solver(output.scriptPubKey, return_values_unused); + script = output.scriptPubKey; } COutput coin(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate); - switch (type) { - case TxoutType::WITNESS_UNKNOWN: - case TxoutType::WITNESS_V1_TAPROOT: - result.bech32m.push_back(coin); - break; - case TxoutType::WITNESS_V0_KEYHASH: - case TxoutType::WITNESS_V0_SCRIPTHASH: - if (is_from_p2sh) { - result.P2SH_segwit.push_back(coin); - break; - } - result.bech32.push_back(coin); - break; - case TxoutType::SCRIPTHASH: - case TxoutType::PUBKEYHASH: - result.legacy.push_back(coin); - break; - default: - result.other.push_back(coin); - }; + + // When parsing a scriptPubKey, Solver returns the parsed pubkeys or hashes (depending on the script) + // We don't need those here, so we are leaving them in return_values_unused + std::vector> return_values_unused; + TxoutType type; + type = Solver(script, return_values_unused); + result.Add(GetOutputType(type, is_from_p2sh), coin); // Cache total amount as we go result.total_amount += output.nValue; @@ -498,17 +476,10 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm { // Run coin selection on each OutputType and compute the Waste Metric std::vector results; - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params)}) { - results.push_back(*result); - } - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.P2SH_segwit, coin_selection_params)}) { - results.push_back(*result); - } - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.bech32, coin_selection_params)}) { - results.push_back(*result); - } - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.bech32m, coin_selection_params)}) { - results.push_back(*result); + for (const auto& it : available_coins.coins) { + if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)}) { + results.push_back(*result); + } } // If we can't fund the transaction from any individual OutputType, run coin selection @@ -633,11 +604,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a // remove preset inputs from coins so that Coin Selection doesn't pick them. if (coin_control.HasSelected()) { - available_coins.legacy.erase(remove_if(available_coins.legacy.begin(), available_coins.legacy.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.legacy.end()); - available_coins.P2SH_segwit.erase(remove_if(available_coins.P2SH_segwit.begin(), available_coins.P2SH_segwit.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.P2SH_segwit.end()); - available_coins.bech32.erase(remove_if(available_coins.bech32.begin(), available_coins.bech32.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.bech32.end()); - available_coins.bech32m.erase(remove_if(available_coins.bech32m.begin(), available_coins.bech32m.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.bech32m.end()); - available_coins.other.erase(remove_if(available_coins.other.begin(), available_coins.other.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.other.end()); + available_coins.Erase(preset_coins); } unsigned int limit_ancestor_count = 0; @@ -653,11 +620,7 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a // Cases where we have 101+ outputs all pointing to the same destination may result in // privacy leaks as they will potentially be deterministically sorted. We solve that by // explicitly shuffling the outputs before processing - Shuffle(available_coins.legacy.begin(), available_coins.legacy.end(), coin_selection_params.rng_fast); - Shuffle(available_coins.P2SH_segwit.begin(), available_coins.P2SH_segwit.end(), coin_selection_params.rng_fast); - Shuffle(available_coins.bech32.begin(), available_coins.bech32.end(), coin_selection_params.rng_fast); - Shuffle(available_coins.bech32m.begin(), available_coins.bech32m.end(), coin_selection_params.rng_fast); - Shuffle(available_coins.other.begin(), available_coins.other.end(), coin_selection_params.rng_fast); + available_coins.Shuffle(coin_selection_params.rng_fast); } // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the diff --git a/src/wallet/spend.h b/src/wallet/spend.h index e11a126cdad..c29e5be5c75 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -34,26 +34,18 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle * This struct is really just a wrapper around OutputType vectors with a convenient * method for concatenating and returning all COutputs as one vector. * - * Clear(), Size() methods are implemented so that one can interact with - * the CoinsResult struct as if it was a vector + * Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to + * allow easy interaction with the struct. */ struct CoinsResult { std::map> coins; - /** Vectors for each OutputType */ - std::vector legacy; - std::vector P2SH_segwit; - std::vector bech32; - std::vector bech32m; - - /** Other is a catch-all for anything that doesn't match the known OutputTypes */ - std::vector other; /** Concatenate and return all COutputs as one vector */ std::vector All() const; /** The following methods are provided so that CoinsResult can mimic a vector, * i.e., methods can work with individual OutputType vectors or on the entire object */ - uint64_t Size() const; + size_t Size() const; void Clear(); void Erase(std::set& preset_coins); void Shuffle(FastRandomContext& rng_fast); diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index cc85b345787..eeecee2e2ce 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/src/wallet/test/availablecoins_tests.cpp @@ -64,7 +64,7 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) // This UTXO is a P2PK, so it should show up in the Other bucket available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.Size(), 1U); - BOOST_CHECK_EQUAL(available_coins.other.size(), 1U); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::UNKNOWN].size(), 1U); // We will create a self transfer for each of the OutputTypes and // verify it is put in the correct bucket after running GetAvailablecoins @@ -78,27 +78,27 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) BOOST_ASSERT(dest); AddTx(CRecipient{{GetScriptForDestination(*dest)}, 1 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::BECH32M].size(), 2U); // Bech32 dest = wallet->GetNewDestination(OutputType::BECH32, ""); BOOST_ASSERT(dest); AddTx(CRecipient{{GetScriptForDestination(*dest)}, 2 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::BECH32].size(), 2U); // P2SH-SEGWIT dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, ""); AddTx(CRecipient{{GetScriptForDestination(*dest)}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::P2SH_SEGWIT].size(), 2U); // Legacy (P2PKH) dest = wallet->GetNewDestination(OutputType::LEGACY, ""); BOOST_ASSERT(dest); AddTx(CRecipient{{GetScriptForDestination(*dest)}, 4 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); - BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U); + BOOST_CHECK_EQUAL(available_coins.coins[OutputType::LEGACY].size(), 2U); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 61aa6db9f47..4dfdc87a380 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -83,7 +83,7 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun assert(ret.second); CWalletTx& wtx = (*ret.first).second; const auto& txout = wtx.tx->vout.at(nInput); - available_coins.bech32.emplace_back(COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate); + available_coins.coins[OutputType::BECH32].emplace_back(COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate); } /** Check if SelectionResult a is equivalent to SelectionResult b. From 0760ce0b9e646b6c86f4cc890c6ab78103a242ab Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 12:30:38 +0200 Subject: [PATCH 5/7] test: add missing BOOST_ASSERT this was missed in the original PR --- src/wallet/test/availablecoins_tests.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index eeecee2e2ce..c8adef7fc28 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/src/wallet/test/availablecoins_tests.cpp @@ -89,6 +89,7 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) // P2SH-SEGWIT dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, ""); + BOOST_ASSERT(dest); AddTx(CRecipient{{GetScriptForDestination(*dest)}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.coins[OutputType::P2SH_SEGWIT].size(), 2U); From f47ff717611182da27461e29b3c23933eb22fbce Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 12:31:40 +0200 Subject: [PATCH 6/7] test: only run test for descriptor wallets since this test uses bech32m, we skip unless sqlite is used, which is the same as checking if we are using descriptor wallets or not --- test/functional/wallet_avoid_mixing_output_types.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/functional/wallet_avoid_mixing_output_types.py b/test/functional/wallet_avoid_mixing_output_types.py index 46f41d9c226..cad9d028081 100755 --- a/test/functional/wallet_avoid_mixing_output_types.py +++ b/test/functional/wallet_avoid_mixing_output_types.py @@ -124,6 +124,7 @@ class AddressInputTypeGrouping(BitcoinTestFramework): def skip_test_if_missing_module(self): self.skip_if_no_wallet() + self.skip_if_no_sqlite() def make_payment(self, A, B, v, addr_type): fee_rate = random.randint(1, 20) From 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 12:42:40 +0200 Subject: [PATCH 7/7] refactor: improve readability for AttemptSelection it was pointed out by a few reviewers that the code block at the end of attempt selection was difficult to follow and lacked comments. refactor to get rid of triple nested if statement and improve readibility. --- src/wallet/spend.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d161393995a..8147ffcb697 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -481,19 +481,20 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm results.push_back(*result); } } + // If we have at least one solution for funding the transaction without mixing, choose the minimum one according to waste metric + // and return the result + if (results.size() > 0) return *std::min_element(results.begin(), results.end()); - // If we can't fund the transaction from any individual OutputType, run coin selection - // over all available coins, else pick the best solution from the results - if (results.size() == 0) { - if (allow_mixed_output_types) { - if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params)}) { - return result; - } + // If we can't fund the transaction from any individual OutputType, run coin selection one last time + // over all available coins, which would allow mixing + if (allow_mixed_output_types) { + if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params)}) { + return result; } - return std::optional(); - }; - std::optional result{*std::min_element(results.begin(), results.end())}; - return result; + } + // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't + // find a solution using all available coins + return std::nullopt; }; std::optional ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector& available_coins, const CoinSelectionParams& coin_selection_params)