-
Notifications
You must be signed in to change notification settings - Fork 247
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
Implement bundle limit runtime api to limit bundle weight and size #2568
Conversation
c09b6f3
to
722a195
Compare
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.
Make sense! just a few nits.
Signed-off-by: linning <[email protected]>
…main_registry and ensure bundle limit can be calculated in can_instantiate_domain Signed-off-by: linning <[email protected]>
…api-for-bundle-weight
GitHub has issues today, see https://status.github.com/ |
This PR is ready to review, PTAL when you get a chance @vedhavyas @dariolina thanks! |
The test is quite hard to follow but the calculations are correct. |
Will look at this today @NingLin-P |
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.
Make sense to me.
Left some questions and nits
crates/pallet-domains/src/lib.rs
Outdated
// This represents: Ceil[2*Sqrt[bundle_slot_probability/SLOT_PROBABILITY]]) | ||
let std_of_expected_bundles_per_block = expected_bundles_per_block | ||
.integer_sqrt() | ||
.checked_mul(2)? |
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.
@dariolina wouldn't it be ideal to make this constant specific to each domain rather than all domains as is?
Some domain executions might not need to more restrictive secifically substrate based one where we would know to a good level what will the execution time while EVM based domains might not be the case and would need to be mor restrictive to avoid longer execution times
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.
Sorry just saw it now. I think it makes sense to let each domain decide via something like "target block time confidence" with levels like 1-3 or smth. I also think we should first address #2365 refactor though.
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 not a blocker for this PR but can you create an issue so that we can track this change please?
Signed-off-by: linning <[email protected]>
…api-for-bundle-weight
Signed-off-by: linning <[email protected]>
Description
This PR implements formula described in #2413 to limit the probability of exceeding the
TargetDomainBlockWeight
. RefactoringMaxDomainBlockWeight
toTargetDomainBlockWeight
will be done in separate PR.This PR takes different approach than #2563 by introducing runtime api and both consensus runtime as well as the client use the underlying logic. The calculation is also now using
checked_*
api to prevent overflow or division by zero.Fixes: #2413
Code contributor checklist: