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 f967c1648a919a28bfb108c84c18cbcad3bea04e

(CI is not finished yet)

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

src/governance/governance.cpp

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

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

src/llmq/utils.h

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 f95ffa7b70d5d20b6321ff11d37ad52425d446d1 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 e210cb734e16e0ecef9601251fe0e074ed86827e 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

…ontext` (1/n, preliminary refactoring) 0114f58 fix: guard `quorumBaseBlockIndexCache` (Kittywhiskers Van Gogh) 2c6cbc9 refactor: use `gsl::not_null` in `CQuorumManager` wherever possible (Kittywhiskers Van Gogh) 8f8078f refactor: initialize `CJWalletManager` after `ActiveContext` (Kittywhiskers Van Gogh) dac4769 refactor: move `CDKGSessionManager` to `{Active,Observer}Context` (Kittywhiskers Van Gogh) 9c61900 refactor: create stub class for watch-only mode context (Kittywhiskers Van Gogh) b9e20f4 refactor: introduce `MakePeerManager()` helper for test setup (Kittywhiskers Van Gogh) 6b811c2 refactor: make `CDKGSessionManager` a plugable obj for `CQuorumManager` (Kittywhiskers Van Gogh) 5d54cfe refactor: couple listener registration with instantiation (Kittywhiskers Van Gogh) 263dd67 refactor: drop remaining `gArgs` usage in LLMQ code (Kittywhiskers Van Gogh) 3f41401 refactor: move `GetEnabledQuorumVvecSyncEntries()` invocation to init (Kittywhiskers Van Gogh) 16459c5 refactor: drop `QuorumDataRecoveryEnabled()` (Kittywhiskers Van Gogh) 7f32d39 refactor: drop `IsWatchQuorumsEnabled()` (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #7062 * As mentioned in [bitcoin#25862](bitcoin#25862), code in `libbitcoin_kernel` should not interact with `ArgsManager` and currently, Dash-specific chainstate validation relies on it to query flags like `-watchquorums` and `-llmq-data-recovery`. They have been refactored to be taken as constructor arguments in the style of [bitcoin#23280](bitcoin#23280) and passed from the context to the constructor of the underlying managers. Similar changes have been extended to remaining `gArgs` usage. * When debugging dependent PRs, it was found that there was a potential race condition as the creation of the signer was done by one entity (`ActiveContext`) but activation was done by the parent entity (e.g. `CChainLocksHandler`). Further complicating the relationship is the introduction of a network entity (e.g. `NetInstantSend`) which could cause potential lifecycle issues. To simpilfy this, the entity that is responsible for creation of the signer is also responsible for registration of the signer. The functions have also been renamed to follow the convention in `CSigSharesManager` to `{Unr,R}egisterAsRecoveredSigsListener()` as it more descriptive as opposed to `Start`/`Stop()`, which is associated with spawning worker entities. * `CDKGSessionManager` is a dormant entity if the node is not in watch-only mode or masternode mode ([source](https://github.com/dashpay/dash/blob/bac6bfadffe6fcf323e5e0208c8619e1e68401fe/src/llmq/dkgsessionmgr.cpp#L46-L49)), to allow moving them to contexts that are mode-specific (in order to keep `LLMQContext` limited to only chainstate validation logic), it is now a connectable entity in the style of [dash#5980](#5980) (a5be37c). * Conversely, we now have ready access to `ChainstateManager` when constructing `CMNHFManager` so we can partially revert the change introduced in [dash#6078](#6078) (208b1c0). * As subsequent PRs will change the ctor arguments for `PeerManager` multiple times, a helper has been added in the form of `MakePeerManager()` to contain the changes to the helper. * Currently, `CJWalletManager` is initialised if `CActiveMasternodeManager` is _not_ initialised. This is fine _until_ we move `CActiveMasternodeManager` into `ActiveContext` as `ActiveContext` is initialised after `CJWalletManager` and cannot be moved upward due to hard dependencies. As there's no reason that `CJWalletManager` needs to be initialised early, we can move it down instead. ## 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 - [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)_ Top commit has no ACKs. Tree-SHA512: d4597f24f918f2b7bc3468111acce004d3df83b2229afd658bb2b4df319369f337cc3a7ae27ed8159ea36265f8f7ad12cba91299d2b156e75b13c62ebdeedc19

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

kwvg mentioned this pull request Dec 16, 2025

refactor: extract CActiveMasternodeManager from LLMQContext (1/n, preliminary refactoring) #7056

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