Skip to content
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

[FRAME] Warn on unchecked weight witness #1818

Merged
merged 20 commits into from
Oct 10, 2023
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Oct 8, 2023

Adds a warning to FRAME pallets when a function argument that starts with _ is used in the weight formula.
This is in most cases an error since the weight witness needs to be checked.

Example:

#[pallet::call_index(0)]
#[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))]
pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo {
	Ok(().into())
}

Produces this warning:

warning: use of deprecated constant `pallet::warnings::UncheckedWeightWitness_0::_w`: 
                 It is deprecated to not check weight witness data.
                 Please instead ensure that all witness data for weight calculation is checked before usage.
         
                 For more info see:
                     <https://github.com/paritytech/polkadot-sdk/pull/1818>
   --> substrate/frame/system/src/lib.rs:424:40
    |
424 |         pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo {
    |                                              ^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

Can be suppressed like this, since in this case it is legit:

#[pallet::call_index(0)]
#[pallet::weight(T::SystemWeightInfo::remark(remark.len() as u32))]
pub fn remark(_origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
	let _ = remark; // We dont need to check the weight witness.
	Ok(().into())
}

Changes:

  • Add warning on uncheded weight witness
  • Respect subkeys limit in System::kill_prefix
  • Fix HRMP pallet and other warnings
  • Updateproc_macro_warning dependency
  • Delete random folder substrate/src/src 🙈
  • Adding Prdoc

ggwpez added 4 commits October 8, 2023 15:05
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez changed the title Oty unchecked weight witness Warn on unchecked weight witness Oct 8, 2023
@ggwpez ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 8, 2023
ggwpez added 7 commits October 8, 2023 15:22
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez marked this pull request as ready for review October 9, 2023 09:36
@ggwpez ggwpez requested review from a team October 9, 2023 09:36
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez changed the title Warn on unchecked weight witness [FRAME] Warn on unchecked weight witness Oct 9, 2023
prdoc/pr_1818.prdoc Outdated Show resolved Hide resolved
@@ -1,297 +0,0 @@
// This file is part of Substrate.
Copy link
Contributor

Choose a reason for hiding this comment

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

was deleting this file intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as i can tell it was a duplicate of substrate/src/lib.rs. Dont think we need this 🤔

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Looks good except my question about clearing defunct voters.

substrate/frame/sudo/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +8 to +16
migrations:
db: [ ]
runtime: [ ]

host_functions: []

crates:
- name: "frame-support-procedural"
semver: minor
Copy link
Member

Choose a reason for hiding this comment

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

This is still the default schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, i used the prdoc schema and prdoc check.

ggwpez added 3 commits October 9, 2023 17:54
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez enabled auto-merge (squash) October 9, 2023 19:31
@ggwpez ggwpez merged commit 6487749 into master Oct 10, 2023
8 checks passed
@ggwpez ggwpez deleted the oty-unchecked-weight-witness branch October 10, 2023 14:02
ordian added a commit that referenced this pull request Oct 12, 2023
* master: (33 commits)
  ci: set CI_IMAGE back to (now updated) .ci-unified (#1854)
  ci: bump ci image to rust 1.73.0 (#1830)
  Refactor Identity to benchmark v2 (#1838)
  PVF worker: bump landlock, update ABI docs (#1850)
  Xcm emulator nits (#1649)
  Fixes path issue in derive-impl (#1823)
  upgrade to macro_magic 0.4.3 (#1832)
  Use safe math when pruning statuses (#1835)
  remote-ext: fix state download stall on slow connections and reduce memory usage (#1295)
  Update testnet bootnode dns name (#1712)
  [FRAME] Warn on unchecked weight witness (#1818)
  [xcm] Use `Weight::MAX` for `reserve_asset_deposited`, `receive_teleported_asset` benchmarks (#1726)
  Update bridges subtree (#1803)
  Check for parent of first ready block being on chain (#1812)
  Make CheckNonce refuse transactions signed by accounts with no providers (#1578)
  Fix Asset Hub collator crashing when starting from genesis (#1788)
  Mixnet integration (#1346)
  [xcm-emulator] Decouple the `AccountId` type from `AccountId32` (#1458)
  Treasury spends various asset kinds (#1333)
  chore: bump zombienter version (#1806)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Adds a warning to FRAME pallets when a function argument that starts
with `_` is used in the weight formula.
This is in most cases an error since the weight witness needs to be
checked.

Example:

```rust
#[pallet::call_index(0)]
#[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))]
pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo {
	Ok(().into())
}
```

Produces this warning:

```pre
warning: use of deprecated constant `pallet::warnings::UncheckedWeightWitness_0::_w`: 
                 It is deprecated to not check weight witness data.
                 Please instead ensure that all witness data for weight calculation is checked before usage.
         
                 For more info see:
                     <paritytech#1818>
   --> substrate/frame/system/src/lib.rs:424:40
    |
424 |         pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo {
    |                                              ^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default
```

Can be suppressed like this, since in this case it is legit:

```rust
#[pallet::call_index(0)]
#[pallet::weight(T::SystemWeightInfo::remark(remark.len() as u32))]
pub fn remark(_origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
	let _ = remark; // We dont need to check the weight witness.
	Ok(().into())
}
```

Changes:
- Add warning on uncheded weight witness
- Respect `subkeys` limit in `System::kill_prefix`
- Fix HRMP pallet and other warnings
- Update`proc_macro_warning` dependency
- Delete random folder `substrate/src/src` 🙈 
- Adding Prdoc

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants