Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pallet-broker] Fix auto renew benchmarks #6505

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[pallet-broker] Fix auto renew benchmarks #6505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e543388
Fix auto renew benchmarks
seadanda Nov 15, 2024
ca6637b
Update from seadanda running command 'prdoc --audience runtime_dev --…
actions-user Nov 15, 2024
3239d4b
Remove extra sale period
seadanda Nov 15, 2024
09347a0
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
Nov 19, 2024
bb8c4d6
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions prdoc/pr_6505.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
title: '[pallet-broker] Fix auto renew benchmarks'
doc:
- audience: Runtime Dev
description: |-
Fix the broker pallet auto-renew benchmarks which have been broken since #4424, yielding `Weightless` due to some prices being set too low, as reported in #6474.

Upon further investigation it turned out that the auto-renew contribution to `rotate_sale` was always failing but the error was mapped. This is also fixed at the cost of a bit of setup overhead.
crates:
- name: pallet-broker
bump: minor
164 changes: 91 additions & 73 deletions substrate/frame/broker/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ use frame_support::{
},
};
use frame_system::{Pallet as System, RawOrigin};
use sp_arithmetic::{traits::Zero, Perbill};
use sp_arithmetic::Perbill;
use sp_core::Get;
use sp_runtime::{
traits::{BlockNumberProvider, MaybeConvert},
SaturatedConversion, Saturating,
Saturating,
};

const SEED: u32 = 0;
Expand Down Expand Up @@ -287,7 +287,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

Broker::<T>::do_assign(region, None, 1001, Final)
.map_err(|_| BenchmarkError::Weightless)?;
Expand Down Expand Up @@ -316,7 +316,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

let recipient: T::AccountId = account("recipient", 0, SEED);

Expand Down Expand Up @@ -349,7 +349,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

#[extrinsic_call]
_(RawOrigin::Signed(caller), region, 2);
Expand Down Expand Up @@ -381,7 +381,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

#[extrinsic_call]
_(RawOrigin::Signed(caller), region, 0x00000_fffff_fffff_00000.into());
Expand Down Expand Up @@ -417,7 +417,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

#[extrinsic_call]
_(RawOrigin::Signed(caller), region, 1000, Provisional);
Expand Down Expand Up @@ -452,7 +452,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

let recipient: T::AccountId = account("recipient", 0, SEED);

Expand Down Expand Up @@ -492,7 +492,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

let recipient: T::AccountId = account("recipient", 0, SEED);
T::Currency::set_balance(&recipient.clone(), T::Currency::minimum_balance());
Expand Down Expand Up @@ -548,7 +548,7 @@ mod benches {
T::Currency::set_balance(&Broker::<T>::account_id(), T::Currency::minimum_balance());

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

let recipient: T::AccountId = account("recipient", 0, SEED);

Expand Down Expand Up @@ -582,7 +582,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

advance_to::<T>(
(T::TimeslicePeriod::get() * (region_len * 4).into()).try_into().ok().unwrap(),
Expand Down Expand Up @@ -616,7 +616,7 @@ mod benches {
);

let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
.expect("Offer not high enough for configuration.");

let recipient: T::AccountId = account("recipient", 0, SEED);

Expand Down Expand Up @@ -786,99 +786,115 @@ mod benches {

#[benchmark]
fn rotate_sale(n: Linear<0, { MAX_CORE_COUNT.into() }>) -> Result<(), BenchmarkError> {
let core_count = n.try_into().unwrap();
let config = new_config_record::<T>();
Configuration::<T>::put(config.clone());

let now = RCBlockNumberProviderOf::<T::Coretime>::current_block_number();
let end_price = 10_000_000u32.into();
let commit_timeslice = Broker::<T>::latest_timeslice_ready_to_commit(&config);
let sale = SaleInfoRecordOf::<T> {
sale_start: now,
leadin_length: Zero::zero(),
end_price,
sellout_price: None,
region_begin: commit_timeslice,
region_end: commit_timeslice.saturating_add(config.region_length),
first_core: 0,
ideal_cores_sold: 0,
cores_offered: 0,
cores_sold: 0,
};
// Ensure there is one buyable core then use the rest to max out reservations and leases, if
// possible for worst case.

// First allocate up to MaxReservedCores for reservations
let n_reservations = T::MaxReservedCores::get().min(n.saturating_sub(1));
setup_reservations::<T>(n_reservations);
// Then allocate remaining cores to leases, up to MaxLeasedCores
let n_leases =
T::MaxLeasedCores::get().min(n.saturating_sub(1).saturating_sub(n_reservations));
setup_leases::<T>(n_leases, 1, 20);

// Start sales so we can test the auto-renewals.
Broker::<T>::do_start_sales(
10_000_000u32.into(),
n.saturating_sub(n_reservations)
.saturating_sub(n_leases)
.try_into()
.expect("Upper limit of n is a u16."),
)
.map_err(|_| BenchmarkError::Weightless)?;

let status = StatusRecord {
core_count,
private_pool_size: 0,
system_pool_size: 0,
last_committed_timeslice: commit_timeslice.saturating_sub(1),
last_timeslice: Broker::<T>::current_timeslice(),
};
// Advance to the fixed price period.
advance_to::<T>(2);

// Assume Reservations to be filled for worst case
setup_reservations::<T>(T::MaxReservedCores::get());
// Assume max auto renewals for worst case. This is between 1 and the value of
// MaxAutoRenewals.
let n_renewable = T::MaxAutoRenewals::get()
.min(n.saturating_sub(n_leases).saturating_sub(n_reservations));

// Assume Leases to be filled for worst case
setup_leases::<T>(T::MaxLeasedCores::get(), 1, 10);
let timeslice_period: u32 = T::TimeslicePeriod::get().try_into().ok().unwrap();
let sale = SaleInfo::<T>::get().expect("Sale has started.");

// Assume max auto renewals for worst case.
(0..T::MaxAutoRenewals::get()).try_for_each(|indx| -> Result<(), BenchmarkError> {
(0..n_renewable.into()).try_for_each(|indx| -> Result<(), BenchmarkError> {
let task = 1000 + indx;
let caller: T::AccountId = T::SovereignAccountOf::maybe_convert(task)
.expect("Failed to get sovereign account");
T::Currency::set_balance(
&caller.clone(),
T::Currency::minimum_balance().saturating_add(100u32.into()),
T::Currency::minimum_balance().saturating_add(100_000_000u32.into()),
);

let region = Broker::<T>::do_purchase(caller.clone(), 10u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.expect("Offer not high enough for configuration.");

Broker::<T>::do_assign(region, None, task, Final)
.map_err(|_| BenchmarkError::Weightless)?;
Broker::<T>::do_assign(region, None, task, Final).expect("assign in loop");

Broker::<T>::do_enable_auto_renew(caller, region.core, task, None)?;
Broker::<T>::do_enable_auto_renew(caller, region.core, task, Some(sale.region_end))?;

Ok(())
})?;

// Advance to the block before `rotate_sale`.
advance_to::<T>(timeslice_period.saturating_mul(config.region_length) - 1);

// Advance one block and manually tick so we can isolate the `rotate_sale` call.
System::<T>::set_block_number(timeslice_period.saturating_mul(config.region_length).into());
RCBlockNumberProviderOf::<T::Coretime>::set_block_number(
timeslice_period.saturating_mul(config.region_length).into(),
);
let mut status = Status::<T>::get().expect("Sale has started.");
let sale = SaleInfo::<T>::get().expect("Sale has started.");
Broker::<T>::process_core_count(&mut status);
Broker::<T>::process_revenue();
status.last_committed_timeslice = config.region_length;

#[block]
{
Broker::<T>::rotate_sale(sale.clone(), &config, &status);
}

assert!(SaleInfo::<T>::get().is_some());
let sale_start = RCBlockNumberProviderOf::<T::Coretime>::current_block_number() +
config.interlude_length;
// Get prices from the actual price adapter.
let new_prices = T::PriceAdapter::adapt_price(SalePerformance::from_sale(&sale));
let new_sale = SaleInfo::<T>::get().expect("Sale has started.");
let now = RCBlockNumberProviderOf::<T::Coretime>::current_block_number();

assert_last_event::<T>(
Event::SaleInitialized {
sale_start,
sale_start: new_sale.region_begin.into(),
leadin_length: 1u32.into(),
start_price: 1_000_000_000u32.into(),
end_price: 10_000_000u32.into(),
start_price: Broker::<T>::sale_price(&new_sale, now),
end_price: new_prices.end_price,
region_begin: sale.region_begin + config.region_length,
region_end: sale.region_end + config.region_length,
ideal_cores_sold: 0,
cores_offered: n
.saturating_sub(T::MaxReservedCores::get())
.saturating_sub(T::MaxLeasedCores::get())
.saturating_sub(n_reservations)
.saturating_sub(n_leases)
.try_into()
.unwrap(),
}
.into(),
);

// Make sure all cores got renewed:
(0..T::MaxAutoRenewals::get()).for_each(|indx| {
(0..n_renewable).for_each(|indx| {
let task = 1000 + indx;
let who = T::SovereignAccountOf::maybe_convert(task)
.expect("Failed to get sovereign account");
assert_has_event::<T>(
Event::Renewed {
who,
old_core: 10 + indx as u16, // first ten cores are allocated to leases.
core: 10 + indx as u16,
price: 10u32.saturated_into(),
begin: 7,
duration: 3,
old_core: n_reservations as u16 + n_leases as u16 + indx as u16,
core: n_reservations as u16 + n_leases as u16 + indx as u16,
price: 10_000_000u32.into(),
begin: new_sale.region_begin,
duration: config.region_length,
workload: Schedule::truncate_from(vec![ScheduleItem {
assignment: Task(task),
mask: CoreMask::complete(),
Expand Down Expand Up @@ -1027,13 +1043,14 @@ mod benches {
let task = 1000 + indx;
let caller: T::AccountId = T::SovereignAccountOf::maybe_convert(task)
.expect("Failed to get sovereign account");
// Sovereign account needs sufficient funds to purchase and renew.
T::Currency::set_balance(
&caller.clone(),
T::Currency::minimum_balance().saturating_add(100u32.into()),
T::Currency::minimum_balance().saturating_add(100_000_000u32.into()),
);

let region = Broker::<T>::do_purchase(caller.clone(), 10u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.expect("Offer not high enough for configuration.1");

Broker::<T>::do_assign(region, None, task, Final)
.map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -1045,14 +1062,15 @@ mod benches {

let caller: T::AccountId =
T::SovereignAccountOf::maybe_convert(2001).expect("Failed to get sovereign account");
// Sovereign account needs sufficient funds to purchase and renew.
T::Currency::set_balance(
&caller.clone(),
T::Currency::minimum_balance().saturating_add(100u32.into()),
T::Currency::minimum_balance().saturating_add(100_000_000u32.into()),
);

// The region for which we benchmark enable auto renew.
let region = Broker::<T>::do_purchase(caller.clone(), 10u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.expect("Offer not high enough for configuration.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we advance to the fixed price phase, this is independent of the price adapter implementation and depends only on the config value set earlier in this file. Therefore it's better to unwrap here so the test fails, rather than always being mapped to Weightless.

Broker::<T>::do_assign(region, None, 2001, Final)
.map_err(|_| BenchmarkError::Weightless)?;

Expand Down Expand Up @@ -1087,11 +1105,11 @@ mod benches {
.expect("Failed to get sovereign account");
T::Currency::set_balance(
&caller.clone(),
T::Currency::minimum_balance().saturating_add(100u32.into()),
T::Currency::minimum_balance().saturating_add(10_000_000u32.into()),
);

let region = Broker::<T>::do_purchase(caller.clone(), 10u32.into())
.map_err(|_| BenchmarkError::Weightless)?;
let region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.expect("Offer not high enough for configuration.");

Broker::<T>::do_assign(region, None, task, Final)
.map_err(|_| BenchmarkError::Weightless)?;
Expand Down Expand Up @@ -1120,11 +1138,11 @@ mod benches {
let caller: T::AccountId = whitelisted_caller();
T::Currency::set_balance(
&caller.clone(),
T::Currency::minimum_balance().saturating_add(u32::MAX.into()),
T::Currency::minimum_balance().saturating_add(10_000_000u32.into()),
);

let _region = Broker::<T>::do_purchase(caller.clone(), (u32::MAX / 2).into())
.map_err(|_| BenchmarkError::Weightless)?;
let _region = Broker::<T>::do_purchase(caller.clone(), 10_000_000u32.into())
.expect("Offer not high enough for configuration.");

let timeslice = Broker::<T>::current_timeslice();

Expand Down
Loading
Loading