-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
The Not sure what is nicer from the naming, but when we do not want to litter the use frame_support::weights::v2::Weight;
// vs
use frame_support::weights::Weight2 as Weight; |
FRAME is oblivious to this concept. PoV is what a collator sends to a validator for validation. In the cumulus case this is roughly a witness + extrinsics. For the extrinsics we already account in FRAME (this is the block lenght). The witness is what is missing from the equation. This PoV metering just assumes that accessing storage will grow the witness. |
Thanks for the explanation @athei ! |
I think we still need to run the bench bot, to see if the template still works. |
@@ -673,11 +675,11 @@ impl pallet_election_provider_multi_phase::MinerConfig for Runtime { | |||
// The unsigned submissions have to respect the weight of the submit_unsigned call, thus their | |||
// weight estimate function is wired to this call's weight. | |||
fn solution_weight(v: u32, t: u32, a: u32, d: u32) -> Weight { | |||
< | |||
Weight::from_computation(< |
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.
does the WeightInfo
not now give the new Weight
back from its functions?
@@ -39,7 +39,7 @@ | |||
#![allow(unused_parens)] | |||
#![allow(unused_imports)] | |||
|
|||
use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; | |||
use frame_support::{traits::Get, weights::{ComputationWeight as Weight, constants::RocksDbWeight}}; |
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.
Shouldn't the weights.rs
files be returning the full weight, not just computation weight? Where would we expect the PoV bandwidth weight dimension to come from?
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, this is a needed follow up to regenerate weights with the proper template and values. This is just a backwards compat hack for now.
} | ||
} | ||
|
||
/// Construct with computation weight only (zero `bandwith`). |
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.
/// Construct with computation weight only (zero `bandwith`). | |
/// Construct with computation weight only (zero `bandwidth`). |
} | ||
} | ||
|
||
/// Try to add some `other` weight while upholding the `limit`. |
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.
Needs explicitly mentioning what happens in case of overflow or exceeding any of the limits
} | ||
} | ||
|
||
/// Get the aggressive max of `self` and `other` weight. |
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.
Unclear what does aggressive mean here
pub const MAX: Self = | ||
Self { computation: ComputationWeight::MAX, bandwidth: BandwidthWeight::MAX }; | ||
|
||
/// Get the conservative min of `self` and `other` weight. |
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.
same as aggressive
Canonical names:
Would be good to make a function explicitly named for backwards compatibility, like |
self.computation < other.computation || self.bandwidth < other.bandwidth | ||
} | ||
|
||
/// Checks if any param of `self` is less than `other`. |
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.
/// Checks if any param of `self` is less than `other`. | |
/// Checks if any param of `self` is greater than `other`. |
This is an initial PR where we change the
Weight
type from an primitiveu64
to:Fields are private to allow adding of new fields without needing to touch everything, again.
The strategy here is to migrate the core weight checking logic within
frame_system
andframe_support
. Majority of pallets will still usetype ComputationWeight = u64
. We will convert all the places that still use the primitive types in a follow up PR.Right now the
bandwidth
parameter correctly handled inframe_executive
andframe_system
, and we will make sure that new extrinsics do not bypass that limit, but basically all Pallets simply return 0 bandwidth for now, so this logic is not really used.The
check_weight
extension takes the bandwith into account. Thetransaction-payment
extension only looks at the computation. We will need to come up with a way to fee PoV usage in a follow up. So right now we only make sure that not too muchbandwith
is used but don't incur a fee for it.For migrating existing pallets to the new
Weight
definition we will simply use aWeight
instance withcomputation: weight_v1, bandwidth: 0
. The computation part can be accessed with a simple getterweight.computation()
.After this PR is merged, each pallet can be migrated one by one to correctly handle the
bandwidth
parameter.WeightV1
and replace it by a normalWeight
with computation = 0.Weight
private so we can add more fields later without breaking all the usersfixes #9584
Polkadot companion: paritytech/polkadot#5435