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

Coretime auto-renew #4424

Merged
merged 53 commits into from
Aug 5, 2024
Merged

Coretime auto-renew #4424

merged 53 commits into from
Aug 5, 2024

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented May 9, 2024

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

Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
substrate/frame/broker/src/lib.rs Outdated Show resolved Hide resolved
@@ -172,6 +181,11 @@ pub mod pallet {
#[pallet::storage]
pub type CoreCountInbox<T> = StorageValue<_, CoreIndex, OptionQuery>;

/// Keeping track of cores which have auto-renewal enabled.
#[pallet::storage]
Copy link
Member

Choose a reason for hiding this comment

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

You have an invariant that this list is supposed to be sorted. This should the very least be documented here.

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, keep in mind that this PR is still very much WIP.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR is merged without this being really addressed, should be a 1 liner fn try_state

@Szegoo
Copy link
Contributor Author

Szegoo commented May 10, 2024

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 Szegoo marked this pull request as ready for review May 17, 2024 08:25
@Szegoo Szegoo requested a review from a team as a code owner May 17, 2024 08:25
@Szegoo
Copy link
Contributor Author

Szegoo commented May 17, 2024

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

@Polkadot-Forum
Copy link

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

@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 23, 2024 09:15
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 1, 2024 08:53
@paritytech-cicd-pr
Copy link

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/6592890

@Szegoo Szegoo requested a review from seadanda August 1, 2024 16:15
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Looks good to me. @seadanda ?

Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

Nice, just did the last pass over your changes since my last review. Thanks!

@seadanda seadanda added this pull request to the merge queue Aug 5, 2024
@seadanda seadanda self-assigned this Aug 5, 2024
Merged via the queue into paritytech:master with commit f170af6 Aug 5, 2024
162 of 163 checks passed
/// Sorted by `CoreIndex` to make the removal of cores from auto-renewal more efficient.
#[pallet::storage]
pub type AutoRenewals<T: Config> =
StorageValue<_, BoundedVec<AutoRenewalRecord, T::MaxAutoRenewals>, ValueQuery>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being a doomer, but isn't this super not scalable? As in, up to N renewals can happen at most, and all of it is on_init, in a single block?

Moreover, if there is congestion (e.g. more than MaxAutoRenewals cores that wanna auto-renew), how do we deal with congestion? The right way to solve this kind of issue is to let user compete via more tx-fees.

I suppose this is still assuming we will only have a maximum of a few dozen cores that want to auto-renew, which is fine, but I think we could have done a bit better.

Copy link
Contributor Author

@Szegoo Szegoo Aug 5, 2024

Choose a reason for hiding this comment

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

You are right; there is an assumption that there is a fairly limited number of cores and also a limited amount that will own an entire core and do auto-renewals. However, what you mentioned is valid, but I don't think this will be an issue until the number of available cores drastically increases.

I am not really sure what you meant by 'let users compete via more tx-fees.'

ordian added a commit that referenced this pull request Aug 6, 2024
* master: (51 commits)
  Remove unused feature gated code from the minimal template (#5237)
  make polkadot-parachain startup errors pretty (#5214)
  Coretime auto-renew (#4424)
  network/strategy: Backoff and ban overloaded peers to avoid submitting the same request multiple times (#5029)
  Fix frame crate usage doc (#5222)
  beefy: Tolerate pruned state on runtime API call (#5197)
  rpc: Enable ChainSpec for polkadot-parachain (#5205)
  Add an adapter for configuring AssetExchanger (#5130)
  Replace env_logger with sp_tracing (#5065)
  Adjust sync templates flow to use new release branch (#5182)
  litep2p/discovery: Publish authority records with external addresses only (#5176)
  Run UI tests in CI for some other crates (#5167)
  Remove `pallet::getter` usage from the pallet-balances (#4967)
  pallet-timestamp: `UnixTime::now` implementation logs error only if called at genesis (#5055)
  [CI] Cache try-runtime check (#5179)
  [Backport] version bumps and the prdocs reordering from stable2407 (#5178)
  [subsystem-benchmark] Update availability-distribution-regression-bench baseline after recent subsystem changes (#5180)
  Remove pallet::getter usage from proxy (#4963)
  Remove pallet::getter macro usage from pallet-election-provider-multi-phase (#4487)
  [email protected] (#5177)
  ...
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Aug 28, 2024
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:
- [x] Write benchmarks for the newly added extrinsics.

Closes: paritytech#4351

---------

Co-authored-by: Dónal Murray <[email protected]>
@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/polkadot-coretime-renewals/10536/1

github-merge-queue bot pushed a commit that referenced this pull request Dec 12, 2024
Fix the broker pallet auto-renew benchmarks which have been broken since
#4424, yielding `Weightless` due to some prices being set too low, as
reported in #6474.

Upon further investigation it turned out that the auto-renew
contribution to `rotate_sale` was always failing but the error was
mapped. This is also fixed at the cost of a bit of setup overhead.

Fixes #6474

TODO:
- [x] Re-run weights

---------

Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Audited
Status: Completed
Development

Successfully merging this pull request may close these issues.

Coretime auto renew
8 participants