feat(consensus): push all ingress messages #2233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yvonneanne
approved these changes
Oct 24, 2024
dist1ll
approved these changes
Oct 24, 2024
nmattia
pushed a commit
that referenced
this pull request
Oct 25, 2024
Currently, we push ingress messages only if the size of the message is below 1024 bytes (see [link](https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/p2p/consensus_manager/src/sender.rs?L232-234)), meaning that for large enough ingress messages, a node first has to see an advert and then request to download the advertized ingress message from a peer, before it can include the message in a block, potentially adding a couple hundreds of milliseconds to the end-to-end ingress message latency. The benefits are even bigger in the context of hashes-in-blocks feature, where we rely on existence of ingress messages in the pool when validating blocks received from peers - pushing ingress messages should increase the chances that all the ingress messages referenced by a "stripped block" are already in the ingress pool, so we don't have to fetch them from peers. One downside of always pushing ingress messages could be a wasted bandwith, i.e. node might receive an ingress message from a peer and then immediately discard it because the [bouncer function](https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/ingress_manager/src/bouncer.rs?L32-40) deemed the ingress message to be not needed (e.g. if the ingress message already expired, from the node point's of view), but this should not be a rather rare occurrence (we actively purge expired ingress messages).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we push ingress messages only if the size of the message is below 1024 bytes (see link), meaning that for large enough ingress messages, a node first has to see an advert and then request to download the advertized ingress message from a peer, before it can include the message in a block, potentially adding a couple hundreds of milliseconds to the end-to-end ingress message latency.
The benefits are even bigger in the context of hashes-in-blocks feature, where we rely on existence of ingress messages in the pool when validating blocks received from peers - pushing ingress messages should increase the chances that all the ingress messages referenced by a "stripped block" are already in the ingress pool, so we don't have to fetch them from peers.
One downside of always pushing ingress messages could be a wasted bandwith, i.e. node might receive an ingress message from a peer and then immediately discard it because the bouncer function deemed the ingress message to be not needed (e.g. if the ingress message already expired, from the node point's of view), but this should not be a rather rare occurrence (we actively purge expired ingress messages).