fix: update masternode rate limit on failed governance trigger path

src/governance/governance.cpp

@@ -344,18 +344,25 @@ void CGovernanceManager::AddGovernanceObjectInternal(CGovernanceObject& insert_o
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- Before trigger block, GetDataAsPlainString = %s, nObjectType = %d\n",
                 Assert(govobj)->GetDataAsPlainString(), std23::to_underlying(govobj->GetObjectType()));
 
+    // Count the attempt against the per-masternode rate buffer before any early
+    // return. Failed AddTrigger paths used to skip this, so a single operator
+    // key could flood mapObjects with unparseable triggers.
+    MasternodeRateUpdate(*govobj);
+
     if (govobj->GetObjectType() == GovernanceObject::TRIGGER && !m_superblocks.AddTrigger(govobj, nCachedBlockHeight)) {
         LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- undo adding invalid trigger object: hash = %s\n", nHash.ToString());
         govobj->PrepareDeletion(GetTime<std::chrono::seconds>().count());
         return;
     }
 
+    // Only objects we keep may be announced. Scheduling this before the AddTrigger
+    // check would make us re-announce, and serve on GETDATA, a trigger we just
+    // undid and marked for deletion.
+    ScheduleTriggerRelay(*govobj);
+
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::AddGovernanceObject -- %s new, received from peer %s\n", strHash, peer_str);
     RelayObject(*govobj);
 
-    // Update the rate buffer
-    MasternodeRateUpdate(*govobj);
-
     m_mn_sync.BumpAssetLastTime("CGovernanceManager::AddGovernanceObject");
 
     // WE MIGHT HAVE PENDING/ORPHAN VOTES FOR THIS OBJECT
@@ -701,15 +708,23 @@ void CGovernanceManager::MasternodeRateUpdate(const CGovernanceObject& govobj)
         it = mapLastMasternodeObject.insert(txout_m_t::value_type(masternodeOutpoint, last_object_rec(true))).first;
     }
 
-    int64_t nTimestamp = govobj.GetCreationTime();
-    it->second.triggerBuffer.AddTimestamp(nTimestamp);
+    it->second.triggerBuffer.AddTimestamp(govobj.GetCreationTime());
+    it->second.fStatusOK = true;
+}
+
+void CGovernanceManager::ScheduleTriggerRelay(const CGovernanceObject& govobj)
+{
+    AssertLockHeld(cs_store);
 
-    if (nTimestamp > GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) {
-        // schedule additional relay for the object
+    if (govobj.GetObjectType() != GovernanceObject::TRIGGER) return;
+
+    // A trigger created this close to the future-deviation limit is still too new for
+    // peers with a lagging clock to accept, so re-announce it once it has aged past
+    // RELIABLE_PROPAGATION_TIME (see CheckPostponedObjects).
+    if (govobj.GetCreationTime() >
+        GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) {
         setAdditionalRelayObjects.insert(govobj.GetHash());
     }
-
-    it->second.fStatusOK = true;
 }
 
 bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus)

src/governance/governance.h

@@ -403,6 +403,11 @@ class CGovernanceManager : public GovernanceStore
     void MasternodeRateUpdate(const CGovernanceObject& govobj)
         EXCLUSIVE_LOCKS_REQUIRED(cs_store);
 
+    /** Queue a deferred re-announcement for a trigger that is too new to propagate
+     *  reliably yet. Only call this for triggers we are keeping. */
+    void ScheduleTriggerRelay(const CGovernanceObject& govobj)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_store);
+
     bool MasternodeRateCheck(const CGovernanceObject& govobj, bool fUpdateFailStatus, bool fForce, bool& fRateCheckBypassed)
         EXCLUSIVE_LOCKS_REQUIRED(cs_store);
 

src/test/governance_inv_tests.cpp

@@ -2,8 +2,11 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <bls/bls.h>
+#include <chainparams.h>
 #include <common/bloom.h>
 #include <evo/chainhelper.h>
+#include <evo/deterministicmns.h>
 #include <governance/governance.h>
 #include <governance/net_governance.h>
 #include <governance/object.h>
@@ -13,52 +16,92 @@
 #include <netfulfilledman.h>
 #include <node/connection_types.h>
 #include <protocol.h>
+#include <script/standard.h>
 #include <streams.h>
 #include <uint256.h>
+#include <util/std23.h>
 #include <util/strencodings.h>
 #include <util/time.h>
+#include <validation.h>
 #include <version.h>
 
+#include <test/util/masternode.h>
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
-#include <test/util/validation.h>
 
 #include <boost/test/unit_test.hpp>
 
+#include <algorithm>
 #include <atomic>
 #include <chrono>
 #include <memory>
+#include <string>
 #include <vector>
 
 using namespace std::chrono_literals;
 
 namespace {
-struct GovernanceInvSetup : public TestingSetup {
-    GovernanceInvSetup() : TestingSetup{CBaseChainParams::MAIN}
+// Unified governance unit-test fixture. The DIP3 / ProRegTx path is required for
+// signed-trigger rate regressions, and it also satisfies the lighter INV/vote
+// authorization tests (govman + NetGovernance handler + mn_sync).
+//
+// DIP3 activation is pushed to 109 so TestChainSetup's fixed-checkpoint assert
+// still succeeds while the early coinbases are mature.
+struct GovernanceInvSetup : public TestChainSetup {
+    COutPoint mn_outpoint;
+    CBLSSecretKey operator_key;
+
+    GovernanceInvSetup() :
+        TestChainSetup(/*num_blocks=*/107, CBaseChainParams::REGTEST, {"-dip3params=109:500"})
     {
-        // ConfirmInventoryRequest and the NetGovernance object/vote handlers
-        // short-circuit on !IsBlockchainSynced().
-        BOOST_REQUIRE(m_node.mn_sync);
-        m_node.mn_sync->SwitchToNextAsset();
-        BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced());
+        auto& chainman = *Assert(m_node.chainman.get());
+        auto& dmnman = *Assert(m_node.dmnman);
+        const CScript coinbase_pk = GetScriptForRawPubKey(coinbaseKey.GetPubKey());
+
+        // Activate DIP3 (fixture tip is one block before activation height 109).
+        // Enforcement is independent of activation; ProRegTx only needs activation.
+        CreateAndProcessBlock({}, coinbase_pk);
+        BOOST_REQUIRE_EQUAL(WITH_LOCK(::cs_main, return chainman.ActiveChain().Height()), 108);
+
+        auto utxos = BuildSimpleUtxoMap(m_coinbase_txns);
+        CKey owner_key;
+        auto proreg_tx = CreateProRegTx(chainman, utxos, /*port=*/1, GenerateRandomAddress(), coinbaseKey, owner_key,
+                                        operator_key);
+        CreateAndProcessBlock({proreg_tx}, coinbase_pk);
+        dmnman.UpdatedBlockTip(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()));
+
+        mn_outpoint = COutPoint{proreg_tx.GetHash(), 0};
+        const auto dmn = dmnman.GetListAtChainTip().GetMNByCollateral(mn_outpoint);
+        BOOST_REQUIRE(dmn);
+        BOOST_REQUIRE(dmn->pdmnState->pubKeyOperator.Get() == operator_key.GetPublicKey());
 
         BOOST_REQUIRE(m_node.mn_metaman);
         // Note: mn_metaman is left unloaded. No test here reaches
         // CGovernanceObject::ProcessVote, which asserts metaman.IsValid() -- a vote whose
         // parent object exists would, and would need it loaded first.
-        m_node.govman = std::make_unique<CGovernanceManager>(*m_node.mn_metaman, *m_node.chainman, *m_node.chain_helper->superblocks, *m_node.dmnman, *m_node.mn_sync);
+        BOOST_REQUIRE(m_node.mn_sync);
+        BOOST_REQUIRE(m_node.chain_helper);
+        BOOST_REQUIRE(m_node.chain_helper->superblocks);
+        m_node.govman = std::make_unique<CGovernanceManager>(*m_node.mn_metaman, *m_node.chainman,
+                                                             *m_node.chain_helper->superblocks, *m_node.dmnman,
+                                                             *m_node.mn_sync);
         // Match runtime preconditions: NetGovernance::AlreadyHave claims we
         // already have the inv when governance isn't loaded (e.g.
         // -disablegovernance), so ConfirmInventoryRequest would never run.
         BOOST_REQUIRE(m_node.govman->LoadCache(/*load_cache=*/false));
 
         BOOST_REQUIRE(m_node.netfulfilledman);
-        // Loaded here for the later test that advances GOVERNANCE -> FINISHED;
-        // the sync notifier asserts netfulfilledman.IsValid().
+        // Loaded before advancing sync: SyncFinished asserts netfulfilledman.IsValid().
         BOOST_REQUIRE(m_node.netfulfilledman->LoadCache(/*load_cache=*/false));
         BOOST_REQUIRE(m_node.connman);
         BOOST_REQUIRE(m_node.peerman);
 
+        // Advance BLOCKCHAIN -> GOVERNANCE -> FINISHED. Rate checks are a no-op until
+        // IsSynced(); INV/vote tests also tolerate a fully-synced start state.
+        m_node.mn_sync->SwitchToNextAsset();
+        m_node.mn_sync->SwitchToNextAsset();
+        BOOST_REQUIRE(m_node.mn_sync->IsSynced());
+
         // Intentional unit-test boundary: TestingSetup does not run the
         // init.cpp/AppInit startup path that registers the Dash-specific
         // handlers, so the INV branch in PeerManagerImpl::AlreadyHave would not
@@ -70,17 +113,33 @@ struct GovernanceInvSetup : public TestingSetup {
             m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
             *m_node.netfulfilledman, *m_node.connman));
 
-        // Anchor the clock so the object and vote timestamps the tests build
-        // from GetTime() are deterministic. Nothing advances it.
-        SetMockTime(1'700'000'000s);
+        // Deterministic clock for object/vote timestamps and rate-check windows.
+        SetMockTime(0s);
     }
-    ~GovernanceInvSetup() {
-        // govman holds a reference to chain_helper->superblocks, so it must be reset before chain_helper (matches PrepareShutdown
-        // ordering in init.cpp).
+
+    ~GovernanceInvSetup()
+    {
+        // govman holds a reference to chain_helper->superblocks, so it must be reset
+        // before chain_helper (matches PrepareShutdown ordering in init.cpp).
         m_node.peerman->RemoveHandlers();
         m_node.govman.reset();
     }
 
+    // Malformed trigger JSON: type=TRIGGER so LoadData/IsValidLocally accept it, but
+    // SuperblockManager::AddTrigger fails while constructing CSuperblock (missing
+    // event_block_height / payment fields). That is the failed-trigger path.
+    CGovernanceObject MakeFailedTrigger(int64_t creation_time, int salt) const
+    {
+        const std::string data = strprintf(R"({"type":2,"salt":%d})", salt);
+        CGovernanceObject govobj{uint256{}, /*revision=*/1, creation_time, uint256{}, HexStr(data)};
+        BOOST_REQUIRE_EQUAL(std23::to_underlying(govobj.GetObjectType()),
+                            std23::to_underlying(GovernanceObject::TRIGGER));
+        govobj.SetMasternodeOutpoint(mn_outpoint);
+        const CBLSSignature sig = operator_key.Sign(govobj.GetSignatureHash(), /*specificLegacyScheme=*/false);
+        BOOST_REQUIRE(sig.IsValid());
+        govobj.SetSignature(sig.ToByteVector(/*specificLegacyScheme=*/false));
+        return govobj;
+    }
 };
 
 size_t CountQueuedMessages(const CNode& peer, const std::string& msg_type)
@@ -187,8 +246,7 @@ BOOST_AUTO_TEST_CASE(per_object_vote_sync_is_fulfilled_request_limited)
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
     // NetGovernance::ProcessMessage ignores MNGOVERNANCESYNC until sync is
-    // fully finished; advance from GOVERNANCE to FINISHED.
-    m_node.mn_sync->SwitchToNextAsset();
+    // fully finished; the fixture already advances to FINISHED.
     BOOST_REQUIRE(m_node.mn_sync->IsSynced());
 
     auto peer{MakeGovernanceInvPeer(/*id=*/1)};
@@ -322,10 +380,6 @@ BOOST_AUTO_TEST_CASE(governance_objects_require_peer_announcement_or_request)
 {
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
-    TestChainState& chainstate =
-        *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
-    chainstate.JumpOutOfIbd();
-
     NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
                           *m_node.netfulfilledman, *m_node.connman);
 
@@ -373,17 +427,12 @@ BOOST_AUTO_TEST_CASE(governance_objects_require_peer_announcement_or_request)
     m_node.peerman->FinalizeNode(*announcing_peer);
     m_node.peerman->FinalizeNode(*second_announcing_peer);
     m_node.peerman->FinalizeNode(*unsolicited_peer);
-    chainstate.ResetIbd();
 }
 
 BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request)
 {
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
-    TestChainState& chainstate =
-        *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
-    chainstate.JumpOutOfIbd();
-
     NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
                           *m_node.netfulfilledman, *m_node.connman);
 
@@ -424,7 +473,6 @@ BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request)
     m_node.peerman->FinalizeNode(*announcing_peer);
     m_node.peerman->FinalizeNode(*second_announcing_peer);
     m_node.peerman->FinalizeNode(*unsolicited_peer);
-    chainstate.ResetIbd();
 }
 
 // A message received while not blockchain-synced is dropped, but the per-peer authorization must
@@ -433,10 +481,6 @@ BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop)
 {
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
-    TestChainState& chainstate =
-        *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
-    chainstate.JumpOutOfIbd();
-
     NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
                           *m_node.netfulfilledman, *m_node.connman);
 
@@ -468,7 +512,91 @@ BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop)
     BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 1U);
 
     m_node.peerman->FinalizeNode(*peer);
-    chainstate.ResetIbd();
 }
 
+BOOST_AUTO_TEST_CASE(failed_trigger_path_advances_masternode_rate_limit)
+{
+    // Deterministic clock inside the allowed rate-check timestamp window.
+    SetMockTime(0s);
+    const int64_t base_time = GetTime<std::chrono::seconds>().count();
+
+    // Submit more failed triggers than RATE_BUFFER_SIZE from the same MN.
+    // Pre-fix the rate buffer never fills, so every object lands in mapObjects.
+    // Post-fix the buffer fills and further submissions are rate-rejected.
+    constexpr int flood_count = 12;
+    int objects_seen = 0;
+    for (int i = 0; i < flood_count; ++i) {
+        CGovernanceObject govobj = MakeFailedTrigger(base_time + i, /*salt=*/i);
+        const uint256 hash = govobj.GetHash();
+        // Unique per-object: salt + time vary both the data and the creation time.
+        BOOST_REQUIRE(!m_node.govman->HaveObjectForHash(hash));
+
+        LOCK(::cs_main);
+        // ProcessObject returns true for both accepted and rate-rejected objects
+        // (rate rejection is not a peer-misbehaviour event).
+        BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj));
+
+        if (m_node.govman->HaveObjectForHash(hash)) {
+            ++objects_seen;
+            // Failed triggers are accepted into mapObjects then marked for deletion.
+            const auto stored = m_node.govman->FindGovernanceObject(hash);
+            BOOST_REQUIRE(stored);
+            BOOST_CHECK(stored->IsSetCachedDelete());
+        }
+    }
+
+    // With the rate buffer size of 5, a fixed limiter records the first five
+    // failed attempts and rejects the rest before insertion. Without the fix
+    // every object is inserted (objects_seen == flood_count).
+    BOOST_CHECK_LT(objects_seen, flood_count);
+    BOOST_CHECK_LE(objects_seen, RATE_BUFFER_SIZE + 1);
+
+    const UniValue stats = m_node.govman->ToJson();
+    BOOST_CHECK_LT(stats["objects_total"].getInt<int>(), flood_count);
+    BOOST_CHECK_EQUAL(stats["objects_total"].getInt<int>(), objects_seen);
+
+    // A trigger we undid must never be announced to peers.
+    BOOST_CHECK(m_node.govman->FetchRelayInventory().empty());
+}
+
+// A failed trigger must not be scheduled for the deferred trigger-relay pass.
+// That bookkeeping (setAdditionalRelayObjects) fires for creation times close
+// to MAX_TIME_FUTURE_DEVIATION, and used to live inside MasternodeRateUpdate()
+// -- so counting the failed attempt against the rate buffer would otherwise
+// make the node re-announce, and serve on GETDATA, an object it had just
+// rejected and marked for deletion. One signature-valid message would then fan
+// out to every peer, which in turn re-announce it.
+BOOST_AUTO_TEST_CASE(failed_trigger_is_not_scheduled_for_additional_relay)
+{
+    SetMockTime(0s);
+    const int64_t now = GetTime<std::chrono::seconds>().count();
+
+    // Inside (now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME,
+    // now + MAX_TIME_FUTURE_DEVIATION], i.e. accepted by MasternodeRateCheck but
+    // "too new to propagate reliably", which is what arms the trigger relay.
+    const int64_t near_future = now + 3550;
+    CGovernanceObject govobj = MakeFailedTrigger(near_future, /*salt=*/0);
+    const uint256 hash = govobj.GetHash();
+
+    WITH_LOCK(::cs_main, BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj)));
+
+    // The object is stored (AddTrigger failure happens after insertion) but is
+    // immediately marked for deletion, so it is not syncable...
+    BOOST_REQUIRE(m_node.govman->HaveObjectForHash(hash));
+    BOOST_CHECK(!m_node.govman->HaveSyncableObjectForHash(hash));
+    // ...and nothing was queued for relay on the accept path either.
+    BOOST_CHECK(m_node.govman->FetchRelayInventory().empty());
+
+    // Move past RELIABLE_PROPAGATION_TIME so the deferred relay would be "ready",
+    // then run the pass. UpdatedBlockTip -> CheckPostponedObjects drains
+    // setAdditionalRelayObjects.
+    SetMockTime(std::chrono::seconds{now + 120});
+    m_node.govman->UpdatedBlockTip(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()));
+
+    const auto invs = m_node.govman->FetchRelayInventory();
+    BOOST_CHECK(std::ranges::none_of(invs, [&](const CInv& inv) { return inv.hash == hash; }));
+    BOOST_CHECK(invs.empty());
+}
+
+
 BOOST_AUTO_TEST_SUITE_END()