[WIP] DIP2/DIP3/DIP4 Implementation #2083

codablock · 2018-05-25T10:58:52Z

Introduction

This is my current version of the DIP2/DIP3/DIP4 implementation. It is not meant to be merged for 12.3 but for a later version (most likely 12.4)
Links to DIPs:
DIP2 — Special Transactions
DIP3 — Deterministic Masternode Lists
DIP4 — Simplified Verification of Deterministic Masternode Lists

Note for previous and new reviewers

Originally, this code was part of a PR in our private dashevo repo. It is however cleaned up (squashed and rebased all the review related commits) and also contains the DIP4 implementation, which was missing in the initial PR. It also contains a few more fixes and a refactoring to optimize the performance of deterministic MN bookkeeping. I decided to not squash these new fixes and refactorings into the older commits to make life easier for reviewers who already reviewed the old version of the PR. For these reviewers, the new changes start from commit 2ffdb238676d462683739a075bfc27fa06cde8d6 ("Implement and enforce CbTx with correct block height and deprecate BIP34").

Deployment

These DIPs will be deployed through a combination of a BIP9 style deployment and a spork (SPORK_15_DETERMINISTIC_MNS_ENABLED). When the BIP9 deployment is activated, MNOs will be allowe to broadcast DIP3 special transactions and miners will be allowed to mine DIP2/DIP3 special transactions. This will also activate the bookkeeping (register, update, remove, ...) of deterministic masternodes.

It will however NOT start the use of deterministic masternodes and the old type of MNB/MNP based masternode system will keep being active. Only when spork15 gets activated, the whole network will switch over to use deterministic masternodes for all kinds of MN operations.

Between the BIP9 activation and the spork15 activation, MNOs are expected to gradually move their MNs to be ProTx based. They'd still have to issue "mn-start" on their masternodes as the old system is still active at this stage. A ProTx is internally just a normal transaction with a special payload, and thus is still a valid collateral for a MN of the old system. Between the BIP9 activation and spork15 activation, masternode broadcasts will be rejected if the collateral was created after BIP9 activation and is not a valid ProTx.

The reason for this multi stage deployment is that we must ensure that enough MNOs have switched to deterministic masternodes first before we actually switch over to use the new deterministic list. This should ensure a smooth migration without interruptions.

Spork15 is block height based and not time based (as other sporks usually are). When we see enough MNs being upgraded to ProTX, we will set spork15 to a block height in the near future. When that height is reached, all nodes in the network will drop the legacy masternode list and switch to the deterministic list. At the same time, all communication in regard to MNBs, MNWs and MNPs will be stopped. Also, mnsync will slightly change so that it completely skips the MASTERNODE_SYNC_LIST and MASTERNODE_SYNC_MNW state.

The masternode.conf file will then be ignored and masternode start-xxx won't be required anymore. As long as the MNO has -masternodeprivkey= correctly set, the node will immediately start functioning when it encounters the ProTX on-chain. If the node was started correctly and functioning as a legacy MN at the time of spork15 activation, it will switch to the deterministic mode automatically without the MNOs intervention.

Code-level compatibility between legacy and deterministic mode

As DIP3 has effects on many parts of the code and we at the same time need to support legacy and deterministic mode in the same code base, I decided to not touch too much of the legacy code and design. Instead, I tried to keep the interfaces of classes like CMasternode, CMasternodeMan, CMasternodePayments, ... as compatible as possible with the legacy code and implement the switching logic into these classes. The result is that these classes now contain some ugly if/else statements but at the same time all the other code that uses these classes is mostly untouched. IMHO, this reduces the risk of introduced bugs dramatically. After spork15 activation, my next task will be to clean up the code again and remove all the legacy code so that we have a clear codebase again.

Maintaining the deterministic MN list

Each block might result in a new version of the MN list. These lists are maintained by CDeterministicMNListManager. ProcessBlock is called for each block, which loops through all ProTx of the block and creates a new MN list based on the previous list and the freshly gathered information. It also scans for spends of ProRegTx collaterals and removes MNs if needed. The information about what has been added/updated/removed is stored in leveldb once per block (with CDeterministicMNListDiff).

Every 576 blocks, a snapshot of the list of the current block is stored in leveldb. Based on the snapshots and the diffs that follow, the list for every block height can be reconstructed (see GetListForBlock). A cache is used to avoid doing this too often.

A list is represented by CDeterministicMNList. It internally uses pure functional maps (immer::map) and shared pointers to hold the actual list and information about each entry. (Note for previous reviewers: this was different in the initial version). The reason for these data structures is that it simplifies the deduplication of slightly changed lists between blocks, which in turn dramatically reduces the memory usage.

codablock · 2018-05-28T06:57:23Z

FYI: I'm currently working on fixing Travis builds, which are failing due to the use of C++14 features and Travis using Ubuntu Trusty. Please ignore the WIP commits at the moment. I'll clean them up later and put them into a separate PR.

Build is mostly fixed, but currently running into timeouts. I hope it's just Travis being slow atm.

The "fix" is actually also a refactoring of how we do Travis builds. ~~I'll put this into a separate PR after the 12.3 release (to not disturb the release). EDIT: done~~

nmarley

Not a complete review, only got as far as src/evo/evodb.cpp for now.

nmarley · 2018-05-28T05:27:25Z

.travis.yml

-    - if [ "$RUN_TESTS" = "true" ]; then qa/pull-tester/rpc-tests.py --coverage; fi
-    - cd ../..
+    - >
+      docker run -t --rm \


Would it make sense to extract this to a docker-compose.yml file? Would be the same amount of complexity, but .travis.yml would be less cluttered.

nmarley · 2018-05-28T05:42:48Z

qa/rpc-tests/test_framework/mininode.py

            self.sha256 = tx.sha256
            self.hash = tx.hash

    def deserialize(self, f):
-        self.nVersion = struct.unpack("<i", f.read(4))[0]
+        ver32bit = struct.unpack("<i", f.read(4))[0]


Is it necessary to read as ver32bit? Since the version field is uint32 and little-endian, and backwards-compatible (meaning, there shouldn't be any current TXes with version fields except 1, 2, so this should be do-able with just:

self.nVersion = struct.unpack("<h", f.read(2))[0], and likewise for self.nType, right?

You're right, but I found this more explicit in what is happening there.

nmarley · 2018-05-28T05:43:42Z

qa/rpc-tests/test_framework/mininode.py

-        self.nVersion = struct.unpack("<i", f.read(4))[0]
+        ver32bit = struct.unpack("<i", f.read(4))[0]
+        self.nVersion = ver32bit & 0xffff
+        self.nType = (ver32bit >> 16) & 0xffff


Since this is already right-shifted 16 bits, it looks like this last bitmask shouldn't be necessary. Unless I'm missing something (which is possible).

But, if my comments above are also correct, then this should be a non-issue anyway.

I've got burned by C/C++ in the past when it comes to right shifting of (possibly) negative numbers as it extends the sign bit. This means, 1110 >> 1 = 1111 and not 0111 as someone might expect. And as I understand this, it's the same in python.

nmarley · 2018-05-28T05:44:48Z

qa/rpc-tests/test_framework/mininode.py

        self.sha256 = None
        self.hash = None

    def serialize(self):
        r = b""
-        r += struct.pack("<i", self.nVersion)
+        ver32bit = int(self.nVersion | (self.nType << 16))


Same logic as above, but in reverse.

nmarley · 2018-05-28T06:49:37Z

src/evo/deterministicmns.cpp

+
+    auto payee = oldList.GetMNPayee();
+
+    for (const auto& dmn : newList.all_range()) {


Is this supposed to do something?

nmarley · 2018-05-28T06:52:17Z

src/evo/deterministicmns.cpp

+    for (const auto& dmn : newList.all_range()) {
+    }
+
+    for (int i = 1; i < (int)block.vtx.size(); i++) {


Does this start at 1 to intentionally skip the coinbase? If so, I'm wondering if maybe this warrants a comment here somewhere, as an explanatory note to other developers.

Allows testing of features which depend on BIP9 deployments

We need to update the CbTx's merkle root for the masternode list here as tests might add transactions into a block which cause the list to change.

As we do with ContextualCheckBlock

…licating the check

This also moves the TX version check logic into ContextualCheckTransaction

Related to dashpay#2237

UdjinM6 · 2018-08-29T08:54:13Z

A couple of patches as a result of local debugging:

UdjinM6@d82885d (Fix sync, see Fix sync in regtest (again) #2241)
UdjinM6@0b7daff (Rewrite CDeterministicMNManager::GetListForBlock in a non-recursive way)
UdjinM6@e4ef4ee (Add sleep intervals: lowers cpu usage + helps finding issues by making logs much less spammy in debug mode)

codablock · 2018-09-14T12:58:47Z

Closing as DIP2, DIP3 and DIP4 have been merged into develop as part of separate PRs.

codablock force-pushed the pr_dip3_dashpay branch 18 times, most recently from c1f5e15 to 8ffe44f Compare May 25, 2018 19:26

UdjinM6 added this to the 12.4 milestone May 25, 2018

codablock force-pushed the pr_dip3_dashpay branch from 8ffe44f to 7dab2df Compare May 28, 2018 06:56

codablock force-pushed the pr_dip3_dashpay branch 2 times, most recently from 3bf12c2 to d5ee817 Compare May 28, 2018 07:09

nmarley reviewed May 28, 2018

View reviewed changes

codablock force-pushed the pr_dip3_dashpay branch 6 times, most recently from a97c0d2 to ca345ad Compare May 28, 2018 17:20

codablock and others added 17 commits August 27, 2018 14:50

Add missing version field in CbTx in mininode.py

072dca8

Review fix: Move comment above if

e441eaa

fix warning

3301d1b

add comment

1d43eb7

revert changes in CPartialMerkleTree

2ff55fe

Invoke ProcessTick on "mnsync next"

a14fd7a

Serialize nVersion in CCbTx

323cb33

Refactor TestChain100Setup to allow different test chain setups

c48f4f7

Allows testing of features which depend on BIP9 deployments

Update CbTx in TestChainSetup

d893120

We need to update the CbTx's merkle root for the masternode list here as tests might add transactions into a block which cause the list to change.

Add unit tests for DIP3 and DIP4

88b46e1

Only revive masternodes when the keys were previously set

555d529

Add version to string/json output of CbTx

2ca9e79

Pass consensus params to ContextualCheckTransaction

16155c0

As we do with ContextualCheckBlock

Use ContextualCheckTransaction in ContextualCheckBlock instead of dup…

77f1686

…licating the check

Only enforce DIP2 version changes when DIP2/DIP3 got activated

abf0049

This also moves the TX version check logic into ContextualCheckTransaction

Fix pindexPrev parameter signature in ContextualCheckTransaction

ccc8497

Use refactored payment logic when spork15 is active

483070a

Related to dashpay#2237

codablock force-pushed the pr_dip3_dashpay branch from e57841e to 483070a Compare August 27, 2018 13:48

codablock mentioned this pull request Aug 28, 2018

DIP2 / Special Transactions base implementation #2239

Merged

UdjinM6 mentioned this pull request Aug 29, 2018

Fix sync in regtest (again) #2241

Merged

This was referenced Aug 30, 2018

First part of DIP3 implementation #2246

Merged

Split keyIDMasternode into keyIDOwner/keyIDOperator/keyIDVoting #2248

Merged

DIP4 implementation #2251

Merged

DIP3 compatibility code #2255

Merged

DIP3 MN reward payment logic #2258

Merged

codablock closed this Sep 14, 2018

codablock deleted the pr_dip3_dashpay branch December 27, 2018 15:23

UdjinM6 removed this from the 13.0 milestone Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] DIP2/DIP3/DIP4 Implementation #2083

[WIP] DIP2/DIP3/DIP4 Implementation #2083

Uh oh!

codablock commented May 25, 2018

Uh oh!

codablock commented May 28, 2018 •

edited

Loading

Uh oh!

nmarley left a comment

Uh oh!

nmarley May 28, 2018

Uh oh!

nmarley May 28, 2018

Uh oh!

codablock Jun 4, 2018

Uh oh!

nmarley May 28, 2018

Uh oh!

codablock Jun 4, 2018

Uh oh!

nmarley May 28, 2018

Uh oh!

nmarley May 28, 2018

Uh oh!

nmarley May 28, 2018

Uh oh!

UdjinM6 commented Aug 29, 2018 •

edited

Loading

Uh oh!

codablock commented Sep 14, 2018

Uh oh!

Uh oh!


		auto payee = oldList.GetMNPayee();

		for (const auto& dmn : newList.all_range()) {

[WIP] DIP2/DIP3/DIP4 Implementation #2083

[WIP] DIP2/DIP3/DIP4 Implementation #2083

Uh oh!

Conversation

codablock commented May 25, 2018

Introduction

Note for previous and new reviewers

Deployment

Code-level compatibility between legacy and deterministic mode

Maintaining the deterministic MN list

Uh oh!

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

Uh oh!

Uh oh!

nmarley left a comment

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!

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 commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codablock commented Sep 14, 2018

Uh oh!

Uh oh!

codablock commented May 28, 2018 •

edited

Loading

UdjinM6 commented Aug 29, 2018 •

edited

Loading