Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor masternode management #1611

Merged
merged 7 commits into from
Sep 11, 2017
Merged

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Sep 5, 2017

Various refactoring for CMasternodeMan + long postponed CTxIn vin -> COutPoint outpoint.

Note: there are no changes on serialization/network level yet, so some legacy classes are still going to use CTxIn vin-like members. Changing this will require network upgrade, I'll submit this patch as a separate PR later - this could potentially break network compatibility and it doesn't qualify for simple refactoring anyway... Or we could postpone this last bit till 12.3 to simplify 12.2 testing.

Commits are atomic, can be reviewed separately, one by one.

@UdjinM6 UdjinM6 added this to the 12.2 milestone Sep 5, 2017
@UdjinM6 UdjinM6 force-pushed the refactorMnodeman branch 2 times, most recently from 37d0065 to 7a61630 Compare September 5, 2017 08:21
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

👍
Nice refactoring, all looks good.

return;
}
pMN->lastPing = mnp;
pmn->lastPing = mnp;
Copy link

@codablock codablock Sep 7, 2017

Choose a reason for hiding this comment

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

Hmm not introduced by this PR, but why is there a critical section in CMasternode if it is not locked in this case (and maybe other cases)?

EDIT: Ah, after reviewing the commits after this one I've seen that you removed all direct access to the CMasternode instances from outside of CMasternodeMan 👍
This probably means that there is no need for the critical section in CMasternode anymore?

Copy link
Author

@UdjinM6 UdjinM6 Sep 7, 2017

Choose a reason for hiding this comment

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

Hmmm... seems like critical section isn't doing much in CMasternode at all... but it doesn't break anything either, so I'd keep it for now. I have another set of changes for locks specifically (in local repo), so that probably would be a better place to change this.

@@ -96,7 +96,7 @@ class CMasternodeMan
READWRITE(strVersion);
}

READWRITE(vMasternodes);
READWRITE(mapMasternodes);

Choose a reason for hiding this comment

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

Wondering why CMasternodeMan needs to be serialized. mncache.dat?
If it's really serialized, this may result in incompatibility.

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I'll bump CMasternodeMan::SERIALIZATION_VERSION_STRING

@UdjinM6
Copy link
Author

UdjinM6 commented Sep 9, 2017

rebased to fix conflicts after #1615

@UdjinM6 UdjinM6 merged commit 05da455 into dashpay:v0.12.2.x Sep 11, 2017
thelazier added a commit to thelazier/sentinel that referenced this pull request Sep 14, 2017
@UdjinM6 UdjinM6 deleted the refactorMnodeman branch January 31, 2019 19:43
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.

None yet

2 participants