fix: A set of `qdata`/`qwatch` related fixes #5745

UdjinM6 · 2023-11-29T16:23:09Z

Issue being fixed or feature implemented

Fix/tidy up a few qdata/qwatch related parts, improve performance for regular non-watching nodes

~~based on #5744 atm~~

What was done?

pls see individual commits

How Has This Been Tested?

run tests

Breaking Changes

n/a

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

…sages, only the final one

…oring 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.

…hing nodes

ogabrielides

Looks good in general, some nit and a question for you

src/rpc/quorums.cpp

ogabrielides · 2023-12-16T16:30:29Z

src/rpc/quorums.cpp

@@ -730,6 +730,9 @@ static UniValue quorum_getdata(const JSONRPCRequest& request, const LLMQContext&
        } else {
            throw JSONRPCError(RPC_INVALID_PARAMETER, "proTxHash missing");
        }
+    } else if (!request.params[4].isNull()) {
+        // Require no proTxHash otherwise
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Should not specify proTxHash");


Could you please explain why?

Because in this case we ask for a quorum wide data only (QUORUM_VERIFICATION_VECTOR) and not for a data about one specific MN, so proTxHash is not used in any way in this case and should not be provided. If it still was provided then maybe user doesn't quite understand what he is doing exactly or maybe he made a typo in dataMask.

Got it. Thanks!

I think this would be a breaking change, no?

ogabrielides

utack

PastaPastaPasta · 2023-12-17T18:24:44Z

src/llmq/dkgsessionmgr.cpp

@@ -429,13 +440,15 @@ bool CDKGSessionManager::GetEncryptedContributions(Consensus::LLMQType llmqType,
        }
    }
    if (nRequestedMemberIdx == std::numeric_limits<size_t>::max()) {
+        LogPrintf("CDKGSessionManager::%s -- not a member, nProTxHash=%s\n", __func__, nProTxHash.ToString());


why not guard this behind a category?

PastaPastaPasta · 2023-12-17T18:24:48Z

src/llmq/dkgsessionmgr.cpp

        return false;
    }

    for (const auto i : irange::range(members.size())) {
        if (validMembers[i]) {
            CBLSIESMultiRecipientObjects<CBLSSecretKey> encryptedContributions;
            if (!db->Read(std::make_tuple(DB_ENC_CONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), members[i]->proTxHash), encryptedContributions)) {
+                LogPrintf("CDKGSessionManager::%s -- can't read from db, nProTxHash=%s\n", __func__, nProTxHash.ToString());


PastaPastaPasta · 2023-12-17T18:25:43Z

src/llmq/quorums.h

@@ -275,7 +275,7 @@ class CQuorumManager
    size_t GetQuorumRecoveryStartOffset(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex) const;

    void StartCachePopulatorThread(const CQuorumCPtr pQuorum) const;
-    void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, uint16_t nDataMask) const;
+    void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, const uint16_t nDataMaskIn) const;


const in a declaration doesn't do anything; generally this should only be present in the definition.

Suggested change

void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, const uint16_t nDataMaskIn) const;

void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, uint16_t nDataMaskIn) const;

PastaPastaPasta · 2023-12-17T18:27:24Z

src/rpc/quorums.cpp

@@ -730,6 +730,9 @@ static UniValue quorum_getdata(const JSONRPCRequest& request, const LLMQContext&
        } else {
            throw JSONRPCError(RPC_INVALID_PARAMETER, "proTxHash missing");
        }
+    } else if (!request.params[4].isNull()) {
+        // Require no proTxHash otherwise
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Should not specify proTxHash");


I think this would be a breaking change, no?

UdjinM6 · 2023-12-17T18:53:56Z

dropped c5a66ff and 16f5e00, adjusted d0a4894

PastaPastaPasta

utACK for squash merge; LGTM if you think c5a66ff is valuable, let's open a new PR targeted for v21 with release notes?

Approval contingent on CI

UdjinM6 · 2023-12-17T19:50:02Z

utACK for squash merge; LGTM if you think c5a66ff is valuable, let's open a new PR targeted for v21 with release notes?

#5772

Approval contingent on CI

Gitlab had some unrelated issues https://gitlab.com/dashpay/dash/-/jobs/5770189961, restarted it

knst

Let's refactor IsWatchQuorumsEnabled to validate fMasternodeMode (see a comment for details).

Other comments are non-actionable, just marks for conflicts resolving.

knst · 2023-12-28T13:51:46Z

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);


just a notice: refactored in #5782 - one of these 2 PR should be changed after other is merged

knst · 2023-12-28T13:54:13Z

src/llmq/dkgsessionmgr.cpp

@@ -198,6 +198,12 @@ void CDKGSessionManager::ProcessMessage(CNode& pfrom, const CQuorumManager& quor
        return;
    }

+    if ((!fMasternodeMode && !utils::IsWatchQuorumsEnabled())) {


potentially conflict with #5790 - one of PR to edit; depends which one get merge first.

knst · 2023-12-28T14:06:42Z

src/llmq/quorums.cpp


    for (const auto& params : Params().GetConsensus().llmqs) {
        CheckQuorumConnections(params, pindexNew);
    }

-    {
+    if (fMasternodeMode || utils::IsWatchQuorumsEnabled()) {


I see many check "fMasternodeMode || utils::IsWatchQuorumsEnabled()" - maybe move a boolean flag fMasternodeMode inside IsWatchQuorumsEnabled such as:

bool IsWatchQuorumsEnabled() { static bool fIsWatchQuroumsEnabled = gArgs.GetBoolArg("-watchquorums", DEFAULT_WATCH_QUORUMS); return fIsWatchQuroumsEnabled ||fMasternodeMode ; }

knst

Let's refactor IsWatchQuorumsEnabled to validate fMasternodeMode (see a comment for details).

can be actually done after by new PR; not that important changes. utACK!

PastaPastaPasta

re-approval to trigger conflicts check

UdjinM6 added this to the 20.1 milestone Nov 29, 2023

UdjinM6 mentioned this pull request Nov 29, 2023

fix: CheckQuorumConnections should not be running on regular non-watching nodes #5738

Closed

5 tasks

UdjinM6 added 5 commits December 6, 2023 21:51

feat: regular nodes should never receive qwatch

3bcac49

feat: make sure regular non-watching nodes never process most DKG mes…

96af245

…sages, only the final one

fix: regular non-watching nodes should not do any qdata related job

825fac1

fix: CheckQuorumConnections should not be running on regular non-watc…

92da3bf

…hing nodes

UdjinM6 force-pushed the fix_watch branch from d5479f1 to d0a4894 Compare December 6, 2023 18:51

UdjinM6 marked this pull request as ready for review December 6, 2023 20:42

UdjinM6 requested review from ogabrielides, knst and PastaPastaPasta December 15, 2023 20:08

ogabrielides reviewed Dec 16, 2023

View reviewed changes

ogabrielides previously approved these changes Dec 16, 2023

View reviewed changes

PastaPastaPasta reviewed Dec 17, 2023

View reviewed changes

UdjinM6 dismissed ogabrielides’s stale review via 3302263 December 17, 2023 18:44

UdjinM6 force-pushed the fix_watch branch from 3302263 to 024691e Compare December 17, 2023 18:48

UdjinM6 added 2 commits December 17, 2023 21:52

refactor: s/exists/inserted/ to better match the logic

78e20eb

trivial: add logging for GetEncryptedContributions failures

33b5ca1

UdjinM6 force-pushed the fix_watch branch from 024691e to 33b5ca1 Compare December 17, 2023 18:52

PastaPastaPasta approved these changes Dec 17, 2023

View reviewed changes

knst requested changes Dec 28, 2023

View reviewed changes

knst approved these changes Dec 28, 2023

View reviewed changes

PastaPastaPasta approved these changes Jan 10, 2024

View reviewed changes

PastaPastaPasta merged commit c25d9ae into dashpay:develop Jan 10, 2024
8 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: A set of `qdata`/`qwatch` related fixes #5745

fix: A set of `qdata`/`qwatch` related fixes #5745

UdjinM6 commented Nov 29, 2023 •

edited

Loading

ogabrielides left a comment

ogabrielides Dec 16, 2023

UdjinM6 Dec 16, 2023

ogabrielides Dec 16, 2023

PastaPastaPasta Dec 17, 2023

ogabrielides left a comment

PastaPastaPasta Dec 17, 2023

PastaPastaPasta Dec 17, 2023

PastaPastaPasta Dec 17, 2023

PastaPastaPasta Dec 17, 2023

UdjinM6 commented Dec 17, 2023

PastaPastaPasta left a comment •

edited

Loading

UdjinM6 commented Dec 17, 2023

knst left a comment

knst Dec 28, 2023

knst Dec 28, 2023

knst Dec 28, 2023

knst left a comment

PastaPastaPasta left a comment

	void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, const uint16_t nDataMaskIn) const;
	void StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, const CBlockIndex* pIndex, uint16_t nDataMaskIn) const;

fix: A set of qdata/qwatch related fixes #5745

fix: A set of qdata/qwatch related fixes #5745

Conversation

UdjinM6 commented Nov 29, 2023 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

ogabrielides left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 commented Dec 17, 2023

PastaPastaPasta left a comment • edited Loading

Choose a reason for hiding this comment

UdjinM6 commented Dec 17, 2023

knst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

fix: A set of `qdata`/`qwatch` related fixes #5745

fix: A set of `qdata`/`qwatch` related fixes #5745

UdjinM6 commented Nov 29, 2023 •

edited

Loading

PastaPastaPasta left a comment •

edited

Loading