From e4fdf3315805c7678500bd7c2babad77cf66148a Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 29 Aug 2025 02:00:38 +0200 Subject: [PATCH] adds additional logging to differentiate why connections were refused (#5690) This is a follow-up to PR #5664 that further improves the specificity of logging for refused peer connections. The previous changes did not account for several key scenarios, leading to potentially misleading log messages. It addresses the following - Inbound Disabled: Connections are now explicitly logged as rejected when the server is not configured to accept inbound peers. Previously, this was logged as the server being "full," which was technically correct but lacked diagnostic clarity. - Duplicate Connections: The logging now distinguishes between two types of duplicate connection refusals: - When a peer with the same node public key is already connected (duplicate connection). - When a connection is rejected because the limit for connections from a single IP address has been reached. These changes provide more accurate and actionable diagnostic information when analyzing peer connection behavior. --- src/test/overlay/tx_reduce_relay_test.cpp | 2 +- src/test/peerfinder/PeerFinder_test.cpp | 274 ++++++++++++++---- src/xrpld/overlay/detail/OverlayImpl.cpp | 11 +- src/xrpld/peerfinder/PeerfinderManager.h | 27 +- src/xrpld/peerfinder/detail/Counts.h | 2 +- src/xrpld/peerfinder/detail/Logic.h | 26 +- .../peerfinder/detail/PeerfinderConfig.cpp | 11 + .../peerfinder/detail/PeerfinderManager.cpp | 4 +- 8 files changed, 280 insertions(+), 77 deletions(-) diff --git a/src/test/overlay/tx_reduce_relay_test.cpp b/src/test/overlay/tx_reduce_relay_test.cpp index 0c67fd581c..83b3013514 100644 --- a/src/test/overlay/tx_reduce_relay_test.cpp +++ b/src/test/overlay/tx_reduce_relay_test.cpp @@ -183,7 +183,7 @@ private: boost::asio::ip::make_address("172.1.1." + std::to_string(rid_))); PublicKey key(std::get<0>(randomKeyPair(KeyType::ed25519))); auto consumer = overlay.resourceManager().newInboundEndpoint(remote); - auto slot = overlay.peerFinder().new_inbound_slot(local, remote); + auto [slot, _] = overlay.peerFinder().new_inbound_slot(local, remote); auto const peer = std::make_shared( env.app(), slot, diff --git a/src/test/peerfinder/PeerFinder_test.cpp b/src/test/peerfinder/PeerFinder_test.cpp index f35cbbdaae..64a1eb5091 100644 --- a/src/test/peerfinder/PeerFinder_test.cpp +++ b/src/test/peerfinder/PeerFinder_test.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -98,7 +99,7 @@ public: if (!list.empty()) { BEAST_EXPECT(list.size() == 1); - auto const slot = logic.new_outbound_slot(list.front()); + auto const [slot, _] = logic.new_outbound_slot(list.front()); BEAST_EXPECT(logic.onConnected( slot, beast::IP::Endpoint::from_string("65.0.0.2:5"))); logic.on_closed(slot); @@ -139,7 +140,7 @@ public: if (!list.empty()) { BEAST_EXPECT(list.size() == 1); - auto const slot = logic.new_outbound_slot(list.front()); + auto const [slot, _] = logic.new_outbound_slot(list.front()); if (!BEAST_EXPECT(logic.onConnected( slot, beast::IP::Endpoint::from_string("65.0.0.2:5")))) return; @@ -158,6 +159,7 @@ public: BEAST_EXPECT(n <= (seconds + 59) / 60); } + // test accepting an incoming slot for an already existing outgoing slot void test_duplicateOutIn() { @@ -166,8 +168,6 @@ public: TestChecker checker; TestStopwatch clock; Logic logic(clock, store, checker, journal_); - logic.addFixedPeer( - "test", beast::IP::Endpoint::from_string("65.0.0.1:5")); { Config c; c.autoConnect = false; @@ -176,28 +176,24 @@ public: logic.config(c); } - auto const list = logic.autoconnect(); - if (BEAST_EXPECT(!list.empty())) - { - BEAST_EXPECT(list.size() == 1); - auto const remote = list.front(); - auto const slot1 = logic.new_outbound_slot(remote); - if (BEAST_EXPECT(slot1 != nullptr)) - { - BEAST_EXPECT( - logic.connectedAddresses_.count(remote.address()) == 1); - auto const local = - beast::IP::Endpoint::from_string("65.0.0.2:1024"); - auto const slot2 = logic.new_inbound_slot(local, remote); - BEAST_EXPECT( - logic.connectedAddresses_.count(remote.address()) == 1); - if (!BEAST_EXPECT(slot2 == nullptr)) - logic.on_closed(slot2); - logic.on_closed(slot1); - } - } + auto const remote = beast::IP::Endpoint::from_string("65.0.0.1:5"); + auto const [slot1, r] = logic.new_outbound_slot(remote); + BEAST_EXPECT(slot1 != nullptr); + BEAST_EXPECT(r == Result::success); + BEAST_EXPECT(logic.connectedAddresses_.count(remote.address()) == 1); + + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1024"); + auto const [slot2, r2] = logic.new_inbound_slot(local, remote); + BEAST_EXPECT(logic.connectedAddresses_.count(remote.address()) == 1); + BEAST_EXPECT(r2 == Result::duplicatePeer); + + if (!BEAST_EXPECT(slot2 == nullptr)) + logic.on_closed(slot2); + + logic.on_closed(slot1); } + // test establishing outgoing slot for an already existing incoming slot void test_duplicateInOut() { @@ -206,8 +202,6 @@ public: TestChecker checker; TestStopwatch clock; Logic logic(clock, store, checker, journal_); - logic.addFixedPeer( - "test", beast::IP::Endpoint::from_string("65.0.0.1:5")); { Config c; c.autoConnect = false; @@ -216,33 +210,202 @@ public: logic.config(c); } - auto const list = logic.autoconnect(); - if (BEAST_EXPECT(!list.empty())) + auto const remote = beast::IP::Endpoint::from_string("65.0.0.1:5"); + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1024"); + + auto const [slot1, r] = logic.new_inbound_slot(local, remote); + BEAST_EXPECT(slot1 != nullptr); + BEAST_EXPECT(r == Result::success); + BEAST_EXPECT(logic.connectedAddresses_.count(remote.address()) == 1); + + auto const [slot2, r2] = logic.new_outbound_slot(remote); + BEAST_EXPECT(r2 == Result::duplicatePeer); + BEAST_EXPECT(logic.connectedAddresses_.count(remote.address()) == 1); + if (!BEAST_EXPECT(slot2 == nullptr)) + logic.on_closed(slot2); + logic.on_closed(slot1); + } + + void + test_peerLimitExceeded() + { + testcase("peer limit exceeded"); + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); { - BEAST_EXPECT(list.size() == 1); - auto const remote = list.front(); - auto const local = - beast::IP::Endpoint::from_string("65.0.0.2:1024"); - auto const slot1 = logic.new_inbound_slot(local, remote); - if (BEAST_EXPECT(slot1 != nullptr)) - { - BEAST_EXPECT( - logic.connectedAddresses_.count(remote.address()) == 1); - auto const slot2 = logic.new_outbound_slot(remote); - BEAST_EXPECT( - logic.connectedAddresses_.count(remote.address()) == 1); - if (!BEAST_EXPECT(slot2 == nullptr)) - logic.on_closed(slot2); - logic.on_closed(slot1); - } + Config c; + c.autoConnect = false; + c.listeningPort = 1024; + c.ipLimit = 2; + logic.config(c); } + + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1024"); + auto const [slot, r] = logic.new_inbound_slot( + local, beast::IP::Endpoint::from_string("55.104.0.2:1025")); + BEAST_EXPECT(slot != nullptr); + BEAST_EXPECT(r == Result::success); + + auto const [slot1, r1] = logic.new_inbound_slot( + local, beast::IP::Endpoint::from_string("55.104.0.2:1026")); + BEAST_EXPECT(slot1 != nullptr); + BEAST_EXPECT(r1 == Result::success); + + auto const [slot2, r2] = logic.new_inbound_slot( + local, beast::IP::Endpoint::from_string("55.104.0.2:1027")); + BEAST_EXPECT(r2 == Result::ipLimitExceeded); + + if (!BEAST_EXPECT(slot2 == nullptr)) + logic.on_closed(slot2); + logic.on_closed(slot1); + logic.on_closed(slot); + } + + void + test_activate_duplicate_peer() + { + testcase("test activate duplicate peer"); + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); + { + Config c; + c.autoConnect = false; + c.listeningPort = 1024; + c.ipLimit = 2; + logic.config(c); + } + + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1024"); + + PublicKey const pk1(randomKeyPair(KeyType::secp256k1).first); + + auto const [slot, rSlot] = logic.new_outbound_slot( + beast::IP::Endpoint::from_string("55.104.0.2:1025")); + BEAST_EXPECT(slot != nullptr); + BEAST_EXPECT(rSlot == Result::success); + + auto const [slot2, r2Slot] = logic.new_outbound_slot( + beast::IP::Endpoint::from_string("55.104.0.2:1026")); + BEAST_EXPECT(slot2 != nullptr); + BEAST_EXPECT(r2Slot == Result::success); + + BEAST_EXPECT(logic.onConnected(slot, local)); + BEAST_EXPECT(logic.onConnected(slot2, local)); + + BEAST_EXPECT(logic.activate(slot, pk1, false) == Result::success); + + // activating a different slot with the same node ID (pk) must fail + BEAST_EXPECT( + logic.activate(slot2, pk1, false) == Result::duplicatePeer); + + logic.on_closed(slot); + + // accept the same key for a new slot after removing the old slot + BEAST_EXPECT(logic.activate(slot2, pk1, false) == Result::success); + logic.on_closed(slot2); + } + + void + test_activate_inbound_disabled() + { + testcase("test activate inbound disabled"); + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); + { + Config c; + c.autoConnect = false; + c.listeningPort = 1024; + c.ipLimit = 2; + logic.config(c); + } + + PublicKey const pk1(randomKeyPair(KeyType::secp256k1).first); + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1024"); + + auto const [slot, rSlot] = logic.new_inbound_slot( + local, beast::IP::Endpoint::from_string("55.104.0.2:1025")); + BEAST_EXPECT(slot != nullptr); + BEAST_EXPECT(rSlot == Result::success); + + BEAST_EXPECT( + logic.activate(slot, pk1, false) == Result::inboundDisabled); + + { + Config c; + c.autoConnect = false; + c.listeningPort = 1024; + c.ipLimit = 2; + c.inPeers = 1; + logic.config(c); + } + // new inbound slot must succeed when inbound connections are enabled + BEAST_EXPECT(logic.activate(slot, pk1, false) == Result::success); + + // creating a new inbound slot must succeed as IP Limit is not exceeded + auto const [slot2, r2Slot] = logic.new_inbound_slot( + local, beast::IP::Endpoint::from_string("55.104.0.2:1026")); + BEAST_EXPECT(slot2 != nullptr); + BEAST_EXPECT(r2Slot == Result::success); + + PublicKey const pk2(randomKeyPair(KeyType::secp256k1).first); + + // an inbound slot exceeding inPeers limit must fail + BEAST_EXPECT(logic.activate(slot2, pk2, false) == Result::full); + + logic.on_closed(slot2); + logic.on_closed(slot); + } + + void + test_addFixedPeer_no_port() + { + testcase("test addFixedPeer no port"); + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); + try + { + logic.addFixedPeer( + "test", beast::IP::Endpoint::from_string("65.0.0.2")); + fail("invalid endpoint successfully added"); + } + catch (std::runtime_error const& e) + { + pass(); + } + } + + void + test_onConnected_self_connection() + { + testcase("test onConnected self connection"); + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); + + auto const local = beast::IP::Endpoint::from_string("65.0.0.2:1234"); + auto const [slot, r] = logic.new_outbound_slot(local); + BEAST_EXPECT(slot != nullptr); + BEAST_EXPECT(r == Result::success); + + // Must fail when a slot is to our own IP address + BEAST_EXPECT(!logic.onConnected(slot, local)); + logic.on_closed(slot); } void test_config() { - // if peers_max is configured then peers_in_max and peers_out_max are - // ignored + // if peers_max is configured then peers_in_max and peers_out_max + // are ignored auto run = [&](std::string const& test, std::optional maxPeers, std::optional maxIn, @@ -282,13 +445,21 @@ public: Counts counts; counts.onConfig(config); BEAST_EXPECT( - counts.out_max() == expectOut && - counts.inboundSlots() == expectIn && + counts.out_max() == expectOut && counts.in_max() == expectIn && config.ipLimit == expectIpLimit); + + TestStore store; + TestChecker checker; + TestStopwatch clock; + Logic logic(clock, store, checker, journal_); + logic.config(config); + + BEAST_EXPECT(logic.config() == config); }; // if max_peers == 0 => maxPeers = 21, - // else if max_peers < 10 => maxPeers = 10 else maxPeers = max_peers + // else if max_peers < 10 => maxPeers = 10 else maxPeers = + // max_peers // expectOut => if legacy => max(0.15 * maxPeers, 10), // if legacy && !wantIncoming => maxPeers else max_out_peers // expectIn => if legacy && wantIncoming => maxPeers - outPeers @@ -364,6 +535,11 @@ public: test_duplicateInOut(); test_config(); test_invalid_config(); + test_peerLimitExceeded(); + test_activate_duplicate_peer(); + test_activate_inbound_disabled(); + test_addFixedPeer_no_port(); + test_onConnected_self_connection(); } }; diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index f2c683b69f..8d295faace 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -195,14 +195,16 @@ OverlayImpl::onHandoff( if (consumer.disconnect(journal)) return handoff; - auto const slot = m_peerFinder->new_inbound_slot( + auto const [slot, result] = m_peerFinder->new_inbound_slot( beast::IPAddressConversion::from_asio(local_endpoint), beast::IPAddressConversion::from_asio(remote_endpoint)); if (slot == nullptr) { - // self-connect, close + // connection refused either IP limit exceeded or self-connect handoff.moved = false; + JLOG(journal.debug()) + << "Peer " << remote_endpoint << " refused, " << to_string(result); return handoff; } @@ -402,10 +404,11 @@ OverlayImpl::connect(beast::IP::Endpoint const& remote_endpoint) return; } - auto const slot = peerFinder().new_outbound_slot(remote_endpoint); + auto const [slot, result] = peerFinder().new_outbound_slot(remote_endpoint); if (slot == nullptr) { - JLOG(journal_.debug()) << "Connect: No slot for " << remote_endpoint; + JLOG(journal_.debug()) << "Connect: No slot for " << remote_endpoint + << ": " << to_string(result); return; } diff --git a/src/xrpld/peerfinder/PeerfinderManager.h b/src/xrpld/peerfinder/PeerfinderManager.h index f399251c38..b7ea738a81 100644 --- a/src/xrpld/peerfinder/PeerfinderManager.h +++ b/src/xrpld/peerfinder/PeerfinderManager.h @@ -109,6 +109,9 @@ struct Config std::uint16_t port, bool validationPublicKey, int ipLimit); + + friend bool + operator==(Config const& lhs, Config const& rhs); }; //------------------------------------------------------------------------------ @@ -136,7 +139,13 @@ using Endpoints = std::vector; //------------------------------------------------------------------------------ /** Possible results from activating a slot. */ -enum class Result { duplicate, full, success }; +enum class Result { + inboundDisabled, + duplicatePeer, + ipLimitExceeded, + full, + success +}; /** * @brief Converts a `Result` enum value to its string representation. @@ -157,12 +166,16 @@ to_string(Result result) noexcept { switch (result) { - case Result::success: - return "success"; - case Result::duplicate: - return "duplicate connection"; + case Result::inboundDisabled: + return "inbound disabled"; + case Result::duplicatePeer: + return "peer already connected"; + case Result::ipLimitExceeded: + return "ip limit exceeded"; case Result::full: return "slots full"; + case Result::success: + return "success"; } return "unknown"; @@ -234,7 +247,7 @@ public: If nullptr is returned, then the slot could not be assigned. Usually this is because of a detected self-connection. */ - virtual std::shared_ptr + virtual std::pair, Result> new_inbound_slot( beast::IP::Endpoint const& local_endpoint, beast::IP::Endpoint const& remote_endpoint) = 0; @@ -243,7 +256,7 @@ public: If nullptr is returned, then the slot could not be assigned. Usually this is because of a duplicate connection. */ - virtual std::shared_ptr + virtual std::pair, Result> new_outbound_slot(beast::IP::Endpoint const& remote_endpoint) = 0; /** Called when mtENDPOINTS is received. */ diff --git a/src/xrpld/peerfinder/detail/Counts.h b/src/xrpld/peerfinder/detail/Counts.h index c91b27b026..821431c5bb 100644 --- a/src/xrpld/peerfinder/detail/Counts.h +++ b/src/xrpld/peerfinder/detail/Counts.h @@ -163,7 +163,7 @@ public: /** Returns the total number of inbound slots. */ int - inboundSlots() const + in_max() const { return m_in_max; } diff --git a/src/xrpld/peerfinder/detail/Logic.h b/src/xrpld/peerfinder/detail/Logic.h index e23bbc29e1..4b92a1d143 100644 --- a/src/xrpld/peerfinder/detail/Logic.h +++ b/src/xrpld/peerfinder/detail/Logic.h @@ -172,9 +172,7 @@ public: void addFixedPeer(std::string const& name, beast::IP::Endpoint const& ep) { - std::vector v; - v.push_back(ep); - addFixedPeer(name, v); + addFixedPeer(name, std::vector{ep}); } void @@ -261,7 +259,7 @@ public: //-------------------------------------------------------------------------- - SlotImp::ptr + std::pair new_inbound_slot( beast::IP::Endpoint const& local_endpoint, beast::IP::Endpoint const& remote_endpoint) @@ -277,12 +275,12 @@ public: { auto const count = connectedAddresses_.count(remote_endpoint.address()); - if (count > config_.ipLimit) + if (count + 1 > config_.ipLimit) { JLOG(m_journal.debug()) << beast::leftw(18) << "Logic dropping inbound " << remote_endpoint << " because of ip limits."; - return SlotImp::ptr(); + return {SlotImp::ptr(), Result::ipLimitExceeded}; } } @@ -292,7 +290,7 @@ public: JLOG(m_journal.debug()) << beast::leftw(18) << "Logic dropping " << remote_endpoint << " as duplicate incoming"; - return SlotImp::ptr(); + return {SlotImp::ptr(), Result::duplicatePeer}; } // Create the slot @@ -314,11 +312,11 @@ public: // Update counts counts_.add(*slot); - return result.first->second; + return {result.first->second, Result::success}; } // Can't check for self-connect because we don't know the local endpoint - SlotImp::ptr + std::pair new_outbound_slot(beast::IP::Endpoint const& remote_endpoint) { JLOG(m_journal.debug()) @@ -332,7 +330,7 @@ public: JLOG(m_journal.debug()) << beast::leftw(18) << "Logic dropping " << remote_endpoint << " as duplicate connect"; - return SlotImp::ptr(); + return {SlotImp::ptr(), Result::duplicatePeer}; } // Create the slot @@ -353,7 +351,7 @@ public: // Update counts counts_.add(*slot); - return result.first->second; + return {result.first->second, Result::success}; } bool @@ -417,7 +415,7 @@ public: // Check for duplicate connection by key if (keys_.find(key) != keys_.end()) - return Result::duplicate; + return Result::duplicatePeer; // If the peer belongs to a cluster or is reserved, // update the slot to reflect that. @@ -430,6 +428,8 @@ public: { if (!slot->inbound()) bootcache_.on_success(slot->remote_endpoint()); + if (slot->inbound() && counts_.in_max() == 0) + return Result::inboundDisabled; return Result::full; } @@ -651,7 +651,7 @@ public: // 2. We have slots // 3. We haven't failed the firewalled test // - if (config_.wantIncoming && counts_.inboundSlots() > 0) + if (config_.wantIncoming && counts_.in_max() > 0) { Endpoint ep; ep.hops = 0; diff --git a/src/xrpld/peerfinder/detail/PeerfinderConfig.cpp b/src/xrpld/peerfinder/detail/PeerfinderConfig.cpp index 3075224189..30eb778770 100644 --- a/src/xrpld/peerfinder/detail/PeerfinderConfig.cpp +++ b/src/xrpld/peerfinder/detail/PeerfinderConfig.cpp @@ -34,6 +34,17 @@ Config::Config() { } +bool +operator==(Config const& lhs, Config const& rhs) +{ + return lhs.autoConnect == rhs.autoConnect && + lhs.peerPrivate == rhs.peerPrivate && + lhs.wantIncoming == rhs.wantIncoming && lhs.inPeers == rhs.inPeers && + lhs.maxPeers == rhs.maxPeers && lhs.outPeers == rhs.outPeers && + lhs.features == lhs.features && lhs.ipLimit == rhs.ipLimit && + lhs.listeningPort == rhs.listeningPort; +} + std::size_t Config::calcOutPeers() const { diff --git a/src/xrpld/peerfinder/detail/PeerfinderManager.cpp b/src/xrpld/peerfinder/detail/PeerfinderManager.cpp index 205df67fa6..462820cca2 100644 --- a/src/xrpld/peerfinder/detail/PeerfinderManager.cpp +++ b/src/xrpld/peerfinder/detail/PeerfinderManager.cpp @@ -125,7 +125,7 @@ public: //-------------------------------------------------------------------------- - std::shared_ptr + std::pair, Result> new_inbound_slot( beast::IP::Endpoint const& local_endpoint, beast::IP::Endpoint const& remote_endpoint) override @@ -133,7 +133,7 @@ public: return m_logic.new_inbound_slot(local_endpoint, remote_endpoint); } - std::shared_ptr + std::pair, Result> new_outbound_slot(beast::IP::Endpoint const& remote_endpoint) override { return m_logic.new_outbound_slot(remote_endpoint);