fix: some fixes for block payee validation and corresponding tests (dashpay#5684)

UdjinM6 · web-flow · commit 704c5942379a · 2023-11-13T10:02:52.000-06:00
## Issue being fixed or feature implemented
1. we _should not_ skip masternode payments checks below
nSuperblockStartBlock or when governance is disabled
2. we _should_ skip superblock payee checks while we aren't synced yet
(should help recovering from missed triggers)

## What was done?
pls see individual commits. 

## How Has This Been Tested?
run tests, sync w/ and w/out `--disablegovernance`, reindexed on testnet

## Breaking Changes
n/a

## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
diff --git a/src/masternode/payments.cpp b/src/masternode/payments.cpp
@@ -261,35 +261,40 @@ bool IsBlockValueValid(const CSporkManager& sporkManager, CGovernanceManager& go
     return true;
 }
 
-bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
+bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager, const CMasternodeSync& mn_sync,
                        const CTransaction& txNew, const CBlockIndex* const pindexPrev, const CAmount blockSubsidy, const CAmount feeReward)
 {
-    if(fDisableGovernance) {
-        //there is no budget data to use to check anything, let's just accept the longest chain
-        LogPrint(BCLog::MNPAYMENTS, "%s -- WARNING: Not enough data, skipping block payee checks\n", __func__);
-        return true;
-    }
-
     const int nBlockHeight = pindexPrev  == nullptr ? 0 : pindexPrev->nHeight + 1;
-    // we are still using budgets, but we have no data about them anymore,
-    // we can only check masternode payments
 
-    const Consensus::Params& consensusParams = Params().GetConsensus();
+    // Check for correct masternode payment
+    if (IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward)) {
+        LogPrint(BCLog::MNPAYMENTS, "%s -- Valid masternode payment at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */
+    } else {
+        LogPrintf("%s -- ERROR: Invalid masternode payment detected at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */
+        return false;
+    }
 
-    if(nBlockHeight < consensusParams.nSuperblockStartBlock) {
+    if (!mn_sync.IsSynced() || fDisableGovernance) {
+        // governance data is either incomplete or non-existent
+        LogPrint(BCLog::MNPAYMENTS, "%s -- WARNING: Not enough data, skipping superblock payee checks\n", __func__);
+        return true;  // not an error
+    }
+
+    if (nBlockHeight < Params().GetConsensus().nSuperblockStartBlock) {
+        // We are still using budgets, but we have no data about them anymore,
+        // we can only check masternode payments.
         // NOTE: old budget system is disabled since 12.1 and we should never enter this branch
         // anymore when sync is finished (on mainnet). We have no old budget data but these blocks
         // have tons of confirmations and can be safely accepted without payee verification
         LogPrint(BCLog::GOBJECT, "%s -- WARNING: Client synced but old budget system is disabled, accepting any payee\n", __func__);
-        return true;
+        return true; // not an error
     }
 
     // superblocks started
-    // SEE IF THIS IS A VALID SUPERBLOCK
 
-    if(AreSuperblocksEnabled(sporkManager)) {
-        if(CSuperblockManager::IsSuperblockTriggered(governanceManager, nBlockHeight)) {
-            if(CSuperblockManager::IsValid(governanceManager, txNew, nBlockHeight, blockSubsidy + feeReward)) {
+    if (AreSuperblocksEnabled(sporkManager)) {
+        if (CSuperblockManager::IsSuperblockTriggered(governanceManager, nBlockHeight)) {
+            if (CSuperblockManager::IsValid(governanceManager, txNew, nBlockHeight, blockSubsidy + feeReward)) {
                 LogPrint(BCLog::GOBJECT, "%s -- Valid superblock at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */
                 // continue validation, should also pay MN
             } else {
@@ -305,14 +310,7 @@ bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& go
         LogPrint(BCLog::GOBJECT, "%s -- Superblocks are disabled, no superblocks allowed\n", __func__);
     }
 
-    // Check for correct masternode payment
-    if(IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward)) {
-        LogPrint(BCLog::MNPAYMENTS, "%s -- Valid masternode payment at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */
-        return true;
-    }
-
-    LogPrintf("%s -- ERROR: Invalid masternode payment detected at height %d: %s", __func__, nBlockHeight, txNew.ToString()); /* Continued */
-    return false;
+    return true;
 }
 
 void FillBlockPayments(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
diff --git a/src/masternode/payments.h b/src/masternode/payments.h
@@ -28,7 +28,7 @@ namespace MasternodePayments
 {
 bool IsBlockValueValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager, const CMasternodeSync& mn_sync,
                        const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet);
-bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
+bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager, const CMasternodeSync& mn_sync,
                        const CTransaction& txNew, const CBlockIndex* const pindexPrev, const CAmount blockSubsidy, const CAmount feeReward);
 void FillBlockPayments(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
                        CMutableTransaction& txNew, const CBlockIndex* const pindexPrev, const CAmount blockSubsidy, const CAmount feeReward,
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -2416,7 +2416,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
     int64_t nTime5_4 = GetTimeMicros(); nTimeValueValid += nTime5_4 - nTime5_3;
     LogPrint(BCLog::BENCHMARK, "      - IsBlockValueValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_4 - nTime5_3), nTimeValueValid * MICRO, nTimeValueValid * MILLI / nBlocksTotal);
 
-    if (!MasternodePayments::IsBlockPayeeValid(*sporkManager, *governance, *block.vtx[0], pindex->pprev, blockSubsidy, feeReward)) {
+    if (!MasternodePayments::IsBlockPayeeValid(*sporkManager, *governance, *::masternodeSync, *block.vtx[0], pindex->pprev, blockSubsidy, feeReward)) {
         // NOTE: Do not punish, the node might be missing governance data
         LogPrintf("ERROR: ConnectBlock(DASH): couldn't find masternode or superblock payments\n");
         return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-payee");
diff --git a/test/functional/feature_dip3_deterministicmns.py b/test/functional/feature_dip3_deterministicmns.py
@@ -9,8 +9,8 @@
 
 from decimal import Decimal
 
-from test_framework.blocktools import create_block, create_coinbase, get_masternode_payment
-from test_framework.messages import CCbTx, COIN, CTransaction, FromHex, ToHex, uint256_to_string
+from test_framework.blocktools import create_block_with_mnpayments
+from test_framework.messages import CTransaction, FromHex, ToHex
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, force_finish_mnsync, p2p_port
 
@@ -131,15 +131,15 @@ def run_test(self):
             self.assert_mnlist(self.nodes[0], mns_tmp)
 
         self.log.info("cause a reorg with a double spend and check that mnlists are still correct on all nodes")
-        self.mine_double_spend(self.nodes[0], dummy_txins, self.nodes[0].getnewaddress(), use_mnmerkleroot_from_tip=True)
+        self.mine_double_spend(mns, self.nodes[0], dummy_txins, self.nodes[0].getnewaddress())
         self.nodes[0].generate(spend_mns_count)
         self.sync_all()
         self.assert_mnlists(mns_tmp)
 
         self.log.info("test mn payment enforcement with deterministic MNs")
         for i in range(20):
             node = self.nodes[i % len(self.nodes)]
-            self.test_invalid_mn_payment(node)
+            self.test_invalid_mn_payment(mns, node)
             self.nodes[0].generate(1)
             self.sync_all()
 
@@ -218,6 +218,7 @@ def prepare_mn(self, node, idx, alias):
         mn.idx = idx
         mn.alias = alias
         mn.p2p_port = p2p_port(mn.idx)
+        mn.operator_reward = (mn.idx % self.num_initial_mn)
 
         blsKey = node.bls('generate')
         mn.fundsAddr = node.getnewaddress()
@@ -247,7 +248,7 @@ def register_fund_mn(self, node, mn):
         mn.collateral_address = node.getnewaddress()
         mn.rewards_address = node.getnewaddress()
 
-        mn.protx_hash = node.protx('register_fund', mn.collateral_address, '127.0.0.1:%d' % mn.p2p_port, mn.ownerAddr, mn.operatorAddr, mn.votingAddr, 0, mn.rewards_address, mn.fundsAddr)
+        mn.protx_hash = node.protx('register_fund', mn.collateral_address, '127.0.0.1:%d' % mn.p2p_port, mn.ownerAddr, mn.operatorAddr, mn.votingAddr, mn.operator_reward, mn.rewards_address, mn.fundsAddr)
         mn.collateral_txid = mn.protx_hash
         mn.collateral_vout = None
 
@@ -263,7 +264,7 @@ def register_mn(self, node, mn):
         node.sendtoaddress(mn.fundsAddr, 0.001)
         mn.rewards_address = node.getnewaddress()
 
-        mn.protx_hash = node.protx('register', mn.collateral_txid, mn.collateral_vout, '127.0.0.1:%d' % mn.p2p_port, mn.ownerAddr, mn.operatorAddr, mn.votingAddr, 0, mn.rewards_address, mn.fundsAddr)
+        mn.protx_hash = node.protx('register', mn.collateral_txid, mn.collateral_vout, '127.0.0.1:%d' % mn.p2p_port, mn.ownerAddr, mn.operatorAddr, mn.votingAddr, mn.operator_reward, mn.rewards_address, mn.fundsAddr)
         node.generate(1)
 
     def start_mn(self, mn):
@@ -353,93 +354,15 @@ def spend_input(self, txid, vout, amount, with_dummy_input_output=False):
 
         return dummy_txin
 
-    def mine_block(self, node, vtx=None, miner_address=None, mn_payee=None, mn_amount=None, use_mnmerkleroot_from_tip=False, expected_error=None):
-        if vtx is None:
-            vtx = []
-        bt = node.getblocktemplate()
-        height = bt['height']
-        tip_hash = bt['previousblockhash']
-
-        tip_block = node.getblock(tip_hash)
-
-        coinbasevalue = bt['coinbasevalue']
-        if miner_address is None:
-            miner_address = self.nodes[0].getnewaddress()
-        if mn_payee is None:
-            if isinstance(bt['masternode'], list):
-                mn_payee = bt['masternode'][0]['payee']
-            else:
-                mn_payee = bt['masternode']['payee']
-        # we can't take the masternode payee amount from the template here as we might have additional fees in vtx
-
-        # calculate fees that the block template included (we'll have to remove it from the coinbase as we won't
-        # include the template's transactions
-        bt_fees = 0
-        for tx in bt['transactions']:
-            bt_fees += tx['fee']
-
-        new_fees = 0
-        for tx in vtx:
-            in_value = 0
-            out_value = 0
-            for txin in tx.vin:
-                txout = node.gettxout(uint256_to_string(txin.prevout.hash), txin.prevout.n, False)
-                in_value += int(txout['value'] * COIN)
-            for txout in tx.vout:
-                out_value += txout.nValue
-            new_fees += in_value - out_value
-
-        # fix fees
-        coinbasevalue -= bt_fees
-        coinbasevalue += new_fees
-
-        if mn_amount is None:
-            realloc_info = node.getblockchaininfo()['softforks']['realloc']
-            realloc_height = 99999999
-            if realloc_info['active']:
-                realloc_height = realloc_info['height']
-            mn_amount = get_masternode_payment(height, coinbasevalue, realloc_height)
-        miner_amount = coinbasevalue - mn_amount
-
-        outputs = {miner_address: str(Decimal(miner_amount) / COIN)}
-        if mn_amount > 0:
-            outputs[mn_payee] = str(Decimal(mn_amount) / COIN)
-
-        coinbase = FromHex(CTransaction(), node.createrawtransaction([], outputs))
-        coinbase.vin = create_coinbase(height).vin
-
-        # We can't really use this one as it would result in invalid merkle roots for masternode lists
-        if len(bt['coinbase_payload']) != 0:
-            cbtx = FromHex(CCbTx(version=1), bt['coinbase_payload'])
-            if use_mnmerkleroot_from_tip:
-                if 'cbTx' in tip_block:
-                    cbtx.merkleRootMNList = int(tip_block['cbTx']['merkleRootMNList'], 16)
-                else:
-                    cbtx.merkleRootMNList = 0
-            coinbase.nVersion = 3
-            coinbase.nType = 5 # CbTx
-            coinbase.vExtraPayload = cbtx.serialize()
-
-        coinbase.calc_sha256()
-
-        block = create_block(int(tip_hash, 16), coinbase)
-        block.vtx += vtx
-
-        # Add quorum commitments from template
-        for tx in bt['transactions']:
-            tx2 = FromHex(CTransaction(), tx['data'])
-            if tx2.nType == 6:
-                block.vtx.append(tx2)
-
-        block.hashMerkleRoot = block.calc_merkle_root()
-        block.solve()
+    def mine_block(self, mns, node, vtx=None, mn_payee=None, mn_amount=None, expected_error=None):
+        block = create_block_with_mnpayments(mns, node, vtx, mn_payee, mn_amount)
         result = node.submitblock(ToHex(block))
         if expected_error is not None and result != expected_error:
             raise AssertionError('mining the block should have failed with error %s, but submitblock returned %s' % (expected_error, result))
         elif expected_error is None and result is not None:
             raise AssertionError('submitblock returned %s' % (result))
 
-    def mine_double_spend(self, node, txins, target_address, use_mnmerkleroot_from_tip=False):
+    def mine_double_spend(self, mns, node, txins, target_address):
         amount = Decimal(0)
         for txin in txins:
             txout = node.gettxout(txin['txid'], txin['vout'], False)
@@ -450,12 +373,12 @@ def mine_double_spend(self, node, txins, target_address, use_mnmerkleroot_from_t
         rawtx = node.signrawtransactionwithwallet(rawtx)['hex']
         tx = FromHex(CTransaction(), rawtx)
 
-        self.mine_block(node, [tx], use_mnmerkleroot_from_tip=use_mnmerkleroot_from_tip)
+        self.mine_block(mns, node, [tx])
 
-    def test_invalid_mn_payment(self, node):
+    def test_invalid_mn_payment(self, mns, node):
         mn_payee = self.nodes[0].getnewaddress()
-        self.mine_block(node, mn_payee=mn_payee, expected_error='bad-cb-payee')
-        self.mine_block(node, mn_amount=1, expected_error='bad-cb-payee')
+        self.mine_block(mns, node, mn_payee=mn_payee, expected_error='bad-cb-payee')
+        self.mine_block(mns, node, mn_amount=1, expected_error='bad-cb-payee')
 
 if __name__ == '__main__':
     DIP3Test().main()
diff --git a/test/functional/feature_llmq_is_cl_conflicts.py b/test/functional/feature_llmq_is_cl_conflicts.py
@@ -10,11 +10,10 @@
 
 '''
 
-from decimal import Decimal
 import struct
 
-from test_framework.blocktools import get_masternode_payment, create_coinbase, create_block
-from test_framework.messages import CCbTx, CInv, COIN, CTransaction, FromHex, hash256, msg_clsig, msg_inv, ser_string, ToHex, uint256_from_str, uint256_to_string
+from test_framework.blocktools import create_block_with_mnpayments
+from test_framework.messages import CInv, CTransaction, FromHex, hash256, msg_clsig, msg_inv, ser_string, ToHex, uint256_from_str
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import DashTestFramework
 from test_framework.util import assert_equal, assert_raises_rpc_error, hex_str_to_bytes, wait_until
@@ -112,7 +111,7 @@ def test_chainlock_overrides_islock(self, test_block_conflict, mine_confllicting
             self.wait_for_instantlock(rawtx1_txid, node)
             self.wait_for_instantlock(rawtx4_txid, node)
 
-        block = self.create_block(self.nodes[0], [rawtx2_obj])
+        block = create_block_with_mnpayments(self.mninfo, self.nodes[0], [rawtx2_obj])
         if test_block_conflict:
             # The block shouldn't be accepted/connected but it should be known to node 0 now
             submit_result = self.nodes[0].submitblock(ToHex(block))
@@ -234,7 +233,7 @@ def test_chainlock_overrides_islock_overrides_nonchainlock(self, deterministic):
         assert_raises_rpc_error(-25, "bad-txns-inputs-missingorspent", self.nodes[0].sendrawtransaction, rawtx2)
 
         # Create the block and the corresponding clsig but do not relay clsig yet
-        cl_block = self.create_block(self.nodes[0])
+        cl_block = create_block_with_mnpayments(self.mninfo, self.nodes[0])
         cl = self.create_chainlock(self.nodes[0].getblockcount() + 1, cl_block)
         self.nodes[0].submitblock(ToHex(cl_block))
         self.sync_all()
@@ -275,74 +274,6 @@ def test_chainlock_overrides_islock_overrides_nonchainlock(self, deterministic):
             # Previous tip should be marked as conflicting now
             assert_equal(node.getchaintips(2)[1]["status"], "conflicting")
 
-    def create_block(self, node, vtx=None):
-        if vtx is None:
-            vtx = []
-        bt = node.getblocktemplate()
-        height = bt['height']
-        tip_hash = bt['previousblockhash']
-
-        coinbasevalue = bt['coinbasevalue']
-        miner_address = node.getnewaddress()
-        mn_payee = bt['masternode'][0]['payee']
-
-        # calculate fees that the block template included (we'll have to remove it from the coinbase as we won't
-        # include the template's transactions
-        bt_fees = 0
-        for tx in bt['transactions']:
-            bt_fees += tx['fee']
-
-        new_fees = 0
-        for tx in vtx:
-            in_value = 0
-            out_value = 0
-            for txin in tx.vin:
-                txout = node.gettxout(uint256_to_string(txin.prevout.hash), txin.prevout.n, False)
-                in_value += int(txout['value'] * COIN)
-            for txout in tx.vout:
-                out_value += txout.nValue
-            new_fees += in_value - out_value
-
-        # fix fees
-        coinbasevalue -= bt_fees
-        coinbasevalue += new_fees
-
-        realloc_info = self.nodes[0].getblockchaininfo()['softforks']['realloc']
-        realloc_height = 99999999
-        if realloc_info['active']:
-            realloc_height = realloc_info['height']
-        mn_amount = get_masternode_payment(height, coinbasevalue, realloc_height)
-        miner_amount = coinbasevalue - mn_amount
-
-        outputs = {miner_address: str(Decimal(miner_amount) / COIN)}
-        if mn_amount > 0:
-            outputs[mn_payee] = str(Decimal(mn_amount) / COIN)
-
-        coinbase = FromHex(CTransaction(), node.createrawtransaction([], outputs))
-        coinbase.vin = create_coinbase(height).vin
-
-        # We can't really use this one as it would result in invalid merkle roots for masternode lists
-        if len(bt['coinbase_payload']) != 0:
-            cbtx = FromHex(CCbTx(version=1), bt['coinbase_payload'])
-            coinbase.nVersion = 3
-            coinbase.nType = 5 # CbTx
-            coinbase.vExtraPayload = cbtx.serialize()
-
-        coinbase.calc_sha256()
-
-        block = create_block(int(tip_hash, 16), coinbase, ntime=bt['curtime'], version=bt['version'])
-        block.vtx += vtx
-
-        # Add quorum commitments from template
-        for tx in bt['transactions']:
-            tx2 = FromHex(CTransaction(), tx['data'])
-            if tx2.nType == 6:
-                block.vtx.append(tx2)
-
-        block.hashMerkleRoot = block.calc_merkle_root()
-        block.solve()
-        return block
-
     def create_chainlock(self, height, block):
         request_id_buf = ser_string(b"clsig") + struct.pack("<I", height)
         request_id = hash256(request_id_buf)[::-1].hex()
diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ namespace MasternodePayments`
`28`	`28`	`{`
`29`	`29`	`bool IsBlockValueValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager, const CMasternodeSync& mn_sync,`
`30`	`30`	`const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet);`
`31`		`-bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,`
	`31`	`+bool IsBlockPayeeValid(const CSporkManager& sporkManager, CGovernanceManager& governanceManager, const CMasternodeSync& mn_sync,`
`32`	`32`	`const CTransaction& txNew, const CBlockIndex* const pindexPrev, const CAmount blockSubsidy, const CAmount feeReward);`
`33`	`33`	`void FillBlockPayments(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,`
`34`	`34`	`CMutableTransaction& txNew, const CBlockIndex* const pindexPrev, const CAmount blockSubsidy, const CAmount feeReward,`