Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mandate flow control in bytes #4353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Mandate flow control in bytes #4353

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0344344
Mandate flow control in bytes
bboston7 Jun 6, 2024
122282c
Make future removal easier
bboston7 Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions docs/stellar-core_example.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
21 changes: 18 additions & 3 deletions src/main/Config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.");
}
Comment on lines +1035 to +1051
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I left this as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else if (item.first == "OUTBOUND_TX_QUEUE_BYTE_LIMIT")
{
Expand Down
5 changes: 0 additions & 5 deletions src/main/Config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
7 changes: 7 additions & 0 deletions src/overlay/OverlayManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
}
14 changes: 14 additions & 0 deletions src/overlay/OverlayManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

}
9 changes: 9 additions & 0 deletions src/overlay/OverlayManagerImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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);
Expand Down
25 changes: 16 additions & 9 deletions src/overlay/Peer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

msg.auth().flags = AUTH_MSG_FLAG_FLOW_CONTROL_BYTES_REQUESTED;
#endif
auto msgPtr = std::make_shared<StellarMessage const>(msg);
sendMessage(msgPtr);
}
Expand Down Expand Up @@ -1740,15 +1745,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#ifdef BUILD_TESTS
if (mAppConnector.getOverlayManager()
.isFlowControlBytesDisabledForTesting())
{
bothWantBytes = false;
}
#endif

std::optional<uint32_t> fcBytes =
bothWantBytes
Expand Down
2 changes: 0 additions & 2 deletions src/overlay/Peer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading