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

@@ -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<PeerTest>(
             env.app(),
             slot,

src/test/peerfinder/PeerFinder_test.cpp

@@ -20,6 +20,7 @@
 #include <test/unit_test/SuiteJournal.h>
 
 #include <xrpld/core/Config.h>
+#include <xrpld/peerfinder/PeerfinderManager.h>
 #include <xrpld/peerfinder/detail/Logic.h>
 
 #include <xrpl/basics/chrono.h>
@@ -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<TestChecker> 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<TestChecker> 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<TestChecker> 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<TestChecker> 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<TestChecker> 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<TestChecker> 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<TestChecker> 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<std::uint16_t> maxPeers,
                        std::optional<std::uint16_t> 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<TestChecker> 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();
     }
 };
 

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;
     }
 

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<Endpoint>;
 //------------------------------------------------------------------------------
 
 /** 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<Slot>
+    virtual std::pair<std::shared_ptr<Slot>, 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<Slot>
+    virtual std::pair<std::shared_ptr<Slot>, Result>
     new_outbound_slot(beast::IP::Endpoint const& remote_endpoint) = 0;
 
     /** Called when mtENDPOINTS is received. */

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;
     }

src/xrpld/peerfinder/detail/Logic.h

@@ -172,9 +172,7 @@ public:
     void
     addFixedPeer(std::string const& name, beast::IP::Endpoint const& ep)
     {
-        std::vector<beast::IP::Endpoint> v;
-        v.push_back(ep);
-        addFixedPeer(name, v);
+        addFixedPeer(name, std::vector<beast::IP::Endpoint>{ep});
     }
 
     void
@@ -261,7 +259,7 @@ public:
 
     //--------------------------------------------------------------------------
 
-    SlotImp::ptr
+    std::pair<SlotImp::ptr, Result>
     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<SlotImp::ptr, Result>
     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;

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
 {

src/xrpld/peerfinder/detail/PeerfinderManager.cpp

@@ -125,7 +125,7 @@ public:
 
     //--------------------------------------------------------------------------
 
-    std::shared_ptr<Slot>
+    std::pair<std::shared_ptr<Slot>, 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<Slot>
+    std::pair<std::shared_ptr<Slot>, Result>
     new_outbound_slot(beast::IP::Endpoint const& remote_endpoint) override
     {
         return m_logic.new_outbound_slot(remote_endpoint);