Mandate flow control in bytes

This is the first step in addressing #3781. It removes the
`ENABLE_FLOW_CONTROL_BYTES` config option and always uses flow control
bytes mode when a peer supports it.

Most of the change is tweaking tests to stress one of two scenarios:
1. Both parties have flow control bytes enabled
2. One party has flow control bytes enabled

It is no longer possible for neither party to have flow control bytes
enabled.

Once the minimum supported overlay version is at least 35 we can remove
all of the code handling cases where a peer does not have flow control
bytes enabled.

docs/stellar-core_example.cfg

@@ -530,11 +530,6 @@ PEER_FLOOD_READING_CAPACITY_BYTES=300000
 # processes `FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES` bytes
 FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES=100000
 
-# Enable flow control in bytes. This config allows core to process large
-# transactions on the network more efficiently and apply back pressure if
-# needed.
-ENABLE_FLOW_CONTROL_BYTES=true
-
 # Byte limit for outbound transaction queue.
 OUTBOUND_TX_QUEUE_BYTE_LIMIT=3145728
 

src/main/Config.cpp

@@ -152,7 +152,7 @@ Config::Config() : NODE_SEED(SecretKey::random())
     LEDGER_PROTOCOL_MIN_VERSION_INTERNAL_ERROR_REPORT = 18;
 
     OVERLAY_PROTOCOL_MIN_VERSION = 32;
-    OVERLAY_PROTOCOL_VERSION = 34;
+    OVERLAY_PROTOCOL_VERSION = 35;
 
     VERSION_STR = STELLAR_CORE_VERSION;
 
@@ -261,7 +261,6 @@ Config::Config() : NODE_SEED(SecretKey::random())
     PEER_FLOOD_READING_CAPACITY_BYTES = 0;
     FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES = 0;
     OUTBOUND_TX_QUEUE_BYTE_LIMIT = 1024 * 1024 * 3;
-    ENABLE_FLOW_CONTROL_BYTES = true;
 
     // WORKER_THREADS: setting this too low risks a form of priority inversion
     // where a long-running background task occupies all worker threads and
@@ -1033,7 +1032,23 @@ Config::processConfig(std::shared_ptr<cpptoml::table> t)
             }
             else if (item.first == "ENABLE_FLOW_CONTROL_BYTES")
             {
-                ENABLE_FLOW_CONTROL_BYTES = readBool(item);
+                if (readBool(item))
+                {
+                    CLOG_WARNING(
+                        Overlay,
+                        "ENABLE_FLOW_CONTROL_BYTES is now mandatory. This "
+                        "config option will be removed in a future release. "
+                        "Please remove the option from your configuration "
+                        "file.");
+                }
+                else
+                {
+                    CLOG_ERROR(Overlay,
+                               "ENABLE_FLOW_CONTROL_BYTES is now mandatory. "
+                               "Overriding to `true`. This config option will "
+                               "be removed in a future release. Please remove "
+                               "the option from your configuration file.");
+                }
             }
             else if (item.first == "OUTBOUND_TX_QUEUE_BYTE_LIMIT")
             {

src/main/Config.h

@@ -324,11 +324,6 @@ class Config : public std::enable_shared_from_this<Config>
     uint32_t PEER_FLOOD_READING_CAPACITY_BYTES;
     uint32_t FLOW_CONTROL_SEND_MORE_BATCH_SIZE_BYTES;
 
-    // Enable flow control in bytes. This config allows core to process large
-    // transactions on the network more efficiently and apply back pressure if
-    // needed.
-    bool ENABLE_FLOW_CONTROL_BYTES;
-
     // Byte limit for outbound transaction queue.
     uint32_t OUTBOUND_TX_QUEUE_BYTE_LIMIT;
 

src/overlay/OverlayManager.h

@@ -211,5 +211,12 @@ class OverlayManager
     virtual ~OverlayManager()
     {
     }
+
+#ifdef BUILD_TESTS
+    // These functions enable testing interoperability with older stellar-core
+    // nodes that allow disabling flow control bytes.
+    virtual void disableFlowControlBytesForTesting() = 0;
+    virtual bool isFlowControlBytesDisabledForTesting() const = 0;
+#endif // BUILD_TESTS
 };
 }

src/overlay/OverlayManagerImpl.cpp

@@ -1369,4 +1369,18 @@ OverlayManagerImpl::recordMessageMetric(StellarMessage const& stellarMsg,
     }
 }
 
+#ifdef BUILD_TESTS
+void
+OverlayManagerImpl::disableFlowControlBytesForTesting()
+{
+    mDisableFlowControlBytesForTesting.store(true);
+}
+
+bool
+OverlayManagerImpl::isFlowControlBytesDisabledForTesting() const
+{
+    return mDisableFlowControlBytesForTesting.load();
+}
+#endif // BUILD_TESTS
+
 }

src/overlay/OverlayManagerImpl.h

@@ -169,6 +169,11 @@ class OverlayManagerImpl : public OverlayManager
     void recordMessageMetric(StellarMessage const& stellarMsg,
                              Peer::pointer peer) override;
 
+#ifdef BUILD_TESTS
+    void disableFlowControlBytesForTesting() override;
+    bool isFlowControlBytesDisabledForTesting() const override;
+#endif // BUILD_TESTS
+
   private:
     struct ResolvedPeers
     {
@@ -182,6 +187,10 @@ class OverlayManagerImpl : public OverlayManager
     bool mResolvingPeersWithBackoff;
     int mResolvingPeersRetryCount;
 
+#ifdef BUILD_TESTS
+    std::atomic<bool> mDisableFlowControlBytesForTesting{false};
+#endif
+
     void triggerPeerResolution();
     std::pair<std::vector<PeerBareAddress>, bool>
     resolvePeers(std::vector<std::string> const& peers);

src/overlay/Peer.cpp

@@ -389,10 +389,15 @@ Peer::sendAuth()
     ZoneScoped;
     StellarMessage msg;
     msg.type(AUTH);
-    if (mAppConnector.getConfig().ENABLE_FLOW_CONTROL_BYTES)
+#ifdef BUILD_TESTS
+    if (!mAppConnector.getOverlayManager()
+             .isFlowControlBytesDisabledForTesting())
     {
         msg.auth().flags = AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED;
     }
+#else
+    msg.auth().flags = AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED;
+#endif
     auto msgPtr = std::make_shared<StellarMessage const>(msg);
     sendMessage(msgPtr);
 }
@@ -1746,15 +1751,17 @@ Peer::recvAuth(StellarMessage const& msg)
         return;
     }
 
-    bool enableBytes =
-        (mAppConnector.getConfig().OVERLAY_PROTOCOL_VERSION >=
-             Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES &&
-         getRemoteOverlayVersion() >=
-             Peer::FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES);
+    // NOTE: Once min overlay version is 35 we can remove this check
     bool bothWantBytes =
-        enableBytes &&
-        msg.auth().flags == AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED &&
-        mAppConnector.getConfig().ENABLE_FLOW_CONTROL_BYTES;
+        msg.auth().flags == AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED;
+
+#ifdef BUILD_TESTS
+    if (mAppConnector.getOverlayManager()
+            .isFlowControlBytesDisabledForTesting())
+    {
+        bothWantBytes = false;
+    }
+#endif
 
     std::optional<uint32_t> fcBytes =
         bothWantBytes

src/overlay/Peer.h

@@ -71,8 +71,6 @@ class Peer : public std::enable_shared_from_this<Peer>,
         std::chrono::milliseconds(1);
     static constexpr std::chrono::nanoseconds PEER_METRICS_RATE_UNIT =
         std::chrono::seconds(1);
-    static constexpr uint32_t FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES =
-        28;
     static constexpr uint32_t FIRST_VERSION_REQUIRED_FOR_PROTOCOL_20 = 32;
 
     // The reporting will be based on the previous