Make sure block_number and now are elastic scaling compatible

[athei]

Those two host functions depend on the block number. With elastic scaling this gets mixed up. We need to make sure that those host functions use the relay chain block when making calculations.

[kianenigma]

Taking a quick look at your code in trait Ext and Stack, the impl is simple enough -- We have access to T: Config everywhere, and we can easily wire this into the relay chain timestamp and block number, as we have done in many other pallets and places like BlockNumberProvider etc.

But what is your exact requirement? you want these two functions to always return the relay? or have two new sets of functions?

Need to double-check if the timestamp is in the inherent, but if not it is trivial to add.

[athei]

But what is your exact requirement? you want these two functions to always return the relay? or have two new sets of functions?

Both functions already exist. We don't want two separate functions. They should just be changed to return the relay chain block number timestamp when appropriate. Which means it should be configured on the Config trait of pallet_revive.

Timestamp

We return to the contract whatever the runtime configures on the pallet_revive config trait. We just need to change the runtime configuration to use the relay chain block number for all runtimes that are using it (Westend Assethub for now).

Block Number

This is currently hardcoded and should be changed to be configurable on the Config trait.

[kianenigma]

For block number, this is more or less all you need:

diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs
index b23d7e4e60..454b7f7e04 100644
--- a/substrate/frame/revive/src/exec.rs
+++ b/substrate/frame/revive/src/exec.rs
@@ -899,8 +899,9 @@ where
 			origin,
 			gas_meter,
 			storage_meter,
+			// TODO: also ensure that the timestamp is provided as the relay.
 			timestamp: T::Time::now(),
-			block_number: <frame_system::Pallet<T>>::block_number(),
+			block_number: T::BlockNumberProvider::current_block_number(),
 			first_frame,
 			frames: Default::default(),
 			debug_message,
diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs
index 1dee1da03b..14fbe80f12 100644
--- a/substrate/frame/revive/src/lib.rs
+++ b/substrate/frame/revive/src/lib.rs
@@ -149,6 +149,12 @@ pub mod pallet {
 		/// The time implementation used to supply timestamps to contracts through `seal_now`.
 		type Time: Time;
 
+		/// Means to provide a custom block-number that is passed on the contracts.
+		///
+		/// common implementations could be a local block number (analogous to
+		/// `frame_system::block_number()`) or a relay chain block number.
+		type BlockNumberProvider: BlockNumberProvider;
+
 		/// The fungible in which fees are paid and contract balances are held.
 		#[pallet::no_default]
 		type Currency: Inspect<Self::AccountId>

for timestamp, it is actually not part of the data that is passed to the parachain inherent, so we need to add it.

polkadot-sdk/polkadot/primitives/src/v8/mod.rs

Lines 661 to 672 in 4788579

	#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, RuntimeDebug)]
	#[cfg_attr(feature = "std", derive(Default))]
	pub struct PersistedValidationData<H = Hash, N = BlockNumber> {
	/// The parent head-data.
	pub parent_head: HeadData,
	/// The relay-chain block number this is in the context of.
	pub relay_parent_number: N,
	/// The relay-chain block storage root this is in the context of.
	pub relay_parent_storage_root: H,
	/// The maximum legal size of a POV block, in bytes.
	pub max_pov_size: u32,
	}

I have explored how this type can be made extensible in #6235.

In any case, @re-gius can help with the block number part, and then together with someone from the node team (cc @skunert) we can explore having AH collators also include the relay chain's timestamp as part of the mandatory inherent.

[bkchr]

The on chain timestamp that we are using is a UTC timestamp. There is no "relay chain timestamp" which would make sense here. Yes you could use the timestamp from the relay chain, but that doesn't make sense in the context you are discussing here. Generally the timestamp in the parachain is already checked against the relay chain timestamp to be correct. So, just use the parachain timestamp.

[xlc]

this doesn’t make much sense to me. it will be helpful to expose an additional relay block number but not making sense to make default block number as relay block number. same for timestamps.

[athei]

You suggest BLOCKNUMBER and TIMESTAMP opcodes should reference the parachain block as they do now? My thinking was that when parachains under the same relay chain communicate block numbers it would be beneficial if they agree on what it means by default.

[xlc]

maybe it can help but not really. you have to propose some use cases and requirements before deciding an implementation

none of the Eth L2 nor EVM parachains are doing this

[athei]

Then I think we should just not do this and keep it the way it is. Thanks for clearing this up.