Masternode related refactorings in preparation of DIP3 by codablock · Pull Request #2212 · dashpay/dash

codablock · 2018-08-10T10:51:54Z

This is a set of commits extracted from #2083. These are mostly independent from DIP3 and can thus be merged before.

These refactorings are included:

Split CActiveMasternode into CActiveMasternodeInfo and CLegacyActiveMasternodeManager

This splits out the locally active masternode's state into it's own class/object. The actual handling/management of the active masternode is now in CLegacyActiveMasternodeManager. DIP3 will later introduce a new manager, which is then responsible for the deterministic version of the active masternode. After DIP3 deployment is completed, a cleanup PR will remove the legacy masternode manager.

## Update masternode outpoint in case a MNB arrives with a changed outpoint
This is not really a refactoring but a new feature. It allows to restart a masternode based on a new collateral. This is likely not so useful for regular MNOs, but quite useful for integration tests and large devnets as it avoids restarting of MNs.

Use CKeyID instead of CPubKey whenever possible in masternode code

This avoids using CPubKey when actually only the key hash is enough. This gets important in DIP3 when the masternode key is split into multiple keys and DIP3 special transactions actually never include the full public key but only the key hash.

UdjinM6

757b6e5 doesn't fit refactoring title (and I don't quite understand the need for it tbh), should be removed from this PR imo. See other comments below.

UdjinM6 · 2018-08-10T11:15:27Z

src/activemasternode.cpp

Should probably change the name to activeMasternodeInfo for it to be a bit more descriptive.

Hmm agree. I tried to reduce the number of changed lines so I left this out 🙈
Adding new commit on top for this.

UdjinM6 · 2018-08-10T11:16:24Z

src/activemasternode.h

s/class/struct/

UdjinM6 · 2018-08-10T11:19:13Z

src/activemasternode.h

Should drop pub prefix i.e. s/pubKeyIDMasternode/keyIDMasternode/. Same for all other new CKeyIDs.

Done in separate commits. I'll probably go crazy because of this when doing the rebase of DIP3 🙈 :D

codablock · 2018-08-10T12:16:06Z

Done the suggested renaming and removed 757b6e5. I don't remember exactly anymore why/when it was needed, will have to figure this out again later when doing the DIP3 rebase.

UdjinM6

Missed one LogPrintf at line 138 https://github.com/dashpay/dash/blob/6a6e9a6a29dec6de9dd5cf1deed6c6ac78b65541/src/activemasternode.cpp#L138, otherwise looks good 👍

UdjinM6 · 2018-08-10T22:39:10Z

Needs rebase

…asternodeManager

codablock · 2018-08-11T13:17:22Z

Rebased and fixed last nit (squashed into the original commit)

UdjinM6

Fixes look 👍 Just noticed one more thing though, see below 🙈

UdjinM6 · 2018-08-11T17:24:32Z

src/masternode.h

        READWRITE(addr);
        READWRITE(pubKeyCollateralAddress);
        READWRITE(pubKeyMasternode);
+        READWRITE(keyIDCollateralAddress);


Should probably bump CMasternodeMan::SERIALIZATION_VERSION_STRING (masternodeman.cpp#L29) because of changes in serialization.

UdjinM6 · 2018-08-11T18:11:01Z

Build fails at

dash/src/activemasternode.cpp

Line 164 in 986e734

if (Lookup("127.0.0.1", activeMasternode.service, GetListenPort(), false)) {

activemasternode.cpp:164:33: error: 'activeMasternode' was not declared in this scope
         if (Lookup("127.0.0.1", activeMasternode.service, GetListenPort(), false)) {
                                 ^~~~~~~~~~~~~~~~
activemasternode.cpp:164:33: note: suggested alternative: 'activeMasternodeInfo'

codablock · 2018-08-11T18:14:06Z

Whoops, sorry about that one. Resulted from the recent rebase...verifying fix locally now and then pushing again.

UdjinM6

Looks good now imo 👍

Let's see if Travis thinks the same :)

UdjinM6 · 2018-08-11T18:32:16Z

src/activemasternode.h

 #include "primitives/transaction.h"

-class CActiveMasternode;
+class CActiveMasternodeInfo;


Heh, compiling locally and

In file included from activemasternode.cpp:5: ./activemasternode.h:25:1: warning: 'CActiveMasternodeInfo' defined as a struct here but previously declared as a class [-Wmismatched-tags] struct CActiveMasternodeInfo { ^ ./activemasternode.h:13:1: note: did you mean struct here? class CActiveMasternodeInfo; ^~~~~ struct 1 warning generated.

:)

These fancy Mac users :P
Added a commit to fix it ;)

UdjinM6

re-utACK

UdjinM6 requested changes Aug 10, 2018

View reviewed changes

UdjinM6 added this to the 12.4 milestone Aug 10, 2018

codablock force-pushed the pr_dip3_masternoderefactorings branch from 1885332 to 6a6e9a6 Compare August 10, 2018 12:15

codablock mentioned this pull request Aug 10, 2018

Refactor MN payee logic in preparation for DIP3 #2215

Merged

UdjinM6 requested changes Aug 10, 2018

View reviewed changes

codablock added 4 commits August 11, 2018 15:16

Split CActiveMasternode into CActiveMasternodeInfo and CLegacyActiveM…

19cb85c

…asternodeManager

Use CKeyID instead of CPubKey whenever possible in masternode code

057489a

Rename activeMasternode to activeMasternodeInfo and make it a struct

99ba65a

Rename pubKeyIDXXX to keyIDXXX

9dd511f

codablock force-pushed the pr_dip3_masternoderefactorings branch from 6a6e9a6 to 9dd511f Compare August 11, 2018 13:16

UdjinM6 requested changes Aug 11, 2018

View reviewed changes

Bump SERIALIZATION_VERSION_STRING

986e734

Fix build error after rebase

e0824ba

UdjinM6 previously approved these changes Aug 11, 2018

View reviewed changes

UdjinM6 reviewed Aug 11, 2018

View reviewed changes

Fix compilation warning/error with clang

b567a95

codablock dismissed UdjinM6’s stale review via b567a95 August 11, 2018 19:19

UdjinM6 approved these changes Aug 11, 2018

View reviewed changes

UdjinM6 merged commit d946f21 into dashpay:develop Aug 11, 2018

codablock deleted the pr_dip3_masternoderefactorings branch September 14, 2018 12:51

Conversation

codablock commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Split CActiveMasternode into CActiveMasternodeInfo and CLegacyActiveMasternodeManager

Use CKeyID instead of CPubKey whenever possible in masternode code

Uh oh!

UdjinM6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock commented Aug 10, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Aug 10, 2018

Uh oh!

codablock commented Aug 11, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Aug 11, 2018

Uh oh!

codablock commented Aug 11, 2018

Uh oh!

UdjinM6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codablock commented Aug 10, 2018 •

edited

Loading

UdjinM6 left a comment •

edited

Loading

UdjinM6 left a comment •

edited

Loading