From 78d91b722100d8f7eb2b2fda6dd8b2520f423f9c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 2 Nov 2023 21:53:50 +0300 Subject: [PATCH 1/6] fix: actually vote `no` on triggers we don't like --- src/governance/governance.cpp | 123 +++++++++++++++++++++------------- src/governance/governance.h | 5 +- 2 files changed, 78 insertions(+), 50 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 3e42b6a09965c..d07ca77a9b6fb 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -554,7 +554,7 @@ struct sortProposalsByVotes { } }; -std::optional CGovernanceManager::CreateSuperblockCandidate(int nHeight) const +const std::optional CGovernanceManager::CreateSuperblockCandidate(int nHeight) const { if (!fMasternodeMode || fDisableGovernance) return std::nullopt; if (::masternodeSync == nullptr || !::masternodeSync->IsSynced()) return std::nullopt; @@ -663,63 +663,91 @@ std::optional CGovernanceManager::CreateSuperblockCandidate(int nHe return CSuperblock(nNextSuperblock, std::move(payments)); } -void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman) +const std::optional CGovernanceManager::CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman) { + // no sb_opt, no trigger + if (!sb_opt.has_value()) return std::nullopt; + //TODO: Check if nHashParentIn, nRevision and nCollateralHashIn are correct LOCK2(cs_main, cs); // Check if identical trigger (equal DataHash()) is already created (signed by other masternode) - CGovernanceObject* gov_sb_voting{nullptr}; - CGovernanceObject gov_sb(uint256(), 1, GetAdjustedTime(), uint256(), sb.GetHexStrData()); - const auto identical_sb = FindGovernanceObjectByDataHash(gov_sb.GetDataHash()); - - if (identical_sb == nullptr) { - // Nobody submitted a trigger we'd like to see, - // so let's do it but only if we are the payee - - const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip()); - auto mnList = deterministicMNManager->GetListForBlock(tip); - auto mn_payees = mnList.GetProjectedMNPayees(tip); - if (mn_payees.empty()) return; - { - LOCK(activeMasternodeInfoCs); - if (mn_payees.front()->proTxHash != activeMasternodeInfo.proTxHash) return; - gov_sb.SetMasternodeOutpoint(activeMasternodeInfo.outpoint); - gov_sb.Sign( *activeMasternodeInfo.blsKeyOperator); - } - if (std::string strError; !gov_sb.IsValidLocally(strError, true)) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Created trigger is invalid:%s\n", __func__, strError); - return; - } - if (!MasternodeRateCheck(gov_sb)) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Trigger rejected because of rate check failure hash(%s)\n", __func__, gov_sb.GetHash().ToString()); - return; - } - // The trigger we just created looks good, submit it - AddGovernanceObject(gov_sb, connman); - gov_sb_voting = &gov_sb; - } else { + CGovernanceObject gov_sb(uint256(), 1, GetAdjustedTime(), uint256(), sb_opt.value().GetHexStrData()); + if (auto identical_sb = FindGovernanceObjectByDataHash(gov_sb.GetDataHash())) { // Somebody submitted a trigger with the same data, support it instead of submitting a duplicate - gov_sb_voting = identical_sb; + return std::make_optional(*identical_sb); } - // Vote YES-FUNDING for the trigger we like - if (!VoteFundingTrigger(gov_sb_voting->GetHash(), VOTE_OUTCOME_YES, connman)) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s failed\n", __func__, gov_sb_voting->GetHash().ToString()); - return; + // Nobody submitted a trigger we'd like to see, so let's do it but only if we are the payee + auto mnList = deterministicMNManager->GetListForBlock(::ChainActive().Tip()); + auto mn_payees = mnList.GetProjectedMNPayees(::ChainActive().Tip()); + + if (mn_payees.empty()) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s payee list is empty\n", __func__); + return std::nullopt; } - votedFundingYesTriggerHash = gov_sb_voting->GetHash(); + { + LOCK(activeMasternodeInfoCs); + if (mn_payees.front()->proTxHash != activeMasternodeInfo.proTxHash) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s we are not the payee, skipping\n", __func__); + return std::nullopt; + } + gov_sb.SetMasternodeOutpoint(activeMasternodeInfo.outpoint); + gov_sb.Sign( *activeMasternodeInfo.blsKeyOperator); + } // activeMasternodeInfoCs - // Vote NO-FUNDING for the rest of the active triggers - auto activeTriggers = triggerman.GetActiveTriggers(); - for (const auto& trigger : activeTriggers) { - if (trigger->GetHash() == gov_sb_voting->GetHash()) continue; // Skip actual trigger + if (std::string strError; !gov_sb.IsValidLocally(strError, true)) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Created trigger is invalid:%s\n", __func__, strError); + return std::nullopt; + } - if (!VoteFundingTrigger(trigger->GetHash(), VOTE_OUTCOME_NO, connman)) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger->GetHash().ToString()); + if (!MasternodeRateCheck(gov_sb)) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Trigger rejected because of rate check failure hash(%s)\n", __func__, gov_sb.GetHash().ToString()); + return std::nullopt; + } + + // The trigger we just created looks good, submit it + AddGovernanceObject(gov_sb, connman); + return std::make_optional(gov_sb); +} + +void CGovernanceManager::VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman) +{ + // only active masternodes can vote on triggers + if (!fMasternodeMode || WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash.IsNull())) return; + + LOCK2(cs_main, cs); + + if (trigger_opt.has_value()) { + // We should never vote "yes" on another trigger or the same trigger twice + assert(!votedFundingYesTriggerHash.has_value()); + // Vote YES-FUNDING for the trigger we like + const uint256 gov_sb_hash = trigger_opt.value().GetHash(); + if (!VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES, connman)) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s failed\n", __func__, gov_sb_hash.ToString()); + // this should never happen, bail out return; } + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s success\n", __func__, gov_sb_hash.ToString()); + votedFundingYesTriggerHash = gov_sb_hash; + } + + // Vote NO-FUNDING for the rest of the active triggers + const auto activeTriggers = triggerman.GetActiveTriggers(); + for (const auto& trigger : activeTriggers) { + const uint256 trigger_hash = trigger->GetGovernanceObject(*this)->GetHash(); + if (trigger_hash == votedFundingYesTriggerHash) { + // Skip actual trigger + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n", __func__, trigger_hash.ToString()); + continue; + } + if (!VoteFundingTrigger(trigger_hash, VOTE_OUTCOME_NO, connman)) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger_hash.ToString()); + // failing here is ok-ish + continue; + } + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s success\n", __func__, trigger_hash.ToString()); } } @@ -1417,10 +1445,9 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co return; } - auto sb_opt = CreateSuperblockCandidate(pindex->nHeight); - if (sb_opt.has_value()) { - CreateGovernanceTrigger(sb_opt.value(), connman); - } + const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight); + const auto trigger_opt = CreateGovernanceTrigger(sb_opt, connman); + VoteGovernanceTriggers(trigger_opt, connman); nCachedBlockHeight = pindex->nHeight; LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: %d\n", nCachedBlockHeight); diff --git a/src/governance/governance.h b/src/governance/governance.h index fa569caaa5e91..6dd92b5545270 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -293,8 +293,9 @@ class CGovernanceManager : public GovernanceStore void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); - std::optional CreateSuperblockCandidate(int nHeight) const; - void CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman); + const std::optional CreateSuperblockCandidate(int nHeight) const; + const std::optional CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman); + void VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman); bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman); bool HasAlreadyVotedFundingTrigger() const; void ResetVotedFundingTrigger(); From 1052500272ba8bde50318a2a318046ca755db0e9 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 2 Nov 2023 21:56:08 +0300 Subject: [PATCH 2/6] fix: drop `IsValid()` which does nothing, the real validation happens inside of `ProcessVoteAndRelay()` --- src/governance/governance.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index d07ca77a9b6fb..fbe1ed8eb844e 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -757,11 +757,6 @@ bool CGovernanceManager::VoteFundingTrigger(const uint256& nHash, const vote_out vote.SetTime(GetAdjustedTime()); vote.Sign(WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator)); - if (!vote.IsValid()) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Vote FUNDING %d for trigger:%s is invalid\n", __func__, outcome, nHash.ToString()); - return false; - } - CGovernanceException exception; if (!ProcessVoteAndRelay(vote, exception, connman)) { LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Vote FUNDING %d for trigger:%s failed:%s\n", __func__, outcome, nHash.ToString(), exception.what()); From c7f5b7d65bde2d45bccb6eeed9425780ba72b1f4 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 3 Nov 2023 03:47:55 +0300 Subject: [PATCH 3/6] test: add test for 2 triggers --- test/functional/feature_governance.py | 85 +++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 6 deletions(-) diff --git a/test/functional/feature_governance.py b/test/functional/feature_governance.py index b790eadbfd7d4..976ae9ef0b56e 100755 --- a/test/functional/feature_governance.py +++ b/test/functional/feature_governance.py @@ -190,16 +190,57 @@ def run_test(self): assert_equal(len(self.nodes[0].gobject("list", "valid", "triggers")), 0) - # Move 1 block enabling the Superblock maturity window + # Detect payee node + mn_list = self.nodes[0].protx("list", "registered", True) + height_protx_list = [] + for mn in mn_list: + height_protx_list.append((mn['state']['lastPaidHeight'], mn['proTxHash'])) + + height_protx_list = sorted(height_protx_list) + _, mn_payee_protx = height_protx_list[1] + + payee_idx = None + for mn in self.mninfo: + if mn.proTxHash == mn_payee_protx: + payee_idx = mn.nodeIdx + break + assert payee_idx is not None + + # Isolate payee node and create a trigger + self.isolate_node(payee_idx) + isolated = self.nodes[payee_idx] + + # Move 1 block inside the Superblock maturity window on the isolated node + isolated.generate(1) + self.bump_mocktime(1) + # The isolated "winner" should submit new trigger and vote for it + wait_until(lambda: len(isolated.gobject("list", "valid", "triggers")) == 1, timeout=5) + isolated_trigger_hash = list(isolated.gobject("list", "valid", "triggers").keys())[0] + wait_until(lambda: list(isolated.gobject("list", "valid", "triggers").values())[0]['YesCount'] == 1, timeout=5) + more_votes = wait_until(lambda: list(isolated.gobject("list", "valid", "triggers").values())[0]['YesCount'] > 1, timeout=5, do_assert=False) + assert_equal(more_votes, False) + + # Move 1 block enabling the Superblock maturity window on non-isolated nodes self.nodes[0].generate(1) self.bump_mocktime(1) - self.sync_blocks() assert_equal(self.nodes[0].getblockcount(), 230) assert_equal(self.nodes[0].getblockchaininfo()["softforks"]["v20"]["bip9"]["status"], "locked_in") self.check_superblockbudget(False) - # The "winner" should submit new trigger and vote for it, no one else should vote yet + # The "winner" should submit new trigger and vote for it, but it's isolated so no triggers should be found + has_trigger = wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) >= 1, timeout=5, do_assert=False) + assert_equal(has_trigger, False) + + # Move 1 block inside the Superblock maturity window on non-isolated nodes + self.nodes[0].generate(1) + self.bump_mocktime(1) + + # There is now new "winner" who should submit new trigger and vote for it wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) == 1, timeout=5) + winning_trigger_hash = list(self.nodes[0].gobject("list", "valid", "triggers").keys())[0] + wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == 1, timeout=5) + more_votes = wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] > 1, timeout=5, do_assert=False) + assert_equal(more_votes, False) # Make sure amounts aren't trimmed payment_amounts_expected = [str(satoshi_round(str(self.p0_amount))), str(satoshi_round(str(self.p1_amount))), str(satoshi_round(str(self.p2_amount)))] @@ -208,13 +249,45 @@ def run_test(self): for amount_str in payment_amounts_trigger: assert(amount_str in payment_amounts_expected) - # Move 1 block inside the Superblock maturity window + # Move another block inside the Superblock maturity window on non-isolated nodes + self.nodes[0].generate(1) + self.bump_mocktime(1) + + # Every non-isolated MN should vote for the same trigger now, no new triggers should be created + wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == self.mn_count - 1, timeout=5) + more_triggers = wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) > 1, timeout=5, do_assert=False) + assert_equal(more_triggers, False) + + self.reconnect_isolated_node(payee_idx, 0) + # self.connect_nodes(0, payee_idx) + self.sync_blocks() + + # re-sync helper + def sync_gov(node): + self.bump_mocktime(1) + return node.mnsync("status")["IsSynced"] + + for node in self.nodes: + # Force sync + node.mnsync("reset") + # fast-forward to governance sync + node.mnsync("next") + wait_until(lambda: sync_gov(node)) + + # Should see two triggers now + wait_until(lambda: len(isolated.gobject("list", "valid", "triggers")) == 2, timeout=5) + wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) == 2, timeout=5) + more_triggers = wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) > 2, timeout=5, do_assert=False) + assert_equal(more_triggers, False) + + # Move another block inside the Superblock maturity window self.nodes[0].generate(1) self.bump_mocktime(1) self.sync_blocks() - # Every MN should vote for the same trigger now, no new triggers should be created - wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == self.mn_count, timeout=5) + # Should see NO votes on both triggers now + wait_until(lambda: self.nodes[0].gobject("list", "valid", "triggers")[winning_trigger_hash]['NoCount'] == 1, timeout=5) + wait_until(lambda: self.nodes[0].gobject("list", "valid", "triggers")[isolated_trigger_hash]['NoCount'] == self.mn_count - 1, timeout=5) block_count = self.nodes[0].getblockcount() n = sb_cycle - block_count % sb_cycle From 4e64443c17cefab94cc2352f8b1e8e802f991738 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 3 Nov 2023 18:26:28 +0300 Subject: [PATCH 4/6] fix: constness --- src/governance/governance.cpp | 6 +++--- src/governance/governance.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index fbe1ed8eb844e..bfc535a6db772 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -554,7 +554,7 @@ struct sortProposalsByVotes { } }; -const std::optional CGovernanceManager::CreateSuperblockCandidate(int nHeight) const +std::optional CGovernanceManager::CreateSuperblockCandidate(int nHeight) const { if (!fMasternodeMode || fDisableGovernance) return std::nullopt; if (::masternodeSync == nullptr || !::masternodeSync->IsSynced()) return std::nullopt; @@ -663,7 +663,7 @@ const std::optional CGovernanceManager::CreateSuperblockCandidate(i return CSuperblock(nNextSuperblock, std::move(payments)); } -const std::optional CGovernanceManager::CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman) +std::optional CGovernanceManager::CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman) { // no sb_opt, no trigger if (!sb_opt.has_value()) return std::nullopt; @@ -712,7 +712,7 @@ const std::optional CGovernanceManager::CreateGovernanceTrigg return std::make_optional(gov_sb); } -void CGovernanceManager::VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman) +void CGovernanceManager::VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman) { // only active masternodes can vote on triggers if (!fMasternodeMode || WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.proTxHash.IsNull())) return; diff --git a/src/governance/governance.h b/src/governance/governance.h index 6dd92b5545270..1dfc5c27f1d84 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -293,9 +293,9 @@ class CGovernanceManager : public GovernanceStore void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); - const std::optional CreateSuperblockCandidate(int nHeight) const; - const std::optional CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman); - void VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman); + std::optional CreateSuperblockCandidate(int nHeight) const; + std::optional CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman); + void VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman); bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman); bool HasAlreadyVotedFundingTrigger() const; void ResetVotedFundingTrigger(); From a4b9e8175740941687467ff4553495c06bdb625d Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 6 Nov 2023 18:10:41 +0300 Subject: [PATCH 5/6] fix: use the same tip, make them const --- src/governance/governance.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index bfc535a6db772..25dff9e48e6f8 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -679,8 +679,9 @@ std::optional CGovernanceManager::CreateGovernanceTrigg } // Nobody submitted a trigger we'd like to see, so let's do it but only if we are the payee - auto mnList = deterministicMNManager->GetListForBlock(::ChainActive().Tip()); - auto mn_payees = mnList.GetProjectedMNPayees(::ChainActive().Tip()); + const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip()); + const auto mnList = deterministicMNManager->GetListForBlock(tip); + const auto mn_payees = mnList.GetProjectedMNPayees(tip); if (mn_payees.empty()) { LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s payee list is empty\n", __func__); From e1f5e4953416d98b98bc812d6df5c2253864d017 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 6 Nov 2023 18:11:08 +0300 Subject: [PATCH 6/6] refactor: make some sb/trigger methods private --- src/governance/governance.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/governance/governance.h b/src/governance/governance.h index 1dfc5c27f1d84..e2a2ffd9df6da 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -293,11 +293,6 @@ class CGovernanceManager : public GovernanceStore void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); - std::optional CreateSuperblockCandidate(int nHeight) const; - std::optional CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman); - void VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman); - bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman); - bool HasAlreadyVotedFundingTrigger() const; void ResetVotedFundingTrigger(); void DoMaintenance(CConnman& connman); @@ -371,6 +366,12 @@ class CGovernanceManager : public GovernanceStore int RequestGovernanceObjectVotes(Span vNodesCopy, CConnman& connman) const; private: + std::optional CreateSuperblockCandidate(int nHeight) const; + std::optional CreateGovernanceTrigger(const std::optional& sb_opt, CConnman& connman); + void VoteGovernanceTriggers(const std::optional& trigger_opt, CConnman& connman); + bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman); + bool HasAlreadyVotedFundingTrigger() const; + void RequestGovernanceObject(CNode* pfrom, const uint256& nHash, CConnman& connman, bool fUseFilter = false) const; void AddInvalidVote(const CGovernanceVote& vote)