-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[Feat] Chunked Message #4055
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.
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
base: staging
Are you sure you want to change the base?
[Feat] Chunked Message #4055
Conversation
Signed-off-by: ljedrz <[email protected]>
Signed-off-by: ljedrz <[email protected]>
vicsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate the work here. In hindsight, a design document first might have been more efficient. But then again seeing all the specific code is very helpful.
I fail to see two benefits of this chunking:
- it may speed up transmission of large objects, but benchmarks we ran showed requesting 5 blocks at a time was faster than asking single blocks at a time. I guess these are trade-offs with a lot of factors and there's no general rule given our complex codebase.
- we may make it harder to DoS. But actually for most messages/events we can just set a much lower message/event-specific size. The one and only exception is
BlockResponse, for which the node is always in control of how much it is requesting, so there's no DoS vector if it's designed right?
|
|
||
| /// The maximum size of a message that can be transmitted in the network. | ||
| pub(crate) const MAXIMUM_MESSAGE_SIZE: usize = 128 * 1024 * 1024; // 128 MiB | ||
| // TODO: with message chunking in place, it can be greatly reduced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we surely need is message/event-specific limits. For the ones with a compile-time sizeof you could already set it a sensible lower value. We could also set consts whose correctness is checked in a unit test...
We can't really do that (at least not as quickly as we'd like - only when parsing) - the
The network message framing we're using is length-based, so as it stands now, someone might declare that they want to send a The true benefits of message chunking is it can reduce (but not remove completely) head-of-line blocking, allowing smaller messages to be sent in between the chunks of a large message; this means that a node that is currently providing blocks to a syncing peer can be more responsive in the meantime (the same applies to the recipient, but they would be less useful in the network until fully synced). This solution is complementary to the aforementioned multi-stream approach (which is a greater mitigation for HoL blocking). |
|
Thank you for explaining. To defend against the attack scenario of spam from large BlockResponses, which can be literally 1000x larger than any other message, maybe you can sketch out different approaches from least to most hacky. If we tackle that, we may be able to lower the message limit to ~100KB and we may in turn not need the chunking. |
|
I've given it some thought, and what I'd consider to be the best (fastest, least hacky, most flexible) approach is short-lived sync streams, which would roughly work as follows:
Benefits:
|
|
Go forth and proof of concept in a new PR ljedrzGPT! |
This PR is a proposal for chunked network messages, specifically the
Message(for now - it could be extended to theEventas well).The way this works is roughly as follows:
Router::sendis calledMessageis check for chunking criteria based on its variant (this can be fine-tuned further, e.g. based on the number of transactions in aBlock, or the number of block locators in aPing), in order to maximize performance - the worst-case scenario is a single-chunk message, but there are no extra allocations just to determine the serialized sizeMessageis chunked, and each chunk gets its own call toRouter::sendAnd when a chunk is received:
Message, it immediately gets deserialized into its true formRouter) is consulted in order to either store the chunk, or finalize theMessagebased on prior chunks plus the new oneThere are 3 TODOs left:
MAXIMUM_MESSAGE_SIZEMessagevariants should undergo chunkingI've tested it in a local devnet and encountered no issues.