-
Notifications
You must be signed in to change notification settings - Fork 243
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
Limit bundle weight based on target domain block weight #2563
Conversation
7b7c7a5
to
9cbefd7
Compare
let max_bundle_weight = domain_block_limit.max_block_weight | ||
/ (expected_bundles_per_block + std_of_expected_bundles_per_block); | ||
|
||
(max_bundle_weight, domain_block_limit.max_block_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.
@NingLin-P do you recall if we discussed anything about limiting bundle size too based on expected number of bundles?
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.
Yes, I believe we also need to limit the bundle size here.
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.
@ParthDesai let's do it the same way as weight then:
(max_bundle_weight, domain_block_limit.max_block_size) | |
let max_bundle_size = domain_block_limit.max_block_size | |
/ (expected_bundles_per_block + std_of_expected_bundles_per_block); | |
(max_bundle_weight, max_bundle_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.
This implements the client side of the bundle weight/size limit check, we also need the same check on the consensus runtime.
let max_bundle_weight = domain_block_limit.max_block_weight | ||
/ (expected_bundles_per_block + std_of_expected_bundles_per_block); | ||
|
||
(max_bundle_weight, domain_block_limit.max_block_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.
Yes, I believe we also need to limit the bundle size here.
@dariolina @NingLin-P Can you tell me how can we limit the bundle size? Is there a formula, or need to be inferred from the max block size and expected number of bundles? |
Closing in favor of #2568 |
Description
This PR implements formula described in #2413 to limit the probability of exceeding the
TargetDomainBlockWeight
. RefactoringMaxDomainBlockWeight
toTargetDomainBlockWeight
will be done in separate PR.First two commits slightly refactor the
DomainBundleProposer
for required data to be available. Last commit actually implements the formula calculation.Fixes: #2413
Code contributor checklist: