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

Decouple mn payments from version.h, drop MIN_MNW_PEER_PROTO_VERSION #960

Merged
merged 2 commits into from
Sep 8, 2016
Merged

Decouple mn payments from version.h, drop MIN_MNW_PEER_PROTO_VERSION #960

merged 2 commits into from
Sep 8, 2016

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 23, 2016

Changes:

  • decouple payments from from version.h, move MIN_MASTERNODE_PAYMENT_PROTO_VERSION_* to masternode-payments.h
  • drop MIN_MNW_PEER_PROTO_VERSION in fav of GetMinMasternodePaymentsProto(), imo when spork10 is switched to "new only" state there is no reason to receive MNWINNER from outdated peers or to allow them to vote for new winner

EDIT: Any cases where having separate MIN_MNW_PEER_PROTO_VERSION could be helpful but I'm missing it?

@UdjinM6 UdjinM6 changed the title Decouple from version.h, drop MIN_MNW_PEER_PROTO_VERSION Decouple mn payments from version.h, drop MIN_MNW_PEER_PROTO_VERSION Aug 23, 2016
@tgflynn
Copy link

tgflynn commented Aug 29, 2016

So it looks like the substantive change here is to put the minimum version for receiving MN payments under control of SPORK_10.

The code looks good to me but I wonder if it wouldn't make more sense to leave all version constants in version.h ? Why do you think it's a good idea to move these ?

@UdjinM6
Copy link
Author

UdjinM6 commented Aug 29, 2016

re version.h: yeah, we had all versions there but imo it's not ideal solution at all. imo it's better to have module specific version right in this module's header and not somewhere outside. Also if you take a closer look at original (non-dash) variables, only PROTOCOL_VERSION and MIN_PEER_PROTO_VERSION are a subject to change sometimes and they are basically global for each and every module. Other *_VERSION variables are kind of static "points" in "version time" when some feature was implemented (or a bug happened).

@schinzelh
Copy link

Needs a rebase

…SION in fav of GetMinMasternodePaymentsProto()
@UdjinM6
Copy link
Author

UdjinM6 commented Aug 30, 2016

Rebased but I still have some concerns about this piece https://github.com/dashpay/dash/pull/960/files#diff-27f99aaf2c6f267cd36ff731a1409af3R559 actually. Probably should use MIN_MASTERNODE_PAYMENT_PROTO_VERSION_1 so that in the time of migration winners signed by non-upgraded masternodes for old blocks (or should I instead add some additional check for that?) still were legit and were able to pass IsValid()? New winners sent by non-upgraded masternodes should be rejected by ProcessMessage() anyway of course. Thoughts?

@UdjinM6
Copy link
Author

UdjinM6 commented Sep 1, 2016

IsValid for old blocks will accept winners signed by old masternodes now.

@UdjinM6 UdjinM6 merged commit b7e81cb into dashpay:v0.12.1.x Sep 8, 2016
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