diff --git a/doc/release-notes-7634.md b/doc/release-notes-7634.md new file mode 100644 index 000000000000..464d5c842c4d --- /dev/null +++ b/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) diff --git a/src/wallet/coinjoin.cpp b/src/wallet/coinjoin.cpp index 0a2b6705d49b..6fab8d0c4b84 100644 --- a/src/wallet/coinjoin.cpp +++ b/src/wallet/coinjoin.cpp @@ -162,7 +162,7 @@ std::vector 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]) { diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index 0afb81d062e0..b00faa631749 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/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 +#include #include #include +#include #include #include #include @@ -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 diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index da4a2e08d885..5a2540b59aea 100644 --- a/src/wallet/wallet.cpp +++ b/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 diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 641065321e1e..0c0b9bb55b5a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -638,6 +638,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::vector 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);