fix: check HPMNs duplicate on tx broadcast (#5257)

<!--
*** Please remove the following help text before submitting: ***

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
  should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
  was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
  consider building the system outside of Dash Core, if possible.
-->

## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Before this fix, uniqueness of HPMN `platformNodeID` was checked only
while processing a block containing a `ProRegTx` or a `ProUpServTx`.
This is not enough as a `ProRegTx` or `ProUpServTx` containing duplicate
HPMN `platformNodeID` must be rejected at tx broadcast level.

## What was done?
<!--- Describe your changes in detail -->
Checking uniqueness when calling respective RPC and when receiving such
txs.

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->


## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->


## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [x] I have performed a self-review of my own code
- [ ] 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

src/evo/deterministicmns.cpp

@@ -1499,6 +1499,13 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
             return state.Invalid(ValidationInvalidReason::TX_BAD_SPECIAL, false, REJECT_DUPLICATE, "bad-protx-dup-key");
         }
 
+        // never allow duplicate platformNodeIds for HPMNs
+        if (ptx.nType == MnType::HighPerformance) {
+            if (mnList.HasUniqueProperty(ptx.platformNodeID)) {
+                return state.Invalid(ValidationInvalidReason::TX_BAD_SPECIAL, false, REJECT_DUPLICATE, "bad-protx-dup-platformnodeid");
+            }
+        }
+
         if (!deterministicMNManager->IsDIP3Enforced(pindexPrev->nHeight)) {
             if (ptx.keyIDOwner != ptx.keyIDVoting) {
                 return state.Invalid(ValidationInvalidReason::TX_BAD_SPECIAL, false, REJECT_INVALID, "bad-protx-key-not-same");
@@ -1564,6 +1571,13 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa
             return state.Invalid(ValidationInvalidReason::TX_BAD_SPECIAL, false, REJECT_DUPLICATE, "bad-protx-dup-addr");
         }
 
+        // don't allow updating to platformNodeIds already used by other HPMNs
+        if (ptx.nType == MnType::HighPerformance) {
+            if (mnList.HasUniqueProperty(ptx.platformNodeID)  && mnList.GetUniquePropertyMN(ptx.platformNodeID)->proTxHash != ptx.proTxHash) {
+                return state.Invalid(ValidationInvalidReason::TX_BAD_SPECIAL, false, REJECT_DUPLICATE, "bad-protx-dup-platformnodeid");
+            }
+        }
+
         if (ptx.scriptOperatorPayout != CScript()) {
             if (mn->nOperatorReward == 0) {
                 // don't allow setting operator reward payee in case no operatorReward was set

test/functional/feature_dip3_v19.py

@@ -43,7 +43,7 @@ class TestP2PConn(P2PInterface):
 
 class DIP3V19Test(DashTestFramework):
     def set_test_params(self):
-        self.set_dash_test_params(6, 5, fast_dip3_enforcement=True)
+        self.set_dash_test_params(6, 5, fast_dip3_enforcement=True, hpmn_count=2)
 
     def run_test(self):
         # Connect all nodes to node1 so that we always have the whole network connected
@@ -78,6 +78,23 @@ class DIP3V19Test(DashTestFramework):
 
         self.mine_cycle_quorum(llmq_type_name='llmq_test_dip0024', llmq_type=103)
 
+        hpmn_info_0 = self.dynamically_add_masternode(hpmn=True, rnd=7)
+        assert hpmn_info_0 is not None
+        self.nodes[0].generate(8)
+        self.sync_blocks(self.nodes)
+
+        self.log.info("Checking that protxs with duplicate HPMN fields are rejected")
+        hpmn_info_1 = self.dynamically_add_masternode(hpmn=True, rnd=7, should_be_rejected=True)
+        assert hpmn_info_1 is None
+        self.dynamically_hpmn_update_service(hpmn_info_0, 8)
+        hpmn_info_2 = self.dynamically_add_masternode(hpmn=True, rnd=8, should_be_rejected=True)
+        assert hpmn_info_2 is None
+        hpmn_info_3 = self.dynamically_add_masternode(hpmn=True, rnd=9)
+        assert hpmn_info_3 is not None
+        self.nodes[0].generate(8)
+        self.sync_blocks(self.nodes)
+        self.dynamically_hpmn_update_service(hpmn_info_0, 9, should_be_rejected=True)
+
         revoke_protx = self.mninfo[-1].proTxHash
         revoke_keyoperator = self.mninfo[-1].keyOperator
         self.log.info(f"Trying to revoke proTx:{revoke_protx}")

test/functional/feature_llmq_hpmn.py

@@ -10,11 +10,9 @@ Checks HPMNs
 
 '''
 from _decimal import Decimal
-import random
 from io import BytesIO
 
 from test_framework.mininode import P2PInterface
-from test_framework.script import hash160
 from test_framework.messages import CBlock, CBlockHeader, CCbTx, CMerkleBlock, FromHex, hash256, msg_getmnlistd, \
     QuorumId, ser_uint256
 from test_framework.test_framework import DashTestFramework
@@ -103,7 +101,7 @@ class LLMQHPMNTest(DashTestFramework):
             self.test_getmnlistdiff(null_hash, b_i, {}, [], expectedUpdated)
 
             self.test_masternode_count(expected_mns_count=4, expected_hpmns_count=i+1)
-            self.test_hpmn_update_service(hpmn_info)
+            self.dynamically_hpmn_update_service(hpmn_info)
 
         self.log.info("Test llmq_platform are formed only with HPMNs")
         for i in range(3):
@@ -235,25 +233,6 @@ class LLMQHPMNTest(DashTestFramework):
         assert_equal(detailed_count['regular']['total'], expected_mns_count)
         assert_equal(detailed_count['hpmn']['total'], expected_hpmns_count)
 
-    def test_hpmn_update_service(self, hpmn_info):
-        funds_address = self.nodes[0].getnewaddress()
-        operator_reward_address = self.nodes[0].getnewaddress()
-
-        # For the sake of the test, generate random nodeid, p2p and http platform values
-        rnd = random.randint(1000, 65000)
-        platform_node_id = hash160(b'%d' % rnd).hex()
-        platform_p2p_port = '%d' % (rnd + 1)
-        platform_http_port = '%d' % (rnd + 2)
-
-        self.nodes[0].sendtoaddress(funds_address, 1)
-        self.nodes[0].generate(1)
-        self.sync_all(self.nodes)
-
-        self.nodes[0].protx('update_service_hpmn', hpmn_info.proTxHash, hpmn_info.addr, hpmn_info.keyOperator, platform_node_id, platform_p2p_port, platform_http_port, operator_reward_address, funds_address)
-        self.nodes[0].generate(1)
-        self.sync_all(self.nodes)
-        self.log.info("Updated HPMN %s: platformNodeID=%s, platformP2PPort=%s, platformHTTPPort=%s" % (hpmn_info.proTxHash, platform_node_id, platform_p2p_port, platform_http_port))
-
     def test_getmnlistdiff(self, baseBlockHash, blockHash, baseMNList, expectedDeleted, expectedUpdated):
         d = self.test_getmnlistdiff_base(baseBlockHash, blockHash)
 

test/functional/test_framework/test_framework.py

@@ -1076,13 +1076,21 @@ class DashTestFramework(BitcoinTestFramework):
         for i in range(0, idx):
             self.connect_nodes(i, idx)
 
-    def dynamically_add_masternode(self, hpmn=False):
+    def dynamically_add_masternode(self, hpmn=False, rnd=None, should_be_rejected=False):
         mn_idx = len(self.nodes)
 
         node_p2p_port = p2p_port(mn_idx)
         node_rpc_port = rpc_port(mn_idx)
 
-        created_mn_info = self.dynamically_prepare_masternode(mn_idx, node_p2p_port, hpmn)
+        protx_success = False
+        try:
+            created_mn_info = self.dynamically_prepare_masternode(mn_idx, node_p2p_port, hpmn, rnd)
+            protx_success = True
+        except:
+            self.log.info("protx_hpmn rejected")
+        if should_be_rejected:
+            assert_equal(protx_success, False)
+            return
 
         self.dynamically_initialize_datadir(self.nodes[0].chain,node_p2p_port, node_rpc_port)
         node_info = self.add_dynamically_node(self.extra_args[1])
@@ -1104,7 +1112,7 @@ class DashTestFramework(BitcoinTestFramework):
         self.log.info("Successfully started and synced proTx:"+str(created_mn_info.proTxHash))
         return created_mn_info
 
-    def dynamically_prepare_masternode(self, idx, node_p2p_port, hpmn=False):
+    def dynamically_prepare_masternode(self, idx, node_p2p_port, hpmn=False, rnd=None):
         bls = self.nodes[0].bls('generate')
         collateral_address = self.nodes[0].getnewaddress()
         funds_address = self.nodes[0].getnewaddress()
@@ -1112,7 +1120,7 @@ class DashTestFramework(BitcoinTestFramework):
         voting_address = self.nodes[0].getnewaddress()
         reward_address = self.nodes[0].getnewaddress()
 
-        platform_node_id = hash160(b'%d' % node_p2p_port).hex()
+        platform_node_id = hash160(b'%d' % rnd).hex() if rnd is not None else hash160(b'%d' % node_p2p_port).hex()
         platform_p2p_port = '%d' % (node_p2p_port + 101) if hpmn else ''
         platform_http_port = '%d' % (node_p2p_port + 102) if hpmn else ''
 
@@ -1146,6 +1154,32 @@ class DashTestFramework(BitcoinTestFramework):
         self.log.info("Prepared %s %d: collateral_txid=%s, collateral_vout=%d, protxHash=%s" % (mn_type_str, idx, collateral_txid, collateral_vout, protx_result))
         return mn_info
 
+    def dynamically_hpmn_update_service(self, hpmn_info, rnd=None, should_be_rejected=False):
+        funds_address = self.nodes[0].getnewaddress()
+        operator_reward_address = self.nodes[0].getnewaddress()
+
+        # For the sake of the test, generate random nodeid, p2p and http platform values
+        r = rnd if rnd is not None else random.randint(1000, 65000)
+        platform_node_id = hash160(b'%d' % r).hex()
+        platform_p2p_port = '%d' % (r + 1)
+        platform_http_port = '%d' % (r + 2)
+
+        self.nodes[0].sendtoaddress(funds_address, 1)
+        self.nodes[0].generate(1)
+        self.sync_all(self.nodes)
+
+        protx_success = False
+        try:
+            self.nodes[0].protx('update_service_hpmn', hpmn_info.proTxHash, hpmn_info.addr, hpmn_info.keyOperator, platform_node_id, platform_p2p_port, platform_http_port, operator_reward_address, funds_address)
+            self.nodes[0].generate(1)
+            self.sync_all(self.nodes)
+            self.log.info("Updated HPMN %s: platformNodeID=%s, platformP2PPort=%s, platformHTTPPort=%s" % (hpmn_info.proTxHash, platform_node_id, platform_p2p_port, platform_http_port))
+            protx_success = True
+        except:
+            self.log.info("protx_hpmn rejected")
+        if should_be_rejected:
+            assert_equal(protx_success, False)
+
     def prepare_masternodes(self):
         self.log.info("Preparing %d masternodes" % self.mn_count)
         rewardsAddr = self.nodes[0].getnewaddress()