Mandate flow control in bytes #4353

bboston7 · 2024-06-07T18:03:50Z

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:

Both parties have flow control bytes enabled
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.

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

This is the first step in addressing stellar#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.

bboston7 · 2024-06-07T18:06:19Z

src/main/Config.cpp

+                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.");
+                }


I want to call this out because I'm not sure what our usual policy is around removing config options. I opted to have it ignore the option entirely, but warn or error depending on whether the config option matched the new requirement. Another option would be to fail to start. What approach do we usually take?

I think ignoring is fine here. Unlike the BucketListDB config migrations, ENABLE_FLOW_CONTROL_BYTES being silently enabled won't break any operators, so we can just warn for a bit then remove the flag.

Sounds good! I left this as-is.

Tbh I doubt anyone would be affected by the removal of this flag, since it's sort of an advanced low-level config. You can probably just remove it without graceful deprecation, but we do need to make sure that our own infra isn't using the flag somewhere (to avoid crashing on startup during the release).

SirTyson

LGTM, one comment on changing a flow control bytes check to make deprecation easier.

SirTyson · 2024-06-17T18:14:19Z

src/main/Config.cpp

+                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.");
+                }


I think ignoring is fine here. Unlike the BucketListDB config migrations, ENABLE_FLOW_CONTROL_BYTES being silently enabled won't break any operators, so we can just warn for a bit then remove the flag.

SirTyson · 2024-06-17T18:16:28Z

src/overlay/Peer.cpp

    {
        msg.auth().flags = AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED;
    }
+#else


Nit: This #else isn't necessary, you can just wrap the if expression:

#ifdef BUILD_TESTS if (!mAppConnector.getOverlayManager() .isFlowControlBytesDisabledForTesting()) #endif

SirTyson · 2024-06-17T18:32:05Z

src/overlay/Peer.cpp

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


Should we change this check to

bool bothWantBytes = msg.auth().flags == AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED || getRemoteOverlayVersion() >= 35;

That way once the minimum protocol version >= 35, we can just stop sending/checking AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED entirely, since the flag would be made redundant.

Yeah, I think that's a good idea. I made that change.

SirTyson

LGTM, thanks for the changes!

marta-lokhova

I think you can go further with this change and remove support for disabling flow control in bytes completely:

Minimum overlay version is far beyond FIRST_VERSION_SUPPORTING_FLOW_CONTROL_IN_BYTES at this point, so most (if not all) nodes on the network will have the feature enabled
I doubt there are any nodes outside of our infra that are manually tuning this config. Looking at a snapshot of peers on one of the watchers, I don't see anyone not using the feature.
I think we can remove any conditional enabling of flow control in bytes, and just implicitly set it to true. You might be able to get rid of the annoying FlowControl::start call at the end of authentication as well, and basically start flow control automatically when we create Peer.
We also don't need to set AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED anymore. I'd suggest we set auth flags back to 0, so we can use it for future feature rollout. AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED can be deprecated.
We still need to bump overlay version. While we're at it: the network recently upgraded to v21, so we can actually bump minimum overlay version as well to the one that supports v21 (meaning that we can drop all v20 clients now)
Dropping support for this flag means we can drop all the tests dealing with mixed modes. Yay to code removal!

marta-lokhova · 2024-06-26T21:09:02Z

src/main/Config.cpp

+                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.");
+                }


Tbh I doubt anyone would be affected by the removal of this flag, since it's sort of an advanced low-level config. You can probably just remove it without graceful deprecation, but we do need to make sure that our own infra isn't using the flag somewhere (to avoid crashing on startup during the release).

bboston7 commented Jun 7, 2024

View reviewed changes

bboston7 requested a review from marta-lokhova June 7, 2024 18:06

bboston7 requested a review from SirTyson June 17, 2024 18:03

SirTyson approved these changes Jun 17, 2024

View reviewed changes

Make future removal easier

122282c

SirTyson approved these changes Jun 20, 2024

View reviewed changes

marta-lokhova requested changes Jun 26, 2024

View reviewed changes

marta-lokhova mentioned this pull request Jun 28, 2024

Kick out older clients from the network #4374

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mandate flow control in bytes #4353

Mandate flow control in bytes #4353

bboston7 commented Jun 7, 2024

bboston7 Jun 7, 2024

SirTyson Jun 17, 2024

bboston7 Jun 17, 2024

marta-lokhova Jun 26, 2024

SirTyson left a comment

SirTyson Jun 17, 2024

SirTyson Jun 17, 2024

bboston7 Jun 17, 2024

SirTyson Jun 17, 2024

bboston7 Jun 17, 2024

SirTyson left a comment

marta-lokhova left a comment •

edited

Loading

marta-lokhova Jun 26, 2024

Mandate flow control in bytes #4353

Are you sure you want to change the base?

Mandate flow control in bytes #4353

Conversation

bboston7 commented Jun 7, 2024

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirTyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirTyson left a comment

Choose a reason for hiding this comment

marta-lokhova left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marta-lokhova left a comment •

edited

Loading