From 3bcac495cf37510415cf804ef8350150dc880a3c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 16:44:30 +0300 Subject: [PATCH 1/7] feat: regular nodes should never receive qwatch --- src/llmq/dkgsessionmgr.cpp | 5 +++++ test/functional/p2p_quorum_data.py | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 9cf5439e71e2..6ccd8eb007b9 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -189,6 +189,11 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor } if (msg_type == NetMsgType::QWATCH) { + if (!fMasternodeMode) { + // non-masternodes should never receive this + m_peerman->Misbehaving(pfrom.GetId(), 10); + return; + } pfrom.qwatch = true; return; } diff --git a/test/functional/p2p_quorum_data.py b/test/functional/p2p_quorum_data.py index 74495e01c142..664bb601e1c8 100755 --- a/test/functional/p2p_quorum_data.py +++ b/test/functional/p2p_quorum_data.py @@ -154,6 +154,9 @@ def test_basics(): "and does bump our score") p2p_node0.test_qgetdata(qgetdata_all, response_expected=False) wait_for_banscore(node0, id_p2p_node0, 10) + self.log.info("Check that normal node bumps our score for qwatch") + p2p_node0.send_message(msg_qwatch()) + wait_for_banscore(node0, id_p2p_node0, 20) # The masternode should not respond to qgetdata for non-masternode connections self.log.info("Check that masternode doesn't respond to " "non-masternode connection and does bump our score") @@ -383,6 +386,10 @@ def test_watchquorums(): p2p_node0.wait_for_qmessage("qgetdata") p2p_node0.send_message(p2p_mn2.get_qdata()) wait_for_banscore(node0, id_p2p_node0, (1 - len(extra_args)) * 10) + # Non-masternodes should bump peer's score for qwatch no matter + # whether they (non-masternodes) are watching or not. + p2p_node0.send_message(msg_qwatch()) + wait_for_banscore(node0, id_p2p_node0, (1 - len(extra_args)) * 10 + 10) node0.disconnect_p2ps() mn2.node.disconnect_p2ps() From 96af24529e150292fde4a8966975ccc56765a49b Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 17:58:00 +0300 Subject: [PATCH 2/7] feat: make sure regular non-watching nodes never process most DKG messages, only the final one --- src/llmq/dkgsessionmgr.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 6ccd8eb007b9..e011a6ab8792 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -198,6 +198,12 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor return; } + if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled())) { + // regular non-watching nodes should never receive any of these + m_peerman->Misbehaving(pfrom.GetId(), 10); + return; + } + if (vRecv.empty()) { m_peerman->Misbehaving(pfrom.GetId(), 100); return; From ca5247771dfeb267d6575ba567358dfb4f0fc962 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 18:10:04 +0300 Subject: [PATCH 3/7] fix: should only request/accept qdata from a verified masternode, ignoring qwatch on our side Checking for `qwatch` doesn't make sense in these conditions because it's set on the node we are watching when it receives `QWATCH` message from us, it's always `false` on the watching node itself. --- src/llmq/quorums.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 879eb75701be..4a3218d8b64b 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -452,8 +452,8 @@ bool CQuorumManager::RequestQuorumData(CNode* pfrom, Consensus::LLMQType llmqTyp LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- Version must be %d or greater.\n", __func__, LLMQ_DATA_MESSAGES_VERSION); return false; } - if (pfrom->GetVerifiedProRegTxHash().IsNull() && !pfrom->qwatch) { - LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- pfrom is neither a verified masternode nor a qwatch connection\n", __func__); + if (pfrom->GetVerifiedProRegTxHash().IsNull()) { + LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- pfrom is not a verified masternode\n", __func__); return false; } if (!GetLLMQParams(llmqType).has_value()) { @@ -719,8 +719,8 @@ void CQuorumManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, C } if (msg_type == NetMsgType::QDATA) { - if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled()) || (pfrom.GetVerifiedProRegTxHash().IsNull() && !pfrom.qwatch)) { - errorHandler("Not a verified masternode or a qwatch connection"); + if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled()) || pfrom.GetVerifiedProRegTxHash().IsNull()) { + errorHandler("Not a verified masternode and -watchquorums is not enabled"); return; } From 825fac1035f0513376d4241305088bfa5455ea6c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 18:19:35 +0300 Subject: [PATCH 4/7] fix: regular non-watching nodes should not do any qdata related job --- src/llmq/quorums.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 4a3218d8b64b..8bdbebe72de4 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -223,7 +223,7 @@ void CQuorumManager::Stop() void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex) const { - if (!fMasternodeMode || !utils::QuorumDataRecoveryEnabled() || pIndex == nullptr) { + if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled()) || !utils::QuorumDataRecoveryEnabled() || pIndex == nullptr) { return; } @@ -274,15 +274,13 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex) void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload) const { - if (!m_mn_sync->IsBlockchainSynced()) { - return; - } + if (!m_mn_sync->IsBlockchainSynced()) return; for (const auto& params : Params().GetConsensus().llmqs) { CheckQuorumConnections(params, pindexNew); } - { + if (fMasternodeMode || utils::IsWatchQuorumsEnabled()) { // Cleanup expired data requests LOCK(cs_data_requests); auto it = mapQuorumDataRequests.begin(); @@ -1011,7 +1009,7 @@ void CQuorumManager::StartCleanupOldQuorumDataThread(const CBlockIndex* pIndex) // window and it's better to have more room so we pick next cycle. // dkgMiningWindowStart for small quorums is 10 i.e. a safe block to start // these calculations is at height 576 + 24 * 2 + 10 = 576 + 58. - if (!fMasternodeMode || pIndex == nullptr || (pIndex->nHeight % 576 != 58)) { + if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled()) || pIndex == nullptr || (pIndex->nHeight % 576 != 58)) { return; } From 92da3bf4cde66c20af02ee6400f5c5f557c8ade3 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 28 Nov 2023 00:11:47 +0300 Subject: [PATCH 5/7] fix: CheckQuorumConnections should not be running on regular non-watching nodes --- src/llmq/quorums.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 8bdbebe72de4..f604407e64f1 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -299,6 +299,8 @@ void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitial void CQuorumManager::CheckQuorumConnections(const Consensus::LLMQParams& llmqParams, const CBlockIndex* pindexNew) const { + if (!fMasternodeMode && !utils::IsWatchQuorumsEnabled()) return; + auto lastQuorums = ScanQuorums(llmqParams.type, pindexNew, (size_t)llmqParams.keepOldConnections); auto connmanQuorumsToDelete = connman.GetMasternodeQuorums(llmqParams.type); From 78e20ebfe14a0d973b5bfcbbec21dc1cb8816da1 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 18:11:02 +0300 Subject: [PATCH 6/7] refactor: s/exists/inserted/ to better match the logic --- src/llmq/quorums.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index f604407e64f1..2971f6df1121 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -472,8 +472,8 @@ bool CQuorumManager::RequestQuorumData(CNode* pfrom, Consensus::LLMQType llmqTyp LOCK(cs_data_requests); const CQuorumDataRequestKey key(pfrom->GetVerifiedProRegTxHash(), true, pQuorumBaseBlockIndex->GetBlockHash(), llmqType); const CQuorumDataRequest request(llmqType, pQuorumBaseBlockIndex->GetBlockHash(), nDataMask, proTxHash); - auto [old_pair, exists] = mapQuorumDataRequests.emplace(key, request); - if (!exists) { + auto [old_pair, inserted] = mapQuorumDataRequests.emplace(key, request); + if (!inserted) { if (old_pair->second.IsExpired(/*add_bias=*/true)) { old_pair->second = request; } else { From 33b5ca1917f9665b3b504c59a9acff189a8fcdcf Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 29 Nov 2023 18:31:05 +0300 Subject: [PATCH 7/7] trivial: add logging for GetEncryptedContributions failures --- src/llmq/dkgsessionmgr.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index e011a6ab8792..54b8255da54b 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -440,6 +440,7 @@ bool CDKGSessionManager::GetEncryptedContributions(Consensus::LLMQType llmqType, } } if (nRequestedMemberIdx == std::numeric_limits::max()) { + LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- not a member, nProTxHash=%s\n", __func__, nProTxHash.ToString()); return false; } @@ -447,6 +448,7 @@ bool CDKGSessionManager::GetEncryptedContributions(Consensus::LLMQType llmqType, if (validMembers[i]) { CBLSIESMultiRecipientObjects encryptedContributions; if (!db->Read(std::make_tuple(DB_ENC_CONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), members[i]->proTxHash), encryptedContributions)) { + LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- can't read from db, nProTxHash=%s\n", __func__, nProTxHash.ToString()); return false; } vecRet.emplace_back(encryptedContributions.Get(nRequestedMemberIdx));