From ce524c4a5404cb5f93a4b72ab4e3428a4b12c76f Mon Sep 17 00:00:00 2001 From: Odysseas Gabrielides Date: Sat, 11 Mar 2023 19:42:27 +0200 Subject: [PATCH] fix: mnUniquePropertyMap repopulate for v19 (#5239) ## Issue being fixed or feature implemented `CDeterministicMNList` stores internally a map containing the hashes of all properties that needed to be unique. `pubKeyOperator` don't differ between the two schemes (legacy and basic(v19)) but their serialisation do: hence their hash. Because this internal map stores only hashes, then we need to re-calculate hashes and repopulate. So when we tried to revoke a masternode after the fork, the `ProUpRevTx` couldn't be mined because the hash of the `pubKeyOperator` differed. ## What was done? When retrieving a `CDeterministicMNList` for a given block, if v19 is active for that block, then we repopulate the internal map. ## How Has This Been Tested? Without this fix, `feature_dip3_v19.py` is failing with `failed-calc-cb-mnmerkleroot` (Error encountered on Testnet) ## Breaking Changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone --------- Co-authored-by: pasta Co-authored-by: UdjinM6 --- src/evo/deterministicmns.cpp | 52 ++++++++- src/evo/deterministicmns.h | 2 + test/functional/feature_dip3_v19.py | 159 ++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 213 insertions(+), 1 deletion(-) create mode 100755 test/functional/feature_dip3_v19.py diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 5557ee9186..ecea6e0dd1 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -435,6 +435,48 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(const CBlockIndex* pindex, return result; } +// RepopulateUniquePropertyMap clears internal mnUniquePropertyMap, and repopulate it with currently MNs unique properties. +// This is needed when the v19 fork activates, we need to store again pubKeyOperator in the mnUniquePropertyMap. +// pubKeyOperator don't differ between the two schemes (legacy and basic(v19)) but their serialisation do: hence their hash. +// And because mnUniquePropertyMap store only hashes, then we need to re-calculate hashes and repopulate. +void CDeterministicMNList::RepopulateUniquePropertyMap() { + decltype(mnUniquePropertyMap) mnUniquePropertyMapEmpty; + mnUniquePropertyMap = mnUniquePropertyMapEmpty; + + for (const auto &p: mnMap) { + auto dmn = p.second; + if (!AddUniqueProperty(*dmn, dmn->collateralOutpoint)) { + throw (std::runtime_error( + strprintf("%s: Can't add a masternode %s with a duplicate collateralOutpoint=%s", __func__, + dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort()))); + } + if (dmn->pdmnState->addr != CService() && !AddUniqueProperty(*dmn, dmn->pdmnState->addr)) { + throw (std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s", __func__, + dmn->proTxHash.ToString(), + dmn->pdmnState->addr.ToStringIPPort(false)))); + } + if (!AddUniqueProperty(*dmn, dmn->pdmnState->keyIDOwner)) { + throw (std::runtime_error( + strprintf("%s: Can't add a masternode %s with a duplicate keyIDOwner=%s", __func__, + dmn->proTxHash.ToString(), EncodeDestination(PKHash(dmn->pdmnState->keyIDOwner))))); + } + if (dmn->pdmnState->pubKeyOperator.Get().IsValid() && + !AddUniqueProperty(*dmn, dmn->pdmnState->pubKeyOperator)) { + throw (std::runtime_error( + strprintf("%s: Can't add a masternode %s with a duplicate pubKeyOperator=%s", __func__, + dmn->proTxHash.ToString(), dmn->pdmnState->pubKeyOperator.Get().ToString()))); + } + + if (dmn->nType == MnType::HighPerformance) { + if (!AddUniqueProperty(*dmn, dmn->pdmnState->platformNodeID)) { + throw (std::runtime_error( + strprintf("%s: Can't add a masternode %s with a duplicate platformNodeID=%s", __func__, + dmn->proTxHash.ToString(), dmn->pdmnState->platformNodeID.ToString()))); + } + } + } +} + void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount) { assert(dmn != nullptr); @@ -617,11 +659,19 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde newList.SetBlockHash(block.GetHash()); + // If the fork is active for pindex block, then we need to repopulate property map + // (Check documentation of CDeterministicMNList::RepopulateUniquePropertyMap()). + // This is needed only when base list is pre-v19 fork and pindex is post-v19 fork. + bool v19_just_activated = pindex == llmq::utils::V19ActivationIndex(pindex); + if (v19_just_activated) { + newList.RepopulateUniquePropertyMap(); + } + oldList = GetListForBlock(pindex->pprev); diff = oldList.BuildDiff(newList); m_evoDb.Write(std::make_pair(DB_LIST_DIFF, newList.GetBlockHash()), diff); - if ((nHeight % DISK_SNAPSHOT_PERIOD) == 0 || oldList.GetHeight() == -1) { + if ((nHeight % DISK_SNAPSHOT_PERIOD) == 0 || oldList.GetHeight() == -1 || v19_just_activated) { m_evoDb.Write(std::make_pair(DB_LIST_SNAPSHOT, newList.GetBlockHash()), newList); mnListsCache.emplace(newList.GetBlockHash(), newList); LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n", diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index a0eb19c60d..bd7f74d15f 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -375,6 +375,8 @@ public: [[nodiscard]] CSimplifiedMNListDiff BuildSimplifiedDiff(const CDeterministicMNList& to, bool extended) const; [[nodiscard]] CDeterministicMNList ApplyDiff(const CBlockIndex* pindex, const CDeterministicMNListDiff& diff) const; + void RepopulateUniquePropertyMap(); + void AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount = true); void UpdateMN(const CDeterministicMN& oldDmn, const std::shared_ptr& pdmnState); void UpdateMN(const uint256& proTxHash, const std::shared_ptr& pdmnState); diff --git a/test/functional/feature_dip3_v19.py b/test/functional/feature_dip3_v19.py new file mode 100755 index 0000000000..5b6839dc4a --- /dev/null +++ b/test/functional/feature_dip3_v19.py @@ -0,0 +1,159 @@ +#!/usr/bin/env python3 +# Copyright (c) 2015-2022 The Dash Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +''' +feature_dip3_v19.py + +Checks DIP3 for v19 + +''' +from io import BytesIO + +from test_framework.mininode import P2PInterface +from test_framework.messages import CBlock, CBlockHeader, CCbTx, CMerkleBlock, FromHex, hash256, msg_getmnlistd, \ + QuorumId, ser_uint256 +from test_framework.test_framework import DashTestFramework +from test_framework.util import ( + assert_equal, wait_until +) + + +class TestP2PConn(P2PInterface): + def __init__(self): + super().__init__() + self.last_mnlistdiff = None + + def on_mnlistdiff(self, message): + self.last_mnlistdiff = message + + def wait_for_mnlistdiff(self, timeout=30): + def received_mnlistdiff(): + return self.last_mnlistdiff is not None + return wait_until(received_mnlistdiff, timeout=timeout) + + def getmnlistdiff(self, base_block_hash, block_hash): + msg = msg_getmnlistd(base_block_hash, block_hash) + self.last_mnlistdiff = None + self.send_message(msg) + self.wait_for_mnlistdiff() + return self.last_mnlistdiff + + +class DIP3V19Test(DashTestFramework): + def set_test_params(self): + self.set_dash_test_params(6, 5, fast_dip3_enforcement=True) + + def run_test(self): + # Connect all nodes to node1 so that we always have the whole network connected + # Otherwise only masternode connections will be established between nodes, which won't propagate TXs/blocks + # Usually node0 is the one that does this, but in this test we isolate it multiple times + + self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn()) + null_hash = format(0, "064x") + + for i in range(len(self.nodes)): + if i != 0: + self.connect_nodes(i, 0) + + self.activate_dip8() + + self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) + self.wait_for_sporks_same() + + expected_updated = [mn.proTxHash for mn in self.mninfo] + b_0 = self.nodes[0].getbestblockhash() + self.test_getmnlistdiff(null_hash, b_0, {}, [], expected_updated) + + self.activate_v19(expected_activation_height=900) + self.log.info("Activated v19 at height:" + str(self.nodes[0].getblockcount())) + + self.move_to_next_cycle() + self.log.info("Cycle H height:" + str(self.nodes[0].getblockcount())) + self.move_to_next_cycle() + self.log.info("Cycle H+C height:" + str(self.nodes[0].getblockcount())) + self.move_to_next_cycle() + self.log.info("Cycle H+2C height:" + str(self.nodes[0].getblockcount())) + + self.mine_cycle_quorum(llmq_type_name='llmq_test_dip0024', llmq_type=103) + + revoke_protx = self.mninfo[-1].proTxHash + revoke_keyoperator = self.mninfo[-1].keyOperator + self.log.info(f"Trying to revoke proTx:{revoke_protx}") + self.test_revoke_protx(revoke_protx, revoke_keyoperator) + + self.mine_quorum(llmq_type_name='llmq_test', llmq_type=100) + + return + + def test_revoke_protx(self, revoke_protx, revoke_keyoperator): + funds_address = self.nodes[0].getnewaddress() + self.nodes[0].sendtoaddress(funds_address, 1) + self.nodes[0].generate(1) + self.sync_all(self.nodes) + + self.nodes[0].protx('revoke', revoke_protx, revoke_keyoperator, 1, funds_address) + self.nodes[0].generate(1) + self.sync_all(self.nodes) + self.log.info(f"Succesfully revoked={revoke_protx}") + for mn in self.mninfo: + if mn.proTxHash == revoke_protx: + self.mninfo.remove(mn) + return + + def test_getmnlistdiff(self, base_block_hash, block_hash, base_mn_list, expected_deleted, expected_updated): + d = self.test_getmnlistdiff_base(base_block_hash, block_hash) + + # Assert that the deletedMNs and mnList fields are what we expected + assert_equal(set(d.deletedMNs), set([int(e, 16) for e in expected_deleted])) + assert_equal(set([e.proRegTxHash for e in d.mnList]), set(int(e, 16) for e in expected_updated)) + + # Build a new list based on the old list and the info from the diff + new_mn_list = base_mn_list.copy() + for e in d.deletedMNs: + new_mn_list.pop(format(e, '064x')) + for e in d.mnList: + new_mn_list[format(e.proRegTxHash, '064x')] = e + + cbtx = CCbTx() + cbtx.deserialize(BytesIO(d.cbTx.vExtraPayload)) + + # Verify that the merkle root matches what we locally calculate + hashes = [] + for mn in sorted(new_mn_list.values(), key=lambda mn: ser_uint256(mn.proRegTxHash)): + hashes.append(hash256(mn.serialize())) + merkle_root = CBlock.get_merkle_root(hashes) + assert_equal(merkle_root, cbtx.merkleRootMNList) + + return new_mn_list + + def test_getmnlistdiff_base(self, base_block_hash, block_hash): + hexstr = self.nodes[0].getblockheader(block_hash, False) + header = FromHex(CBlockHeader(), hexstr) + + d = self.test_node.getmnlistdiff(int(base_block_hash, 16), int(block_hash, 16)) + assert_equal(d.baseBlockHash, int(base_block_hash, 16)) + assert_equal(d.blockHash, int(block_hash, 16)) + + # Check that the merkle proof is valid + proof = CMerkleBlock(header, d.merkleProof) + proof = proof.serialize().hex() + assert_equal(self.nodes[0].verifytxoutproof(proof), [d.cbTx.hash]) + + # Check if P2P messages match with RPCs + d2 = self.nodes[0].protx("diff", base_block_hash, block_hash) + assert_equal(d2["baseBlockHash"], base_block_hash) + assert_equal(d2["blockHash"], block_hash) + assert_equal(d2["cbTxMerkleTree"], d.merkleProof.serialize().hex()) + assert_equal(d2["cbTx"], d.cbTx.serialize().hex()) + assert_equal(set([int(e, 16) for e in d2["deletedMNs"]]), set(d.deletedMNs)) + assert_equal(set([int(e["proRegTxHash"], 16) for e in d2["mnList"]]), set([e.proRegTxHash for e in d.mnList])) + assert_equal(set([QuorumId(e["llmqType"], int(e["quorumHash"], 16)) for e in d2["deletedQuorums"]]), set(d.deletedQuorums)) + assert_equal(set([QuorumId(e["llmqType"], int(e["quorumHash"], 16)) for e in d2["newQuorums"]]), set([QuorumId(e.llmqType, e.quorumHash) for e in d.newQuorums])) + + return d + + +if __name__ == '__main__': + DIP3V19Test().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 474da55260..f2fac09566 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -106,6 +106,7 @@ BASE_SCRIPTS = [ 'wallet_dump.py', 'wallet_listtransactions.py', 'feature_multikeysporks.py', + 'feature_dip3_v19.py', 'feature_llmq_signing.py', # NOTE: needs dash_hash to pass 'feature_llmq_signing.py --spork21', # NOTE: needs dash_hash to pass 'feature_llmq_chainlocks.py', # NOTE: needs dash_hash to pass