diff --git a/src/test/overlay/base_squelch_test.cpp b/src/test/overlay/base_squelch_test.cpp index b1679506db..2a951d925b 100644 --- a/src/test/overlay/base_squelch_test.cpp +++ b/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); diff --git a/src/test/overlay/enhanced_squelch_test.cpp b/src/test/overlay/enhanced_squelch_test.cpp index 5e57bb983f..d2f3c9aee7 100644 --- a/src/test/overlay/enhanced_squelch_test.cpp +++ b/src/test/overlay/enhanced_squelch_test.cpp @@ -19,14 +19,13 @@ #include +#include #include #include #include #include -#include "xrpld/overlay/Peer.h" - #include #include #include diff --git a/src/test/overlay/squelch_store_test.cpp b/src/test/overlay/squelch_store_test.cpp index 3ca7d7c9ee..a3a94c2cbb 100644 --- a/src/test/overlay/squelch_store_test.cpp +++ b/src/test/overlay/squelch_store_test.cpp @@ -19,19 +19,33 @@ #include +#include #include #include #include -#include "xrpld/overlay/ReduceRelayCommon.h" - #include namespace ripple { namespace test { +class TestSquelchStore : public reduce_relay::SquelchStore +{ +public: + TestSquelchStore(beast::Journal journal, TestStopwatch& clock) + : reduce_relay::SquelchStore(journal, clock) + { + } + + hash_map 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(); } }; diff --git a/src/xrpld/overlay/Slot.h b/src/xrpld/overlay/Slot.h index c365bf8bae..2e4b94db52 100644 --- a/src/xrpld/overlay/Slot.h +++ b/src/xrpld/overlay/Slot.h @@ -885,6 +885,7 @@ protected: { count = 0; peers.clear(); + lastMessage = time_point{}; } }; diff --git a/src/xrpld/overlay/SquelchStore.h b/src/xrpld/overlay/SquelchStore.h index d27a4a2a1f..4d0bc03921 100644 --- a/src/xrpld/overlay/SquelchStore.h +++ b/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 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 squelched_; + void + removeExpired(); /** * @brief The logging interface used by this store. diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index 80211c27e2..8942005b2f 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -1518,7 +1518,7 @@ OverlayImpl::handleUntrustedSquelch(PublicKey const& validator) auto total = 0; for_each([&](std::shared_ptr&& p) { ++total; - if (p->expireAndIsSquelched(validator)) + if (p->isSquelched(validator)) ++count; }); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 624baadd2b..024a26d33b 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -44,8 +45,6 @@ #include #include -#include "xrpld/overlay/Peer.h" - #include #include #include @@ -251,7 +250,7 @@ PeerImp::send(std::shared_ptr 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); } //------------------------------------------------------------------------------ diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index eab8d24802..3e657e33db 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/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 diff --git a/src/xrpld/overlay/detail/SquelchStore.cpp b/src/xrpld/overlay/detail/SquelchStore.cpp index 229db9ca16..a4e5dc8e5c 100644 --- a/src/xrpld/overlay/detail/SquelchStore.cpp +++ b/src/xrpld/overlay/detail/SquelchStore.cpp @@ -27,7 +27,7 @@ #include #include -#include +#include 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