merge bitcoin#15652: Update transactions with current mempool after load

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

src/coinjoin/client.cpp

@@ -15,7 +15,6 @@
 #include <netmessagemaker.h>
 #include <script/sign.h>
 #include <shutdown.h>
-#include <txmempool.h>
 #include <util/moneystr.h>
 #include <util/ranges.h>
 #include <util/system.h>
@@ -555,7 +554,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans
     if (!mixingMasternode) return false;
 
     LOCK(cs_main);
-    LOCK2(mempool.cs, mixingWallet.cs_wallet);
+    LOCK(mixingWallet.cs_wallet);
     LOCK(cs_coinjoin);
 
     finalMutableTransaction = CMutableTransaction{finalTransactionNew};
@@ -779,8 +778,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(CConnman& connman, bool fDr
     CAmount nBalanceNeedsAnonymized;
 
     {
-        LOCK2(cs_main, mempool.cs);
-        LOCK(mixingWallet.cs_wallet);
+        LOCK2(cs_main, mixingWallet.cs_wallet);
 
         if (!fDryRun && mixingWallet.IsLocked(true)) {
             strAutoDenomResult = _("Wallet is locked.");
@@ -1242,8 +1240,7 @@ bool CCoinJoinClientManager::MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq)
 
 bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman)
 {
-    LOCK2(cs_main, mempool.cs);
-    LOCK(mixingWallet.cs_wallet);
+    LOCK2(cs_main, mixingWallet.cs_wallet);
 
     std::string strError;
     std::vector<CTxDSIn> vecTxDSIn;
@@ -1386,8 +1383,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts()
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
 
-    LOCK2(cs_main, mempool.cs);
-    LOCK(mixingWallet.cs_wallet);
+    LOCK2(cs_main, mixingWallet.cs_wallet);
 
     // NOTE: We do not allow txes larger than 100 kB, so we have to limit number of inputs here.
     // We still want to consume a lot of inputs to avoid creating only smaller denoms though.
@@ -1425,7 +1421,6 @@ bool CCoinJoinClientSession::MakeCollateralAmounts()
 bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated)
 {
     AssertLockHeld(cs_main);
-    AssertLockHeld(mempool.cs);
     AssertLockHeld(mixingWallet.cs_wallet);
 
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
@@ -1571,8 +1566,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate)
 {
     if (!CCoinJoinClientOptions::IsEnabled()) return false;
 
-    LOCK2(cs_main, mempool.cs);
-    LOCK(mixingWallet.cs_wallet);
+    LOCK2(cs_main, mixingWallet.cs_wallet);
 
     // NOTE: We do not allow txes larger than 100 kB, so we have to limit number of inputs here.
     // We still want to consume a lot of inputs to avoid creating only smaller denoms though.
@@ -1604,7 +1598,6 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate)
 bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals)
 {
     AssertLockHeld(cs_main);
-    AssertLockHeld(mempool.cs);
     AssertLockHeld(mixingWallet.cs_wallet);
 
     if (!CCoinJoinClientOptions::IsEnabled()) return false;

src/interfaces/chain.cpp

@@ -371,6 +371,13 @@ class ChainImpl : public Chain
     {
         return MakeUnique<RpcHandlerImpl>(command);
     }
+    void requestMempoolTransactions(Notifications& notifications) override
+    {
+        LOCK2(::cs_main, ::mempool.cs);
+        for (const CTxMemPoolEntry& entry : ::mempool.mapTx) {
+            notifications.TransactionAddedToMempool(entry.GetSharedTx(), 0);
+        }
+    }
 };
 } // namespace
 

src/interfaces/chain.h

@@ -256,6 +256,16 @@ class Chain
     //! Register handler for RPC. Command is not copied, so reference
     //! needs to remain valid until Handler is disconnected.
     virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
+
+    //! Synchronously send TransactionAddedToMempool notifications about all
+    //! current mempool transactions to the specified handler and return after
+    //! the last one is sent. These notifications aren't coordinated with async
+    //! notifications sent by handleNotifications, so out of date async
+    //! notifications from handleNotifications can arrive during and after
+    //! synchronous notifications from requestMempoolTransactions. Clients need
+    //! to be prepared to handle this by ignoring notifications about unknown
+    //! removed transactions and already added new transactions.
+    virtual void requestMempoolTransactions(Notifications& notifications) = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for

src/interfaces/wallet.cpp

@@ -18,7 +18,6 @@
 #include <script/standard.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
-#include <txmempool.h> // for mempool.cs
 #include <ui_interface.h>
 #include <uint256.h>
 #include <util/system.h>
@@ -284,7 +283,7 @@ class WalletImpl : public Wallet
         std::string& fail_reason) override
     {
         auto locked_chain = m_wallet->chain().lock();
-        LOCK2(mempool.cs, m_wallet->cs_wallet);
+        LOCK(m_wallet->cs_wallet);
         CReserveKey m_key(m_wallet.get());
         CTransactionRef tx;
         if (!m_wallet->CreateTransaction(*locked_chain, recipients, tx, m_key, fee, change_pos,
@@ -299,7 +298,7 @@ class WalletImpl : public Wallet
         std::string& reject_reason) override
     {
         auto locked_chain = m_wallet->chain().lock();
-        LOCK2(mempool.cs, m_wallet->cs_wallet);
+        LOCK(m_wallet->cs_wallet);
         CReserveKey m_key(m_wallet.get());
         CValidationState state;
         if (!m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form), m_key, state)) {

src/net_processing.cpp

@@ -4519,7 +4519,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         //
         std::vector<CInv> vInv;
         {
-            LOCK2(mempool.cs, pto->cs_inventory);
+            LOCK(pto->cs_inventory);
 
             size_t reserve = std::min<size_t>(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000);
             reserve = std::max<size_t>(reserve, pto->vInventoryBlockToSend.size());

src/rpc/governance.cpp

@@ -17,7 +17,6 @@
 #include <net.h>
 #include <rpc/server.h>
 #include <rpc/util.h>
-#include <txmempool.h>
 #include <util/system.h>
 #include <validation.h>
 #include <wallet/rpcwallet.h>
@@ -206,7 +205,7 @@ static UniValue gobject_prepare(const JSONRPCRequest& request)
     }
 
     auto locked_chain = wallet->chain().lock();
-    LOCK2(mempool.cs, pwallet->cs_wallet);
+    LOCK(pwallet->cs_wallet);
 
     {
         LOCK(cs_main);

src/rpc/rpcevo.cpp

@@ -17,7 +17,6 @@
 #include <netbase.h>
 #include <rpc/server.h>
 #include <rpc/util.h>
-#include <txmempool.h>
 #include <util/moneystr.h>
 #include <util/validation.h>
 #include <validation.h>
@@ -182,7 +181,7 @@ static void FundSpecialTx(CWallet* pwallet, CMutableTransaction& tx, const Speci
     pwallet->BlockUntilSyncedToCurrentChain();
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK2(mempool.cs, pwallet->cs_wallet);
+    LOCK(pwallet->cs_wallet);
 
     CTxDestination nodest = CNoDestination();
     if (fundDest == nodest) {

src/wallet/rpcdump.cpp

@@ -317,7 +317,11 @@ UniValue importaddress(const JSONRPCRequest& request)
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        pwallet->ReacceptWalletTransactions();
+        {
+            auto locked_chain = pwallet->chain().lock();
+            LOCK(pwallet->cs_wallet);
+            pwallet->ReacceptWalletTransactions(*locked_chain);
+        }
     }
 
     return NullUniValue;
@@ -501,7 +505,11 @@ UniValue importpubkey(const JSONRPCRequest& request)
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        pwallet->ReacceptWalletTransactions();
+        {
+            auto locked_chain = pwallet->chain().lock();
+            LOCK(pwallet->cs_wallet);
+            pwallet->ReacceptWalletTransactions(*locked_chain);
+        }
     }
 
     return NullUniValue;
@@ -1598,7 +1606,11 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     }
     if (fRescan && fRunScan && requests.size()) {
         int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
-        pwallet->ReacceptWalletTransactions();
+        {
+            auto locked_chain = pwallet->chain().lock();
+            LOCK(pwallet->cs_wallet);
+            pwallet->ReacceptWalletTransactions(*locked_chain);
+        }
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");

src/wallet/rpcwallet.cpp

@@ -19,7 +19,6 @@
 #include <rpc/util.h>
 #include <script/descriptor.h>
 #include <timedata.h>
-#include <txmempool.h>
 #include <util/fees.h>
 #include <util/system.h>
 #include <util/moneystr.h>
@@ -388,7 +387,7 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
     pwallet->BlockUntilSyncedToCurrentChain();
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK2(mempool.cs, pwallet->cs_wallet);
+    LOCK(pwallet->cs_wallet);
 
     CTxDestination dest = DecodeDestination(request.params[0].get_str());
     if (!IsValidDestination(dest)) {
@@ -921,7 +920,7 @@ static UniValue sendmany(const JSONRPCRequest& request)
     pwallet->BlockUntilSyncedToCurrentChain();
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK2(mempool.cs, pwallet->cs_wallet);
+    LOCK(pwallet->cs_wallet);
 
     if (pwallet->GetBroadcastTransactions() && !pwallet->chain().p2pEnabled()) {
         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
@@ -3483,7 +3482,7 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request)
 
     // Sign the transaction
     auto locked_chain = pwallet->chain().lock();
-    LOCK2(mempool.cs, pwallet->cs_wallet);
+    LOCK(pwallet->cs_wallet);
     EnsureWalletIsUnlocked(pwallet);
 
     return SignTransaction(pwallet->chain(), mtx, request.params[1], pwallet, false, request.params[2]);

src/wallet/wallet.cpp

@@ -2267,13 +2267,11 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     return result;
 }
 
-void CWallet::ReacceptWalletTransactions()
+void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
 {
     // If transactions aren't being broadcasted, don't let them into local mempool either
     if (!fBroadcastTransactions)
         return;
-    auto locked_chain = chain().lock();
-    LOCK2(mempool.cs, cs_wallet);
     std::map<int64_t, CWalletTx*> mapSorted;
 
     // Sort pending wallet transactions based on their initial wallet insertion order
@@ -2282,7 +2280,7 @@ void CWallet::ReacceptWalletTransactions()
         CWalletTx& wtx = item.second;
         assert(wtx.GetHash() == wtxid);
 
-        int nDepth = wtx.GetDepthInMainChain(*locked_chain);
+        int nDepth = wtx.GetDepthInMainChain(locked_chain);
 
         if (!wtx.IsCoinBase() && (nDepth == 0 && !wtx.IsLockedByInstantSend() && !wtx.isAbandoned())) {
             mapSorted.insert(std::make_pair(wtx.nOrderPos, &wtx));
@@ -2293,7 +2291,7 @@ void CWallet::ReacceptWalletTransactions()
     for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
         CWalletTx& wtx = *(item.second);
         std::string unused_err_string;
-        wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, *locked_chain);
+        wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain);
     }
 }
 
@@ -2640,9 +2638,9 @@ void CWallet::ResendWalletTransactions()
 
     int submitted_tx_count = 0;
 
-    { // locked_chain, mempool.cs and cs_wallet scope
+    { // locked_chain and cs_wallet scope
         auto locked_chain = chain().lock();
-        LOCK2(mempool.cs, cs_wallet);
+        LOCK(cs_wallet);
 
         // Relay transactions
         for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
@@ -2653,7 +2651,7 @@ void CWallet::ResendWalletTransactions()
             std::string unused_err_string;
             if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true, *locked_chain)) ++submitted_tx_count;
         }
-    } // locked_chain, mempool.cs and cs_wallet
+    } // locked_chain and cs_wallet
 
     if (submitted_tx_count > 0) {
         WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, submitted_tx_count);
@@ -3203,7 +3201,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
     // Acquire the locks to prevent races to the new locked unspents between the
     // CreateTransaction call and LockCoin calls (when lockUnspents is true).
     auto locked_chain = chain().lock();
-    LOCK2(mempool.cs, cs_wallet);
+    LOCK(cs_wallet);
 
     int nExtraPayloadSize = 0;
     if (tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL)
@@ -3572,7 +3570,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
     {
         std::vector<CInputCoin> vecCoins;
         auto locked_chain = chain().lock();
-        LOCK2(mempool.cs, cs_wallet);
+        LOCK(cs_wallet);
         {
             CAmount nAmountAvailable{0};
             std::vector<COutput> vAvailableCoins;
@@ -3924,7 +3922,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
 {
     {
         auto locked_chain = chain().lock();
-        LOCK2(mempool.cs, cs_wallet);
+        LOCK(cs_wallet);
 
         CWalletTx wtxNew(this, std::move(tx));
         wtxNew.mapValue = std::move(mapValue);
@@ -5281,9 +5279,15 @@ void CWallet::handleNotifications()
 
 void CWallet::postInitProcess()
 {
+    auto locked_chain = chain().lock();
+    LOCK(cs_wallet);
+
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
-    ReacceptWalletTransactions();
+    ReacceptWalletTransactions(*locked_chain);
+
+    // Update wallet transactions with current mempool transactions.
+    chain().requestMempoolTransactions(*this);
 }
 
 bool CWallet::InitAutoBackup()

src/wallet/wallet.h

@@ -998,7 +998,7 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
     };
     ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
     void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
-    void ReacceptWalletTransactions();
+    void ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void ResendWalletTransactions();
     struct Balance {
         CAmount m_mine_trusted{0};           //!< Trusted, at depth=GetBalance.min_depth or more

test/functional/wallet_balance.py

@@ -129,5 +129,17 @@ def run_test(self):
         # getbalance with minconf=2 will show the new balance.
         assert_equal(self.nodes[1].getbalance(minconf=2), Decimal('0'))
 
+        # check mempool transactions count for wallet unconfirmed balance after
+        # dynamically loading the wallet.
+        before = self.nodes[1].getunconfirmedbalance()
+        dst = self.nodes[1].getnewaddress()
+        self.nodes[1].unloadwallet('')
+        self.nodes[0].sendtoaddress(dst, 0.1)
+        self.sync_all()
+        self.nodes[1].loadwallet('')
+        after = self.nodes[1].getunconfirmedbalance()
+        assert_equal(before + Decimal('0.1'), after)
+
+
 if __name__ == '__main__':
     WalletTest().main()