Skip to content

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Aug 31, 2023

Motivation

As highlighted in https://github.com/dashpay/dash-issues/issues/52, decoupling of CFlatDB-interacting components from managers of objects like CGovernanceManager and CSporkManager is a key task for achieving deglobalization of Dash-specific components.

The design of CFlatDB as a flat database agent relies on hooking into the object's state its meant to load and store, using its (de)serialization routines and other miscellaneous functions (notably, without defining an interface) to achieve those ends. This approach was taken predominantly for components that want a single-file cache.

Because of the method it uses to hook into the object (templates and the use of temporary objects), it explicitly prevented passing arguments into the object constructor, an explicit requirement for storing references to other components during construction. This, in turn, created an explicit dependency on those same components being available in the global context, which would block the backport of bitcoin#21866, a requirement for future backports meant to achieve parity in assumeutxo support.

The design of these objects made no separation between persistent (i.e. cached) and ephemeral (i.e. generated/fetched during initialization or state transitions) data and the design of CFlatDB attempts to "clean" the database by breaching this separation and attempting to access this ephemeral data.

This might be acceptable if it is contained within the manager itself, like CSporkManager's CheckAndRemove() but is utterly unacceptable when it relies on other managers (that, as a reminder, are only accessible through the global state because of restrictions caused by existing design), like CGovernanceManager's UpdateCachesAndClean().

This pull request aims to separate the CFlatDB-interacting portions of these managers into a struct, with CFlatDB interacting only with this struct, while the manager inherits the struct and manages load/store/update of the database through the CFlatDB instance initialized within its scope, though the instance only has knowledge of what is exposed through the limited parent struct.

Additional information

  • As regards to existing behaviour, CFlatDB is written entirely as a header as it relies on templates to specialize itself for the object it hooks into. Attempting to split the logic and function definitions into separate files will require you to explicitly define template specializations, which is tedious.

  • m_db is defined as a pointer as you cannot instantiate a forward-declared template (see this Stack Overflow answer for more information), which is done when defined as a member in the object scope.

  • The conditional cache flush predicating on RPC not being in the warm-up state has been replaced with unconditional flushing of the database on object destruction (@UdjinM6, is this acceptable?)

TODOs

This is a list of things that aren't within the scope of this pull request but should be addressed in subsequent pull requests

  • Definition of an interface that CFlatDB stores are expected to implement
  • Lock annotations for all potential uses of members protected by the cs mutex in each manager object and store
  • Additional comments documenting what each function and member does
  • Deglobalization of affected managers

@kwvg kwvg force-pushed the flatdb branch 3 times, most recently from eb64490 to 57505ab Compare September 4, 2023 20:40
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

This pull request has conflicts, please rebase.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

This pull request has conflicts, please rebase.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, however let's keep stuff as classes, IMO structs should only be used for very simple data structures, normally with no member functions and minimal data structures

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check my comments, especially about using mmetaman in net.cpp and net_processing.cpp

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for squash merge

@PastaPastaPasta PastaPastaPasta merged commit ee31352 into dashpay:develop Sep 24, 2023
@UdjinM6 UdjinM6 added this to the 20 milestone Oct 5, 2023
PastaPastaPasta added a commit that referenced this pull request Apr 26, 2024
…overnance global

b4477e4 trivial: don't print `fDisableGovernance` value anymore (Kittywhiskers Van Gogh)
a42370d refactor: remove fDisableGovernance global, define default in variable (Kittywhiskers Van Gogh)
b152759 refactor: remove fMasternodeMode and fDisableGovernance from Qt code (Kittywhiskers Van Gogh)
9402ce7 refactor: limit usage of fDisableGovernance, use `IsValid()` instead (Kittywhiskers Van Gogh)
106f6bd refactor: reduce fMasternodeMode usage in governance and mnauth (Kittywhiskers Van Gogh)
3ba293f refactor: remove fMasternodeMode checks in CActiveMasternodeManager (Kittywhiskers Van Gogh)
b0216ac refactor: remove fMasternodeMode usage in rpc logic (Kittywhiskers Van Gogh)
4d629a0 refactor: limit fMasternodeMode usage in blockstorage, init, net_processing (Kittywhiskers Van Gogh)
a9cbdfc refactor: remove fMasternodeMode usage from llmq logic (Kittywhiskers Van Gogh)
c62a3d5 refactor: remove fMasternodeMode usage from coinjoin logic (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  Since #5940, `CActiveMasternodeManager` ceased to be a global variable and became a conditional smart pointer initialized based on the value of `fMasternodeMode`.

  Likewise, since #5555, we can tell if any `CFlatDB`-based manager has successfully loaded its database. `CGovernanceManager` is one of them and conditionally loads its database based on the value of `fGovernanceDisabled`.

  `fMasternodeMode` and `fGovernanceDisabled` were (and the former to a certain degree still is) unavoidable globals due to the way the functionality they influenced was structured (i.e. decided in initialization code with no way to query from the manager itself). As we can directly ask the managers now, we can start reducing the usage of these globals and at least in this PR, get rid of one of them.

  This PR was the idea of PastaPastaPasta, special thanks for the suggestion!

  ## Additional Information

  * There are two conventions being used for checking `nullptr`-ity of a pointer, `if (mn_activeman)` and `if (mn_activeman != nullptr)`. The former is used in initialization and RPC code due to existing conventions there ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/init.cpp#L1659-L1677), [source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/rpc/net.cpp#L942-L945), [source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/rpc/misc.cpp#L215-L218)). The latter is used whenever the value has to be passed as a `bool` (you cannot pass the implicit conversion to a `bool` argument without explicitly casting it) and in Dash-specific code where it is the prevalent convention ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/governance/governance.cpp#L125), [source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/coinjoin/client.cpp#L1064)).

    Unfortunately, that means this PR expresses the same thing sometimes in two different ways but this approach was taken so that reading is consistent within the same file. Codebase-wide harmonization is outside the scope of this PR.

  * Where `mn_activeman` isn't directly available, the result of the check is passed as an argument named `is_masternode` and/or set for the manager during its construction as `m_is_masternode` (`const bool`) as it is expected for the `CActiveMasternodeManager`'s presence or absence to remain as-is for the duration of the manager's lifetime.

    This does mean that some parts of the codebase check for `mn_activeman` while others check for {`m_`}`is_masternode`, which does reduce clarity while reading. Suggestions on improving this are welcomed.

  * One of the reasons this PR was made was to avoid having to deal the _possibility_ of `fMasternodeMode` or `fDisableGovernance` from desynchronizing from the behaviour of the managers it's suppose to influence. It's why additional assertions were placed in to make sure that `fMasternodeMode` and the existence of `mn_activeman` were always in sync ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/evo/mnauth.cpp#L137-L139), [source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/rpc/governance.cpp#L319-L320)).

    But removing the tracking global and relying on a manager's state itself prevents a potential desync, which is what this PR is aiming to do.

  ## 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: 7861afd17c83b92af4c95b2841e9b0f676116eb3f234c4d0b1dcd3c20395452893e8ca3a17c7225389c8411dac80aeb5050f06a2ae35df5ec48998a571ef120c
PastaPastaPasta added a commit that referenced this pull request Oct 29, 2025
…` and `GovernanceStore`, move away from `RecursiveMutex`es

f88f15f fix: `CGovernanceManager::Clear()` should clear all fields (Kittywhiskers Van Gogh)
dc9787f fix: use `std::shared_ptr` to manage `CGovernanceObject` lifetime (Kittywhiskers Van Gogh)
828cad5 trivial: use structured bindings where possible, limit iterator scope (Kittywhiskers Van Gogh)
0bc28c3 fix: resolve potential deadlock (Kittywhiskers Van Gogh)
035dfee fix: resolve lock order issues (Kittywhiskers Van Gogh)
14c44cb refactor: switch `CGovernanceObject::cs` to `Mutex` (Kittywhiskers Van Gogh)
009f5cb refactor: add `cs` annotations for `CGovernanceObject` (Kittywhiskers Van Gogh)
2ee408f refactor: protect some `CGovernanceObject` variables (Kittywhiskers Van Gogh)
2378a99 trivial: tidy-up `governance/object.h` (Kittywhiskers Van Gogh)
f077ca3 refactor: move from `RecursiveMutex` to `Mutex`, rename to `cs_store` (Kittywhiskers Van Gogh)
7b9997c refactor: add `cs` annotations for public functions (Kittywhiskers Van Gogh)
e8a1e8b refactor: add `cs` annotations for private functions (Kittywhiskers Van Gogh)
9f5cf4a refactor: privatize internally used functions (Kittywhiskers Van Gogh)
1731272 refactor: protect `GovernanceStore::cs`, add accessor to remove ext. use (Kittywhiskers Van Gogh)
423c1eb refactor: add `cs` annotations for accessor functions (Kittywhiskers Van Gogh)
4080ee9 refactor: group together public accessor functions (Kittywhiskers Van Gogh)
249d021 refactor: guard all `GovernanceStore` vars, move `cmapVoteToObject` (Kittywhiskers Van Gogh)
9d647e8 refactor: remove unused code (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6838

  * The `RecursiveMutex` in `CGovernanceObject` was first used in b02d526 ([source](https://github.com/dashpay/dash/blob/b02d5260fdc03d560ed9ab3162150bc0b0ae8fe7/src/masternode-budget.h#L115-L116)) but as the structure evolved from `CBudgetProposal` to its current form, at no point has the mutex guarded any members (using the `GUARDED_BY` macro).

    This has been resolved by guarding variables that are (de)serialised and updating annotations to reflect this change. As the usage of `CGovernanceObject::cs` does not warrant recursive locking, it has been updated to be a simpler `Mutex` and has been made private.

  * `cmapVoteToObject` was incorrectly moved to `GovernanceStore` in [dash#5555](#5555) despite it being memory-only. This has been resolved and it has been moved to `CGovernanceManager`, where memory-only variables reside.
    * The fields in `GovernanceStorage` are now protected by the mutex `cs_store` (formerly `cs`) and annotations in `CGovernanceManager` has been updated to reflect this explicitly. This has _not_ been extended to memory-only variables.

  * With the compile-time annotations, runtime assertions and internal functions that assume the lock is already held, we can reasonably detect double-locking scenarios and can safely move the `GovernanceStore::cs_store` `RecursiveMutex` to a `Mutex`

    * The loss of recursive locking resulted in detection of inconsistent lock ordering (see below), this has been resolved in a separate commit.

      <details>

      <summary>Error:</summary>

      ```
      Previous lock order was:
      'm_chainstate_mutex' in validation.cpp:3394 (in thread 'httpworker.2')
      (2) 'cs_main' in validation.cpp:3411 (in thread 'httpworker.2')
      'MempoolMutex()' in validation.cpp:3413 (in thread 'httpworker.2')
      'pqueue->m_control_mutex' in ./checkqueue.h:226 (in thread 'httpworker.2')
      (2) '::cs_main' in governance/governance.cpp:478 (in thread 'httpworker.2')
      (1) 'cs_store' in governance/governance.cpp:478 (in thread 'httpworker.2')
      Current lock order is:
      (1) 'cs_store' in governance/governance.cpp:1384 (in thread 'scheduler')
      (2) '::cs_main' in governance/governance.cpp:998 (in thread 'scheduler')
      ```

      </details>

  ## 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 **(note: N/A)**
  - [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 [f88f15f](f88f15f)
  UdjinM6:
    utACK f88f15f

Tree-SHA512: 2426b2b2d42781f17fa7a4fb57741a67fdbcae2b3efac58315963949282c99304eab89b54c52872ec472f85378c6e350cd067f5071f5c71e34eb084c972cb748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants