Handle Onboarding in Current Lease Period (#2731)

* Lease out current period and trigger onboard

* Add test for trigger_onboard

* patch and add benchmark

* finish benchmarks

* Use result instead of panic for test_registrar

* nits

runtime/common/src/mock.rs

@@ -19,7 +19,7 @@
 use std::{cell::RefCell, collections::HashMap};
 use parity_scale_codec::{Encode, Decode};
 use sp_runtime::traits::SaturatedConversion;
-use frame_support::dispatch::DispatchResult;
+use frame_support::dispatch::{DispatchError, DispatchResult};
 use primitives::v1::{HeadData, ValidationCode, Id as ParaId};
 use crate::traits::Registrar;
 
@@ -57,18 +57,21 @@ impl<T: frame_system::Config> Registrar for TestRegistrar<T> {
 		PARACHAINS.with(|x| {
 			let parachains = x.borrow_mut();
 			match parachains.binary_search(&id) {
-				Ok(_) => panic!("Already Parachain"),
-				Err(_) => {},
+				Ok(_) => Err(DispatchError::Other("Already Parachain")),
+				Err(_) => Ok(()),
 			}
-		});
+		})?;
 		// Should not be parathread, then make it.
 		PARATHREADS.with(|x| {
 			let mut parathreads = x.borrow_mut();
 			match parathreads.binary_search(&id) {
-				Ok(_) => panic!("Already Parathread"),
-				Err(i) => parathreads.insert(i, id),
+				Ok(_) => Err(DispatchError::Other("Already Parathread")),
+				Err(i) => {
+					parathreads.insert(i, id);
+					Ok(())
+				},
 			}
-		});
+		})?;
 		MANAGERS.with(|x| x.borrow_mut().insert(id, manager.encode()));
 		Ok(())
 	}
@@ -76,66 +79,77 @@ impl<T: frame_system::Config> Registrar for TestRegistrar<T> {
 	fn deregister(id: ParaId) -> DispatchResult {
 		// Should not be parachain.
 		PARACHAINS.with(|x| {
-			let mut parachains = x.borrow_mut();
+			let parachains = x.borrow_mut();
 			match parachains.binary_search(&id) {
-				Ok(i) => {
-					parachains.remove(i);
-				},
-				Err(_) => {},
+				Ok(_) => Err(DispatchError::Other("cannot deregister parachain")),
+				Err(_) => Ok(()),
 			}
-		});
+		})?;
 		// Remove from parathread.
 		PARATHREADS.with(|x| {
 			let mut parathreads = x.borrow_mut();
 			match parathreads.binary_search(&id) {
 				Ok(i) => {
 					parathreads.remove(i);
+					Ok(())
 				},
-				Err(_) => {},
+				Err(_) => Err(DispatchError::Other("not parathread, so cannot `deregister`")),
 			}
-		});
+		})?;
 		MANAGERS.with(|x| x.borrow_mut().remove(&id));
 		Ok(())
 	}
 
 	fn make_parachain(id: ParaId) -> DispatchResult {
-		OPERATIONS.with(|x| x.borrow_mut().push((id, frame_system::Pallet::<T>::block_number().saturated_into(), true)));
 		PARATHREADS.with(|x| {
 			let mut parathreads = x.borrow_mut();
 			match parathreads.binary_search(&id) {
 				Ok(i) => {
 					parathreads.remove(i);
+					Ok(())
 				},
-				Err(_) => panic!("not parathread, so cannot `make_parachain`"),
+				Err(_) => Err(DispatchError::Other("not parathread, so cannot `make_parachain`")),
 			}
-		});
+		})?;
 		PARACHAINS.with(|x| {
 			let mut parachains = x.borrow_mut();
 			match parachains.binary_search(&id) {
-				Ok(_) => {},
-				Err(i) => parachains.insert(i, id),
+				Ok(_) => Err(DispatchError::Other("already parachain, so cannot `make_parachain`")),
+				Err(i) => {
+					parachains.insert(i, id);
+					Ok(())
+				},
 			}
-		});
+		})?;
+		OPERATIONS.with(|x| x.borrow_mut().push(
+			(id, frame_system::Pallet::<T>::block_number().saturated_into(), true)
+		));
 		Ok(())
 	}
 	fn make_parathread(id: ParaId) -> DispatchResult {
-		OPERATIONS.with(|x| x.borrow_mut().push((id, frame_system::Pallet::<T>::block_number().saturated_into(), false)));
 		PARACHAINS.with(|x| {
 			let mut parachains = x.borrow_mut();
 			match parachains.binary_search(&id) {
 				Ok(i) => {
 					parachains.remove(i);
+					Ok(())
 				},
-				Err(_) => panic!("not parachain, so cannot `make_parathread`"),
+				Err(_) => Err(DispatchError::Other("not parachain, so cannot `make_parathread`")),
 			}
-		});
+		})?;
 		PARATHREADS.with(|x| {
 			let mut parathreads = x.borrow_mut();
 			match parathreads.binary_search(&id) {
-				Ok(_) => {},
-				Err(i) => parathreads.insert(i, id),
+				Ok(_) => Err(DispatchError::Other("already parathread, so cannot `make_parathread`")),
+				Err(i) => {
+					parathreads.insert(i, id);
+					Ok(())
+				},
 			}
-		});
+		})?;
+		OPERATIONS.with(|x| x.borrow_mut().push(
+			(id, frame_system::Pallet::<T>::block_number().saturated_into(), false)
+		));
 		Ok(())
 	}
 

runtime/common/src/slots.rs

@@ -28,7 +28,7 @@ use frame_support::{
 	traits::{Currency, ReservableCurrency, Get}, weights::Weight,
 };
 use primitives::v1::Id as ParaId;
-use frame_system::ensure_root;
+use frame_system::{ensure_signed, ensure_root};
 use crate::traits::{Leaser, LeaseError, Registrar};
 
 type BalanceOf<T> = <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
@@ -37,12 +37,16 @@ type LeasePeriodOf<T> = <T as frame_system::Config>::BlockNumber;
 pub trait WeightInfo {
 	fn force_lease() -> Weight;
 	fn manage_lease_period_start(c: u32, t: u32) -> Weight;
+	fn clear_all_leases() -> Weight;
+	fn trigger_onboard() -> Weight;
 }
 
 pub struct TestWeightInfo;
 impl WeightInfo for TestWeightInfo {
 	fn force_lease() -> Weight { 0 }
 	fn manage_lease_period_start(_c: u32, _t:u32) -> Weight { 0 }
+	fn clear_all_leases() -> Weight { 0 }
+	fn trigger_onboard() -> Weight { 0 }
 }
 
 /// The module's configuration trait.
@@ -173,10 +177,8 @@ decl_module! {
 		/// Clear all leases for a Para Id, refunding any deposits back to the original owners.
 		///
 		/// Can only be called by the Root origin.
-		#[weight = 0] // TODO: Benchmarks
-		fn clear_all_leases(origin,
-			para: ParaId,
-		) -> DispatchResult {
+		#[weight = T::WeightInfo::clear_all_leases()]
+		fn clear_all_leases(origin, para: ParaId) -> DispatchResult {
 			ensure_root(origin)?;
 			let deposits = Self::all_deposits_held(para);
 
@@ -189,6 +191,31 @@ decl_module! {
 			Leases::<T>::remove(para);
 			Ok(())
 		}
+
+		/// Try to onboard a parachain that has a lease for the current lease period.
+		///
+		/// This function can be useful if there was some state issue with a para that should
+		/// have onboarded, but was unable to. As long as they have a lease period, we can
+		/// let them onboard from here.
+		///
+		/// Origin must be signed, but can be called by anyone.
+		#[weight = T::WeightInfo::trigger_onboard()]
+		fn trigger_onboard(origin, para: ParaId) -> DispatchResult {
+			let _ = ensure_signed(origin)?;
+			let leases = Leases::<T>::get(para);
+			match leases.first() {
+				// If the first element in leases is present, then it has a lease!
+				// We can try to onboard it.
+				Some(Some(_lease_info)) => {
+					T::Registrar::make_parachain(para)?
+				},
+				// Otherwise, it does not have a lease.
+				Some(None) | None => {
+					return Err(Error::<T>::ParaNotOnboarding.into());
+				}
+			};
+			Ok(())
+		}
 	}
 }
 
@@ -323,10 +350,11 @@ impl<T: Config> Leaser for Module<T> {
 		period_begin: Self::LeasePeriod,
 		period_count: Self::LeasePeriod,
 	) -> Result<(), LeaseError> {
+		let current_lease_period = Self::lease_period_index();
 		// Finally, we update the deposit held so it is `amount` for the new lease period
 		// indices that were won in the auction.
 		let offset = period_begin
-			.checked_sub(&Self::lease_period_index())
+			.checked_sub(&current_lease_period)
 			.and_then(|x| x.checked_into::<usize>())
 			.ok_or(LeaseError::AlreadyEnded)?;
 
@@ -376,6 +404,14 @@ impl<T: Config> Leaser for Module<T> {
 			}
 
 			let reserved = maybe_additional.unwrap_or_default();
+
+			// Check if current lease period is same as period begin, and onboard them directly.
+			// This will allow us to support onboarding new parachains in the middle of a lease period.
+			if current_lease_period == period_begin {
+				// Best effort. Not much we can do if this fails.
+				let _ = T::Registrar::make_parachain(para);
+			}
+
 			Self::deposit_event(
 				RawEvent::Leased(para, leaser.clone(), period_begin, period_count, reserved, amount)
 			);
@@ -417,7 +453,7 @@ mod tests {
 	use sp_core::H256;
 	use sp_runtime::traits::{BlakeTwo256, IdentityLookup};
 	use frame_support::{
-		parameter_types, assert_ok,
+		parameter_types, assert_ok, assert_noop,
 		traits::{OnInitialize, OnFinalize}
 	};
 	use pallet_balances;
@@ -537,7 +573,7 @@ mod tests {
 
 			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(1), Default::default(), Default::default()));
 
-			assert!(Slots::lease_out(1.into(), &1, 1, 1, 1).is_ok());
+			assert_ok!(Slots::lease_out(1.into(), &1, 1, 1, 1));
 			assert_eq!(Slots::deposit_held(1.into(), &1), 1);
 			assert_eq!(Balances::reserved_balance(1), 1);
 
@@ -563,8 +599,8 @@ mod tests {
 
 			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(1), Default::default(), Default::default()));
 
-			assert!(Slots::lease_out(1.into(), &1, 6, 1, 1).is_ok());
-			assert!(Slots::lease_out(1.into(), &1, 4, 3, 1).is_ok());
+			assert_ok!(Slots::lease_out(1.into(), &1, 6, 1, 1));
+			assert_ok!(Slots::lease_out(1.into(), &1, 4, 3, 1));
 
 			run_to_block(19);
 			assert_eq!(Slots::deposit_held(1.into(), &1), 6);
@@ -734,6 +770,62 @@ mod tests {
 			assert!(Leases::<Test>::get(ParaId::from(1)).is_empty());
 		});
 	}
+
+	#[test]
+	fn lease_out_current_lease_period() {
+		new_test_ext().execute_with(|| {
+			run_to_block(1);
+
+			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(1), Default::default(), Default::default()));
+			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(2), Default::default(), Default::default()));
+
+			run_to_block(20);
+			assert_eq!(Slots::lease_period_index(), 2);
+			// Can't lease from the past
+			assert!(Slots::lease_out(1.into(), &1, 1, 1, 1).is_err());
+			// Lease in the current period triggers onboarding
+			assert_ok!(Slots::lease_out(1.into(), &1, 1, 2, 1));
+			// Lease in the future doesn't
+			assert_ok!(Slots::lease_out(2.into(), &1, 1, 3, 1));
+
+			assert_eq!(TestRegistrar::<Test>::operations(), vec![
+				(1.into(), 20, true),
+			]);
+		});
+	}
+
+	#[test]
+	fn trigger_onboard_works() {
+		new_test_ext().execute_with(|| {
+			run_to_block(1);
+			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(1), Default::default(), Default::default()));
+			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(2), Default::default(), Default::default()));
+			assert_ok!(TestRegistrar::<Test>::register(1, ParaId::from(3), Default::default(), Default::default()));
+
+			// We will directly manipulate leases to emulate some kind of failure in the system.
+			// Para 1 will have no leases
+			// Para 2 will have a lease period in the current index
+			Leases::<Test>::insert(ParaId::from(2), vec![Some((0, 0))]);
+			// Para 3 will have a lease period in a future index
+			Leases::<Test>::insert(ParaId::from(3), vec![None, None, Some((0, 0))]);
+
+			// Para 1 should fail cause they don't have any leases
+			assert_noop!(Slots::trigger_onboard(Origin::signed(1), 1.into()), Error::<Test>::ParaNotOnboarding);
+
+			// Para 2 should succeed
+			assert_ok!(Slots::trigger_onboard(Origin::signed(1), 2.into()));
+
+			// Para 3 should fail cause their lease is in the future
+			assert_noop!(Slots::trigger_onboard(Origin::signed(1), 3.into()), Error::<Test>::ParaNotOnboarding);
+
+			// Trying Para 2 again should fail cause they are not currently a parathread
+			assert!(Slots::trigger_onboard(Origin::signed(1), 2.into()).is_err());
+
+			assert_eq!(TestRegistrar::<Test>::operations(), vec![
+				(2.into(), 1, true),
+			]);
+		});
+	}
 }
 
 #[cfg(feature = "runtime-benchmarks")]
@@ -743,7 +835,7 @@ mod benchmarking {
 	use frame_support::assert_ok;
 	use sp_runtime::traits::Bounded;
 
-	use frame_benchmarking::{benchmarks, account};
+	use frame_benchmarking::{benchmarks, account, whitelisted_caller, impl_benchmark_test_suite};
 
 	use crate::slots::Module as Slots;
 
@@ -774,7 +866,7 @@ mod benchmarking {
 			let leaser: T::AccountId = account("leaser", 0, 0);
 			T::Currency::make_free_balance_be(&leaser, BalanceOf::<T>::max_value());
 			let amount = T::Currency::minimum_balance();
-			let period_begin = 0u32.into();
+			let period_begin = 69u32.into();
 			let period_count = 3u32.into();
 		}: _(RawOrigin::Root, para, leaser.clone(), amount, period_begin, period_count)
 		verify {
@@ -787,12 +879,18 @@ mod benchmarking {
 			let c in 1 .. 100;
 			let t in 1 .. 100;
 
-			let period_begin = 0u32.into();
-			let period_count = 3u32.into();
+			let period_begin = 1u32.into();
+			let period_count = 4u32.into();
+
+			// Make T parathreads
+			let paras_info = (0..t).map(|i| {
+				register_a_parathread::<T>(i)
+			}).collect::<Vec<_>>();
+
+			T::Registrar::execute_pending_transitions();
 
 			// T parathread are upgrading to parachains
-			for i in 0 .. t {
-				let (para, leaser) = register_a_parathread::<T>(i);
+			for (para, leaser) in paras_info {
 				let amount = T::Currency::minimum_balance();
 
 				Slots::<T>::force_lease(RawOrigin::Root.into(), para, leaser, amount, period_begin, period_count)?;
@@ -827,26 +925,53 @@ mod benchmarking {
 				assert!(T::Registrar::is_parathread(ParaId::from(i)));
 			}
 		}
-	}
 
-	#[cfg(test)]
-	mod tests {
-		use super::*;
-		use crate::integration_tests::{new_test_ext, Test};
-		use frame_support::assert_ok;
+		// Assume that at most 8 people have deposits for leases on a parachain.
+		// This would cover at least 4 years of leases in the worst case scenario.
+		clear_all_leases {
+			let max_people = 8;
+			let (para, _) = register_a_parathread::<T>(1);
 
-		#[test]
-		fn force_lease_benchmark() {
-			new_test_ext().execute_with(|| {
-				assert_ok!(test_benchmark_force_lease::<Test>());
-			});
+			for i in 0 .. max_people {
+				let leaser = account("lease_deposit", i, 0);
+				let amount = T::Currency::minimum_balance();
+				T::Currency::make_free_balance_be(&leaser, BalanceOf::<T>::max_value());
+
+				// Average slot has 4 lease periods.
+				let period_count: LeasePeriodOf<T> = 4u32.into();
+				let period_begin = period_count * i.into();
+				Slots::<T>::force_lease(RawOrigin::Root.into(), para, leaser, amount, period_begin, period_count)?;
+			}
+
+			for i in 0 .. max_people {
+				let leaser = account("lease_deposit", i, 0);
+				assert_eq!(T::Currency::reserved_balance(&leaser), T::Currency::minimum_balance());
+			}
+
+		}: _(RawOrigin::Root, para)
+		verify {
+			for i in 0 .. max_people {
+				let leaser = account("lease_deposit", i, 0);
+				assert_eq!(T::Currency::reserved_balance(&leaser), 0u32.into());
+			}
 		}
 
-		#[test]
-		fn manage_lease_period_start_benchmark() {
-			new_test_ext().execute_with(|| {
-				assert_ok!(test_benchmark_manage_lease_period_start::<Test>());
-			});
+		trigger_onboard {
+			// get a parachain into a bad state where they did not onboard
+			let (para, _) = register_a_parathread::<T>(1);
+			Leases::<T>::insert(para, vec![Some((T::AccountId::default(), BalanceOf::<T>::default()))]);
+			assert!(T::Registrar::is_parathread(para));
+			let caller = whitelisted_caller();
+		}: _(RawOrigin::Signed(caller), para)
+		verify {
+			T::Registrar::execute_pending_transitions();
+			assert!(T::Registrar::is_parachain(para));
 		}
 	}
+
+	impl_benchmark_test_suite!(
+		Slots,
+		crate::integration_tests::new_test_ext(),
+		crate::integration_tests::Test,
+	);
 }

runtime/rococo/src/lib.rs

@@ -627,7 +627,7 @@ impl Randomness<Hash, BlockNumber> for ParentHashRandomness {
 }
 
 parameter_types! {
-	pub const EndingPeriod: BlockNumber = 15 * MINUTES;
+	pub const EndingPeriod: BlockNumber = 1 * HOURS;
 	pub const SampleLength: BlockNumber = 1;
 }