[FRAME] Prepare pallets for dynamic block durations

[ggwpez]

Currently we just use System::block_number() in many pallet and derive a timestamp from it. This will not work anymore when parachains have changing block times, either from async backing or coretime.

Possible Solution

^{(the naming here are just placeholders)}

• Create a new system config item: BlockNumberProvider which can then either be configured to RelaychainDataProvider when its a parachain runtime or () for relay runtimes.
• Add a function System::provided_block_number() -> Number
• Add a function System::local_block_number() -> Number (for migrations and to avoid ambiguity)
• Deprecate System::block_number()

We then need to adapt a ton of pallets and check whether their storage needs to be migrated.

[kianenigma]

This is more or less the proposed solution that I extracted from the call, mostly my attempt to capture an idea that @gupnik expressed:

diff --git a/substrate/frame/scheduler/src/lib.rs b/substrate/frame/scheduler/src/lib.rs
index e94f154eee..bcb5aff728 100644
--- a/substrate/frame/scheduler/src/lib.rs
+++ b/substrate/frame/scheduler/src/lib.rs
@@ -334,7 +334,9 @@ pub mod pallet {
 		#[pallet::weight(<T as Config>::WeightInfo::schedule(T::MaxScheduledPerBlock::get()))]
 		pub fn schedule(
 			origin: OriginFor<T>,
-			when: BlockNumberFor<T>,
+			// we provide this type to all dispatchables wishing to reference the future. Later on,
+			// `fn passed` can be used to check if this time is already passed.
+			when: T::RuntimeTime,
 			maybe_periodic: Option<schedule::Period<BlockNumberFor<T>>>,
 			priority: schedule::Priority,
 			call: Box<<T as Config>::RuntimeCall>,
diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs
index 069217bcee..ab725a7dd5 100644
--- a/substrate/frame/system/src/lib.rs
+++ b/substrate/frame/system/src/lib.rs
@@ -570,6 +570,64 @@ pub mod pallet {
 
 		/// The maximum number of consumers allowed on a single account.
 		type MaxConsumers: ConsumerLimits;
+
+		/// Something that can represent a notion of time within this runtime.
+		///
+		/// It can be one's block number, timestamp or similar, depending on wether this is being
+		/// used in a parachain or relay chain context, and with or without async backing.
+		///
+		/// Be aware that changing this type midflight probably has a lot of consequences.
+		type RuntimeTime: RuntimeTime;
+	}
+
+	/// An operation that can happen far in the future.
+	trait RuntimeTime:
+		// This type should be storage-friendly..
+		codec::Codec
+		// dispatchable	friendly..
+		+ Parameter
+		+ Member
+		// and compare-able ..
+		+ core::cmp::PartialOrd
+		+ core::cmp::Eq
+		// and subtract-able.
+		+ sp_runtime::traits::CheckedSub
+	{
+		/// Return the notion of time "now".
+		fn now() -> Self;
+
+		/// Just a shorthand for `now() >= other`.
+		fn passed(&self, other: &Self::Time) -> bool {
+			*self >= *other
+		}
+
+		/// Just a shorthand for `now() - other`.
+		fn remaining(&self, other: &Self::Time) -> Self::Time {
+			self.now() - *other
+		}
+	}
+
+	/// Use my own block number.
+	pub struct SelfBlockNumber<T>(BlockNumberFor<T>);
+	impl<T: Config> RuntimeTime for SelfBlockNumber<T> {
+		fn now() -> Self {
+			Self(Pallet::<T>::deprecated_dont_use_block_number())
+		}
+	}
+
+	/// TOOD: should be provided by parachain-system, not here.
+	pub struct RelayBlockNumber<T>(BlockNumberFor<T>);
+	impl<T: Config> RuntimeTime for RelayBlockNumber<T> {
+		fn now() -> Self {
+			unimplemented!("read from some hardcoded key?")
+		}
+	}
+
+	pub struct Timestamp<T>(u64);
+	impl<T: Config> RuntimeTime for Timestamp<T> {
+		fn now() -> Self {
+			unimplemented!("call into pallet-timestamp")
+		}
 	}
 
 	#[pallet::pallet]
@@ -869,7 +927,7 @@ pub mod pallet {
 	/// The current block number being processed. Set by `execute_block`.
 	#[pallet::storage]
 	#[pallet::whitelist_storage]
-	#[pallet::getter(fn block_number)]
+	#[pallet::getter(fn deprecated_dont_use_block_number)]
 	pub(super) type Number<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;
 
 	/// Hash of the previous block.

If it works, it looks elegant and future-proof to me, but I would be open to a simpler solution as well. @ggwpez's proposed solution seems more aligned with the goal of simplicity.

[ggwpez]

Yea the RuntimeTime (or Instant maybe) does still make sense to me. Its not prevented by the BlockNumberProvider proposal.

I dont know how much effort it is to refactor the pallets to use the new Instant type. Otherwise we can use BlockNumberProvider for legacy pallets to quickly port them and then the Instant for new pallets?
But if its easy to port them, then just having the Instant would be better i think.

[xlc]

I am not sure if we want a central config. In fact, a well written pallet should not be reading System::block_number and instead have its own config type to allow the runtime to specify the block number / timestamp. So this doesn't apply to all the well written pallets.

For other pallets, yeah, they need to migrate. But instead of migrate to System::provided_block_number or System::local_block_number, they really should be just switch to a BlockNumberProvider in config.

[ggwpez]

I am not sure if we want a central config. In fact, a well written pallet should not be reading System::block_number and instead have its own config type to allow the runtime to specify the block number / timestamp. So this doesn't apply to all the well written pallets.

But this will lead to all pallets having a BlockNumberProvider config item, which is why it could be de-duplicated by just putting into System.
It also ensure that all pallets use the same BlockNumberProvider, since otherwise it could lead to issues.

[ggwpez]

Actually this does not work since there can be multiple para blocks per relay block...
Gav mentioned that we could use the relay timeslot. So it would basically be Kians suggestion with the Opaque type.

[xlc]

But this will lead to all pallets having a BlockNumberProvider config item, which is why it could be de-duplicated by just putting into System.

No. Only pallets depends on BlockNumberProvider will need to add it. Many pallets don't need it. And explicit dependency is a good thing.

It also ensure that all pallets use the same BlockNumberProvider, since otherwise it could lead to issues.

I argue exact the opposite. For some pallet, it is perfectly fine to use local block number and for some, they should use relay, and for some others, they should use timestamp. It is just not possible to come up something that works for all the pallets.

[ggwpez]

I argue exact the opposite. For some pallet, it is perfectly fine to use local block number and for some, they should use relay, and for some others, they should use timestamp. It is just not possible to come up something that works for all the pallets.

Okay makes sense. Then i assume a generic InstantProvider (or TimestampProvider) on a per-pallet basis could work?
This could then be configured to use local blocknumer, relay blocknumber of some timestamp inherent. If you have something else in mind then please make a concrete proposal.
I will add a point to the next Fellowship call agenda.

[xlc]

Yes. I don’t see much changes needed in frame system. We just need to migrate each pallet one by one and maybe add some helpers.

[kianenigma]

It is just not possible to come up something that works for all the pallets.

I see the point in this, yeah. I was hoping by making this be part of frame-system we can make the process of using it easier, but I don't see a better way for now either.

For such cases, there could perhaps be "opinionated" versions of frame-system that has all this stuff configured for you, and you won't need to infiltrate each pallet with new types to use common functionalities such as BlockNumber or even a basic currency.

[xlc]

Most of the pallet doesn’t need to access block number / timestamp so it should be perfectly fine to require few more lines for those

[ggwpez]

I put a draft up here: #3298
You can treat it as a scratch pad. Its just what i came up with now, so probably needs improvements.

Lets settle on the basic types first before implementing it in runtime/pallets.

[gupnik]

Saw that these are the pallets that use System::block_number.

I believe that these are the ones that need to be migrated to use the relay chain block number.

Already present on AH:

• multisig Adds BlockNumberProvider in multisig, proxy and nft pallets #5723
• proxy Adds BlockNumberProvider in multisig, proxy and nft pallets #5723
• nfts Adds BlockNumberProvider in multisig, proxy and nft pallets #5723

Others:

• alliance: Adds BlockNumberProvider in alliance, collective and identity pallets #6282
• atomic-swap: Can potentially use either but might be better to use the relay BN
• bounties: Update Treasury to Support Relay Chain Block Number Provider #3970
• child-bounties: Update Treasury to Support Relay Chain Block Number Provider #3970
• collective: Adds BlockNumberProvider in alliance, collective and identity pallets #6282
• conviction-voting
• core-fellowship
• democracy
• identity: Adds BlockNumberProvider in alliance, collective and identity pallets #6282
• lottery
• nis
• nomination-pools
• recovery
• referenda
• safe-mode
• salary
• society
• tips
• transaction_storage
• treasury: Update Treasury to Support Relay Chain Block Number Provider #3970

while these can continue to use the parachain block number and do not need a migration:

• babe
• beefy
• broker (Already supports block number provider)
• election-provider-multi-phase
• executive
• grandpa
• im-online
• insecure-randomness-collective-flip
• merkle-mountain-range
• migrations
• mixnet
• scheduler: Not needed as per @xlc's comment below
• revive

Please call out if something looks incorrect @ggwpez @kianenigma @xlc @shawntabrizi

[xlc]

scheduler doesn’t need migration as it has to use local block number or need some major refactoring

[ggwpez]

I believe that these are the ones that need to be migrated to use the relay chain block number:

We have to prioritize them by need for Plaza, so please check what is currently on AssetHub and then check with Jan-Jan about the pallets that we want to move from the Relay.

scheduler doesn’t need migration as it has to use local block number or need some major refactoring

Yea we can probably do it later, since the scheduler relies on every block number being reached eventually.

[xlc]

We need to figure out a migration strategy first before committing to implement migrations, otherwise it will just be wasted work #5656 (comment)

[kianenigma]

I think we should go with the migration and be over with it. Deploying some trick that assumes Para's block time a fixed function of the RC block time (para = rc / 2 + offset) is a time bomb that will never work well with elastic scaling.

For pallets that are currently in parachains, such as broker #5656 (comment), they should migrate their actual data to use the relay chain block number. For example a deployed version of multisig on a parachain should do the same.

The pallets that we intend to move from the Relay chain to asset hub won't need any migration, as they already move the relay chain block number, and in the future they will continue to use the relay chain block number.

[xlc]

I don't think you fully understand my suggestion. It will work regardless para local block time.
Bit more explanation:

Right now, all the code (i.e. runtime code and UI code) assumes parachain have ~12s block time (invariant 1).
We are planning to break such invariant and therefore need some migration.
One potential solution is to migrate to use relay block time, which is 6s (invariant 2).
This is fine for runtime as it never need to deal with historical data. So it only need to deal with current invariant and that's easy.
However, for UI, block explorer, analytic service etc, they need to deal with historical data. i.e. Before block X, use invariant 1, after block X, use invariant 2. That's not going to work. It is also significantly worse than not working as it will result in corrupted data if the indexer did not update before the migration is enacted. And how can they know on which block the invariant changed? which runtime release for all the chains? This is just a VERY bad idea.

So my suggestion is just ditch the original meaning of the block numbers and strictly follow invariant 1. They are no longer block numbers. They are just some number expected to be increment by 1 every ~12s and that's it. All the block number to time calculation logic will be compatible. The only broken thing is it is no longer a parachain block number (so does the previous solution) which may or may not be fine (case by case).

Maybe there are better ways but whatever it is, please make sure it will never result in corrupted data. For example, change the storage name will be fine by me. It is a fully breaking change but it will never result in unexpected values in db that could be hard to correct.

[joepetrowski]

Responding to #5656 (comment) here...

I don't think this is a good approach because it puts a lower bound on the block time at 12s. If a chain uses multiple cores or does 500ms blocks then it's impossible to fit into this paradigm. To adapt, we would just have to change block number to something like timestamp (which may not be entirely bad, but we still have the same storage migration issue here).

However, for UI, block explorer, analytic service etc, they need to deal with historical data. i.e. Before block X, use invariant 1, after block X, use invariant 2. That's not going to work. It is also significantly worse than not working as it will result in corrupted data if the indexer did not update before the migration is enacted. And how can they know on which block the invariant changed? which runtime release for all the chains?

I'm not convinced. The context here is mostly about projecting future enactment times. Yes, these services need historical data, but things like vesting schedules, scheduler, proxy delays, etc. are all related to, "from this point in time, how far in the future do we expect X to happen". There are not many historical queries of this nature because most historical queries are about the state of something at the time, not how far away something is from that point in time.

[xlc]

I think lower bound of 12s is fine? because otherwise is using relaychain block number and the lower bound is 6s and in what case 6s is required and 12s is not enough?

Maybe we should talk to the actual service builder for their opinions? As they are the people going to be impacted by this decision. So that will be the teams building the core time UI and gov UI and wallets.

[joepetrowski]

I think lower bound of 12s is fine? because otherwise is using relaychain block number and the lower bound is 6s and in what case 6s is required and 12s is not enough?

It's using 6s as a clock to allow things in the future. It's fundamentally different to what you are proposing by altering the meaning of block number. By using the RC block time, you can say, "I want this proxied call to expire in 10 minutes (100 RC blocks)". The parachain prescribing this could theoretically have 1,000 blocks in this timeframe, and blocks 990-1000 would all fall between RC blocks 99 and 100. As in, many parachain blocks could get the same result from block_number() with Relay Chain as the provider. But saying that the block number type can only increment by 1 every 12 seconds means that that is not the case (and prevents faster block production).

Maybe we should talk to the actual service builder for their opinions? As they are the people going to be impacted by this decision. So that will be the teams building the core time UI and gov UI and wallets.

Sure :). cc @kianenigma @gupnik @seadanda

[muharem]

I’d like to summarize one option available to us. I’ve chosen the simplest solution that appears to meet our needs. Please share your feedback with a focus on finding a way forward. We need to start implementing a solution as part for the migration to Asset Hub. Our goal can be specified as follows:

• ensure greater determinism (at least as good as we currently have) than the parachains block number clock, making it more useful for specific use cases like referenda periods (though it doesn’t have to be applicable to every use case).
• support the migration from the Relay Chain to the Asset Hub (pallets like OpenGov, Staking, etc).

We probably want the solution to be:

• correct;
• simple (for both runtime engineers and service/client engineers);
• minimal in added complexity or entropy from new concepts and logic.

Solution:

Use BlockNumberProvider as a configurable type parameter in pallets that need it. The provider can be set to follow either the Relay Chain block numbers or the Parachain's block numbers, depending on the requirement.

The Relay Chain block number clock is already in use and proven by time. We do not add anything new, except that some pallets instances on a Parachain might follow the Relay Chain block numbers instead of the Local Chain.

This approach simplifies state migration for pallets moving to the Asset Hub, as block numbers will not usually need to be mapped (although in some cases, shifting may be required to account for the time the migration took).

Pallets that rely on block number-based logic (generally within hooks) should adjust to operate based on conditions like greater/less than or equal instead only equal, since parachains may not execute state transitions with every Relay Chain block.

Do we miss any use case where this approach is not applicable?

@josepot @Tbaut @ERussel we need your feedback here

[kianenigma]

Thank you for the summary @muharem! In short, I would name the action items as:

Per pallet:

1. Add type BlockNumberProvider
2. Explore if == needs to be >=/<= now
3. Explore if any BlockNumber type is stored in storage. This might entail a migration

Ideas to improve this globally:

1. Use a newtype to distinguish relay block number and para block number.
2. Remove BlockNumber parameter from Hooks. It seems like a footgun, see here.

The list of pallets to be migrated is here. A few examples are in place to serve as inspiration.

[kianenigma]

I would like to close this since the organization is better tracked in #6297

the content in this issue can remain useful as background knowledge.