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

Add storage size component to weights #12277

Merged
merged 41 commits into from
Sep 28, 2022

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Sep 15, 2022

This PR adds the long-awaited storage size weight component to the Weight struct in sp-weights.

Part of paritytech/polkadot-sdk#256.

@KiChjang KiChjang added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 15, 2022
)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
pub struct Weight {
#[codec(compact)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need a migration?

Copy link
Member

Choose a reason for hiding this comment

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

yes. but the whole PR will need migration, since weight is now 2-dimensional, so good to have both of these changes in at once.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just ideas and nits.

primitives/weights/src/weight_v2.rs Outdated Show resolved Hide resolved
Comment on lines +126 to +129
Self {
ref_time: self.ref_time.saturating_add(rhs.ref_time),
proof_size: self.proof_size.saturating_add(rhs.proof_size),
}
Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

Just an idea: Could have opted for a private field iter, such that this becomes something like:

Suggested change
Self {
ref_time: self.ref_time.saturating_add(rhs.ref_time),
proof_size: self.proof_size.saturating_add(rhs.proof_size),
}
self.fields_iter().map(|s| saturating_add(s, rhs)).collect::<Self>()

Then it is future proof when changing or adding fields. But could be overkill for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That also assumes that all fields are u64, which might not be the case in the future.

Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

No... it just assumes that they are Saturating. This iter would probably not be a homogeneous collection but a custom type.
I just saw that my example code does not work, since it would need to be zipped with the rhs.fields_iter() first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That, and the fact that all the fields must be homogeneous, because you're mapping over a collection of Items, which must have the same type.

@@ -230,14 +300,20 @@ impl Zero for Weight {
impl Add for Weight {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we could make this std only, since the runtime should not use unsafe math anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Not really unsafe math.

Comment on lines +413 to +416
*self = Self {
ref_time: self.ref_time + other.ref_time,
proof_size: self.proof_size + other.proof_size,
};
Copy link
Member

Choose a reason for hiding this comment

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

Is Rust not default implementing these?!

Suggested change
*self = Self {
ref_time: self.ref_time + other.ref_time,
proof_size: self.proof_size + other.proof_size,
};
*self = *self + other

Comment on lines +422 to +425
*self = Self {
ref_time: self.ref_time - other.ref_time,
proof_size: self.proof_size - other.proof_size,
};
Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

Suggested change
*self = Self {
ref_time: self.ref_time - other.ref_time,
proof_size: self.proof_size - other.proof_size,
};
*self = *self - other

Copy link
Contributor

@BustaNit BustaNit left a comment

Choose a reason for hiding this comment

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

fmt lgtm

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@@ -230,14 +300,20 @@ impl Zero for Weight {
impl Add for Weight {
Copy link
Member

Choose a reason for hiding this comment

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

Not really unsafe math.

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

needs migration before we can merge right?

@shawntabrizi shawntabrizi added E5-breaksapi and removed B0-silent Changes should not be mentioned in any release notes labels Sep 15, 2022
@KiChjang
Copy link
Contributor Author

What I just realized is that we don't need a CompactAs implementation when the fields of the Weight struct are compact by default. In that case, when Weight appears in a parameter of an extrinsic, no #[pallet::compact] is necessary at all.

// Try decoding it in the old way where we only had a non-compact ref time weight
let ref_time = u64::decode(input)
.map_err(|e| e.chain("Could not decode `Weight::ref_time`"))?;
return Ok(Self { ref_time, proof_size: 0 })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm not 100% certain about here is this: for 1D Weight structs that were originally marked with #[compact], would the above code be enough to migrate the old struct over to the new 2D Weight struct?

Copy link
Member

Choose a reason for hiding this comment

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

You could write a small test that encoded a WeightV1 and tries to decode it as new WeightV2. Probably need to account for the compact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit difficult, since it requires a place where I can annotate #[pallet::compact], and the only place I know is within the parameters of a dispatchable function. I'm not sure exactly how I can encode a Weight in the old way and try decoding it in the new way -- you can't really suddenly change the Weight parameter to a 2D weight type from a 1D one, yeah?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if i really understood your pain-point, but is this not enough?

#[test]
fn compact_weight_v1_can_be_decoded_as_v2() {
	use codec::{Decode, Encode, Compact};
	type WeightV1 = u64;

	let weight_v1: WeightV1 = 12345;
	let compact_weight_v1 = Compact(weight_v1);
	let encoded = Compact::<WeightV1>::encode(&compact_weight_v1);

	// Decode as weight v2
	let decoded: Weight = Decode::decode(&mut &encoded[..]).unwrap();
	assert_eq!(decoded.ref_time(), weight_v1);
}

Copy link
Contributor Author

@KiChjang KiChjang Sep 18, 2022

Choose a reason for hiding this comment

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

It is not a bad unit test on its own, but it doesn't really test the #[pallet::compact] OldWeight to the NewWeight transition directly. This is important, as we don't know whether the bytes passed to decode is wrapped inside a Compact or that it's already been unwrapped.

@KiChjang
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 17c07af into master Sep 28, 2022
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/storage-size-weights branch September 28, 2022 10:21
ggwpez added a commit that referenced this pull request Sep 30, 2022
bkontur pushed a commit that referenced this pull request Oct 3, 2022
* Add storage size component to weights

* Rename storage_size to proof_size

* Update primitives/weights/src/weight_v2.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fixes

* cargo fmt

* Implement custom Decode and CompactAs

* Add missing import

* Fixes

* Remove CompactAs implementation

* Properly migrate from 1D weight

* Remove #[pallet::compact] from Weight parameters

* More #[pallet::compact] removals

* Add unit tests

* Set appropriate default block proof size

* cargo fmt

* Remove nonsensical weight constant

* Test only for the reference time weight in frame_system::limits

* Only check for reference time weight on idle

* Use destructuring syntax

* Update test expectations

* Fixes

* Fixes

* Fixes

* Correctly migrate from 1D weights

* cargo fmt

* Migrate using extra extrinsics instead of custom Decode

* Fixes

* Silence dispatch call warnings that were previously allowed

* Fix gas_left test

* Use OldWeight instead of u64

* Fixes

* Only check for reference time weight in election provider

* Fix test expectations

* Fix test expectations

* Use only reference time weight in grandpa test

* Use only reference time weight in examples test

* Use only reference time weight in examples test

* Fix test expectations

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
coderobe added a commit that referenced this pull request Oct 4, 2022
coderobe added a commit that referenced this pull request Oct 6, 2022
* Revert "Add storage size component to weights (#12277)"

This reverts commit 17c07af.

* Revert "Properly set the max proof size weight on defaults and tests (#12383)"

This reverts commit 61b9a4d.
@nazar-pc
Copy link
Contributor

nazar-pc commented Nov 9, 2022

Hm... this is really annoying for chains that don't have PoV, now I need to learn the concept which is not applicable to the chain whatsoever. Is there a chance to make it optional somehow?

@KiChjang
Copy link
Contributor Author

KiChjang commented Nov 9, 2022

You can do so by setting the max proof size to u64::MAX. The docs somewhere should mention this, but unsure where exactly.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis/1026/2

wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Dec 6, 2022
## fixes KILTProtocol/ticket#2289 and KILTProtocol/ticket#2296
* Upgrades from Polkadot v0.9.29 to v0.9.32
* Adds missing feature implementations for all tomls (checked via
`subalfred check features` in all crates)
* Actually necessary for application of
paritytech/substrate#10592 (see runtime changes
in
[dd81eac](dd81eac))
* Migrates Democracy, Preimage and Scheduler pallets to use bounded
Calls, see below

## Summary of changes (Polkadot v0.9.30-0.9.32)

* Weights v2 are not fully there yet, but the struct now includes the
[second field for storage
size](paritytech/substrate#12277) (tracking
issue: https://github.com/paritytech/substrate/issues/12176)

### Breaking Changes
* Breaking: Outer enums
(paritytech/substrate#11981)
  * `Origin` --> `RuntimeOrigin`
  * `Call` --> `RuntimeCall`
  * `Event` --> `RuntimeEvent`
* ~Convention seems to be to keep `Event`, `Call`, `Origin` for inner
pallet usage, e.g. `Did::Origin`~ Update: We use `Runtime` prefix
internally as well

### Noteworthy PRs
* paritytech/substrate#12109
* paritytech/substrate#12328
* paritytech/cumulus#1585
* Following the effort of decoupling collators and full relay nodes,
this PR adds the possibility of pointing the collator to an “external”
(non in-process) relay node. This is still considered experimental, and
the **relay node is suggested to run on the same machine than the
collator for the moment**.
* To specify the relay full node rpc: `polkadot-parachain --alice
--collator --relay-chain-rpc-url <rpc-websocket-url>`
* paritytech/substrate#12486
* Before this change only the interpreted WASM executor was included in
per default compilations. Making the compiled executor opt-in, now,
compiled WASM executor is set by default and an opt-out instead. This
could lead to **big performance difference** between using these two, as
more recent versions of the interpreter see a regression in performance.

### Scheduler, Preimage, Democracy Migration

* paritytech/substrate#11649
* Referenda, Democracy, Scheduler and Preimage pallets are all now
bounded in storage access footprint
* Removed the concept of a "hard deadline" or weight-override priority
and no longer guarantees that at least one scheduled item will be
executed per block (since these are both dangerous to parachains which
have a strict need of weight limits). This means you must ensure that
scheduled items are below the MaximumWeight or they will not be
executed.
* Interesting comment:
paritytech/substrate#11649 (comment)

> There is migration code which moves existing proposals and referenda
over to the new format. However IT DOES NOT MIGRATE EVERYTHING:
> 
> * Preimages are **NOT** migrated. Any registered preimages in
Democracy at the time of migration are dropped. Their balance is **NOT
UNRESERVED**.
> * The re-dispatcher used in the old Democracy implementation is
removed. Any proposals scheduled for dispatch by Democracy **WILL NOT
EXECUTE**.
>
> This means you SHOULD ensure that:
> 
> * **the preimage for the runtime upgrade is placed as an imminent
preimage, not with a deposit;**
> * **no other preimages are in place at the time of upgrade;**
> * **there are no other proposals scheduled for dispatch by Democracy
at the time of upgrade.**
> 
> The Democracy pallet will be marked as deprecated immediately once
Referenda is considered production-ready. **ALL TEAMS ARE RECOMMENDED TO
SWITCH SWAY FROM DEMOCRACY PALLET TO REFERENDA/CONVICTION-VOTING PALLETS
ASAP**

#### Result of `try-runtime` against Spiritnet on Friday Nov 18, 2022:
```
2022-11-18 09:27:23.917  INFO                 main runtime::preimage::migration::v1: Migrating 0 images
2022-11-18 09:27:23.917  INFO                 main runtime::scheduler::migration: Trying to migrate 0 agendas...
2022-11-18 09:27:23.917  INFO                 main runtime::scheduler::migration: Migrated 0 agendas.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: 0 public proposals will be migrated.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: 25 referenda will be migrated.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #7
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #20
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #13
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #5
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #8
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #1
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #19
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #9
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #16
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #14
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #21
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #15
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #24
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #22
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #2
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #10
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #0
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #6
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #11
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #3
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #17
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #18
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #23
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #4
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #25
2022-11-18 09:27:23.918  INFO                 main runtime::democracy::migration::v1: 0 public proposals migrated, 25 referenda migrated
```

## Checklist:

- [x] I have verified that the code works
- [x] No panics! (checked arithmetic ops, no indexing `array[3]` use
`get(3)`, ...)
- [x] I have verified that the code is easy to understand
  - [ ] If not, I have left a well-balanced amount of inline comments
- [x] I have [left the code in a better
state](https://deviq.com/principles/boy-scout-rule)
- [x] I have documented the changes (where applicable)
saraswatpuneet added a commit to frequency-chain/frequency that referenced this pull request Dec 13, 2022
# Goal
The goal of this PR is to upgrade substrate to polkadot release .9.30

Closes #704 

Notes:

[PR for Substrate upgrade to
0.9.30](https://github.com/substrate-developer-hub/substrate-parachain-template/pull/133)
[Release
Notes](https://github.com/paritytech/polkadot/releases/tag/v0.9.30)
# Major updates from 0.9.29 to 0.9.30: 

* Only breaking changes: Rename of Enums in Pallet Config
    ```Call``` -> ```RuntimeCall```
    ```Event``` -> ```RuntimeEvent``` and 
    ```Origin``` -> ```RuntimeOrigin``` . 
Here is the PR paritytech/substrate#11981

* Upgrades from Polkadot v0.9.29 to v0.9.30, version update in
dependencies tree

* Weights update: V2 is slowly getting released and benign updates
carried in this PR. https://github.com/paritytech/substrate/issues/12176
and paritytech/substrate#12277

* @enddynayn @shannonwells some updates in vesting pallet
paritytech/substrate#12109
* @wilwade some pubsub stuff
paritytech/substrate#12328
* @wilwade  paritytech/cumulus#1585 
* @shannonwells some updates in Democracy
paritytech/substrate#11649


# Checklist
- [ ] n/a Chain spec updated
- [ ] n/a Custom RPC OR Runtime API added/changed? Updated
js/api-augment.
- [ ] n/a Design doc(s) updated
- [ ] n/a Tests added
- [ ] n/a Benchmarks added
- [x] Weights updated
- [x] Run benchmarks

Co-authored-by: Dmitri <4452412+demisx@users.noreply.github.com>
Co-authored-by: Jenkins <jenkins@frequency.xyz>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add storage size component to weights

* Rename storage_size to proof_size

* Update primitives/weights/src/weight_v2.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fixes

* cargo fmt

* Implement custom Decode and CompactAs

* Add missing import

* Fixes

* Remove CompactAs implementation

* Properly migrate from 1D weight

* Remove #[pallet::compact] from Weight parameters

* More #[pallet::compact] removals

* Add unit tests

* Set appropriate default block proof size

* cargo fmt

* Remove nonsensical weight constant

* Test only for the reference time weight in frame_system::limits

* Only check for reference time weight on idle

* Use destructuring syntax

* Update test expectations

* Fixes

* Fixes

* Fixes

* Correctly migrate from 1D weights

* cargo fmt

* Migrate using extra extrinsics instead of custom Decode

* Fixes

* Silence dispatch call warnings that were previously allowed

* Fix gas_left test

* Use OldWeight instead of u64

* Fixes

* Only check for reference time weight in election provider

* Fix test expectations

* Fix test expectations

* Use only reference time weight in grandpa test

* Use only reference time weight in examples test

* Use only reference time weight in examples test

* Fix test expectations

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants