From 97e3dae6f4390cf7516ed1f797c1ba31b3e1aa4a Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 13 Feb 2025 11:54:01 -0500 Subject: [PATCH] fix: Replace charge() by fee_.update() in OnMessage functions (#5269) In PeerImpl.cpp, if the function is a message handler (onMessage) or called directly from a message handler, then it should use fee_, since when the handler returns (OnMessageEnd) then the charge function is called. If the function is not a message handler, such as a job queue item, it should remain charge. --- include/xrpl/resource/Charge.h | 3 ++ src/libxrpl/resource/Charge.cpp | 6 ++++ src/xrpld/overlay/detail/PeerImp.cpp | 53 +++++++++++++++++----------- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/include/xrpl/resource/Charge.h b/include/xrpl/resource/Charge.h index e5799710b2..a75ad32624 100644 --- a/include/xrpl/resource/Charge.h +++ b/include/xrpl/resource/Charge.h @@ -57,6 +57,9 @@ public: std::strong_ordering operator<=>(Charge const&) const; + Charge + operator*(value_type m) const; + private: value_type m_cost; std::string m_label; diff --git a/src/libxrpl/resource/Charge.cpp b/src/libxrpl/resource/Charge.cpp index 66b5049dbd..53df4e592d 100644 --- a/src/libxrpl/resource/Charge.cpp +++ b/src/libxrpl/resource/Charge.cpp @@ -67,5 +67,11 @@ Charge::operator<=>(Charge const& c) const return m_cost <=> c.m_cost; } +Charge +Charge::operator*(value_type m) const +{ + return Charge(m_cost * m, m_label); +} + } // namespace Resource } // namespace ripple diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index fe88fcfd2c..c3656c9445 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -31,14 +31,11 @@ #include #include #include -#include #include #include #include #include #include -#include -// #include #include #include @@ -1111,7 +1108,7 @@ PeerImp::onMessage(std::shared_ptr const& m) // VFALCO NOTE I think we should drop the peer immediately if (!cluster()) { - fee_.fee = Resource::feeUselessData; + fee_.update(Resource::feeUselessData, "unknown cluster"); return; } @@ -1189,13 +1186,14 @@ PeerImp::onMessage(std::shared_ptr const& m) // implication for the protocol. if (m->endpoints_v2().size() >= 1024) { - charge(Resource::feeInvalidData, "endpoints too large"); + fee_.update(Resource::feeUselessData, "endpoints too large"); return; } std::vector endpoints; endpoints.reserve(m->endpoints_v2().size()); + auto malformed = 0; for (auto const& tm : m->endpoints_v2()) { auto result = beast::IP::Endpoint::from_string_checked(tm.endpoint()); @@ -1204,7 +1202,7 @@ PeerImp::onMessage(std::shared_ptr const& m) { JLOG(p_journal_.error()) << "failed to parse incoming endpoint: {" << tm.endpoint() << "}"; - charge(Resource::feeInvalidData, "endpoints malformed"); + malformed++; continue; } @@ -1220,6 +1218,15 @@ PeerImp::onMessage(std::shared_ptr const& m) endpoints.emplace_back(*result, tm.hops()); } + // Charge the peer for each malformed endpoint. As there still may be + // multiple valid endpoints we don't return early. + if (malformed > 0) + { + fee_.update( + Resource::feeInvalidData * malformed, + std::to_string(malformed) + " malformed endpoints"); + } + if (!endpoints.empty()) overlay_.peerFinder().on_endpoints(slot_, endpoints); } @@ -1340,7 +1347,7 @@ void PeerImp::onMessage(std::shared_ptr const& m) { auto badData = [&](std::string const& msg) { - charge(Resource::feeInvalidData, "get_ledger " + msg); + fee_.update(Resource::feeInvalidData, "get_ledger " + msg); JLOG(p_journal_.warn()) << "TMGetLedger: " << msg; }; auto const itype{m->itype()}; @@ -1431,7 +1438,8 @@ PeerImp::onMessage(std::shared_ptr const& m) JLOG(p_journal_.trace()) << "onMessage, TMProofPathRequest"; if (!ledgerReplayEnabled_) { - charge(Resource::feeMalformedRequest, "proof_path_request disabled"); + fee_.update( + Resource::feeMalformedRequest, "proof_path_request disabled"); return; } @@ -1468,13 +1476,14 @@ PeerImp::onMessage(std::shared_ptr const& m) { if (!ledgerReplayEnabled_) { - charge(Resource::feeMalformedRequest, "proof_path_response disabled"); + fee_.update( + Resource::feeMalformedRequest, "proof_path_response disabled"); return; } if (!ledgerReplayMsgHandler_.processProofPathResponse(m)) { - charge(Resource::feeInvalidData, "proof_path_response"); + fee_.update(Resource::feeInvalidData, "proof_path_response"); } } @@ -1484,7 +1493,8 @@ PeerImp::onMessage(std::shared_ptr const& m) JLOG(p_journal_.trace()) << "onMessage, TMReplayDeltaRequest"; if (!ledgerReplayEnabled_) { - charge(Resource::feeMalformedRequest, "replay_delta_request disabled"); + fee_.update( + Resource::feeMalformedRequest, "replay_delta_request disabled"); return; } @@ -1521,13 +1531,14 @@ PeerImp::onMessage(std::shared_ptr const& m) { if (!ledgerReplayEnabled_) { - charge(Resource::feeMalformedRequest, "replay_delta_response disabled"); + fee_.update( + Resource::feeMalformedRequest, "replay_delta_response disabled"); return; } if (!ledgerReplayMsgHandler_.processReplayDeltaResponse(m)) { - charge(Resource::feeInvalidData, "replay_delta_response"); + fee_.update(Resource::feeInvalidData, "replay_delta_response"); } } @@ -2408,10 +2419,6 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - fee_.update( - Resource::feeModerateBurdenPeer, - " received a get object by hash request"); - protocol::TMGetObjectByHash reply; reply.set_query(false); @@ -2432,6 +2439,10 @@ PeerImp::onMessage(std::shared_ptr const& m) reply.set_ledgerhash(packet.ledgerhash()); } + fee_.update( + Resource::feeModerateBurdenPeer, + " received a get object by hash request"); + // This is a very minimal implementation for (int i = 0; i < packet.objects_size(); ++i) { @@ -2628,14 +2639,14 @@ PeerImp::onMessage(std::shared_ptr const& m) if (!m->has_validatorpubkey()) { - charge(Resource::feeInvalidData, "squelch no pubkey"); + fee_.update(Resource::feeInvalidData, "squelch no pubkey"); return; } auto validator = m->validatorpubkey(); auto const slice{makeSlice(validator)}; if (!publicKeyType(slice)) { - charge(Resource::feeInvalidData, "squelch bad pubkey"); + fee_.update(Resource::feeInvalidData, "squelch bad pubkey"); return; } PublicKey key(slice); @@ -2643,7 +2654,7 @@ PeerImp::onMessage(std::shared_ptr const& m) // Ignore non-validator squelch if (!app_.validators().listed(key)) { - charge(Resource::feeInvalidData, "squelch non-validator"); + fee_.update(Resource::feeInvalidData, "squelch non-validator"); JLOG(p_journal_.debug()) << "onMessage: TMSquelch discarding non-validator squelch " << slice; @@ -2663,7 +2674,7 @@ PeerImp::onMessage(std::shared_ptr const& m) if (!m->squelch()) squelch_.removeSquelch(key); else if (!squelch_.addSquelch(key, std::chrono::seconds{duration})) - charge(Resource::feeInvalidData, "squelch duration"); + fee_.update(Resource::feeInvalidData, "squelch duration"); JLOG(p_journal_.debug()) << "onMessage: TMSquelch " << slice << " " << id() << " " << duration;