Merge #6155: fix: avoid voting for the same trigger multiple times

e92aad7cff test: make sure MNs don't vote twice even when they are allowed to (UdjinM6)
3d75390e4e fix: correct conditions for YES voting (UdjinM6)
ec1392c6de chore: make clang-format and linter happy (UdjinM6)
a6320865c4 fix: avoid voting for the same trigger multiple times (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  We just had a sb voting period and I noticed that the network is way too spammy and produces too many votes (10x+ the expected numbers). It turns out that relying on `ProcessVoteAndRelay` on mainnet is not enough because rate-check expires too soon and MNs are able to vote again and again. On testnet it was never an issue because the voting period there is really short.

  ## What was done?
  Check known votes to make sure we never voted earlier.

  ## How Has This Been Tested?
  Run tests, run a MN on mainnet and check logs.

  ## 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
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK e92aad7cff

Tree-SHA512: 142e23d3a19fa9527fa5257eb790e558d3507a7a857f17c6e02fd58eeb5643fcfb48d824d227e0ea7cd3dae6a6d7d871b3af88b13077f5af074ed1911e42bb28

src/governance/governance.cpp

@@ -23,6 +23,7 @@
 #include <protocol.h>
 #include <shutdown.h>
 #include <spork.h>
+#include <util/ranges.h>
 #include <util/time.h>
 #include <validation.h>
 
@@ -736,21 +737,47 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGover
     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
+        // Vote YES-FUNDING for the trigger we like, unless we already did
         const uint256 gov_sb_hash = trigger_opt.value().GetHash();
-        if (!VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES, connman, peerman, mn_activeman)) {
+        bool voted_already{false};
-            LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s failed\n", __func__, gov_sb_hash.ToString());
+        if (vote_rec_t voteRecord; trigger_opt.value().GetCurrentMNVotes(mn_activeman.GetOutPoint(), voteRecord)) {
-            // this should never happen, bail out
+            const auto& strFunc = __func__;
-            return;
+            // Let's see if there is a VOTE_SIGNAL_FUNDING vote from us already
+            voted_already = ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) {
+                if (voteInstancePair.first == VOTE_SIGNAL_FUNDING) {
+                    if (voteInstancePair.second.eOutcome == VOTE_OUTCOME_YES) {
+                        votedFundingYesTriggerHash = gov_sb_hash;
+                    }
+                    LogPrint(BCLog::GOBJECT, /* Continued */
+                             "CGovernanceManager::%s "
+                             "Not voting YES-FUNDING for trigger:%s, we voted %s for it already\n",
+                             strFunc, gov_sb_hash.ToString(),
+                             CGovernanceVoting::ConvertOutcomeToString(voteInstancePair.second.eOutcome));
+                    return true;
+                }
+                return false;
+            });
+        }
+        if (!voted_already) {
+            // No previous VOTE_SIGNAL_FUNDING was found, vote now
+            if (VoteFundingTrigger(gov_sb_hash, VOTE_OUTCOME_YES, connman, peerman, mn_activeman)) {
+                LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting YES-FUNDING for new trigger:%s success\n",
+                         __func__, gov_sb_hash.ToString());
+                votedFundingYesTriggerHash = gov_sb_hash;
+            } else {
+                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 = GetActiveTriggers();
     for (const auto& trigger : activeTriggers) {
-        const uint256 trigger_hash = trigger->GetGovernanceObject(*this)->GetHash();
+        const auto govobj = trigger->GetGovernanceObject(*this);
+        const uint256 trigger_hash = govobj->GetHash();
         if (trigger->GetBlockHeight() <= nCachedBlockHeight) {
             // ignore triggers from the past
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Not voting NO-FUNDING for outdated trigger:%s\n", __func__, trigger_hash.ToString());
@@ -761,6 +788,22 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optional<const CGover
             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 (vote_rec_t voteRecord; govobj->GetCurrentMNVotes(mn_activeman.GetOutPoint(), voteRecord)) {
+            const auto& strFunc = __func__;
+            if (ranges::any_of(voteRecord.mapInstances, [&](const auto& voteInstancePair) {
+                    if (voteInstancePair.first == VOTE_SIGNAL_FUNDING) {
+                        LogPrint(BCLog::GOBJECT, /* Continued */
+                                 "CGovernanceManager::%s "
+                                 "Not voting NO-FUNDING for trigger:%s, we voted %s for it already\n",
+                                 strFunc, trigger_hash.ToString(),
+                                 CGovernanceVoting::ConvertOutcomeToString(voteInstancePair.second.eOutcome));
+                        return true;
+                    }
+                    return false;
+                })) {
+                continue;
+            }
+        }
         if (!VoteFundingTrigger(trigger_hash, VOTE_OUTCOME_NO, connman, peerman, mn_activeman)) {
             LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s Voting NO-FUNDING for trigger:%s failed\n", __func__, trigger_hash.ToString());
             // failing here is ok-ish

test/functional/feature_governance.py

@@ -11,6 +11,8 @@ from test_framework.test_framework import DashTestFramework
 from test_framework.governance import have_trigger_for_height, prepare_object
 from test_framework.util import assert_equal, satoshi_round, set_node_times, wait_until_helper
 
+GOVERNANCE_UPDATE_MIN = 60 * 60 # src/governance/object.h
+
 class DashGovernanceTest (DashTestFramework):
     def set_test_params(self):
         self.v20_start_time = 1417713500 + 80
@@ -283,6 +285,23 @@ class DashGovernanceTest (DashTestFramework):
         self.wait_until(lambda: self.nodes[0].gobject("list", "valid", "triggers")[winning_trigger_hash]['NoCount'] == 1, timeout=5)
         self.wait_until(lambda: self.nodes[0].gobject("list", "valid", "triggers")[isolated_trigger_hash]['NoCount'] == self.mn_count - 1, timeout=5)
 
+        # Remember vote count
+        before = self.nodes[1].gobject("count")["votes"]
+
+        # Bump mocktime to let MNs vote again
+        self.bump_mocktime(GOVERNANCE_UPDATE_MIN + 1)
+
+        # Move another block inside the Superblock maturity window
+        with self.nodes[1].assert_debug_log(["CGovernanceManager::VoteGovernanceTriggers"]):
+            self.nodes[0].generate(1)
+            self.bump_mocktime(1)
+            self.sync_blocks()
+
+        # Vote count should not change even though MNs are allowed to vote again
+        assert_equal(before, self.nodes[1].gobject("count")["votes"])
+        # Revert mocktime back to avoid issues in tests below
+        self.bump_mocktime(GOVERNANCE_UPDATE_MIN * -1)
+
         block_count = self.nodes[0].getblockcount()
         n = sb_cycle - block_count % sb_cycle