refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #5980

kwvg · 2024-04-09T15:57:36Z

Additional Information

Depends on refactor: rearrange database initialization, trim globals use in net and net processing, move CheckSpecialTx into CSpecialTxProcessor #5966
CTxMemPool's ProTx logic took the presence of CDeterministicMNManager for granted. To account for CTxMemPool instances that aren't seeded with the pointer to the manager (usually because the unit test doesn't call for them), refactoring was done to make ProTx code paths conditional on the manager pointer being set to something.
- Setting the manager pointer is done through CTxMemPool::Init(), to avoid having to pass another std::unique_ptr const-ref and also because CTxMemPool can technically work without the manager if ProTx logic is not needed (and CTxMemPool::existsProviderTxConflict() isn't called)
  - CTxMemPool::Init() doesn't allow overwriting a not-nullptr manager pointer. If that is desired, then CTxMemPool::Reset() should be called first. This was done to avoid unintentional double-initialization/overwriting.

Breaking Changes

None. Changes are limited to refactoring.

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 (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

knst

LGTM f967c16

(CI is not finished yet)

knst · 2024-04-09T16:08:08Z

src/governance/governance.cpp

-        LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- Checking object for deletion: %s, deletion time = %d, time since deletion = %d, delete flag = %d, expired flag = %d\n",
-            strHash, pObj->GetDeletionTime(), nTimeSinceDeletion, pObj->IsSetCachedDelete(), pObj->IsSetExpired());
+        LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- Checking object for deletion: %s, deletion time = %d, time since deletion = %d, delete flag = %d, expired flag = %d\n",
+            __func__, strHash, pObj->GetDeletionTime(), nTimeSinceDeletion, pObj->IsSetCachedDelete(), pObj->IsSetExpired());


nit: After dca7ae2 get merged no need more __func__ here.

So far as function is not actually renamed and it still has a correct name, consider removing this refactoring from PR.

These changes are introduced in #5966 and have been remedied over there in the latest push.

src/txmempool.cpp

knst · 2024-04-09T16:17:48Z

src/llmq/chainlocks.cpp

+CChainLocksHandler::CChainLocksHandler(CChainState& chainstate, CConnman& _connman, CQuorumManager& _qman,
                                       CSigningManager& _sigman, CSigSharesManager& _shareman, CSporkManager& sporkman,
-                                       CTxMemPool& _mempool) :
+                                       CTxMemPool& _mempool, const CMasternodeSync& mn_sync) :


knst · 2024-04-09T16:47:22Z

src/llmq/utils.h


+using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;


nit: that's still forward declaration, not sure that empty line here is reasonable but nevermind

Suggested change

using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;

using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;

kwvg · 2024-04-09T20:55:02Z

Marked as draft to avoid confusion with dependency PR

…s use in net and net processing, move CheckSpecialTx into CSpecialTxProcessor 191b3de refactor: move CheckSpecialTx into CSpecialTxProcessor (Kittywhiskers Van Gogh) 91f4588 refactor: const the pointer of type `const CActiveMasternodeManager` (Kittywhiskers Van Gogh) 1d9b7fa refactor: trim globals use in net processing functions (Kittywhiskers Van Gogh) 2a4fdbf refactor: trim globals use in net threads and functions (Kittywhiskers Van Gogh) ff825ac init: load databases of governance dependencies before loading its own (Kittywhiskers Van Gogh) cf940e8 init: decouple CMasternodeMetaMan construction from initialization (Kittywhiskers Van Gogh) fae5696 init: move CActiveMasternodeManager construction before PeerManager (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #5980 * `CActiveMasternodeManager` construction is moved upwards (_before_ `PeerManager` construction) to avoid having to pass it as a `std::unique_ptr` const-ref. Shouldn't have any effect since _initialization_ is done in `ThreadImport`. * `CConnman::SetNetworkActive` will require a pointer to `CMasternodeSync` in order to call `CMasternodeSync::Reset()`. As it is no longer available as a global, it will need to be manually called through for the effect to happen as the `CConnman` constructor will simply pass a `nullptr` when calling `SetNetworkActive`, bypassing the `Reset()` call. `CMasternodeSync` is appropriately passed through in RPC (`setnetworkactive`) and interfaces. * `CheckSpecialTx` was moved into `CSpecialTxProcessor` to avoid having to expose `CDeterministicMNManager` to `MemPoolAccept` (though it has been exposed to `CChainstateHelper` through a helper, `CChainState::ChainHelper()` that was introduced in this PR). * As a drawback, some RPC functions that otherwise only needed access to `CDeterministicMNManager`, will also be accessing `CChainstateHelper`. ## Concerns * ~~Some tests seem to sporadically fail (fail as part of a suite but succeed when run individually, no changes in this PR should worsen resource contention) but attempting to reproduce them reliably hasn't succeeded so far.~~ It appears the sporadic failure is shown in `p2p_node_network_limited.py` during TSan runs (see [failed run](https://gitlab.com/dashpay/dash/-/jobs/6529052189) for commit f95ffa7 that then succeeded in a [second attempt](https://gitlab.com/dashpay/dash/-/jobs/6530402585) versus a similar [failed run](https://gitlab.com/dashpay/dash/-/jobs/6546952668) for e210cb7 that also succeeded on [second try](https://gitlab.com/dashpay/dash/-/jobs/6549470339)). Similar behaviour has not been observed on `develop` (as of this writing is 27c0813). ## Breaking changes `CMasternodeSync::Reset()` will not be called on every `CConnman` entity instantiated. Behaviour changes as a result of that are not substantiated. Protocol, networking or interface changes are not expected, changes are primarily refactoring. ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 191b3de Tree-SHA512: 73a79818b0a79e3f26f079019c078ec8f81b2dae17638f695243b87fa76e9bed906a33ad7dd4e600f699100c5e994628cbb596e78a7802fa630c91f4d69cce4c

The merge-base changed after approval.

github-actions · 2024-04-12T15:17:31Z

This pull request has conflicts, please rebase.

…Manager presence Despite removeUnchecked not explicitly requiring CDeterministicMNManager, it has also been made conditional due to addUnchecked requiring the manager and allowing for some operations but not others when pertaining to a transaction type could allow inconsistencies to arise. Better to treat as one unit and skip both if manager isn't present.

PastaPastaPasta

utACK a5be37c

knst

LGTM a5be37c

UdjinM6

utACK

…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

kwvg mentioned this pull request Apr 9, 2024

refactor: rearrange database initialization, trim globals use in net and net processing, move CheckSpecialTx into CSpecialTxProcessor #5966

Merged

5 tasks

knst previously approved these changes Apr 9, 2024

View reviewed changes

kwvg marked this pull request as draft April 9, 2024 20:42

kwvg added this to the 21 milestone Apr 10, 2024

github-actions bot added the needs rebase label Apr 12, 2024

kwvg added 4 commits April 12, 2024 17:01

refactor: remove CMasternodeSync global, move to NodeContext

c99fb42

refactor: remove CActiveMasternodeManager global, move to NodeContext

81b1247

refactor: remove CMasternodeMetaMan global, move to NodeContext

cf90cf2

kwvg force-pushed the mn_deglob_final branch from f967c16 to 52ba288 Compare April 12, 2024 18:22

github-actions bot removed the needs rebase label Apr 12, 2024

kwvg marked this pull request as ready for review April 12, 2024 18:23

kwvg requested review from PastaPastaPasta, UdjinM6 and knst April 12, 2024 18:55

refactor: remove CDeterministicMNManager global, move to NodeContext

a5be37c

PastaPastaPasta force-pushed the mn_deglob_final branch from 52ba288 to a5be37c Compare April 16, 2024 17:55

PastaPastaPasta approved these changes Apr 16, 2024

View reviewed changes

knst approved these changes Apr 16, 2024

View reviewed changes

UdjinM6 approved these changes Apr 16, 2024

View reviewed changes

PastaPastaPasta merged commit 2dacfb0 into dashpay:develop Apr 17, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #5980

refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #5980

Uh oh!

kwvg commented Apr 9, 2024 •

edited

Loading

Uh oh!

knst left a comment

Uh oh!

knst Apr 9, 2024 •

edited

Loading

Uh oh!

kwvg Apr 9, 2024 •

edited

Loading

Uh oh!

Uh oh!

knst Apr 9, 2024

Uh oh!

knst Apr 9, 2024

Uh oh!

kwvg commented Apr 9, 2024

Uh oh!

github-actions bot commented Apr 12, 2024

Uh oh!

PastaPastaPasta left a comment

Uh oh!

knst left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		using CDeterministicMNCPtr = std::shared_ptr<const CDeterministicMN>;

refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #5980

refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #5980

Uh oh!

Conversation

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

Uh oh!

Additional Information

Breaking Changes

Checklist:

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Apr 9, 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 Apr 9, 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!

Uh oh!

knst Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

knst Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

kwvg commented Apr 9, 2024

Uh oh!

github-actions bot commented Apr 12, 2024

Uh oh!

PastaPastaPasta 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!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwvg commented Apr 9, 2024 •

edited

Loading

knst Apr 9, 2024 •

edited

Loading

kwvg Apr 9, 2024 •

edited

Loading