refactor: reduce fMasternodeMode usage, remove fDisableGovernance global#5987
Merged
PastaPastaPasta merged 10 commits intodashpay:developfrom Apr 26, 2024
Merged
refactor: reduce fMasternodeMode usage, remove fDisableGovernance global#5987PastaPastaPasta merged 10 commits intodashpay:developfrom
PastaPastaPasta merged 10 commits intodashpay:developfrom
Conversation
5 tasks
knst
reviewed
Apr 24, 2024
UdjinM6
reviewed
Apr 24, 2024
|
This pull request has conflicts, please rebase. |
CActiveMasternodeManager no longer exists as a global variable, it is a conditionally initialized smart pointer. If it exists, then it's in masternode mode, no need to check if we're in masternode mode anymore.
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
CGovernanceManager::IsValid() returns true only if its db is successfully initialized. If we attempt to initialize it and fail, init logic will report to us that it failed. If we don't attempt to initialize it at all, it will remain false. Since fDisableGovernance is the same as not initializing it at all and the other case where IsValid() is false is dealt with in init, we can use IsValid() to infer if governance is enabled.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
knst
approved these changes
Apr 25, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Since #5940,
CActiveMasternodeManagerceased to be a global variable and became a conditional smart pointer initialized based on the value offMasternodeMode.Likewise, since #5555, we can tell if any
CFlatDB-based manager has successfully loaded its database.CGovernanceManageris one of them and conditionally loads its database based on the value offGovernanceDisabled.fMasternodeModeandfGovernanceDisabledwere (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)andif (mn_activeman != nullptr). The former is used in initialization and RPC code due to existing conventions there (source, source, source). The latter is used whenever the value has to be passed as abool(you cannot pass the implicit conversion to aboolargument without explicitly casting it) and in Dash-specific code where it is the prevalent convention (source, source).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_activemanisn't directly available, the result of the check is passed as an argument namedis_masternodeand/or set for the manager during its construction asm_is_masternode(const bool) as it is expected for theCActiveMasternodeManager'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_activemanwhile 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
fMasternodeModeorfDisableGovernancefrom desynchronizing from the behaviour of the managers it's suppose to influence. It's why additional assertions were placed in to make sure thatfMasternodeModeand the existence ofmn_activemanwere always in sync (source, source).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: