From dd6fdf0f1444c047b5bdd9d7e54330bfb5cfd660 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 17 Mar 2022 11:56:50 -0400 Subject: [PATCH] Fix Benchmark Regressions in Polkadot Parachain Auction System (#5139) * integration tests use offset * fix slots * fix auctions * add auctions benchmark * fix crowdloan --- polkadot/runtime/common/src/auctions.rs | 16 +++++++-- polkadot/runtime/common/src/crowdloan/mod.rs | 4 +-- .../runtime/common/src/integration_tests.rs | 36 +++++++++++++------ polkadot/runtime/common/src/slots.rs | 10 +++++- polkadot/runtime/polkadot/src/lib.rs | 1 + 5 files changed, 52 insertions(+), 15 deletions(-) diff --git a/polkadot/runtime/common/src/auctions.rs b/polkadot/runtime/common/src/auctions.rs index d20adf7386..59be761bfa 100644 --- a/polkadot/runtime/common/src/auctions.rs +++ b/polkadot/runtime/common/src/auctions.rs @@ -1772,6 +1772,10 @@ mod benchmarking { // Worst case scenario a new bid comes in which kicks out an existing bid for the same slot. bid { + // If there is an offset, we need to be on that block to be able to do lease things. + let (_, offset) = T::Leaser::lease_period_length(); + frame_system::Pallet::::set_block_number(offset + One::one()); + // Create a new auction let duration = T::BlockNumber::max_value(); let lease_period_index = LeasePeriodOf::::zero(); @@ -1818,8 +1822,12 @@ mod benchmarking { // Worst case: 10 bidders taking all wining spots, and we need to calculate the winner for auction end. // Entire winner map should be full and removed at the end of the benchmark. on_initialize { + // If there is an offset, we need to be on that block to be able to do lease things. + let (lease_length, offset) = T::Leaser::lease_period_length(); + frame_system::Pallet::::set_block_number(offset + One::one()); + // Create a new auction - let duration: T::BlockNumber = 99u32.into(); + let duration: T::BlockNumber = lease_length / 2u32.into(); let lease_period_index = LeasePeriodOf::::zero(); let now = frame_system::Pallet::::block_number(); Auctions::::new_auction(RawOrigin::Root.into(), duration, lease_period_index)?; @@ -1857,8 +1865,12 @@ mod benchmarking { // Worst case: 10 bidders taking all wining spots, and winning data is full. cancel_auction { + // If there is an offset, we need to be on that block to be able to do lease things. + let (lease_length, offset) = T::Leaser::lease_period_length(); + frame_system::Pallet::::set_block_number(offset + One::one()); + // Create a new auction - let duration: T::BlockNumber = 99u32.into(); + let duration: T::BlockNumber = lease_length / 2u32.into(); let lease_period_index = LeasePeriodOf::::zero(); let now = frame_system::Pallet::::block_number(); Auctions::::new_auction(RawOrigin::Root.into(), duration, lease_period_index)?; diff --git a/polkadot/runtime/common/src/crowdloan/mod.rs b/polkadot/runtime/common/src/crowdloan/mod.rs index 3175183184..d2eab09ba3 100644 --- a/polkadot/runtime/common/src/crowdloan/mod.rs +++ b/polkadot/runtime/common/src/crowdloan/mod.rs @@ -2014,7 +2014,7 @@ mod benchmarking { let caller: T::AccountId = whitelisted_caller(); let contributor = account("contributor", 0, 0); contribute_fund::(&contributor, fund_index); - frame_system::Pallet::::set_block_number(200u32.into()); + frame_system::Pallet::::set_block_number(T::BlockNumber::max_value()); }: _(RawOrigin::Signed(caller), contributor.clone(), fund_index) verify { assert_last_event::(Event::::Withdrew(contributor, fund_index, T::MinContribution::get()).into()); @@ -2034,7 +2034,7 @@ mod benchmarking { } let caller: T::AccountId = whitelisted_caller(); - frame_system::Pallet::::set_block_number(200u32.into()); + frame_system::Pallet::::set_block_number(T::BlockNumber::max_value()); }: _(RawOrigin::Signed(caller), fund_index) verify { assert_last_event::(Event::::AllRefunded(fund_index).into()); diff --git a/polkadot/runtime/common/src/integration_tests.rs b/polkadot/runtime/common/src/integration_tests.rs index 63f654bb01..abfba2bcd9 100644 --- a/polkadot/runtime/common/src/integration_tests.rs +++ b/polkadot/runtime/common/src/integration_tests.rs @@ -239,7 +239,7 @@ impl auctions::Config for Test { parameter_types! { pub const LeasePeriod: BlockNumber = 100; - pub static LeaseOffset: BlockNumber = 0; + pub static LeaseOffset: BlockNumber = 5; } impl slots::Config for Test { @@ -321,6 +321,11 @@ fn para_origin(id: u32) -> ParaOrigin { ParaOrigin::Parachain(id.into()) } +fn add_blocks(n: u32) { + let block_number = System::block_number(); + run_to_block(block_number + n); +} + fn run_to_block(n: u32) { assert!(System::block_number() < n); while System::block_number() < n { @@ -1307,19 +1312,25 @@ fn gap_bids_work() { )); // Finish the auction - run_to_block(110); + run_to_block(110 + LeaseOffset::get()); // Should have won the lease periods assert_eq!( slots::Leases::::get(ParaId::from(2000)), - // -- 1 --- 2 --- 3 ---------- 4 -------------- 5 -------------- 6 -------------- 7 ------- vec![ + // LP 1 None, + // LP 2 None, + // LP 3 None, + // LP 4 Some((account_id(10), 100)), + // LP 5 Some((account_id(20), 800)), + // LP 6 Some((account_id(20), 200)), + // LP 7 Some((account_id(10), 400)) ], ); @@ -1330,14 +1341,17 @@ fn gap_bids_work() { // Progress through the leases and note the correct amount of balance is reserved. - run_to_block(400); + add_blocks(300 + LeaseOffset::get()); assert_eq!( slots::Leases::::get(ParaId::from(2000)), - // --------- 4 -------------- 5 -------------- 6 -------------- 7 ------- vec![ + // LP 4 Some((account_id(10), 100)), + // LP 5 Some((account_id(20), 800)), + // LP 6 Some((account_id(20), 200)), + // LP 7 Some((account_id(10), 400)) ], ); @@ -1346,13 +1360,15 @@ fn gap_bids_work() { assert_eq!(Balances::reserved_balance(&account_id(20)), 800); // Lease period 4 is done, but nothing is unreserved since user 1 has a debt on lease 7 - run_to_block(500); + add_blocks(100); assert_eq!( slots::Leases::::get(ParaId::from(2000)), - // --------- 5 -------------- 6 -------------- 7 ------- vec![ + // LP 5 Some((account_id(20), 800)), + // LP 6 Some((account_id(20), 200)), + // LP 7 Some((account_id(10), 400)) ], ); @@ -1361,7 +1377,7 @@ fn gap_bids_work() { assert_eq!(Balances::reserved_balance(&account_id(20)), 800); // Lease period 5 is done, and 20 will unreserve down to 200. - run_to_block(600); + add_blocks(100); assert_eq!( slots::Leases::::get(ParaId::from(2000)), // --------- 6 -------------- 7 ------- @@ -1371,7 +1387,7 @@ fn gap_bids_work() { assert_eq!(Balances::reserved_balance(&account_id(20)), 200); // Lease period 6 is done, and 20 will unreserve everything. - run_to_block(700); + add_blocks(100); assert_eq!( slots::Leases::::get(ParaId::from(2000)), // --------- 7 ------- @@ -1381,7 +1397,7 @@ fn gap_bids_work() { assert_eq!(Balances::reserved_balance(&account_id(20)), 0); // All leases are done. Everything is unreserved. - run_to_block(800); + add_blocks(100); assert_eq!(slots::Leases::::get(ParaId::from(2000)), vec![]); assert_eq!(Balances::reserved_balance(&account_id(10)), 0); assert_eq!(Balances::reserved_balance(&account_id(20)), 0); diff --git a/polkadot/runtime/common/src/slots.rs b/polkadot/runtime/common/src/slots.rs index 6625fc459f..0b7c71c87f 100644 --- a/polkadot/runtime/common/src/slots.rs +++ b/polkadot/runtime/common/src/slots.rs @@ -981,7 +981,7 @@ mod benchmarking { use super::*; use frame_support::assert_ok; use frame_system::RawOrigin; - use sp_runtime::traits::Bounded; + use sp_runtime::traits::{Bounded, One}; use frame_benchmarking::{account, benchmarks, whitelisted_caller}; @@ -1015,6 +1015,8 @@ mod benchmarking { benchmarks! { force_lease { + // If there is an offset, we need to be on that block to be able to do lease things. + frame_system::Pallet::::set_block_number(T::LeaseOffset::get() + One::one()); let para = ParaId::from(1337); let leaser: T::AccountId = account("leaser", 0, 0); T::Currency::make_free_balance_be(&leaser, BalanceOf::::max_value()); @@ -1035,6 +1037,9 @@ mod benchmarking { let period_begin = 1u32.into(); let period_count = 4u32.into(); + // If there is an offset, we need to be on that block to be able to do lease things. + frame_system::Pallet::::set_block_number(T::LeaseOffset::get() + One::one()); + // Make T parathreads let paras_info = (0..t).map(|i| { register_a_parathread::(i) @@ -1085,6 +1090,9 @@ mod benchmarking { let max_people = 8; let (para, _) = register_a_parathread::(1); + // If there is an offset, we need to be on that block to be able to do lease things. + frame_system::Pallet::::set_block_number(T::LeaseOffset::get() + One::one()); + for i in 0 .. max_people { let leaser = account("lease_deposit", i, 0); let amount = T::Currency::minimum_balance(); diff --git a/polkadot/runtime/polkadot/src/lib.rs b/polkadot/runtime/polkadot/src/lib.rs index 566fc1e7f6..33c2b6bf8e 100644 --- a/polkadot/runtime/polkadot/src/lib.rs +++ b/polkadot/runtime/polkadot/src/lib.rs @@ -1696,6 +1696,7 @@ mod benches { // Polkadot // NOTE: Make sure to prefix these with `runtime_common::` so // the that path resolves correctly in the generated file. + [runtime_common::auctions, Auctions] [runtime_common::claims, Claims] [runtime_common::crowdloan, Crowdloan] [runtime_common::slots, Slots]