Merge #6712: fix: request governance votes from more nodes on regtest

bd2aa80 refactor: use named constants, tweak and explain chosen numbers (UdjinM6)
c16a62f test: adjust feature_governance.py (UdjinM6)
4bcacc5 fix: request governance votes from more nodes on regtest (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  We only ask `nPeersPerHashMax` (3) nodes for governance votes for the same governance object when syncing. However on regtest we also isolate nodes to create conflicting triggers and since we have 5 nodes to sync from asking 3 of them often results in asking "non-isolated" nodes only (24 votes) yet sometimes we do ask previously isolated node too (25 votes).

  Should fix `feature_governance.py` flakiness. Alternative to #6710.

  ## What was done?
  Bump `nPeersPerHashMax` for regtest. Add more asserts in tests to fail earlier if smth isn't right, check votes on all nodes.

  ## How Has This Been Tested?
  run `feature_governance.py`

  ## Breaking Changes
  n/a

  ## Checklist:
  - [ ] 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)_

Top commit has no ACKs.

Tree-SHA512: 10885df4d1252d09051b053703c097f9522c79a0f3fa744ea1287aeb23ff47d24371dc38478255378898fc1b7e87785d0a852f7315562d1e19089637c15e03f3

Author: PastaPastaPasta

src/governance/governance.cpp

@@ -1291,12 +1291,20 @@ int CGovernanceManager::RequestGovernanceObjectVotes(const std::vector<CNode*>&
                                                      const PeerManager& peerman) const
 {
     static std::map<uint256, std::map<CService, int64_t> > mapAskedRecently;
+    // Maximum number of nodes to request votes from for the same object hash on real networks
+    // (mainnet, testnet, devnets). Keep this low to avoid unnecessary bandwidth usage.
+    static constexpr size_t REALNET_PEERS_PER_HASH{3};
+    // Maximum number of nodes to request votes from for the same object hash on regtest.
+    // During testing, nodes are isolated to create conflicting triggers. Using the real
+    // networks limit of 3 nodes often results in querying only "non-isolated" nodes, missing the
+    // isolated ones we need to test. This high limit ensures all available nodes are queried.
+    static constexpr size_t REGTEST_PEERS_PER_HASH{std::numeric_limits<size_t>::max()};
 
     if (vNodesCopy.empty()) return -1;
 
     int64_t nNow = GetTime();
     int nTimeout = 60 * 60;
-    size_t nPeersPerHashMax = 3;
+    size_t nPeersPerHashMax = Params().IsMockableChain() ? REGTEST_PEERS_PER_HASH : REALNET_PEERS_PER_HASH;
 
     std::vector<uint256> vTriggerObjHashes;
     std::vector<uint256> vOtherObjHashes;

test/functional/feature_governance.py

@@ -166,6 +166,9 @@ def run_test(self):
         self.wait_until(lambda: self.nodes[1].gobject("get", self.p2_hash)["FundingResult"]["NoCount"] == 2, timeout = 5)
 
         assert_equal(len(self.nodes[0].gobject("list", "valid", "triggers")), 0)
+        # 5 nodes voted on 3 proposals so we expect to see 15 votes total
+        assert_equal(self.nodes[0].gobject("count")["votes"], 15)
+        assert_equal(self.nodes[1].gobject("count")["votes"], 15)
 
         block_count = self.nodes[0].getblockcount()
 
@@ -209,6 +212,10 @@ def run_test(self):
         self.wait_until(lambda: list(isolated.gobject("list", "valid", "triggers").values())[0]['YesCount'] == 1, timeout=5)
         more_votes = self.wait_until(lambda: list(isolated.gobject("list", "valid", "triggers").values())[0]['YesCount'] > 1, timeout=5, do_assert=False)
         assert_equal(more_votes, False)
+        # Isolated node created a trigger and voted YES for it (16 votes total)
+        assert_equal(isolated.gobject("count")["votes"], 16)
+        # Non-isolated nodes don't see this (still 15 votes total)
+        assert_equal(self.nodes[0].gobject("count")["votes"], 15)
 
         self.log.info("Move 1 block enabling the Superblock maturity window on non-isolated nodes")
         self.bump_mocktime(1)
@@ -231,6 +238,10 @@ def run_test(self):
         self.wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == 1, timeout=5)
         more_votes = self.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)
+        # Non-isolated node created a trigger and voted YES for it (16 votes total)
+        assert_equal(self.nodes[0].gobject("count")["votes"], 16)
+        # Isolated node don't see this (still 16 votes total)
+        assert_equal(isolated.gobject("count")["votes"], 16)
 
         self.log.info("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)))]
@@ -247,6 +258,10 @@ def run_test(self):
         self.wait_until(lambda: list(self.nodes[0].gobject("list", "valid", "triggers").values())[0]['YesCount'] == self.mn_count - 1, timeout=5)
         more_triggers = self.wait_until(lambda: len(self.nodes[0].gobject("list", "valid", "triggers")) > 1, timeout=5, do_assert=False)
         assert_equal(more_triggers, False)
+        # All 4 non-isolated nodes voted YES for a trigger created by a non-isolated node earlier (19 votes total)
+        assert_equal(self.nodes[0].gobject("count")["votes"], 19)
+        # Isolated node don't see this (still 16 votes total)
+        assert_equal(isolated.gobject("count")["votes"], 16)
 
         self.reconnect_isolated_node(payee_idx, 0)
         # self.connect_nodes(0, payee_idx)
@@ -279,11 +294,21 @@ def sync_gov(node):
         self.bump_mocktime(1)
         self.generate(self.nodes[0], 1, sync_fun=self.sync_blocks())
 
-        self.log.info("Should see NO votes on both triggers now")
-        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)
-        self.log.info("Should wait until all 24 votes are counted for success on next stages")
-        self.wait_until(lambda: self.nodes[1].gobject("count")["votes"] == 24, timeout=5)
+        self.log.info("Should see same YES and NO vote count for both triggers on all nodes now")
+        for node in self.nodes:
+            self.wait_until(lambda: node.gobject("list", "valid", "triggers")[winning_trigger_hash]['YesCount'] == self.mn_count - 1, timeout=5)
+            self.wait_until(lambda: node.gobject("list", "valid", "triggers")[winning_trigger_hash]['NoCount'] == 1, timeout=5)
+            self.wait_until(lambda: node.gobject("list", "valid", "triggers")[isolated_trigger_hash]['YesCount'] == 1, timeout=5)
+            self.wait_until(lambda: node.gobject("list", "valid", "triggers")[isolated_trigger_hash]['NoCount'] == self.mn_count - 1, timeout=5)
+
+        self.log.info("Should have 25 votes on all nodes")
+        # All 4 non-isolated nodes voted NO for a trigger created by a now reconnected node.
+        # They also see 1 YES vote for this trigger the reconnected node created earlier.
+        # The reconnected node received earlier votes from non-isolated ones and
+        # voted NO vote for the trigger non-isolated node created.
+        # So everyone should be on the same page now with 25 votes total.
+        for node in self.nodes:
+            assert_equal(node.gobject("count")["votes"], 25)
 
         self.log.info("Remember vote count")
         before = self.nodes[1].gobject("count")["votes"]