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

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Nov 2, 2021

should be ready, but I need to a final review myself

@gui1117 gui1117 changed the title whitelist pallet new pallet: whitelist pallet Nov 2, 2021
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 2, 2021
@xlc
Copy link
Contributor

xlc commented Nov 2, 2021

We have already implemented exactly this in our pallet open-web3-stack/open-runtime-module-library#603

@gui1117 gui1117 force-pushed the gui-shawntabrizi-preimage-pallet branch from ddc71ca to dd84eda Compare November 24, 2021 00:44
@gui1117 gui1117 changed the base branch from shawntabrizi-preimage-pallet to gav-schedule-by-hash November 24, 2021 00:44
@gui1117 gui1117 force-pushed the gui-shawntabrizi-preimage-pallet branch 3 times, most recently from 0eb5166 to 81d8e9e Compare December 1, 2021 07:05
Base automatically changed from gav-schedule-by-hash to master December 11, 2021 14:55
@gui1117 gui1117 force-pushed the gui-shawntabrizi-preimage-pallet branch from 0f0530a to 4fe4ca3 Compare December 13, 2021 07:00
@gui1117 gui1117 force-pushed the gui-shawntabrizi-preimage-pallet branch from 4fe4ca3 to 4826e1e Compare December 13, 2021 07:04
@gui1117 gui1117 marked this pull request as ready for review December 13, 2021 07:19
@gui1117 gui1117 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed A3-in_progress Pull request is in progress. No review needed at this stage. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Dec 13, 2021
@viniul viniul added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 17, 2022
Comment on lines 39 to 44
assert_err!(
Whitelist::whitelist_call(Origin::signed(1), call_hash),
DispatchError::BadOrigin,
);

assert!(!Preimage::preimage_requested(&call_hash));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_err!(
Whitelist::whitelist_call(Origin::signed(1), call_hash),
DispatchError::BadOrigin,
);
assert!(!Preimage::preimage_requested(&call_hash));
assert_noop!(
Whitelist::whitelist_call(Origin::signed(1), call_hash),
DispatchError::BadOrigin,
);

Copy link
Member

@shawntabrizi shawntabrizi Jan 19, 2022

Choose a reason for hiding this comment

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

more stuff noop below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes should be fixed by 7020ecd

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
Copy link
Member

ggwpez commented Feb 25, 2022

/bench runtime pallet pallet_whitelist

@parity-benchapp
Copy link

parity-benchapp bot commented Feb 25, 2022

Benchmark Runtime Pallet for branch "gui-shawntabrizi-preimage-pallet" with command cargo run --quiet --profile=production --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_whitelist --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/whitelist/src/weights.rs --template=./.maintain/frame-weight-template.hbs

Toolchain: stable-x86_64-unknown-linux-gnu (default)
rustc 1.57.0 (f1edd0429 2021-11-29)

Results
Pallet: "pallet_whitelist", Extrinsic: "whitelist_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    16.12
              µs

Reads = 2
Writes = 2

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    16.12
              µs

Reads = 2
Writes = 2

Pallet: "pallet_whitelist", Extrinsic: "remove_whitelisted_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)
Storage: Preimage PreimageFor (r:0 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    17.79
              µs

Reads = 2
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    17.79
              µs

Reads = 2
Writes = 3

Pallet: "pallet_whitelist", Extrinsic: "dispatch_whitelisted_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage PreimageFor (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=     6696
              µs

Reads = 3
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=     6696
              µs

Reads = 3
Writes = 3

Pallet: "pallet_whitelist", Extrinsic: "dispatch_whitelisted_call_with_preimage", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)
Storage: Preimage PreimageFor (r:0 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    20.48
    + n    0.001
              µs

Reads = 2 + (0 * n)
Writes = 3 + (0 * n)

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    n   mean µs  sigma µs       %
    1     19.83     0.183    0.9%
  200     20.73     0.112    0.5%
  399     21.19     0.068    0.3%
  598     21.35      0.14    0.6%
  797     21.89     0.128    0.5%
  996     21.95     0.166    0.7%
 1195     22.15      0.11    0.4%
 1394      22.7     0.126    0.5%
 1593     22.85     0.131    0.5%
 1792     23.05     0.171    0.7%
 1991     23.56     0.076    0.3%
 2190     24.05     0.171    0.7%
 2389     24.25     0.208    0.8%
 2588     24.71     0.161    0.6%
 2787     24.76     0.169    0.6%
 2986        25     0.161    0.6%
 3185     25.35     0.233    0.9%
 3384     25.63     0.192    0.7%
 3583     25.95     0.121    0.4%
 3782     26.12     0.164    0.6%
 3981     26.53     0.161    0.6%
 4180     26.69     0.069    0.2%
 4379      27.2     0.308    1.1%
 4578     27.39     0.183    0.6%
 4777     27.74     0.193    0.6%
 4976     27.99     0.065    0.2%
 5175      28.5     0.139    0.4%
 5374     28.84     0.135    0.4%
 5573     28.71     0.285    0.9%
 5772     29.27     0.284    0.9%
 5971     29.39     0.123    0.4%
 6170     29.96     0.244    0.8%
 6369     30.05     0.102    0.3%
 6568     30.23     0.185    0.6%
 6767     30.93     0.114    0.3%
 6966     31.29     0.124    0.3%
 7165     31.68     0.227    0.7%
 7364     31.71     0.159    0.5%
 7563     31.88     0.136    0.4%
 7762     32.25     0.123    0.3%
 7961     32.72     0.216    0.6%
 8160     32.86     0.158    0.4%
 8359     33.43     0.218    0.6%
 8558     33.44     0.094    0.2%
 8757     34.01     0.217    0.6%
 8956     34.31      0.36    1.0%
 9155     34.46     0.254    0.7%
 9354     34.84     0.232    0.6%
 9553      35.1      0.18    0.5%
 9752     35.11      0.13    0.3%
 9951     35.41     0.233    0.6%

Quality and confidence:
param     error
n             0

Model:
Time ~=    20.45
    + n    0.002
              µs

Reads = 2 + (0 * n)
Writes = 3 + (0 * n)


…--manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_whitelist --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/whitelist/src/weights.rs --template=./.maintain/frame-weight-template.hbs
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member

ggwpez commented Feb 25, 2022

/bench runtime pallet pallet_whitelist

@parity-benchapp
Copy link

parity-benchapp bot commented Feb 25, 2022

Benchmark Runtime Pallet for branch "gui-shawntabrizi-preimage-pallet" with command cargo run --quiet --profile=production --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_whitelist --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/whitelist/src/weights.rs --template=./.maintain/frame-weight-template.hbs

Toolchain: stable-x86_64-unknown-linux-gnu (default)
rustc 1.57.0 (f1edd0429 2021-11-29)

Results
Pallet: "pallet_whitelist", Extrinsic: "whitelist_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    16.25
              µs

Reads = 2
Writes = 2

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    16.25
              µs

Reads = 2
Writes = 2

Pallet: "pallet_whitelist", Extrinsic: "remove_whitelisted_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)
Storage: Preimage PreimageFor (r:0 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    18.34
              µs

Reads = 2
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    18.34
              µs

Reads = 2
Writes = 3

Pallet: "pallet_whitelist", Extrinsic: "dispatch_whitelisted_call", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage PreimageFor (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=     6618
              µs

Reads = 3
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=     6618
              µs

Reads = 3
Writes = 3

Pallet: "pallet_whitelist", Extrinsic: "dispatch_whitelisted_call_with_preimage", Lowest values: [], Highest values: [], Steps: 50, Repeat: 20
Raw Storage Info
========
Storage: Whitelist WhitelistedCall (r:1 w:1)
Storage: Preimage StatusFor (r:1 w:1)
Storage: Preimage PreimageFor (r:0 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    20.62
    + n    0.001
              µs

Reads = 2 + (0 * n)
Writes = 3 + (0 * n)

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    n   mean µs  sigma µs       %
    1     19.89     0.072    0.3%
  200     21.02     0.127    0.6%
  399     21.45     0.139    0.6%
  598     21.65     0.238    1.0%
  797     22.15     0.147    0.6%
  996      22.1     0.102    0.4%
 1195     22.42     0.159    0.7%
 1394     22.88     0.085    0.3%
 1593     22.82     0.167    0.7%
 1792     23.27     0.085    0.3%
 1991     23.67     0.137    0.5%
 2190     23.82     0.106    0.4%
 2389     23.98     0.113    0.4%
 2588     24.84     0.176    0.7%
 2787     24.93     0.111    0.4%
 2986      25.1     0.148    0.5%
 3185     25.41     0.263    1.0%
 3384     25.61     0.151    0.5%
 3583     26.19     0.099    0.3%
 3782     26.25     0.099    0.3%
 3981      26.6     0.232    0.8%
 4180     27.17     0.367    1.3%
 4379     27.04     0.169    0.6%
 4578     27.22     0.211    0.7%
 4777     27.77     0.199    0.7%
 4976     27.97     0.158    0.5%
 5175     29.69     0.847    2.8%
 5374      28.8     0.124    0.4%
 5573     28.95     0.209    0.7%
 5772     29.12     0.215    0.7%
 5971     29.54     0.093    0.3%
 6170     29.91     0.114    0.3%
 6369     30.49     0.173    0.5%
 6568     30.64     0.181    0.5%
 6767     30.68     0.138    0.4%
 6966     31.16      0.12    0.3%
 7165     31.54     0.214    0.6%
 7364     31.62     0.219    0.6%
 7563     32.31      0.23    0.7%
 7762     32.36     0.206    0.6%
 7961     32.85     0.175    0.5%
 8160     33.07     0.208    0.6%
 8359     32.97     0.185    0.5%
 8558      33.4     0.183    0.5%
 8757     34.11     0.223    0.6%
 8956     33.89      0.24    0.7%
 9155     34.88     0.285    0.8%
 9354      34.8     0.133    0.3%
 9553     34.89     0.171    0.4%
 9752     35.36     0.299    0.8%
 9951     35.55     0.128    0.3%

Quality and confidence:
param     error
n             0

Model:
Time ~=    20.61
    + n    0.002
              µs

Reads = 2 + (0 * n)
Writes = 3 + (0 * n)


Parity Bot and others added 2 commits February 25, 2022 15:05
…--manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_whitelist --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/whitelist/src/weights.rs --template=./.maintain/frame-weight-template.hbs
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
.ok_or(Error::<T>::UnavailablePreImage)?;

let call = <T as Config>::Call::decode_all_with_depth_limit(
sp_api::MAX_EXTRINSIC_DEPTH,
Copy link
Member

Choose a reason for hiding this comment

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

For completeness we may need to add a bench which tests it with nested calls.
Currently we only bench the length of an encoded call, but if nesting is more expensive than length, it would be missed.

Copy link
Member

Choose a reason for hiding this comment

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

I dont think nesting makes a difference here. The depth limit here is to prevent against stackoverflow and other kinds of call decoding bombs. But the actual computational overhead here should be the same with anything being encoded or decoded really.

And then the weight of the call can help us determine cheaply and ahead of time that the call itself is okay.

@shawntabrizi
Copy link
Member

bot merge

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. 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. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants