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

Fuzzer for Pallet Bags List #9851

Merged
merged 17 commits into from
Oct 31, 2021
Merged

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Sep 24, 2021

This PR adds a fuzzer for the SortedListProvider API exposed by pallet-bags-list.

This PR adds a fuzzer for the `SortedListProvider` API exposed by pallet-bags-list.
@emostov emostov 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Sep 24, 2021
let id = account_id_seed % ID_RANGE;

match account_id_seed % OPTIONS {
0 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can use an enum here to specify which operation is which number. transmute will work, more options here: https://stackoverflow.com/questions/28028854/how-do-i-match-enum-values-with-an-integer/29530566

Copy link
Contributor Author

@emostov emostov Sep 25, 2021

Choose a reason for hiding this comment

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

Nice, lmk what you think about the style of my approach - a bit more verbose than using transmute, but I rather avoid unsafe code if its not neccesary

ids: Vec<(AccountId, VoteWeight)>,
}

impl ExtBuilder {
/// Add some AccountIds to insert into `List`.
#[cfg(not(feature = "fuzz"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, why?

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 was an attempt to get the CI not break when it sees unused code .. but seems to be causing other issues in the CI now

assert!(BagsList::contains(&id));
},
1 => {
if !BagsList::contains(&id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

arguably we could also let the raw apis be accessed without us helping it such as "insert before update if it is not there". I think on_update handles non-existing ids, right?

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 handles non-existing ids by just ignoring them. I changed it to make sure we exercise that codepath. My thinking though was the faster we hit the max number of ids the better.

7111f38

},
}

assert!(BagsList::sanity_check().is_ok());
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a good time for you to go and re-read sanity_check and make sure it is checking everything there is to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found one small check to add 7cca905

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks reasonable. I think our CI team already runs this stuff for a few hours per day or sth. Let's also make sure we give it a solid days/week.

@emostov
Copy link
Contributor Author

emostov commented Sep 24, 2021

Not sure if there is a better way to share this data, but here is stats from a 22 hour run on commit 7d43123

Screen Shot 2021-09-24 at 4 08 07 PM

@emostov
Copy link
Contributor Author

emostov commented Oct 13, 2021

From aa8a523 :
Screen Shot 2021-10-13 at 3 00 15 PM

@emostov
Copy link
Contributor Author

emostov commented Oct 31, 2021

bot merge

@gui1117
Copy link
Contributor

gui1117 commented Oct 31, 2021

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@shawntabrizi
Copy link
Member

@emostov can you post the updated status from the running process?

@paritytech-processbot paritytech-processbot bot merged commit 26d69bc into master Oct 31, 2021
@paritytech-processbot paritytech-processbot bot deleted the zeke-bags-list-fuzzer branch October 31, 2021 21:10
@emostov
Copy link
Contributor Author

emostov commented Oct 31, 2021

Status as of October 31st:

Screen Shot 2021-10-31 at 10 33 54 PM

grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Fuzzer for Pallet Bags List

* Some small updates

* Fuzzer for Pallet Bags List

This PR adds a fuzzer for the `SortedListProvider` API exposed by pallet-bags-list.

* Feature gate code NOT used by fuzz feature

* Create Enum for list actions

* fix some small mistakes

* try and make CI happy

* fmt

* Do not insert before updating

* clean up some misc. comments

* marginally improve Node::sanity_check

* Change ID_RANGE to 25_000

* comma

* try improve correct feature gating so no unused code

Co-authored-by: thiolliere <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Fuzzer for Pallet Bags List

* Some small updates

* Fuzzer for Pallet Bags List

This PR adds a fuzzer for the `SortedListProvider` API exposed by pallet-bags-list.

* Feature gate code NOT used by fuzz feature

* Create Enum for list actions

* fix some small mistakes

* try and make CI happy

* fmt

* Do not insert before updating

* clean up some misc. comments

* marginally improve Node::sanity_check

* Change ID_RANGE to 25_000

* comma

* try improve correct feature gating so no unused code

Co-authored-by: thiolliere <[email protected]>
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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants