[WIP] Remove all legacy pre-DIP3 masternode and compatibility code #2566

codablock · 2018-12-18T16:28:54Z

This PR is meant for v14 and will not be merged into v13. This should be one of the first things we takle when starting development on v14.

It does the following:

Get rid of all the ugly compatibility code which was necessary while deploying DIP3. It basically changes all uses of CMasternodeMan to directly use CDeterministicMNManager.
Introduce CMasternodeMetaMan as a replacement for the things that would have stayed in CMasternodeMan. The idea behind it is to store meta-information (e.g. about mixing and governance) that is not derived from on-chain data.
Remove all legacy MN P2P messages (MNB, MNP, ...)
Remove multiple sporks which are not required anymore in v14 (including SPORK_6_NEW_SIGS and all the logic for it)
Many other cleanups of stuff that we won't need anymore in v14

This PR got quite huge and I'm not sure if I'll be able to extract portions of it into smaller PRs. The problem here is that touching a single place very often breaks everything, requiring to fix more and more until you're back to very huge PR.

In the current state, tests will very likely fail. I'll start fixing them later when work on v14 actually starts.

There are still places where these are used in the code. The next commits will clean these up.

Also replace all uses of IsMasternodeListSynced and IsWinnersListSynced with IsBlockchainSynced.

And only leave the code paths that would be executed when the spork is active (which it implicitely is with the next release). Also remove all methods that do nothing with the spork active.

…Votes

…ministicMNManager

…nager

And remove them with direct use of deterministicMNManager

…e used directly Additionally, implement GetLastDsq in CMasternodeMan as a replacement for direct access to the masternode_info_t object. Will move this variable to another (PS specific) place later.

And adapt it to directly use deterministicMNManager

…kTxOuts ...and let it crash instead. We expect this method to be called with the correct height now (after DIP3 was fully deployed).

And always enforce payments.

…PAY_UPDATED_NODES

Only masternodes sign governance objects, so there is no need for ECDSA support here anymore.

IsDIP3Active will now use a fixed parameter from consensus params. Values for DIP0003Height/DIP0003Hash need to be updated when spork15 activates on testnet/mainnet. Also enforce correct block hash on mainnet for DIP3 activation block.

Also delete guide-startmany.md and masternode_conf.md

…ests

UdjinM6

Noticed few things, see below.

UdjinM6 · 2018-12-18T18:33:06Z

src/init.cpp

-    strUsage += HelpMessageOpt("-mnconf=<file>", strprintf(_("Specify masternode configuration file (default: %s)"), "masternode.conf"));
-    strUsage += HelpMessageOpt("-mnconflock=<n>", strprintf(_("Lock masternodes from masternode configuration file (default: %u)"), 1));
-    strUsage += HelpMessageOpt("-masternodeprivkey=<n>", _("Set the masternode private key"));
+    strUsage += HelpMessageOpt("-masternodeblsprivkey=<n>", _("Set the masternode BLS private key"));


This help message should be added in 0.13 actually

UdjinM6 · 2018-12-18T18:34:09Z

src/init.cpp

@@ -2036,29 +1998,18 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)

        strDBName = "mncache.dat";
        uiInterface.InitMessage(_("Loading masternode cache..."));
-        CFlatDB<CMasternodeMan> flatdb1(strDBName, "magicMasternodeCache");
-        if(!flatdb1.Load(mnodeman)) {
+        CFlatDB<CMasternodeMetaMan> flatdb1(strDBName, "magicMasternodeMetaCache");


Is altering magic necessary? This will require user to manually delete the file on wallet upgrade which could be annoying. Altering "version" should already do the job and clear all the previous data.

ah didn't notice the extra handling of the magic string and assumed it's only the string compare in the serialization methods. Will change this later.

Yep, the purpose of the magic string is to ensure that this is exactly the kind of file we expected and not some renamed wallet.dat for example (in which case user will have to manage things manually and rename it to smth else).

UdjinM6 · 2018-12-18T18:53:28Z

src/spork.cpp

-                Misbehaving(pfrom->GetId(), 100);
-                return;
-            }
+        if (!spork.GetSignerKeyID(keyIDSigner) || !setSporkPubKeyIDs.count(keyIDSigner)) {


This can only be done after spork6 is activated on mainnet (it's still OFF atm). I would not include these changes (and similar) in this PR.

Ok, will remove these changes

nmarley · 2018-12-21T13:33:39Z

Hey @codablock , this is a great effort. Glad to see us start removing the old legacy code.

This PR got quite huge and I'm not sure if I'll be able to extract portions of it into smaller PRs. The problem here is that touching a single place very often breaks everything, requiring to fix more and more until you're back to very huge PR.

I like that you're removing all this old stuff, but I don't agree with your rationale for the really big PR. For example, you can remove protocol documentation in a later PR and keep the code PR minimal. Or another example (and I've only glanced at this for now), some of the string / RPC usage cleanup is just general "cleanup" and not specific to removing old MN logic, but it gets too easy to stuff simple things in here and this becomes too large and generic.

As you know, I'm very skeptical of overly large PRs because they inevitably introduce bugs which are harder to catch due to the size of the diff. And that's of course the reason that is in our PR guidelines as well:

Patchsets should always be focused. For example, a pull request could add a feature, fix a bug, or refactor code; but not a mixture. Please also avoid super pull requests which attempt to do too much, are overly large, or overly complex as this makes review difficult.

I wanna remove this old stuff as much as you do, and I do understand it's all connected once you dive down this rabbit hole, but in general I think we should be able start sticking closer to these guidelines and make these smaller.

Here are some ideas for separate PRs:

Introduce CMasternodeMetaMan as a replacement for the things that would have stayed in CMasternodeMan. The idea behind it is to store meta-information (e.g. about mixing and governance) that is not derived from on-chain data.
Remove multiple sporks which are not required anymore in v14 (including SPORK_6_NEW_SIGS and all the logic for it)
Many other cleanups of stuff that we won't need anymore in v14

Here are 2 more which are probably related and harder to tease apart, so these might be fine together (would still prefer if possible to separate them):

Get rid of all the ugly compatibility code which was necessary while deploying DIP3. It basically changes all uses of CMasternodeMan to directly use CDeterministicMNManager.
Remove all legacy MN P2P messages (MNB, MNP, ...)

Of course those are just your own bullet points, split up. I'm going to try a full review of this (again, this becomes harder the larger these get), but this first one I think should esp. be separate, otherwise we mix refactoring, cleanup, new features, documentation, all in one massive PR.

I hope you understand and can agree with this, but please let me know your thoughts.

codablock · 2018-12-21T14:12:32Z

@nmarley Thanks for your review and notes. I will work on splitting this further up after the v13 release, until then it's probably better to wait with deeper reviews.

nmarley · 2018-12-23T14:20:54Z

Sounds good, thanks @codablock

codablock · 2018-12-28T11:01:49Z

closing this now as I started doing smaller PRs

codablock added 30 commits December 18, 2018 08:42

Also allow non-const T in Serialize/Unserialize for shared_ptr

db4d471

Remove CActiveLegacyMasternodeManager

99ffce5

Remove sentinelping RPC

730ce25

Remove unused P2P messages and inv types

c649922

There are still places where these are used in the code. The next commits will clean these up.

Remove MNB/MNP/MNVERIFY related code from masternode(man).h/cpp

916d892

Remove all legacy code regarding block MN payee voting

5f8ef2a

Remove MASTERNODE_SYNC_LIST and MASTERNODE_SYNC_MNW states

899a692

Also replace all uses of IsMasternodeListSynced and IsWinnersListSynced with IsBlockchainSynced.

Remove conditionals for IsDeterministicMNsSporkActive

46aadde

And only leave the code paths that would be executed when the spork is active (which it implicitely is with the next release). Also remove all methods that do nothing with the spork active.

Remove nProtocolVersion filtering from CountMasternodes/CountEnabled

24feb38

Remove unsupported masternode RPCs

7323056

Let "masternode winner/current" directly use deterministicMNManager

6c824bf

Remove UpdateLastPaid methods

0cf9f4b

Remove unsupported types/fields from "masternode list"

f07fbbb

Remove duplicate deterministicmns.h include

7185bc5

Directly use deterministicMNManager in CGovernanceManager::GetCurrent…

c3df089

…Votes

Remove support for legacy operator keys in CPrivateSendBroadcastTx

aad090e

Directly use deterministicMNManager when processing DSTX

c0fa78f

Remove compatibility code from governance RPCs and directly use deter…

fe1e8ab

…ministicMNManager

Remove masternode.conf support

fd1b590

Remove legacy MN lists support from masternode list GUI

39673a7

Implement HasValidMN, HasValidMNByCollateral and GetValidMNByCollateral

5b8923b

Remove Has/Get from CMasternodeMan and directly use deterministicMNMa…

3543613

…nager

Remove unnecessary AskForMN call

7e87502

Don't use GetMasternodeInfo in CTxLockVote::IsValid

c5c0457

Remove support for legacy signatures in CTxLockVote

d66c0bb

Remove legacy signatures support in CPrivateSendQueue

b6a4660

Remove a few uses of mnodeman from governance code

50e24ad

And remove them with direct use of deterministicMNManager

Remove use of mnodeman.GetMasternodeInfo from IX code

087605a

Replace uses of mnodeman in PS code when deterministicMNManager can b…

a0ff198

…e used directly Additionally, implement GetLastDsq in CMasternodeMan as a replacement for direct access to the masternode_info_t object. Will move this variable to another (PS specific) place later.

Move logic from FindRandomNotInVec into GetRandomNotUsedMasternode

1cdf5aa

And adapt it to directly use deterministicMNManager

codablock added 18 commits December 18, 2018 09:05

Rename CActiveDeterministicMasternodeManager to CActiveMasternodeManager

75f48c6

Remove support for "-masternodeprivkey"

6b1ac19

Remove pre-DIP3 vote cleanup

271e14e

Remove compatibility code for quorumModifierHash/masternodeProTxHash

8dd7b19

Remove check for IsDeterministicMNsSporkActive in "masternode status"

6ec9cdd

Remove check for invalid nBlockHeight in CMasternodePayments::GetBloc…

154fcd3

…kTxOuts ...and let it crash instead. We expect this method to be called with the correct height now (after DIP3 was fully deployed).

Remove SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT

49a6073

And always enforce payments.

Remove SPORK_10_MASTERNODE_PAY_UPDATED_NODES and SPORK_10_MASTERNODE_…

4049d61

…PAY_UPDATED_NODES

Remove ECDSA based Sign/CheckSignature from CGovernanceObject

4655b50

Only masternodes sign governance objects, so there is no need for ECDSA support here anymore.

Remove handling of SPORK_6_NEW_SIGS from governance code

c270562

Remove SPORK_6_NEW_SIGS and handling of it in spork.cpp

ea9c83f

Always add superblock and MN reward payments into new block

8ae2036

Always check block payees (except if fLiteMode==true)

3d23bd2

Always allow superblock and MN payees in same block

83430e2

Remove SPORK_15_DETERMINISTIC_MNS_ENABLED

2863e6e

Remove/Fix a few references to masternode.conf and related stuff

21194af

Also delete guide-startmany.md and masternode_conf.md

Remove DIP3 upgrade-path tests and directly start with DIP3 enabled t…

d96ca94

…ests

codablock added this to the 14.0 milestone Dec 18, 2018

UdjinM6 reviewed Dec 18, 2018

View reviewed changes

UdjinM6 mentioned this pull request Dec 21, 2018

Fix IsBlockPayeeValid #2577

Merged

codablock mentioned this pull request Dec 28, 2018

Harden spork15 on testnet #2586

Merged

codablock closed this Dec 28, 2018

codablock mentioned this pull request Dec 31, 2018

Directly use deterministicMNManager instead of compat code in CMasternodeMan #2594

Merged

thephez mentioned this pull request May 28, 2019

Remove masternode start-many check #2950

Merged

UdjinM6 removed this from the 14.0 milestone Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Remove all legacy pre-DIP3 masternode and compatibility code #2566

[WIP] Remove all legacy pre-DIP3 masternode and compatibility code #2566

codablock commented Dec 18, 2018 •

edited

Loading

UdjinM6 left a comment

UdjinM6 Dec 18, 2018

UdjinM6 Dec 18, 2018

UdjinM6 Dec 18, 2018

codablock Dec 18, 2018

UdjinM6 Dec 18, 2018

UdjinM6 Dec 18, 2018

codablock Dec 18, 2018

nmarley commented Dec 21, 2018

codablock commented Dec 21, 2018

nmarley commented Dec 23, 2018

codablock commented Dec 28, 2018

[WIP] Remove all legacy pre-DIP3 masternode and compatibility code #2566

[WIP] Remove all legacy pre-DIP3 masternode and compatibility code #2566

Conversation

codablock commented Dec 18, 2018 • edited Loading

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 Dec 18, 2018

Choose a reason for hiding this comment

UdjinM6 Dec 18, 2018

Choose a reason for hiding this comment

UdjinM6 Dec 18, 2018

Choose a reason for hiding this comment

codablock Dec 18, 2018

Choose a reason for hiding this comment

UdjinM6 Dec 18, 2018

Choose a reason for hiding this comment

UdjinM6 Dec 18, 2018

Choose a reason for hiding this comment

codablock Dec 18, 2018

Choose a reason for hiding this comment

nmarley commented Dec 21, 2018

codablock commented Dec 21, 2018

nmarley commented Dec 23, 2018

codablock commented Dec 28, 2018

codablock commented Dec 18, 2018 •

edited

Loading