improves expired squelch deletion

src/test/overlay/base_squelch_test.cpp

@@ -483,7 +483,7 @@ public:
     {
         auto validator = m->getValidatorKey();
         assert(validator);
-        if (squelchStore_.expireAndIsSquelched(*validator))
+        if (squelchStore_.isSquelched(*validator))
             return;
 
         overlay_.updateSlotAndSquelch({}, *validator, id(), f);

src/test/overlay/enhanced_squelch_test.cpp

@@ -19,14 +19,13 @@
 
 #include <test/jtx/Env.h>
 
+#include <xrpld/overlay/Peer.h>
 #include <xrpld/overlay/ReduceRelayCommon.h>
 #include <xrpld/overlay/Slot.h>
 
 #include <xrpl/beast/unit_test.h>
 #include <xrpl/protocol/SecretKey.h>
 
-#include "xrpld/overlay/Peer.h"
-
 #include <chrono>
 #include <cstdint>
 #include <functional>

src/test/overlay/squelch_store_test.cpp

@@ -19,19 +19,33 @@
 
 #include <test/jtx/Env.h>
 
+#include <xrpld/overlay/ReduceRelayCommon.h>
 #include <xrpld/overlay/SquelchStore.h>
 
 #include <xrpl/beast/unit_test.h>
 #include <xrpl/protocol/PublicKey.h>
 
-#include "xrpld/overlay/ReduceRelayCommon.h"
-
 #include <chrono>
 
 namespace ripple {
 
 namespace test {
 
+class TestSquelchStore : public reduce_relay::SquelchStore
+{
+public:
+    TestSquelchStore(beast::Journal journal, TestStopwatch& clock)
+        : reduce_relay::SquelchStore(journal, clock)
+    {
+    }
+
+    hash_map<PublicKey, TestStopwatch::time_point> const&
+    getSquelched() const
+    {
+        return squelched_;
+    }
+};
+
 class squelch_store_test : public beast::unit_test::suite
 {
     using seconds = std::chrono::seconds;
@@ -49,7 +63,7 @@ public:
         testcase("SquelchStore handleSquelch");
 
         TestStopwatch clock;
-        auto store = reduce_relay::SquelchStore(env_.journal, clock);
+        auto store = TestSquelchStore(env_.journal, clock);
 
         auto const validator = randomKeyPair(KeyType::ed25519).first;
 
@@ -58,8 +72,7 @@ public:
             validator, true, reduce_relay::MIN_UNSQUELCH_EXPIRE - seconds{1});
 
         // the peer must not be squelched
-        BEAST_EXPECTS(
-            !store.expireAndIsSquelched(validator), "peer is squelched");
+        BEAST_EXPECTS(!store.isSquelched(validator), "peer is squelched");
 
         // attempt to squelch the peer with a too big duration
         store.handleSquelch(
@@ -68,8 +81,7 @@ public:
             reduce_relay::MAX_UNSQUELCH_EXPIRE_PEERS + seconds{1});
 
         // the peer must not be squelched
-        BEAST_EXPECTS(
-            !store.expireAndIsSquelched(validator), "peer is squelched");
+        BEAST_EXPECTS(!store.isSquelched(validator), "peer is squelched");
 
         // squelch the peer with a good duration
         store.handleSquelch(
@@ -77,22 +89,21 @@ public:
 
         // the peer for the validator should be squelched
         BEAST_EXPECTS(
-            store.expireAndIsSquelched(validator),
+            store.isSquelched(validator),
             "peer and validator are not squelched");
 
         // unsquelch the validator
         store.handleSquelch(validator, false, seconds{0});
 
-        BEAST_EXPECTS(
-            !store.expireAndIsSquelched(validator), "peer is squelched");
+        BEAST_EXPECTS(!store.isSquelched(validator), "peer is squelched");
     }
 
     void
-    testExpireAndIsSquelched()
+    testIsSquelched()
     {
-        testcase("SquelchStore expireAndIsSquelched");
+        testcase("SquelchStore IsSquelched");
         TestStopwatch clock;
-        auto store = reduce_relay::SquelchStore(env_.journal, clock);
+        auto store = TestSquelchStore(env_.journal, clock);
 
         auto const validator = randomKeyPair(KeyType::ed25519).first;
         auto const duration = reduce_relay::MIN_UNSQUELCH_EXPIRE + seconds{1};
@@ -100,21 +111,50 @@ public:
         store.handleSquelch(
             validator, true, reduce_relay::MIN_UNSQUELCH_EXPIRE + seconds{1});
         BEAST_EXPECTS(
-            store.expireAndIsSquelched(validator),
+            store.isSquelched(validator),
             "peer and validator are not squelched");
 
         clock.advance(duration + seconds{1});
 
         // the peer with short squelch duration must be not squelched
         BEAST_EXPECTS(
-            !store.expireAndIsSquelched(validator),
-            "peer and validator are squelched");
+            !store.isSquelched(validator), "peer and validator are squelched");
+    }
+
+    void
+    testClearExpiredSquelches()
+    {
+        testcase("SquelchStore testClearExpiredSquelches");
+        TestStopwatch clock;
+        auto store = TestSquelchStore(env_.journal, clock);
+
+        auto const validator = randomKeyPair(KeyType::ed25519).first;
+        auto const duration = reduce_relay::MIN_UNSQUELCH_EXPIRE + seconds{1};
+        store.handleSquelch(validator, true, duration);
+        BEAST_EXPECTS(
+            store.getSquelched().size() == 1,
+            "validators were not registered in the store");
+
+        clock.advance(duration + seconds{1});
+
+        auto const validator2 = randomKeyPair(KeyType::ed25519).first;
+        auto const duration2 = reduce_relay::MIN_UNSQUELCH_EXPIRE + seconds{2};
+        store.handleSquelch(validator2, true, duration2);
+
+        BEAST_EXPECTS(
+            !store.getSquelched().contains(validator),
+            "expired squelch was not deleted");
+
+        BEAST_EXPECTS(
+            store.getSquelched().contains(validator2),
+            "validators were not registered in the store");
     }
     void
     run() override
     {
         testHandleSquelch();
-        testExpireAndIsSquelched();
+        testIsSquelched();
+        testClearExpiredSquelches();
     }
 };
 

src/xrpld/overlay/Slot.h

@@ -885,6 +885,7 @@ protected:
         {
             count = 0;
             peers.clear();
+            lastMessage = time_point{};
         }
     };
 

src/xrpld/overlay/SquelchStore.h

@@ -60,7 +60,7 @@ public:
      * @details This function acts as the primary public interface for
      * controlling a validator's squelch state. Based on the `squelch` flag, it
      * either adds a new squelch entry for the specified duration or removes an
-     * existing one.
+     * existing one. This function also clears all expired squelches.
      *
      * @param validator The public key of the validator to manage.
      * @param squelch If `true`, the validator will be squelched. If `false`,
@@ -75,22 +75,24 @@ public:
         std::chrono::seconds duration);
 
     /**
-     * @brief Checks if a validator is currently squelched, cleaning up expired
-     * entries.
+     * @brief Checks if a validator is currently squelched.
      *
-     * @details This function performs two roles: it first checks if the
-     * validator's squelch period has expired and, if so, removes the entry from
-     * the store. It then returns the current squelch status.
-     *
-     * @note Because this function has the side effect of removing expired
-     * entries, its result reflects the state *after* the potential cleanup.
+     * @details This function checks if the validator's squelch has expired.
      *
      * @param validator The public key of the validator to check.
      * @return `true` if a non-expired squelch entry exists for the
      * validator, `false` otherwise.
      */
     bool
-    expireAndIsSquelched(PublicKey const& validator);
+    isSquelched(PublicKey const& validator) const;
+
+    // The following field is protected for unit tests.
+protected:
+    /**
+     * @brief The core data structure mapping a validator's public key to the
+     * time point when their squelch expires.
+     */
+    hash_map<PublicKey, time_point> squelched_;
 
 private:
     /**
@@ -120,10 +122,12 @@ private:
     remove(PublicKey const& validator);
 
     /**
-     * @brief The core data structure mapping a validator's public key to the
-     * time point when their squelch expires.
+     * @brief Internal implementation to remove all expired squelches.
+     *
+     * @details Erases all squelch entries whose expiration is in the past.
      */
-    hash_map<PublicKey, time_point> squelched_;
+    void
+    removeExpired();
 
     /**
      * @brief The logging interface used by this store.

src/xrpld/overlay/detail/OverlayImpl.cpp

@@ -1518,7 +1518,7 @@ OverlayImpl::handleUntrustedSquelch(PublicKey const& validator)
     auto total = 0;
     for_each([&](std::shared_ptr<PeerImp>&& p) {
         ++total;
-        if (p->expireAndIsSquelched(validator))
+        if (p->isSquelched(validator))
             ++count;
     });
 

src/xrpld/overlay/detail/PeerImp.cpp

@@ -29,6 +29,7 @@
 #include <xrpld/app/misc/ValidatorList.h>
 #include <xrpld/app/tx/apply.h>
 #include <xrpld/overlay/Cluster.h>
+#include <xrpld/overlay/Peer.h>
 #include <xrpld/overlay/ReduceRelayCommon.h>
 #include <xrpld/overlay/detail/PeerImp.h>
 #include <xrpld/overlay/detail/Tuning.h>
@@ -44,8 +45,6 @@
 #include <boost/algorithm/string/predicate.hpp>
 #include <boost/beast/core/ostream.hpp>
 
-#include "xrpld/overlay/Peer.h"
-
 #include <algorithm>
 #include <chrono>
 #include <memory>
@@ -251,7 +250,7 @@ PeerImp::send(std::shared_ptr<Message> const& m)
         return;
 
     auto const validator = m->getValidatorKey();
-    if (validator && expireAndIsSquelched(*validator))
+    if (validator && isSquelched(*validator))
     {
         overlay_.reportOutboundTraffic(
             TrafficCount::category::squelch_suppressed,
@@ -575,9 +574,9 @@ PeerImp::hasRange(std::uint32_t uMin, std::uint32_t uMax)
 }
 
 bool
-PeerImp::expireAndIsSquelched(PublicKey const& validator)
+PeerImp::isSquelched(PublicKey const& validator) const
 {
-    return squelchStore_.expireAndIsSquelched(validator);
+    return squelchStore_.isSquelched(validator);
 }
 
 //------------------------------------------------------------------------------

src/xrpld/overlay/detail/PeerImp.h

@@ -440,13 +440,12 @@ public:
         return txReduceRelayEnabled_;
     }
 
-    /** Check if a given validator is squelched. If the validator is no longer
-     * squelched, clear the squelch entry.
+    /** Check if a given validator is squelched.
      * @param validator Validator's public key
      * @return true if squelch exists and it is not expired. False otherwise.
      */
     bool
-    expireAndIsSquelched(PublicKey const& validator);
+    isSquelched(PublicKey const& validator) const;
 
 private:
     void

src/xrpld/overlay/detail/SquelchStore.cpp

@@ -27,7 +27,7 @@
 
 #include <chrono>
 #include <unordered_map>
-#include <utility>
+#include <vector>
 
 namespace ripple {
 
@@ -39,6 +39,10 @@ SquelchStore::handleSquelch(
     bool squelch,
     std::chrono::seconds duration)
 {
+    // Remove all expired squelches. This call is here, as it is on the least
+    // critical execution path, that does not require periodic cleanup calls.
+    removeExpired();
+
     if (squelch)
     {
         // This should never trigger. The squelh duration is validated in
@@ -61,7 +65,7 @@ SquelchStore::handleSquelch(
 }
 
 bool
-SquelchStore::expireAndIsSquelched(PublicKey const& validator)
+SquelchStore::isSquelched(PublicKey const& validator) const
 {
     auto const now = clock_.now();
 
@@ -69,13 +73,7 @@ SquelchStore::expireAndIsSquelched(PublicKey const& validator)
     if (it == squelched_.end())
         return false;
 
-    if (it->second > now)
-        return true;
-
-    // erase the entry if the squelch expired
-    squelched_.erase(it);
-
-    return false;
+    return it->second > now;
 }
 
 void
@@ -92,6 +90,14 @@ SquelchStore::remove(PublicKey const& validator)
     squelched_.erase(validator);
 }
 
+void
+SquelchStore::removeExpired()
+{
+    auto const now = clock_.now();
+    std::erase_if(
+        squelched_, [&](auto const& entry) { return entry.second < now; });
+}
+
 }  // namespace reduce_relay
 
 }  // namespace ripple