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

Fuzz testing for nomination pools #12002

Merged
merged 19 commits into from
Sep 12, 2022

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Aug 10, 2022

This PR is itself pretty insubstantial, there is not real code change other than adding a fuzz-style test for nomination pools.

Some notes about the approach: For now, I mainly did the fuzz-testing without using a proper fuzzer because it would allow for the mock to be reused. In the past we gated mock.rs behind some feature flag to make it available, and this time around I wanted to try the alternative approach.

Having done this, I can say that the main advantage of this type of testing is seamless setup, and the downside is reproducibility of failing tests.

For now, in order to have reproducible errors, you have to use a seeded randomness, and then re-use it. It requires a little bit of manual work, but it did allow me to solve every single test failure.

It is probably still better to move the fuzz-test use a proper fuzzer as follow-up. Nonetheless, it must be done such that the mock file is reused (a good first issue for a new team member or external contributor).

Some notes about the fuzzing outcome: As you see in the code, I randomly generate calls that may or may not be valid and bombard the pallet with these calls, and every so often I execute the entire set of sanity checks that are defined in the pallet.

Moreover, I inject a single agent into a random pool that is manually tracking the expected reward amount (see Agent in the code for details).

The tests generally get quite slow, particularly if you want to execute the sanity checks often. In the default setup, the state grows until 1000 pools and 10000 members, and with 1 million random submissions, this is the final outcome:

Aug 10 16:16:23.564  INFO runtime::nomination-pools: [1] 🏊‍♂️ running sanity checks at 1010000
Aug 10 16:16:23.640  INFO runtime::nomination-pools: [1] 🏊‍♂️ iteration 1010000, 1000 pools, 10000 members, 325223 ok 684777 err, events = [("Event::Created", 1000), ("Event::Bonded", 72300), ("Event::Unbonded", 41458), ("Event::PaidOut", 164750), ("Event::Withdrawn", 21622), ("Event::MemberRemoved", 1)]

All in all, this was super useful and I feel way more confident about nomination pools now.

  • follow-up Issue to migrate this to a proper fuzzer.

@kianenigma kianenigma 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 Aug 10, 2022
@kianenigma kianenigma marked this pull request as ready for review August 10, 2022 11:48
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. and removed A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 10, 2022
@@ -428,6 +428,13 @@ macro_rules! parameter_types_impl_thread_local {
pub fn set(t: $type) {
[<$name:snake:upper>].with(|v| *v.borrow_mut() = t);
}

/// Mutate the internal value in place.
pub fn mutate<F: FnOnce(&mut $type) -> ()>(mutate: F) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a unrelated change -- small enough to sneak in here, but I can also extract.

@@ -825,7 +827,7 @@ pub fn verify_encoded_lazy<V: Verify, T: codec::Encode>(
macro_rules! assert_eq_error_rate {
($x:expr, $y:expr, $error:expr $(,)?) => {
assert!(
($x) >= (($y) - ($error)) && ($x) <= (($y) + ($error)),
($x >= $crate::Saturating::saturating_sub($y, $error)) && ($x <= $crate::Saturating::saturating_add($y, $error)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a unrelated change -- small enough to sneak in here, but I can also extract.

Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

Really cool to see some fuzzing. Looks good to me!

@ggwpez
Copy link
Member

ggwpez commented Sep 12, 2022

Some notes about the fuzzing outcome: As you see in the code, I randomly generate calls that may or may not be valid and bombard the pallet with these calls, and every so often I execute the entire set of sanity checks that are defined in the pallet.

We could create a tool which generates a set of calls from the metadata of a pallet. Could even be done in PolkadotJS, as general fuzzy testing framework.

frame/nomination-pools/src/tests.rs Outdated Show resolved Hide resolved
// NOTE: use this to get predictable (non)randomness:
// use::{rngs::SmallRng, SeedableRng};
// let mut rng = SmallRng::from_seed([0u8; 32]);
let mut rng = thread_rng();
Copy link
Member

Choose a reason for hiding this comment

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

Good follow up:

From experience I can tell you that it is a good idea to create a random seed, print it, and add the possibility to overide it via env variable.
Then you always have a different set of tests, but if it fails, you can reproduce by using the printed seed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent idea, I actually have this in my code as a commented code that: If you want reproducible output, do this and that, but using a seed and env var makes a whole lot more sense.

let mut rng = thread_rng();
let mut ext = sp_io::TestExternalities::new_empty();
// NOTE: sadly events don't fulfill the requirements of hashmap or btreemap.
let mut events_histogram = Vec::<(PoolsEvents<T>, u32)>::default();
Copy link
Member

Choose a reason for hiding this comment

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

Is it not enough to use the event index, or do you also want the values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah index could have been enough too

frame/nomination-pools/src/tests.rs Outdated Show resolved Hide resolved
});
System::reset_events();

if iteration % ERA == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Some smol doc please.

kianenigma and others added 5 commits September 12, 2022 14:42
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@kianenigma
Copy link
Contributor Author

/cmd queue -c fmt $ 1

@command-bot
Copy link

command-bot bot commented Sep 12, 2022

@kianenigma https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1831586 was started for your command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 5-94895519-352d-4b29-bbfc-766e28330acf to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 12, 2022

@kianenigma Command "$PIPELINE_SCRIPTS_DIR/fmt.sh" 1 has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1831586 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1831586/artifacts/download.

@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit d5de897 into master Sep 12, 2022
@paritytech-processbot paritytech-processbot bot deleted the kiz-pools-battle-testing-2 branch September 12, 2022 14:27
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* some additional tests and stuff

* make sanity public

* add some sort of fuzz test for pools

* breaks every now and then

* breaks every now and then

* IT WORKS AND PASSES 100k TESTS

* cleanup

* safe id addition

* fix assert_eq_error_rate

* Update frame/nomination-pools/src/tests.rs

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

* Update frame/nomination-pools/src/tests.rs

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

* add some doc

* Fix

* ".git/.scripts/fmt.sh" 1

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
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
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants