From e800d9d09c8837f2b5a4e89904accc2c5165e766 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 2 Dec 2024 17:20:50 +0300 Subject: [PATCH 1/3] fix: hold wallet shared pointer in CJ Manager/Sessions to prevent concurrent unload --- src/coinjoin/client.cpp | 132 ++++++++++++++++-------------------- src/coinjoin/client.h | 32 +++++---- src/coinjoin/interfaces.cpp | 5 +- src/coinjoin/util.cpp | 2 +- src/coinjoin/util.h | 4 +- src/interfaces/coinjoin.h | 2 +- src/wallet/wallet.cpp | 24 +++---- src/wallet/wallet.h | 2 +- 8 files changed, 96 insertions(+), 107 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 9804b1be5b..9ee0fa0d91 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -160,7 +160,7 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_cha } } -CCoinJoinClientSession::CCoinJoinClientSession(CWallet& wallet, CoinJoinWalletManager& walletman, +CCoinJoinClientSession::CCoinJoinClientSession(const std::shared_ptr& wallet, CoinJoinWalletManager& walletman, CCoinJoinClientManager& clientman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync, const std::unique_ptr& queueman, @@ -258,7 +258,7 @@ void CCoinJoinClientSession::ResetPool() { txMyCollateral = CMutableTransaction(); UnlockCoins(); - WITH_LOCK(m_wallet.cs_wallet, keyHolderStorage.ReturnAll()); + WITH_LOCK(m_wallet->cs_wallet, keyHolderStorage.ReturnAll()); WITH_LOCK(cs_coinjoin, SetNull()); } @@ -292,13 +292,13 @@ void CCoinJoinClientSession::UnlockCoins() if (!CCoinJoinClientOptions::IsEnabled()) return; while (true) { - TRY_LOCK(m_wallet.cs_wallet, lockWallet); + TRY_LOCK(m_wallet->cs_wallet, lockWallet); if (!lockWallet) { UninterruptibleSleep(std::chrono::milliseconds{50}); continue; } for (const auto& outpoint : vecOutPointLocked) - m_wallet.UnlockCoin(outpoint); + m_wallet->UnlockCoin(outpoint); break; } @@ -419,7 +419,7 @@ bool CCoinJoinClientSession::CheckTimeout() SetState(POOL_STATE_ERROR); UnlockCoins(); - WITH_LOCK(m_wallet.cs_wallet, keyHolderStorage.ReturnAll()); + WITH_LOCK(m_wallet->cs_wallet, keyHolderStorage.ReturnAll()); nTimeLastSuccessfulStep = GetTime(); strLastMessage = CoinJoin::GetMessageByID(ERR_SESSION); @@ -530,7 +530,7 @@ void CCoinJoinClientSession::ProcessPoolStateUpdate(CCoinJoinStatusUpdate psssup WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- rejected by Masternode: %s\n", __func__, strMessageTmp.translated); SetState(POOL_STATE_ERROR); UnlockCoins(); - WITH_LOCK(m_wallet.cs_wallet, keyHolderStorage.ReturnAll()); + WITH_LOCK(m_wallet->cs_wallet, keyHolderStorage.ReturnAll()); nTimeLastSuccessfulStep = GetTime(); strLastMessage = strMessageTmp; break; @@ -564,7 +564,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, CChainState& acti if (m_is_masternode) return false; if (!mixingMasternode) return false; - LOCK(m_wallet.cs_wallet); + LOCK(m_wallet->cs_wallet); LOCK(cs_coinjoin); finalMutableTransaction = CMutableTransaction{finalTransactionNew}; @@ -646,9 +646,9 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, CChainState& acti } // fill values for found outpoints - m_wallet.chain().findCoins(coins); + m_wallet->chain().findCoins(coins); std::map signing_errors; - m_wallet.SignTransaction(finalMutableTransaction, coins, SIGHASH_ALL | SIGHASH_ANYONECANPAY, signing_errors); + m_wallet->SignTransaction(finalMutableTransaction, coins, SIGHASH_ALL | SIGHASH_ANYONECANPAY, signing_errors); for (const auto& [input_index, error_string] : signing_errors) { // NOTE: this is a partial signing so it's expected for SignTransaction to return @@ -697,7 +697,7 @@ void CCoinJoinClientSession::CompletedTransaction(PoolMessage nMessageID) keyHolderStorage.KeepAll(); WalletCJLogPrint(m_wallet, "CompletedTransaction -- success\n"); } else { - WITH_LOCK(m_wallet.cs_wallet, keyHolderStorage.ReturnAll()); + WITH_LOCK(m_wallet->cs_wallet, keyHolderStorage.ReturnAll()); WalletCJLogPrint(m_wallet, "CompletedTransaction -- error\n"); } UnlockCoins(); @@ -725,14 +725,14 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return false; // We don't need auto-backups for descriptor wallets - if (!m_wallet.IsLegacy()) return true; + if (!m_wallet->IsLegacy()) return true; switch (nWalletBackups) { case 0: strAutoDenomResult = _("Automatic backups disabled") + Untranslated(", ") + _("no mixing available."); WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original); StopMixing(); - m_wallet.nKeysLeftSinceAutoBackup = 0; // no backup, no "keys since last backup" + m_wallet->nKeysLeftSinceAutoBackup = 0; // no backup, no "keys since last backup" return false; case -1: // Automatic backup failed, nothing else we can do until user fixes the issue manually. @@ -750,16 +750,18 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() return false; } - if (m_wallet.nKeysLeftSinceAutoBackup < COINJOIN_KEYS_THRESHOLD_STOP) { + if (m_wallet->nKeysLeftSinceAutoBackup < COINJOIN_KEYS_THRESHOLD_STOP) { // We should never get here via mixing itself but probably something else is still actively using keypool - strAutoDenomResult = strprintf(_("Very low number of keys left: %d") + Untranslated(", ") + _("no mixing available."), m_wallet.nKeysLeftSinceAutoBackup); + strAutoDenomResult = strprintf(_("Very low number of keys left: %d") + Untranslated(", ") + + _("no mixing available."), + m_wallet->nKeysLeftSinceAutoBackup); WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original); // It's getting really dangerous, stop mixing StopMixing(); return false; - } else if (m_wallet.nKeysLeftSinceAutoBackup < COINJOIN_KEYS_THRESHOLD_WARNING) { + } else if (m_wallet->nKeysLeftSinceAutoBackup < COINJOIN_KEYS_THRESHOLD_WARNING) { // Low number of keys left, but it's still more or less safe to continue - strAutoDenomResult = strprintf(_("Very low number of keys left: %d"), m_wallet.nKeysLeftSinceAutoBackup); + strAutoDenomResult = strprintf(_("Very low number of keys left: %d"), m_wallet->nKeysLeftSinceAutoBackup); WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original); if (fCreateAutoBackups) { @@ -767,7 +769,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() bilingual_str errorString; std::vector warnings; - if (!m_wallet.AutoBackupWallet("", errorString, warnings)) { + if (!m_wallet->AutoBackupWallet("", errorString, warnings)) { if (!warnings.empty()) { // There were some issues saving backup but yet more or less safe to continue WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- WARNING! Something went wrong on automatic backup: %s\n", Join(warnings, Untranslated("\n")).translated); @@ -785,7 +787,8 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() } } - WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- Keys left since latest backup: %d\n", m_wallet.nKeysLeftSinceAutoBackup); + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- Keys left since latest backup: %d\n", + m_wallet->nKeysLeftSinceAutoBackup); return true; } @@ -809,9 +812,9 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman CAmount nBalanceNeedsAnonymized; { - LOCK(m_wallet.cs_wallet); + LOCK(m_wallet->cs_wallet); - if (!fDryRun && m_wallet.IsLocked(true)) { + if (!fDryRun && m_wallet->IsLocked(true)) { strAutoDenomResult = _("Wallet is locked."); return false; } @@ -834,7 +837,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman return false; } - const auto bal = m_wallet.GetBalance(); + const auto bal = m_wallet->GetBalance(); // check if there is anything left to do CAmount nBalanceAnonymized = bal.m_anonymized; @@ -849,13 +852,13 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman CAmount nValueMin = CoinJoin::GetSmallestDenomination(); // if there are no confirmed DS collateral inputs yet - if (!m_wallet.HasCollateralInputs()) { + if (!m_wallet->HasCollateralInputs()) { // should have some additional amount for them nValueMin += CoinJoin::GetMaxCollateralAmount(); } // including denoms but applying some restrictions - CAmount nBalanceAnonymizable = m_wallet.GetAnonymizableBalance(); + CAmount nBalanceAnonymizable = m_wallet->GetAnonymizableBalance(); // mixable balance is way too small if (nBalanceAnonymizable < nValueMin) { @@ -865,7 +868,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman } // excluding denoms - CAmount nBalanceAnonimizableNonDenom = m_wallet.GetAnonymizableBalance(true); + CAmount nBalanceAnonimizableNonDenom = m_wallet->GetAnonymizableBalance(true); // denoms CAmount nBalanceDenominatedConf = bal.m_denominated_trusted; CAmount nBalanceDenominatedUnconf = bal.m_denominated_untrusted_pending; @@ -917,8 +920,8 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman } //check if we have the collateral sized inputs - if (!m_wallet.HasCollateralInputs()) { - return !m_wallet.HasCollateralInputs(false) && MakeCollateralAmounts(); + if (!m_wallet->HasCollateralInputs()) { + return !m_wallet->HasCollateralInputs(false) && MakeCollateralAmounts(); } if (nSessionID) { @@ -957,10 +960,10 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman } // lock the funds we're going to use for our collateral for (const auto& txin : txMyCollateral.vin) { - m_wallet.LockCoin(txin.prevout); + m_wallet->LockCoin(txin.prevout); vecOutPointLocked.push_back(txin.prevout); } - } // LOCK(m_wallet.cs_wallet); + } // LOCK(m_wallet->cs_wallet); // Always attempt to join an existing queue if (JoinExistingQueue(nBalanceNeedsAnonymized, connman)) { @@ -985,7 +988,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman return false; } - if (!fDryRun && m_wallet.IsLocked(true)) { + if (!fDryRun && m_wallet->IsLocked(true)) { strAutoDenomResult = _("Wallet is locked."); return false; } @@ -1107,7 +1110,7 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, std::vector vecTxDSInTmp; // Try to match their denominations if possible, select exact number of denominations - if (!m_wallet.SelectTxDSInsByDenomination(dsq.nDenom, nBalanceNeedsAnonymized, vecTxDSInTmp)) { + if (!m_wallet->SelectTxDSInsByDenomination(dsq.nDenom, nBalanceNeedsAnonymized, vecTxDSInTmp)) { WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Couldn't match denomination %d (%s)\n", dsq.nDenom, CoinJoin::DenominationToString(dsq.nDenom)); continue; } @@ -1153,7 +1156,7 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon // find available denominated amounts std::set setAmounts; - if (!m_wallet.SelectDenominatedAmounts(nBalanceNeedsAnonymized, setAmounts)) { + if (!m_wallet->SelectDenominatedAmounts(nBalanceNeedsAnonymized, setAmounts)) { // this should never happen strAutoDenomResult = _("Can't mix: no compatible inputs found!"); WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- %s\n", strAutoDenomResult.original); @@ -1288,7 +1291,7 @@ bool CCoinJoinClientManager::MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq) bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman) { - LOCK(m_wallet.cs_wallet); + LOCK(m_wallet->cs_wallet); std::string strError; std::vector vecTxDSIn; @@ -1342,7 +1345,7 @@ bool CCoinJoinClientSession::SelectDenominate(std::string& strErrorRet, std::vec { if (!CCoinJoinClientOptions::IsEnabled()) return false; - if (m_wallet.IsLocked(true)) { + if (m_wallet->IsLocked(true)) { strErrorRet = "Wallet locked, unable to create transaction!"; return false; } @@ -1354,7 +1357,7 @@ bool CCoinJoinClientSession::SelectDenominate(std::string& strErrorRet, std::vec vecTxDSInRet.clear(); - bool fSelected = m_wallet.SelectTxDSInsByDenomination(nSessionDenom, CoinJoin::GetMaxPoolAmount(), vecTxDSInRet); + bool fSelected = m_wallet->SelectTxDSInsByDenomination(nSessionDenom, CoinJoin::GetMaxPoolAmount(), vecTxDSInRet); if (!fSelected) { strErrorRet = "Can't select current denominated inputs"; return false; @@ -1365,7 +1368,7 @@ bool CCoinJoinClientSession::SelectDenominate(std::string& strErrorRet, std::vec bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector >& vecPSInOutPairsRet, bool fDryRun) { - AssertLockHeld(m_wallet.cs_wallet); + AssertLockHeld(m_wallet->cs_wallet); if (!CoinJoin::IsValidDenomination(nSessionDenom)) { strErrorRet = "Incorrect session denom"; @@ -1395,12 +1398,7 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s ++nSteps; continue; } - const auto pwallet = GetWallet(m_wallet.GetName()); - if (!pwallet) { - strErrorRet ="Couldn't get wallet pointer"; - return false; - } - scriptDenom = keyHolderStorage.AddKey(pwallet.get()); + scriptDenom = keyHolderStorage.AddKey(m_wallet.get()); } vecPSInOutPairsRet.emplace_back(entry, CTxOut(nDenomAmount, scriptDenom)); // step is complete @@ -1418,7 +1416,7 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s } for (const auto& [txDsIn, txDsOut] : vecPSInOutPairsRet) { - m_wallet.LockCoin(txDsIn.prevout); + m_wallet->LockCoin(txDsIn.prevout); vecOutPointLocked.push_back(txDsIn.prevout); } @@ -1430,13 +1428,13 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() { if (!CCoinJoinClientOptions::IsEnabled()) return false; - LOCK(m_wallet.cs_wallet); + LOCK(m_wallet->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. // Knowing that each CTxIn is at least 148 B big, 400 inputs should take 400 x ~148 B = ~60 kB. // This still leaves more than enough room for another data of typical MakeCollateralAmounts tx. - std::vector vecTally = m_wallet.SelectCoinsGroupedByAddresses(false, false, true, 400); + std::vector vecTally = m_wallet->SelectCoinsGroupedByAddresses(false, false, true, 400); if (vecTally.empty()) { WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::MakeCollateralAmounts -- SelectCoinsGroupedByAddresses can't find any inputs!\n"); return false; @@ -1448,14 +1446,14 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() }); // First try to use only non-denominated funds - if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) { return MakeCollateralAmounts(item, false); })) { return true; } // There should be at least some denominated funds we should be able to break in pieces to continue mixing - if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) { return MakeCollateralAmounts(item, true); })) { return true; @@ -1470,7 +1468,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated) { // TODO: consider refactoring to remove duplicated code with CCoinJoinClientSession::CreateDenominated - AssertLockHeld(m_wallet.cs_wallet); + AssertLockHeld(m_wallet->cs_wallet); if (!CCoinJoinClientOptions::IsEnabled()) return false; @@ -1484,14 +1482,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tally return false; } - const auto pwallet = GetWallet(m_wallet.GetName()); - - if (!pwallet) { - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- Couldn't get wallet pointer\n", __func__); - return false; - } - - CTransactionBuilder txBuilder(pwallet, tallyItem); + CTransactionBuilder txBuilder(m_wallet, tallyItem); WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- Start %s\n", __func__, txBuilder.ToString()); @@ -1562,13 +1553,13 @@ bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tally bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason) { - AssertLockHeld(m_wallet.cs_wallet); + AssertLockHeld(m_wallet->cs_wallet); std::vector vCoins; CCoinControl coin_control; coin_control.nCoinType = CoinType::ONLY_COINJOIN_COLLATERAL; - m_wallet.AvailableCoins(vCoins, &coin_control); + m_wallet->AvailableCoins(vCoins, &coin_control); if (vCoins.empty()) { strReason = strprintf("%s requires a collateral transaction and could not locate an acceptable input!", gCoinJoinName); @@ -1589,7 +1580,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx // make our change address CScript scriptChange; CTxDestination dest; - ReserveDestination reserveDest(&m_wallet); + ReserveDestination reserveDest(m_wallet.get()); bool success = reserveDest.GetReservedDestination(dest, true); assert(success); // should never fail, as we just unlocked scriptChange = GetScriptForDestination(dest); @@ -1601,7 +1592,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx txCollateral.vout.emplace_back(0, CScript() << OP_RETURN); } - if (!m_wallet.SignTransaction(txCollateral)) { + if (!m_wallet->SignTransaction(txCollateral)) { strReason = "Unable to sign collateral transaction!"; return false; } @@ -1614,13 +1605,13 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) { if (!CCoinJoinClientOptions::IsEnabled()) return false; - LOCK(m_wallet.cs_wallet); + LOCK(m_wallet->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. // Knowing that each CTxIn is at least 148 B big, 400 inputs should take 400 x ~148 B = ~60 kB. // This still leaves more than enough room for another data of typical CreateDenominated tx. - std::vector vecTally = m_wallet.SelectCoinsGroupedByAddresses(true, true, true, 400); + std::vector vecTally = m_wallet->SelectCoinsGroupedByAddresses(true, true, true, 400); if (vecTally.empty()) { WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::CreateDenominated -- SelectCoinsGroupedByAddresses can't find any inputs!\n"); return false; @@ -1631,7 +1622,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) return a.nAmount > b.nAmount; }); - bool fCreateMixingCollaterals = !m_wallet.HasCollateralInputs(); + bool fCreateMixingCollaterals = !m_wallet->HasCollateralInputs(); for (const auto& item : vecTally) { if (!CreateDenominated(nBalanceToDenominate, item, fCreateMixingCollaterals)) continue; @@ -1645,7 +1636,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate) // Create denominations bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals) { - AssertLockHeld(m_wallet.cs_wallet); + AssertLockHeld(m_wallet->cs_wallet); if (!CCoinJoinClientOptions::IsEnabled()) return false; @@ -1654,14 +1645,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con return false; } - const auto pwallet = GetWallet(m_wallet.GetName()); - - if (!pwallet) { - WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- Couldn't get wallet pointer\n", __func__); - return false; - } - - CTransactionBuilder txBuilder(pwallet, tallyItem); + CTransactionBuilder txBuilder(m_wallet, tallyItem); WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- Start %s\n", __func__, txBuilder.ToString()); @@ -1679,7 +1663,7 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con std::map mapDenomCount; for (auto nDenomValue : denoms) { - mapDenomCount.insert(std::pair(nDenomValue, m_wallet.CountInputsWithAmount(nDenomValue))); + mapDenomCount.insert(std::pair(nDenomValue, m_wallet->CountInputsWithAmount(nDenomValue))); } // Will generate outputs for the createdenoms up to coinjoinmaxdenoms per denom @@ -1922,11 +1906,11 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const obj.pushKV("sessions", arrSessions); } -void CoinJoinWalletManager::Add(CWallet& wallet) +void CoinJoinWalletManager::Add(const std::shared_ptr& wallet) { { LOCK(cs_wallet_manager_map); - m_wallet_manager_map.try_emplace(wallet.GetName(), + m_wallet_manager_map.try_emplace(wallet->GetName(), std::make_unique(wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_queueman, m_is_masternode)); } diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index 17239b9a71..0dd1cccb90 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -96,7 +96,7 @@ public: } } - void Add(CWallet& wallet); + void Add(const std::shared_ptr& wallet); void DoMaintenance(); void Remove(const std::string& name); @@ -138,7 +138,7 @@ private: class CCoinJoinClientSession : public CCoinJoinBaseSession { private: - CWallet& m_wallet; + const std::shared_ptr m_wallet; CoinJoinWalletManager& m_walletman; CCoinJoinClientManager& m_clientman; CDeterministicMNManager& m_dmnman; @@ -163,15 +163,15 @@ private: /// 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 @@ private: /// step 1: prepare denominated inputs and outputs bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector>& 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 >& vecPSInOutPairsIn, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); @@ -200,7 +200,7 @@ private: void SetNull() override EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin); public: - explicit CCoinJoinClientSession(CWallet& wallet, CoinJoinWalletManager& walletman, + explicit CCoinJoinClientSession(const std::shared_ptr& wallet, CoinJoinWalletManager& walletman, CCoinJoinClientManager& clientman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync, const std::unique_ptr& queueman, bool is_masternode); @@ -267,7 +267,7 @@ public: class CCoinJoinClientManager { private: - CWallet& m_wallet; + const std::shared_ptr m_wallet; CoinJoinWalletManager& m_walletman; CDeterministicMNManager& m_dmnman; CMasternodeMetaMan& m_mn_metaman; @@ -306,11 +306,19 @@ public: 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& wallet, CoinJoinWalletManager& walletman, + CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, + const CMasternodeSync& mn_sync, const std::unique_ptr& 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 index 78a9719431..6c40a3b6d3 100644 --- a/src/coinjoin/interfaces.cpp +++ b/src/coinjoin/interfaces.cpp @@ -67,10 +67,7 @@ public: explicit CoinJoinLoaderImpl(CoinJoinWalletManager& walletman) : m_walletman(walletman) {} - void AddWallet(CWallet& wallet) override - { - m_walletman.Add(wallet); - } + void AddWallet(const std::shared_ptr& 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 index d936baf354..8748606375 100644 --- 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 pwalletIn, const CompactTallyItem& tallyItemIn) : +CTransactionBuilder::CTransactionBuilder(const std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn) : pwallet(pwalletIn), dummyReserveDestination(pwalletIn.get()), tallyItem(tallyItemIn) diff --git a/src/coinjoin/util.h b/src/coinjoin/util.h index 672d27432a..e2a04fc69e 100644 --- a/src/coinjoin/util.h +++ b/src/coinjoin/util.h @@ -77,7 +77,7 @@ public: class CTransactionBuilder { /// Wallet the transaction will be build for - std::shared_ptr pwallet; + const std::shared_ptr 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 pwalletIn, const CompactTallyItem& tallyItemIn); + CTransactionBuilder(const std::shared_ptr 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 index 345780cde0..7f881bf07f 100644 --- 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&) = 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 index 346a2ec199..c396a5fb3e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -125,7 +125,7 @@ bool AddWallet(const std::shared_ptr& wallet) } wallet->ConnectScriptPubKeyManNotifiers(); wallet->AutoLockMasternodeCollaterals(); - wallet->coinjoin_loader().AddWallet(*wallet); + wallet->coinjoin_loader().AddWallet(wallet); wallet->NotifyCanGetAddressesChanged(); return true; } @@ -1432,7 +1432,7 @@ 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; } @@ -1440,7 +1440,7 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound 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; } @@ -1448,14 +1448,14 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound 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; } @@ -1463,7 +1463,7 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound 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; } } @@ -1471,7 +1471,7 @@ int CWallet::GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRound // 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::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 index 7b41966216..9538e877c9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -153,7 +153,7 @@ extern const std::map WALLET_FLAG_CAVEATS; #define WalletCJLogPrint(wallet, ...) \ do { \ if (LogAcceptCategory(BCLog::COINJOIN, BCLog::Level::Debug)) { \ - wallet.WalletLogPrintf(__VA_ARGS__); \ + wallet->WalletLogPrintf(__VA_ARGS__); \ } \ } while (0) From 0aeeb8583a0fbfd218c21d18433d78e4469382ab Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 2 Dec 2024 23:00:09 +0300 Subject: [PATCH 2/3] fix: add missing `AddWallet` call in `TestLoadWallet` --- src/wallet/test/wallet_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index e7aa85c6d4..edbebf46bc 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -59,6 +59,10 @@ static std::shared_ptr TestLoadWallet(interfaces::Chain* chain, interfa std::vector warnings; auto database = MakeWalletDatabase("", options, status, error); auto wallet = CWallet::Create(chain, coinjoin_loader, "", std::move(database), options.create_flags, error, warnings); + if (coinjoin_loader) { + // TODO: see CreateWalletWithoutChain + AddWallet(wallet); + } if (chain) { wallet->postInitProcess(); } From 2d7c7f81b8a4e2b31986129bcc76fc012a324a26 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 3 Dec 2024 12:54:42 +0300 Subject: [PATCH 3/3] fix: do not transfer wallet ownership to CTransactionBuilder{Output} --- src/coinjoin/util.cpp | 41 +++++++++++++++++++++-------------------- src/coinjoin/util.h | 6 +++--- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/coinjoin/util.cpp b/src/coinjoin/util.cpp index 8748606375..e188972b68 100644 --- a/src/coinjoin/util.cpp +++ b/src/coinjoin/util.cpp @@ -87,14 +87,15 @@ void CKeyHolderStorage::ReturnAll() } } -CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBuilderIn, std::shared_ptr pwalletIn, CAmount nAmountIn) : +CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBuilderIn, + const std::shared_ptr& wallet, CAmount nAmountIn) : pTxBuilder(pTxBuilderIn), - dest(pwalletIn.get()), + dest(wallet.get()), nAmount(nAmountIn) { assert(pTxBuilder); CTxDestination txdest; - LOCK(pwalletIn->cs_wallet); + LOCK(wallet->cs_wallet); dest.GetReservedDestination(txdest, false); script = ::GetScriptForDestination(txdest); } @@ -108,15 +109,15 @@ bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount) return true; } -CTransactionBuilder::CTransactionBuilder(const std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn) : - pwallet(pwalletIn), - dummyReserveDestination(pwalletIn.get()), +CTransactionBuilder::CTransactionBuilder(const std::shared_ptr& wallet, const CompactTallyItem& tallyItemIn) : + m_wallet(wallet), + dummyReserveDestination(wallet.get()), tallyItem(tallyItemIn) { // Generate a feerate which will be used to consider if the remainder is dust and will go into fees or not - coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet); + coinControl.m_discard_feerate = ::GetDiscardRate(*m_wallet); // Generate a feerate which will be used by calculations of this class and also by CWallet::CreateTransaction - coinControl.m_feerate = std::max(GetRequiredFeeRate(*pwallet), pwallet->m_pay_tx_fee); + coinControl.m_feerate = std::max(GetRequiredFeeRate(*m_wallet), m_wallet->m_pay_tx_fee); // Change always goes back to origin coinControl.destChange = tallyItemIn.txdest; // Only allow tallyItems inputs for tx creation @@ -131,16 +132,16 @@ CTransactionBuilder::CTransactionBuilder(const std::shared_ptr pwalletI // Get a comparable dummy scriptPubKey, avoid writing/flushing to the actual wallet db CScript dummyScript; { - LOCK(pwallet->cs_wallet); - WalletBatch dummyBatch(pwallet->GetDatabase(), false); + LOCK(m_wallet->cs_wallet); + WalletBatch dummyBatch(m_wallet->GetDatabase(), false); dummyBatch.TxnBegin(); CKey secret; - secret.MakeNewKey(pwallet->CanSupportFeature(FEATURE_COMPRPUBKEY)); + secret.MakeNewKey(m_wallet->CanSupportFeature(FEATURE_COMPRPUBKEY)); CPubKey dummyPubkey = secret.GetPubKey(); dummyBatch.TxnAbort(); dummyScript = ::GetScriptForDestination(PKHash(dummyPubkey)); // Calculate required bytes for the dummy signed tx with tallyItem's inputs only - nBytesBase = CalculateMaximumSignedTxSize(CTransaction(dummyTx), pwallet.get(), false); + nBytesBase = CalculateMaximumSignedTxSize(CTransaction(dummyTx), m_wallet.get(), false); } // Calculate the output size nBytesOutput = ::GetSerializeSize(CTxOut(0, dummyScript), PROTOCOL_VERSION); @@ -204,7 +205,7 @@ CTransactionBuilderOutput* CTransactionBuilder::AddOutput(CAmount nAmountOutput) { if (CouldAddOutput(nAmountOutput)) { LOCK(cs_outputs); - vecOutputs.push_back(std::make_unique(this, pwallet, nAmountOutput)); + vecOutputs.push_back(std::make_unique(this, m_wallet, nAmountOutput)); return vecOutputs.back().get(); } return nullptr; @@ -233,12 +234,12 @@ CAmount CTransactionBuilder::GetAmountUsed() const CAmount CTransactionBuilder::GetFee(unsigned int nBytes) const { CAmount nFeeCalc = coinControl.m_feerate->GetFee(nBytes); - CAmount nRequiredFee = GetRequiredFee(*pwallet, nBytes); + CAmount nRequiredFee = GetRequiredFee(*m_wallet, nBytes); if (nRequiredFee > nFeeCalc) { nFeeCalc = nRequiredFee; } - if (nFeeCalc > pwallet->m_default_max_tx_fee) { - nFeeCalc = pwallet->m_default_max_tx_fee; + if (nFeeCalc > m_wallet->m_default_max_tx_fee) { + nFeeCalc = m_wallet->m_default_max_tx_fee; } return nFeeCalc; } @@ -273,9 +274,9 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult) CTransactionRef tx; { - LOCK2(pwallet->cs_wallet, cs_main); + LOCK2(m_wallet->cs_wallet, cs_main); FeeCalculation fee_calc_out; - if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl, fee_calc_out)) { + if (!m_wallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl, fee_calc_out)) { return false; } } @@ -312,8 +313,8 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult) } { - LOCK2(pwallet->cs_wallet, cs_main); - pwallet->CommitTransaction(tx, {}, {}); + LOCK2(m_wallet->cs_wallet, cs_main); + m_wallet->CommitTransaction(tx, {}, {}); } fKeepKeys = true; diff --git a/src/coinjoin/util.h b/src/coinjoin/util.h index e2a04fc69e..f4e75aabc0 100644 --- a/src/coinjoin/util.h +++ b/src/coinjoin/util.h @@ -54,7 +54,7 @@ class CTransactionBuilderOutput CScript script; public: - CTransactionBuilderOutput(CTransactionBuilder* pTxBuilderIn, std::shared_ptr pwalletIn, CAmount nAmountIn); + CTransactionBuilderOutput(CTransactionBuilder* pTxBuilderIn, const std::shared_ptr& wallet, CAmount nAmountIn); CTransactionBuilderOutput(CTransactionBuilderOutput&&) = delete; CTransactionBuilderOutput& operator=(CTransactionBuilderOutput&&) = delete; /// Get the scriptPubKey of this output @@ -77,7 +77,7 @@ public: class CTransactionBuilder { /// Wallet the transaction will be build for - const std::shared_ptr pwallet; + const std::shared_ptr& m_wallet; /// 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(const std::shared_ptr pwalletIn, const CompactTallyItem& tallyItemIn); + CTransactionBuilder(const std::shared_ptr& wallet, 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);