Skip to content

Commit

Permalink
Fix data races triggered by functional tests.
Browse files Browse the repository at this point in the history 
Function CWallet::KeepKey requires locking as it has concurrent access to database and member nKeysLeftSinceAutoBackup.

Avoid data race when reading setInventoryTxToSend size by locking the read. If locking happens after the read, the size may change.

Lock cs_mnauth when reading verifiedProRegTxHash.

Make fRPCRunning atomic as it can be read/written from different threads simultaneously.

Make m_masternode_iqr_connection atomic as it can be read/written from different threads simultaneously.

Use a recursive mutex to synchronize concurrent access to quorumVvec.

Make m_masternode_connection atomic as it can be read/written from different threads simultaneously.

Make m_masternode_probe_connection atomic as it can be read/written from different threads simultaneously.

Use a recursive mutex in order to lock access to activeMasterNode.

Use a recursive mutex to synchronize concurrent access to skShare.
  • Loading branch information
@fanquake @gabriel-bjg
fanquake authored and gabriel-bjg committed Jul 16, 2021
1 parent 9bb7a60 commit 9540a56
Show file tree
Hide file tree
Showing 17 changed files with 133 additions and 85 deletions.
10 changes: 5 additions & 5 deletions src/coinjoin/coinjoin-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand
LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CCoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */

auto mnList = deterministicMNManager->GetListAtChainTip();
auto dmn = mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint);
auto dmn = WITH_LOCK(activeMasternodeInfoCs, return mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint));
if (!dmn) {
PushStatus(pfrom, STATUS_REJECTED, ERR_MN_LIST, connman);
return;
Expand All @@ -68,7 +68,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand
if (!lockRecv) return;

for (const auto& q : vecCoinJoinQueue) {
if (q.masternodeOutpoint == activeMasternodeInfo.outpoint) {
if (WITH_LOCK(activeMasternodeInfoCs, return q.masternodeOutpoint == activeMasternodeInfo.outpoint)) {
// refuse to create another queue this often
LogPrint(BCLog::COINJOIN, "DSACCEPT -- last dsq is still in queue, refuse to mix\n");
PushStatus(pfrom, STATUS_REJECTED, ERR_RECENT, connman);
Expand Down Expand Up @@ -334,7 +334,7 @@ void CCoinJoinServer::CommitFinalTransaction(CConnman& connman)

// create and sign masternode dstx transaction
if (!CCoinJoin::GetDSTX(hashTx)) {
CCoinJoinBroadcastTx dstxNew(finalTransaction, activeMasternodeInfo.outpoint, GetAdjustedTime());
CCoinJoinBroadcastTx dstxNew(finalTransaction, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime());
dstxNew.Sign();
CCoinJoin::AddDSTX(dstxNew);
}
Expand Down Expand Up @@ -501,7 +501,7 @@ void CCoinJoinServer::CheckForCompleteQueue(CConnman& connman)
if (nState == POOL_STATE_QUEUE && IsSessionReady()) {
SetState(POOL_STATE_ACCEPTING_ENTRIES);

CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), true);
CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), true);
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckForCompleteQueue -- queue is ready, signing and relaying (%s) " /* Continued */
"with %d participants\n", dsq.ToString(), vecSessionCollaterals.size());
dsq.Sign();
Expand Down Expand Up @@ -708,7 +708,7 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage&

if (!fUnitTest) {
//broadcast that I'm accepting entries, only if it's the first entry through
CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), false);
CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), false);
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString());
dsq.Sign();
dsq.Relay(connman);
Expand Down
4 changes: 2 additions & 2 deletions src/coinjoin/coinjoin.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ bool CCoinJoinQueue::Sign()


uint256 hash = GetSignatureHash();
CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
if (!sig.IsValid()) {
return false;
}
Expand Down Expand Up @@ -96,7 +96,7 @@ bool CCoinJoinBroadcastTx::Sign()

uint256 hash = GetSignatureHash();

CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
if (!sig.IsValid()) {
return false;
}
Expand Down
5 changes: 3 additions & 2 deletions src/evo/mnauth.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman)
{
LOCK(activeMasternodeInfoCs);
if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) {
return;
}
Expand Down Expand Up @@ -149,10 +150,10 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS

if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) {
if (fMasternodeMode) {
auto deterministicOutbound = llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash);
auto deterministicOutbound = WITH_LOCK(activeMasternodeInfoCs, return llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash));
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode %s has already verified as peer %d, deterministicOutbound=%s. peer=%d\n",
mnauth.proRegTxHash.ToString(), pnode2->GetId(), deterministicOutbound.ToString(), pnode->GetId());
if (deterministicOutbound == activeMasternodeInfo.proTxHash) {
if (WITH_LOCK(activeMasternodeInfoCs, return deterministicOutbound == activeMasternodeInfo.proTxHash)) {
if (pnode2->fInbound) {
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old inbound, peer=%d\n", pnode2->GetId());
pnode2->fDisconnect = true;
Expand Down
30 changes: 20 additions & 10 deletions src/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,12 @@ void PrepareShutdown()
UnregisterValidationInterface(activeMasternodeManager);
}

// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
activeMasternodeInfo.blsKeyOperator.reset();
activeMasternodeInfo.blsPubKeyOperator.reset();
{
LOCK(activeMasternodeInfoCs);
// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
activeMasternodeInfo.blsKeyOperator.reset();
activeMasternodeInfo.blsPubKeyOperator.reset();
}

#ifndef WIN32
try {
Expand Down Expand Up @@ -2316,8 +2319,12 @@ bool AppInitMain()
return InitError(_("Invalid masternodeblsprivkey. Please see documentation."));
}
fMasternodeMode = true;
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(activeMasternodeInfo.blsKeyOperator->GetPublicKey());
{
LOCK(activeMasternodeInfoCs);
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(
activeMasternodeInfo.blsKeyOperator->GetPublicKey());
}
LogPrintf("MASTERNODE:\n");
LogPrintf(" blsPubKeyOperator: %s\n", keyOperator.GetPublicKey().ToString());
}
Expand All @@ -2328,11 +2335,14 @@ bool AppInitMain()
RegisterValidationInterface(activeMasternodeManager);
}

if (activeMasternodeInfo.blsKeyOperator == nullptr) {
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
}
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
{
LOCK(activeMasternodeInfoCs);
if (activeMasternodeInfo.blsKeyOperator == nullptr) {
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
}
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
}
}

// ********************************************************* Step 10b: setup CoinJoin
Expand Down
77 changes: 48 additions & 29 deletions src/llmq/quorums.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ void CQuorum::Init(const CFinalCommitmentPtr& _qc, const CBlockIndex* _pindexQuo

bool CQuorum::SetVerificationVector(const BLSVerificationVector& quorumVecIn)
{
if (::SerializeHash(quorumVecIn) != qc->quorumVvecHash) {
const auto quorumVecInSerialized = ::SerializeHash(quorumVecIn);

LOCK(cs);
if (quorumVecInSerialized != qc->quorumVvecHash) {
return false;
}
quorumVvec = std::make_shared<BLSVerificationVector>(quorumVecIn);
Expand All @@ -70,9 +73,10 @@ bool CQuorum::SetVerificationVector(const BLSVerificationVector& quorumVecIn)

bool CQuorum::SetSecretKeyShare(const CBLSSecretKey& secretKeyShare)
{
if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(GetMemberIndex(activeMasternodeInfo.proTxHash)))) {
if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(WITH_LOCK(activeMasternodeInfoCs, return GetMemberIndex(activeMasternodeInfo.proTxHash))))) {
return false;
}
LOCK(cs);
skShare = secretKeyShare;
return true;
}
Expand All @@ -99,15 +103,22 @@ bool CQuorum::IsValidMember(const uint256& proTxHash) const

CBLSPublicKey CQuorum::GetPubKeyShare(size_t memberIdx) const
{
if (quorumVvec == nullptr || memberIdx >= members.size() || !qc->validMembers[memberIdx]) {
LOCK(cs);
if (!HasVerificationVector() || memberIdx >= members.size() || !qc->validMembers[memberIdx]) {
return CBLSPublicKey();
}
auto& m = members[memberIdx];
return blsCache.BuildPubKeyShare(m->proTxHash, quorumVvec, CBLSId(m->proTxHash));
}

const CBLSSecretKey& CQuorum::GetSkShare() const
bool CQuorum::HasVerificationVector() const {
LOCK(cs);
return quorumVvec != nullptr;
}

CBLSSecretKey CQuorum::GetSkShare() const
{
LOCK(cs);
return skShare;
}

Expand All @@ -125,7 +136,8 @@ void CQuorum::WriteContributions(CEvoDB& evoDb) const
{
uint256 dbKey = MakeQuorumKey(*this);

if (quorumVvec != nullptr) {
LOCK(cs);
if (HasVerificationVector()) {
evoDb.GetRawDB().Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), *quorumVvec);
}
if (skShare.IsValid()) {
Expand All @@ -139,14 +151,14 @@ bool CQuorum::ReadContributions(CEvoDB& evoDb)

BLSVerificationVector qv;
if (evoDb.Read(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), qv)) {
quorumVvec = std::make_shared<BLSVerificationVector>(std::move(qv));
WITH_LOCK(cs, quorumVvec = std::make_shared<BLSVerificationVector>(std::move(qv)));
} else {
return false;
}

// We ignore the return value here as it is ok if this fails. If it fails, it usually means that we are not a
// member of the quorum but observed the whole DKG process to have the quorum verification vector.
evoDb.Read(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare);
WITH_LOCK(cs, evoDb.Read(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare));

return true;
}
Expand Down Expand Up @@ -197,8 +209,10 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex)

// First check if we are member of any quorum of this type
bool fWeAreQuorumTypeMember{false};
for (const auto& pQuorum : vecQuorums) {
if (pQuorum->IsValidMember(activeMasternodeInfo.proTxHash)) {

auto proTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash);
for (const auto &pQuorum : vecQuorums) {
if (pQuorum->IsValidMember(proTxHash)) {
fWeAreQuorumTypeMember = true;
break;
}
Expand All @@ -211,16 +225,16 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex)
}

uint16_t nDataMask{0};
const bool fWeAreQuorumMember = pQuorum->IsValidMember(activeMasternodeInfo.proTxHash);
const bool fWeAreQuorumMember = pQuorum->IsValidMember(proTxHash);
const bool fSyncForTypeEnabled = mapQuorumVvecSync.count(pQuorum->qc->llmqType) > 0;
const QvvecSyncMode syncMode = fSyncForTypeEnabled ? mapQuorumVvecSync.at(pQuorum->qc->llmqType) : QvvecSyncMode::Invalid;
const bool fSyncCurrent = syncMode == QvvecSyncMode::Always || (syncMode == QvvecSyncMode::OnlyIfTypeMember && fWeAreQuorumTypeMember);

if ((fWeAreQuorumMember || (fSyncForTypeEnabled && fSyncCurrent)) && pQuorum->quorumVvec == nullptr) {
if ((fWeAreQuorumMember || (fSyncForTypeEnabled && fSyncCurrent)) && !pQuorum->HasVerificationVector()) {
nDataMask |= llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR;
}

if (fWeAreQuorumMember && !pQuorum->skShare.IsValid()) {
if (fWeAreQuorumMember && !pQuorum->GetSkShare().IsValid()) {
nDataMask |= llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS;
}

Expand Down Expand Up @@ -266,7 +280,6 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const
{
const auto& llmq_params = GetLLMQParams(llmqType);

const auto& myProTxHash = activeMasternodeInfo.proTxHash;
auto lastQuorums = ScanQuorums(llmqType, pindexNew, (size_t)llmq_params.keepOldConnections);

auto connmanQuorumsToDelete = g_connman->GetMasternodeQuorums(llmqType);
Expand All @@ -277,7 +290,7 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const
connmanQuorumsToDelete.erase(curDkgBlock);

for (const auto& quorum : lastQuorums) {
if (CLLMQUtils::EnsureQuorumConnections(llmqType, quorum->pindexQuorum, myProTxHash)) {
if (CLLMQUtils::EnsureQuorumConnections(llmqType, quorum->pindexQuorum, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash))) {
continue;
}
if (connmanQuorumsToDelete.count(quorum->qc->quorumHash) > 0) {
Expand Down Expand Up @@ -339,8 +352,9 @@ bool CQuorumManager::BuildQuorumContributions(const CFinalCommitmentPtr& fqc, co
}

cxxtimer::Timer t2(true);
LOCK(quorum->cs);
quorum->quorumVvec = blsWorker.BuildQuorumVerificationVector(vvecs);
if (quorum->quorumVvec == nullptr) {
if (quorum->HasVerificationVector()) {
LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- failed to build quorumVvec\n", __func__);
// without the quorum vvec, there can't be a skShare, so we fail here. Failure is not fatal here, as it still
// allows to use the quorum as a non-member (verification through the quorum pub key)
Expand Down Expand Up @@ -517,10 +531,13 @@ size_t CQuorumManager::GetQuorumRecoveryStartOffset(const CQuorumCPtr pQuorum, c
});
std::sort(vecProTxHashes.begin(), vecProTxHashes.end());
size_t nIndex{0};
for (size_t i = 0; i < vecProTxHashes.size(); ++i) {
if (activeMasternodeInfo.proTxHash == vecProTxHashes[i]) {
nIndex = i;
break;
{
LOCK(activeMasternodeInfoCs);
for (size_t i = 0; i < vecProTxHashes.size(); ++i) {
if (activeMasternodeInfo.proTxHash == vecProTxHashes[i]) {
nIndex = i;
break;
}
}
}
return nIndex % pQuorum->qc->validMembers.size();
Expand Down Expand Up @@ -592,13 +609,12 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand,

// Check if request wants QUORUM_VERIFICATION_VECTOR data
if (request.GetDataMask() & CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR) {

if (!pQuorum->quorumVvec) {
if (!pQuorum->HasVerificationVector()) {
sendQDATA(CQuorumDataRequest::Errors::QUORUM_VERIFICATION_VECTOR_MISSING);
return;
}

ssResponseData << *pQuorum->quorumVvec;
WITH_LOCK(pQuorum->cs, ssResponseData << *pQuorum->quorumVvec);
}

// Check if request wants ENCRYPTED_CONTRIBUTIONS data
Expand Down Expand Up @@ -682,7 +698,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand,
// Check if request has ENCRYPTED_CONTRIBUTIONS data
if (request.GetDataMask() & CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS) {

if (pQuorum->quorumVvec->size() != pQuorum->params.threshold) {
if (WITH_LOCK(pQuorum->cs, return pQuorum->quorumVvec->size() != pQuorum->params.threshold)) {
errorHandler("No valid quorum verification vector available", 0); // Don't bump score because we asked for it
return;
}
Expand All @@ -698,8 +714,9 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand,

BLSSecretKeyVector vecSecretKeys;
vecSecretKeys.resize(vecEncrypted.size());
auto secret = WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator);
for (size_t i = 0; i < vecEncrypted.size(); ++i) {
if (!vecEncrypted[i].Decrypt(memberIdx, *activeMasternodeInfo.blsKeyOperator, vecSecretKeys[i], PROTOCOL_VERSION)) {
if (!vecEncrypted[i].Decrypt(memberIdx, secret, vecSecretKeys[i], PROTOCOL_VERSION)) {
errorHandler("Failed to decrypt");
return;
}
Expand All @@ -718,7 +735,7 @@ void CQuorumManager::ProcessMessage(CNode* pFrom, const std::string& strCommand,

void CQuorumManager::StartCachePopulatorThread(const CQuorumCPtr pQuorum) const
{
if (pQuorum->quorumVvec == nullptr) {
if (!pQuorum->HasVerificationVector()) {
return;
}

Expand Down Expand Up @@ -771,7 +788,7 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co

vecMemberHashes.reserve(pQuorum->qc->validMembers.size());
for (auto& member : pQuorum->members) {
if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != activeMasternodeInfo.proTxHash) {
if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash)) {
vecMemberHashes.push_back(member->proTxHash);
}
}
Expand All @@ -781,12 +798,13 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co

while (nDataMask > 0 && !quorumThreadInterrupt) {

if (nDataMask & llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR && pQuorum->quorumVvec != nullptr) {
if (nDataMask & llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR &&
pQuorum->HasVerificationVector()) {
nDataMask &= ~llmq::CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR;
printLog("Received quorumVvec");
}

if (nDataMask & llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS && pQuorum->skShare.IsValid()) {
if (nDataMask & llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS && pQuorum->GetSkShare().IsValid()) {
nDataMask &= ~llmq::CQuorumDataRequest::ENCRYPTED_CONTRIBUTIONS;
printLog("Received skShare");
}
Expand Down Expand Up @@ -818,13 +836,14 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co
printLog("Connect");
}

auto proTxHash = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash);
g_connman->ForEachNode([&](CNode* pNode) {

if (pCurrentMemberHash == nullptr || pNode->verifiedProRegTxHash != *pCurrentMemberHash) {
return;
}

if (quorumManager->RequestQuorumData(pNode, pQuorum->qc->llmqType, pQuorum->pindexQuorum, nDataMask, activeMasternodeInfo.proTxHash)) {
if (quorumManager->RequestQuorumData(pNode, pQuorum->qc->llmqType, pQuorum->pindexQuorum, nDataMask, proTxHash)) {
nTimeLastSuccess = GetAdjustedTime();
printLog("Requested");
} else {
Expand Down
Loading

0 comments on commit 9540a56

Please sign in to comment.