Backport broker new price adapter (#4521) without extra changes

cumulus/parachains/runtimes/coretime/coretime-rococo/src/coretime.rs

@@ -233,5 +233,5 @@ impl pallet_broker::Config for Runtime {
 	type WeightInfo = weights::pallet_broker::WeightInfo<Runtime>;
 	type PalletId = BrokerPalletId;
 	type AdminOrigin = EnsureRoot<AccountId>;
-	type PriceAdapter = pallet_broker::Linear;
+	type PriceAdapter = pallet_broker::CenterTargetPrice<Balance>;
 }

substrate/bin/node/runtime/src/lib.rs

@@ -2060,7 +2060,7 @@ impl pallet_broker::Config for Runtime {
 	type WeightInfo = ();
 	type PalletId = BrokerPalletId;
 	type AdminOrigin = EnsureRoot<AccountId>;
-	type PriceAdapter = pallet_broker::Linear;
+	type PriceAdapter = pallet_broker::CenterTargetPrice<Balance>;
 }
 
 parameter_types! {

substrate/frame/broker/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-broker"
-version = "0.6.0"
+version = "0.7.2"
 description = "Brokerage tool for managing Polkadot Core scheduling"
 authors.workspace = true
 homepage = "https://substrate.io"
@@ -15,6 +15,7 @@ workspace = true
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+log = { version = "0.4.20", default-features = false }
 codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] }
 scale-info = { version = "2.10.0", default-features = false, features = ["derive"] }
 bitvec = { version = "1.0.0", default-features = false }
@@ -35,6 +36,7 @@ default = ["std"]
 std = [
 	"bitvec/std",
 	"codec/std",
+	"log/std",
 	"frame-benchmarking?/std",
 	"frame-support/std",
 	"frame-system/std",

substrate/frame/broker/src/adapt_price.rs

@@ -17,48 +17,122 @@
 
 #![deny(missing_docs)]
 
-use crate::CoreIndex;
+use crate::{CoreIndex, SaleInfoRecord};
 use sp_arithmetic::{traits::One, FixedU64};
+use sp_runtime::{FixedPointNumber, FixedPointOperand, Saturating};
+
+/// Performance of a past sale.
+#[derive(Copy, Clone)]
+pub struct SalePerformance<Balance> {
+	/// The price at which the last core was sold.
+	///
+	/// Will be `None` if no cores have been offered.
+	pub sellout_price: Option<Balance>,
+
+	/// The minimum price that was achieved in this sale.
+	pub end_price: Balance,
+
+	/// The number of cores we want to sell, ideally.
+	pub ideal_cores_sold: CoreIndex,
+
+	/// Number of cores which are/have been offered for sale.
+	pub cores_offered: CoreIndex,
+
+	/// Number of cores which have been sold; never more than cores_offered.
+	pub cores_sold: CoreIndex,
+}
+
+/// Result of `AdaptPrice::adapt_price`.
+#[derive(Copy, Clone)]
+pub struct AdaptedPrices<Balance> {
+	/// New minimum price to use.
+	pub end_price: Balance,
+
+	/// Price the controller is optimizing for.
+	///
+	/// This is the price "expected" by the controller based on the previous sale. We assume that
+	/// sales in this period will be around this price, assuming stable market conditions.
+	///
+	/// Think of it as the expected market price. This can be used for determining what to charge
+	/// for renewals, that don't yet have any price information for example. E.g. for expired
+	/// legacy leases.
+	pub target_price: Balance,
+}
+
+impl<Balance: Copy> SalePerformance<Balance> {
+	/// Construct performance via data from a `SaleInfoRecord`.
+	pub fn from_sale<BlockNumber>(record: &SaleInfoRecord<Balance, BlockNumber>) -> Self {
+		Self {
+			sellout_price: record.sellout_price,
+			end_price: record.price,
+			ideal_cores_sold: record.ideal_cores_sold,
+			cores_offered: record.cores_offered,
+			cores_sold: record.cores_sold,
+		}
+	}
+
+	#[cfg(test)]
+	fn new(sellout_price: Option<Balance>, end_price: Balance) -> Self {
+		Self { sellout_price, end_price, ideal_cores_sold: 0, cores_offered: 0, cores_sold: 0 }
+	}
+}
 
 /// Type for determining how to set price.
-pub trait AdaptPrice {
+pub trait AdaptPrice<Balance> {
 	/// Return the factor by which the regular price must be multiplied during the leadin period.
 	///
 	/// - `when`: The amount through the leadin period; between zero and one.
 	fn leadin_factor_at(when: FixedU64) -> FixedU64;
-	/// Return the correction factor by which the regular price must be multiplied based on market
-	/// performance.
+
+	/// Return adapted prices for next sale.
 	///
-	/// - `sold`: The number of cores sold.
-	/// - `target`: The target number of cores to be sold (must be larger than zero).
-	/// - `limit`: The maximum number of cores to be sold.
-	fn adapt_price(sold: CoreIndex, target: CoreIndex, limit: CoreIndex) -> FixedU64;
+	/// Based on the previous sale's performance.
+	fn adapt_price(performance: SalePerformance<Balance>) -> AdaptedPrices<Balance>;
 }
 
-impl AdaptPrice for () {
+impl<Balance: Copy> AdaptPrice<Balance> for () {
 	fn leadin_factor_at(_: FixedU64) -> FixedU64 {
 		FixedU64::one()
 	}
-	fn adapt_price(_: CoreIndex, _: CoreIndex, _: CoreIndex) -> FixedU64 {
-		FixedU64::one()
+	fn adapt_price(performance: SalePerformance<Balance>) -> AdaptedPrices<Balance> {
+		let price = performance.sellout_price.unwrap_or(performance.end_price);
+		AdaptedPrices { end_price: price, target_price: price }
 	}
 }
 
-/// Simple implementation of `AdaptPrice` giving a monotonic leadin and a linear price change based
-/// on cores sold.
-pub struct Linear;
-impl AdaptPrice for Linear {
+/// Simple implementation of `AdaptPrice` with two linear phases.
+///
+/// One steep one downwards to the target price, which is 1/10 of the maximum price and a more flat
+/// one down to the minimum price, which is 1/100 of the maximum price.
+pub struct CenterTargetPrice<Balance>(core::marker::PhantomData<Balance>);
+
+impl<Balance: FixedPointOperand> AdaptPrice<Balance> for CenterTargetPrice<Balance> {
 	fn leadin_factor_at(when: FixedU64) -> FixedU64 {
-		FixedU64::from(2) - when
-	}
-	fn adapt_price(sold: CoreIndex, target: CoreIndex, limit: CoreIndex) -> FixedU64 {
-		if sold <= target {
-			FixedU64::from_rational(sold.into(), target.into())
+		if when <= FixedU64::from_rational(1, 2) {
+			FixedU64::from(100).saturating_sub(when.saturating_mul(180.into()))
 		} else {
-			FixedU64::one() +
-				FixedU64::from_rational((sold - target).into(), (limit - target).into())
+			FixedU64::from(19).saturating_sub(when.saturating_mul(18.into()))
 		}
 	}
+
+	fn adapt_price(performance: SalePerformance<Balance>) -> AdaptedPrices<Balance> {
+		let Some(sellout_price) = performance.sellout_price else {
+			return AdaptedPrices {
+				end_price: performance.end_price,
+				target_price: FixedU64::from(10).saturating_mul_int(performance.end_price),
+			}
+		};
+
+		let price = FixedU64::from_rational(1, 10).saturating_mul_int(sellout_price);
+		let price = if price == Balance::zero() {
+			// We could not recover from a price equal 0 ever.
+			sellout_price
+		} else {
+			price
+		};
+
+		AdaptedPrices { end_price: price, target_price: sellout_price }
+	}
 }
 
 #[cfg(test)]
@@ -67,18 +141,103 @@ mod tests {
 
 	#[test]
 	fn linear_no_panic() {
-		for limit in 0..10 {
-			for target in 1..10 {
-				for sold in 0..=limit {
-					let price = Linear::adapt_price(sold, target, limit);
-
-					if sold > target {
-						assert!(price > FixedU64::one());
-					} else {
-						assert!(price <= FixedU64::one());
-					}
-				}
+		for sellout in 0..11 {
+			for price in 0..10 {
+				let sellout_price = if sellout == 11 { None } else { Some(sellout) };
+				CenterTargetPrice::adapt_price(SalePerformance::new(sellout_price, price));
 			}
 		}
 	}
+
+	#[test]
+	fn leadin_price_bound_check() {
+		assert_eq!(
+			CenterTargetPrice::<u64>::leadin_factor_at(FixedU64::from(0)),
+			FixedU64::from(100)
+		);
+		assert_eq!(
+			CenterTargetPrice::<u64>::leadin_factor_at(FixedU64::from_rational(1, 4)),
+			FixedU64::from(55)
+		);
+
+		assert_eq!(
+			CenterTargetPrice::<u64>::leadin_factor_at(FixedU64::from_float(0.5)),
+			FixedU64::from(10)
+		);
+
+		assert_eq!(
+			CenterTargetPrice::<u64>::leadin_factor_at(FixedU64::from_rational(3, 4)),
+			FixedU64::from_float(5.5)
+		);
+		assert_eq!(CenterTargetPrice::<u64>::leadin_factor_at(FixedU64::one()), FixedU64::one());
+	}
+
+	#[test]
+	fn no_op_sale_is_good() {
+		let prices = CenterTargetPrice::adapt_price(SalePerformance::new(None, 1));
+		assert_eq!(prices.target_price, 10);
+		assert_eq!(prices.end_price, 1);
+	}
+
+	#[test]
+	fn price_stays_stable_on_optimal_sale() {
+		// Check price stays stable if sold at the optimal price:
+		let mut performance = SalePerformance::new(Some(1000), 100);
+		for _ in 0..10 {
+			let prices = CenterTargetPrice::adapt_price(performance);
+			performance.sellout_price = Some(1000);
+			performance.end_price = prices.end_price;
+
+			assert!(prices.end_price <= 101);
+			assert!(prices.end_price >= 99);
+			assert!(prices.target_price <= 1001);
+			assert!(prices.target_price >= 999);
+		}
+	}
+
+	#[test]
+	fn price_adjusts_correctly_upwards() {
+		let performance = SalePerformance::new(Some(10_000), 100);
+		let prices = CenterTargetPrice::adapt_price(performance);
+		assert_eq!(prices.target_price, 10_000);
+		assert_eq!(prices.end_price, 1000);
+	}
+
+	#[test]
+	fn price_adjusts_correctly_downwards() {
+		let performance = SalePerformance::new(Some(100), 100);
+		let prices = CenterTargetPrice::adapt_price(performance);
+		assert_eq!(prices.target_price, 100);
+		assert_eq!(prices.end_price, 10);
+	}
+
+	#[test]
+	fn price_never_goes_to_zero_and_recovers() {
+		// Check price stays stable if sold at the optimal price:
+		let sellout_price = 1;
+		let mut performance = SalePerformance::new(Some(sellout_price), 1);
+		for _ in 0..11 {
+			let prices = CenterTargetPrice::adapt_price(performance);
+			performance.sellout_price = Some(sellout_price);
+			performance.end_price = prices.end_price;
+
+			assert!(prices.end_price <= sellout_price);
+			assert!(prices.end_price > 0);
+		}
+	}
+
+	#[test]
+	fn renewal_price_is_correct_on_no_sale() {
+		let performance = SalePerformance::new(None, 100);
+		let prices = CenterTargetPrice::adapt_price(performance);
+		assert_eq!(prices.target_price, 1000);
+		assert_eq!(prices.end_price, 100);
+	}
+
+	#[test]
+	fn renewal_price_is_sell_out() {
+		let performance = SalePerformance::new(Some(1000), 100);
+		let prices = CenterTargetPrice::adapt_price(performance);
+		assert_eq!(prices.target_price, 1000);
+	}
 }

substrate/frame/broker/src/benchmarking.rs

@@ -210,7 +210,7 @@ mod benches {
 			Event::SaleInitialized {
 				sale_start: 2u32.into(),
 				leadin_length: 1u32.into(),
-				start_price: 20u32.into(),
+				start_price: 1000u32.into(),
 				regular_price: 10u32.into(),
 				region_begin: latest_region_begin + config.region_length,
 				region_end: latest_region_begin + config.region_length * 2,
@@ -811,7 +811,7 @@ mod benches {
 			Event::SaleInitialized {
 				sale_start: 2u32.into(),
 				leadin_length: 1u32.into(),
-				start_price: 20u32.into(),
+				start_price: 1000u32.into(),
 				regular_price: 10u32.into(),
 				region_begin: sale.region_begin + config.region_length,
 				region_end: sale.region_end + config.region_length,

substrate/frame/broker/src/dispatchable_impls.rs

@@ -112,12 +112,8 @@ impl<T: Config> Pallet<T> {
 		let price = Self::sale_price(&sale, now);
 		ensure!(price_limit >= price, Error::<T>::Overpriced);
 
-		Self::charge(&who, price)?;
-		let core = sale.first_core.saturating_add(sale.cores_sold);
-		sale.cores_sold.saturating_inc();
-		if sale.cores_sold <= sale.ideal_cores_sold || sale.sellout_price.is_none() {
-			sale.sellout_price = Some(price);
-		}
+		let core = Self::purchase_core(&who, price, &mut sale)?;
+
 		SaleInfo::<T>::put(&sale);
 		let id = Self::issue(core, sale.region_begin, sale.region_end, who.clone(), Some(price));
 		let duration = sale.region_end.saturating_sub(sale.region_begin);
@@ -140,8 +136,9 @@ impl<T: Config> Pallet<T> {
 			record.completion.drain_complete().ok_or(Error::<T>::IncompleteAssignment)?;
 
 		let old_core = core;
-		let core = sale.first_core.saturating_add(sale.cores_sold);
-		Self::charge(&who, record.price)?;
+
+		let core = Self::purchase_core(&who, record.price, &mut sale)?;
+
 		Self::deposit_event(Event::Renewed {
 			who,
 			old_core,
@@ -152,19 +149,24 @@ impl<T: Config> Pallet<T> {
 			workload: workload.clone(),
 		});
 
-		sale.cores_sold.saturating_inc();
-
 		Workplan::<T>::insert((sale.region_begin, core), &workload);
 
 		let begin = sale.region_end;
 		let price_cap = record.price + config.renewal_bump * record.price;
 		let now = frame_system::Pallet::<T>::block_number();
 		let price = Self::sale_price(&sale, now).min(price_cap);
+		log::debug!(
+			"Renew with: sale price: {:?}, price cap: {:?}, old price: {:?}",
+			price,
+			price_cap,
+			record.price
+		);
 		let new_record = AllowedRenewalRecord { price, completion: Complete(workload) };
 		AllowedRenewals::<T>::remove(renewal_id);
 		AllowedRenewals::<T>::insert(AllowedRenewalId { core, when: begin }, &new_record);
 		SaleInfo::<T>::put(&sale);
 		if let Some(workload) = new_record.completion.drain_complete() {
+			log::debug!("Recording renewable price for next run: {:?}", price);
 			Self::deposit_event(Event::Renewable { core, price, begin, workload });
 		}
 		Ok(core)
@@ -282,6 +284,8 @@ impl<T: Config> Pallet<T> {
 					let workload =
 						if assigned.is_complete() { Complete(workplan) } else { Partial(assigned) };
 					let record = AllowedRenewalRecord { price, completion: workload };
+					// Note: This entry alone does not yet actually allow renewals (the completion
+					// status has to be complete for `do_renew` to accept it).
 					AllowedRenewals::<T>::insert(&renewal_id, &record);
 					if let Some(workload) = record.completion.drain_complete() {
 						Self::deposit_event(Event::Renewable {