refactor: remove llmq::CQuorum{BlockProcessor, Manager} globals, move to LLMQContext #6030

kwvg · 2024-05-19T20:47:36Z

Breaking Changes

None expected.

Checklist

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

knst · 2024-05-20T03:19:24Z

src/init.cpp

                // Have to start it early to let VerifyDB check ChainLock signatures in coinbase
                node.llmq_ctx->Start();

+                node.mnhf_manager.reset();


why that is done? mnhf_manager is needed to determine if soft-forks are started and this manager if it is not used right now widely, but it means to be initialized close to CChainState soon as possible, because everything that uses chain may also be interested by status of soft-fork by BIP9. Everything else, such as QuorumManager, Metaman, mn_sync, peerman, quorum-instant-send-lock....

Mnhf_manager should be initilized early as possible to be sure when CChainState (and its helpers) are initialized you already have a knowledge about soft-forks

src/evo/mnhftx.cpp:94: const auto quorum = llmq::quorumManager->GetQuorum(llmqType, quorumHash);

due to that. Hm, is there any way to resolve this deglob without reversing initialization order?

I initially took the approach of having {Process, Undo}Block accept llmq::CQuorumManager, which seemed sensible since it would be called through CSpecialTxProcessor, which has access to llmq::CQuorumManager. Unfortunately, ProcessBlock is also used by GetForBlock, which is used by GetSignalsStage and AddSignal, the former being used in VersionBitsConditionChecker.

Since threading that needle would be unpleasant, the less unpleasant means would be to give CMNHFManager knowledge of llmq::CQuorumManager. That's why I took the approach that I did.

Alternatively, we can do what we did for CTxMemPool and leave certain functionality unavailable unless you call ConnectManagers, which then lets you use the desired functionality. Since that was quite easy to do for CTxMemPool (most of the ProTx validation code was quite easy to segment), it was done there, I wasn't sure about that here.

If CMNHFManager could be given a read-only mode (which only needs CEvoDB) and full mode (needing llmq::CQuorumManager), this approach would be plausible.

Refactored the code to now have (Dis)connectManagers, llmq::CQuorumManager is used only {Process, Undo}Block, appropriate comments and asserts have been added.

knst

overall LGTM but maybe I'd not touch CMNHFManager initialization before we got mn_rr on mainnet (so far as it is well tested on testnet in the current version)

knst · 2024-05-21T13:49:04Z

src/llmq/context.h

    const std::shared_ptr<CBLSWorker> bls_worker;
    const std::unique_ptr<llmq::CDKGDebugManager> dkg_debugman;
-    llmq::CQuorumBlockProcessor* const quorum_block_processor;
+    llmq::CQuorumBlockProcessor* const qblockman;


the name of class is CQuorumBlockProcessor, why the name is qblockMAN ?

it makes a confusion with CQuorumManager, I'd avoid renaming it without a good cause

I went with qblockman because quorum_block_processor is a long name and because it's the name already used in CSpecialTxProcessor (source).

The ending with -man does conflict with the name but it's also the only -Processor class being passed around in different parts of the codebase like it's a distinct entity and it seems to behave like one, having its own internal state (unlike CSpecialTxProcessor or CMNPaymentsProcessor, which only hold references but no state for itself).

qman and qblockman are distinct enough IMO to avoid confusion and in all likelihood, any confusion make itself apparent as they don't implement an interface that can be confused for each other.

kwvg · 2024-05-21T14:40:33Z

Placing PR as draft to address CMNHFManager initialization concerns

knst · 2024-05-23T12:14:03Z

Placing PR as draft to address CMNHFManager initialization concerns

@kwvg let's get merged these 4 commits as a spare PR?

To avoid further conflicts and reduce rebases.

…tions

kwvg · 2024-05-26T15:44:28Z

To avoid further conflicts and reduce rebases.

llmq::CQuorumManager requires a smidge change in the ConnectManagers call to work properly (using get()), preventing a clean separation. Apart, the changes are too trivial to justify a PR. Have pushed updates and am continuing in this PR.

UdjinM6

utACK

knst

LGTM f46df86

PastaPastaPasta

I don't think I support the renaming qman is too vague IMO, and there's no real value in changing it from quorum_manager quorum_man would be fine imo; but I don't like qman. Same with qblockman I don't like dropping quorum. Otherwise fine

PastaPastaPasta · 2024-05-28T13:39:57Z

src/evo/mnhftx.cpp

    AddToCache(signals, pindex);
 }

+void CMNHFManager::ConnectManagers(llmq::CQuorumManager* qman)


make this gsl::not_null

Resolved in latest push

PastaPastaPasta · 2024-05-28T13:44:19Z

src/init.cpp

+        if (node.mnhf_manager) {
+            node.mnhf_manager->DisconnectManagers();
        }
+        node.llmq_ctx.reset();


why no longer checkin for llmq_ctx?

Because reset() is a no-op on containers that have already been reset, as is the case with the containers being reset() above and below.

The conditional for node.mnhf_manager is because we're interacting with the underlying type while reset() interacts with the container type.

PastaPastaPasta · 2024-05-28T13:52:06Z

Placing PR as draft to address CMNHFManager initialization concerns

@kwvg let's get merged these 4 commits as a spare PR?

refactor: s/quorum_block_processor/qblockman/g
544484d

refactor: remove llmq::CQuorumBlockProcessor global, move to `LLMQC…
48545cf

refactor: rename leftover quorumBlockProcessor-named args and variables
3ad6957

refactor: s/quorum_manager/qman/g
ff10e2c

To avoid further conflicts and reduce rebases.

I think I support this

…ontext` We used to store an alias to the `CQuorumBlockProcessor` instance present in the global state in `LLMQContext` but handled construction and destruction of the instance in `LLMQContext`. As direct global usage no longer exists, we can fully subsume it into `LLMQContext`.

kwvg · 2024-05-29T19:04:09Z

Renaming has been removed from this PR

knst

utACK d7c35d0

knst · 2024-05-30T06:33:01Z

src/txmempool.h

 #include <spentindex.h>
 #include <amount.h>
 #include <coins.h>
+#include <gsl/pointers.h>


nit:

~~gsl/pointers.h is a heavy header due to algorithm and txmempool.h is used in hundreds of other compilation units. It affects compilation time.~~

see #6039 which reduces gsl's impact

UdjinM6

utACK

knst

re-utACK d7c35d0

PastaPastaPasta

utACK d7c35d0

…er pointers, check before `queueman` access, update P2P message map in tests 24f9357 fix: update P2P message map in functional test framework (Kittywhiskers Van Gogh) a432a95 fix: check if `queueman` is initialized before accessing it (Kittywhiskers Van Gogh) 0444e59 trivial: use `std::atomic` to protect connected manager pointers (Kittywhiskers Van Gogh) b7700b3 trivial: use `std::atomic` to protect connected signer pointers (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6838 This pull request contains the following: * Minor follow-up to [dash#6828](#6828) based on feedback received during review also extended to similar connections introduced in [dash#5980](#5980) ([commit](a5be37c#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522R350-R355)) and [dash#6030](#6030) ([commit](805537e#diff-59f8e9f1b35c1428332caab753a818e3b2146e73bb6c998a2aed5f7eddbc6fa1R357-R363)) * A bugfix to avoid potential crash caused by missing `nullptr` check after `CCoinJoinClientQueueManager` was conditionally initialized in [dash#5163](#5163) ([commit](2d2814e#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR2308-R2310)) * Updating the Python mapping of Dash-specific P2P messages to avoid unexpected test failures ([build](https://github.com/dashpay/dash/actions/runs/17707917238/job/50323243018#step:6:328)), observed when working on [dash#6838](#6838) ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 24f9357 Tree-SHA512: 90b0b2536a7704e3f3da4ece05b6ad09c393a4348aaff87682b7547f6a7d6ffede504176fa1f63bd9ad88961fb1e3b113aae5df357c0dfb70df2fc55500c2b5f

knst reviewed May 20, 2024

View reviewed changes

kwvg marked this pull request as ready for review May 20, 2024 13:01

kwvg added this to the 21 milestone May 21, 2024

knst reviewed May 21, 2024

View reviewed changes

kwvg marked this pull request as draft May 21, 2024 14:40

refactor: access llmq::CQuorumManager through arg in MNHFTx, func…

cbb6828

…tions

kwvg force-pushed the llmq_ctx_csl branch 2 times, most recently from ec4a9d4 to f46df86 Compare May 26, 2024 15:40

kwvg marked this pull request as ready for review May 26, 2024 16:36

kwvg requested review from knst, UdjinM6 and PastaPastaPasta May 26, 2024 18:30

UdjinM6 previously approved these changes May 27, 2024

View reviewed changes

knst previously approved these changes May 27, 2024

View reviewed changes

PastaPastaPasta requested changes May 28, 2024

View reviewed changes

kwvg dismissed stale reviews from knst and UdjinM6 via 1964073 May 28, 2024 14:16

kwvg force-pushed the llmq_ctx_csl branch 2 times, most recently from 1964073 to 62ee3d4 Compare May 28, 2024 14:34

kwvg added 5 commits May 28, 2024 15:20

evo: add CMNHFManager::(Dis)connectManagers to remove global use

805537e

evo: use gsl::not_null in CTxMemPool::ConnectManagers

1d3afe7

refactor: reduce llmq::CQuorumManager globals use, use args

5b86df6

refactor: remove llmq::CQuorumManager global, move to LLMQContext

d7c35d0

kwvg force-pushed the llmq_ctx_csl branch from 62ee3d4 to d7c35d0 Compare May 29, 2024 17:00

kwvg requested review from knst, UdjinM6 and PastaPastaPasta May 29, 2024 19:04

knst approved these changes May 30, 2024

View reviewed changes

UdjinM6 approved these changes May 31, 2024

View reviewed changes

knst approved these changes Jun 2, 2024

View reviewed changes

PastaPastaPasta approved these changes Jun 3, 2024

View reviewed changes

PastaPastaPasta merged commit 44f237a into dashpay:develop Jun 4, 2024

kwvg mentioned this pull request Sep 9, 2025

fix: use std::atomic to protect connected manager/signer pointers, check before queueman access, update P2P message map in tests #6842

Merged

5 tasks

refactor: remove llmq::CQuorum{BlockProcessor, Manager} globals, move to LLMQContext #6030

refactor: remove llmq::CQuorum{BlockProcessor, Manager} globals, move to LLMQContext #6030

Uh oh!

Conversation

kwvg commented May 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwvg May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwvg commented May 21, 2024

Uh oh!

knst commented May 23, 2024

Uh oh!

kwvg commented May 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented May 28, 2024

Uh oh!

kwvg commented May 29, 2024

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwvg commented May 19, 2024 •

edited

Loading

knst May 20, 2024 •

edited

Loading

kwvg May 20, 2024 •

edited

Loading

kwvg commented May 26, 2024 •

edited

Loading