fix: update masternode rate limit on failed governance trigger path

src/governance/governance.cpp

@@ -345,18 +345,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 before AddTrigger can reject it: MasternodeRateCheck allows any
+    // masternode without a rate-buffer entry, so a key whose triggers all fail AddTrigger
+    // would otherwise never be throttled.
+    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)
     const COutPoint& masternodeOutpoint = govobj.GetMasternodeOutpoint();
     auto it = mapLastMasternodeObject.try_emplace(masternodeOutpoint, /*fStatusOKIn=*/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

@@ -404,6 +404,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_vote_processing_tests.cpp

@@ -2,6 +2,7 @@
 // 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 <consensus/amount.h>
 #include <evo/chainhelper.h>
 #include <evo/deterministicmns.h>
@@ -60,6 +61,20 @@ void SignWithOperatorKey(CGovernanceVote& vote, const CBLSSecretKey& key)
         key.Sign(vote.GetSignatureHash(), /*specificLegacyScheme=*/false).ToByteVector(/*specificLegacyScheme=*/false));
 }
 
+//! A trigger that is signed by the fixture masternode and passes IsValidLocally, but whose payload
+//! is not a superblock, so SuperblockManager::AddTrigger rejects it after it was inserted.
+CGovernanceObject MakeFailedTrigger(const COutPoint& mn_collateral, const CBLSSecretKey& operator_key,
+                                    int64_t creation_time, int salt)
+{
+    const std::string data{strprintf(R"({"type":2,"salt":%d})", salt)};
+    CGovernanceObject govobj{uint256{}, /*revision=*/1, creation_time, uint256{}, HexStr(data)};
+    BOOST_REQUIRE(govobj.GetObjectType() == GovernanceObject::TRIGGER);
+    govobj.SetMasternodeOutpoint(mn_collateral);
+    govobj.SetSignature(operator_key.Sign(govobj.GetSignatureHash(), /*specificLegacyScheme=*/false)
+                            .ToByteVector(/*specificLegacyScheme=*/false));
+    return govobj;
+}
+
 // A chain with one registered masternode whose voting (ECDSA) and operator (BLS) keys are known
 // to the test, so votes can be signed for real and CGovernanceVote::CheckSignature is actually
 // reached. Without a populated masternode list every CGovernanceVote::IsValid() call would
@@ -473,4 +488,50 @@ BOOST_AUTO_TEST_CASE(legacy_invalid_vote_cache_is_discarded)
     BOOST_CHECK_EQUAL(saved_invalid_votes.GetSize(), 0U);
 }
 
+// MasternodeRateCheck allows a masternode that has no rate-buffer entry yet, and only accepted
+// triggers used to create one. A key whose triggers all fail AddTrigger was therefore never
+// throttled: every one cost a BLS verification and a mapErasedGovernanceObjects entry.
+BOOST_AUTO_TEST_CASE(failed_triggers_advance_the_masternode_rate_limit)
+{
+    const int64_t now{GetTime()};
+    constexpr int attempts{12};
+    int stored{0};
+    for (int i = 0; i < attempts; ++i) {
+        CGovernanceObject govobj{MakeFailedTrigger(mn_collateral, mn_operator_key, now + i, /*salt=*/i)};
+        const uint256 hash{govobj.GetHash()};
+        // Rate rejection is not misbehaviour, so ProcessObject reports success either way.
+        WITH_LOCK(::cs_main, BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj)));
+        if (m_node.govman->HaveObjectForHash(hash)) {
+            ++stored;
+            BOOST_CHECK(Assert(m_node.govman->FindGovernanceObject(hash))->IsSetCachedDelete());
+        }
+    }
+
+    // The rate is only computed over a full buffer, so the attempt that would fill it is the
+    // first one rated, and these timestamps are far too close together to pass.
+    BOOST_CHECK_EQUAL(stored, RATE_BUFFER_SIZE - 1);
+    BOOST_CHECK(m_node.govman->FetchRelayInventory().empty());
+}
+
+// Rate accounting now runs before AddTrigger, but scheduling the deferred re-announcement of a
+// trigger created close to the future-deviation limit must not: a trigger that AddTrigger rejected
+// would be announced, and served on GETDATA, once it aged past RELIABLE_PROPAGATION_TIME.
+BOOST_AUTO_TEST_CASE(failed_trigger_is_not_scheduled_for_deferred_relay)
+{
+    const int64_t now{GetTime()};
+    // Inside (now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME, now + MAX_TIME_FUTURE_DEVIATION],
+    // i.e. 1h - 60s < offset <= 1h: accepted by MasternodeRateCheck, too new to propagate reliably.
+    CGovernanceObject govobj{MakeFailedTrigger(mn_collateral, mn_operator_key, now + 3550, /*salt=*/0)};
+    const uint256 hash{govobj.GetHash()};
+    WITH_LOCK(::cs_main, BOOST_CHECK(m_node.govman->ProcessObject(/*peer_str=*/"test-peer", hash, govobj)));
+    BOOST_REQUIRE(m_node.govman->HaveObjectForHash(hash));
+    BOOST_CHECK(!m_node.govman->HaveSyncableObjectForHash(hash));
+    BOOST_CHECK(m_node.govman->FetchRelayInventory().empty());
+
+    // Age it past RELIABLE_PROPAGATION_TIME and run the deferred-relay pass.
+    SetMockTime(std::chrono::seconds{now + 120});
+    m_node.govman->UpdatedBlockTip(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()));
+    BOOST_CHECK(m_node.govman->FetchRelayInventory().empty());
+}
+
 BOOST_AUTO_TEST_SUITE_END()