Merge branch 'ximinez/lending-refactoring-2' into ximinez/lending-refactoring-3

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