Coinjoin cs adjustments (#4463)

* Adjust coinjoin-client-options.* by making values atomic, removing CCriticalSection, and adjusting constructor to be default

* coinjoin-client.* adjust cs_deqsessions usage

* coinjoin add GUARDED_BY notations

* coinjoin make some values atomic

* coinjoin: move lock to where needed

* Ensure values are properly protected

* LOCK(cs_vecqueue) as needed

* Ensure vecOutputs is properly protected

Signed-off-by: pasta <pasta@dashboost.org>
This commit is contained in:
PastaPastaPasta 2021-09-30 14:58:33 -04:00 committed by GitHub
parent 94959c9adc
commit 7508790282
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 70 additions and 68 deletions

View File

@ -22,28 +22,24 @@ CCoinJoinClientOptions& CCoinJoinClientOptions::Get()
void CCoinJoinClientOptions::SetEnabled(bool fEnabled)
{
CCoinJoinClientOptions& options = CCoinJoinClientOptions::Get();
LOCK(options.cs_cj_options);
options.fEnableCoinJoin = fEnabled;
}
void CCoinJoinClientOptions::SetMultiSessionEnabled(bool fEnabled)
{
CCoinJoinClientOptions& options = CCoinJoinClientOptions::Get();
LOCK(options.cs_cj_options);
options.fCoinJoinMultiSession = fEnabled;
}
void CCoinJoinClientOptions::SetRounds(int nRounds)
{
CCoinJoinClientOptions& options = CCoinJoinClientOptions::Get();
LOCK(options.cs_cj_options);
options.nCoinJoinRounds = nRounds;
}
void CCoinJoinClientOptions::SetAmount(CAmount amount)
{
CCoinJoinClientOptions& options = CCoinJoinClientOptions::Get();
LOCK(options.cs_cj_options);
options.nCoinJoinAmount = amount;
}
@ -51,7 +47,6 @@ void CCoinJoinClientOptions::Init()
{
assert(!CCoinJoinClientOptions::_instance);
static CCoinJoinClientOptions instance;
LOCK(instance.cs_cj_options);
instance.fCoinJoinMultiSession = gArgs.GetBoolArg("-coinjoinmultisession", DEFAULT_COINJOIN_MULTISESSION);
instance.nCoinJoinSessions = std::min(std::max((int)gArgs.GetArg("-coinjoinsessions", DEFAULT_COINJOIN_SESSIONS), MIN_COINJOIN_SESSIONS), MAX_COINJOIN_SESSIONS);
instance.nCoinJoinRounds = std::min(std::max((int)gArgs.GetArg("-coinjoinrounds", DEFAULT_COINJOIN_ROUNDS), MIN_COINJOIN_ROUNDS), MAX_COINJOIN_ROUNDS);

View File

@ -6,7 +6,8 @@
#define BITCOIN_COINJOIN_COINJOIN_CLIENT_OPTIONS_H
#include <amount.h>
#include <sync.h>
#include <atomic>
#include <mutex>
class UniValue;
@ -72,26 +73,16 @@ private:
static CCoinJoinClientOptions* _instance;
static std::once_flag onceFlag;
CCriticalSection cs_cj_options;
int nCoinJoinSessions;
int nCoinJoinRounds;
int nCoinJoinRandomRounds;
int nCoinJoinAmount;
int nCoinJoinDenomsGoal;
int nCoinJoinDenomsHardCap;
bool fEnableCoinJoin;
bool fCoinJoinMultiSession;
std::atomic<int> nCoinJoinSessions{DEFAULT_COINJOIN_SESSIONS};
std::atomic<int> nCoinJoinRounds{DEFAULT_COINJOIN_ROUNDS};
std::atomic<int> nCoinJoinRandomRounds{COINJOIN_RANDOM_ROUNDS};
std::atomic<int> nCoinJoinAmount{DEFAULT_COINJOIN_AMOUNT};
std::atomic<int> nCoinJoinDenomsGoal{DEFAULT_COINJOIN_DENOMS_GOAL};
std::atomic<int> nCoinJoinDenomsHardCap{DEFAULT_COINJOIN_DENOMS_HARDCAP};
std::atomic<bool> fEnableCoinJoin{false};
std::atomic<bool> fCoinJoinMultiSession{DEFAULT_COINJOIN_MULTISESSION};
CCoinJoinClientOptions() :
nCoinJoinRounds(DEFAULT_COINJOIN_ROUNDS),
nCoinJoinRandomRounds(COINJOIN_RANDOM_ROUNDS),
nCoinJoinAmount(DEFAULT_COINJOIN_AMOUNT),
nCoinJoinDenomsGoal(DEFAULT_COINJOIN_DENOMS_GOAL),
nCoinJoinDenomsHardCap(DEFAULT_COINJOIN_DENOMS_HARDCAP),
fEnableCoinJoin(false),
fCoinJoinMultiSession(DEFAULT_COINJOIN_MULTISESSION)
{
}
CCoinJoinClientOptions() = default;
CCoinJoinClientOptions(const CCoinJoinClientOptions& other) = delete;
CCoinJoinClientOptions& operator=(const CCoinJoinClientOptions&) = delete;

View File

@ -265,9 +265,9 @@ void CCoinJoinClientSession::ResetPool()
void CCoinJoinClientManager::ResetPool()
{
LOCK(cs_deqsessions);
nCachedLastSuccessBlock = 0;
vecMasternodesUsed.clear();
LOCK(cs_deqsessions);
for (auto& session : deqSessions) {
session.ResetPool();
}
@ -346,10 +346,10 @@ std::string CCoinJoinClientSession::GetStatus(bool fWaitForBlock) const
std::string CCoinJoinClientManager::GetStatuses()
{
LOCK(cs_deqsessions);
std::string strStatus;
bool fWaitForBlock = WaitForAnotherBlock();
LOCK(cs_deqsessions);
for (const auto& session : deqSessions) {
strStatus += session.GetStatus(fWaitForBlock) + "; ";
}
@ -358,9 +358,9 @@ std::string CCoinJoinClientManager::GetStatuses()
std::string CCoinJoinClientManager::GetSessionDenoms()
{
LOCK(cs_deqsessions);
std::string strSessionDenoms;
LOCK(cs_deqsessions);
for (const auto& session : deqSessions) {
strSessionDenoms += CCoinJoin::DenominationToString(session.nSessionDenom) + "; ";
}
@ -490,6 +490,7 @@ bool CCoinJoinClientSession::SendDenominate(const std::vector<std::pair<CTxDSIn,
LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SendDenominate -- Submitting partial tx %s", tx.ToString()); /* Continued */
// store our entry for later use
LOCK(cs_coinjoin);
vecEntries.emplace_back(vecTxDSInTmp, vecTxOutTmp, txMyCollateral);
RelayIn(vecEntries.back(), connman);
nTimeLastSuccessfulStep = GetTime();
@ -557,6 +558,8 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans
if (fMasternodeMode || pnode == nullptr) return false;
if (!mixingMasternode) return false;
LOCK(cs_coinjoin);
finalMutableTransaction = CMutableTransaction{finalTransactionNew};
LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString()); /* Continued */
@ -588,11 +591,11 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans
std::vector<CTxIn> sigs;
for (const auto& entry : vecEntries) {
for (const auto &entry: vecEntries) {
// Check that the final transaction has all our outputs
for (const auto& txout : entry.vecTxOut) {
for (const auto &txout: entry.vecTxOut) {
bool fFound = false;
for (const auto& txoutFinal : finalMutableTransaction.vout) {
for (const auto &txoutFinal: finalMutableTransaction.vout) {
if (txoutFinal == txout) {
fFound = true;
break;
@ -641,7 +644,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans
sigs.push_back(finalMutableTransaction.vin[nMyInputIndex]);
LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- nMyInputIndex: %d, sigs.size(): %d, scriptSig=%s\n",
__func__, nMyInputIndex, (int)sigs.size(), ScriptToAsmStr(finalMutableTransaction.vin[nMyInputIndex].scriptSig));
__func__, nMyInputIndex, (int) sigs.size(),ScriptToAsmStr(finalMutableTransaction.vin[nMyInputIndex].scriptSig));
}
}
@ -973,8 +976,8 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(CConnman& connman, bool fDr
LogPrint(BCLog::COINJOIN, " vecMasternodesUsed: new size: %d, threshold: %d\n", (int)vecMasternodesUsed.size(), nThreshold_high);
}
LOCK(cs_deqsessions);
bool fResult = true;
LOCK(cs_deqsessions);
if ((int)deqSessions.size() < CCoinJoinClientOptions::GetSessions()) {
deqSessions.emplace_back(mixingWallet);
}
@ -1865,11 +1868,11 @@ void CCoinJoinClientSession::GetJsonInfo(UniValue& obj) const
void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
{
LOCK(cs_deqsessions);
assert(obj.isObject());
obj.pushKV("running", IsMixing());
UniValue arrSessions(UniValue::VARR);
LOCK(cs_deqsessions);
for (const auto& session : deqSessions) {
if (session.GetState() != POOL_STATE_IDLE) {
UniValue objSession(UniValue::VOBJ);

View File

@ -174,9 +174,9 @@ private:
// Keep track of the used Masternodes
std::vector<COutPoint> vecMasternodesUsed;
// TODO: or map<denom, CCoinJoinClientSession> ??
std::deque<CCoinJoinClientSession> deqSessions;
mutable CCriticalSection cs_deqsessions;
// TODO: or map<denom, CCoinJoinClientSession> ??
std::deque<CCoinJoinClientSession> deqSessions GUARDED_BY(cs_deqsessions);
bool fMixing{false};

View File

@ -302,6 +302,8 @@ void CCoinJoinServer::CreateFinalTransaction(CConnman& connman)
{
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- FINALIZE TRANSACTIONS\n");
LOCK(cs_coinjoin);
CMutableTransaction txNew;
// make our new transaction
@ -329,7 +331,7 @@ void CCoinJoinServer::CommitFinalTransaction(CConnman& connman)
{
if (!fMasternodeMode) return; // check and relay final tx only on masternode
CTransactionRef finalTransaction = MakeTransactionRef(finalMutableTransaction);
CTransactionRef finalTransaction = WITH_LOCK(cs_coinjoin, return MakeTransactionRef(finalMutableTransaction));
uint256 hashTx = finalTransaction->GetHash();
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- finalTransaction=%s", finalTransaction->ToString()); /* Continued */
@ -395,6 +397,7 @@ void CCoinJoinServer::ChargeFees(CConnman& connman) const
std::vector<CTransactionRef> vecOffendersCollaterals;
if (nState == POOL_STATE_ACCEPTING_ENTRIES) {
LOCK(cs_coinjoin);
for (const auto& txCollateral : vecSessionCollaterals) {
bool fFound = false;
for (const auto& entry : vecEntries) {
@ -414,6 +417,7 @@ void CCoinJoinServer::ChargeFees(CConnman& connman) const
if (nState == POOL_STATE_SIGNING) {
// who didn't sign?
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
for (const auto& txdsin : entry.vecTxDSIn) {
if (!txdsin.fHasSig) {
@ -538,21 +542,23 @@ bool CCoinJoinServer::IsInputScriptSigValid(const CTxIn& txin) const
int nTxInIndex = -1;
CScript sigPubKey = CScript();
for (const auto& entry : vecEntries) {
for (const auto& txout : entry.vecTxOut) {
txNew.vout.push_back(txout);
}
for (const auto& txdsin : entry.vecTxDSIn) {
txNew.vin.push_back(txdsin);
if (txdsin.prevout == txin.prevout) {
nTxInIndex = i;
sigPubKey = txdsin.prevPubKey;
{
LOCK(cs_coinjoin);
for (const auto &entry: vecEntries) {
for (const auto &txout: entry.vecTxOut) {
txNew.vout.push_back(txout);
}
for (const auto &txdsin: entry.vecTxDSIn) {
txNew.vin.push_back(txdsin);
if (txdsin.prevout == txin.prevout) {
nTxInIndex = i;
sigPubKey = txdsin.prevPubKey;
}
i++;
}
i++;
}
}
if (nTxInIndex >= 0) { //might have to do this one input at a time?
txNew.vin[nTxInIndex].scriptSig = txin.scriptSig;
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- verifying scriptSig %s\n", ScriptToAsmStr(txin.scriptSig).substr(0, 24));
@ -599,7 +605,7 @@ bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, P
std::vector<CTxIn> vin;
for (const auto& txin : entry.vecTxDSIn) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- txin=%s\n", __func__, txin.ToString());
LOCK(cs_coinjoin);
for (const auto& inner_entry : vecEntries) {
for (const auto& txdsin : inner_entry.vecTxDSIn) {
if (txdsin.prevout == txin.prevout) {
@ -624,7 +630,7 @@ bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, P
return false;
}
vecEntries.push_back(entry);
WITH_LOCK(cs_coinjoin, vecEntries.push_back(entry));
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- adding entry %d of %d required\n", __func__, GetEntriesCount(), CCoinJoin::GetMaxPoolParticipants());
nMessageIDRet = MSG_ENTRIES_ADDED;
@ -636,6 +642,7 @@ bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew)
{
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
for (const auto& txdsin : entry.vecTxDSIn) {
if (txdsin.scriptSig == txinNew.scriptSig) {
@ -672,6 +679,7 @@ bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew)
// Check to make sure everything is signed
bool CCoinJoinServer::IsSignaturesComplete() const
{
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
for (const auto& txdsin : entry.vecTxDSIn) {
if (!txdsin.fHasSig) return false;
@ -730,6 +738,7 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage&
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString());
dsq.Sign();
dsq.Relay(connman);
LOCK(cs_vecqueue);
vecCoinJoinQueue.push_back(dsq);
}
@ -796,10 +805,11 @@ void CCoinJoinServer::RelayFinalTransaction(const CTransaction& txFinal, CConnma
__func__, nSessionID, nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom));
// final mixing tx with empty signatures should be relayed to mixing participants only
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
bool fOk = connman.ForNode(entry.addr, [&txFinal, &connman, this](CNode* pnode) {
CNetMsgMaker msgMaker(pnode->GetSendVersion());
connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSFINALTX, nSessionID, txFinal));
connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSFINALTX, nSessionID.load(), txFinal));
return true;
});
if (!fOk) {
@ -821,6 +831,7 @@ void CCoinJoinServer::RelayStatus(PoolStatusUpdate nStatusUpdate, CConnman& conn
{
unsigned int nDisconnected{};
// status updates should be relayed to mixing participants only
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
// make sure everyone is still connected
bool fOk = connman.ForNode(entry.addr, [&nStatusUpdate, &nMessageID, &connman, this](CNode* pnode) {
@ -859,10 +870,11 @@ void CCoinJoinServer::RelayCompletedTransaction(PoolMessage nMessageID, CConnman
__func__, nSessionID, nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom));
// final mixing tx with empty signatures should be relayed to mixing participants only
LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
bool fOk = connman.ForNode(entry.addr, [&nMessageID, &connman, this](CNode* pnode) {
CNetMsgMaker msgMaker(pnode->GetSendVersion());
connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSCOMPLETE, nSessionID, nMessageID));
connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSCOMPLETE, nSessionID.load(), nMessageID));
return true;
});
if (!fOk) {

View File

@ -98,7 +98,6 @@ CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBui
bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount)
{
LOCK(pTxBuilder->cs_outputs);
if (nNewAmount <= 0 || nNewAmount - nAmount > pTxBuilder->GetAmountLeft()) {
return false;
}
@ -200,8 +199,8 @@ bool CTransactionBuilder::CouldAddOutputs(const std::vector<CAmount>& vecOutputA
CTransactionBuilderOutput* CTransactionBuilder::AddOutput(CAmount nAmountOutput)
{
LOCK(cs_outputs);
if (CouldAddOutput(nAmountOutput)) {
LOCK(cs_outputs);
vecOutputs.push_back(std::make_unique<CTransactionBuilderOutput>(this, pwallet, nAmountOutput));
return vecOutputs.back().get();
}
@ -210,6 +209,7 @@ CTransactionBuilderOutput* CTransactionBuilder::AddOutput(CAmount nAmountOutput)
unsigned int CTransactionBuilder::GetBytesTotal() const
{
LOCK(cs_outputs);
// Adding other outputs can change the serialized size of the vout size hence + GetSizeOfCompactSizeDiff()
return nBytesBase + vecOutputs.size() * nBytesOutput + ::GetSizeOfCompactSizeDiff(0, vecOutputs.size());
}
@ -222,6 +222,7 @@ CAmount CTransactionBuilder::GetAmountLeft(const CAmount nAmountInitial, const C
CAmount CTransactionBuilder::GetAmountUsed() const
{
CAmount nAmountUsed{0};
LOCK(cs_outputs);
for (const auto& out : vecOutputs) {
nAmountUsed += out->GetAmount();
}
@ -243,7 +244,7 @@ CAmount CTransactionBuilder::GetFee(unsigned int nBytes) const
int CTransactionBuilder::GetSizeOfCompactSizeDiff(size_t nAdd) const
{
size_t nSize = vecOutputs.size();
size_t nSize = WITH_LOCK(cs_outputs, return vecOutputs.size());
unsigned int ret = ::GetSizeOfCompactSizeDiff(nSize, nSize + nAdd);
assert(ret <= INT_MAX);
return (int)ret;

View File

@ -28,8 +28,8 @@ public:
class CKeyHolderStorage
{
private:
std::vector<std::unique_ptr<CKeyHolder> > storage;
mutable CCriticalSection cs_storage;
std::vector<std::unique_ptr<CKeyHolder> > storage GUARDED_BY(cs_storage);
public:
CScript AddKey(CWallet* pwalletIn);
@ -94,7 +94,7 @@ class CTransactionBuilder
/// Protect vecOutputs
mutable CCriticalSection cs_outputs;
/// Contains all outputs already added to the transaction
std::vector<std::unique_ptr<CTransactionBuilderOutput>> vecOutputs;
std::vector<std::unique_ptr<CTransactionBuilderOutput>> vecOutputs GUARDED_BY(cs_outputs);
/// Needed by CTransactionBuilderOutput::UpdateAmount to lock cs_outputs
friend class CTransactionBuilderOutput;
@ -114,7 +114,7 @@ public:
/// Check if an amounts should be considered as dust
bool IsDust(CAmount nAmount) const;
/// Get the total number of added outputs
int CountOutputs() const { return vecOutputs.size(); }
int CountOutputs() const { LOCK(cs_outputs); return vecOutputs.size(); }
/// Create and Commit the transaction to the wallet
bool Commit(std::string& strResult);
/// Convert to a string

View File

@ -303,8 +303,8 @@ bool CCoinJoinBaseSession::IsValidInOuts(const std::vector<CTxIn>& vin, const st
// Definitions for static data members
std::vector<CAmount> CCoinJoin::vecStandardDenominations;
std::map<uint256, CCoinJoinBroadcastTx> CCoinJoin::mapDSTX;
CCriticalSection CCoinJoin::cs_mapdstx;
std::map<uint256, CCoinJoinBroadcastTx> CCoinJoin::mapDSTX GUARDED_BY(CCoinJoin::cs_mapdstx);
void CCoinJoin::InitStandardDenominations()
{

View File

@ -337,14 +337,14 @@ class CCoinJoinBaseSession
protected:
mutable CCriticalSection cs_coinjoin;
std::vector<CCoinJoinEntry> vecEntries; // Masternode/clients entries
std::vector<CCoinJoinEntry> vecEntries GUARDED_BY(cs_coinjoin); // Masternode/clients entries
PoolState nState; // should be one of the POOL_STATE_XXX values
int64_t nTimeLastSuccessfulStep; // the time when last successful mixing step was performed
std::atomic<PoolState> nState; // should be one of the POOL_STATE_XXX values
std::atomic<int64_t> nTimeLastSuccessfulStep; // the time when last successful mixing step was performed
int nSessionID; // 0 if no mixing session is active
std::atomic<int> nSessionID; // 0 if no mixing session is active
CMutableTransaction finalMutableTransaction; // the finalized transaction ready for signing
CMutableTransaction finalMutableTransaction GUARDED_BY(cs_coinjoin); // the finalized transaction ready for signing
void SetNull();
@ -366,7 +366,7 @@ public:
int GetState() const { return nState; }
std::string GetStateString() const;
int GetEntriesCount() const { return vecEntries.size(); }
int GetEntriesCount() const { LOCK(cs_coinjoin); return vecEntries.size(); }
};
// base class
@ -376,7 +376,7 @@ protected:
mutable CCriticalSection cs_vecqueue;
// The current mixing sessions in progress on the network
std::vector<CCoinJoinQueue> vecCoinJoinQueue;
std::vector<CCoinJoinQueue> vecCoinJoinQueue GUARDED_BY(cs_vecqueue);
void SetNull();
void CheckQueue();
@ -385,7 +385,7 @@ public:
CCoinJoinBaseManager() :
vecCoinJoinQueue() {}
int GetQueueSize() const { return vecCoinJoinQueue.size(); }
int GetQueueSize() const { LOCK(cs_vecqueue); return vecCoinJoinQueue.size(); }
bool GetQueueItemAndTry(CCoinJoinQueue& dsqRet);
};