From 832a3847cef7f4a9984c887641fcd3ee08b21b94 Mon Sep 17 00:00:00 2001 From: Murch Date: Thu, 7 Dec 2023 16:30:16 -0500 Subject: [PATCH] Move BnB SFFO restriction test --- src/wallet/test/coinselection_tests.cpp | 54 +++++++++++++++++++++++++ src/wallet/test/coinselector_tests.cpp | 38 ----------------- 2 files changed, 54 insertions(+), 38 deletions(-) diff --git a/src/wallet/test/coinselection_tests.cpp b/src/wallet/test/coinselection_tests.cpp index d8edbc13518779..f200467a0fd28d 100644 --- a/src/wallet/test/coinselection_tests.cpp +++ b/src/wallet/test/coinselection_tests.cpp @@ -52,6 +52,35 @@ static CoinSelectionParams init_default_params() static const CoinSelectionParams default_cs_params = init_default_params(); +static void AddCoinToWallet(CoinsResult& available_coins, CWallet& wallet, const CAmount& nValue, CFeeRate feerate = default_cs_params.m_effective_feerate, int nAge = 6*24, bool fIsFromMe = false, int nInput =0, bool spendable = false, int custom_size = 0) +{ + CMutableTransaction tx; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes + tx.vout.resize(nInput + 1); + tx.vout[nInput].nValue = nValue; + if (spendable) { + tx.vout[nInput].scriptPubKey = GetScriptForDestination(*Assert(wallet.GetNewDestination(OutputType::BECH32, ""))); + } + 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; + const auto& txout = wtx.tx->vout.at(nInput); + available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, custom_size == 0 ? CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr) : custom_size, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate}); +} + +static std::unique_ptr NewWallet(const node::NodeContext& m_node, const std::string& wallet_name = "") +{ + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), wallet_name, CreateMockableWalletDatabase()); + BOOST_CHECK(wallet->LoadWallet() == DBErrors::LOAD_OK); + LOCK(wallet->cs_wallet); + wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet->SetupDescriptorScriptPubKeyMans(); + return wallet; +} + /** Check if SelectionResult a is equivalent to SelectionResult b. * Equivalent means same input values, but maybe different inputs (i.e. same value, different prevout) */ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) @@ -375,6 +404,31 @@ BOOST_AUTO_TEST_CASE(bnb_feerate_sensitivity_test) TestBnBSuccess("Select one input at high feerates", high_feerate_pool, /*selection_target=*/ 10 * CENT, /*expected_input_amounts=*/ {10 * CENT}, CFeeRate{25'000}); } +BOOST_AUTO_TEST_CASE(tx_creation_bnb_sffo_restriction) +{ + // Verify the transaction creation process does not produce a BnB solution when SFFO is enabled. + // This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. + std::unique_ptr wallet = NewWallet(m_node); + WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(300, uint256{})); // set a high block so internal UTXOs are selectable + + CoinSelectionParams params = init_default_params(); + params.m_long_term_feerate = CFeeRate(1000); // LFTRE is less than Feerate, thrifty mode + params.m_subtract_fee_outputs = true; + + // Add spendable coin at the BnB selection upper bound + CoinsResult available_coins; + AddCoinToWallet(available_coins, *wallet, COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6*24, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + AddCoinToWallet(available_coins, *wallet, 0.7 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6*24, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + AddCoinToWallet(available_coins, *wallet, 0.6 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6*24, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); + // Now verify coin selection does not produce BnB result + auto result = WITH_LOCK(wallet->cs_wallet, return SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, COIN, /*coin_control=*/{}, params)); + BOOST_CHECK(result.has_value()); + BOOST_CHECK_NE(result->GetAlgo(), SelectionAlgorithm::BNB); + // Knapsack will only find a changeless solution on an exact match to the satoshi, SRD doesn’t look for changeless + BOOST_CHECK(result->GetInputSet().size() == 2); + BOOST_CHECK(result->GetAlgo() == SelectionAlgorithm::SRD || result->GetAlgo() == SelectionAlgorithm::KNAPSACK); +} + //TODO: SelectCoins Test // TODO: Test at `SelectCoins`/spend.cpp level that changeless solutions can be achieved by combining preset inputs with BnB solutions // TODO: Test that immature coins are not considered diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 47029b489cbdb6..54dd36a8ec271b 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -72,44 +72,6 @@ static std::unique_ptr NewWallet(const node::NodeContext& m_node, const return wallet; } -BOOST_AUTO_TEST_CASE(bnb_sffo_restriction) -{ - // Verify the coin selection process does not produce a BnB solution when SFFO is enabled. - // This is currently problematic because it could require a change output. And BnB is specialized on changeless solutions. - std::unique_ptr wallet = NewWallet(m_node); - WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(300, uint256{})); // set a high block so internal UTXOs are selectable - - FastRandomContext rand{}; - CoinSelectionParams params{ - rand, - /*change_output_size=*/ 31, // unused value, p2wpkh output size (wallet default change type) - /*change_spend_size=*/ 68, // unused value, p2wpkh input size (high-r signature) - /*min_change_target=*/ 0, // dummy, set later - /*effective_feerate=*/ CFeeRate(3000), - /*long_term_feerate=*/ CFeeRate(1000), - /*discard_feerate=*/ CFeeRate(1000), - /*tx_noinputs_size=*/ 0, - /*avoid_partial=*/ false, - }; - params.m_subtract_fee_outputs = true; - params.m_change_fee = params.m_effective_feerate.GetFee(params.change_output_size); - params.m_cost_of_change = params.m_discard_feerate.GetFee(params.change_spend_size) + params.m_change_fee; - params.m_min_change_target = params.m_cost_of_change + 1; - // Add spendable coin at the BnB selection upper bound - CoinsResult available_coins; - add_coin(available_coins, *wallet, COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); - add_coin(available_coins, *wallet, 0.5 * COIN + params.m_cost_of_change, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); - add_coin(available_coins, *wallet, 0.5 * COIN, /*feerate=*/params.m_effective_feerate, /*nAge=*/6, /*fIsFromMe=*/true, /*nInput=*/0, /*spendable=*/true); - // Knapsack will only find a changeless solution on an exact match to the satoshi, SRD doesn’t look for changeless - // If BnB were run, it would produce a single input solution with the best waste score - auto result = WITH_LOCK(wallet->cs_wallet, return SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, COIN, /*coin_control=*/{}, params)); - BOOST_CHECK(result.has_value()); - BOOST_CHECK_NE(result->GetAlgo(), SelectionAlgorithm::BNB); - BOOST_CHECK(result->GetInputSet().size() == 2); - // We have only considered BnB, SRD, and Knapsack. Test needs to be reevaluated if new algo is added - BOOST_CHECK(result->GetAlgo() == SelectionAlgorithm::SRD || result->GetAlgo() == SelectionAlgorithm::KNAPSACK); -} - // Tests that with the ideal conditions, the coin selector will always be able to find a solution that can pay the target value BOOST_AUTO_TEST_CASE(SelectCoins_test) {