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.

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

Migrate parachain swaps to Coretime #3714

Merged
merged 24 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
36a6368
Move trait `OnSwap` from `runtime_common` to `parachains`
tdimitrov Mar 14, 2024
37962d0
Benchmark and weights
tdimitrov Mar 15, 2024
d60b31e
`swap_leases` implementation
tdimitrov Mar 15, 2024
03aaad2
Fix compilation errors
tdimitrov Mar 15, 2024
bd32142
Fix benchmark
tdimitrov Mar 18, 2024
4bd69ab
Undo trait move and fix `OnSwap` registration
tdimitrov Mar 18, 2024
518a072
Change call id of `swap_leases` to 99
tdimitrov Mar 18, 2024
296521a
Fix a compilation error
tdimitrov Mar 18, 2024
e0a1424
clippy
tdimitrov Mar 18, 2024
ff0d3d7
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
Mar 20, 2024
2f72d26
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Mar 20, 2024
b2c5ddd
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Mar 20, 2024
1953bc2
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Mar 20, 2024
e419bb7
Merge branch 'master' into tsv-ct-parachain-swaps
tdimitrov Mar 21, 2024
8986843
Merge branch 'master' into tsv-ct-parachain-swaps
eskimor Mar 21, 2024
7f630c8
Add a prdoc
tdimitrov Mar 21, 2024
cc2efb7
Implement `OnSwap` for a struct instead of for `Runtime` directly :sw…
tdimitrov Mar 21, 2024
a8f17f7
Merge branch 'master' into tsv-ct-parachain-swaps
tdimitrov Mar 21, 2024
f5f6dce
Swap no more than two leases
tdimitrov Mar 21, 2024
113bbcf
Merge branch 'master' into tsv-ct-parachain-swaps
tdimitrov Mar 21, 2024
89ef634
Apply suggestions from code review
tdimitrov Mar 22, 2024
fce11f9
Fix compilation errors and apply more code review suggestions
tdimitrov Mar 22, 2024
7351ea6
Merge branch 'master' into tsv-ct-parachain-swaps
tdimitrov Mar 26, 2024
0bbcfd1
Fix taplo errors
tdimitrov Mar 26, 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

Large diffs are not rendered by default.

Large diffs are not rendered by default.

20 changes: 20 additions & 0 deletions polkadot/runtime/parachains/src/coretime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ enum CoretimeCalls {
SetLease(pallet_broker::TaskId, pallet_broker::Timeslice),
#[codec(index = 19)]
NotifyCoreCount(u16),
#[codec(index = 99)]
SwapLeases(ParaId, ParaId),
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
}

#[frame_support::pallet]
Expand Down Expand Up @@ -233,6 +235,24 @@ impl<T: Config> Pallet<T> {
}
}
}

// Handle legacy swaps in coretime. Notifies broker parachain that a lease swap has occurred via
// XCM message. This function is meant to be used in an implementation of `OnSwap` trait.
pub fn on_legacy_lease_swap(one: ParaId, other: ParaId) {
let message = Xcm(vec![
Instruction::UnpaidExecution {
weight_limit: WeightLimit::Unlimited,
check_origin: None,
},
mk_coretime_call(crate::coretime::CoretimeCalls::SwapLeases(one, other)),
]);
if let Err(err) = send_xcm::<T::SendXcm>(
Location::new(0, [Junction::Parachain(T::BrokerId::get())]),
message,
) {
log::error!("Sending `SwapLeases` to coretime chain failed: {:?}", err);
}
}
}

impl<T: Config> OnNewSession<BlockNumberFor<T>> for Pallet<T> {
Expand Down
11 changes: 9 additions & 2 deletions polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use runtime_common::{
LocatableAssetConverter, ToAuthor, VersionedLocatableAsset, VersionedLocationConverter,
},
paras_registrar, paras_sudo_wrapper, prod_or_fast, slots,
traits::Leaser,
traits::{Leaser, OnSwap},
BlockHashCount, BlockLength, SlowAdjustingFeeUpdate,
};
use scale_info::TypeInfo;
Expand Down Expand Up @@ -1078,7 +1078,7 @@ impl paras_registrar::Config for Runtime {
type RuntimeOrigin = RuntimeOrigin;
type RuntimeEvent = RuntimeEvent;
type Currency = Balances;
type OnSwap = (Crowdloan, Slots);
type OnSwap = (Crowdloan, Slots, Self);
type ParaDeposit = ParaDeposit;
type DataDepositPerByte = DataDepositPerByte;
type WeightInfo = weights::runtime_common_paras_registrar::WeightInfo<Runtime>;
Expand Down Expand Up @@ -1306,6 +1306,13 @@ impl pallet_asset_rate::Config for Runtime {
type BenchmarkHelper = runtime_common::impls::benchmarks::AssetRateArguments;
}

// Notify `coretime` pallet when a lease swap occurs
impl OnSwap for Runtime {
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
fn on_swap(one: ParaId, other: ParaId) {
coretime::Pallet::<Runtime>::on_legacy_lease_swap(one, other);
}
}

construct_runtime! {
pub enum Runtime
{
Expand Down
11 changes: 9 additions & 2 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use runtime_common::{
LocatableAssetConverter, ToAuthor, VersionedLocatableAsset, VersionedLocationConverter,
},
paras_registrar, paras_sudo_wrapper, prod_or_fast, slots,
traits::Leaser,
traits::{Leaser, OnSwap},
BalanceToU256, BlockHashCount, BlockLength, CurrencyToVote, SlowAdjustingFeeUpdate,
U256ToBalance,
};
Expand Down Expand Up @@ -1302,7 +1302,7 @@ impl paras_registrar::Config for Runtime {
type RuntimeOrigin = RuntimeOrigin;
type RuntimeEvent = RuntimeEvent;
type Currency = Balances;
type OnSwap = (Crowdloan, Slots);
type OnSwap = (Crowdloan, Slots, Self);
type ParaDeposit = ParaDeposit;
type DataDepositPerByte = RegistrarDataDepositPerByte;
type WeightInfo = weights::runtime_common_paras_registrar::WeightInfo<Runtime>;
Expand Down Expand Up @@ -1414,6 +1414,13 @@ impl pallet_asset_rate::Config for Runtime {
type BenchmarkHelper = runtime_common::impls::benchmarks::AssetRateArguments;
}

// Notify `coretime` pallet when a lease swap occurs
impl OnSwap for Runtime {
fn on_swap(one: ParaId, other: ParaId) {
coretime::Pallet::<Runtime>::on_legacy_lease_swap(one, other);
}
}

#[frame_support::runtime(legacy_ordering)]
mod runtime {
#[runtime::runtime]
Expand Down
17 changes: 17 additions & 0 deletions substrate/frame/broker/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,23 @@ mod benches {
Ok(())
}

#[benchmark]
fn swap_leases() -> Result<(), BenchmarkError> {
let admin_origin =
T::AdminOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;

// Add two leases in `Leases`
let n = (T::MaxLeasedCores::get() / 2) as usize;
let mut leases = vec![LeaseRecordItem { task: 1, until: 10u32.into() }; n];
leases.extend(vec![LeaseRecordItem { task: 2, until: 20u32.into() }; n]);
Leases::<T>::put(BoundedVec::try_from(leases).unwrap());

#[extrinsic_call]
_(admin_origin as T::RuntimeOrigin, 1, 2);

Ok(())
}

// Implements a test for each benchmark. Execute with:
// `cargo test -p pallet-broker --features runtime-benchmarks`.
impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test);
Expand Down
33 changes: 33 additions & 0 deletions substrate/frame/broker/src/dispatchable_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use frame_support::{
};
use sp_arithmetic::traits::{CheckedDiv, Saturating, Zero};
use sp_runtime::traits::Convert;
use sp_std::vec::Vec;
use CompletionStatus::{Complete, Partial};

impl<T: Config> Pallet<T> {
Expand Down Expand Up @@ -437,4 +438,36 @@ impl<T: Config> Pallet<T> {
Self::deposit_event(Event::AllowedRenewalDropped { core, when });
Ok(())
}

pub(crate) fn do_swap_leases(id: TaskId, other: TaskId) -> DispatchResult {
let mut id_exists = false;
let mut other_exists = false;
let leases = Leases::<T>::get()
.into_inner()
.into_iter()
.map(|mut lease| {
if lease.task == id {
lease.task = other;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think TaskId uniqueness is enforced anywhere in Leases, so this effectively swaps all leases sharing a TaskId. This shouldn't really matter as there should only be one task for both id and other.

Copy link
Member

Choose a reason for hiding this comment

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

Good point though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought there could be multiple leases and we do want to swap all of them.

If this is the case, we'd better add another ensure! for it?

Copy link
Contributor Author

@tdimitrov tdimitrov Mar 21, 2024

Choose a reason for hiding this comment

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

Like this -> f5f6dce

The error code I use is not entirely correct but I don't see a point adding a new one just for a temp call.

Copy link
Member

Choose a reason for hiding this comment

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

No Basti has a point. The "correctness" of the swap has already been verified on the relay chain, we should be very lenient on the Coretime chain. In particular, there could be a race and the lease of one para expired by the time the message arrives at the coretime chain (admittedly super unlikely edge case, but still), we should still do the swap, although one of the leases does not exist.

I mean that race in particular is really super niche, if a user updates that close he could miss the deadline himself already.

Nevertheless, I would err on as little ensures as possible: The swap is confirmed, it actually has to be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I came to the same conclusion and removed everything. Even if someone decides to use the extrinsic directly it can be called again and 'undo' the result.

id_exists = true;
} else if lease.task == other {
lease.task = id;
other_exists = true;
}
lease
})
.collect::<Vec<_>>();

ensure!(id_exists && other_exists, Error::<T>::UnknownReservation);

let leases = if let Ok(leases) = BoundedVec::try_from(leases) {
leases
} else {
defensive!("leases is extracted from a `BoundedVec` without adding new elements so the conversation should always succeed");
return Err(DispatchError::Other("Failed to convert leases to BoundedVec"));
};

Leases::<T>::set(leases);

tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}
}
8 changes: 8 additions & 0 deletions substrate/frame/broker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,5 +786,13 @@ pub mod pallet {
Self::do_notify_core_count(core_count)?;
Ok(())
}

#[pallet::call_index(99)]
#[pallet::weight(T::WeightInfo::swap_leases())]
pub fn swap_leases(origin: OriginFor<T>, id: TaskId, other: TaskId) -> DispatchResult {
T::AdminOrigin::ensure_origin_or_root(origin)?;
Self::do_swap_leases(id, other)?;
Ok(())
}
}
}
Loading
Loading