From 1540e4c82b3247d64a2689b793ae55a51a825c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 16 Jan 2023 13:39:13 +0100 Subject: [PATCH] pallet-offences-benchmarking: Box events in verify (#13151) * pallet-offences-benchmarking: Box events in verify Events in frame are represented by an enum in the pallet and the runtime. The size of an enum in Rust depends on the size of biggest variant. This means we always need to allocate memory for the biggest variant when allocating memory for an event. The offences benchmarking is verifying the benchmarking results by checking the events. To check the events it is generating all the expected events. With the recent changes in Polkadot the events are too big and lead to issues when running this verify functions. The solution is to box each event, as the vector holding all the events will then only need to hold fat pointers * expected events, instead of size_of(event) * expected events. This issue isn't a problem in production, as we never read the events on chain. When we are reading the events, it is done in an offchain context and they are only decoded one by one. Besides that this also enables the benchmarking verification for everyone running these benchmarks. * FMT * Disable checking again --- Cargo.lock | 1 + frame/offences/benchmarking/Cargo.toml | 2 + frame/offences/benchmarking/src/lib.rs | 189 +++++++++++++------------ 3 files changed, 104 insertions(+), 88 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 68c144b3f177e..e4e3e1b84625c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6079,6 +6079,7 @@ dependencies = [ "frame-election-provider-support", "frame-support", "frame-system", + "log", "pallet-babe", "pallet-balances", "pallet-grandpa", diff --git a/frame/offences/benchmarking/Cargo.toml b/frame/offences/benchmarking/Cargo.toml index e20aefd69ad4d..e31e8bbed929f 100644 --- a/frame/offences/benchmarking/Cargo.toml +++ b/frame/offences/benchmarking/Cargo.toml @@ -29,6 +29,7 @@ pallet-staking = { version = "4.0.0-dev", default-features = false, path = "../. sp-runtime = { version = "7.0.0", default-features = false, path = "../../../primitives/runtime" } sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../../primitives/staking" } sp-std = { version = "5.0.0", default-features = false, path = "../../../primitives/std" } +log = { version = "0.4.17", default-features = false } [dev-dependencies] pallet-staking-reward-curve = { version = "4.0.0-dev", path = "../../staking/reward-curve" } @@ -55,6 +56,7 @@ std = [ "sp-runtime/std", "sp-staking/std", "sp-std/std", + "log/std", ] runtime-benchmarks = [ diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index e5ec2952f8114..7e586a96bbe3e 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -28,8 +28,10 @@ use frame_benchmarking::{account, benchmarks}; use frame_support::traits::{Currency, Get, ValidatorSet, ValidatorSetWithIdentification}; use frame_system::{Config as SystemConfig, Pallet as System, RawOrigin}; +#[cfg(test)] +use sp_runtime::traits::UniqueSaturatedInto; use sp_runtime::{ - traits::{Convert, Saturating, StaticLookup, UniqueSaturatedInto}, + traits::{Convert, Saturating, StaticLookup}, Perbill, }; use sp_staking::offence::{Offence, ReportOffence}; @@ -43,9 +45,11 @@ use pallet_session::{ historical::{Config as HistoricalConfig, IdentificationTuple}, Config as SessionConfig, SessionManager, }; +#[cfg(test)] +use pallet_staking::Event as StakingEvent; use pallet_staking::{ - Config as StakingConfig, Event as StakingEvent, Exposure, IndividualExposure, - Pallet as Staking, RewardDestination, ValidatorPrefs, + Config as StakingConfig, Exposure, IndividualExposure, Pallet as Staking, RewardDestination, + ValidatorPrefs, }; const SEED: u32 = 0; @@ -89,7 +93,9 @@ type BalanceOf = struct Offender { pub controller: T::AccountId, + #[allow(dead_code)] pub stash: T::AccountId, + #[allow(dead_code)] pub nominator_stashes: Vec, } @@ -217,55 +223,63 @@ fn make_offenders_im_online( } #[cfg(test)] -fn check_events::RuntimeEvent>>(expected: I) { +fn check_events< + T: Config, + I: Iterator, + Item: sp_std::borrow::Borrow<::RuntimeEvent> + sp_std::fmt::Debug, +>( + expected: I, +) { let events = System::::events() .into_iter() .map(|frame_system::EventRecord { event, .. }| event) .collect::>(); let expected = expected.collect::>(); - fn pretty(header: &str, ev: &[D], offset: usize) { - println!("{}", header); + fn pretty(header: &str, ev: &[D], offset: usize) { + log::info!("{}", header); for (idx, ev) in ev.iter().enumerate() { - println!("\t[{:04}] {:?}", idx + offset, ev); + log::info!("\t[{:04}] {:?}", idx + offset, ev); } } - fn print_events(idx: usize, events: &[D], expected: &[D]) { + fn print_events( + idx: usize, + events: &[D], + expected: &[E], + ) { let window = 10; let start = idx.saturating_sub(window / 2); let end_got = (idx + window / 2).min(events.len()); pretty("Got(window):", &events[start..end_got], start); let end_expected = (idx + window / 2).min(expected.len()); pretty("Expected(window):", &expected[start..end_expected], start); - println!("---------------"); + log::info!("---------------"); let start_got = events.len().saturating_sub(window); pretty("Got(end):", &events[start_got..], start_got); let start_expected = expected.len().saturating_sub(window); pretty("Expected(end):", &expected[start_expected..], start_expected); } - let events_copy = events.clone(); - let expected_copy = expected.clone(); - - for (idx, (a, b)) in events.into_iter().zip(expected).enumerate() { - if a != b { - print_events(idx, &events_copy, &expected_copy); - println!("Mismatch at: {}", idx); - println!(" Got: {:?}", b); - println!("Expected: {:?}", a); - if events_copy.len() != expected_copy.len() { - println!( + + for (idx, (a, b)) in events.iter().zip(expected.iter()).enumerate() { + if a != sp_std::borrow::Borrow::borrow(b) { + print_events(idx, &events, &expected); + log::info!("Mismatch at: {}", idx); + log::info!(" Got: {:?}", b); + log::info!("Expected: {:?}", a); + if events.len() != expected.len() { + log::info!( "Mismatching lengths. Got: {}, Expected: {}", - events_copy.len(), - expected_copy.len() + events.len(), + expected.len() ) } panic!("Mismatching events."); } } - if events_copy.len() != expected_copy.len() { - print_events(0, &events_copy, &expected_copy); - panic!("Mismatching lengths. Got: {}, Expected: {}", events_copy.len(), expected_copy.len()) + if events.len() != expected.len() { + print_events(0, &events, &expected); + panic!("Mismatching lengths. Got: {}, Expected: {}", events.len(), expected.len(),) } } @@ -304,79 +318,78 @@ benchmarks! { ); } verify { - let bond_amount: u32 = UniqueSaturatedInto::::unique_saturated_into(bond_amount::()); - let slash_amount = slash_fraction * bond_amount; - let reward_amount = slash_amount.saturating_mul(1 + n) / 2; - let reward = reward_amount / r; - let slash_report = |id| core::iter::once( - ::RuntimeEvent::from(StakingEvent::::SlashReported{ validator: id, fraction: slash_fraction, slash_era: 0}) - ); - let slash = |id| core::iter::once( - ::RuntimeEvent::from(StakingEvent::::Slashed{ staker: id, amount: BalanceOf::::from(slash_amount) }) - ); - let balance_slash = |id| core::iter::once( - ::RuntimeEvent::from(pallet_balances::Event::::Slashed{ who: id, amount: slash_amount.into() }) - ); - let chill = |id| core::iter::once( - ::RuntimeEvent::from(StakingEvent::::Chilled{ stash: id }) - ); - let balance_deposit = |id, amount: u32| + #[cfg(test)] + { + let bond_amount: u32 = UniqueSaturatedInto::::unique_saturated_into(bond_amount::()); + let slash_amount = slash_fraction * bond_amount; + let reward_amount = slash_amount.saturating_mul(1 + n) / 2; + let reward = reward_amount / r; + let slash_report = |id| core::iter::once( + ::RuntimeEvent::from(StakingEvent::::SlashReported{ validator: id, fraction: slash_fraction, slash_era: 0}) + ); + let slash = |id| core::iter::once( + ::RuntimeEvent::from(StakingEvent::::Slashed{ staker: id, amount: BalanceOf::::from(slash_amount) }) + ); + let balance_slash = |id| core::iter::once( + ::RuntimeEvent::from(pallet_balances::Event::::Slashed{ who: id, amount: slash_amount.into() }) + ); + let chill = |id| core::iter::once( + ::RuntimeEvent::from(StakingEvent::::Chilled{ stash: id }) + ); + let balance_deposit = |id, amount: u32| ::RuntimeEvent::from(pallet_balances::Event::::Deposit{ who: id, amount: amount.into() }); - let mut first = true; - let slash_events = raw_offenders.into_iter() - .flat_map(|offender| { - let nom_slashes = offender.nominator_stashes.into_iter().flat_map(|nom| { - balance_slash(nom.clone()).map(Into::into) - .chain(slash(nom).map(Into::into)) - }); - - let mut events = chill(offender.stash.clone()).map(Into::into) - .chain(slash_report(offender.stash.clone()).map(Into::into)) - .chain(balance_slash(offender.stash.clone()).map(Into::into)) - .chain(slash(offender.stash).map(Into::into)) - .chain(nom_slashes) - .collect::>(); - - // the first deposit creates endowed events, see `endowed_reward_events` - if first { - first = false; - let mut reward_events = reporters.clone().into_iter() - .flat_map(|reporter| vec![ - balance_deposit(reporter.clone(), reward).into(), - frame_system::Event::::NewAccount { account: reporter.clone() }.into(), - ::RuntimeEvent::from( - pallet_balances::Event::::Endowed{account: reporter, free_balance: reward.into()} - ).into(), - ]) + let mut first = true; + + // We need to box all events to prevent running into too big allocations in wasm. + // The event in FRAME is represented as an enum and the size of the enum depends on the biggest variant. + // So, instead of requiring `size_of() * expected_events` we only need to + // allocate `size_of>() * expected_events`. + let slash_events = raw_offenders.into_iter() + .flat_map(|offender| { + let nom_slashes = offender.nominator_stashes.into_iter().flat_map(|nom| { + balance_slash(nom.clone()).map(Into::into).chain(slash(nom).map(Into::into)).map(Box::new) + }); + + let events = chill(offender.stash.clone()).map(Into::into).map(Box::new) + .chain(slash_report(offender.stash.clone()).map(Into::into).map(Box::new)) + .chain(balance_slash(offender.stash.clone()).map(Into::into).map(Box::new)) + .chain(slash(offender.stash).map(Into::into).map(Box::new)) + .chain(nom_slashes) .collect::>(); - events.append(&mut reward_events); - events.into_iter() - } else { - let mut reward_events = reporters.clone().into_iter() - .map(|reporter| balance_deposit(reporter, reward).into()) - .collect::>(); - events.append(&mut reward_events); - events.into_iter() - } - }) - .collect::>(); - + // the first deposit creates endowed events, see `endowed_reward_events` + if first { + first = false; + let reward_events = reporters.iter() + .flat_map(|reporter| vec![ + Box::new(balance_deposit(reporter.clone(), reward).into()), + Box::new(frame_system::Event::::NewAccount { account: reporter.clone() }.into()), + Box::new(::RuntimeEvent::from( + pallet_balances::Event::::Endowed{ account: reporter.clone(), free_balance: reward.into() } + ).into()), + ]) + .collect::>(); + events.into_iter().chain(reward_events) + } else { + let reward_events = reporters.iter() + .map(|reporter| Box::new(balance_deposit(reporter.clone(), reward).into())) + .collect::>(); + events.into_iter().chain(reward_events) + } + }); - #[cfg(test)] - { // In case of error it's useful to see the inputs - println!("Inputs: r: {}, o: {}, n: {}", r, o, n); + log::info!("Inputs: r: {}, o: {}, n: {}", r, o, n); // make sure that all slashes have been applied - check_events::( - std::iter::empty() - .chain(slash_events.into_iter().map(Into::into)) - .chain(std::iter::once(::RuntimeEvent::from( + check_events::( + sp_std::iter::empty() + .chain(slash_events) + .chain(sp_std::iter::once(Box::new(::RuntimeEvent::from( pallet_offences::Event::Offence{ kind: UnresponsivenessOffence::::ID, timeslot: 0_u32.to_le_bytes().to_vec(), } - ).into())) + ).into()))) ); } }