Remove predicate-based peer dropping mechanism

This reverts commit d60d3c3, reversing
changes made to a15c993.

src/herder/HerderUtils.cpp

@@ -4,7 +4,6 @@
 
 #include "herder/HerderUtils.h"
 #include "scp/Slot.h"
-#include "util/ProtocolVersion.h"
 #include "xdr/Stellar-ledger.h"
 #include <algorithm>
 #include <xdrpp/marshal.h>
@@ -41,18 +40,4 @@ getStellarValues(SCPStatement const& statement)
 
     return result;
 }
-
-bool
-shouldDropPeerPredicate(Peer::pointer peer, uint32_t protocolVersion)
-{
-    bool upgraded =
-        protocolVersionStartsFrom(protocolVersion, SOROBAN_PROTOCOL_VERSION);
-    auto ovVersion = peer->getRemoteOverlayVersion();
-    if (upgraded && ovVersion &&
-        ovVersion.value() < Peer::FIRST_VERSION_REQUIRED_FOR_PROTOCOL_20)
-    {
-        return true;
-    }
-    return false;
-}
 }

src/herder/HerderUtils.h

@@ -4,7 +4,6 @@
 // under the Apache License, Version 2.0. See the COPYING file at the root
 // of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
 
-#include "overlay/Peer.h"
 #include "xdr/Stellar-types.h"
 #include <vector>
 
@@ -17,5 +16,4 @@ struct StellarValue;
 
 std::vector<Hash> getTxSetHashes(SCPEnvelope const& envelope);
 std::vector<StellarValue> getStellarValues(SCPStatement const& envelope);
-bool shouldDropPeerPredicate(Peer::pointer peer, uint32_t protocolVersion);
 }

src/ledger/LedgerManagerImpl.cpp

@@ -13,7 +13,6 @@
 #include "database/Database.h"
 #include "herder/Herder.h"
 #include "herder/HerderPersistence.h"
-#include "herder/HerderUtils.h"
 #include "herder/LedgerCloseData.h"
 #include "herder/TxSetFrame.h"
 #include "herder/Upgrades.h"
@@ -978,8 +977,6 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData)
     if (protocolVersionStartsFrom(maybeNewVersion, SOROBAN_PROTOCOL_VERSION))
     {
         updateNetworkConfig(ltx);
-        mApp.getOverlayManager().dropPeersIf(
-            shouldDropPeerPredicate, maybeNewVersion, "version too old");
     }
 
     ledgerClosed(ltx, ledgerCloseMeta, initialLedgerVers);

src/overlay/OverlayManager.h

@@ -112,8 +112,6 @@ class OverlayManager
 
     // Return a list of random peers from the set of authenticated peers.
     virtual std::vector<Peer::pointer> getRandomAuthenticatedPeers() = 0;
-    virtual std::vector<Peer::pointer>
-    getAuthenticatedPeers(bool randomize) = 0;
 
     // Return a list of random peers from the set of inbound authenticated
     // peers.
@@ -207,12 +205,7 @@ class OverlayManager
 
     virtual void recordMessageMetric(StellarMessage const& stellarMsg,
                                      Peer::pointer peer) = 0;
-
     virtual AdjustedFlowControlConfig getFlowControlBytesConfig() const = 0;
-
-    virtual void
-    dropPeersIf(std::function<bool(Peer::pointer, uint32_t)> predicate,
-                uint32_t version, std::string const& reason) = 0;
     virtual ~OverlayManager()
     {
     }

src/overlay/OverlayManagerImpl.cpp

@@ -366,37 +366,6 @@ OverlayManagerImpl::getFlowControlBytesConfig() const
             cfg.FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES};
 }
 
-void
-OverlayManagerImpl::dropPeersIf(
-    std::function<bool(Peer::pointer, uint32_t)> predicate, uint32_t version,
-    std::string const& reason)
-{
-    auto maybeDrop = [](auto peers,
-                        std::function<bool(Peer::pointer, uint32_t)> predicate,
-                        uint32_t version, std::string const& reason) {
-        std::vector<Peer::pointer> peersToDrop;
-        for (auto it = peers.begin(); it != peers.end(); ++it)
-        {
-            if (predicate(*it, version))
-            {
-                peersToDrop.emplace_back(*it);
-            }
-        }
-
-        for (auto& peer : peersToDrop)
-        {
-            // Drop will cleanup peer lists and remove peer references from
-            // overlay manager
-            peer->drop(reason, Peer::DropDirection::WE_DROPPED_REMOTE,
-                       Peer::DropMode::IGNORE_WRITE_QUEUE);
-        }
-    };
-
-    maybeDrop(getPendingPeers(), predicate, version, reason);
-    maybeDrop(getAuthenticatedPeers(/* randomize */ false), predicate, version,
-              reason);
-}
-
 void
 OverlayManagerImpl::connectTo(PeerBareAddress const& address)
 {
@@ -1071,22 +1040,13 @@ OverlayManager::isFloodMessage(StellarMessage const& msg)
 }
 std::vector<Peer::pointer>
 OverlayManagerImpl::getRandomAuthenticatedPeers()
-{
-    return getAuthenticatedPeers(true);
-}
-
-std::vector<Peer::pointer>
-OverlayManagerImpl::getAuthenticatedPeers(bool randomize)
 {
     std::vector<Peer::pointer> result;
     result.reserve(mInboundPeers.mAuthenticated.size() +
                    mOutboundPeers.mAuthenticated.size());
     extractPeersFromMap(mInboundPeers.mAuthenticated, result);
     extractPeersFromMap(mOutboundPeers.mAuthenticated, result);
-    if (randomize)
-    {
-        shufflePeerList(result);
-    }
+    shufflePeerList(result);
     return result;
 }
 

src/overlay/OverlayManagerImpl.h

@@ -143,8 +143,6 @@ class OverlayManagerImpl : public OverlayManager
     Peer::pointer getConnectedPeer(PeerBareAddress const& address) override;
 
     std::vector<Peer::pointer> getRandomAuthenticatedPeers() override;
-    std::vector<Peer::pointer> getAuthenticatedPeers(bool randomize) override;
-
     std::vector<Peer::pointer> getRandomInboundAuthenticatedPeers() override;
     std::vector<Peer::pointer> getRandomOutboundAuthenticatedPeers() override;
 
@@ -207,9 +205,6 @@ class OverlayManagerImpl : public OverlayManager
     void shufflePeerList(std::vector<Peer::pointer>& peerList);
     AdjustedFlowControlConfig getFlowControlBytesConfig() const override;
 
-    void dropPeersIf(std::function<bool(Peer::pointer, uint32_t)> predicate,
-                     uint32_t version, std::string const& reason) override;
-
     // Returns `true` iff the overlay can accept the outbound peer at `address`.
     // Logs whenever a peer cannot be accepted.
     bool canAcceptOutboundPeer(PeerBareAddress const& address) const;

src/overlay/Peer.cpp

@@ -28,7 +28,6 @@
 #include "util/ProtocolVersion.h"
 #include "util/finally.h"
 
-#include "herder/HerderUtils.h"
 #include "medida/meter.h"
 #include "medida/timer.h"
 #include "xdrpp/marshal.h"
@@ -63,7 +62,7 @@ Peer::Peer(Application& app, PeerRole role)
     , mRecurringTimer(app)
     , mState(role == WE_CALLED_REMOTE ? CONNECTING : CONNECTED)
     , mRemoteOverlayMinVersion(0)
-    , mRemoteOverlayVersion(std::nullopt)
+    , mRemoteOverlayVersion(0)
     , mCreationTime(app.getClock().now())
     , mDelayedExecutionTimer(app)
     , mTxAdverts(std::make_shared<TxAdverts>(app))
@@ -291,10 +290,7 @@ Peer::getJsonInfo(bool compact) const
     res["elapsed"] = (int)getLifeTime().count();
     res["latency"] = (int)getPing().count();
     res["ver"] = getRemoteVersion();
-    if (getRemoteOverlayVersion())
-    {
-        res["olver"] = getRemoteOverlayVersion().value();
-    }
+    res["olver"] = (int)getRemoteOverlayVersion();
     if (mFlowControl)
     {
         res["flow_control"] = mFlowControl->getFlowControlJsonInfo(compact);
@@ -1408,19 +1404,14 @@ Peer::recvHello(Hello const& elo)
         dropMode = Peer::DropMode::FLUSH_WRITE_QUEUE;
     }
 
-    bool rejectBasedOnLedgerVersion = shouldDropPeerPredicate(
-        shared_from_this(), mAppConnector.getLedgerManager()
-                                .getLastClosedLedgerHeader()
-                                .header.ledgerVersion);
-    if (mRemoteOverlayMinVersion > mRemoteOverlayVersion.value() ||
-        mRemoteOverlayVersion.value() <
+    if (mRemoteOverlayMinVersion > mRemoteOverlayVersion ||
+        mRemoteOverlayVersion <
             mAppConnector.getConfig().OVERLAY_PROTOCOL_MIN_VERSION ||
         mRemoteOverlayMinVersion >
-            mAppConnector.getConfig().OVERLAY_PROTOCOL_VERSION ||
-        rejectBasedOnLedgerVersion)
+            mAppConnector.getConfig().OVERLAY_PROTOCOL_VERSION)
     {
         CLOG_DEBUG(Overlay, "Protocol = [{},{}] expected: [{},{}]",
-                   mRemoteOverlayMinVersion, mRemoteOverlayVersion.value(),
+                   mRemoteOverlayMinVersion, mRemoteOverlayVersion,
                    mAppConnector.getConfig().OVERLAY_PROTOCOL_MIN_VERSION,
                    mAppConnector.getConfig().OVERLAY_PROTOCOL_VERSION);
         sendErrorAndDrop(ERR_CONF, "wrong protocol version", dropMode);

src/overlay/Peer.h

@@ -220,7 +220,7 @@ class Peer : public std::enable_shared_from_this<Peer>,
 
     std::string mRemoteVersion;
     uint32_t mRemoteOverlayMinVersion;
-    std::optional<uint32_t> mRemoteOverlayVersion;
+    uint32_t mRemoteOverlayVersion;
     PeerBareAddress mAddress;
 
     VirtualClock::time_point mCreationTime;
@@ -356,7 +356,7 @@ class Peer : public std::enable_shared_from_this<Peer>,
         return mRemoteOverlayMinVersion;
     }
 
-    std::optional<uint32_t>
+    uint32_t
     getRemoteOverlayVersion() const
     {
         return mRemoteOverlayVersion;

src/overlay/test/OverlayTests.cpp

@@ -135,58 +135,6 @@ TEST_CASE("loopback peer send auth before hello", "[overlay][connections]")
     testutil::shutdownWorkScheduler(*app1);
 }
 
-TEST_CASE("overlay version peer drop post-protocol-20")
-{
-    auto cfg1 = getTestConfig(0);
-    auto cfg2 = getTestConfig(1);
-
-    cfg1.OVERLAY_PROTOCOL_VERSION =
-        Peer::FIRST_VERSION_REQUIRED_FOR_PROTOCOL_20 - 1;
-    cfg1.OVERLAY_PROTOCOL_MIN_VERSION = cfg1.OVERLAY_PROTOCOL_VERSION - 1;
-    cfg2.OVERLAY_PROTOCOL_MIN_VERSION = cfg1.OVERLAY_PROTOCOL_MIN_VERSION;
-    cfg1.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-        static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-    cfg2.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-        static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-
-    VirtualClock clock;
-    auto app1 = createTestApplication(clock, cfg1);
-    auto app2 = createTestApplication(clock, cfg2);
-
-    auto result = LedgerUpgrade{LEDGER_UPGRADE_VERSION};
-    result.newLedgerVersion() = static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION);
-    LoopbackPeerConnection conn(*app1, *app2);
-
-    std::string expectedDropReason = "version too old";
-    SECTION("fully authenticated")
-    {
-        testutil::crankSome(clock);
-        REQUIRE(conn.getInitiator()->isAuthenticated());
-        REQUIRE(conn.getAcceptor()->isAuthenticated());
-    }
-    SECTION("pending peers")
-    {
-        REQUIRE(!conn.getInitiator()->isAuthenticated());
-        REQUIRE(!conn.getAcceptor()->isAuthenticated());
-        expectedDropReason = "wrong protocol version";
-    }
-
-    txtest::executeUpgrade(*app1, result);
-    txtest::executeUpgrade(*app2, result);
-
-    testutil::crankSome(clock);
-    REQUIRE(!conn.getInitiator()->isConnected());
-    REQUIRE(!conn.getAcceptor()->isConnected());
-    REQUIRE(conn.getAcceptor()->getDropReason() == expectedDropReason);
-
-    // Post-upgrade, try to re-connect and get rejected due to old protocol
-    LoopbackPeerConnection conn2(*app1, *app2);
-    testutil::crankSome(clock);
-    REQUIRE(!conn2.getInitiator()->isConnected());
-    REQUIRE(!conn2.getAcceptor()->isConnected());
-    REQUIRE(conn2.getAcceptor()->getDropReason() == "wrong protocol version");
-}
-
 TEST_CASE("flow control byte capacity", "[overlay][flowcontrol]")
 {
     VirtualClock clock;
@@ -329,17 +277,7 @@ TEST_CASE("flow control byte capacity", "[overlay][flowcontrol]")
     }
     SECTION("mixed versions")
     {
-        cfg1.OVERLAY_PROTOCOL_VERSION =
-            Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES;
-        cfg2.OVERLAY_PROTOCOL_VERSION =
-            Peer::FIRST_VERSION_REQUIRED_FOR_PROTOCOL_20 - 1;
-        cfg1.OVERLAY_PROTOCOL_MIN_VERSION = cfg1.OVERLAY_PROTOCOL_VERSION;
-        cfg2.OVERLAY_PROTOCOL_MIN_VERSION = cfg1.OVERLAY_PROTOCOL_VERSION;
-        // Mixed versions do not work in v20, as older clients are banned
-        cfg1.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-            static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-        cfg2.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-            static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
+        cfg1.OVERLAY_PROTOCOL_VERSION = cfg1.OVERLAY_PROTOCOL_MIN_VERSION;
 
         cfg1.PEER_FLOOD_READING_CAPACITY_BYTES =
             2 * getTxSize(tx1) + Herder::FLOW_CONTROL_BYTES_EXTRA_BUFFER;
@@ -352,17 +290,8 @@ TEST_CASE("flow control byte capacity", "[overlay][flowcontrol]")
     }
     SECTION("older versions")
     {
-        cfg1.OVERLAY_PROTOCOL_VERSION =
-            Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES;
-        cfg2.OVERLAY_PROTOCOL_VERSION =
-            Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES;
-        cfg1.OVERLAY_PROTOCOL_MIN_VERSION = cfg1.OVERLAY_PROTOCOL_VERSION;
-        cfg2.OVERLAY_PROTOCOL_MIN_VERSION = cfg2.OVERLAY_PROTOCOL_VERSION;
-        // Mixed versions do not work in v20, as older clients are banned
-        cfg1.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-            static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-        cfg2.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-            static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
+        cfg1.OVERLAY_PROTOCOL_VERSION = cfg1.OVERLAY_PROTOCOL_MIN_VERSION;
+        cfg2.OVERLAY_PROTOCOL_VERSION = cfg2.OVERLAY_PROTOCOL_MIN_VERSION;
         cfg1.PEER_FLOOD_READING_CAPACITY_BYTES =
             2 * getTxSize(tx1) + Herder::FLOW_CONTROL_BYTES_EXTRA_BUFFER;
         cfg1.FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES = getTxSize(tx1);
@@ -614,8 +543,7 @@ runWithBothFlowControlModes(std::vector<Config>& cfgs,
     {
         for (auto& cfg : cfgs)
         {
-            cfg.OVERLAY_PROTOCOL_VERSION =
-                Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES - 1;
+            cfg.ENABLE_FLOW_CONTROL_BYTES = false;
         }
         f(false);
     }
@@ -712,21 +640,6 @@ TEST_CASE("loopback peer flow control activation", "[overlay][flowcontrol]")
                 // Successfully enabled flow control
                 runTest({cfg1, cfg2}, false);
             }
-            SECTION("mix versions")
-            {
-                cfg1.OVERLAY_PROTOCOL_VERSION =
-                    Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES;
-                cfg1.OVERLAY_PROTOCOL_MIN_VERSION =
-                    cfg1.OVERLAY_PROTOCOL_VERSION;
-                cfg2.OVERLAY_PROTOCOL_MIN_VERSION =
-                    cfg1.OVERLAY_PROTOCOL_VERSION;
-
-                cfg1.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-                    static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-                cfg2.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-                    static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-                runTest({cfg1, cfg2}, false);
-            }
             SECTION("bad peer")
             {
                 runTest({cfg1, cfg2}, true);
@@ -2352,21 +2265,6 @@ TEST_CASE("overlay flow control", "[overlay][flowcontrol]")
         {
             setupSimulation();
         }
-        SECTION("one peer does not support flow control in bytes")
-        {
-            configs[2].OVERLAY_PROTOCOL_VERSION =
-                Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES - 1;
-
-            for (auto& cfg : configs)
-            {
-                cfg.OVERLAY_PROTOCOL_MIN_VERSION =
-                    configs[2].OVERLAY_PROTOCOL_VERSION - 1;
-                cfg.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION =
-                    static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION) - 1;
-            }
-
-            setupSimulation();
-        }
         SECTION("one peer disables")
         {
             configs[2].ENABLE_FLOW_CONTROL_BYTES = false;