diff --git a/src/tests/libxrpl/telemetry/ValidationTracker.cpp b/src/tests/libxrpl/telemetry/ValidationTracker.cpp index 05dcec22c0..9a4d3d3b4b 100644 --- a/src/tests/libxrpl/telemetry/ValidationTracker.cpp +++ b/src/tests/libxrpl/telemetry/ValidationTracker.cpp @@ -287,3 +287,74 @@ TEST_F(ValidationTrackerTest, OnlyWeValidated) EXPECT_EQ(tracker_.missed1h(), 1u); EXPECT_DOUBLE_EQ(tracker_.agreementPct1h(), 0.0); } + +// --------------------------------------------------------------- +// 10. A counted ledger is never counted twice +// reconcile() drops the oldest reconciled events once the +// pending map passes kMaxPendingEvents. A validation arriving +// for one of those ledgers afterwards must not reach the +// agreement or missed totals a second time. +// +// Two hashes are made the oldest so the trim drops both: +// - evictedMiss (network only) reconciles as a miss. Our late +// validation cannot repair an entry the trim dropped, so +// totalMissed staying at 1 proves the trim really dropped +// it. A fixture where the trim did not run would repair it +// and report 0 misses. +// - evictedAgreed (both sides) reconciles as an agreement. +// Re-recording both sides is what double-counts an +// agreement. +// --------------------------------------------------------------- +TEST_F(ValidationTrackerTest, CountedLedgerNotCountedTwice) +{ + // The trim drops the oldest reconciled entries first. Each pause makes + // the next record time strictly larger, so these two are the oldest. + auto const evictedMiss = makeHash(1); + tracker_.recordNetworkValidation(evictedMiss, 1); + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + + auto const evictedAgreed = makeHash(2); + tracker_.recordOurValidation(evictedAgreed, 2); + tracker_.recordNetworkValidation(evictedAgreed, 2); + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + + // Fill to three over the bound so the trim drops three entries: the two + // above plus one filler. + constexpr std::size_t kFill = ValidationTracker::kMaxPendingEvents + 1; + for (std::size_t i = 0; i < kFill; ++i) + { + auto const hash = makeHash(i + 3); + auto const seq = static_cast(i + 3); + tracker_.recordOurValidation(hash, seq); + tracker_.recordNetworkValidation(hash, seq); + } + + std::this_thread::sleep_for(std::chrono::seconds(9)); + tracker_.reconcile(); + + // Every filler plus evictedAgreed agrees; evictedMiss is the one miss. + EXPECT_EQ(tracker_.totalAgreements(), kFill + 1); + EXPECT_EQ(tracker_.totalMissed(), 1u); + EXPECT_EQ(tracker_.agreements1h(), kFill + 1); + EXPECT_EQ(tracker_.missed1h(), 1u); + + // Validations arrive again for the two dropped ledgers. + tracker_.recordOurValidation(evictedMiss, 1); + tracker_.recordOurValidation(evictedAgreed, 2); + tracker_.recordNetworkValidation(evictedAgreed, 2); + + // Long enough for a re-created pending entry to pass the grace period. + std::this_thread::sleep_for(std::chrono::seconds(9)); + tracker_.reconcile(); + + // Both ledgers were already counted, so every total is unchanged. + EXPECT_EQ(tracker_.totalAgreements(), kFill + 1); + EXPECT_EQ(tracker_.totalMissed(), 1u); + EXPECT_EQ(tracker_.agreements1h(), kFill + 1); + EXPECT_EQ(tracker_.missed1h(), 1u); + + // The send and check counters count messages, not ledgers, so the + // repeated validations do count towards them. + EXPECT_EQ(tracker_.totalValidationsSent(), kFill + 3); + EXPECT_EQ(tracker_.totalValidationsChecked(), kFill + 3); +} diff --git a/src/xrpld/telemetry/ValidationTracker.h b/src/xrpld/telemetry/ValidationTracker.h index 278332ad8e..3c4d26dc6e 100644 --- a/src/xrpld/telemetry/ValidationTracker.h +++ b/src/xrpld/telemetry/ValidationTracker.h @@ -47,6 +47,7 @@ namespace xrpl::telemetry { * | ValidationTracker | * |---------------------------| * | pending_ (hash_map) |----> LedgerEvent per hash + * | tallied_ (hash_set) |----> hashes already counted * | window1h_ (deque) |----> WindowEvent sliding window * | window24h_ (deque) |----> WindowEvent sliding window * | atomic totals | @@ -104,6 +105,13 @@ public: */ using TimePoint = Clock::time_point; + /** + * Maximum number of pending (unreconciled + recently reconciled) events. + * Once the pending map passes this size, reconcile() drops the oldest + * reconciled events. Public so a test can size a fixture against it. + */ + static constexpr std::size_t kMaxPendingEvents = 1000; + /** * Record that this node sent a validation for the given ledger. * @param ledgerHash Hash of the ledger we validated. @@ -267,9 +275,12 @@ private: static constexpr auto kLateRepairWindow = std::chrono::minutes(5); /** - * Maximum number of pending (unreconciled + recently reconciled) events. + * Maximum number of ledger hashes remembered as already counted. + * At one ledger every four seconds this spans about eleven hours. + * A validation arriving for a ledger counted before that is counted + * again. */ - static constexpr std::size_t kMaxPendingEvents = 1000; + static constexpr std::size_t kMaxTalliedEvents = 10000; /** * Duration of the short rolling window. @@ -287,7 +298,8 @@ private: static constexpr auto kWindow7d = std::chrono::hours(168); /** - * Protects pending_, window1h_, window24h_, and window7d_. + * Protects pending_, tallied_, talliedOrder_, window1h_, window24h_, + * and window7d_. */ mutable std::mutex mutex_; @@ -296,6 +308,19 @@ private: */ hash_map pending_; + /** + * Ledger hashes already counted into the agreement and missed totals. + * Membership survives eviction from pending_, so a ledger reaches the + * totals once. Holds at most kMaxTalliedEvents hashes. + */ + hash_set tallied_; + + /** + * The hashes in tallied_ in the order they were counted. The front is + * the oldest and is dropped first once the bound is reached. + */ + std::deque talliedOrder_; + /** * Sliding window of reconciled events (last 1 hour). */ @@ -331,6 +356,27 @@ private: */ std::atomic totalValidationsChecked_{0}; + /** + * Locate the pending event for a ledger, creating it on first sight. + * @param ledgerHash Hash of the ledger being recorded. + * @param seq Ledger sequence number, stored only on creation. + * @return Pointer to the event, or nullptr for a ledger that already + * reached the totals and left pending_. The caller records nothing in + * that case. + * @note Called with mutex_ held. + */ + [[nodiscard]] LedgerEvent* + pendingEvent(uint256 const& ledgerHash, LedgerIndex seq); + + /** + * Remember a ledger hash as counted, dropping the oldest remembered + * hash once kMaxTalliedEvents is reached. + * @param ledgerHash Hash of the ledger just counted into the totals. + * @note Called with mutex_ held. + */ + void + noteTallied(uint256 const& ledgerHash); + /** * Remove entries older than their respective window durations. * @param now Current time point. diff --git a/src/xrpld/telemetry/detail/ValidationTracker.cpp b/src/xrpld/telemetry/detail/ValidationTracker.cpp index b198913959..1609cf696c 100644 --- a/src/xrpld/telemetry/detail/ValidationTracker.cpp +++ b/src/xrpld/telemetry/detail/ValidationTracker.cpp @@ -17,35 +17,58 @@ namespace xrpl::telemetry { +ValidationTracker::LedgerEvent* +ValidationTracker::pendingEvent(uint256 const& ledgerHash, LedgerIndex seq) +{ + if (auto const it = pending_.find(ledgerHash); it != pending_.end()) + return &it->second; + + // A hash in tallied_ already reached the totals and left pending_. + // Building a fresh record for it would count the same ledger twice. + if (tallied_.contains(ledgerHash)) + return nullptr; + + auto& evt = pending_[ledgerHash]; + evt.ledgerHash = ledgerHash; + evt.seq = seq; + evt.recordTime = Clock::now(); + return &evt; +} + +void +ValidationTracker::noteTallied(uint256 const& ledgerHash) +{ + if (!tallied_.insert(ledgerHash).second) + return; + + talliedOrder_.push_back(ledgerHash); + while (talliedOrder_.size() > kMaxTalliedEvents) + { + tallied_.erase(talliedOrder_.front()); + talliedOrder_.pop_front(); + } +} + void ValidationTracker::recordOurValidation(uint256 const& ledgerHash, LedgerIndex seq) { std::scoped_lock const lock(mutex_); - auto& evt = pending_[ledgerHash]; - if (evt.recordTime == TimePoint{}) - { - // First time seeing this ledger hash -- initialize. - evt.ledgerHash = ledgerHash; - evt.seq = seq; - evt.recordTime = Clock::now(); - } - evt.weValidated = true; totalValidationsSent_.fetch_add(1, std::memory_order_relaxed); + + // The counter above counts messages, so it also counts a ledger that is + // already tallied. Only the per-ledger record is skipped. + if (auto* const evt = pendingEvent(ledgerHash, seq)) + evt->weValidated = true; } void ValidationTracker::recordNetworkValidation(uint256 const& ledgerHash, LedgerIndex seq) { std::scoped_lock const lock(mutex_); - auto& evt = pending_[ledgerHash]; - if (evt.recordTime == TimePoint{}) - { - evt.ledgerHash = ledgerHash; - evt.seq = seq; - evt.recordTime = Clock::now(); - } - evt.networkValidated = true; totalValidationsChecked_.fetch_add(1, std::memory_order_relaxed); + + if (auto* const evt = pendingEvent(ledgerHash, seq)) + evt->networkValidated = true; } void @@ -61,6 +84,7 @@ ValidationTracker::reconcile() // Initial reconciliation after grace period. evt.reconciled = true; evt.agreed = evt.weValidated && evt.networkValidated; + noteTallied(hash); if (evt.agreed) {