Fuzzer for Pallet Bags List (#9851)

* 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 <gui.thiolliere@gmail.com>

substrate/Cargo.lock

@@ -5180,6 +5180,16 @@ dependencies = [
  "sp-tracing",
 ]
 
+[[package]]
+name = "pallet-bags-list-fuzzer"
+version = "4.0.0-dev"
+dependencies = [
+ "frame-election-provider-support",
+ "honggfuzz",
+ "pallet-bags-list",
+ "rand 0.8.4",
+]
+
 [[package]]
 name = "pallet-balances"
 version = "4.0.0-dev"

substrate/Cargo.toml

@@ -135,6 +135,7 @@ members = [
 	"frame/utility",
 	"frame/vesting",
 	"frame/bags-list",
+	"frame/bags-list/fuzzer",
 	"primitives/api",
 	"primitives/api/proc-macro",
 	"primitives/api/test",

substrate/frame/bags-list/Cargo.toml

@@ -63,4 +63,10 @@ runtime-benchmarks = [
 	"sp-tracing",
 	"frame-election-provider-support/runtime-benchmarks",
 ]
+fuzz = [
+  "sp-core",
+	"sp-io",
+  "pallet-balances",
+	"sp-tracing",
+]
 

substrate/frame/bags-list/fuzzer/.gitignore

@@ -0,0 +1,2 @@
+hfuzz_target
+hfuzz_workspace

substrate/frame/bags-list/fuzzer/Cargo.toml

@@ -0,0 +1,22 @@
+[package]
+name = "pallet-bags-list-fuzzer"
+version = "4.0.0-dev"
+authors = ["Parity Technologies <admin@parity.io>"]
+edition = "2018"
+license = "Apache-2.0"
+homepage = "https://substrate.dev"
+repository = "https://github.com/paritytech/substrate/"
+description = "Fuzzer for FRAME pallet bags list"
+readme = "README.md"
+publish = false
+
+[dependencies]
+honggfuzz = "0.5"
+rand = { version = "0.8", features = ["std", "small_rng"] }
+
+pallet-bags-list = { version = "4.0.0-dev", features = ["fuzz"], path = ".." }
+frame-election-provider-support = { version = "4.0.0-dev", path = "../../election-provider-support", features = ["runtime-benchmarks"] }
+
+[[bin]]
+name = "bags-list"
+path = "src/main.rs"

substrate/frame/bags-list/fuzzer/src/main.rs

@@ -0,0 +1,88 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2021 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! # Running
+//! Running this fuzzer can be done with `cargo hfuzz run bags-list`. `honggfuzz` CLI options can
+//! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads.
+//!
+//! # Debugging a panic
+//! Once a panic is found, it can be debugged with
+//! `cargo hfuzz run-debug fixed_point hfuzz_workspace/bags_list/*.fuzz`.
+//!
+//! # More information
+//! More information about `honggfuzz` can be found
+//! [here](https://docs.rs/honggfuzz/).
+
+use frame_election_provider_support::{SortedListProvider, VoteWeight};
+use honggfuzz::fuzz;
+use pallet_bags_list::mock::{AccountId, BagsList, ExtBuilder};
+use std::convert::From;
+
+const ID_RANGE: AccountId = 25_000;
+
+/// Actions of a `SortedListProvider` that we fuzz.
+enum Action {
+	Insert,
+	Update,
+	Remove,
+}
+
+impl From<u32> for Action {
+	fn from(v: u32) -> Self {
+		let num_variants = Self::Remove as u32 + 1;
+		match v % num_variants {
+			_x if _x == Action::Insert as u32 => Action::Insert,
+			_x if _x == Action::Update as u32 => Action::Update,
+			_x if _x == Action::Remove as u32 => Action::Remove,
+			_ => unreachable!(),
+		}
+	}
+}
+
+fn main() {
+	ExtBuilder::default().build_and_execute(|| loop {
+		fuzz!(|data: (AccountId, VoteWeight, u32)| {
+			let (account_id_seed, vote_weight, action_seed) = data;
+
+			let id = account_id_seed % ID_RANGE;
+			let action = Action::from(action_seed);
+
+			match action {
+				Action::Insert => {
+					if BagsList::on_insert(id.clone(), vote_weight).is_err() {
+						// this was a duplicate id, which is ok. We can just update it.
+						BagsList::on_update(&id, vote_weight);
+					}
+					assert!(BagsList::contains(&id));
+				},
+				Action::Update => {
+					let already_contains = BagsList::contains(&id);
+					BagsList::on_update(&id, vote_weight);
+					if already_contains {
+						assert!(BagsList::contains(&id));
+					}
+				},
+				Action::Remove => {
+					BagsList::on_remove(&id);
+					assert!(!BagsList::contains(&id));
+				},
+			}
+
+			assert!(BagsList::sanity_check().is_ok());
+		})
+	});
+}

substrate/frame/bags-list/src/lib.rs

@@ -59,8 +59,8 @@ use sp_std::prelude::*;
 mod benchmarks;
 
 mod list;
-#[cfg(test)]
-mod mock;
+#[cfg(any(test, feature = "fuzz"))]
+pub mod mock;
 #[cfg(test)]
 mod tests;
 pub mod weights;

substrate/frame/bags-list/src/list/mod.rs

@@ -391,8 +391,8 @@ impl<T: Config> List<T> {
 	///
 	/// * there are no duplicate ids,
 	/// * length of this list is in sync with `CounterForListNodes`,
-	/// * and sanity-checks all bags. This will cascade down all the checks and makes sure all bags
-	///   are checked per *any* update to `List`.
+	/// * and sanity-checks all bags and nodes. This will cascade down all the checks and makes sure
+	/// all bags and nodes are checked per *any* update to `List`.
 	#[cfg(feature = "std")]
 	pub(crate) fn sanity_check() -> Result<(), &'static str> {
 		use frame_support::ensure;
@@ -414,7 +414,6 @@ impl<T: Config> List<T> {
 			let thresholds = T::BagThresholds::get().iter().copied();
 			let thresholds: Vec<u64> = if thresholds.clone().last() == Some(VoteWeight::MAX) {
 				// in the event that they included it, we don't need to make any changes
-				// Box::new(thresholds.collect()
 				thresholds.collect()
 			} else {
 				// otherwise, insert it here.
@@ -774,10 +773,13 @@ impl<T: Config> Node<T> {
 			"node does not exist in the expected bag"
 		);
 
+		let non_terminal_check = !self.is_terminal() &&
+			expected_bag.head.as_ref() != Some(id) &&
+			expected_bag.tail.as_ref() != Some(id);
+		let terminal_check =
+			expected_bag.head.as_ref() == Some(id) || expected_bag.tail.as_ref() == Some(id);
 		frame_support::ensure!(
-			!self.is_terminal() ||
-				expected_bag.head.as_ref() == Some(id) ||
-				expected_bag.tail.as_ref() == Some(id),
+			non_terminal_check || terminal_check,
 			"a terminal node is neither its bag head or tail"
 		);
 

substrate/frame/bags-list/src/mock.rs

@@ -101,12 +101,13 @@ pub(crate) const GENESIS_IDS: [(AccountId, VoteWeight); 4] =
 	[(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)];
 
 #[derive(Default)]
-pub(crate) struct ExtBuilder {
+pub struct ExtBuilder {
 	ids: Vec<(AccountId, VoteWeight)>,
 }
 
 impl ExtBuilder {
 	/// Add some AccountIds to insert into `List`.
+	#[cfg(test)]
 	pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, VoteWeight)>) -> Self {
 		self.ids = ids;
 		self
@@ -126,18 +127,20 @@ impl ExtBuilder {
 		ext
 	}
 
-	pub(crate) fn build_and_execute(self, test: impl FnOnce() -> ()) {
+	pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
 		self.build().execute_with(|| {
 			test();
 			List::<Runtime>::sanity_check().expect("Sanity check post condition failed")
 		})
 	}
 
+	#[cfg(test)]
 	pub(crate) fn build_and_execute_no_post_check(self, test: impl FnOnce() -> ()) {
 		self.build().execute_with(test)
 	}
 }
 
+#[cfg(test)]
 pub(crate) mod test_utils {
 	use super::*;
 	use list::Bag;