fix: hold wallet shared pointer in CJ Manager/Sessions to prevent concurrent unload

UdjinM6 · UdjinM6 · commit e800d9d09c88 · 2024-12-02T17:20:50.000+03:00
diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp
diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h
@@ -96,7 +96,7 @@ class CoinJoinWalletManager {
         }
     }
 
-    void Add(CWallet& wallet);
+    void Add(const std::shared_ptr<CWallet>& wallet);
     void DoMaintenance();
 
     void Remove(const std::string& name);
@@ -138,7 +138,7 @@ class CoinJoinWalletManager {
 class CCoinJoinClientSession : public CCoinJoinBaseSession
 {
 private:
-    CWallet& m_wallet;
+    const std::shared_ptr<CWallet> m_wallet;
     CoinJoinWalletManager& m_walletman;
     CCoinJoinClientManager& m_clientman;
     CDeterministicMNManager& m_dmnman;
@@ -163,15 +163,15 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     /// Create denominations
     bool CreateDenominated(CAmount nBalanceToDenominate);
     bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals)
-        EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet);
+        EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet);
 
     /// Split up large inputs or make fee sized inputs
     bool MakeCollateralAmounts();
     bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated)
-        EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet);
+        EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet);
 
     bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason)
-        EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet);
+        EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet);
 
     bool JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman);
     bool StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman);
@@ -181,7 +181,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     /// step 1: prepare denominated inputs and outputs
     bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<CTxDSIn>& vecTxDSIn,
                            std::vector<std::pair<CTxDSIn, CTxOut>>& vecPSInOutPairsRet, bool fDryRun = false)
-        EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet);
+        EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet);
     /// step 2: send denominated inputs and outputs prepared in step 1
     bool SendDenominate(const std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsIn, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
 
@@ -200,7 +200,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
     void SetNull() override EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
 
 public:
-    explicit CCoinJoinClientSession(CWallet& wallet, CoinJoinWalletManager& walletman,
+    explicit CCoinJoinClientSession(const std::shared_ptr<CWallet>& wallet, CoinJoinWalletManager& walletman,
                                     CCoinJoinClientManager& clientman, CDeterministicMNManager& dmnman,
                                     CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync,
                                     const std::unique_ptr<CCoinJoinClientQueueManager>& queueman, bool is_masternode);
@@ -267,7 +267,7 @@ class CCoinJoinClientQueueManager : public CCoinJoinBaseManager
 class CCoinJoinClientManager
 {
 private:
-    CWallet& m_wallet;
+    const std::shared_ptr<CWallet> m_wallet;
     CoinJoinWalletManager& m_walletman;
     CDeterministicMNManager& m_dmnman;
     CMasternodeMetaMan& m_mn_metaman;
@@ -306,11 +306,19 @@ class CCoinJoinClientManager
     CCoinJoinClientManager(CCoinJoinClientManager const&) = delete;
     CCoinJoinClientManager& operator=(CCoinJoinClientManager const&) = delete;
 
-    explicit CCoinJoinClientManager(CWallet& wallet, CoinJoinWalletManager& walletman, CDeterministicMNManager& dmnman,
-                                    CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync,
+    explicit CCoinJoinClientManager(const std::shared_ptr<CWallet>& wallet, CoinJoinWalletManager& walletman,
+                                    CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
+                                    const CMasternodeSync& mn_sync,
                                     const std::unique_ptr<CCoinJoinClientQueueManager>& queueman, bool is_masternode) :
-        m_wallet(wallet), m_walletman(walletman), m_dmnman(dmnman), m_mn_metaman(mn_metaman), m_mn_sync(mn_sync), m_queueman(queueman),
-        m_is_masternode{is_masternode} {}
+        m_wallet(wallet),
+        m_walletman(walletman),
+        m_dmnman(dmnman),
+        m_mn_metaman(mn_metaman),
+        m_mn_sync(mn_sync),
+        m_queueman(queueman),
+        m_is_masternode{is_masternode}
+    {
+    }
 
     void ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
 
diff --git a/src/coinjoin/interfaces.cpp b/src/coinjoin/interfaces.cpp
@@ -67,10 +67,7 @@ class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
     explicit CoinJoinLoaderImpl(CoinJoinWalletManager& walletman)
         : m_walletman(walletman) {}
 
-    void AddWallet(CWallet& wallet) override
-    {
-        m_walletman.Add(wallet);
-    }
+    void AddWallet(const std::shared_ptr<CWallet>& wallet) override { m_walletman.Add(wallet); }
     void RemoveWallet(const std::string& name) override
     {
         m_walletman.Remove(name);
diff --git a/src/coinjoin/util.cpp b/src/coinjoin/util.cpp
@@ -108,7 +108,7 @@ bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount)
     return true;
 }
 
-CTransactionBuilder::CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn) :
+CTransactionBuilder::CTransactionBuilder(const std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn) :
     pwallet(pwalletIn),
     dummyReserveDestination(pwalletIn.get()),
     tallyItem(tallyItemIn)
diff --git a/src/coinjoin/util.h b/src/coinjoin/util.h
@@ -77,7 +77,7 @@ class CTransactionBuilderOutput
 class CTransactionBuilder
 {
     /// Wallet the transaction will be build for
-    std::shared_ptr<CWallet> pwallet;
+    const std::shared_ptr<CWallet> pwallet;
     /// See CTransactionBuilder() for initialization
     CCoinControl coinControl;
     /// Dummy since we anyway use tallyItem's destination as change destination in coincontrol.
@@ -100,7 +100,7 @@ class CTransactionBuilder
     friend class CTransactionBuilderOutput;
 
 public:
-    CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn);
+    CTransactionBuilder(const std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn);
     ~CTransactionBuilder();
     /// Check it would be possible to add a single output with the amount nAmount. Returns true if its possible and false if not.
     bool CouldAddOutput(CAmount nAmountOutput) const EXCLUSIVE_LOCKS_REQUIRED(!cs_outputs);
diff --git a/src/interfaces/coinjoin.h b/src/interfaces/coinjoin.h
@@ -33,7 +33,7 @@ class Loader
 public:
     virtual ~Loader() {}
     //! Add new wallet to CoinJoin client manager
-    virtual void AddWallet(CWallet&) = 0;
+    virtual void AddWallet(const std::shared_ptr<CWallet>&) = 0;
     //! Remove wallet from CoinJoin client manager
     virtual void RemoveWallet(const std::string&) = 0;
     virtual void FlushWallet(const std::string&) = 0;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -125,7 +125,7 @@ bool AddWallet(const std::shared_ptr<CWallet>& wallet)
     }
     wallet->ConnectScriptPubKeyManNotifiers();
     wallet->AutoLockMasternodeCollaterals();
-    wallet->coinjoin_loader().AddWallet(*wallet);
+    wallet->coinjoin_loader().AddWallet(wallet);
     wallet->NotifyCanGetAddressesChanged();
     return true;
 }
@@ -1432,46 +1432,46 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound
     if (wtx == nullptr || wtx->tx == nullptr) {
         // no such tx in this wallet
         *nRoundsRef = -1;
-        WalletCJLogPrint((*this), "%s FAILED    %-70s %3d\n", __func__, outpoint.ToStringShort(), -1);
+        WalletCJLogPrint(this, "%s FAILED    %-70s %3d\n", __func__, outpoint.ToStringShort(), -1);
         return *nRoundsRef;
     }
 
     // bounds check
     if (outpoint.n >= wtx->tx->vout.size()) {
         // should never actually hit this
         *nRoundsRef = -4;
-        WalletCJLogPrint((*this), "%s FAILED    %-70s %3d\n", __func__, outpoint.ToStringShort(), -4);
+        WalletCJLogPrint(this, "%s FAILED    %-70s %3d\n", __func__, outpoint.ToStringShort(), -4);
         return *nRoundsRef;
     }
 
     auto txOutRef = &wtx->tx->vout[outpoint.n];
 
     if (CoinJoin::IsCollateralAmount(txOutRef->nValue)) {
         *nRoundsRef = -3;
-        WalletCJLogPrint((*this), "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
+        WalletCJLogPrint(this, "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
         return *nRoundsRef;
     }
 
     // make sure the final output is non-denominate
     if (!CoinJoin::IsDenominatedAmount(txOutRef->nValue)) { //NOT DENOM
         *nRoundsRef = -2;
-        WalletCJLogPrint((*this), "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
+        WalletCJLogPrint(this, "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
         return *nRoundsRef;
     }
 
     for (const auto& out : wtx->tx->vout) {
         if (!CoinJoin::IsDenominatedAmount(out.nValue)) {
             // this one is denominated but there is another non-denominated output found in the same tx
             *nRoundsRef = 0;
-            WalletCJLogPrint((*this), "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
+            WalletCJLogPrint(this, "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
             return *nRoundsRef;
         }
     }
 
     // make sure we spent all of it with 0 fee, reset to 0 rounds otherwise
     if (wtx->GetDebit(ISMINE_SPENDABLE) != wtx->GetCredit(ISMINE_SPENDABLE)) {
         *nRoundsRef = 0;
-        WalletCJLogPrint((*this), "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
+        WalletCJLogPrint(this, "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
         return *nRoundsRef;
     }
 
@@ -1491,7 +1491,7 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound
     *nRoundsRef = fDenomFound
             ? (nShortest >= nRoundsMax - 1 ? nRoundsMax : nShortest + 1) // good, we a +1 to the shortest one but only nRoundsMax rounds max allowed
             : 0;            // too bad, we are the fist one in that chain
-    WalletCJLogPrint((*this), "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
+    WalletCJLogPrint(this, "%s UPDATED   %-70s %3d\n", __func__, outpoint.ToStringShort(), *nRoundsRef);
     return *nRoundsRef;
 }
 
@@ -3253,7 +3253,7 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve
     CCoinControl coin_control;
     coin_control.nCoinType = CoinType::ONLY_READY_TO_MIX;
     AvailableCoins(vCoins, &coin_control);
-    WalletCJLogPrint((*this), "CWallet::%s -- vCoins.size(): %d\n", __func__, vCoins.size());
+    WalletCJLogPrint(this, "CWallet::%s -- vCoins.size(): %d\n", __func__, vCoins.size());
 
     Shuffle(vCoins.rbegin(), vCoins.rend(), FastRandomContext());
 
@@ -3271,11 +3271,11 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve
         nValueTotal += nValue;
         vecTxDSInRet.emplace_back(CTxDSIn(txin, scriptPubKey, nRounds));
         setRecentTxIds.emplace(txHash);
-        WalletCJLogPrint((*this), "CWallet::%s -- hash: %s, nValue: %d.%08d\n",
+        WalletCJLogPrint(this, "CWallet::%s -- hash: %s, nValue: %d.%08d\n",
                         __func__, txHash.ToString(), nValue / COIN, nValue % COIN);
     }
 
-    WalletCJLogPrint((*this), "CWallet::%s -- setRecentTxIds.size(): %d\n", __func__, setRecentTxIds.size());
+    WalletCJLogPrint(this, "CWallet::%s -- setRecentTxIds.size(): %d\n", __func__, setRecentTxIds.size());
 
     return nValueTotal > 0;
 }
@@ -4980,7 +4980,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, interfaces::C
     }
 
     if (coinjoin_loader) {
-        coinjoin_loader->AddWallet(*walletInstance);
+        coinjoin_loader->AddWallet(walletInstance);
     }
 
     {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -153,7 +153,7 @@ extern const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS;
 #define WalletCJLogPrint(wallet, ...)                                  \
     do {                                                               \
         if (LogAcceptCategory(BCLog::COINJOIN, BCLog::Level::Debug)) { \
-            wallet.WalletLogPrintf(__VA_ARGS__);                       \
+            wallet->WalletLogPrintf(__VA_ARGS__);                      \
         }                                                              \
     } while (0)
 

Original file line number	Diff line number	Diff line change
`@@ -108,7 +108,7 @@ bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount)`
`108`	`108`	`return true;`
`109`	`109`	`}`
`110`	`110`
`111`		`-CTransactionBuilder::CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn) :`
	`111`	`+CTransactionBuilder::CTransactionBuilder(const std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn) :`
`112`	`112`	`pwallet(pwalletIn),`
`113`	`113`	`dummyReserveDestination(pwalletIn.get()),`
`114`	`114`	`tallyItem(tallyItemIn)`