Broker new price adapter

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

@@ -154,8 +154,8 @@ impl<T: frame_system::Config> pallet_broker::WeightInfo for WeightInfo<T> {
 	/// Proof: `Broker::Status` (`max_values`: Some(1), `max_size`: Some(18), added: 513, mode: `MaxEncodedLen`)
 	/// Storage: `Broker::SaleInfo` (r:1 w:1)
 	/// Proof: `Broker::SaleInfo` (`max_values`: Some(1), `max_size`: Some(57), added: 552, mode: `MaxEncodedLen`)
-	/// Storage: `Broker::AllowedRenewals` (r:1 w:2)
-	/// Proof: `Broker::AllowedRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
+	/// Storage: `Broker::PotentialRenewals` (r:1 w:2)
+	/// Proof: `Broker::PotentialRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
 	/// Storage: `System::Account` (r:1 w:0)
 	/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
 	/// Storage: `Broker::Workplan` (r:0 w:1)
@@ -337,8 +337,8 @@ impl<T: frame_system::Config> pallet_broker::WeightInfo for WeightInfo<T> {
 	}
 	/// Storage: `Broker::Status` (r:1 w:0)
 	/// Proof: `Broker::Status` (`max_values`: Some(1), `max_size`: Some(18), added: 513, mode: `MaxEncodedLen`)
-	/// Storage: `Broker::AllowedRenewals` (r:1 w:1)
-	/// Proof: `Broker::AllowedRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
+	/// Storage: `Broker::PotentialRenewals` (r:1 w:1)
+	/// Proof: `Broker::PotentialRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
 	fn drop_renewal() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `957`

cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_broker.rs

@@ -152,8 +152,8 @@ impl<T: frame_system::Config> pallet_broker::WeightInfo for WeightInfo<T> {
 	/// Proof: `Broker::Status` (`max_values`: Some(1), `max_size`: Some(18), added: 513, mode: `MaxEncodedLen`)
 	/// Storage: `Broker::SaleInfo` (r:1 w:1)
 	/// Proof: `Broker::SaleInfo` (`max_values`: Some(1), `max_size`: Some(57), added: 552, mode: `MaxEncodedLen`)
-	/// Storage: `Broker::AllowedRenewals` (r:1 w:2)
-	/// Proof: `Broker::AllowedRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
+	/// Storage: `Broker::PotentialRenewals` (r:1 w:2)
+	/// Proof: `Broker::PotentialRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
 	/// Storage: `System::Account` (r:1 w:0)
 	/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
 	/// Storage: `Broker::Workplan` (r:0 w:1)
@@ -335,8 +335,8 @@ impl<T: frame_system::Config> pallet_broker::WeightInfo for WeightInfo<T> {
 	}
 	/// Storage: `Broker::Status` (r:1 w:0)
 	/// Proof: `Broker::Status` (`max_values`: Some(1), `max_size`: Some(18), added: 513, mode: `MaxEncodedLen`)
-	/// Storage: `Broker::AllowedRenewals` (r:1 w:1)
-	/// Proof: `Broker::AllowedRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
+	/// Storage: `Broker::PotentialRenewals` (r:1 w:1)
+	/// Proof: `Broker::PotentialRenewals` (`max_values`: None, `max_size`: Some(1233), added: 3708, mode: `MaxEncodedLen`)
 	fn drop_renewal() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `556`

prdoc/pr_4521.prdoc

@@ -0,0 +1,23 @@
+title: Broker pallet: AdaptPrice price controller is now price controlled.
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      We discovered a flaw of the current price controller interface in the
+      broker pallet. This is fixed by changing the interface to no longer
+      operate on the number of cores sold, but rather on the price that was
+      achieved during the sale. More information here:
+
+      https://github.com/paritytech/polkadot-sdk/issues/4360
+
+  - audience: Runtime User
+    description: |
+      The price controller of the Coretime chain will be adjusted with this
+      release. This will very likely be used in the fellowship production
+      runtime to have a much larger leadin. This fixes a price manipulation
+      issue we discovered with the Kusama launch.
+
+crates:
+  - name: pallet-broker
+    bump: major
+

substrate/frame/broker/src/adapt_price.rs

@@ -17,58 +17,97 @@
 
 #![deny(missing_docs)]
 
-use crate::CoreIndex;
+use crate::SaleInfoRecord;
 use sp_arithmetic::{traits::One, FixedU64};
-use sp_runtime::Saturating;
+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 base price (lowest possible price) that was used in this sale.
+	pub price: Balance,
+}
+
+/// Result of `AdaptPrice::adapt_price`.
+#[derive(Copy, Clone)]
+pub struct AdaptedPrices<Balance> {
+	/// New base price to use.
+	pub price: Balance,
+	/// Price to use for renewals of leases.
+	pub renewal_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, price: record.price }
+	}
+}
 
 /// 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 this 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.price);
+		AdaptedPrices { price, renewal_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 {
+pub struct Linear<Balance>(std::marker::PhantomData<Balance>);
+
+impl<Balance: FixedPointOperand> AdaptPrice<Balance> for Linear<Balance> {
 	fn leadin_factor_at(when: FixedU64) -> FixedU64 {
-		FixedU64::from(2).saturating_sub(when)
+		FixedU64::from(101).saturating_sub(when.saturating_mul(100.into()))
 	}
-	fn adapt_price(sold: CoreIndex, target: CoreIndex, limit: CoreIndex) -> FixedU64 {
-		if sold <= target {
-			// Range of [0.5, 1.0].
-			FixedU64::from_rational(1, 2).saturating_add(FixedU64::from_rational(
-				sold.into(),
-				target.saturating_mul(2).into(),
-			))
+
+	fn adapt_price(performance: SalePerformance<Balance>) -> AdaptedPrices<Balance> {
+		let leadin_max = Self::leadin_factor_at(0.into());
+		let leadin_min = Self::leadin_factor_at(1.into());
+		let spread = leadin_max.saturating_sub(leadin_min);
+
+		let Some(sellout_price) = performance.sellout_price else {
+			return AdaptedPrices {
+				price: performance.price,
+				renewal_price: spread
+					.saturating_add(2.into())
+					.div(2.into())
+					.saturating_mul_int(performance.price),
+			}
+		};
+
+		let price = FixedU64::from(2)
+			.div(spread.saturating_add(2.into()))
+			.saturating_mul_int(sellout_price);
+
+		let price = if price == Balance::zero() {
+			// We could not recover from a price equal 0 ever.
+			sellout_price
 		} else {
-			// Range of (1.0, 2].
-
-			// Unchecked math: In this branch we know that sold > target. The limit must be >= sold
-			// by construction, and thus target must be < limit.
-			FixedU64::one().saturating_add(FixedU64::from_rational(
-				(sold - target).into(),
-				(limit - target).into(),
-			))
-		}
+			price
+		};
+
+		AdaptedPrices { price, renewal_price: sellout_price }
 	}
 }
 
@@ -78,37 +117,80 @@ 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) };
+				Linear::adapt_price(SalePerformance { sellout_price, price });
 			}
 		}
 	}
 
 	#[test]
-	fn linear_bound_check() {
-		// Using constraints from pallet implementation i.e. `limit >= sold`.
-		// Check extremes
-		let limit = 10;
-		let target = 5;
-
-		// Maximally sold: `sold == limit`
-		assert_eq!(Linear::adapt_price(limit, target, limit), FixedU64::from_float(2.0));
-		// Ideally sold: `sold == target`
-		assert_eq!(Linear::adapt_price(target, target, limit), FixedU64::one());
-		// Minimally sold: `sold == 0`
-		assert_eq!(Linear::adapt_price(0, target, limit), FixedU64::from_float(0.5));
-		// Optimistic target: `target == limit`
-		assert_eq!(Linear::adapt_price(limit, limit, limit), FixedU64::one());
-		// Pessimistic target: `target == 0`
-		assert_eq!(Linear::adapt_price(limit, 0, limit), FixedU64::from_float(2.0));
+	fn no_op_sale_is_good() {
+		let prices = Linear::adapt_price(SalePerformance { sellout_price: None, price: 1 });
+		assert_eq!(prices.renewal_price, 51);
+		assert_eq!(prices.price, 1);
+	}
+
+	#[test]
+	fn price_stays_stable_on_optimal_sale() {
+		// Check price stays stable if sold at the optimal price:
+		let mut performance = SalePerformance { sellout_price: Some(5100), price: 100 };
+		for _ in 0..10 {
+			let prices = Linear::adapt_price(performance);
+			performance.sellout_price = Some(5100);
+			performance.price = prices.price;
+
+			assert!(prices.price <= 101);
+			assert!(prices.price >= 99);
+			assert!(prices.renewal_price <= 5101);
+			assert!(prices.renewal_price >= 5099);
+		}
+	}
+
+	#[test]
+	fn price_adjusts_correctly_upwards() {
+		let performance = SalePerformance { sellout_price: Some(10_100), price: 100 };
+		let prices = Linear::adapt_price(performance);
+		assert_eq!(prices.renewal_price, 10_100);
+		assert_eq!(prices.price, 2 * 10_100 / 102);
+	}
+
+	#[test]
+	fn price_adjusts_correctly_downwards() {
+		let performance = SalePerformance { sellout_price: Some(100), price: 100 };
+		let prices = Linear::adapt_price(performance);
+		assert_eq!(prices.renewal_price, 100);
+		assert_eq!(prices.price, 2 * 100 / 102);
+	}
+
+	#[test]
+	fn price_never_goes_to_zero_and_recovers() {
+		// Check price stays stable if sold at the optimal price:
+		let sellout_price = 51;
+		let mut performance = SalePerformance { sellout_price: Some(sellout_price), price: 1 };
+		for _ in 0..11 {
+			let prices = Linear::adapt_price(performance);
+			performance.sellout_price = Some(sellout_price);
+			performance.price = prices.price;
+
+			assert!(prices.price <= sellout_price);
+			assert!(prices.price > 0);
+		}
+	}
+
+	#[test]
+	fn renewal_price_is_correct_on_no_sale() {
+		let performance = SalePerformance { sellout_price: None, price: 100 };
+		let prices = Linear::adapt_price(performance);
+		assert_eq!(prices.renewal_price, 5100);
+		assert_eq!(prices.price, 100);
+	}
+
+	#[test]
+	fn renewal_price_is_sell_out() {
+		let performance = SalePerformance { sellout_price: Some(1000), price: 100 };
+		let prices = Linear::adapt_price(performance);
+		assert_eq!(prices.renewal_price, 1000);
 	}
 }

substrate/frame/broker/src/benchmarking.rs

@@ -288,8 +288,8 @@ mod benches {
 		#[extrinsic_call]
 		_(RawOrigin::Signed(caller), region.core);
 
-		let id = AllowedRenewalId { core: region.core, when: region.begin + region_len * 2 };
-		assert!(AllowedRenewals::<T>::get(id).is_some());
+		let id = PotentialRenewalId { core: region.core, when: region.begin + region_len * 2 };
+		assert!(PotentialRenewals::<T>::get(id).is_some());
 
 		Ok(())
 	}
@@ -670,20 +670,20 @@ mod benches {
 			(T::TimeslicePeriod::get() * (region_len * 3).into()).try_into().ok().unwrap(),
 		);
 
-		let id = AllowedRenewalId { core, when };
-		let record = AllowedRenewalRecord {
+		let id = PotentialRenewalId { core, when };
+		let record = PotentialRenewalRecord {
 			price: 1u32.into(),
 			completion: CompletionStatus::Complete(new_schedule()),
 		};
-		AllowedRenewals::<T>::insert(id, record);
+		PotentialRenewals::<T>::insert(id, record);
 
 		let caller: T::AccountId = whitelisted_caller();
 
 		#[extrinsic_call]
 		_(RawOrigin::Signed(caller), core, when);
 
-		assert!(AllowedRenewals::<T>::get(id).is_none());
-		assert_last_event::<T>(Event::AllowedRenewalDropped { core, when }.into());
+		assert!(PotentialRenewals::<T>::get(id).is_none());
+		assert_last_event::<T>(Event::PotentialRenewalDropped { core, when }.into());
 
 		Ok(())
 	}