-
Notifications
You must be signed in to change notification settings - Fork 1k
Limit the size of the statement for further gossiping #9965
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
Conversation
…e_dev --bump patch'
|
/cmd prdoc --audience node_dev --bump minor |
|
|
||
| /// Maximum allowed size for a statement notification. | ||
| pub(crate) const MAX_STATEMENT_SIZE: u64 = 256 * 1024; | ||
| pub const MAX_STATEMENT_SIZE: u64 = 256 * 1024; |
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.
Will be 1MiB after #9912
| pub const DEFAULT_MAX_TOTAL_SIZE: usize = 2 * 1024 * 1024 * 1024; // 2GiB | ||
| /// The maximum size of a single statement in bytes. | ||
| /// Accounts for the 1-byte vector length prefix when statements are gossiped as `Vec<Statement>`. | ||
| pub const MAX_STATEMENT_SIZE: usize = sc_network_statement::config::MAX_STATEMENT_SIZE as usize - 1; |
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.
Maybe we can do it the other way around and allow this as is and increase the MAX_STATEMENT_NOTIFICATION_SIZE value to what it is +1.
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.
It creates cyclic package dependency with the other way around.
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.
I was thinking of just moving the -1 from here to +1 there but anyway it's not that important.
| if statement_len > validation.max_size as usize { | ||
| if statement_len > validation.max_size as usize || | ||
| statement.encoded_size() > MAX_STATEMENT_SIZE | ||
| { |
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.
we could also directly reject in the implementation of fn submit.
Both way are good.
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.
Good catch, we can skip expensive validation with the early return in submittion
| pub const DEFAULT_MAX_TOTAL_SIZE: usize = 2 * 1024 * 1024 * 1024; // 2GiB | ||
| /// The maximum size of a single statement in bytes. | ||
| /// Accounts for the 1-byte vector length prefix when statements are gossiped as `Vec<Statement>`. | ||
| pub const MAX_STATEMENT_SIZE: usize = sc_network_statement::config::MAX_STATEMENT_SIZE as usize - 1; |
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.
I don't know how to make this constraint more visible.
People should be aware that this client implementation only support such statement size.
But for now it is good and better than before.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
# Description Fixes gossiping and scalability issues in the statement-store networking. 1. Reduced gossiping traffic by propagating only recent statements instead of all. 2. Added an early check for statements that the node already has to skip duplicate processing. 3. Added splitting of large statement batches to stay under MAX_STATEMENT_NOTIFICATION_SIZE; oversized individual statements are skipped. 4. MAX_STATEMENT_NOTIFICATION_SIZE was updated to the commonly used 1MB, which drastically improved the gossiping speed. 5. Notifications are sent asynchronously. I don't see much difference in performance, but according to @lexnv, it's better to do: #9296. 6. Added a 10s timeout to handle very slow or disconnected peers. ## Integration Internal optimizations to the gossip protocol. No downstream changes required. Related PR: #9965 ## Things to handle in further PRs - After this PR, nodes don't send all statements to new peers anymore, only the recent ones. - After restarting, the node doesn't re-gossip statements it wasn't gossiped. - Broadcasting notifications to all peers when the first peer is slow is limited. We could instead use a FuturesUnordered. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <[email protected]>
# Description Fixes gossiping and scalability issues in the statement-store networking. 1. Reduced gossiping traffic by propagating only recent statements instead of all. 2. Added an early check for statements that the node already has to skip duplicate processing. 3. Added splitting of large statement batches to stay under MAX_STATEMENT_NOTIFICATION_SIZE; oversized individual statements are skipped. 4. MAX_STATEMENT_NOTIFICATION_SIZE was updated to the commonly used 1MB, which drastically improved the gossiping speed. 5. Notifications are sent asynchronously. I don't see much difference in performance, but according to @lexnv, it's better to do: #9296. 6. Added a 10s timeout to handle very slow or disconnected peers. ## Integration Internal optimizations to the gossip protocol. No downstream changes required. Related PR: #9965 ## Things to handle in further PRs - After this PR, nodes don't send all statements to new peers anymore, only the recent ones. - After restarting, the node doesn't re-gossip statements it wasn't gossiped. - Broadcasting notifications to all peers when the first peer is slow is limited. We could instead use a FuturesUnordered. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <[email protected]>
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9965-to-stable2506
git worktree add --checkout .worktree/backport-9965-to-stable2506 backport-9965-to-stable2506
cd .worktree/backport-9965-to-stable2506
git reset --hard HEAD^
git cherry-pick -x de740e1892f47e91c356858324f6e3e4b0f1be60
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9965-to-stable2509
git worktree add --checkout .worktree/backport-9965-to-stable2509 backport-9965-to-stable2509
cd .worktree/backport-9965-to-stable2509
git reset --hard HEAD^
git cherry-pick -x de740e1892f47e91c356858324f6e3e4b0f1be60
git push --force-with-lease |
Fixes gossiping and scalability issues in the statement-store networking. 1. Reduced gossiping traffic by propagating only recent statements instead of all. 2. Added an early check for statements that the node already has to skip duplicate processing. 3. Added splitting of large statement batches to stay under MAX_STATEMENT_NOTIFICATION_SIZE; oversized individual statements are skipped. 4. MAX_STATEMENT_NOTIFICATION_SIZE was updated to the commonly used 1MB, which drastically improved the gossiping speed. 5. Notifications are sent asynchronously. I don't see much difference in performance, but according to @lexnv, it's better to do: #9296. 6. Added a 10s timeout to handle very slow or disconnected peers. Internal optimizations to the gossip protocol. No downstream changes required. Related PR: #9965 - After this PR, nodes don't send all statements to new peers anymore, only the recent ones. - After restarting, the node doesn't re-gossip statements it wasn't gossiped. - Broadcasting notifications to all peers when the first peer is slow is limited. We could instead use a FuturesUnordered. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bastian Köcher <[email protected]> (cherry picked from commit b21cbb5)
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9965-to-unstable2507
git worktree add --checkout .worktree/backport-9965-to-unstable2507 backport-9965-to-unstable2507
cd .worktree/backport-9965-to-unstable2507
git reset --hard HEAD^
git cherry-pick -x de740e1892f47e91c356858324f6e3e4b0f1be60
git push --force-with-lease |
Description
Limits the size of statements that are further gossiped over the network to prevent skipping oversized messages. The limit is set to match the network protocol's max statement notification size (1 MB), accounting for 1-byte vector length overhead because statements are sent as
Vec<Statement>.Integration
Affected crates:
sc-statement-store: Now depends onsc-network-statementfor size constantssc-network-statement:MAX_STATEMENT_NOTIFICATION_SIZEis now publicFor downstream users: