From 1cf2380c44ff083bbd91df5d4d9ff1d255db5762 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 31 Jul 2022 13:19:51 +0100 Subject: [PATCH 01/14] some additional tests and stuff --- frame/nomination-pools/src/lib.rs | 23 ++++++++++++-- frame/nomination-pools/src/tests.rs | 48 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 09f1746b8e5ba..b2cc51f2a0111 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -965,6 +965,7 @@ impl RewardPool { id: PoolId, bonded_points: BalanceOf, ) -> Result> { + dbg!(&self, &bonded_points); let balance = Self::current_balance(id); let payouts_since_last_record = balance .saturating_add(self.total_rewards_claimed) @@ -2379,7 +2380,7 @@ impl Pallet { /// To cater for tests that want to escape parts of these checks, this function is split into /// multiple `level`s, where the higher the level, the more checks we performs. So, /// `sanity_check(255)` is the strongest sanity check, and `0` performs no checks. - #[cfg(any(test, debug_assertions))] + #[cfg(any(feature = "std", test, debug_assertions))] pub fn sanity_checks(level: u8) -> Result<(), &'static str> { if level.is_zero() { return Ok(()) @@ -2401,12 +2402,30 @@ impl Pallet { } let mut pools_members = BTreeMap::::new(); + let mut pools_members_pending_rewards = BTreeMap::>::new(); let mut all_members = 0u32; PoolMembers::::iter().for_each(|(_, d)| { - assert!(BondedPools::::contains_key(d.pool_id)); + let bonded_pool = BondedPools::::get(d.pool_id).unwrap(); assert!(!d.total_points().is_zero(), "no member should have zero points: {:?}", d); *pools_members.entry(d.pool_id).or_default() += 1; all_members += 1; + + let reward_pool = RewardPools::::get(d.pool_id).unwrap(); + if !bonded_pool.points.is_zero() { + let current_rc = + reward_pool.current_reward_counter(d.pool_id, bonded_pool.points).unwrap(); + *pools_members_pending_rewards.entry(d.pool_id).or_default() += + d.pending_rewards(current_rc).unwrap(); + } // else this pool has been heavily slashed and cannot have any rewards anymore. + }); + + RewardPools::::iter_keys().for_each(|id| { + // the sum of the pending rewards must be less than the leftover balance. Since the + // reward math rounds down, we might accumulate some dust here. + assert!( + RewardPool::::current_balance(id) >= + pools_members_pending_rewards.get(&id).map(|x| *x).unwrap_or_default() + ) }); BondedPools::::iter().for_each(|(id, inner)| { diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index b59f18cca72a2..9dbde7e8e50e8 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1720,6 +1720,54 @@ mod claim_payout { }) } + #[test] + fn bond_extra_pending_rewards_works() { + ExtBuilder::default().add_members(vec![(20, 20)]).build_and_execute(|| { + MaxPoolMembers::::set(None); + MaxPoolMembersPerPool::::set(None); + + // pool receives some rewards. + Balances::mutate_account(&default_reward_account(), |f| f.free += 30).unwrap(); + System::reset_events(); + + // 10 cashes it out, and bonds it. + { + assert_ok!(Pools::claim_payout(Origin::signed(10))); + let (member, _, reward_pool) = Pools::get_member_with_pools(&10).unwrap(); + // there is 30 points and 30 reward points in the system RC is 1. + assert_eq!(member.last_recorded_reward_counter, 1.into()); + assert_eq!(reward_pool.total_rewards_claimed, 10); + // these two are not updated -- only updated when the points change. + assert_eq!(reward_pool.last_recorded_total_payouts, 0); + assert_eq!(reward_pool.last_recorded_reward_counter, 0.into()); + + assert_eq!( + pool_events_since_last_call(), + vec![Event::PaidOut { member: 10, pool_id: 1, payout: 10 }] + ); + } + + // 20 re-bonds it. + { + assert_ok!(Pools::bond_extra(Origin::signed(20), BondExtra::Rewards)); + let (member, _, reward_pool) = Pools::get_member_with_pools(&10).unwrap(); + assert_eq!(member.last_recorded_reward_counter, 1.into()); + assert_eq!(reward_pool.total_rewards_claimed, 30); + // since points change, these two are updated. + assert_eq!(reward_pool.last_recorded_total_payouts, 30); + assert_eq!(reward_pool.last_recorded_reward_counter, 1.into()); + + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::PaidOut { member: 20, pool_id: 1, payout: 20 }, + Event::Bonded { member: 20, pool_id: 1, bonded: 20, joined: false } + ] + ); + } + }) + } + #[test] fn unbond_updates_recorded_data() { ExtBuilder::default() From a07d2db2b38e1b499659e0bc09ae209fd82a8cb6 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 8 Aug 2022 11:39:44 +0100 Subject: [PATCH 02/14] make sanity public --- frame/nomination-pools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index b2cc51f2a0111..8171ead53e824 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -965,7 +965,6 @@ impl RewardPool { id: PoolId, bonded_points: BalanceOf, ) -> Result> { - dbg!(&self, &bonded_points); let balance = Self::current_balance(id); let payouts_since_last_record = balance .saturating_add(self.total_rewards_claimed) @@ -1106,7 +1105,7 @@ impl SubPools { } /// The sum of all unbonding balance, regardless of whether they are actually unlocked or not. - #[cfg(any(test, debug_assertions))] + #[cfg(any(feature = "std", test, debug_assertions))] fn sum_unbonding_balance(&self) -> BalanceOf { self.no_era.balance.saturating_add( self.with_era @@ -2470,6 +2469,7 @@ impl Pallet { sum_unbonding_balance ); } + Ok(()) } From 2bbccc41c37ea842bf723886e4ff08f251d07035 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 9 Aug 2022 19:28:57 +0100 Subject: [PATCH 03/14] add some sort of fuzz test for pools --- Cargo.lock | 75 +++--- frame/nomination-pools/Cargo.toml | 2 + frame/nomination-pools/src/lib.rs | 22 +- frame/nomination-pools/src/tests.rs | 387 ++++++++++++++++++++++++++++ frame/support/src/traits/misc.rs | 15 +- 5 files changed, 447 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 703eeffce54a1..a2338c9ea6c75 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -896,7 +896,7 @@ dependencies = [ "ansi_term", "clap 3.1.18", "node-cli", - "rand 0.8.4", + "rand 0.8.5", "sc-chain-spec", "sc-keystore", "sp-core", @@ -2015,7 +2015,7 @@ dependencies = [ "num-traits", "parity-scale-codec", "parking_lot 0.12.0", - "rand 0.8.4", + "rand 0.8.5", "scale-info", ] @@ -2026,7 +2026,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cfcf0ed7fe52a17a03854ec54a9f76d6d84508d1c0e66bc1793301c73fc8493c" dependencies = [ "byteorder", - "rand 0.8.4", + "rand 0.8.5", "rustc-hex", "static_assertions", ] @@ -2118,7 +2118,7 @@ dependencies = [ "log", "memory-db", "parity-scale-codec", - "rand 0.8.4", + "rand 0.8.5", "rand_pcg 0.3.1", "sc-block-builder", "sc-cli", @@ -2190,7 +2190,7 @@ dependencies = [ "frame-support", "honggfuzz", "parity-scale-codec", - "rand 0.8.4", + "rand 0.8.5", "scale-info", "sp-arithmetic", "sp-npos-elections", @@ -3188,7 +3188,7 @@ dependencies = [ "jsonrpsee-types", "lazy_static", "parking_lot 0.12.0", - "rand 0.8.4", + "rand 0.8.5", "rustc-hash", "serde", "serde_json", @@ -3569,7 +3569,7 @@ dependencies = [ "log", "prost", "prost-build", - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -3596,7 +3596,7 @@ dependencies = [ "pin-project", "prost", "prost-build", - "rand 0.8.4", + "rand 0.8.5", "ring", "rw-stream-sink", "sha2 0.10.2", @@ -3743,7 +3743,7 @@ dependencies = [ "libp2p-core", "libp2p-swarm", "log", - "rand 0.8.4", + "rand 0.8.5", "smallvec", "socket2", "void", @@ -3797,7 +3797,7 @@ dependencies = [ "log", "prost", "prost-build", - "rand 0.8.4", + "rand 0.8.5", "sha2 0.10.2", "snow", "static_assertions", @@ -3871,7 +3871,7 @@ dependencies = [ "prost", "prost-build", "prost-codec", - "rand 0.8.4", + "rand 0.8.5", "smallvec", "static_assertions", "thiserror", @@ -3894,7 +3894,7 @@ dependencies = [ "log", "prost", "prost-build", - "rand 0.8.4", + "rand 0.8.5", "sha2 0.10.2", "thiserror", "unsigned-varint", @@ -4052,7 +4052,7 @@ dependencies = [ "libsecp256k1-core", "libsecp256k1-gen-ecmult", "libsecp256k1-gen-genmult", - "rand 0.8.4", + "rand 0.8.5", "serde", "sha2 0.9.8", "typenum", @@ -4484,7 +4484,7 @@ dependencies = [ "num-complex", "num-rational 0.4.0", "num-traits", - "rand 0.8.4", + "rand 0.8.5", "rand_distr", "simba", "typenum", @@ -4507,7 +4507,7 @@ version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e7d66043b25d4a6cccb23619d10c19c25304b355a7dccd4a8e11423dd2382146" dependencies = [ - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -4667,7 +4667,7 @@ dependencies = [ "pallet-transaction-payment", "parity-scale-codec", "platforms", - "rand 0.8.4", + "rand 0.8.5", "regex", "remote-externalities", "sc-authority-discovery", @@ -5298,7 +5298,7 @@ dependencies = [ "frame-election-provider-support", "honggfuzz", "pallet-bags-list", - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -5452,7 +5452,7 @@ dependencies = [ "pallet-utility", "parity-scale-codec", "pretty_assertions", - "rand 0.8.4", + "rand 0.8.5", "rand_pcg 0.3.1", "scale-info", "serde", @@ -5888,6 +5888,7 @@ dependencies = [ "log", "pallet-balances", "parity-scale-codec", + "rand 0.8.5", "scale-info", "sp-core", "sp-io", @@ -6511,7 +6512,7 @@ dependencies = [ "lz4", "memmap2 0.2.1", "parking_lot 0.11.2", - "rand 0.8.4", + "rand 0.8.5", "snap", ] @@ -7125,7 +7126,7 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "588f6378e4dd99458b60ec275b4477add41ce4fa9f64dcba6f15adccb19b50d6" dependencies = [ - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -7164,20 +7165,19 @@ dependencies = [ "libc", "rand_chacha 0.2.2", "rand_core 0.5.1", - "rand_hc 0.2.0", + "rand_hc", "rand_pcg 0.2.1", ] [[package]] name = "rand" -version = "0.8.4" +version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2e7573632e6454cf6b99d7aac4ccca54be06da05aca2ef7423d22d27d4d4bcd8" +checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" dependencies = [ "libc", "rand_chacha 0.3.0", "rand_core 0.6.2", - "rand_hc 0.3.0", ] [[package]] @@ -7225,7 +7225,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32cb0b9bc82b0a0876c2dd994a7e7a2683d3e7390ca40e6886785ef0c7e3ee31" dependencies = [ "num-traits", - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -7237,15 +7237,6 @@ dependencies = [ "rand_core 0.5.1", ] -[[package]] -name = "rand_hc" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3190ef7066a446f2e7f42e239d161e905420ccab01eb967c9eb27d21b2322a73" -dependencies = [ - "rand_core 0.6.2", -] - [[package]] name = "rand_pcg" version = "0.2.1" @@ -8249,7 +8240,7 @@ dependencies = [ "log", "parity-scale-codec", "parking_lot 0.12.0", - "rand 0.8.4", + "rand 0.8.5", "sc-block-builder", "sc-chain-spec", "sc-client-api", @@ -9357,7 +9348,7 @@ dependencies = [ "futures", "httparse", "log", - "rand 0.8.4", + "rand 0.8.5", "sha-1 0.9.4", ] @@ -9836,7 +9827,7 @@ dependencies = [ "clap 3.1.18", "honggfuzz", "parity-scale-codec", - "rand 0.8.4", + "rand 0.8.5", "scale-info", "sp-npos-elections", "sp-runtime", @@ -10233,7 +10224,7 @@ dependencies = [ "lazy_static", "nalgebra", "num-traits", - "rand 0.8.4", + "rand 0.8.5", ] [[package]] @@ -11004,7 +10995,7 @@ dependencies = [ "ipnet", "lazy_static", "log", - "rand 0.8.4", + "rand 0.8.5", "smallvec", "thiserror", "tinyvec", @@ -11090,7 +11081,7 @@ checksum = "97fee6b57c6a41524a810daee9286c02d7752c4253064d0b05472833a438f675" dependencies = [ "cfg-if 1.0.0", "digest 0.10.3", - "rand 0.8.4", + "rand 0.7.3", "static_assertions", ] @@ -11799,7 +11790,7 @@ dependencies = [ "memfd", "memoffset", "more-asserts", - "rand 0.8.4", + "rand 0.8.5", "region 2.2.0", "rustix 0.33.7", "thiserror", @@ -12091,7 +12082,7 @@ dependencies = [ "log", "nohash-hasher", "parking_lot 0.12.0", - "rand 0.8.4", + "rand 0.8.5", "static_assertions", ] diff --git a/frame/nomination-pools/Cargo.toml b/frame/nomination-pools/Cargo.toml index be5c38d85552c..de12609d43421 100644 --- a/frame/nomination-pools/Cargo.toml +++ b/frame/nomination-pools/Cargo.toml @@ -29,10 +29,12 @@ log = { version = "0.4.0", default-features = false } [dev-dependencies] pallet-balances = { version = "4.0.0-dev", path = "../balances" } sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" } +rand = "0.8.5" [features] runtime-benchmarks = [] try-runtime = [] +fuzz-test = [] default = ["std"] std = [ "codec/std", diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 8171ead53e824..e5bdd44ceb8dd 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1448,7 +1448,7 @@ pub mod pallet { PartialUnbondNotAllowedPermissionlessly, } - #[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError)] + #[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError, RuntimeDebug)] pub enum DefensiveError { /// There isn't enough space in the unbond pool. NotEnoughSpaceInUnbondPool, @@ -1751,8 +1751,8 @@ pub mod pallet { let bonded_pool = BondedPool::::get(member.pool_id) .defensive_ok_or::>(DefensiveError::PoolNotFound.into())?; - let mut sub_pools = SubPoolsStorage::::get(member.pool_id) - .defensive_ok_or::>(DefensiveError::SubPoolsNotFound.into())?; + let mut sub_pools = + SubPoolsStorage::::get(member.pool_id).ok_or(Error::::SubPoolsNotFound)?; bonded_pool.ok_to_withdraw_unbonded_with(&caller, &member_account)?; @@ -2379,7 +2379,7 @@ impl Pallet { /// To cater for tests that want to escape parts of these checks, this function is split into /// multiple `level`s, where the higher the level, the more checks we performs. So, /// `sanity_check(255)` is the strongest sanity check, and `0` performs no checks. - #[cfg(any(feature = "std", test, debug_assertions))] + #[cfg(any(feature = "try-runtime", test, debug_assertions))] pub fn sanity_checks(level: u8) -> Result<(), &'static str> { if level.is_zero() { return Ok(()) @@ -2397,7 +2397,12 @@ impl Pallet { for id in reward_pools { let account = Self::create_reward_account(id); - assert!(T::Currency::free_balance(&account) >= T::Currency::minimum_balance()); + assert!( + T::Currency::free_balance(&account) >= T::Currency::minimum_balance(), + "reward pool of {id}: {:?} (ed = {:?})", + T::Currency::free_balance(&account), + T::Currency::minimum_balance() + ); } let mut pools_members = BTreeMap::::new(); @@ -2421,6 +2426,13 @@ impl Pallet { RewardPools::::iter_keys().for_each(|id| { // the sum of the pending rewards must be less than the leftover balance. Since the // reward math rounds down, we might accumulate some dust here. + log!( + trace, + "pool {:?}, sum pending rewards = {:?}, remaining balance = {:?}", + id, + pools_members_pending_rewards.get(&id), + RewardPool::::current_balance(id) + ); assert!( RewardPool::::current_balance(id) >= pools_members_pending_rewards.get(&id).map(|x| *x).unwrap_or_default() diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index c3e2f28ab3575..1ee8c3dde544b 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -3720,6 +3720,170 @@ mod withdraw_unbonded { }) } + #[test] + fn out_of_sync_unbonding_chunks() { + // the unbonding_eras in pool member are always fixed to the era at which they are unlocked, + // but the actual unbonding pools get pruned and might get combined in the no_era pool. + // Pools are only merged when one unbonds, so we unbond a little bit on every era to + // simulate this. + ExtBuilder::default() + .add_members(vec![(20, 100), (30, 100)]) + .build_and_execute(|| { + System::reset_events(); + + // when + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); + assert_ok!(Pools::unbond(Origin::signed(30), 30, 5)); + + // then member-local unbonding is pretty much in sync with the global pools. + assert_eq!( + PoolMembers::::get(20).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 5) + ); + assert_eq!( + PoolMembers::::get(30).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 5) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 10, balance: 10 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Unbonded { member: 20, pool_id: 1, points: 5, balance: 5, era: 3 }, + Event::Unbonded { member: 30, pool_id: 1, points: 5, balance: 5, era: 3 }, + ] + ); + + // when + CurrentEra::set(1); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); + + // then still member-local unbonding is pretty much in sync with the global pools. + assert_eq!( + PoolMembers::::get(20).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 5, 4 => 5) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 10, balance: 10 }, + 4 => UnbondPool { points: 5, balance: 5 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 20, pool_id: 1, points: 5, balance: 5, era: 4 }] + ); + + // when + CurrentEra::set(2); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); + + // then still member-local unbonding is pretty much in sync with the global pools. + assert_eq!( + PoolMembers::::get(20).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 5, 4 => 5, 5 => 5) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 10, balance: 10 }, + 4 => UnbondPool { points: 5, balance: 5 }, + 5 => UnbondPool { points: 5, balance: 5 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 20, pool_id: 1, points: 5, balance: 5, era: 5 }] + ); + + // when + CurrentEra::set(5); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); + + // then + assert_eq!( + PoolMembers::::get(20).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 5, 4 => 5, 5 => 5, 8 => 5) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + // era 3 is merged into no_era. + no_era: UnbondPool { points: 10, balance: 10 }, + with_era: unbonding_pools_with_era! { + 4 => UnbondPool { points: 5, balance: 5 }, + 5 => UnbondPool { points: 5, balance: 5 }, + 8 => UnbondPool { points: 5, balance: 5 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 20, pool_id: 1, points: 5, balance: 5, era: 8 }] + ); + + // now we start withdrawing unlocked bonds. + + // when + assert_ok!(Pools::withdraw_unbonded(Origin::signed(20), 20, 0)); + // then + assert_eq!( + PoolMembers::::get(20).unwrap().unbonding_eras, + member_unbonding_eras!(8 => 5) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + // era 3 is merged into no_era. + no_era: UnbondPool { points: 5, balance: 5 }, + with_era: unbonding_pools_with_era! { + 8 => UnbondPool { points: 5, balance: 5 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Withdrawn { member: 20, pool_id: 1, points: 15, balance: 15 }] + ); + + // when + assert_ok!(Pools::withdraw_unbonded(Origin::signed(30), 30, 0)); + // then + assert_eq!( + PoolMembers::::get(30).unwrap().unbonding_eras, + member_unbonding_eras!() + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + // era 3 is merged into no_era. + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 8 => UnbondPool { points: 5, balance: 5 } + } + } + ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Withdrawn { member: 30, pool_id: 1, points: 5, balance: 5 }] + ); + }) + } + #[test] fn full_multi_step_withdrawing_depositor() { ExtBuilder::default().ed(1).build_and_execute(|| { @@ -4754,3 +4918,226 @@ mod reward_counter_precision { }); } } + +// NOTE: run this with debug_assertions, but in release mode. +#[cfg(feature = "fuzz-test")] +mod fuzz_test { + use super::*; + use crate::pallet::{Call as PoolsCall, Event as PoolsEvents}; + use frame_support::traits::UnfilteredDispatchable; + use rand::{seq::SliceRandom, thread_rng, Rng}; + + /// Grab random accounts, either known ones, or new ones. + fn random_signed_origin(rng: &mut R) -> (Origin, AccountId) { + let count = PoolMembers::::count(); + if rng.gen::() && count > 0 { + // take an existing account. + let skip = rng.gen_range(0..count as usize); + let acc = PoolMembers::::iter_keys().skip(skip).take(1).next().unwrap(); + (Origin::signed(acc), acc) + } else { + // create a new account + let acc = rng.gen::(); + (Origin::signed(acc), acc) + } + } + + fn random_ed_multiple(rng: &mut R) -> Balance { + const MAX_ED_MULTIPLE: Balance = 10_000; + const MIN_ED_MULTIPLE: Balance = 10; + let multiple = rng.gen_range(MIN_ED_MULTIPLE..MAX_ED_MULTIPLE); + ExistentialDeposit::get() * multiple + } + + fn fund_account(rng: &mut R, account: &AccountId) { + let target_amount = random_ed_multiple(rng); + if let Some(top_up) = target_amount.checked_sub(Balances::free_balance(account)) { + let _ = Balances::deposit_creating(account, top_up); + } + assert!(Balances::free_balance(account) >= target_amount); + } + + fn random_existing_pool(mut rng: &mut R) -> Option { + BondedPools::::iter_keys().collect::>().choose(&mut rng).map(|x| *x) + } + + fn random_call(mut rng: &mut R) -> (crate::pallet::Call, Origin) { + let op = rng.gen::(); + match op % 8 { + 0 => { + // join + let pool_id = random_existing_pool(&mut rng).unwrap_or_default(); + let (origin, who) = random_signed_origin(&mut rng); + fund_account(&mut rng, &who); + let amount = random_ed_multiple(&mut rng); + (PoolsCall::::join { amount, pool_id }, origin) + }, + 1 => { + // bond_extra + let (origin, who) = random_signed_origin(&mut rng); + let extra = if rng.gen::() { + BondExtra::Rewards + } else { + fund_account(&mut rng, &who); + let amount = random_ed_multiple(&mut rng); + BondExtra::FreeBalance(amount) + }; + (PoolsCall::::bond_extra { extra }, origin) + }, + 2 => { + // claim_payout + let (origin, _) = random_signed_origin(&mut rng); + (PoolsCall::::claim_payout {}, origin) + }, + 3 => { + // unbond + let (origin, who) = random_signed_origin(&mut rng); + let amount = random_ed_multiple(&mut rng); + (PoolsCall::::unbond { member_account: who, unbonding_points: amount }, origin) + }, + 4 => { + // pool_withdraw_unbonded + let pool_id = random_existing_pool(&mut rng).unwrap_or_default(); + let (origin, _) = random_signed_origin(&mut rng); + (PoolsCall::::pool_withdraw_unbonded { pool_id, num_slashing_spans: 0 }, origin) + }, + 5 => { + // withdraw_unbonded + let (origin, who) = random_signed_origin(&mut rng); + ( + PoolsCall::::withdraw_unbonded { + member_account: who, + num_slashing_spans: 0, + }, + origin, + ) + }, + 6 => { + // create + let (origin, who) = random_signed_origin(&mut rng); + let amount = random_ed_multiple(&mut rng); + fund_account(&mut rng, &who); + let root = who.clone(); + let state_toggler = who.clone(); + let nominator = who.clone(); + (PoolsCall::::create { amount, root, state_toggler, nominator }, origin) + }, + 7 => { + // nominate + let (origin, _) = random_signed_origin(&mut rng); + let pool_id = random_existing_pool(&mut rng).unwrap_or_default(); + let validators = Default::default(); + (PoolsCall::::nominate { pool_id, validators }, origin) + }, + _ => unreachable!(), + } + } + + #[test] + fn fuzz_test() { + sp_tracing::try_init_simple(); + 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, u32)>::default(); + // TODO: randomly distribute funds reward accounts. + let mut iteration = 0u32; + let mut ok = 0; + let mut err = 0; + + ext.execute_with(|| { + MaxPoolMembers::::set(Some(10_000)); + MaxPoolMembersPerPool::::set(Some(1000)); + MaxPools::::set(Some(1_000)); + + MinCreateBond::::set(10 * ExistentialDeposit::get()); + MinJoinBond::::set(5 * ExistentialDeposit::get()); + System::set_block_number(1); + }); + ExistentialDeposit::set(10u128.pow(12u32)); + BondingDuration::set(8); + + loop { + ext.execute_with(|| { + iteration += 1; + let (call, origin) = random_call(&mut rng); + let outcome = call.clone().dispatch_bypass_filter(origin.clone()); + + match outcome { + Ok(_) => ok += 1, + Err(_) => err += 1, + }; + + log!( + debug, + "iteration {}, call {:?}, origin {:?}, outcome: {:?}, so far {} ok {} err", + iteration, + call, + origin, + outcome, + ok, + err, + ); + + if iteration % + (std::env::var("SANITY").ok().and_then(|x| x.parse::().ok())) + .unwrap_or(1) == 0 + { + log!(info, "running sanity checks at {}", iteration); + Pools::sanity_checks(u8::MAX).unwrap(); + } + + // collect and reset events. + System::events() + .into_iter() + .map(|r| r.event) + .filter_map( + |e| if let mock::Event::Pools(inner) = e { Some(inner) } else { None }, + ) + .for_each(|e| { + if let Some((_, c)) = events_histogram + .iter_mut() + .find(|(x, _)| std::mem::discriminant(x) == std::mem::discriminant(&e)) + { + *c += 1; + } else { + events_histogram.push((e, 1)) + } + }); + System::reset_events(); + + if iteration % 1000 == 0 { + CurrentEra::set(CurrentEra::get() + 1); + BondedPools::::iter().for_each(|(id, _)| { + let _ = Balances::deposit_creating( + &Pools::create_reward_account(id), + random_ed_multiple(&mut rng), + ); + }); + log!( + info, + "iteration {}, {} pools, {} members, {} ok {} err, events = {:?}", + iteration, + BondedPools::::count(), + PoolMembers::::count(), + ok, + err, + events_histogram + .iter() + .map(|(x, c)| ( + format!("{:?}", x) + .split(" ") + .map(|x| x.to_string()) + .collect::>() + .first() + .cloned() + .unwrap(), + c, + )) + .collect::>(), + ); + } + }); + } + } +} diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index ddb7f6f41b378..aa596a23cf408 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -150,10 +150,10 @@ pub trait DefensiveOption { /// Defensively transform this option to a result, mapping `None` to the return value of an /// error closure. - fn defensive_ok_or_else E>(self, err: F) -> Result; + fn defensive_ok_or_else E>(self, err: F) -> Result; /// Defensively transform this option to a result, mapping `None` to a default value. - fn defensive_ok_or(self, err: E) -> Result; + fn defensive_ok_or(self, err: E) -> Result; /// Exactly the same as `map`, but it prints the appropriate warnings if the value being mapped /// is `None`. @@ -317,16 +317,17 @@ impl DefensiveOption for Option { ) } - fn defensive_ok_or_else E>(self, err: F) -> Result { + fn defensive_ok_or_else E>(self, err: F) -> Result { self.ok_or_else(|| { - defensive!(); - err() + let err_value = err(); + defensive!(err_value); + err_value }) } - fn defensive_ok_or(self, err: E) -> Result { + fn defensive_ok_or(self, err: E) -> Result { self.ok_or_else(|| { - defensive!(); + defensive!(err); err }) } From 75b1ebe9556fa73950f4226f01a3d55f7aa4484e Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 9 Aug 2022 20:48:31 +0100 Subject: [PATCH 04/14] breaks every now and then --- frame/nomination-pools/src/mock.rs | 3 +- frame/nomination-pools/src/tests.rs | 102 +++++++++++++++++++++++++--- primitives/runtime/src/lib.rs | 4 +- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 5138c55afccac..d536d3aff360f 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -4,6 +4,7 @@ use frame_support::{assert_ok, parameter_types, PalletId}; use frame_system::RawOrigin; use sp_runtime::FixedU128; +pub type BlockNumber = u64; pub type AccountId = u128; pub type Balance = u128; pub type RewardCounter = FixedU128; @@ -129,7 +130,7 @@ impl frame_system::Config for Runtime { type BaseCallFilter = frame_support::traits::Everything; type Origin = Origin; type Index = u64; - type BlockNumber = u64; + type BlockNumber = BlockNumber; type Call = Call; type Hash = sp_core::H256; type Hashing = sp_runtime::traits::BlakeTwo256; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 1ee8c3dde544b..33a453d3f489c 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4926,6 +4926,11 @@ mod fuzz_test { use crate::pallet::{Call as PoolsCall, Event as PoolsEvents}; use frame_support::traits::UnfilteredDispatchable; use rand::{seq::SliceRandom, thread_rng, Rng}; + use sp_runtime::{assert_eq_error_rate, Perquintill}; + + const ERA: BlockNumber = 1000; + const MAX_ED_MULTIPLE: Balance = 1000; + const MIN_ED_MULTIPLE: Balance = 10; /// Grab random accounts, either known ones, or new ones. fn random_signed_origin(rng: &mut R) -> (Origin, AccountId) { @@ -4943,8 +4948,6 @@ mod fuzz_test { } fn random_ed_multiple(rng: &mut R) -> Balance { - const MAX_ED_MULTIPLE: Balance = 10_000; - const MIN_ED_MULTIPLE: Balance = 10; let multiple = rng.gen_range(MIN_ED_MULTIPLE..MAX_ED_MULTIPLE); ExistentialDeposit::get() * multiple } @@ -5033,15 +5036,63 @@ mod fuzz_test { } } + #[derive(Default)] + struct RewardAgent { + who: AccountId, + pool_id: Option, + expected_reward: Balance, + } + + impl RewardAgent { + fn new(who: AccountId) -> Self { + Self { who, ..Default::default() } + } + + fn join(&mut self) { + let pool_id = LastPoolId::::get(); + let amount = 10 * ExistentialDeposit::get(); + let origin = Origin::signed(self.who); + let _ = Balances::deposit_creating(&self.who, 10 * amount); + self.pool_id = Some(pool_id); + assert_ok!(PoolsCall::join:: { amount, pool_id }.dispatch_bypass_filter(origin)); + } + + fn claim_payout(&mut self) { + // 10 era later, we claim our payout. We expect our income to be roughly what we + // calculated. + if !PoolMembers::::contains_key(&self.who) { + log!(warn, "reward agent is not in the pool yet, cannot claim"); + return + } + let pre = Balances::free_balance(&42); + let origin = Origin::signed(42); + assert_ok!(PoolsCall::::claim_payout {}.dispatch_bypass_filter(origin)); + let post = Balances::free_balance(&42); + + let income = post - pre; + log!( + info, + "CLAIM: actual: {}, expected: {}, {:?} ({:?})", + income, + self.expected_reward, + System::events(), + BondedPool::::get(self.pool_id.unwrap()) + ); + assert_eq_error_rate!(income, self.expected_reward, 100); + self.expected_reward = 0; + } + } + #[test] fn fuzz_test() { + let mut reward_agent = RewardAgent::new(42); + sp_tracing::try_init_simple(); 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, u32)>::default(); - // TODO: randomly distribute funds reward accounts. - let mut iteration = 0u32; + let mut iteration = 0 as BlockNumber; let mut ok = 0; let mut err = 0; @@ -5054,7 +5105,7 @@ mod fuzz_test { MinJoinBond::::set(5 * ExistentialDeposit::get()); System::set_block_number(1); }); - ExistentialDeposit::set(10u128.pow(12u32)); + ExistentialDeposit::set(10u128.pow(1u32)); BondingDuration::set(8); loop { @@ -5079,8 +5130,20 @@ mod fuzz_test { err, ); + // possibly join the reward_agent + if iteration == ERA / 2 { + log!(info, "reward agent joining in"); + reward_agent.join(); + } + // and possibly roughly every era, trigger payout for the agent. Doing this more + // frequent is also harmless. + if rng.gen_range(0..ERA) == 0 { + reward_agent.claim_payout(); + } + + // execute sanity checks at a fixed interval, possibly on every block. if iteration % - (std::env::var("SANITY").ok().and_then(|x| x.parse::().ok())) + (std::env::var("SANITY").ok().and_then(|x| x.parse::().ok())) .unwrap_or(1) == 0 { log!(info, "running sanity checks at {}", iteration); @@ -5106,14 +5169,31 @@ mod fuzz_test { }); System::reset_events(); - if iteration % 1000 == 0 { + if iteration % ERA == 0 { CurrentEra::set(CurrentEra::get() + 1); BondedPools::::iter().for_each(|(id, _)| { - let _ = Balances::deposit_creating( - &Pools::create_reward_account(id), - random_ed_multiple(&mut rng), - ); + let amount = random_ed_multiple(&mut rng); + let _ = + Balances::deposit_creating(&Pools::create_reward_account(id), amount); + // if we just paid out the reward agent, let's calculate how much we expect + // our reward agent to have earned. + if reward_agent.pool_id.map_or(false, |mid| mid == id) { + let all_points = BondedPool::::get(id).map(|p| p.points).unwrap(); + let member_points = + PoolMembers::::get(reward_agent.who).map(|m| m.points).unwrap(); + let agent_share = Perquintill::from_rational(member_points, all_points); + log!( + info, + "REWARD = amount = {:?}, ratio: {:?}, share {:?}, ({:?})", + amount, + agent_share, + agent_share * amount, + BondedPool::::get(id).unwrap() + ); + reward_agent.expected_reward += agent_share * amount; + } }); + log!( info, "iteration {}, {} pools, {} members, {} ok {} err, events = {:?}", diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index bf77c08b76906..ea44f1b22bf7d 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -36,6 +36,8 @@ pub use sp_std; #[doc(hidden)] pub use paste; +#[doc(hidden)] +pub use sp_arithmetic::traits::Saturating; #[doc(hidden)] pub use sp_application_crypto as app_crypto; @@ -825,7 +827,7 @@ pub fn verify_encoded_lazy( 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)), "{:?} != {:?} (with error rate {:?})", $x, $y, From 3404b7637a760a114ebf15def53b48120276c1d7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 9 Aug 2022 20:49:11 +0100 Subject: [PATCH 05/14] breaks every now and then --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 33a453d3f489c..d77b1b9af3e86 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5078,7 +5078,7 @@ mod fuzz_test { System::events(), BondedPool::::get(self.pool_id.unwrap()) ); - assert_eq_error_rate!(income, self.expected_reward, 100); + assert_eq!(income, self.expected_reward); self.expected_reward = 0; } } From 0b5cd9f14d7138ef7462360f0eb95e8798e643fe Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 9 Aug 2022 22:00:02 +0100 Subject: [PATCH 06/14] IT WORKS AND PASSES 100k TESTS --- frame/nomination-pools/Cargo.toml | 2 +- frame/nomination-pools/src/tests.rs | 49 ++++++++++++++++++----------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/frame/nomination-pools/Cargo.toml b/frame/nomination-pools/Cargo.toml index de12609d43421..8b0ea4b7d5758 100644 --- a/frame/nomination-pools/Cargo.toml +++ b/frame/nomination-pools/Cargo.toml @@ -29,7 +29,7 @@ log = { version = "0.4.0", default-features = false } [dev-dependencies] pallet-balances = { version = "4.0.0-dev", path = "../balances" } sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" } -rand = "0.8.5" +rand = { version = "0.8.5", features = ["small_rng"] } [features] runtime-benchmarks = [] diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index d77b1b9af3e86..11e083e03abb4 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4925,20 +4925,30 @@ mod fuzz_test { use super::*; use crate::pallet::{Call as PoolsCall, Event as PoolsEvents}; use frame_support::traits::UnfilteredDispatchable; - use rand::{seq::SliceRandom, thread_rng, Rng}; + use rand::{rngs::SmallRng, seq::SliceRandom, thread_rng, Rng, SeedableRng}; use sp_runtime::{assert_eq_error_rate, Perquintill}; const ERA: BlockNumber = 1000; - const MAX_ED_MULTIPLE: Balance = 1000; + const MAX_ED_MULTIPLE: Balance = 10_000; const MIN_ED_MULTIPLE: Balance = 10; + // not quite elegant, just to make it available in random_signed_origin. + const REWARD_AGENT_ACCOUNT: AccountId = 42; + /// Grab random accounts, either known ones, or new ones. fn random_signed_origin(rng: &mut R) -> (Origin, AccountId) { let count = PoolMembers::::count(); if rng.gen::() && count > 0 { // take an existing account. let skip = rng.gen_range(0..count as usize); - let acc = PoolMembers::::iter_keys().skip(skip).take(1).next().unwrap(); + + // this is tricky: the account might be our reward agent, which we never want to be + // randomly chosen here. Try another one, or, if it is only our agent, return a random + // one nonetheless. + let candidate = PoolMembers::::iter_keys().skip(skip).take(1).next().unwrap(); + let acc = + if candidate == REWARD_AGENT_ACCOUNT { rng.gen::() } else { candidate }; + (Origin::signed(acc), acc) } else { // create a new account @@ -5043,17 +5053,22 @@ mod fuzz_test { expected_reward: Balance, } + // TODO: inject some slashes into the game. impl RewardAgent { fn new(who: AccountId) -> Self { Self { who, ..Default::default() } } fn join(&mut self) { + if self.pool_id.is_some() { + return + } let pool_id = LastPoolId::::get(); let amount = 10 * ExistentialDeposit::get(); let origin = Origin::signed(self.who); let _ = Balances::deposit_creating(&self.who, 10 * amount); self.pool_id = Some(pool_id); + log::info!(target: "reward-agent", "🤖 reward agent joining in {} with {}", pool_id, amount); assert_ok!(PoolsCall::join:: { amount, pool_id }.dispatch_bypass_filter(origin)); } @@ -5070,15 +5085,12 @@ mod fuzz_test { let post = Balances::free_balance(&42); let income = post - pre; - log!( - info, - "CLAIM: actual: {}, expected: {}, {:?} ({:?})", + log::info!( + target: "reward-agent", "🤖 CLAIM: actual: {}, expected: {}", income, self.expected_reward, - System::events(), - BondedPool::::get(self.pool_id.unwrap()) ); - assert_eq!(income, self.expected_reward); + assert_eq_error_rate!(income, self.expected_reward, 10); self.expected_reward = 0; } } @@ -5086,8 +5098,8 @@ mod fuzz_test { #[test] fn fuzz_test() { let mut reward_agent = RewardAgent::new(42); - sp_tracing::try_init_simple(); + // let mut rng = SmallRng::from_seed([0u8; 32]); 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. @@ -5105,7 +5117,8 @@ mod fuzz_test { MinJoinBond::::set(5 * ExistentialDeposit::get()); System::set_block_number(1); }); - ExistentialDeposit::set(10u128.pow(1u32)); + + ExistentialDeposit::set(10u128.pow(12u32)); BondingDuration::set(8); loop { @@ -5131,13 +5144,12 @@ mod fuzz_test { ); // possibly join the reward_agent - if iteration == ERA / 2 { - log!(info, "reward agent joining in"); + if iteration > ERA / 2 && BondedPools::::count() > 0 { reward_agent.join(); } - // and possibly roughly every era, trigger payout for the agent. Doing this more + // and possibly roughly every 4 era, trigger payout for the agent. Doing this more // frequent is also harmless. - if rng.gen_range(0..ERA) == 0 { + if rng.gen_range(0..(4 * ERA)) == 0 { reward_agent.claim_payout(); } @@ -5182,13 +5194,12 @@ mod fuzz_test { let member_points = PoolMembers::::get(reward_agent.who).map(|m| m.points).unwrap(); let agent_share = Perquintill::from_rational(member_points, all_points); - log!( - info, - "REWARD = amount = {:?}, ratio: {:?}, share {:?}, ({:?})", + log::info!( + target: "reward-agent", + "🤖 REWARD = amount = {:?}, ratio: {:?}, share {:?}", amount, agent_share, agent_share * amount, - BondedPool::::get(id).unwrap() ); reward_agent.expected_reward += agent_share * amount; } From 95d4d1de06b8d359d75a78e8a1904ce96dc664d3 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 9 Aug 2022 22:09:54 +0100 Subject: [PATCH 07/14] cleanup --- frame/nomination-pools/src/lib.rs | 2 +- frame/nomination-pools/src/tests.rs | 6 ++++-- frame/support/src/lib.rs | 7 +++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index e5bdd44ceb8dd..8fdcd07c6df13 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1105,7 +1105,7 @@ impl SubPools { } /// The sum of all unbonding balance, regardless of whether they are actually unlocked or not. - #[cfg(any(feature = "std", test, debug_assertions))] + #[cfg(any(feature = "try-runtime", test, debug_assertions))] fn sum_unbonding_balance(&self) -> BalanceOf { self.no_era.balance.saturating_add( self.with_era diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 11e083e03abb4..a6100061b934f 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4925,7 +4925,7 @@ mod fuzz_test { use super::*; use crate::pallet::{Call as PoolsCall, Event as PoolsEvents}; use frame_support::traits::UnfilteredDispatchable; - use rand::{rngs::SmallRng, seq::SliceRandom, thread_rng, Rng, SeedableRng}; + use rand::{seq::SliceRandom, thread_rng, Rng}; use sp_runtime::{assert_eq_error_rate, Perquintill}; const ERA: BlockNumber = 1000; @@ -5099,6 +5099,8 @@ mod fuzz_test { fn fuzz_test() { let mut reward_agent = RewardAgent::new(42); sp_tracing::try_init_simple(); + // 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(); let mut ext = sp_io::TestExternalities::new_empty(); @@ -5182,7 +5184,7 @@ mod fuzz_test { System::reset_events(); if iteration % ERA == 0 { - CurrentEra::set(CurrentEra::get() + 1); + CurrentEra::mutate(|c| *c += 1); BondedPools::::iter().for_each(|(id, _)| { let amount = random_ed_multiple(&mut rng); let _ = diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 8e43df82a284c..407f297b3fe8c 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -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 ()>(mutate: F) { + let mut current = Self::get(); + mutate(&mut current); + Self::set(current); + } } )* } From a3cdac8380aac0add0bee607925527a4e28119ed Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 10 Aug 2022 16:17:21 +0430 Subject: [PATCH 08/14] safe id addition --- frame/nomination-pools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 8fdcd07c6df13..a5edc40b12ef4 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1877,10 +1877,10 @@ pub mod pallet { ); ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); - let pool_id = LastPoolId::::mutate(|id| { - *id += 1; - *id - }); + let pool_id = LastPoolId::::try_mutate::<_, Error, _>(|id| { + *id = id.checked_add(1).ok_or(Error::::OverflowRisk)?; + Ok(*id) + })?; let mut bonded_pool = BondedPool::::new( pool_id, PoolRoles { From f30a9762accf0194dc17bc07ec512475ab8e393a Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 11 Aug 2022 11:06:11 +0430 Subject: [PATCH 09/14] fix assert_eq_error_rate --- primitives/runtime/src/lib.rs | 19 ++++++++++++++++++- .../benchmarking-cli/src/machine/hardware.rs | 8 ++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index ea44f1b22bf7d..bee6869b4a529 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -827,7 +827,24 @@ pub fn verify_encoded_lazy( macro_rules! assert_eq_error_rate { ($x:expr, $y:expr, $error:expr $(,)?) => { assert!( - ($x >= $crate::Saturating::saturating_sub($y, $error)) && ($x <= $crate::Saturating::saturating_add($y, $error)), + ($x >= $crate::Saturating::saturating_sub($y, $error)) && + ($x <= $crate::Saturating::saturating_add($y, $error)), + "{:?} != {:?} (with error rate {:?})", + $x, + $y, + $error, + ); + }; +} + +/// Same as [`assert_eq_error_rate`], but intended to be used with floating point number, or +/// generally those who do not have over/underflow potentials. +#[macro_export] +#[cfg(feature = "std")] +macro_rules! assert_eq_error_rate_float { + ($x:expr, $y:expr, $error:expr $(,)?) => { + assert!( + ($x >= $y - $error) && ($x <= $y + $error), "{:?} != {:?} (with error rate {:?})", $x, $y, diff --git a/utils/frame/benchmarking-cli/src/machine/hardware.rs b/utils/frame/benchmarking-cli/src/machine/hardware.rs index 5c62660cc7cf4..97960de99c4bf 100644 --- a/utils/frame/benchmarking-cli/src/machine/hardware.rs +++ b/utils/frame/benchmarking-cli/src/machine/hardware.rs @@ -161,7 +161,7 @@ impl fmt::Display for Throughput { #[cfg(test)] mod tests { use super::*; - use sp_runtime::assert_eq_error_rate; + use sp_runtime::assert_eq_error_rate_float; /// `SUBSTRATE_REFERENCE_HARDWARE` can be en- and decoded. #[test] @@ -179,9 +179,9 @@ mod tests { const EPS: f64 = 0.1; let gib = Throughput::GiBs(14.324); - assert_eq_error_rate!(14.324, gib.to_gibs(), EPS); - assert_eq_error_rate!(14667.776, gib.to_mibs(), EPS); - assert_eq_error_rate!(14667.776 * 1024.0, gib.to_kibs(), EPS); + assert_eq_error_rate_float!(14.324, gib.to_gibs(), EPS); + assert_eq_error_rate_float!(14667.776, gib.to_mibs(), EPS); + assert_eq_error_rate_float!(14667.776 * 1024.0, gib.to_kibs(), EPS); assert_eq!("14.32 GiB/s", gib.to_string()); assert_eq!("14.32 GiB/s", gib.normalize().to_string()); From 637a4f43bf05e7ea77d3564772bf13cd3e084fb3 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 12 Sep 2022 14:42:01 +0100 Subject: [PATCH 10/14] Update frame/nomination-pools/src/tests.rs Co-authored-by: Oliver Tale-Yazdi --- frame/nomination-pools/src/tests.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 6e63d22d5f77a..62e73f6ac1f73 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5037,8 +5037,14 @@ mod fuzz_test { } fn random_call(mut rng: &mut R) -> (crate::pallet::Call, Origin) { - let op = rng.gen::(); - match op % 8 { + let op = rng.gen::(); + let mut op_count = + as frame_support::dispatch::GetCallName>::get_call_names() + .len(); + // Exclude set_state, set_metadata, set_configs, update_roles and chill. + max_op_index -= 5; + + match op % op_count { 0 => { // join let pool_id = random_existing_pool(&mut rng).unwrap_or_default(); From e49613c4b994a72bae1e55b03f044673934beac1 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 12 Sep 2022 14:42:15 +0100 Subject: [PATCH 11/14] Update frame/nomination-pools/src/tests.rs Co-authored-by: Oliver Tale-Yazdi --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 62e73f6ac1f73..17aa8ccaf3fe0 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5225,7 +5225,7 @@ mod fuzz_test { // execute sanity checks at a fixed interval, possibly on every block. if iteration % - (std::env::var("SANITY").ok().and_then(|x| x.parse::().ok())) + (std::env::var("SANITY_CHECK_INTERVAL").ok().and_then(|x| x.parse::().ok())) .unwrap_or(1) == 0 { log!(info, "running sanity checks at {}", iteration); From bb1a7521dfd7a8f47e62149e946be87b00378ac7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 12 Sep 2022 13:43:23 +0000 Subject: [PATCH 12/14] add some doc --- frame/nomination-pools/src/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 6e63d22d5f77a..b7a4376384c1a 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5245,6 +5245,7 @@ mod fuzz_test { }); System::reset_events(); + // trigger an era change, and check the status of the reward agent. if iteration % ERA == 0 { CurrentEra::mutate(|c| *c += 1); BondedPools::::iter().for_each(|(id, _)| { From c759723883710cda75a2703543c2ace268ecbd9e Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 12 Sep 2022 13:48:24 +0000 Subject: [PATCH 13/14] Fix --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 8f78ff76635c4..87c576bdc608b 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5042,7 +5042,7 @@ mod fuzz_test { as frame_support::dispatch::GetCallName>::get_call_names() .len(); // Exclude set_state, set_metadata, set_configs, update_roles and chill. - max_op_index -= 5; + op_count -= 5; match op % op_count { 0 => { From 94ae1d467ba88e3df82ea5a24f79f741154a68eb Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Mon, 12 Sep 2022 13:58:12 +0000 Subject: [PATCH 14/14] ".git/.scripts/fmt.sh" 1 --- frame/nomination-pools/src/tests.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 87c576bdc608b..362fece3f3daf 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5225,8 +5225,10 @@ mod fuzz_test { // execute sanity checks at a fixed interval, possibly on every block. if iteration % - (std::env::var("SANITY_CHECK_INTERVAL").ok().and_then(|x| x.parse::().ok())) - .unwrap_or(1) == 0 + (std::env::var("SANITY_CHECK_INTERVAL") + .ok() + .and_then(|x| x.parse::().ok())) + .unwrap_or(1) == 0 { log!(info, "running sanity checks at {}", iteration); Pools::do_try_state(u8::MAX).unwrap();