Merge #6156: fix: let internal govobj/vote inv request trackers expir…

…e after 60 sec

1f4e1a1 test: add test for governance inv expiration (UdjinM6)
06b4dba feat: make CInv python implementation aware of governance invs (UdjinM6)
c7c930e fix: use correct condition in logs (UdjinM6)
d41d87a fix: use `std::chrono::seconds` (UdjinM6)
d6fe714 chore: make clang-format and linter happy (UdjinM6)
b57a922 refactor: sqash 2 hash maps into one, use proper naming (UdjinM6)
4116ba3 fix: let internal govobj/vote inv request trackers expire after 60 sec (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Another issue noticed during recent sb voting period. Lots of inv are coming from peers that never send the object/vote they announced. Let's not wait for these forever, 60 seconds should be enough.

  ## What was done?
  Add a "timer" to each request.

  ## 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:
  knst:
    utACK 1f4e1a1

Tree-SHA512: f5c5a896f61b5aed27c6f42c926254156c604edb4efd2a3cd3738a7e8d1ed7bafffadc584148e4a5c1a172c2b3d61a06884a1f6d56eb4ebe37a4b0dbc050edd6

src/governance/governance.cpp

@@ -55,7 +55,6 @@ CGovernanceManager::CGovernanceManager(CMasternodeMetaMan& mn_metaman, CNetFulfi
     m_mn_sync{mn_sync},
     nTimeLastDiff(0),
     nCachedBlockHeight(0),
-    setRequestedObjects(),
     fRateChecksEnabled(true),
     votedFundingYesTriggerHash(std::nullopt),
     mapTrigger{}
@@ -172,7 +171,7 @@ PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, Pe
 
         LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received object: %s\n", strHash);
 
-        if (!AcceptObjectMessage(nHash)) {
+        if (!AcceptMessage(nHash)) {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received unrequested object: %s\n", strHash);
             return {};
         }
@@ -240,7 +239,7 @@ PeerMsgRet CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, Pe
 
         std::string strHash = nHash.ToString();
 
-        if (!AcceptVoteMessage(nHash)) {
+        if (!AcceptMessage(nHash)) {
             LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Received unrequested vote object: %s, hash: %s, peer = %d\n",
                 vote.ToString(tip_mn_list), strHash, peer.GetId());
             return {};
@@ -453,7 +452,18 @@ void CGovernanceManager::CheckAndRemove()
         }
     }
 
-    LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- %s\n", ToString());
+    // forget about expired requests
+    auto r_it = m_requested_hash_time.begin();
+    while (r_it != m_requested_hash_time.end()) {
+        if (r_it->second < std::chrono::seconds(nNow)) {
+            m_requested_hash_time.erase(r_it++);
+        } else {
+            ++r_it;
+        }
+    }
+
+    LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- %s, m_requested_hash_time size=%d\n",
+             ToString(), m_requested_hash_time.size());
 }
 
 const CGovernanceObject* CGovernanceManager::FindConstGovernanceObject(const uint256& nHash) const
@@ -837,23 +847,13 @@ bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv)
         return false;
     }
 
-
-    hash_s_t* setHash = nullptr;
-    switch (inv.type) {
-    case MSG_GOVERNANCE_OBJECT:
-        setHash = &setRequestedObjects;
-        break;
-    case MSG_GOVERNANCE_OBJECT_VOTE:
-        setHash = &setRequestedVotes;
-        break;
-    default:
-        return false;
-    }
-
-    const auto& [_itr, inserted] = setHash->insert(inv.hash);
+    const auto valid_until = GetTime<std::chrono::seconds>() + std::chrono::seconds(RELIABLE_PROPAGATION_TIME);
+    const auto& [_itr, inserted] = m_requested_hash_time.emplace(inv.hash, valid_until);
 
     if (inserted) {
-        LogPrint(BCLog::GOBJECT, "CGovernanceManager::ConfirmInventoryRequest added inv to requested set\n");
+        LogPrint(BCLog::GOBJECT, /* Continued */
+                 "CGovernanceManager::ConfirmInventoryRequest added %s inv hash to m_requested_hash_time, size=%d\n",
+                 inv.type == MSG_GOVERNANCE_OBJECT ? "object" : "vote", m_requested_hash_time.size());
     }
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::ConfirmInventoryRequest reached end, returning true\n");
@@ -1330,27 +1330,16 @@ int CGovernanceManager::RequestGovernanceObjectVotes(const std::vector<CNode*>&
     return int(vTriggerObjHashes.size() + vOtherObjHashes.size());
 }
 
-bool CGovernanceManager::AcceptObjectMessage(const uint256& nHash)
+bool CGovernanceManager::AcceptMessage(const uint256& nHash)
 {
     LOCK(cs);
-    return AcceptMessage(nHash, setRequestedObjects);
-}
-
-bool CGovernanceManager::AcceptVoteMessage(const uint256& nHash)
-{
-    LOCK(cs);
-    return AcceptMessage(nHash, setRequestedVotes);
-}
-
-bool CGovernanceManager::AcceptMessage(const uint256& nHash, hash_s_t& setHash)
-{
-    auto it = setHash.find(nHash);
-    if (it == setHash.end()) {
+    auto it = m_requested_hash_time.find(nHash);
+    if (it == m_requested_hash_time.end()) {
         // We never requested this
         return false;
     }
     // Only accept one response
-    setHash.erase(it);
+    m_requested_hash_time.erase(it);
     return true;
 }
 
@@ -1580,7 +1569,7 @@ void CGovernanceManager::RemoveInvalidVotes()
                 cmapVoteToObject.Erase(voteHash);
                 cmapInvalidVotes.Erase(voteHash);
                 cmmapOrphanVotes.Erase(voteHash);
-                setRequestedVotes.erase(voteHash);
+                m_requested_hash_time.erase(voteHash);
             }
         }
     }

src/governance/governance.h

@@ -270,8 +270,7 @@ class CGovernanceManager : public GovernanceStore
     int nCachedBlockHeight;
     std::map<uint256, CGovernanceObject> mapPostponedObjects;
     hash_s_t setAdditionalRelayObjects;
-    hash_s_t setRequestedObjects;
-    hash_s_t setRequestedVotes;
+    std::map<uint256, std::chrono::seconds> m_requested_hash_time;
     bool fRateChecksEnabled;
     std::optional<uint256> votedFundingYesTriggerHash;
     std::map<uint256, std::shared_ptr<CSuperblock>> mapTrigger;
@@ -389,13 +388,8 @@ class CGovernanceManager : public GovernanceStore
 
     bool ProcessVote(CNode* pfrom, const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman);
 
-    /// Called to indicate a requested object has been received
-    bool AcceptObjectMessage(const uint256& nHash);
-
-    /// Called to indicate a requested vote has been received
-    bool AcceptVoteMessage(const uint256& nHash);
-
-    static bool AcceptMessage(const uint256& nHash, hash_s_t& setHash);
+    /// Called to indicate a requested object or vote has been received
+    bool AcceptMessage(const uint256& nHash);
 
     void CheckOrphanVotes(CGovernanceObject& govobj, PeerManager& peerman);
 

test/functional/p2p_governance_invs.py

@@ -0,0 +1,62 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024 The Dash Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+"""
+Test inv expiration for governance objects/votes
+"""
+
+from test_framework.messages import (
+    CInv,
+    msg_inv,
+    MSG_GOVERNANCE_OBJECT,
+    MSG_GOVERNANCE_OBJECT_VOTE,
+)
+from test_framework.p2p import P2PInterface
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import force_finish_mnsync
+
+RELIABLE_PROPAGATION_TIME = 60  # src/governance/governance.cpp
+DATA_CLEANUP_TIME = 5 * 60      # src/init.cpp
+MSG_INV_ADDED = 'CGovernanceManager::ConfirmInventoryRequest added {} inv hash to m_requested_hash_time'
+
+class GovernanceInvsTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        node = self.nodes[0]
+        force_finish_mnsync(node)
+        inv = msg_inv([CInv(MSG_GOVERNANCE_OBJECT, 1)])
+        self.test_request_expiration(inv, "object")
+        inv = msg_inv([CInv(MSG_GOVERNANCE_OBJECT_VOTE, 2)])
+        self.test_request_expiration(inv, "vote")
+
+    def test_request_expiration(self, inv, name):
+        msg = MSG_INV_ADDED.format(name)
+        node = self.nodes[0]
+        peer = node.add_p2p_connection(P2PInterface())
+        self.log.info(f"Send dummy governance {name} inv and make sure it's added to requested map")
+        with node.assert_debug_log([msg]):
+            peer.send_message(inv)
+        self.log.info(f"Send dummy governance {name} inv again and make sure it's not added because we know about it already")
+        with node.assert_debug_log([], [msg]):
+            peer.send_message(inv)
+        self.log.info("Force internal cleanup")
+        with node.assert_debug_log(['UpdateCachesAndClean']):
+            node.mockscheduler(DATA_CLEANUP_TIME + 1)
+        self.log.info(f"Send dummy governance {name} inv again and make sure it's not added because we still know about it")
+        with node.assert_debug_log([], [msg]):
+            peer.send_message(inv)
+        self.log.info(f"Bump mocktime, force internal cleanup, send dummy governance {name} inv again and make sure it's accepted again")
+        self.bump_mocktime(RELIABLE_PROPAGATION_TIME + 1, nodes=[node])
+        with node.assert_debug_log(['UpdateCachesAndClean']):
+            node.mockscheduler(DATA_CLEANUP_TIME + 1)
+        with node.assert_debug_log([msg]):
+            peer.send_message(inv)
+        node.disconnect_p2ps()
+
+
+if __name__ == '__main__':
+    GovernanceInvsTest().main()

test/functional/test_framework/messages.py

@@ -59,6 +59,8 @@
 MSG_TX = 1
 MSG_BLOCK = 2
 MSG_FILTERED_BLOCK = 3
+MSG_GOVERNANCE_OBJECT = 17
+MSG_GOVERNANCE_OBJECT_VOTE = 18
 MSG_CMPCT_BLOCK = 20
 MSG_TYPE_MASK = 0xffffffff >> 2
 
@@ -350,6 +352,8 @@ class CInv:
         MSG_TX: "TX",
         MSG_BLOCK: "Block",
         MSG_FILTERED_BLOCK: "filtered Block",
+        MSG_GOVERNANCE_OBJECT: "Governance Object",
+        MSG_GOVERNANCE_OBJECT_VOTE: "Governance Vote",
         MSG_CMPCT_BLOCK: "CompactBlock",
     }
 

test/functional/test_runner.py

@@ -293,6 +293,7 @@
     'feature_governance.py --descriptors',
     'feature_governance_cl.py --legacy-wallet',
     'feature_governance_cl.py --descriptors',
+    'p2p_governance_invs.py',
     'rpc_uptime.py',
     'feature_discover.py',
     'wallet_resendwallettransactions.py --legacy-wallet',