overlay: add byte limit to flow control#3737
Conversation
3da16d1 to
6012cdb
Compare
|
(note: still need to update vNext XDR, so the build is currently failing) |
src/overlay/Peer.h
Outdated
There was a problem hiding this comment.
heads up: this const and capacity configs are placeholders for now. I'll update those once I finish simulation tuning
src/main/Config.cpp
Outdated
src/main/Config.cpp
Outdated
src/main/Config.cpp
Outdated
There was a problem hiding this comment.
what is max classic tx size? do we read it from somewhere?
There was a problem hiding this comment.
it's a const. In the future, the actual max tx size value will be derived from this const and Soroban network config.
src/main/Config.cpp
Outdated
There was a problem hiding this comment.
can send_more_batch_size be less than max_tx_size?
There was a problem hiding this comment.
yes, batch size is just a frequency of data requests.
src/overlay/FlowControl.cpp
Outdated
src/overlay/FlowControl.cpp
Outdated
src/overlay/FlowControl.cpp
Outdated
There was a problem hiding this comment.
is this enable/disable flag for flow control in bytes?
There was a problem hiding this comment.
Yeah, this pointer is set if a connection decides to use fc in bytes
src/main/Config.h
Outdated
There was a problem hiding this comment.
I think it will be nice to add a description in this PR to explain peer flood capacity, batch size for both sender and receiver (maybe a simple graph will be better?)
src/overlay/FlowControl.cpp
Outdated
There was a problem hiding this comment.
send more and send more extended, how they are different?
There was a problem hiding this comment.
send_more is for current fc (messages only), while extended includes byte counts
c9d5a09 to
ceb3bef
Compare
8508969 to
5ca9b25
Compare
SirTyson
left a comment
There was a problem hiding this comment.
A few questions but LGTM
docs/stellar-core_example.cfg
Outdated
There was a problem hiding this comment.
Add OUTBOUND_TX_QUEUE_BYTE_LIMIT here too?
There was a problem hiding this comment.
Is it worth the overhead to add an extension switch here to avoid SendMoreExtenededExteneded in the future?
There was a problem hiding this comment.
for overlay, we normally introduce new messages instead of extending old ones, since older overlay versions become obsolete and this approach makes it easy to deprecate old behavior (note that this is different from protocol, where we need to support old behavior forever)
src/overlay/FlowControlCapacity.cpp
Outdated
There was a problem hiding this comment.
Just curious, why do we have a message count reading capacity but not a byte based reading capacity?
There was a problem hiding this comment.
good question. the reading capacity is a purely internal concept, needed to ensure fairness across all of our connections (what we used to do was read from a peer while there was any data available, potentially starving other peers). the reason I decided to keep only the message counting for now was because the largest message we allow in overlay is 16MB anyway, so it seemed like it wasn't super useful to try to enforce the existing byte limits, since they would be way off when we receive larger messages like tx sets. That being said, we might do something better than this in the future, but for now I'm leaving it as-is.
src/overlay/test/OverlayTests.cpp
Outdated
There was a problem hiding this comment.
This value seems cryptic, I'd add some comment saying something like "must be a value > 0 && value != 100" with a brief explanation as to why.
5ca9b25 to
784ad75
Compare
| "can't be greater than PEER_FLOOD_READING_CAPACITY_BYTES"; | ||
| throw std::runtime_error(msg); | ||
| } | ||
|
|
05d6b28 to
80ac752
Compare
80ac752 to
da8e7c7
Compare
|
r+ da8e7c7 |
|
r+ f012569 |
Functionally the change is equivalent to #3607, but opening a new PR since with the latest refactor the code is sufficiently different.
One more TODO I'm currently working on is proper handling of Soroban txMaxSize upgrades, but we can start reviewing this change already (the upgrade handling is behind the vNext flag anyways)