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

kwvg · 2024-04-02T13:38:49Z

Additional Information

Dependency for refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext #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 for commit f95ffa7 that then succeeded in a second attempt versus a similar failed run for e210cb7 that also succeeded on second try). 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.

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)

src/txmempool.h

knst · 2024-04-03T11:22:29Z

src/coinjoin/client.cpp

 PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataStream& vRecv)
 {
-    assert(::mmetaman->IsValid());
+    assert(m_mn_metaman.IsValid());


600 lines up and down is not an easy thing to review :(

That one "refactor: remove CMasternodeMetaMan global, move to NodeContext" alone is worth a PR for itself only

src/txmempool.h

github-actions · 2024-04-03T15:38:28Z

This pull request has conflicts, please rebase.

UdjinM6 · 2024-04-05T11:11:14Z

It appears the sporadic failure is shown in p2p_node_network_limited.py during TSan runs (see failed run for commit f95ffa7 that then succeeded in a second attempt versus a similar failed run for e210cb7 that also succeeded on second try). Similar behaviour has not been observed on develop (as of this writing is 27c0813).

Same failure on develop https://gitlab.com/dashpay/dash/-/jobs/6483372677

knst

these commits are ACK:

To other commits I have a comments or haven't carefully reviewed yet

knst · 2024-04-06T11:45:12Z

src/evo/mnauth.cpp


 PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, const CDeterministicMNList& tip_mn_list, std::string_view msg_type, CDataStream& vRecv)
 {
+    assert(::mmetaman->IsValid());


what happen if mmetaman is not initialized yet and we will ignore this message? is it fatal?

I feel like this assert is a little bit too much. also if message is not MNAUTH - it's not even need any processing

Database initialization (setting IsValid() to true) is done (source) before networking starts (source). If there is a database problem that didn't get caught earlier on, starting networking threads and read-writing values could result in an inconsistent state. This is a problem regardless of which database and therefore, should fast-fail if it happens.

CMNAuth::ProcessMessage is only called by PeerManagerImpl::ProcessMessages (source), there aren't any other threads or entities that can call it, making a state where peerman exists but mn_metaman doesn't improbable.

let's add doxygen here to require mn_metamann be non nullptr here:

dash/src/evo/mnauth.h

Line 56 in e210cb7

static PeerMsgRet ProcessMessage(CNode& peer, CConnman& connman, CMasternodeMetaMan& mn_metaman, const CActiveMasternodeManager* const mn_activeman,

We're already passing CMasternodeMetaMan as a reference here, there's no opportunity for it to be a nullptr.

sorry, I mean mn_metaman to be valid due to assert(mn_metaman.IsValid());. That can't be nullptr indeed, but it can be invalid

Resolved in latest push (comment added in header)

src/governance/governance.cpp

src/net_processing.cpp

src/governance/governance.cpp

knst · 2024-04-06T15:04:07Z

src/txmempool.cpp

        vTxHashes.clear();

+    if (::deterministicMNManager) {
+        removeUncheckedProTx(it->GetTx());


what if deterministicMNManager will be nullptr when tx is added, but won't be nullptr when tx is removed -- will it work correctly?

That would be highly problematic as that shows us that in the order of (de)initialization has been messed with, which is highly unlikely in init and the constructors in setup_common, meaning, at worst, it's a faulty test where the values of globals are being manually manipulated.

The refactoring of CTxMemPool split up from the rest of CDeterministicMNManager deglobalization was done for ease of review, eventually, we use the pointer set with ConnectManagers instead of using globals directly (source), making any modifications explicit.

src/governance/governance.cpp

knst · 2024-04-06T15:09:37Z

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


why that? add const - ok, why changing order here?

Because we place const args after non-const args, see LLMQContext, CJContext and CDSNotificationInterface for examples.

there's no clear agreement about order of const/non-const argument in methods, but this one is the closest:

For consistency and friendliness to code generation tools, interface method input and inout parameters should be ordered first and output parameters should come last.
https://github.com/dashpay/dash/blob/e210cb734e16e0ecef9601251fe0e074ed86827e/doc/developer-notes.md

I would say that const argument are better to go first rather than being after non-const, thoughts?

If we're talking about arguments that can be clearly understood as inputs/outputs, then I agree.

Though we have this middle ground with references to managers that are read-write (non-const) or read-only (const), some references are used to access functions that seem to do only read-only things even when behind the scenes they are read-write (like CDeterministicMNManager::GetListAtTip(), which is non-const).

Plus, sometimes we use const for unique_ptr refs, where we are only consting the container ref but intend to do read-write activities with the underlying type. It starts becoming less clear.

Maybe it can examined at some point but I think current convention is inoffensive, not particularly misleading.

sometimes we use const for unique_ptr refs

indeed, but all const unique_ptr are just temporary workarounds until circular dependencies are broken

IMO as long as they're there in the reasonable future, they need to be considered.

so, still, is there any reason why you moved const argument from the beginning to the end? If there's anything uses not the best code style that's not a reason to spread it over codebase.

I think the placement here is fine. I think it only needs to be enforced that "out params" are at the end. Ordering of const and non-const in or inout params, is not important.

knst · 2024-04-06T17:50:20Z

src/txmempool.cpp

    // fully checked by AcceptToMemoryPool() at this point, so we just assume that
    // everything is fine here.
-    if (::deterministicMNManager) {
+    if (m_dmnman) {


should we assert here then?
see #5966 (comment)

I think we should not let transactions to be added or removed from mempool if that's not fully initialized yet.

Not necessary, the value of m_dmnman is assert'ed before being set (source). If in the middle of operation, a manager is gone, there are bigger problems.

CTxMemPool can actually operate without m_dmnman so long as you don't require ProTx logic (and this is the case with some unit tests), so we don't need to make it a compulsion.

kwvg · 2024-04-09T15:58:37Z

PR has been split between refactors in preparation for deglobalization and deglobalization, the latter is now available at #5980

The order in which they're loaded is dictated by the order in which they are initialized. This also lets us get rid of that fast-fail because governance db was always going to load before mncache db and replace it with an assert. Also, rename UpdateCachesAndClean to CheckAndRemove in CGovernanceManager as CheckAndRemove is already an existing alias to UpdateCachesAndClean and CheckAndRemove is associated with similar functionality in C{NetFulfilledRequest, Spork}Manager

knst

utACK

UdjinM6

utACK

PastaPastaPasta

utACK 191b3de

…er, CMasternode{MetaMan, Sync} to NodeContext a5be37c refactor: remove CDeterministicMNManager global, move to NodeContext (Kittywhiskers Van Gogh) cf90cf2 refactor: remove CMasternodeMetaMan global, move to NodeContext (Kittywhiskers Van Gogh) 81b1247 refactor: remove CActiveMasternodeManager global, move to NodeContext (Kittywhiskers Van Gogh) c99fb42 refactor: remove CMasternodeSync global, move to NodeContext (Kittywhiskers Van Gogh) a247a63 refactor: make CTxMemPool ProTx paths conditional on CDeterministicMNManager presence (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #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: - [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 a5be37c Tree-SHA512: 8dc7ada6597a265b7753603bca6a43b69cfe3b0d330bae98c3e27b6aa24cd3cdff80f6939bb39ffc00902b76b6b145667f0b7ac98a17fe8255d6fd143088a98c

kwvg added this to the 21 milestone Apr 2, 2024

kwvg force-pushed the mn_deglob branch from 254c800 to f95ffa7 Compare April 2, 2024 14:06

kwvg marked this pull request as ready for review April 2, 2024 17:39

kwvg requested review from PastaPastaPasta, UdjinM6 and knst April 2, 2024 17:39

knst reviewed Apr 3, 2024

View reviewed changes

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

kwvg force-pushed the mn_deglob branch from f95ffa7 to fe4a824 Compare April 4, 2024 10:34

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

kwvg force-pushed the mn_deglob branch from fe4a824 to e210cb7 Compare April 4, 2024 10:58

kwvg requested a review from knst April 4, 2024 17:45

knst reviewed Apr 6, 2024

View reviewed changes

init: move CActiveMasternodeManager construction before PeerManager

fae5696

kwvg force-pushed the mn_deglob branch from e210cb7 to 8edc958 Compare April 9, 2024 15:49

kwvg changed the title ~~refactor: move C{ActiveMasternode, DeterministicMN}Manager, CMasternode{MetaMan, Sync} to NodeContext~~ refactor: rearrange database initialization, trim globals use in net and net processing, move CheckSpecialTx into CSpecialTxProcessor Apr 9, 2024

kwvg mentioned this pull request Apr 9, 2024

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

Merged

5 tasks

kwvg added 6 commits April 9, 2024 20:45

init: decouple CMasternodeMetaMan construction from initialization

cf940e8

refactor: trim globals use in net threads and functions

2a4fdbf

refactor: trim globals use in net processing functions

1d9b7fa

refactor: const the pointer of type const CActiveMasternodeManager

91f4588

refactor: move CheckSpecialTx into CSpecialTxProcessor

191b3de

kwvg force-pushed the mn_deglob branch from 8edc958 to 191b3de Compare April 9, 2024 20:54

kwvg requested a review from knst April 9, 2024 20:55

knst approved these changes Apr 10, 2024

View reviewed changes

UdjinM6 approved these changes Apr 10, 2024

View reviewed changes

PastaPastaPasta approved these changes Apr 12, 2024

View reviewed changes

PastaPastaPasta merged commit 5fdaa45 into dashpay:develop Apr 12, 2024

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

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

Uh oh!

Conversation

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

Uh oh!

Additional Information

Concerns

Breaking changes

Checklist:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 3, 2024

Uh oh!

UdjinM6 commented Apr 5, 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!

kwvg Apr 6, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

kwvg commented Apr 9, 2024

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!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

kwvg commented Apr 2, 2024 •

edited

Loading

kwvg Apr 6, 2024 •

edited

Loading