Merge #6036: feat: skip governance checks for blocks below the best chainlock

18328279ec fix: force mnsync to skip gov obj sync on reconnection (UdjinM6)
08331bb950 fix: apply suggestions (UdjinM6)
3c3489d7a1 test: add test (UdjinM6)
41ab95dbf8 feat: skip governance checks for blocks below the best chainlock (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  A node can miss governance trigger sometimes and then it would stuck not being able to sync any further. This issue can be fixed manually by resetting sync status and reconsidering the "invalid" block. However, that's inconvenient. Also, what it does under the hood is it simply disables some parts of block validation. We could do that automagically and more precise if we would trust ChainLocks instead.

  ## What was done?
  Skip governance checks for blocks below the best known chainlock, add tests.

  ## How Has This Been Tested?
  Run tests.

  ## 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)_

ACKs for top commit:
  knst:
    utACK 18328279ec
  PastaPastaPasta:
    utACK 18328279ec

Tree-SHA512: 3cc4e2707e24b36c9f64502561667d0cb66eced7019db7941781ab1b84cfd267b3dab4684c71b059e074450ea76dc8e342744bffdd1ca1be6ccceb34b3580659

src/masternode/payments.cpp

@@ -181,7 +181,7 @@ CAmount PlatformShare(const CAmount reward)
 *   - Other blocks are 10% lower in outgoing value, so in total, no extra coins are created
 *   - When non-superblocks are detected, the normal schedule should be maintained
 */
-bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet)
+bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet, const bool check_superblock)
 {
     bool isBlockRewardValueMet = (block.vtx[0]->GetValueOut() <= blockReward);
 
@@ -242,6 +242,8 @@ bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlo
         return isBlockRewardValueMet;
     }
 
+    if (!check_superblock) return true;
+
     const auto tip_mn_list = m_dmnman.GetListAtChainTip();
 
     if (!CSuperblockManager::IsSuperblockTriggered(m_govman, tip_mn_list, nBlockHeight)) {
@@ -267,7 +269,7 @@ bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlo
     return true;
 }
 
-bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward)
+bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward, const bool check_superblock)
 {
     const int nBlockHeight = pindexPrev  == nullptr ? 0 : pindexPrev->nHeight + 1;
 
@@ -298,6 +300,7 @@ bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CB
     // superblocks started
 
     if (AreSuperblocksEnabled(m_sporkman)) {
+        if (!check_superblock) return true;
         const auto tip_mn_list = m_dmnman.GetListAtChainTip();
         if (CSuperblockManager::IsSuperblockTriggered(m_govman, tip_mn_list, nBlockHeight)) {
             if (CSuperblockManager::IsValid(m_govman, tip_mn_list, txNew, nBlockHeight, blockSubsidy + feeReward)) {

src/masternode/payments.h

@@ -52,8 +52,8 @@ public:
                                   const CMasternodeSync& mn_sync, const CSporkManager& sporkman) :
         m_dmnman{dmnman}, m_govman{govman}, m_consensus_params{consensus_params}, m_mn_sync{mn_sync}, m_sporkman{sporkman} {}
 
-    bool IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet);
+    bool IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet, const bool check_superblock);
-    bool IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward);
+    bool IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward, const bool check_superblock);
     void FillBlockPayments(CMutableTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward,
                            std::vector<CTxOut>& voutMasternodePaymentsRet, std::vector<CTxOut>& voutSuperblockPaymentsRet);
 };

src/validation.cpp

@@ -2445,7 +2445,9 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
     int64_t nTime5_3 = GetTimeMicros(); nTimeCreditPool += nTime5_3 - nTime5_2;
     LogPrint(BCLog::BENCHMARK, "      - CheckCreditPoolDiffForBlock: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_3 - nTime5_2), nTimeCreditPool * MICRO, nTimeCreditPool * MILLI / nBlocksTotal);
 
-    if (!m_chain_helper->mn_payments->IsBlockValueValid(block, pindex->nHeight, blockSubsidy + feeReward, strError)) {
+    const bool check_superblock = m_clhandler->GetBestChainLock().getHeight() < pindex->nHeight;
+
+    if (!m_chain_helper->mn_payments->IsBlockValueValid(block, pindex->nHeight, blockSubsidy + feeReward, strError, check_superblock)) {
         // NOTE: Do not punish, the node might be missing governance data
         LogPrintf("ERROR: ConnectBlock(DASH): %s\n", strError);
         return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-amount");
@@ -2454,7 +2456,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 (!m_chain_helper->mn_payments->IsBlockPayeeValid(*block.vtx[0], pindex->pprev, blockSubsidy, feeReward)) {
+    if (!m_chain_helper->mn_payments->IsBlockPayeeValid(*block.vtx[0], pindex->pprev, blockSubsidy, feeReward, check_superblock)) {
         // 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");

test/functional/feature_governance_cl.py

@@ -0,0 +1,144 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024 The Dash Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Tests governance checks can be skipped for blocks covered by the best chainlock."""
+
+import json
+
+from test_framework.messages import uint256_to_string
+from test_framework.test_framework import DashTestFramework
+from test_framework.util import assert_equal, force_finish_mnsync, satoshi_round
+
+class DashGovernanceTest (DashTestFramework):
+    def set_test_params(self):
+        self.set_dash_test_params(6, 5, [["-budgetparams=10:10:10"]] * 6, fast_dip3_enforcement=True)
+
+    def prepare_object(self, object_type, parent_hash, creation_time, revision, name, amount, payment_address):
+        proposal_rev = revision
+        proposal_time = int(creation_time)
+        proposal_template = {
+            "type": object_type,
+            "name": name,
+            "start_epoch": proposal_time,
+            "end_epoch": proposal_time + 24 * 60 * 60,
+            "payment_amount": float(amount),
+            "payment_address": payment_address,
+            "url": "https://dash.org"
+        }
+        proposal_hex = ''.join(format(x, '02x') for x in json.dumps(proposal_template).encode())
+        collateral_hash = self.nodes[0].gobject("prepare", parent_hash, proposal_rev, proposal_time, proposal_hex)
+        return {
+            "parentHash": parent_hash,
+            "collateralHash": collateral_hash,
+            "createdAt": proposal_time,
+            "revision": proposal_rev,
+            "hex": proposal_hex,
+            "data": proposal_template,
+        }
+
+    def have_trigger_for_height(self, sb_block_height, nodes = None):
+        if nodes is None:
+            nodes = self.nodes
+        count = 0
+        for node in nodes:
+            valid_triggers = node.gobject("list", "valid", "triggers")
+            for trigger in list(valid_triggers.values()):
+                if json.loads(trigger["DataString"])["event_block_height"] != sb_block_height:
+                    continue
+                if trigger['AbsoluteYesCount'] > 0:
+                    count = count + 1
+                    break
+        return count == len(nodes)
+
+    def run_test(self):
+        sb_cycle = 20
+
+        self.log.info("Make sure ChainLocks are active")
+
+        self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0)
+        self.wait_for_sporks_same()
+        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)
+
+        self.sync_blocks()
+        self.wait_for_chainlocked_block_all_nodes(self.nodes[0].getbestblockhash())
+
+        self.nodes[0].sporkupdate("SPORK_9_SUPERBLOCKS_ENABLED", 0)
+        self.wait_for_sporks_same()
+
+        self.log.info("Prepare and submit proposals")
+
+        proposal_time = self.mocktime
+        self.p0_payout_address = self.nodes[0].getnewaddress()
+        self.p1_payout_address = self.nodes[0].getnewaddress()
+        self.p0_amount = satoshi_round("1.1")
+        self.p1_amount = satoshi_round("3.3")
+
+        p0_collateral_prepare = self.prepare_object(1, uint256_to_string(0), proposal_time, 1, "Proposal_0", self.p0_amount, self.p0_payout_address)
+        p1_collateral_prepare = self.prepare_object(1, uint256_to_string(0), proposal_time, 1, "Proposal_1", self.p1_amount, self.p1_payout_address)
+        self.bump_mocktime(60 * 10 + 1)
+
+        self.nodes[0].generate(6)
+        self.bump_mocktime(6 * 156)
+        self.sync_blocks()
+
+        assert_equal(len(self.nodes[0].gobject("list-prepared")), 2)
+        assert_equal(len(self.nodes[0].gobject("list")), 0)
+
+        self.p0_hash = self.nodes[0].gobject("submit", "0", 1, proposal_time, p0_collateral_prepare["hex"], p0_collateral_prepare["collateralHash"])
+        self.p1_hash = self.nodes[0].gobject("submit", "0", 1, proposal_time, p1_collateral_prepare["hex"], p1_collateral_prepare["collateralHash"])
+
+        assert_equal(len(self.nodes[0].gobject("list")), 2)
+
+        self.log.info("Isolate a node so that it would miss votes and triggers")
+        # Isolate a node
+        self.isolate_node(5)
+
+        self.log.info("Vote proposals")
+
+        self.nodes[0].gobject("vote-many", self.p0_hash, "funding", "yes")
+        self.nodes[0].gobject("vote-many", self.p1_hash, "funding", "yes")
+        assert_equal(self.nodes[0].gobject("get", self.p0_hash)["FundingResult"]["YesCount"], self.mn_count)
+        assert_equal(self.nodes[0].gobject("get", self.p1_hash)["FundingResult"]["YesCount"], self.mn_count)
+
+        assert_equal(len(self.nodes[0].gobject("list", "valid", "triggers")), 0)
+
+        n = sb_cycle - self.nodes[0].getblockcount() % sb_cycle
+        assert n > 1
+
+        # Move remaining n blocks until the next Superblock
+        for _ in range(n - 1):
+            self.nodes[0].generate(1)
+            self.bump_mocktime(156)
+            self.sync_blocks(self.nodes[0:5])
+
+        self.log.info("Wait for new trigger and votes on non-isolated nodes")
+        sb_block_height = self.nodes[0].getblockcount() + 1
+        self.wait_until(lambda: self.have_trigger_for_height(sb_block_height, self.nodes[0:5]), timeout=5)
+        # Mine superblock
+        self.nodes[0].generate(1)
+        self.bump_mocktime(156)
+        self.sync_blocks(self.nodes[0:5])
+        self.wait_for_chainlocked_block(self.nodes[0], self.nodes[0].getbestblockhash())
+
+        self.log.info("Reconnect isolated node and confirm the next ChainLock will let it sync")
+        self.reconnect_isolated_node(5, 0)
+        # Force isolated node to be fully synced so that it would not request gov objects when reconnected
+        assert_equal(self.nodes[5].mnsync("status")["IsSynced"], False)
+        force_finish_mnsync(self.nodes[5])
+        self.nodes[0].generate(1)
+        self.bump_mocktime(156)
+        self.sync_blocks()
+
+
+if __name__ == '__main__':
+    DashGovernanceTest().main()

test/functional/test_runner.py

@@ -286,6 +286,7 @@ BASE_SCRIPTS = [
     'feature_new_quorum_type_activation.py',
     'feature_governance_objects.py',
     'feature_governance.py --legacy-wallet',
+    'feature_governance_cl.py --legacy-wallet',
     'rpc_uptime.py',
     'feature_discover.py',
     'wallet_resendwallettransactions.py --legacy-wallet',