Merge #6654: backport: merge bitcoin#17526, bitcoin#22928, bitcoin#22019, bitcoin#23762, bitcoin#24067, bitcoin#24091, bitcoin#24560, partial bitcoin#17211 (wallet backports: part 4)

e901183 merge bitcoin#24560: Use single FastRandomContext when creating a wallet tx (Kittywhiskers Van Gogh)
5a1850d merge bitcoin#24091: Consolidate CInputCoin and COutput (Kittywhiskers Van Gogh)
8b815dc refactor: make `vecInputCoins` use `COutPoint`, rename to `outpoints` (Kittywhiskers Van Gogh)
3260e07 merge bitcoin#24067: Actually treat (un)confirmed txs as (un)confirmed (Kittywhiskers Van Gogh)
84d9194 merge bitcoin#23762: Replace Assume with Assert where needed in coinselection (Kittywhiskers Van Gogh)
66b5302 wallet: trigger coin selection fast-fail condition over ignoring result (UdjinM6)
d9ee3db wallet: fast-fail coin selection when supplied empty output groups (UdjinM6)
c5038c1 merge bitcoin#22019: Introduce SelectionResult for encapsulating a coin selection solution (Kittywhiskers Van Gogh)
c352062 trivial: invert `SelectCoins` condition check to move bail-out condition (Kittywhiskers Van Gogh)
40550c9 merge bitcoin#22928: Remove gArgs from wallet.h and wallet.cpp (2) (Kittywhiskers Van Gogh)
aae8e91 partial bitcoin#17211: Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs (Kittywhiskers Van Gogh)
870548a merge bitcoin#17526: Add Single Random Draw as an additional coin selection algorithm (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * While [bitcoin#17526](bitcoin#17526) introduces Single Random Draw (SRD) as a new coin-selection algorithm, it remains disabled (like BnB) as it is unaware of mixed funds.
    * UTXO (un)locking as introduced in df765a4 for `rpc_fundrawtransaction.py` has been omitted in the `test_fee_p2pkh()` sub-test as we do not have any other outputs except for `p2pkh`. Failing to do so results in test failure (see below).

      <details>

      <summary>Test error:</summary>

       ```
       dash@507c3774b68c:/src/dash$ ./test/functional/rpc_fundrawtransaction.py
       2025-05-06T06:48:01.101000Z TestFramework (INFO): PRNG seed is: 4612096820677160739
       2025-05-06T06:48:01.101000Z TestFramework (INFO): Initializing test directory    /tmp/dash_func_test_n3i0k9yc
       2025-05-06T06:48:04.177000Z TestFramework (INFO): Connect nodes, set fees, generate blocks, and sync
       [...]
       2025-05-06T06:48:09.249000Z TestFramework (INFO): Test fundrawtxn p2pkh fee
       2025-05-06T06:48:09.298000Z TestFramework (ERROR): JSONRPC error
       Traceback (most recent call last):
         File "/src/dash/test/functional/test_framework/test_framework.py", line 162, in main
           self.run_test()
         File "/src/dash/./test/functional/rpc_fundrawtransaction.py", line 115, in run_test
           self.test_fee_p2pkh()
         File "/src/dash/./test/functional/rpc_fundrawtransaction.py", line 398, in test_fee_p2pkh
           fundedTx = self.nodes[0].fundrawtransaction(rawtx)
         File "/src/dash/test/functional/test_framework/coverage.py", line 49, in __call__
           return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
         File "/src/dash/test/functional/test_framework/authproxy.py", line 143, in __call__
           raise JSONRPCException(response['error'], status)
       test_framework.authproxy.JSONRPCException: Insufficient funds. (-4)
       2025-05-06T06:48:09.802000Z TestFramework (INFO): Stopping nodes
       ```

      </details>

  * In [bitcoin#22019](bitcoin#22019) we don't use `SelectionResult::GetShuffledInputVector()` in `CreateTransactionInternal()` because it interferes with our BIP69 sorting implementation that sorts `vin`s instead of `CRecipient`s.

  ## How Has This Been Tested?

  A full CoinJoin session run on 0e33db2

  ![CoinJoin session run on build 0e33db2](https://github.com/user-attachments/assets/2aca71dc-4931-44fd-93e4-fc260b22b885)

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    LGTM, utACK e901183
  PastaPastaPasta:
    utACK e901183

Tree-SHA512: 8631107e23fc581f23a366e468c10079d72c41b81475e1f33f79262ec3706b48cbc464fe83e31d33070f58c03340189431e4fbd81a7d13b5f056bfcc2f96929a

Author: PastaPastaPasta

src/bench/coin_selection.cpp

@@ -32,7 +32,7 @@ static void CoinSelection(benchmark::Bench& bench)
 {
     NodeContext node;
     auto chain = interfaces::MakeChain(node);
-    CWallet wallet(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase());
+    CWallet wallet(chain.get(), /*coinjoin_loader=*/nullptr, "", gArgs, CreateDummyWalletDatabase());
     std::vector<std::unique_ptr<CWalletTx>> wtxs;
     LOCK(wallet.cs_wallet);
 
@@ -45,34 +45,37 @@ static void CoinSelection(benchmark::Bench& bench)
     // Create coins
     std::vector<COutput> 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(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);
-    const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34,
-                                                    /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
-                                                    /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
-                                                    /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
+    FastRandomContext rand{};
+    const CoinSelectionParams coin_selection_params{
+        rand,
+        /* change_output_size= */ 34,
+        /* change_spend_size= */ 148,
+        /* effective_feerate= */ CFeeRate(0),
+        /* long_term_feerate= */ CFeeRate(0),
+        /* discard_feerate= */ CFeeRate(0),
+        /* tx_noinputs_size= */ 0,
+        /* avoid_partial= */ false,
+    };
     bench.run([&] {
-        std::set<CInputCoin> setCoinsRet;
-        CAmount nValueRet;
-        bool success = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params);
-        assert(success);
-        assert(nValueRet == 1003 * COIN);
-        assert(setCoinsRet.size() == 2);
+        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
+        assert(result);
+        assert(result->GetSelectedValue() == 1003 * COIN);
+        assert(result->GetInputSet().size() == 2);
     });
 }
 
-typedef std::set<CInputCoin> CoinSet;
-
 // Copied from src/wallet/test/coinselector_tests.cpp
 static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>& set)
 {
     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<OutputGroup>& utxo_pool)
@@ -91,17 +94,14 @@ static void BnBExhaustion(benchmark::Bench& bench)
 {
     // Setup
     std::vector<OutputGroup> utxo_pool;
-    CoinSet selection;
-    CAmount value_ret = 0;
 
     bench.run([&] {
         // Benchmark
         CAmount target = make_hard_case(17, utxo_pool);
-        SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust
+        SelectCoinsBnB(utxo_pool, target, 0); // Should exhaust
 
         // Cleanup
         utxo_pool.clear();
-        selection.clear();
     });
 }
 

src/bench/wallet_balance.cpp

@@ -19,7 +19,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
     const auto test_setup = MakeNoLogFileContext<const TestingSetup>();
     const auto& ADDRESS_WATCHONLY = ADDRESS_B58T_UNSPENDABLE;
 
-    CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase()};
+    CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase()};
     {
         LOCK(wallet.cs_wallet);
         wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);

src/coinjoin/client.cpp

@@ -1468,12 +1468,12 @@ bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tally
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
 
     // Denominated input is always a single one, so we can check its amount directly and return early
-    if (!fTryDenominated && tallyItem.vecInputCoins.size() == 1 && CoinJoin::IsDenominatedAmount(tallyItem.nAmount)) {
+    if (!fTryDenominated && tallyItem.outpoints.size() == 1 && CoinJoin::IsDenominatedAmount(tallyItem.nAmount)) {
         return false;
     }
 
     // Skip single inputs that can be used as collaterals already
-    if (tallyItem.vecInputCoins.size() == 1 && CoinJoin::IsCollateralAmount(tallyItem.nAmount)) {
+    if (tallyItem.outpoints.size() == 1 && CoinJoin::IsCollateralAmount(tallyItem.nAmount)) {
         return false;
     }
 
@@ -1562,10 +1562,10 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
     }
 
     const auto& output = vCoins.at(GetRand(vCoins.size()));
-    const CTxOut txout = output.tx->tx->vout[output.i];
+    const CTxOut txout = output.txout;
 
     txCollateral.vin.clear();
-    txCollateral.vin.emplace_back(output.tx->GetHash(), output.i);
+    txCollateral.vin.emplace_back(output.outpoint.hash, output.outpoint.n);
     txCollateral.vout.clear();
 
     // pay collateral charge in fees
@@ -1636,7 +1636,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
 
     // denominated input is always a single one, so we can check its amount directly and return early
-    if (tallyItem.vecInputCoins.size() == 1 && CoinJoin::IsDenominatedAmount(tallyItem.nAmount)) {
+    if (tallyItem.outpoints.size() == 1 && CoinJoin::IsDenominatedAmount(tallyItem.nAmount)) {
         return false;
     }
 

src/coinjoin/util.cpp

@@ -124,9 +124,9 @@ CTransactionBuilder::CTransactionBuilder(CWallet& wallet, const CompactTallyItem
     // Create dummy tx to calculate the exact required fees upfront for accurate amount and fee calculations
     CMutableTransaction dummyTx;
     // Select all tallyItem outputs in the coinControl so that CreateTransaction knows what to use
-    for (const auto& coin : tallyItem.vecInputCoins) {
-        coinControl.Select(coin.outpoint);
-        dummyTx.vin.emplace_back(coin.outpoint, CScript());
+    for (const auto& outpoint : tallyItem.outpoints) {
+        coinControl.Select(outpoint);
+        dummyTx.vin.emplace_back(outpoint, CScript());
     }
     // Get a comparable dummy scriptPubKey, avoid writing/flushing to the actual wallet db
     CScript dummyScript;

src/interfaces/chain.h

@@ -133,9 +133,6 @@ class Chain
     //! or one of its ancestors.
     virtual std::optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
 
-    //! Check if transaction will be final given chain height current time.
-    virtual bool checkFinalTx(const CTransaction& tx) = 0;
-
     //! Check if transaction is locked by InstantSendManager
     virtual bool isInstantSendLockedTx(const uint256& hash) = 0;
 

src/interfaces/wallet.h

@@ -433,7 +433,6 @@ struct WalletTxStatus
     int depth_in_main_chain;
     unsigned int time_received;
     uint32_t lock_time;
-    bool is_final;
     bool is_trusted;
     bool is_abandoned;
     bool is_coinbase;

src/node/interfaces.cpp

@@ -771,11 +771,6 @@ class ChainImpl : public Chain
         }
         return std::nullopt;
     }
-    bool checkFinalTx(const CTransaction& tx) override
-    {
-        LOCK(cs_main);
-        return CheckFinalTxAtTip(*Assert(m_node.chainman->ActiveChain().Tip()), tx);
-    }
     bool isInstantSendLockedTx(const uint256& hash) override
     {
         if (m_node.llmq_ctx == nullptr || m_node.llmq_ctx->isman == nullptr) return false;

src/qt/test/addressbooktests.cpp

@@ -64,7 +64,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
 {
     TestChain100Setup test;
     node.setContext(&test.m_node);
-    const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
+    const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase());
     wallet->LoadWallet();
     wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
     {

src/qt/test/wallettests.cpp

@@ -110,7 +110,7 @@ void TestGUI(interfaces::Node& node)
     }
     node.setContext(&test.m_node);
     WalletContext& context = *node.walletLoader().context();
-    const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
+    const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase());
     AddWallet(context, wallet);
     wallet->LoadWallet();
     wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);

src/qt/transactiondesc.cpp

@@ -18,7 +18,6 @@
 #include <key_io.h>
 #include <interfaces/node.h>
 #include <interfaces/wallet.h>
-#include <script/script.h>
 #include <util/system.h>
 #include <validation.h>
 #include <wallet/ismine.h>
@@ -30,14 +29,6 @@
 
 QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const interfaces::WalletTxStatus& status, bool inMempool, int numBlocks)
 {
-    if (!status.is_final)
-    {
-        if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)
-            return tr("Open for %n more block(s)", "", wtx.tx->nLockTime - numBlocks);
-        else
-            return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.tx->nLockTime));
-    }
-    else
     {
         int nDepth = status.depth_in_main_chain;
         if (nDepth < 0) return tr("conflicted");

src/qt/transactionrecord.cpp

@@ -278,21 +278,8 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons
     status.lockedByChainLocks = wtx.is_chainlocked;
     status.lockedByInstantSend = wtx.is_islocked;
 
-    const bool up_to_date = ((int64_t)QDateTime::currentMSecsSinceEpoch() / 1000 - block_time < MAX_BLOCK_TIME_GAP);
-    if (up_to_date && !wtx.is_final) {
-        if (wtx.lock_time < LOCKTIME_THRESHOLD) {
-            status.status = TransactionStatus::OpenUntilBlock;
-            status.open_for = wtx.lock_time - numBlocks;
-        }
-        else
-        {
-            status.status = TransactionStatus::OpenUntilDate;
-            status.open_for = wtx.lock_time;
-        }
-    }
     // For generated transactions, determine maturity
-    else if(type == TransactionRecord::Generated)
-    {
+    if (type == TransactionRecord::Generated) {
         if (wtx.blocks_to_maturity > 0)
         {
             status.status = TransactionStatus::Immature;

src/qt/transactionrecord.h

@@ -33,8 +33,6 @@ class TransactionStatus
     enum Status {
         Confirmed,          /**< Have 6 or more confirmations (normal tx) or fully mature (mined tx) **/
         /// Normal (sent/received) transactions
-        OpenUntilDate,      /**< Transaction not yet final, waiting for date */
-        OpenUntilBlock,     /**< Transaction not yet final, waiting for block */
         Unconfirmed,        /**< Not yet mined into a block **/
         Confirming,         /**< Confirmed, but waiting for the recommended number of confirmations **/
         Conflicted,         /**< Conflicts with other transaction or mempool **/

src/qt/transactiontablemodel.cpp

@@ -350,12 +350,6 @@ QString TransactionTableModel::formatTxStatus(const TransactionRecord *wtx) cons
 
     switch(wtx->status.status)
     {
-    case TransactionStatus::OpenUntilBlock:
-        status = tr("Open for %n more block(s)","",wtx->status.open_for);
-        break;
-    case TransactionStatus::OpenUntilDate:
-        status = tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx->status.open_for));
-        break;
     case TransactionStatus::Unconfirmed:
         status = tr("Unconfirmed");
         break;
@@ -564,9 +558,6 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
 {
     switch(wtx->status.status)
     {
-    case TransactionStatus::OpenUntilBlock:
-    case TransactionStatus::OpenUntilDate:
-        return GUIUtil::getThemedQColor(GUIUtil::ThemedColor::TX_STATUS_OPENUNTILDATE);
     case TransactionStatus::Unconfirmed:
         return GUIUtil::getIcon("transaction_0");
     case TransactionStatus::Abandoned:

src/rpc/evo.cpp

@@ -261,8 +261,8 @@ static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const Specia
 
     for (const auto& out : vecOutputs) {
         CTxDestination txDest;
-        if (ExtractDestination(out.tx->tx->vout[out.i].scriptPubKey, txDest) && txDest == fundDest) {
-            coinControl.Select(COutPoint(out.tx->tx->GetHash(), out.i));
+        if (ExtractDestination(out.txout.scriptPubKey, txDest) && txDest == fundDest) {
+            coinControl.Select(out.outpoint);
         }
     }
 

src/rpc/masternode.cpp

@@ -140,7 +140,7 @@ static RPCHelpMan masternode_outputs()
     }
     UniValue outputsArr(UniValue::VARR);
     for (const auto& out : vPossibleCoins) {
-        outputsArr.push_back(out.GetInputCoin().outpoint.ToStringShort());
+        outputsArr.push_back(out.outpoint.ToStringShort());
     }
 
     return outputsArr;

src/wallet/coinjoin.cpp

@@ -68,14 +68,14 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve
     Shuffle(vCoins.rbegin(), vCoins.rend(), FastRandomContext());
 
     for (const auto& out : vCoins) {
-        uint256 txHash = out.tx->GetHash();
-        CAmount nValue = out.tx->tx->vout[out.i].nValue;
+        uint256 txHash = out.outpoint.hash;
+        CAmount nValue = out.txout.nValue;
         if (setRecentTxIds.find(txHash) != setRecentTxIds.end()) continue; // no duplicate txids
         if (nValueTotal + nValue > nValueMax) continue;
         if (nValue != nDenomAmount) continue;
 
-        CTxIn txin = CTxIn(txHash, out.i);
-        CScript scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;
+        CTxIn txin = CTxIn(txHash, out.outpoint.n);
+        CScript scriptPubKey = out.txout.scriptPubKey;
         int nRounds = GetRealOutpointCoinJoinRounds(txin.prevout);
 
         nValueTotal += nValue;
@@ -95,7 +95,7 @@ struct CompareByPriority
     bool operator()(const COutput& t1,
                     const COutput& t2) const
     {
-        return CoinJoin::CalculateAmountPriority(t1.GetInputCoin().effective_value) > CoinJoin::CalculateAmountPriority(t2.GetInputCoin().effective_value);
+        return CoinJoin::CalculateAmountPriority(t1.effective_value) > CoinJoin::CalculateAmountPriority(t2.effective_value);
     }
 };
 
@@ -114,7 +114,7 @@ bool CWallet::SelectDenominatedAmounts(CAmount nValueMax, std::set<CAmount>& set
     std::sort(vCoins.rbegin(), vCoins.rend(), CompareByPriority());
 
     for (const auto& out : vCoins) {
-        CAmount nValue = out.tx->tx->vout[out.i].nValue;
+        CAmount nValue = out.txout.nValue;
         if (nValueTotal + nValue <= nValueMax) {
             nValueTotal += nValue;
             setAmountsRet.emplace(nValue);
@@ -169,7 +169,7 @@ std::vector<CompactTallyItem> CWallet::SelectCoinsGroupedByAddresses(bool fSkipD
             if(!(mine & filter)) continue;
 
             auto itTallyItem = mapTally.find(txdest);
-            if (nMaxOupointsPerAddress != -1 && itTallyItem != mapTally.end() && int64_t(itTallyItem->second.vecInputCoins.size()) >= nMaxOupointsPerAddress) continue;
+            if (nMaxOupointsPerAddress != -1 && itTallyItem != mapTally.end() && int64_t(itTallyItem->second.outpoints.size()) >= nMaxOupointsPerAddress) continue;
 
             if(IsSpent(outpoint.hash, i) || IsLockedCoin(outpoint.hash, i)) continue;
 
@@ -191,7 +191,7 @@ std::vector<CompactTallyItem> CWallet::SelectCoinsGroupedByAddresses(bool fSkipD
                 itTallyItem->second.txdest = txdest;
             }
             itTallyItem->second.nAmount += wtx.tx->vout[i].nValue;
-            itTallyItem->second.vecInputCoins.emplace_back(wtx.tx, i);
+            itTallyItem->second.outpoints.emplace_back(COutPoint{outpoint.hash, i});
         }
     }