From 69c7c52de630219c27a41c332e8f9e758ead8855 Mon Sep 17 00:00:00 2001
From: Shawn Tabrizi <shawntabrizi@gmail.com>
Date: Sat, 27 Mar 2021 21:16:10 +0100
Subject: [PATCH] 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
---
polkadot/runtime/common/src/mock.rs | 68 ++++++----
polkadot/runtime/common/src/slots.rs | 189 ++++++++++++++++++++++-----
polkadot/runtime/rococo/src/lib.rs | 2 +-
3 files changed, 199 insertions(+), 60 deletions(-)
diff --git a/polkadot/runtime/common/src/mock.rs b/polkadot/runtime/common/src/mock.rs
index 93f639ae8a0..79ae6014f93 100644
--- a/polkadot/runtime/common/src/mock.rs
+++ b/polkadot/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(())
}
diff --git a/polkadot/runtime/common/src/slots.rs b/polkadot/runtime/common/src/slots.rs
index b38c36e9022..768c5ff957d 100644
--- a/polkadot/runtime/common/src/slots.rs
+++ b/polkadot/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(¤t_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,
+ );
}
diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs
index 33e1d585028..14c988b552a 100644
--- a/polkadot/runtime/rococo/src/lib.rs
+++ b/polkadot/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;
}
--
GitLab