Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove Ord impl for Weights V2 and add comparison fns #12183

Merged
merged 12 commits into from
Sep 8, 2022
2 changes: 1 addition & 1 deletion frame/collective/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
ensure!(proposal_len <= length_bound, Error::<T, I>::WrongProposalLength);
let proposal = ProposalOf::<T, I>::get(hash).ok_or(Error::<T, I>::ProposalMissing)?;
let proposal_weight = proposal.get_dispatch_info().weight;
ensure!(proposal_weight <= weight_bound, Error::<T, I>::WrongProposalWeight);
ensure!(proposal_weight.all_lte(weight_bound), Error::<T, I>::WrongProposalWeight);
Ok((proposal, proposal_len as usize))
}

Expand Down
2 changes: 1 addition & 1 deletion frame/contracts/src/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ where

// NOTE that it is ok to allocate all available gas since it still ensured
// by `charge` that it doesn't reach zero.
if self.gas_left < amount {
if self.gas_left.any_lt(amount) {
Copy link
Contributor

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.

Copy link
Member

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.

Err(<Error<T>>::OutOfGas.into())
} else {
self.gas_left -= amount;
Expand Down
2 changes: 1 addition & 1 deletion frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ pub mod pallet {
let size = Self::snapshot_metadata().ok_or(Error::<T>::MissingSnapshotMetadata)?;

ensure!(
Self::solution_weight_of(&raw_solution, size) < T::SignedMaxWeight::get(),
Self::solution_weight_of(&raw_solution, size).all_lt(T::SignedMaxWeight::get()),
Error::<T>::SignedTooMuchWeight,
);

Expand Down
28 changes: 15 additions & 13 deletions frame/election-provider-multi-phase/src/unsigned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,17 +638,19 @@ impl<T: MinerConfig> Miner<T> {
};

let next_voters = |current_weight: Weight, voters: u32, step: u32| -> Result<u32, ()> {
match current_weight.cmp(&max_weight) {
Ordering::Less => {
let next_voters = voters.checked_add(step);
match next_voters {
Some(voters) if voters < max_voters => Ok(voters),
_ => Err(()),
}
},
Ordering::Greater => voters.checked_sub(step).ok_or(()),
Ordering::Equal => Ok(voters),
if current_weight.all_lt(max_weight) {
let next_voters = voters.checked_add(step);
match next_voters {
Some(voters) if voters < max_voters => Ok(voters),
_ => Err(()),
}
} else if current_weight.all_gt(max_weight) {
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
voters.checked_sub(step).ok_or(())
} else {
// If any of the constituent weights is equal to the max weight, we're at max
Ok(voters)
}
// TODO: What to do when time weight is greater and size weight is lesser (and vice versa)?
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
};

// First binary-search the right amount of voters
Expand All @@ -672,16 +674,16 @@ impl<T: MinerConfig> Miner<T> {

// Time to finish. We might have reduced less than expected due to rounding error. Increase
// one last time if we have any room left, the reduce until we are sure we are below limit.
while voters < max_voters && weight_with(voters + 1) < max_weight {
while voters < max_voters && weight_with(voters + 1).all_lt(max_weight) {
voters += 1;
}
while voters.checked_sub(1).is_some() && weight_with(voters) > max_weight {
while voters.checked_sub(1).is_some() && weight_with(voters).any_gt(max_weight) {
voters -= 1;
}

let final_decision = voters.min(size.voters);
debug_assert!(
weight_with(final_decision) <= max_weight,
weight_with(final_decision).all_lte(max_weight),
"weight_with({}) <= {}",
final_decision,
max_weight,
Expand Down
2 changes: 1 addition & 1 deletion frame/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ where
let max_weight = <System::BlockWeights as frame_support::traits::Get<_>>::get().max_block;
let remaining_weight = max_weight.saturating_sub(weight.total());

if remaining_weight > Weight::zero() {
if remaining_weight.all_gt(Weight::zero()) {
let used_weight = <AllPalletsWithSystem as OnIdle<System::BlockNumber>>::on_idle(
block_number,
remaining_weight,
Expand Down
2 changes: 1 addition & 1 deletion frame/multisig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ impl<T: Config> Pallet<T> {

if let Some((call, call_len)) = maybe_approved_call {
// verify weight
ensure!(call.get_dispatch_info().weight <= max_weight, Error::<T>::MaxWeightTooLow);
ensure!(call.get_dispatch_info().weight.any_lte(max_weight), Error::<T>::MaxWeightTooLow);
KiChjang marked this conversation as resolved.
Show resolved Hide resolved

// Clean up storage before executing call to avoid an possibility of reentrancy
// attack.
Expand Down
2 changes: 1 addition & 1 deletion frame/scheduler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ pub mod pallet {
let hard_deadline = s.priority <= schedule::HARD_DEADLINE;
let test_weight =
total_weight.saturating_add(call_weight).saturating_add(item_weight);
if !hard_deadline && order > 0 && test_weight > limit {
if !hard_deadline && order > 0 && test_weight.any_gt(limit) {
// Cannot be scheduled this block - postpone until next.
total_weight.saturating_accrue(T::WeightInfo::item(false, named, None));
if let Some(ref id) = s.maybe_id {
Expand Down
8 changes: 4 additions & 4 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1571,10 +1571,10 @@ pub mod pallet {
/// to kick people under the new limits, `chill_other` should be called.
// We assume the worst case for this call is either: all items are set or all items are
// removed.
#[pallet::weight(max(
T::WeightInfo::set_staking_configs_all_set(),
T::WeightInfo::set_staking_configs_all_remove()
))]
#[pallet::weight(
T::WeightInfo::set_staking_configs_all_set()
.max(T::WeightInfo::set_staking_configs_all_remove())
)]
pub fn set_staking_configs(
origin: OriginFor<T>,
min_nominator_bond: ConfigOp<BalanceOf<T>>,
Expand Down
58 changes: 56 additions & 2 deletions frame/support/src/weights/weight_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ use super::*;
Clone,
RuntimeDebug,
Default,
Ord,
PartialOrd,
CompactAs,
)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
Expand Down Expand Up @@ -163,6 +161,62 @@ impl Weight {
pub const fn zero() -> Self {
Self { ref_time: 0 }
}

/// Returns true if any of `self`'s constituent weights is strictly greater than that of the
/// `other`'s, otherwise returns false.
pub const fn any_gt(self, other: Self) -> bool {
self.ref_time > other.ref_time
}

/// Returns true if all of `self`'s constituent weights is strictly greater than that of the
/// `other`'s, otherwise returns false.
pub const fn all_gt(self, other: Self) -> bool {
self.ref_time > other.ref_time
}

/// Returns true if any of `self`'s constituent weights is strictly less than that of the
/// `other`'s, otherwise returns false.
pub const fn any_lt(self, other: Self) -> bool {
self.ref_time < other.ref_time
}

/// Returns true if all of `self`'s constituent weights is strictly less than that of the
/// `other`'s, otherwise returns false.
pub const fn all_lt(self, other: Self) -> bool {
self.ref_time < other.ref_time
}

/// Returns true if any of `self`'s constituent weights is greater than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn any_gte(self, other: Self) -> bool {
self.ref_time >= other.ref_time
}

/// Returns true if all of `self`'s constituent weights is greater than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn all_gte(self, other: Self) -> bool {
self.ref_time >= other.ref_time
}

/// Returns true if any of `self`'s constituent weights is less than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn any_lte(self, other: Self) -> bool {
self.ref_time <= other.ref_time
}

/// Returns true if all of `self`'s constituent weights is less than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn all_lte(self, other: Self) -> bool {
self.ref_time <= other.ref_time
}

/// Returns true if any of `self`'s constituent weights is equal to that of the `other`'s,
/// otherwise returns false.
pub const fn any_eq(self, other: Self) -> bool {
self.ref_time == other.ref_time
}

// NOTE: `all_eq` does not exist, as it's simply the `eq` method from the `PartialEq` trait.
}

impl Zero for Weight {
Expand Down
11 changes: 6 additions & 5 deletions frame/system/src/extensions/check_weight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(()),
}
}
Expand Down Expand Up @@ -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) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 Weight shouldn't be generic it is the only way. IMHO we should rethink that. Having Weight as an associated type on frame_system::Config wouldn't probably introduce too many new type parameters as anything is generic over T: Config anyways.

Copy link
Member

Choose a reason for hiding this comment

The 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 size = 0?

Otherwise, adding some feature doesn't sound like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the size = 0, or max-size = infinity should allow solo-chains.

👎 to features, as they have bitten us in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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);
})
Expand Down
18 changes: 10 additions & 8 deletions frame/system/src/limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,16 +229,18 @@ impl BlockWeights {
// Make sure that if total is set it's greater than base_block &&
// base_for_class
error_assert!(
(max_for_class > self.base_block && max_for_class > base_for_class)
(max_for_class.all_gt(self.base_block) && max_for_class.all_gt(base_for_class))
|| max_for_class == Weight::zero(),
&mut error,
"[{:?}] {:?} (total) has to be greater than {:?} (base block) & {:?} (base extrinsic)",
class, max_for_class, self.base_block, base_for_class,
);
// Max extrinsic can't be greater than max_for_class.
error_assert!(
weights.max_extrinsic.unwrap_or(Weight::zero()) <=
max_for_class.saturating_sub(base_for_class),
weights
.max_extrinsic
.unwrap_or(Weight::zero())
.all_lte(max_for_class.saturating_sub(base_for_class)),
&mut error,
"[{:?}] {:?} (max_extrinsic) can't be greater than {:?} (max for class)",
class,
Expand All @@ -247,14 +249,14 @@ impl BlockWeights {
);
// Max extrinsic should not be 0
error_assert!(
weights.max_extrinsic.unwrap_or_else(Weight::max_value) > Weight::zero(),
weights.max_extrinsic.unwrap_or_else(Weight::max_value).all_gt(Weight::zero()),
&mut error,
"[{:?}] {:?} (max_extrinsic) must not be 0. Check base cost and average initialization cost.",
class, weights.max_extrinsic,
);
// Make sure that if reserved is set it's greater than base_for_class.
error_assert!(
reserved > base_for_class || reserved == Weight::zero(),
reserved.all_gt(base_for_class) || reserved == Weight::zero(),
&mut error,
"[{:?}] {:?} (reserved) has to be greater than {:?} (base extrinsic) if set",
class,
Expand All @@ -263,7 +265,7 @@ impl BlockWeights {
);
// Make sure max block is greater than max_total if it's set.
error_assert!(
self.max_block >= weights.max_total.unwrap_or(Weight::zero()),
self.max_block.all_gte(weights.max_total.unwrap_or(Weight::zero())),
&mut error,
"[{:?}] {:?} (max block) has to be greater than {:?} (max for class)",
class,
Expand All @@ -272,7 +274,7 @@ impl BlockWeights {
);
// Make sure we can fit at least one extrinsic.
error_assert!(
self.max_block > base_for_class + self.base_block,
self.max_block.all_gt(base_for_class + self.base_block),
&mut error,
"[{:?}] {:?} (max block) must fit at least one extrinsic {:?} (base weight)",
class,
Expand Down Expand Up @@ -400,7 +402,7 @@ impl BlockWeightsBuilder {
// compute max block size.
for class in DispatchClass::all() {
weights.max_block = match weights.per_class.get(*class).max_total {
Some(max) if max > weights.max_block => max,
Some(max) => max.max(weights.max_block),
_ => weights.max_block,
};
}
Expand Down
2 changes: 1 addition & 1 deletion frame/whitelist/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ pub mod pallet {
.map_err(|_| Error::<T>::UndecodableCall)?;

ensure!(
call.get_dispatch_info().weight <= call_weight_witness,
call.get_dispatch_info().weight.all_lte(call_weight_witness),
Error::<T>::InvalidCallWeightWitness
);

Expand Down