You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merge dashpay#5987: refactor: reduce fMasternodeMode usage, remove fDisableGovernance 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 dashpay#5940, `CActiveMasternodeManager` ceased to be a global variable and became a conditional smart pointer initialized based on the value of `fMasternodeMode`.
Likewise, since dashpay#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
0 commit comments