Bootstrap/restore bandwidth limit #4059
Conversation
| @@ -179,7 +179,7 @@ fn stop_with_controller_still_exists() { | |||
| let mut config1 = ProtocolConfig::default(); | |||
| config1 | |||
| .listeners | |||
| .insert("127.0.0.1:8081".parse().unwrap(), TransportType::Tcp); | |||
| .insert("127.0.0.1:8083".parse().unwrap(), TransportType::Tcp); | |||
There was a problem hiding this comment.
This provided a fix locally, but fix doesn't work in CI
There was a problem hiding this comment.
...apparently it does?
There was a problem hiding this comment.
...this is weird. last time I looked at this page, the CI had passed.
|
Note that for now |
|
After massalabs/stream_limiter#13 is merged, and a release created, you'll be able to update the dependency on it so we can have a working test setup :-) |
|
@Ben-PH You can now update |
| cfg: BootstrapClientConfig, | ||
| limit: Option<u64>, | ||
| ) -> Self { | ||
| // A 1s window breaks anything requiring a 1s window |
There was a problem hiding this comment.
With the latest versions, you should be able to specify a bigger window version.
The limits will be divided to more granular values until an accepted threshold of rate is reached
(See https://github.com/massalabs/stream_limiter/blob/main/src/lib.rs#L47)
So you should be able to specify your rate without needing to divide and set the timewindow as millis
| max_bytes_read_write: _limit, | ||
| thread_count, | ||
| max_datastore_key_length, | ||
| randomness_size_bytes, | ||
| consensus_bootstrap_part_size, | ||
| write_error_timeout, | ||
| } = cfg; | ||
|
|
||
| // A 1s window breaks anything requiring a 1s window |
massa-bootstrap/src/tests/binders.rs
Outdated
| .unwrap(); | ||
| let dur = before.elapsed(); | ||
| assert!(dbg!(dur) > Duration::from_secs(10)); | ||
| assert!(dur < Duration::from_millis(11_500)); |
There was a problem hiding this comment.
Please note that this kind of assert may fail sometimes
That's because TCP packets are bound to 64Kb per packet, so even if we allow more data to be sent, we'll have to do multiple read.
Even without any limiting (sleep) occuring, this means that it's possible to be still slower than the rate limit set.
There was a problem hiding this comment.
That could possibly explain why CI is failing on windows.
Any suggestions for limit setup in tests, and how much of an assert window to provide?
There was a problem hiding this comment.
I've changed the window to 1s, with a 1:2 window to bucket ratio. I'll see how that behaves in CI, then adjust the assert window accordingly.
There was a problem hiding this comment.
Oh sorry the thing I said is outdated, normally the overhead isn't strong enough to make the whole thing fail (that was a bug on a version that I forgot I fixed)
You should be fine with this assert now
There was a problem hiding this comment.
yeah. Got things working with windows, but now mac is failing. takes about 20s. I'll account for mac, and assuming that's good, and green across the board, ill hit merge. Thanks!
🤖 Generated by Copilot at c71d8d6
Summary
🚀🛠️🧪
This pull request adds bandwidth limiting to the bootstrap client and server using the
stream_limitercrate. It changes the type of themax_bytes_read_writefield in the bootstrap config structs and files fromf64tou64, and updates the tests and port numbers accordingly.Walkthrough
stream_limitercrate and use it to wrapTcpStreamin the bootstrap client and server binders (link, link, link, link, link, link, link)Limiterwrapper to set the read and write timeouts and transfer the message bytes (link, link, link, link, link)Limiterwrapper by sending and receiving long error messages (link, link, link, link, link, link, link)