Merge #7521: fix: update masternode rate limit on failed governance trigger path

a551177 fix: update masternode rate limit on failed governance trigger path (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  In `AddGovernanceObjectInternal()`, a trigger object is emplaced into `mapObjects`, then `AddTrigger()` is attempted. On failure the function calls `PrepareDeletion()` and returns early, before `MasternodeRateUpdate()`.

  `MasternodeRateCheck()` allows any masternode that has no entry in `mapLastMasternodeObject`, and only `MasternodeRateUpdate()` creates that entry. A masternode that never lands a *successful* trigger therefore never gets a rate-buffer entry, and every malformed but signed trigger it sends passes the rate check. That defeats the limiter meant to stop exactly this.

  Each rejected trigger still costs a BLS verification, a `mapObjects` entry held for the deletion delay, and, after erasure, a `mapErasedGovernanceObjects` entry retained for about two superblock cycles (roughly 60 days on mainnet).

  Sending one needs a valid operator key for a masternode in the tip list, and the victim must have requested the hash after an INV. Rejected triggers are not relayed, so there is no fan-out; the attacker has to connect to each victim directly.

  ## What was done?

  - Call `MasternodeRateUpdate()` before the `AddTrigger` check, so failed attempts advance the masternode's rate buffer too.
  - `MasternodeRateUpdate()` also queued near-future triggers for the deferred re-announcement in `CheckPostponedObjects`. That part is split out into `ScheduleTriggerRelay()` and still runs only after `AddTrigger` succeeds. Otherwise moving the rate update would make the node re-announce, and serve on GETDATA, a trigger it had just undone and marked for deletion.

  ### Scope note: creation-time rate limiting on short-cycle networks

  The rate buffer stores the object's signer-chosen creation time, and `MasternodeRateCheck` accepts creation times from `now - 2 * cycle` to `now + 1h`, with a maximum rate of `2.2 / cycle` over five entries. On mainnet (cycle = 16616 blocks, about 28.8 days) the widest possible span is about `2 * cycle`, so a full buffer is always rejected (`5 / (2 * cycle)` = `2.5 / cycle` > `2.2 / cycle`). On testnet, devnet (1 h cycle) and regtest the accepted window is wider than `5 / 2.2` cycles, so an operator who alternates creation times between the window ends can stay under the rate. That weakness is in the existing limiter and applies equally to well-formed triggers today. This PR only brings failed triggers under the same limiter. Moving the limiter to receipt time would change how valid triggers are throttled and is left for a separate change.

  ## How Has This Been Tested?

  Two new cases in `src/test/governance_vote_processing_tests.cpp`, using its existing chain-backed fixture, which already registers a masternode with a known operator key. No new file or fixture, and `governance_inv_tests` is unchanged.

  - `failed_triggers_advance_the_masternode_rate_limit`: submits 12 signed triggers that fail `AddTrigger` and checks that exactly `RATE_BUFFER_SIZE - 1` (4) are stored. It fails with `12 != 4` on develop's `governance.cpp`/`governance.h`.
  - `failed_trigger_is_not_scheduled_for_deferred_relay`: a failed trigger created inside the deferred-relay window is never announced, even after it ages past `RELIABLE_PROPAGATION_TIME`. It fails when `ScheduleTriggerRelay()` is called before `AddTrigger`, which is the regression the split prevents.

  Run locally on macOS arm64 with `--enable-debug --enable-werror`:
  - `src/test/test_dash --run_test=governance_vote_processing_tests,governance_inv_tests,governance_superblock_tests,governance_validators_tests` passes.
  - `test/functional/test_runner.py feature_governance.py feature_governance_objects.py feature_governance_cl.py` passes.
  - `test/lint/lint-whitespace.py` passes.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] 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

Top commit has no ACKs.

Tree-SHA512: 2892edb480e2c3c63c1be7d231d745ecea2d2dcf6161f48ec0cd3f3e6484a120af77de540db06e3e7892840dc6a1fb234df98025686fb27a121731a7996338ac

Author: PastaPastaPasta

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
@@ -705,15 +712,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

@@ -398,6 +398,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/deterministicmns.h>
 #include <governance/governance.h>
@@ -59,6 +60,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
@@ -485,4 +500,50 @@ BOOST_AUTO_TEST_CASE(legacy_governance_cache_is_discarded)
     BOOST_CHECK(saved.empty());
 }
 
+// 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()