Only send one collation per relay parent at a time to validators #3360
Conversation
This changes the way we are sending collations to validators. Before we answered every collation request immediatley. Now we only answer one pov request at a time per relay parent. This should bring down the bandwidth requirements and should help parachains to include bigger blocks more easily.
There was a problem hiding this comment.
Looks good. What we could do about the DOS by a single bad validator would be to not only move on with the queue on completion of the transfer, but also after some relatively short timeout (short enough so another validator will have enough time to do the fetch). So we won't cancel, but instead if a validator was too slow it will stop getting our full bandwidth at some delay (as apparently it does not utilize it fully anyway).
| /// Stores the state for waiting collation fetches. | ||
| #[derive(Default)] | ||
| struct WaitingCollationFetches { | ||
| /// Is there currently a collation being fetched? |
There was a problem hiding this comment.
getting fetched would make it a bit more clear what's happening, I think.
| async fn wait_for_collation_fetch(active: &mut ActiveCollationFetches) -> Hash { | ||
| loop { | ||
| if active.is_empty() { | ||
| futures::pending!() |
There was a problem hiding this comment.
I mean you defined that function locally in the same function that uses it with select, but it still looks like standalone code, but it really is not. A comment referring to the select below would help, I think.
There was a problem hiding this comment.
pending!() doesn't automatically wake up. Is that really what you want here?
There was a problem hiding this comment.
I think it is - look at the select! below. There is not much better to do that returning Pending in that case and we will be woken up on new messages, which could lead to filling up the FuturesUnordered again. I had a similar use in my code just now and ended up writing the future myself with poll_fn as it becomes more clear what is going on that way, also then the code is tightly coupled with the other future, which makes their inter-dependency more obvious. If one sticks to those high level primitives, I would not know a better way of writing this either.
There was a problem hiding this comment.
Yes I want this ;) This FuturesUnordered returns Ready(None) if it is empty, so not that helpful and can not be filled anywhere in between.
There was a problem hiding this comment.
You can use select_next_some instead fwiw: https://docs.rs/futures/0.3.15/futures/stream/trait.StreamExt.html#method.select_next_some
| @@ -75,6 +82,7 @@ impl Metrics { | |||
| struct MetricsInner { | |||
| advertisements_made: prometheus::Counter<prometheus::U64>, | |||
| collations_sent: prometheus::Counter<prometheus::U64>, | |||
| collations_sent_requested: prometheus::Counter<prometheus::U64>, | |||
There was a problem hiding this comment.
Nit: I think grammatically it would be more correct to use "send". In this case "requested" gets the 3rd participle form.
|
bot merge |
|
Waiting for commit status. |
* master: (21 commits) cleanup stream polls (#3397) Staking Miner (#3141) Companion for Substrate#8953 (#3140) Bump version, specs & substrate in prep for v0.9.8 (#3387) Fix busy loops. (#3392) Minor refactor (#3386) add simnet tests (#3381) BEEFY: adjust gossip (#3372) Companion for #9193 (#3376) Companion for Decouple Staking and Election - Part 3: Signed Phase (#2793) Ensure that we fetch another collation if the first collation was invalid (#3362) Only send one collation per relay parent at a time to validators (#3360) disable approval-checking-grandpa on dev chain (#3364) Use associated constant for max (#3375) Use wasm-builder from git (#3354) Squashed 'bridges/' changes from b2099c5..23dda62 (#3369) Bump versions & spec_versions (#3368) Don't allow bids for a ParaId where there is an overlapping lease period (#3361) Companion for upgrade of transaction-payment to pallet macro (#3267) Do not allow any crowdloan contributions during the VRF period (#3346) ...
This changes the way we are sending collations to validators. Before we
answered every collation request immediatley. Now we only answer one
pov request at a time per relay parent. This should bring down the
bandwidth requirements and should help parachains to include bigger
blocks more easily.