Coretime auto-renew #4424

Szegoo · 2024-05-09T16:56:41Z

This PR adds functionality that allows tasks to enable auto-renewal. Each task eligible for renewal can enable auto-renewal.

A new storage value is added to track all the cores with auto-renewal enabled and the associated task running on the core. The BoundedVec is sorted by CoreIndex to make disabling auto-renewal more efficient.

Cores are renewed at the start of a new bulk sale. If auto-renewal fails(e.g. due to the sovereign account of the task not holding sufficient balance), an event will be emitted, and the renewal will continue for the other cores.

The two added extrinsics are:

enable_auto_renew: Extrinsic for enabling auto renewal.
disable_auto_renew: Extrinsic for disabling auto renewal.

TODOs:

Write benchmarks for the newly added extrinsics.

Closes: #4351

eskimor

If it is the sovereign account of the para, why don't we implement auto renew on the para itself? Major reason, I guess: It would be impossible to mess up the timing. Otherwise each para would need to keep track of the sale cycle on Coretime.

substrate/frame/broker/src/lib.rs

Szegoo · 2024-05-10T12:26:37Z

If it is the sovereign account of the para, why don't we implement auto renew on the para itself?

But what if the parachain has multiple cores assigned to it? For example, perhaps the parachain wants to keep renewing only one core but has acquired an additional core for this bulk period due to higher demand.

The reason I am storing the TaskId here as well is to avoid having to look it up for each core when performing auto-renewals.

Szegoo · 2024-05-17T08:27:00Z

@eskimor I would appreciate a review to see if what I implemented here is sensible before writing benchmarks.

Polkadot-Forum · 2024-05-21T17:08:01Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/kusama-coretime-renewal/8219/1

substrate/frame/broker/src/tick_impls.rs

poppyseedDev · 2024-05-30T13:27:35Z

Could you please explain why this PR wasn't addressed with the comment, "This is out of scope for the broker pallet," similar to how it was done in this PR: link to PR?

seadanda · 2024-05-30T13:45:24Z

I think this PR is totally in scope for the broker pallet, renewals are a first-class operation supported directly on the coretime chain and this is a UX improvement for teams who want to renew regularly.
The PR you've linked introduces secondary market functionality directly in the broker pallet, which is not in scope for the coretime chain as defined in RFC-1. It's not that we're making arbitrary decisions about scope, if an RFC were to be accepted that defines the functionality in that PR then it would be a welcome addition

Szegoo · 2024-05-30T14:26:33Z

substrate/frame/broker/src/coretime_interface.rs

+pub trait TaskAccountInterface {
+	/// The type for representing accounts.
+	type AccountId;
+
+	/// The overarching origin type.
+	type OuterOrigin: Into<Result<Self::TaskOrigin, Self::OuterOrigin>>;
+
+	/// The custom task origin. Given that all tasks on Polkadot are parachains this will most
+	/// likely be the `Parachain` origin.
+	type TaskOrigin;
+
+	/// Ensures that the origin is a task origin and returns the associated `TaskId`.
+	fn ensure_task_sovereign_account(o: Self::OuterOrigin) -> Result<TaskId, BadOrigin>;
+
+	/// Returns the associated account of a task.
+	fn sovereign_account(task: TaskId) -> Option<Self::AccountId>;
+}


I just realized we don't need this at all. We can deal with sovereign accounts just as simple signed origin so there is no need to use the parachains custom origin.

polkadot-sdk/polkadot/xcm/xcm-builder/src/origin_conversion.rs

Lines 27 to 29 in d539778

/// Sovereign accounts use the system's `Signed` origin with an account ID derived from the

/// `LocationConverter`.

pub struct SovereignSignedViaLocation<LocationConverter, RuntimeOrigin>(

This will simplify a lot of the code.

OK cool, I'll abandon my review for now then - I'm not all the way through but just have some comments

paritytech-cicd-pr · 2024-05-30T15:20:50Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6355563

seadanda · 2024-05-30T13:57:17Z

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

@@ -232,5 +260,7 @@ impl pallet_broker::Config for Runtime {
 	type WeightInfo = weights::pallet_broker::WeightInfo<Runtime>;
 	type PalletId = BrokerPalletId;
 	type AdminOrigin = EnsureRoot<AccountId>;
+	type SovereignAccountOf = TaskSovereignAccount;
+	type MaxAutoRenewals = ConstU32<50>;


Should we set this to at least the number of cores? Slightly lower might come back to bite us

seadanda · 2024-05-30T14:04:50Z

substrate/frame/broker/src/dispatchable_impls.rs

+				// If we couldn't find the renewal record for the current bulk period we should
+				// be able to find it for the upcoming bulk period.
+				//
+				// If not the core is not eligable for renewal.


Suggested change

// If not the core is not eligable for renewal.

// If not the core is not eligible for renewal.

seadanda · 2024-05-30T14:06:33Z

substrate/frame/broker/src/migration.rs

@@ -85,7 +85,7 @@ mod v2 {
 	};

 	#[storage_alias]
-	pub type AllowedRenewals<T: Config> = StorageMap<
+	pub type PotentialRenewals<T: Config> = StorageMap<


I think your changes to this file were accidental - these should be reverted

seadanda · 2024-05-30T14:07:55Z

substrate/frame/broker/src/types.rs

+pub struct AutoRenewalRecord {
+	/// The core for which auto renewal is enabled.
+	pub core: CoreIndex,
+	/// The task is assigned to the core. We keep track of it so we don't have to look it up when


Suggested change

/// The task is assigned to the core. We keep track of it so we don't have to look it up when

/// The task assigned to the core. We keep track of it so we don't have to look it up when

seadanda · 2024-05-30T14:10:01Z

substrate/frame/broker/src/lib.rs

+		/// will be charged at the start of every bulk period for renewing core time.
+		///
+		/// - `origin`: Must be the sovereign account of the task
+		/// - `core`: The core for which we want to enable auto renewal.


Suggested change

/// - `core`: The core for which we want to enable auto renewal.

/// - `core`: The core to which the task to be renewed is currently assigned.

seadanda · 2024-05-30T15:22:47Z

substrate/frame/broker/src/coretime_interface.rs

+pub trait TaskAccountInterface {
+	/// The type for representing accounts.
+	type AccountId;
+
+	/// The overarching origin type.
+	type OuterOrigin: Into<Result<Self::TaskOrigin, Self::OuterOrigin>>;
+
+	/// The custom task origin. Given that all tasks on Polkadot are parachains this will most
+	/// likely be the `Parachain` origin.
+	type TaskOrigin;
+
+	/// Ensures that the origin is a task origin and returns the associated `TaskId`.
+	fn ensure_task_sovereign_account(o: Self::OuterOrigin) -> Result<TaskId, BadOrigin>;
+
+	/// Returns the associated account of a task.
+	fn sovereign_account(task: TaskId) -> Option<Self::AccountId>;
+}


OK cool, I'll abandon my review for now then - I'm not all the way through but just have some comments

Szegoo added 4 commits May 4, 2024 09:02

start

afb5e75

setup

2716e0f

outlining the main logic

e443c9e

bounded vec

2a8a68b

eskimor reviewed May 10, 2024

View reviewed changes

substrate/frame/broker/src/lib.rs Outdated Show resolved Hide resolved

substrate/frame/broker/src/lib.rs Outdated Show resolved Hide resolved

substrate/frame/broker/src/lib.rs Show resolved Hide resolved

Szegoo added 15 commits May 11, 2024 09:19

resolvinig some todos

8df2f81

auto-renew for legacy leases

82859ad

init test

e2a4dd5

more tests

9c1c7e3

enable_auto_renew_renews

c7ccf5a

different approach

c3eaa21

docs

edffbfc

enable_auto_renewal_with_end_hint_works

68f244c

auto_renewal_works

893fa9d

progress

0b50a58

store account instead of task

9dc9b6a

fix builds

9baa77a

disable auto-renewal

ee7e3c4

disable auto renew test

e0f1c71

docs

1a46abc

Szegoo marked this pull request as ready for review May 17, 2024 08:25

Szegoo requested a review from a team as a code owner May 17, 2024 08:25

seadanda reviewed May 22, 2024

View reviewed changes

substrate/frame/broker/src/tick_impls.rs Outdated Show resolved Hide resolved

account for changing core indices

2f17544

paritytech-review-bot bot requested a review from a team May 23, 2024 09:15

Szegoo added 2 commits May 23, 2024 11:57

fix

63b6f66

bench enable_auto_renew

aaba50d

Szegoo added 8 commits May 23, 2024 14:34

bench disable & auto_renew

7d682e6

weights

af8f109

fix

f22c451

dummy weights

a6c0676

don't auto renew when not needed

a4f3b6c

fix auto renewal

4f18949

more checks

5d29cd3

test fixes/improvements

0d9e91d

Szegoo requested a review from seadanda May 27, 2024 08:05

Szegoo added 2 commits May 30, 2024 12:59

merge

dad2b91

merge fix

88c9ce1

seadanda added the T2-pallets This PR/Issue is related to a particular pallet. label May 30, 2024

Szegoo commented May 30, 2024

View reviewed changes

cleanup | benchmarks: TODO

17e0ee5

seadanda reviewed May 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coretime auto-renew #4424

Coretime auto-renew #4424

Szegoo commented May 9, 2024 •

edited

eskimor left a comment •

edited

Szegoo commented May 10, 2024 •

edited

Szegoo commented May 17, 2024

Polkadot-Forum commented May 21, 2024

poppyseedDev commented May 30, 2024

seadanda commented May 30, 2024

Szegoo May 30, 2024 •

edited

seadanda May 30, 2024

paritytech-cicd-pr commented May 30, 2024

seadanda May 30, 2024

seadanda May 30, 2024

seadanda May 30, 2024

seadanda May 30, 2024

seadanda May 30, 2024

seadanda May 30, 2024

	/// Sovereign accounts use the system's `Signed` origin with an account ID derived from the
	/// `LocationConverter`.
	pub struct SovereignSignedViaLocation<LocationConverter, RuntimeOrigin>(

	// If not the core is not eligable for renewal.
	// If not the core is not eligible for renewal.

	/// The task is assigned to the core. We keep track of it so we don't have to look it up when
	/// The task assigned to the core. We keep track of it so we don't have to look it up when

	/// - `core`: The core for which we want to enable auto renewal.
	/// - `core`: The core to which the task to be renewed is currently assigned.

Coretime auto-renew #4424

Are you sure you want to change the base?

Coretime auto-renew #4424

Conversation

Szegoo commented May 9, 2024 • edited

eskimor left a comment • edited

Choose a reason for hiding this comment

Szegoo commented May 10, 2024 • edited

Szegoo commented May 17, 2024

Polkadot-Forum commented May 21, 2024

poppyseedDev commented May 30, 2024

seadanda commented May 30, 2024

Szegoo May 30, 2024 • edited

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

paritytech-cicd-pr commented May 30, 2024

seadanda May 30, 2024

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

seadanda May 30, 2024

Choose a reason for hiding this comment

Szegoo commented May 9, 2024 •

edited

eskimor left a comment •

edited

Szegoo commented May 10, 2024 •

edited

Szegoo May 30, 2024 •

edited