Skip to content

Commit 7ca4812

Browse files
Merge dashpay#6036: feat: skip governance checks for blocks below the best chainlock
1832827 fix: force mnsync to skip gov obj sync on reconnection (UdjinM6) 08331bb fix: apply suggestions (UdjinM6) 3c3489d test: add test (UdjinM6) 41ab95d 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 1832827 PastaPastaPasta: utACK 1832827 Tree-SHA512: 3cc4e2707e24b36c9f64502561667d0cb66eced7019db7941781ab1b84cfd267b3dab4684c71b059e074450ea76dc8e342744bffdd1ca1be6ccceb34b3580659
2 parents 91e9dd4 + 1832827 commit 7ca4812

File tree

5 files changed

+156
-6
lines changed

5 files changed

+156
-6
lines changed

src/masternode/payments.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ CAmount PlatformShare(const CAmount reward)
181181
* - Other blocks are 10% lower in outgoing value, so in total, no extra coins are created
182182
* - When non-superblocks are detected, the normal schedule should be maintained
183183
*/
184-
bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet)
184+
bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet, const bool check_superblock)
185185
{
186186
bool isBlockRewardValueMet = (block.vtx[0]->GetValueOut() <= blockReward);
187187

@@ -242,6 +242,8 @@ bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlo
242242
return isBlockRewardValueMet;
243243
}
244244

245+
if (!check_superblock) return true;
246+
245247
const auto tip_mn_list = m_dmnman.GetListAtChainTip();
246248

247249
if (!CSuperblockManager::IsSuperblockTriggered(m_govman, tip_mn_list, nBlockHeight)) {
@@ -267,7 +269,7 @@ bool CMNPaymentsProcessor::IsBlockValueValid(const CBlock& block, const int nBlo
267269
return true;
268270
}
269271

270-
bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward)
272+
bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward, const bool check_superblock)
271273
{
272274
const int nBlockHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;
273275

@@ -298,6 +300,7 @@ bool CMNPaymentsProcessor::IsBlockPayeeValid(const CTransaction& txNew, const CB
298300
// superblocks started
299301

300302
if (AreSuperblocksEnabled(m_sporkman)) {
303+
if (!check_superblock) return true;
301304
const auto tip_mn_list = m_dmnman.GetListAtChainTip();
302305
if (CSuperblockManager::IsSuperblockTriggered(m_govman, tip_mn_list, nBlockHeight)) {
303306
if (CSuperblockManager::IsValid(m_govman, tip_mn_list, txNew, nBlockHeight, blockSubsidy + feeReward)) {

src/masternode/payments.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ class CMNPaymentsProcessor
5252
const CMasternodeSync& mn_sync, const CSporkManager& sporkman) :
5353
m_dmnman{dmnman}, m_govman{govman}, m_consensus_params{consensus_params}, m_mn_sync{mn_sync}, m_sporkman{sporkman} {}
5454

55-
bool IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet);
56-
bool IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward);
55+
bool IsBlockValueValid(const CBlock& block, const int nBlockHeight, const CAmount blockReward, std::string& strErrorRet, const bool check_superblock);
56+
bool IsBlockPayeeValid(const CTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward, const bool check_superblock);
5757
void FillBlockPayments(CMutableTransaction& txNew, const CBlockIndex* pindexPrev, const CAmount blockSubsidy, const CAmount feeReward,
5858
std::vector<CTxOut>& voutMasternodePaymentsRet, std::vector<CTxOut>& voutSuperblockPaymentsRet);
5959
};

src/validation.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2445,7 +2445,9 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
24452445
int64_t nTime5_3 = GetTimeMicros(); nTimeCreditPool += nTime5_3 - nTime5_2;
24462446
LogPrint(BCLog::BENCHMARK, " - CheckCreditPoolDiffForBlock: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_3 - nTime5_2), nTimeCreditPool * MICRO, nTimeCreditPool * MILLI / nBlocksTotal);
24472447

2448-
if (!m_chain_helper->mn_payments->IsBlockValueValid(block, pindex->nHeight, blockSubsidy + feeReward, strError)) {
2448+
const bool check_superblock = m_clhandler->GetBestChainLock().getHeight() < pindex->nHeight;
2449+
2450+
if (!m_chain_helper->mn_payments->IsBlockValueValid(block, pindex->nHeight, blockSubsidy + feeReward, strError, check_superblock)) {
24492451
// NOTE: Do not punish, the node might be missing governance data
24502452
LogPrintf("ERROR: ConnectBlock(DASH): %s\n", strError);
24512453
return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-amount");
@@ -2454,7 +2456,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
24542456
int64_t nTime5_4 = GetTimeMicros(); nTimeValueValid += nTime5_4 - nTime5_3;
24552457
LogPrint(BCLog::BENCHMARK, " - IsBlockValueValid: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_4 - nTime5_3), nTimeValueValid * MICRO, nTimeValueValid * MILLI / nBlocksTotal);
24562458

2457-
if (!m_chain_helper->mn_payments->IsBlockPayeeValid(*block.vtx[0], pindex->pprev, blockSubsidy, feeReward)) {
2459+
if (!m_chain_helper->mn_payments->IsBlockPayeeValid(*block.vtx[0], pindex->pprev, blockSubsidy, feeReward, check_superblock)) {
24582460
// NOTE: Do not punish, the node might be missing governance data
24592461
LogPrintf("ERROR: ConnectBlock(DASH): couldn't find masternode or superblock payments\n");
24602462
return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-payee");
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
#!/usr/bin/env python3
2+
# Copyright (c) 2024 The Dash Core developers
3+
# Distributed under the MIT software license, see the accompanying
4+
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
"""Tests governance checks can be skipped for blocks covered by the best chainlock."""
6+
7+
import json
8+
9+
from test_framework.messages import uint256_to_string
10+
from test_framework.test_framework import DashTestFramework
11+
from test_framework.util import assert_equal, force_finish_mnsync, satoshi_round
12+
13+
class DashGovernanceTest (DashTestFramework):
14+
def set_test_params(self):
15+
self.set_dash_test_params(6, 5, [["-budgetparams=10:10:10"]] * 6, fast_dip3_enforcement=True)
16+
17+
def prepare_object(self, object_type, parent_hash, creation_time, revision, name, amount, payment_address):
18+
proposal_rev = revision
19+
proposal_time = int(creation_time)
20+
proposal_template = {
21+
"type": object_type,
22+
"name": name,
23+
"start_epoch": proposal_time,
24+
"end_epoch": proposal_time + 24 * 60 * 60,
25+
"payment_amount": float(amount),
26+
"payment_address": payment_address,
27+
"url": "https://dash.org"
28+
}
29+
proposal_hex = ''.join(format(x, '02x') for x in json.dumps(proposal_template).encode())
30+
collateral_hash = self.nodes[0].gobject("prepare", parent_hash, proposal_rev, proposal_time, proposal_hex)
31+
return {
32+
"parentHash": parent_hash,
33+
"collateralHash": collateral_hash,
34+
"createdAt": proposal_time,
35+
"revision": proposal_rev,
36+
"hex": proposal_hex,
37+
"data": proposal_template,
38+
}
39+
40+
def have_trigger_for_height(self, sb_block_height, nodes = None):
41+
if nodes is None:
42+
nodes = self.nodes
43+
count = 0
44+
for node in nodes:
45+
valid_triggers = node.gobject("list", "valid", "triggers")
46+
for trigger in list(valid_triggers.values()):
47+
if json.loads(trigger["DataString"])["event_block_height"] != sb_block_height:
48+
continue
49+
if trigger['AbsoluteYesCount'] > 0:
50+
count = count + 1
51+
break
52+
return count == len(nodes)
53+
54+
def run_test(self):
55+
sb_cycle = 20
56+
57+
self.log.info("Make sure ChainLocks are active")
58+
59+
self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0)
60+
self.wait_for_sporks_same()
61+
self.activate_v19(expected_activation_height=900)
62+
self.log.info("Activated v19 at height:" + str(self.nodes[0].getblockcount()))
63+
self.move_to_next_cycle()
64+
self.log.info("Cycle H height:" + str(self.nodes[0].getblockcount()))
65+
self.move_to_next_cycle()
66+
self.log.info("Cycle H+C height:" + str(self.nodes[0].getblockcount()))
67+
self.move_to_next_cycle()
68+
self.log.info("Cycle H+2C height:" + str(self.nodes[0].getblockcount()))
69+
70+
self.mine_cycle_quorum(llmq_type_name='llmq_test_dip0024', llmq_type=103)
71+
72+
self.sync_blocks()
73+
self.wait_for_chainlocked_block_all_nodes(self.nodes[0].getbestblockhash())
74+
75+
self.nodes[0].sporkupdate("SPORK_9_SUPERBLOCKS_ENABLED", 0)
76+
self.wait_for_sporks_same()
77+
78+
self.log.info("Prepare and submit proposals")
79+
80+
proposal_time = self.mocktime
81+
self.p0_payout_address = self.nodes[0].getnewaddress()
82+
self.p1_payout_address = self.nodes[0].getnewaddress()
83+
self.p0_amount = satoshi_round("1.1")
84+
self.p1_amount = satoshi_round("3.3")
85+
86+
p0_collateral_prepare = self.prepare_object(1, uint256_to_string(0), proposal_time, 1, "Proposal_0", self.p0_amount, self.p0_payout_address)
87+
p1_collateral_prepare = self.prepare_object(1, uint256_to_string(0), proposal_time, 1, "Proposal_1", self.p1_amount, self.p1_payout_address)
88+
self.bump_mocktime(60 * 10 + 1)
89+
90+
self.nodes[0].generate(6)
91+
self.bump_mocktime(6 * 156)
92+
self.sync_blocks()
93+
94+
assert_equal(len(self.nodes[0].gobject("list-prepared")), 2)
95+
assert_equal(len(self.nodes[0].gobject("list")), 0)
96+
97+
self.p0_hash = self.nodes[0].gobject("submit", "0", 1, proposal_time, p0_collateral_prepare["hex"], p0_collateral_prepare["collateralHash"])
98+
self.p1_hash = self.nodes[0].gobject("submit", "0", 1, proposal_time, p1_collateral_prepare["hex"], p1_collateral_prepare["collateralHash"])
99+
100+
assert_equal(len(self.nodes[0].gobject("list")), 2)
101+
102+
self.log.info("Isolate a node so that it would miss votes and triggers")
103+
# Isolate a node
104+
self.isolate_node(5)
105+
106+
self.log.info("Vote proposals")
107+
108+
self.nodes[0].gobject("vote-many", self.p0_hash, "funding", "yes")
109+
self.nodes[0].gobject("vote-many", self.p1_hash, "funding", "yes")
110+
assert_equal(self.nodes[0].gobject("get", self.p0_hash)["FundingResult"]["YesCount"], self.mn_count)
111+
assert_equal(self.nodes[0].gobject("get", self.p1_hash)["FundingResult"]["YesCount"], self.mn_count)
112+
113+
assert_equal(len(self.nodes[0].gobject("list", "valid", "triggers")), 0)
114+
115+
n = sb_cycle - self.nodes[0].getblockcount() % sb_cycle
116+
assert n > 1
117+
118+
# Move remaining n blocks until the next Superblock
119+
for _ in range(n - 1):
120+
self.nodes[0].generate(1)
121+
self.bump_mocktime(156)
122+
self.sync_blocks(self.nodes[0:5])
123+
124+
self.log.info("Wait for new trigger and votes on non-isolated nodes")
125+
sb_block_height = self.nodes[0].getblockcount() + 1
126+
self.wait_until(lambda: self.have_trigger_for_height(sb_block_height, self.nodes[0:5]), timeout=5)
127+
# Mine superblock
128+
self.nodes[0].generate(1)
129+
self.bump_mocktime(156)
130+
self.sync_blocks(self.nodes[0:5])
131+
self.wait_for_chainlocked_block(self.nodes[0], self.nodes[0].getbestblockhash())
132+
133+
self.log.info("Reconnect isolated node and confirm the next ChainLock will let it sync")
134+
self.reconnect_isolated_node(5, 0)
135+
# Force isolated node to be fully synced so that it would not request gov objects when reconnected
136+
assert_equal(self.nodes[5].mnsync("status")["IsSynced"], False)
137+
force_finish_mnsync(self.nodes[5])
138+
self.nodes[0].generate(1)
139+
self.bump_mocktime(156)
140+
self.sync_blocks()
141+
142+
143+
if __name__ == '__main__':
144+
DashGovernanceTest().main()

test/functional/test_runner.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@
286286
'feature_new_quorum_type_activation.py',
287287
'feature_governance_objects.py',
288288
'feature_governance.py --legacy-wallet',
289+
'feature_governance_cl.py --legacy-wallet',
289290
'rpc_uptime.py',
290291
'feature_discover.py',
291292
'wallet_resendwallettransactions.py --legacy-wallet',

0 commit comments

Comments
 (0)