Skip to content

Less cs_main locks in quorums #2702

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
codablock merged 3 commits into from
Feb 15, 2019
Merged

Less cs_main locks in quorums #2702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
550065f
Drop cs_main from CQuorumManager::UpdatedBlockTip()
UdjinM6 Feb 13, 2019
81293f2
CLLMQUtils::IsQuorumActive() shouldn't require cs_main to be held
UdjinM6 Feb 13, 2019
a088b5e
Revert comment deletion
UdjinM6 Feb 14, 2019
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
64 changes: 36 additions & 28 deletions src/llmq/quorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,27 +166,23 @@ void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockI
return;
}

LOCK(cs_main);

for (auto& p : Params().GetConsensus().llmqs) {
EnsureQuorumConnections(p.first, pindexNew);
}
}

void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex* pindexNew)
{
AssertLockHeld(cs_main);

const auto& params = Params().GetConsensus().llmqs.at(llmqType);

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

auto connmanQuorumsToDelete = g_connman->GetMasternodeQuorums(llmqType);

// don't remove connections for the currently in-progress DKG round
int curDkgHeight = pindexNew->nHeight - (pindexNew->nHeight % params.dkgInterval);
auto curDkgBlock = chainActive[curDkgHeight]->GetBlockHash();
auto curDkgBlock = pindexNew->GetAncestor(curDkgHeight)->GetBlockHash();
connmanQuorumsToDelete.erase(curDkgBlock);

for (auto& quorum : lastQuorums) {
Expand Down Expand Up @@ -222,19 +218,14 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const
}
}

bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, std::shared_ptr<CQuorum>& quorum) const
bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, std::shared_ptr<CQuorum>& quorum) const
{
AssertLockHeld(cs_main);
assert(pindexQuorum);
assert(qc.quorumHash == pindexQuorum->GetBlockHash());

if (!mapBlockIndex.count(qc.quorumHash)) {
LogPrintf("CQuorumManager::%s -- block %s not found", __func__, qc.quorumHash.ToString());
return false;
}
auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qc.llmqType);
auto quorumIndex = mapBlockIndex[qc.quorumHash];
auto members = CLLMQUtils::GetAllQuorumMembers((Consensus::LLMQType)qc.llmqType, qc.quorumHash);

quorum->Init(qc.quorumHash, quorumIndex->nHeight, members, qc.validMembers, qc.quorumPublicKey);
quorum->Init(qc.quorumHash, pindexQuorum->nHeight, members, qc.validMembers, qc.quorumPublicKey);

bool hasValidVvec = false;
if (quorum->ReadContributions(evoDb)) {
Expand Down Expand Up @@ -301,11 +292,15 @@ bool CQuorumManager::HasQuorum(Consensus::LLMQType llmqType, const uint256& quor

std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, size_t maxCount)
{
LOCK(cs_main);
return ScanQuorums(llmqType, chainActive.Tip()->GetBlockHash(), maxCount);
const CBlockIndex* pindex;
{
LOCK(cs_main);
pindex = chainActive.Tip();
}
return ScanQuorums(llmqType, pindex, maxCount);
}

std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, const uint256& startBlock, size_t maxCount)
std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t maxCount)
{
std::vector<CQuorumCPtr> result;
result.reserve(maxCount);
Expand All @@ -318,19 +313,13 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp
return result;
}

LOCK(cs_main);
if (!mapBlockIndex.count(startBlock)) {
return result;
}

CBlockIndex* pindex = mapBlockIndex[startBlock];
pindex = chainActive[pindex->nHeight - (pindex->nHeight % params.dkgInterval)];
auto pindex = pindexStart->GetAncestor(pindexStart->nHeight - (pindexStart->nHeight % params.dkgInterval));

while (pindex != nullptr
&& pindex->nHeight >= params.dkgInterval
&& result.size() < maxCount
&& deterministicMNManager->IsDIP3Enforced(pindex->nHeight)) {
auto quorum = GetQuorum(llmqType, pindex->GetBlockHash());
auto quorum = GetQuorum(llmqType, pindex);
if (quorum) {
result.emplace_back(quorum);
}
Expand All @@ -348,7 +337,25 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp

CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash)
{
AssertLockHeld(cs_main);
CBlockIndex* pindexQuorum;
{
LOCK(cs_main);
auto quorumIt = mapBlockIndex.find(quorumHash);

if (quorumIt == mapBlockIndex.end()) {
LogPrintf("CQuorumManager::%s -- block %s not found", __func__, quorumHash.ToString());
return nullptr;
}
pindexQuorum = quorumIt->second;
}
return GetQuorum(llmqType, pindexQuorum);
}

CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindexQuorum)
{
assert(pindexQuorum);

auto quorumHash = pindexQuorum->GetBlockHash();

// we must check this before we look into the cache. Reorgs might have happened which would mean we might have
// cached quorums which are not in the active chain anymore
Expand All @@ -371,7 +378,8 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25
auto& params = Params().GetConsensus().llmqs.at(llmqType);

auto quorum = std::make_shared<CQuorum>(params, blsWorker);
if (!BuildQuorumFromCommitment(qc, quorum)) {

if (!BuildQuorumFromCommitment(qc, pindexQuorum, quorum)) {
return nullptr;
}

Expand Down
14 changes: 9 additions & 5 deletions src/llmq/quorums.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,20 +91,24 @@ class CQuorumManager

void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);

public:
bool HasQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);

// all these methods will lock cs_main
CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType,const uint256& quorumHash);
// all these methods will lock cs_main for a short period of time
CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);
CQuorumCPtr GetNewestQuorum(Consensus::LLMQType llmqType);
std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, size_t maxCount);
std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, const uint256& startBlock, size_t maxCount);

// this one is cs_main-free
std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t maxCount);

private:
// all private methods here are cs_main-free
void EnsureQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex *pindexNew);

bool BuildQuorumFromCommitment(const CFinalCommitment& qc, std::shared_ptr<CQuorum>& quorum) const;
bool BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, std::shared_ptr<CQuorum>& quorum) const;
bool BuildQuorumContributions(const CFinalCommitment& fqc, std::shared_ptr<CQuorum>& quorum) const;

CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindex);
};

extern CQuorumManager* quorumManager;
Expand Down
79 changes: 36 additions & 43 deletions src/llmq/quorums_signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,19 +259,15 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
return false;
}

CQuorumCPtr quorum;
{
LOCK(cs_main);
CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, recoveredSig.quorumHash);

quorum = quorumManager->GetQuorum(llmqType, recoveredSig.quorumHash);
if (!quorum) {
LogPrintf("CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
recoveredSig.quorumHash.ToString(), nodeId);
return false;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
return false;
}
if (!quorum) {
LogPrintf("CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
recoveredSig.quorumHash.ToString(), nodeId);
return false;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
return false;
}

if (!recoveredSig.sig.IsValid()) {
Expand Down Expand Up @@ -316,37 +312,34 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
}
}

{
LOCK(cs_main);
for (auto& p : retSigShares) {
NodeId nodeId = p.first;
auto& v = p.second;

for (auto it = v.begin(); it != v.end();) {
auto& recSig = *it;

Consensus::LLMQType llmqType = (Consensus::LLMQType) recSig.llmqType;
auto quorumKey = std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.quorumHash);
if (!retQuorums.count(quorumKey)) {
CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, recSig.quorumHash);
if (!quorum) {
LogPrintf("CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
recSig.quorumHash.ToString(), nodeId);
it = v.erase(it);
continue;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
LogPrintf("CSigningManager::%s -- quorum %s not active anymore, node=%d\n", __func__,
recSig.quorumHash.ToString(), nodeId);
it = v.erase(it);
continue;
}

retQuorums.emplace(quorumKey, quorum);
for (auto& p : retSigShares) {
NodeId nodeId = p.first;
auto& v = p.second;

for (auto it = v.begin(); it != v.end();) {
auto& recSig = *it;

Consensus::LLMQType llmqType = (Consensus::LLMQType) recSig.llmqType;
auto quorumKey = std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.quorumHash);
if (!retQuorums.count(quorumKey)) {
CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, recSig.quorumHash);
if (!quorum) {
LogPrintf("CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
recSig.quorumHash.ToString(), nodeId);
it = v.erase(it);
continue;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
LogPrintf("CSigningManager::%s -- quorum %s not active anymore, node=%d\n", __func__,
recSig.quorumHash.ToString(), nodeId);
it = v.erase(it);
continue;
}

++it;
retQuorums.emplace(quorumKey, quorum);
}

++it;
}
}
}
Expand Down Expand Up @@ -561,17 +554,17 @@ CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType
auto& llmqParams = Params().GetConsensus().llmqs.at(llmqType);
size_t poolSize = (size_t)llmqParams.signingActiveQuorumCount;

uint256 startBlock;
CBlockIndex* pindexStart;
{
LOCK(cs_main);
int startBlockHeight = signHeight - SIGN_HEIGHT_OFFSET;
if (startBlockHeight > chainActive.Height()) {
return nullptr;
}
startBlock = chainActive[startBlockHeight]->GetBlockHash();
pindexStart = chainActive[startBlockHeight];
}

auto quorums = quorumManager->ScanQuorums(llmqType, startBlock, poolSize);
auto quorums = quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
if (quorums.empty()) {
return nullptr;
}
Expand Down
61 changes: 27 additions & 34 deletions src/llmq/quorums_signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,31 +347,26 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedS
return false;
}

CQuorumCPtr quorum;
{
LOCK(cs_main);

quorum = quorumManager->GetQuorum(llmqType, batchedSigShares.quorumHash);
if (!quorum) {
// TODO should we ban here?
LogPrintf("CSigSharesManager::%s -- quorum %s not found, node=%d\n", __func__,
batchedSigShares.quorumHash.ToString(), nodeId);
return false;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
// quorum is too old
return false;
}
if (!quorum->IsMember(activeMasternodeInfo.proTxHash)) {
// we're not a member so we can't verify it (we actually shouldn't have received it)
return false;
}
if (quorum->quorumVvec == nullptr) {
// TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG
LogPrintf("CSigSharesManager::%s -- we don't have the quorum vvec for %s, no verification possible. node=%d\n", __func__,
batchedSigShares.quorumHash.ToString(), nodeId);
return false;
}
CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, batchedSigShares.quorumHash);
if (!quorum) {
// TODO should we ban here?
LogPrintf("CSigSharesManager::%s -- quorum %s not found, node=%d\n", __func__,
batchedSigShares.quorumHash.ToString(), nodeId);
return false;
}
if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
// quorum is too old
return false;
}
if (!quorum->IsMember(activeMasternodeInfo.proTxHash)) {
// we're not a member so we can't verify it (we actually shouldn't have received it)
return false;
}
if (quorum->quorumVvec == nullptr) {
// TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG
LogPrintf("CSigSharesManager::%s -- we don't have the quorum vvec for %s, no verification possible. node=%d\n", __func__,
batchedSigShares.quorumHash.ToString(), nodeId);
return false;
}

std::set<uint16_t> dupMembers;
Expand Down Expand Up @@ -994,17 +989,15 @@ void CSigSharesManager::Cleanup()
}
}

{
// Find quorums which became inactive

@codablock codablock Feb 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should probably stay

@UdjinM6 UdjinM6 Feb 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops. Fixed :)

LOCK(cs_main);
for (auto it = quorumsToCheck.begin(); it != quorumsToCheck.end(); ) {
if (CLLMQUtils::IsQuorumActive(it->first, it->second)) {
it = quorumsToCheck.erase(it);
} else {
++it;
}
// Find quorums which became inactive
for (auto it = quorumsToCheck.begin(); it != quorumsToCheck.end(); ) {
if (CLLMQUtils::IsQuorumActive(it->first, it->second)) {
it = quorumsToCheck.erase(it);
} else {
++it;
}
}

{
// Now delete sessions which are for inactive quorums
LOCK(cs);
Expand Down
2 changes: 0 additions & 2 deletions src/llmq/quorums_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,6 @@ std::set<size_t> CLLMQUtils::CalcDeterministicWatchConnections(Consensus::LLMQTy

bool CLLMQUtils::IsQuorumActive(Consensus::LLMQType llmqType, const uint256& quorumHash)
{
AssertLockHeld(cs_main);

auto& params = Params().GetConsensus().llmqs.at(llmqType);

// sig shares and recovered sigs are only accepted from recent/active quorums
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/rpcquorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ UniValue quorum_list(const JSONRPCRequest& request)
for (auto& p : Params().GetConsensus().llmqs) {
UniValue v(UniValue::VARR);

auto quorums = llmq::quorumManager->ScanQuorums(p.first, chainActive.Tip()->GetBlockHash(), count);
auto quorums = llmq::quorumManager->ScanQuorums(p.first, chainActive.Tip(), count);
for (auto& q : quorums) {
v.push_back(q->quorumHash.ToString());
}
Expand Down