-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Remove Ord impl for Weights V2 and add comparison fns #12183
Changes from 2 commits
7b573cd
36ee9ee
02ea3c1
6154116
b5247bd
ab81288
7a298c2
db8e5eb
ae64eb0
09ff913
9bf2428
cddea50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,8 @@ where | |
) -> Result<(), TransactionValidityError> { | ||
let max = T::BlockWeights::get().get(info.class).max_extrinsic; | ||
match max { | ||
Some(max) if info.weight > max => Err(InvalidTransaction::ExhaustsResources.into()), | ||
Some(max) if info.weight.any_gt(max) => | ||
Err(InvalidTransaction::ExhaustsResources.into()), | ||
_ => Ok(()), | ||
} | ||
} | ||
|
@@ -144,18 +145,18 @@ where | |
|
||
// Check if we don't exceed per-class allowance | ||
match limit_per_class.max_total { | ||
Some(max) if per_class > max => return Err(InvalidTransaction::ExhaustsResources.into()), | ||
Some(max) if per_class.any_gt(max) => return Err(InvalidTransaction::ExhaustsResources.into()), | ||
// There is no `max_total` limit (`None`), | ||
// or we are below the limit. | ||
_ => {}, | ||
} | ||
|
||
// In cases total block weight is exceeded, we need to fall back | ||
// to `reserved` pool if there is any. | ||
if all_weight.total() > maximum_weight.max_block { | ||
if all_weight.total().any_gt(maximum_weight.max_block) { | ||
match limit_per_class.reserved { | ||
// We are over the limit in reserved pool. | ||
Some(reserved) if per_class > reserved => | ||
Some(reserved) if per_class.any_gt(reserved) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be problematic, because some solo chains don't care about the storage size and only the computational time component... one solution that I have in mind is to add a feature flag for ignoring PoV sizes in weights, that then would allow the code above to still work as intended. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Features should be our last resort. They are really annoying to deal with and not a good fit here (is standalone vs parachain really additive?). But since we decided There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be not disable size metering for solo chains at all? Aka all Weights on solo chains should have Otherwise, adding some feature doesn't sound like a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making the 👎 to features, as they have bitten us in the past. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, then the code above should be fine as-is, it's then up to the solo-chain maintainer to put an appropriate value for the max size allowed? |
||
return Err(InvalidTransaction::ExhaustsResources.into()), | ||
// There is either no limit in reserved pool (`None`), | ||
// or we are below the limit. | ||
|
@@ -238,7 +239,7 @@ where | |
} | ||
|
||
let unspent = post_info.calc_unspent(info); | ||
if unspent > Weight::zero() { | ||
if unspent.any_gt(Weight::zero()) { | ||
crate::BlockWeight::<T>::mutate(|current_weight| { | ||
current_weight.sub(unspent, info.class); | ||
}) | ||
|
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.
would be good if @athei checks this.
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.
That looks good. I imagine the gas meter will be used also size metering and then it needs to error out if any of those is exhausted. We could replace this by an
ensure!
, though.