Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Oct 6, 2024

Issue being fixed or feature implemented

This PR is preparation for bitcoin#19668, otherwise impossible to make lock annotations for CGovernanceManager properly.

What was done?

  1. object mn_sync and peerman is pass to many methods of CGovernanceManager instead passing it to constructor.
  2. methods of class CSuperblockManager moved to CGovernanceManager where they belongs to.
  3. removed CSuperblock::GetGovernanceObject which makes a lot of mess with annotations of govman.cs

And minor relevant improvements: moved ScopedLockBool from header to implementation, added multiple const for methods, added one more file flat-database.h to non-backported list

How Has This Been Tested?

Run unit and functional tests.

Breaking Changes

N/A

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
  • I have assigned this pull request to a milestone

@knst knst added this to the 21.2 milestone Oct 6, 2024
@knst knst force-pushed the fix-circular-governance-classes branch 3 times, most recently from 5ecd62c to e10e3b3 Compare October 7, 2024 17:38
@UdjinM6
Copy link

UdjinM6 commented Oct 8, 2024

Not sure I understand the reason for 7bb236e. Why would we want this?

@knst
Copy link
Collaborator Author

knst commented Oct 8, 2024

Not sure I understand the reason for 7bb236e. Why would we want this?

This commit aims 2 reasons:

  • break circular dependency over PeerManager (net_processing) same as in this PR: refactor: remove dependency of llmq/chainlocks, llmq/quorum_block_processor, ehf_signals on PeerManager #6292
  • CGovernanceManager's constructor accepts not a reference, but the unique_ptr (const std::unique_ptr<CMasternodeSync>& mn_sync);). It happens due to order of objects initialization: CGovernanceManager is init before CMasternodeSync, but it requires CMasternodeSync. To fix this order of initialization, better to pass mn_sync when it's really need rather then keep "unique_ptr" somewhere inside class, which can be in valid state or invalid state and changed at any moment back to nullptr...

1st one (break circular dependency over PeerManager) is not done yet, that's just a preparartion
2nd one is compete here. Need to do something similar with const std::unique_ptr<CDeterministicMNManager>& dmnman later one.

This commit can be move to own PR in theory, or even be split to 2 commits (CMasternodeSync, PeerManager), but I'd prefer to don't do it to avoid multiple conflicts resolving

@UdjinM6
Copy link

UdjinM6 commented Oct 8, 2024

How about 05a717b instead?

@knst knst force-pushed the fix-circular-governance-classes branch from e10e3b3 to 8f02803 Compare October 8, 2024 17:10
@knst
Copy link
Collaborator Author

knst commented Oct 8, 2024

How about 05a717b instead?

That looks good, I replaced my commit to this one; changes related to PeerManager will go to other PR

@knst knst force-pushed the fix-circular-governance-classes branch from 8f02803 to 2e36832 Compare October 8, 2024 17:13
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.

generally LGTM 2e36832

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 2e36832

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 2e36832

@PastaPastaPasta PastaPastaPasta merged commit f93c763 into dashpay:develop Oct 8, 2024
37 checks passed
@knst knst deleted the fix-circular-governance-classes branch October 9, 2024 13:17
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
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.

3 participants