fix: actually vote NO on triggers we don't like, some additional cleanups and tests (#5670)

## Issue being fixed or feature implemented
MNs don't really vote NO on triggers that do not match their local
candidates because:
1. they bail out too early when they see that they are not the payee
2. the hash for objects to vote NO on was picked incorrectly. 

## What was done?
Moved voting out of `CreateGovernanceTrigger` and into its own
`VoteGovernanceTriggers`. Refactored related code to use `optional`
while at it, dropped useless/misleading `IsValid()` call. Added some
safety belts, logging, tests.

## How Has This Been Tested?
Run tests.

## Breaking Changes
n/a

## Checklist:
- [x] I have performed a self-review of my own code
- [x] 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)_

src/governance/governance.cpp

@@ -554,7 +554,7 @@ struct sortProposalsByVotes {
     }
 };
 
-std::optional<CSuperblock> CGovernanceManager::CreateSuperblockCandidate(int nHeight) const
+std::optional<const CSuperblock> CGovernanceManager::CreateSuperblockCandidate(int nHeight) const
 {
     if (!fMasternodeMode || fDisableGovernance) return std::nullopt;
     if (::masternodeSync == nullptr || !::masternodeSync->IsSynced()) return std::nullopt;
@@ -663,63 +663,92 @@ std::optional<CSuperblock> CGovernanceManager::CreateSuperblockCandidate(int nHe
     return CSuperblock(nNextSuperblock, std::move(payments));
 }
 
-void CGovernanceManager::CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman)
+std::optional<const CGovernanceObject> CGovernanceManager::CreateGovernanceTrigger(const std::optional<const CSuperblock>& 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_opt.value().GetHexStrData());
-    CGovernanceObject gov_sb(uint256(), 1, GetAdjustedTime(), uint256(), sb.GetHexStrData());
+    if (auto identical_sb = FindGovernanceObjectByDataHash(gov_sb.GetDataHash())) {
-    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 {
         // Somebody submitted a trigger with the same data, support it instead of submitting a duplicate
-        gov_sb_voting = identical_sb;
+        return std::make_optional<CGovernanceObject>(*identical_sb);
     }
 
-    // Vote YES-FUNDING for the trigger we like
+    // Nobody submitted a trigger we'd like to see, so let's do it but only if we are the payee
-    if (!VoteFundingTrigger(gov_sb_voting->GetHash(), VOTE_OUTCOME_YES, connman)) {
+    const CBlockIndex *tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip());
-        LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s failed\n", __func__, gov_sb_voting->GetHash().ToString());
+    const auto mnList = deterministicMNManager->GetListForBlock(tip);
-        return;
+    const auto mn_payees = mnList.GetProjectedMNPayees(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
+
+    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 (!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<CGovernanceObject>(gov_sb);
+}
+
+void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& 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
-    auto activeTriggers = triggerman.GetActiveTriggers();
+    const auto activeTriggers = triggerman.GetActiveTriggers();
     for (const auto& trigger : activeTriggers) {
-        if (trigger->GetHash() == gov_sb_voting->GetHash()) continue; // Skip actual trigger
+        const uint256 trigger_hash = trigger->GetGovernanceObject(*this)->GetHash();
-
+        if (trigger_hash == votedFundingYesTriggerHash) {
-        if (!VoteFundingTrigger(trigger->GetHash(), VOTE_OUTCOME_NO, connman)) {
+            // Skip actual trigger
-            LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger->GetHash().ToString());
+            LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n", __func__, trigger_hash.ToString());
-            return;
+            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());
     }
 }
 
@@ -729,11 +758,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());
@@ -1417,10 +1441,9 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
         return;
     }
 
-    auto sb_opt = CreateSuperblockCandidate(pindex->nHeight);
+    const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight);
-    if (sb_opt.has_value()) {
+    const auto trigger_opt = CreateGovernanceTrigger(sb_opt, connman);
-        CreateGovernanceTrigger(sb_opt.value(), connman);
+    VoteGovernanceTriggers(trigger_opt, connman);
-    }
 
     nCachedBlockHeight = pindex->nHeight;
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: %d\n", nCachedBlockHeight);

src/governance/governance.h

@@ -293,10 +293,6 @@ public:
 
     void ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connman, std::string_view msg_type, CDataStream& vRecv);
 
-    std::optional<CSuperblock> CreateSuperblockCandidate(int nHeight) const;
-    void CreateGovernanceTrigger(const CSuperblock& sb, CConnman& connman);
-    bool VoteFundingTrigger(const uint256& nHash, const vote_outcome_enum_t outcome, CConnman& connman);
-    bool HasAlreadyVotedFundingTrigger() const;
     void ResetVotedFundingTrigger();
 
     void DoMaintenance(CConnman& connman);
@@ -370,6 +366,12 @@ public:
     int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const;
 
 private:
+    std::optional<const CSuperblock> CreateSuperblockCandidate(int nHeight) const;
+    std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt, CConnman& connman);
+    void VoteGovernanceTriggers(const std::optional<const CGovernanceObject>& 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)

test/functional/feature_governance.py

@@ -190,16 +190,57 @@ class DashGovernanceTest (DashTestFramework):
 
         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 @@ class DashGovernanceTest (DashTestFramework):
         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
+        # Should see NO votes on both triggers now
-        wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == self.mn_count, timeout=5)
+        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