From 7eb4cf03bbabc2733a52b0720dda0305ccb1732b Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 22 Aug 2026 22:00:40 -0400 Subject: [PATCH] test: Read a SHAMapAddNode verdict as counts SHAMapAddNode gains getBad() and getDuplicate() beside getGood(), so a verdict can be read as counts instead of just a log string. get()'s wording is pinned by src/tests/libxrpl/shamap/SHAMapAddNode.cpp, the one place that depends on it. --- include/xrpl/shamap/SHAMapAddNode.h | 179 +++++++++++++++++---- src/tests/libxrpl/shamap/SHAMapAddNode.cpp | 81 ++++++++++ 2 files changed, 229 insertions(+), 31 deletions(-) create mode 100644 src/tests/libxrpl/shamap/SHAMapAddNode.cpp diff --git a/include/xrpl/shamap/SHAMapAddNode.h b/include/xrpl/shamap/SHAMapAddNode.h index 87222f74d8..c9e2bc5493 100644 --- a/include/xrpl/shamap/SHAMapAddNode.h +++ b/include/xrpl/shamap/SHAMapAddNode.h @@ -14,35 +14,140 @@ private: public: SHAMapAddNode(); + + /** + * Record one node that was rejected. + * + * Counted rather than merely flagged, so a batch that carries on past a + * rejected node reports one per node instead of just that it happened. + */ void incInvalid(); + + /** + * Record one node that was hooked into the map. + * + * Counted so a batch's tally can be read back through getGood(). + */ void incUseful(); + + /** + * Record one node the map already held. + * + * Counted separately from a useful node: it is not new data, but it is + * also not a rejection - see isGood(). + */ void incDuplicate(); - void - reset(); + + /** + * How many nodes were hooked into the map, which isUseful() only reports + * the presence of. + * + * @return The count. + */ [[nodiscard]] int getGood() const; + /** + * How many nodes were rejected, which isInvalid() only reports the presence + * of. A batch that stops on its first bad node counts one; one that carries + * on counts each. + * + * @return The count. + */ + [[nodiscard]] int + getBad() const; + + /** + * How many nodes the batch already held, which no other accessor reports: a + * duplicate counts as neither good nor bad. + * + * @return The count. + */ + [[nodiscard]] int + getDuplicate() const; + + /** + * Whether the batch overall was worth the exchange: nodes accepted or + * already held outnumber the ones rejected. + * + * A duplicate counts on the accepted side, since the peer answered a + * request rather than sent something unasked for; see incDuplicate(). + * + * @return Whether the batch was good. + */ [[nodiscard]] bool isGood() const; + + /** + * Whether any node in the batch was rejected. + * + * @return Whether at least one node was bad. + */ [[nodiscard]] bool isInvalid() const; + + /** + * Whether any node in the batch was hooked into the map. + * + * @return Whether at least one node was useful. + */ [[nodiscard]] bool isUseful() const; + + /** + * A verdict recording one duplicate node. + * + * @return The verdict. + */ + static SHAMapAddNode + duplicate(); + + /** + * A verdict recording one useful node. + * + * @return The verdict. + */ + static SHAMapAddNode + useful(); + + /** + * A verdict recording one invalid node. + * + * @return The verdict. + */ + static SHAMapAddNode + invalid(); + + /** + * Clear every count back to zero. + */ + void + reset(); + + /** + * Render the tally as a log line. + * + * A format rather than an API: a caller that needs the counts themselves + * should read them through getGood(), getBad() and getDuplicate() instead + * of parsing this. + * + * @return The tally, e.g. "good:2 bad:1 dupe:1", or "no nodes processed" if + * every count is zero. + */ [[nodiscard]] std::string get() const; + /** + * Add another verdict's counts into this one. + * + * @param n The verdict to add. + * @return This verdict, updated. + */ SHAMapAddNode& operator+=(SHAMapAddNode const& n); - static SHAMapAddNode - duplicate(); - static SHAMapAddNode - useful(); - static SHAMapAddNode - invalid(); - private: SHAMapAddNode(int good, int bad, int duplicate); }; @@ -74,18 +179,30 @@ SHAMapAddNode::incDuplicate() ++duplicate_; } -inline void -SHAMapAddNode::reset() -{ - good_ = bad_ = duplicate_ = 0; -} - inline int SHAMapAddNode::getGood() const { return good_; } +inline int +SHAMapAddNode::getBad() const +{ + return bad_; +} + +inline int +SHAMapAddNode::getDuplicate() const +{ + return duplicate_; +} + +inline bool +SHAMapAddNode::isGood() const +{ + return (good_ + duplicate_) > bad_; +} + inline bool SHAMapAddNode::isInvalid() const { @@ -98,22 +215,6 @@ SHAMapAddNode::isUseful() const return good_ > 0; } -inline SHAMapAddNode& -SHAMapAddNode::operator+=(SHAMapAddNode const& n) -{ - good_ += n.good_; - bad_ += n.bad_; - duplicate_ += n.duplicate_; - - return *this; -} - -inline bool -SHAMapAddNode::isGood() const -{ - return (good_ + duplicate_) > bad_; -} - inline SHAMapAddNode SHAMapAddNode::duplicate() { @@ -132,6 +233,12 @@ SHAMapAddNode::invalid() return SHAMapAddNode(0, 1, 0); } +inline void +SHAMapAddNode::reset() +{ + good_ = bad_ = duplicate_ = 0; +} + inline std::string SHAMapAddNode::get() const { @@ -160,4 +267,14 @@ SHAMapAddNode::get() const return ret; } +inline SHAMapAddNode& +SHAMapAddNode::operator+=(SHAMapAddNode const& n) +{ + good_ += n.good_; + bad_ += n.bad_; + duplicate_ += n.duplicate_; + + return *this; +} + } // namespace xrpl diff --git a/src/tests/libxrpl/shamap/SHAMapAddNode.cpp b/src/tests/libxrpl/shamap/SHAMapAddNode.cpp new file mode 100644 index 0000000000..0fb8f56be4 --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapAddNode.cpp @@ -0,0 +1,81 @@ +#include + +#include + +namespace xrpl::tests { + +// get() is a log format rather than an API, so it is pinned here, once, instead of at every site +// that has a verdict to check. Those check the tally through getGood()/getBad()/getDuplicate() +// (see tallyIs() in AcquireTestHelpers.h and SHAMapSync.cpp) or through +// isGood()/isUseful()/isInvalid(), which say the same thing without depending on the wording. +TEST(SHAMapAddNode, getNamesEveryNonEmptyCount) +{ + EXPECT_EQ(SHAMapAddNode{}.get(), "no nodes processed"); + EXPECT_EQ(SHAMapAddNode::useful().get(), "good:1"); + EXPECT_EQ(SHAMapAddNode::invalid().get(), "bad:1"); + EXPECT_EQ(SHAMapAddNode::duplicate().get(), "dupe:1"); + + // Several of a kind are counted, and the counts are joined in a fixed order with a single + // space, whichever order they were recorded in. + SHAMapAddNode san; + san.incInvalid(); + san.incUseful(); + san.incUseful(); + san.incDuplicate(); + EXPECT_EQ(san.get(), "good:2 bad:1 dupe:1"); + + san.reset(); + EXPECT_EQ(san.get(), "no nodes processed"); +} + +// The three counts the tests assert on, and the verdicts derived from them, so a tally check and +// the log line cannot drift apart. +TEST(SHAMapAddNode, countsAndVerdictsAgree) +{ + SHAMapAddNode san; + EXPECT_EQ(san.getGood(), 0); + EXPECT_EQ(san.getBad(), 0); + EXPECT_EQ(san.getDuplicate(), 0); + EXPECT_FALSE(san.isInvalid()); + EXPECT_FALSE(san.isUseful()); + + // Good counts what was hooked in, and useful is that count being non-zero. + san.incUseful(); + EXPECT_EQ(san.getGood(), 1); + EXPECT_TRUE(san.isUseful()); + EXPECT_TRUE(san.isGood()); + + // A duplicate counts towards good without needing to be useful itself: isUseful() here still + // reflects the incUseful() above, not this increment. + san.incDuplicate(); + EXPECT_EQ(san.getDuplicate(), 1); + EXPECT_FALSE(san.isInvalid()); + EXPECT_TRUE(san.isGood()); + + // Bad is counted, not merely flagged: a batch that carries on past a rejected node reports one + // per node, so a test can tell "stopped on the first" from "rejected several". + san.incInvalid(); + EXPECT_EQ(san.getBad(), 1); + EXPECT_TRUE(san.isInvalid()); + EXPECT_TRUE(san.isGood()) << "one bad node among two accepted ones is still a good batch"; + + san.incInvalid(); + san.incInvalid(); + EXPECT_EQ(san.getGood(), 1); + EXPECT_EQ(san.getBad(), 3); + EXPECT_EQ(san.getDuplicate(), 1); + EXPECT_FALSE(san.isGood()) << "more bad nodes than accepted ones is not"; + + // Adding one verdict to another sums every count, which is how a batch's verdict is built up + // one node at a time. + SHAMapAddNode total; + total += SHAMapAddNode::useful(); + total += SHAMapAddNode::invalid(); + total += SHAMapAddNode::invalid(); + total += SHAMapAddNode::duplicate(); + EXPECT_EQ(total.getGood(), 1); + EXPECT_EQ(total.getBad(), 2); + EXPECT_EQ(total.getDuplicate(), 1); +} + +} // namespace xrpl::tests