Skip to content

Fix data races triggered by functional tests. #4247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 1 commit into from
Jul 26, 2021
Merged

Fix data races triggered by functional tests. #4247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/coinjoin/coinjoin-server.cpp
View file
Open in desktop
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
Open in desktop
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
93 changes: 41 additions & 52 deletions src/evo/mnauth.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,30 @@

void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman)
{
LOCK(activeMasternodeInfoCs);
if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) {
return;
}

uint256 signHash;
{
LOCK(pnode->cs_mnauth);
if (pnode->receivedMNAuthChallenge.IsNull()) {
return;
}
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
// we protect ourselves against MITM in this form:
// node1 <- Eve -> node2
// It does not protect against:
// node1 -> Eve -> node2
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
}
auto receivedMNAuthChallenge = pnode->GetReceivedMNAuthChallenge();
if (receivedMNAuthChallenge.IsNull()) {
return;
}
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
// we protect ourselves against MITM in this form:
// node1 <- Eve -> node2
// It does not protect against:
// node1 -> Eve -> node2
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
}

CMNAuth mnauth;
Expand All @@ -66,11 +65,7 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
vRecv >> mnauth;

// only one MNAUTH allowed
bool fAlreadyHaveMNAUTH = false;
{
LOCK(pnode->cs_mnauth);
fAlreadyHaveMNAUTH = !pnode->verifiedProRegTxHash.IsNull();
}
bool fAlreadyHaveMNAUTH = !pnode->GetVerifiedProRegTxHash().IsNull();
if (fAlreadyHaveMNAUTH) {
LOCK(cs_main);
Misbehaving(pnode->GetId(), 100, "duplicate mnauth");
Expand Down Expand Up @@ -108,20 +103,17 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
}

uint256 signHash;
{
LOCK(pnode->cs_mnauth);
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound, pnode->nVersion.load()));
}
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound, pnode->nVersion.load()));
}
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());

if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) {
LOCK(cs_main);
Expand All @@ -147,12 +139,12 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
return;
}

if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) {
if (pnode2->GetVerifiedProRegTxHash() == 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 Expand Up @@ -181,13 +173,10 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
return;
}

{
LOCK(pnode->cs_mnauth);
pnode->verifiedProRegTxHash = mnauth.proRegTxHash;
pnode->verifiedPubKeyHash = dmn->pdmnState->pubKeyOperator.GetHash();
}
pnode->SetVerifiedProRegTxHash(mnauth.proRegTxHash);
pnode->SetVerifiedPubKeyHash(dmn->pdmnState->pubKeyOperator.GetHash());

if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->verifiedProRegTxHash)) {
if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->GetVerifiedProRegTxHash())) {
// Tell our peer that we're interested in plain LLMQ recovered signatures.
// Otherwise the peer would only announce/send messages resulting from QRECSIG,
// e.g. InstantSend locks or ChainLocks. SPV and regular full nodes should not send
Expand All @@ -209,11 +198,11 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
}

g_connman->ForEachNode([&](CNode* pnode) {
LOCK(pnode->cs_mnauth);
if (pnode->verifiedProRegTxHash.IsNull()) {
auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash();
if (verifiedProRegTxHash.IsNull()) {
return;
}
auto verifiedDmn = oldMNList.GetMN(pnode->verifiedProRegTxHash);
auto verifiedDmn = oldMNList.GetMN(verifiedProRegTxHash);
if (!verifiedDmn) {
return;
}
Expand All @@ -223,15 +212,15 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
} else {
auto it = diff.updatedMNs.find(verifiedDmn->GetInternalId());
if (it != diff.updatedMNs.end()) {
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->verifiedPubKeyHash) {
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->GetVerifiedPubKeyHash()) {
doRemove = true;
}
}
}

if (doRemove) {
LogPrint(BCLog::NET_NETCONN, "CMNAuth::NotifyMasternodeListChanged -- Disconnecting MN %s due to key changed/removed, peer=%d\n",
pnode->verifiedProRegTxHash.ToString(), pnode->GetId());
verifiedProRegTxHash.ToString(), pnode->GetId());
pnode->fDisconnect = true;
}
});
Expand Down
30 changes: 20 additions & 10 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,9 +369,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 @@ -2329,8 +2332,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 @@ -2341,11 +2348,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
Loading