fix(wallet): exclude unconfirmable outputs from CoinJoin accounting

doc/release-notes-7634.md

@@ -0,0 +1,10 @@
+Wallet changes
+--------------
+
+- CoinJoin denomination counts, average mixing rounds and the normalized
+  anonymized balance no longer include outputs of transactions that cannot
+  confirm as they stand: conflicted ones, and ones that were abandoned, never
+  broadcast, or rejected from the mempool. Previously such outputs inflated
+  these figures, which could make the wallet create fewer denominations than
+  intended and report mixing progress that did not match the coins it could
+  actually use. (#7634)

src/wallet/coinjoin.cpp

@@ -162,7 +162,7 @@ std::vector<CompactTallyItem> CWallet::SelectCoinsGroupedByAddresses(bool fSkipD
 
         if (wtx.IsCoinBase() && GetTxBlocksToMaturity(wtx) > 0) continue;
         if (fSkipUnconfirmed && !CachedTxIsTrusted(*this, wtx)) continue;
-        if (GetTxDepthInMainChain(wtx) < 0) continue;
+        if (!IsWalletUTXOSpendable(wtx)) continue;
 
         for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
             CTxDestination txdest;
@@ -253,7 +253,7 @@ int CWallet::CountInputsWithAmount(CAmount nInputAmount) const
         const auto it{mapWallet.find(outpoint.hash)};
         if (it == mapWallet.end()) continue;
         if (it->second.tx->vout[outpoint.n].nValue != nInputAmount) continue;
-        if (GetTxDepthInMainChain(it->second) < 0) continue;
+        if (!IsWalletUTXOSpendable(it->second)) continue;
 
         nTotal++;
     }
@@ -542,6 +542,9 @@ float CWallet::GetAverageAnonymizedRounds() const
 
     LOCK(cs_wallet);
     for (const auto& outpoint : setWalletUTXO) {
+        const auto it{mapWallet.find(outpoint.hash)};
+        if (it == mapWallet.end()) continue;
+        if (!IsWalletUTXOSpendable(it->second)) continue;
         if (!IsDenominated(outpoint)) continue;
 
         nTotal += GetCappedOutpointCoinJoinRounds(outpoint);
@@ -568,7 +571,7 @@ CAmount CWallet::GetNormalizedAnonymizedBalance() const
 
         CAmount nValue = it->second.tx->vout[outpoint.n].nValue;
         if (!CoinJoin::IsDenominatedAmount(nValue)) continue;
-        if (GetTxDepthInMainChain(it->second) < 0) continue;
+        if (!IsWalletUTXOSpendable(it->second)) continue;
 
         int nRounds = GetCappedOutpointCoinJoinRounds(outpoint);
         nTotal += nValue * nRounds / CCoinJoinClientOptions::GetRounds();
@@ -581,8 +584,8 @@ CAmount CachedTxGetAnonymizedCredit(const CWallet& wallet, const CWalletTx& wtx,
 {
     AssertLockHeld(wallet.cs_wallet);
 
-    // Exclude coinbase and conflicted txes
-    if (wtx.IsCoinBase() || wallet.GetTxDepthInMainChain(wtx) < 0) return 0;
+    // Exclude coinbase transactions, and any that cannot confirm as they stand
+    if (wtx.IsCoinBase() || !wallet.IsWalletUTXOSpendable(wtx)) return 0;
 
     CAmount nCredit = 0;
     uint256 hashTx = wtx.GetHash();
@@ -620,9 +623,9 @@ CoinJoinCredits CachedTxGetAvailableCoinJoinCredits(const CWallet& wallet, const
     // Must wait until coinbase is safely deep enough in the chain before valuing it
     if (wtx.IsCoinBase() && wallet.GetTxBlocksToMaturity(wtx) > 0) return ret;
 
-    int nDepth = wallet.GetTxDepthInMainChain(wtx);
-    if (nDepth < 0) return ret;
+    if (!wallet.IsWalletUTXOSpendable(wtx)) return ret;
 
+    const int nDepth{wallet.GetTxDepthInMainChain(wtx)};
     ret.is_unconfirmed = CachedTxIsTrusted(wallet, wtx) && nDepth == 0;
 
     if (wtx.m_amounts[CWalletTx::ANON_CREDIT].m_cached[ISMINE_SPENDABLE]) {

src/wallet/test/availablecoins_tests.cpp

@@ -2,8 +2,11 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
 
+#include <coinjoin/coinjoin.h>
+#include <txmempool.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
+#include <wallet/coinjoin.h>
 #include <wallet/spend.h>
 #include <wallet/test/util.h>
 #include <wallet/test/wallet_test_fixture.h>
@@ -89,5 +92,67 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup)
     BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U);
 }
 
+BOOST_FIXTURE_TEST_CASE(UnconfirmableOutputsAreNotWalletFunds, AvailableCoinsTestingSetup)
+{
+    LOCK(wallet->cs_wallet);
+
+    const auto dest{wallet->GetNewDestination("")};
+    BOOST_ASSERT(dest);
+
+    // Use a real CoinJoin denomination so the denominated-credit paths apply.
+    const CAmount denom{CoinJoin::GetSmallestDenomination()};
+    CMutableTransaction mtx;
+    mtx.vin.emplace_back(COutPoint{uint256::ONE, 0});
+    mtx.vout.emplace_back(denom, GetScriptForDestination(*dest));
+    const CTransactionRef tx{MakeTransactionRef(mtx)};
+
+    // A transaction the wallet knows about but that never reached the mempool cannot
+    // confirm as it stands, so its outputs are not funds the wallet can spend or mix.
+    BOOST_CHECK(wallet->AddToWallet(tx, TxStateInactive{}));
+    BOOST_CHECK_EQUAL(wallet->CountInputsWithAmount(denom), 0);
+
+    // The aggregate CoinJoin balances have to agree: an output that is not wallet funds
+    // is not denominated or anonymized funds either.
+    const CWalletTx& wtx{wallet->mapWallet.at(tx->GetHash())};
+    BOOST_CHECK_EQUAL(CachedTxGetAvailableCoinJoinCredits(*wallet, wtx).m_denominated, 0);
+
+    // Once it is in the mempool they count.
+    BOOST_CHECK(wallet->AddToWallet(tx, TxStateInMempool{}));
+    BOOST_CHECK_EQUAL(wallet->CountInputsWithAmount(denom), 1);
+    BOOST_CHECK_EQUAL(CachedTxGetAvailableCoinJoinCredits(*wallet, wtx).m_denominated, denom);
+}
+
+BOOST_FIXTURE_TEST_CASE(MempoolRemovalInvalidatesAnonymizableTally, AvailableCoinsTestingSetup)
+{
+    LOCK(wallet->cs_wallet);
+
+    const auto dest{wallet->GetNewDestination("")};
+    BOOST_ASSERT(dest);
+
+    // A wallet transaction in the mempool is trusted at depth zero, so its outputs count
+    // towards the anonymizable tally and that tally is cacheable.
+    auto created{CreateTransaction(*wallet, {CRecipient{GetScriptForDestination(*dest), 1 * COIN,
+                                                        /*fSubtractFeeFromAmount=*/false}},
+                                   RANDOM_CHANGE_POSITION, CCoinControl{})};
+    BOOST_REQUIRE(created);
+    const CTransactionRef tx{created->tx};
+    BOOST_CHECK(wallet->AddToWallet(tx, TxStateInMempool{}));
+
+    const auto tallied = [&](const CTxDestination& target) {
+        for (const auto& item : wallet->SelectCoinsGroupedByAddresses()) {
+            if (item.txdest == target) return true;
+        }
+        return false;
+    };
+
+    // Prime the cache, so that the check below cannot pass by recomputing the tally.
+    BOOST_REQUIRE(tallied(*dest));
+
+    // Leaving the mempool makes the transaction unconfirmable as it stands; the cached
+    // tally must not keep handing out its outputs.
+    wallet->transactionRemovedFromMempool(tx, MemPoolRemovalReason::EXPIRY);
+    BOOST_CHECK(!tallied(*dest));
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/wallet.cpp

@@ -762,6 +762,14 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
     return false;
 }
 
+bool CWallet::IsWalletUTXOSpendable(const CWalletTx& wtx) const
+{
+    AssertLockHeld(cs_wallet);
+    const int depth{GetTxDepthInMainChain(wtx)};
+    if (depth < 0) return false;
+    return depth > 0 || wtx.InMempool();
+}
+
 void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch)
 {
     mapTxSpends.insert(std::make_pair(outpoint, wtxid));
@@ -1440,6 +1448,11 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
         auto it = mapWallet.find(tx->GetHash());
         if (it != mapWallet.end()) {
             RefreshMempoolStatus(it->second, chain());
+            // The transaction is inactive now, so its outputs stop counting as wallet
+            // funds. The anonymizable tallies are served from a cache that would keep
+            // handing out the old answer until some unrelated event cleared it.
+            fAnonymizableTallyCached = false;
+            fAnonymizableTallyCachedNonDenom = false;
         }
     }
     // Handle transactions that were removed from the mempool because they

src/wallet/wallet.h

@@ -638,6 +638,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     std::vector<COutPoint> SelectFullyMixedForPromotion(int nDenom, int nCount) const;
 
     bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    /** May `wtx`'s outputs be counted as wallet funds?
+     *
+     *  The wallet knows about transactions that cannot confirm as they stand: conflicted
+     *  ones, and ones that were abandoned, never broadcast or rejected from the mempool.
+     *  AvailableCoins() filters their outputs out, so anything else that values wallet
+     *  outputs must do the same. */
+    bool IsWalletUTXOSpendable(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     // Whether this or any known UTXO with the same single key has been spent.
     bool IsSpentKey(const CScript& scriptPubKey) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);