fix NextFeeMultiplier update before Revive::on_finalize

prdoc/pr_10271.prdoc

@@ -0,0 +1,17 @@
+title: fix NextFeeMultiplier update before Revive::on_finalize
+doc:
+- audience: Runtime Dev
+  description: |
+    Fix https://github.com/paritytech/polkadot-sdk/issues/10177
+
+    Store `base_fee_per_gas` and `block_gas_limit` in the  EthBlockBuilderIR, since these values are derived from the NextFeeMultiplier and we can't read it from Revive::on_finalize since this runs (in most Runtime) after TransactionPayment::on_finalize where the value is updated for the next block.
+
+    also use `BlockNumberFor<T>` for the BlockHash map instead of U256. No release have been performed yet since the introduction of that change, so that should not require any migration.
+    ```rust
+    	#[pallet::storage]
+    	pub(crate) type BlockHash<T: Config> =
+    		StorageMap<_, Identity, BlockNumberFor<T>, H256, ValueQuery>;
+    ```
+crates:
+- name: pallet-revive
+  bump: major

substrate/frame/revive/src/benchmarking.rs

@@ -1086,7 +1086,7 @@ mod benchmarks {
 		let block_hash = H256::from([1; 32]);
 
 		// Store block hash in pallet-revive BlockHash mapping
-		crate::BlockHash::<T>::insert(U256::from(0u32), block_hash);
+		crate::BlockHash::<T>::insert(crate::BlockNumberFor::<T>::from(0u32), block_hash);
 
 		let result;
 		#[block]

substrate/frame/revive/src/evm/block_hash/block_builder.rs

@@ -25,28 +25,32 @@ use crate::{
 		},
 		Block, HashesOrTransactionInfos, TYPE_EIP1559, TYPE_EIP2930, TYPE_EIP4844, TYPE_EIP7702,
 	},
-	ReceiptGasInfo,
+	Config, Pallet, ReceiptGasInfo,
 };
 
 use alloc::{vec, vec::Vec};
 
 use codec::{Decode, Encode};
-use frame_support::DefaultNoBound;
+use frame_support::traits::Time;
 use scale_info::TypeInfo;
+use sp_arithmetic::traits::Saturating;
 use sp_core::{keccak_256, H160, H256, U256};
+use sp_runtime::traits::{One, Zero};
 
 const LOG_TARGET: &str = "runtime::revive::block_builder";
 
 /// Ethereum block builder designed to incrementally build the transaction and receipt trie roots.
 ///
 /// This builder is optimized to minimize memory usage and pallet storage by leveraging the internal
 /// structure of the Ethereum trie and the RLP encoding of receipts.
-#[derive(DefaultNoBound)]
+#[cfg_attr(test, derive(frame_support::DefaultNoBound))]
 pub struct EthereumBlockBuilder<T> {
 	pub(crate) transaction_root_builder: IncrementalHashBuilder,
 	pub(crate) receipts_root_builder: IncrementalHashBuilder,
 	pub(crate) tx_hashes: Vec<H256>,
 	gas_used: U256,
+	base_fee_per_gas: U256,
+	block_gas_limit: U256,
 	logs_bloom: LogsBloom,
 	gas_info: Vec<ReceiptGasInfo>,
 	_phantom: core::marker::PhantomData<T>,
@@ -56,25 +60,30 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 	/// Converts the builder into an intermediate representation.
 	///
 	/// The intermediate representation is extracted from the pallet storage.
-	pub fn to_ir(self) -> EthereumBlockBuilderIR {
+	pub fn to_ir(self) -> EthereumBlockBuilderIR<T> {
 		EthereumBlockBuilderIR {
 			transaction_root_builder: self.transaction_root_builder.to_ir(),
 			receipts_root_builder: self.receipts_root_builder.to_ir(),
 			gas_used: self.gas_used,
 			tx_hashes: self.tx_hashes,
 			logs_bloom: self.logs_bloom.bloom,
 			gas_info: self.gas_info,
+			base_fee_per_gas: self.base_fee_per_gas,
+			block_gas_limit: self.block_gas_limit,
+			_phantom: core::marker::PhantomData,
 		}
 	}
 
 	/// Converts the intermediate representation back into a builder.
 	///
 	/// The intermediate representation is placed into the pallet storage.
-	pub fn from_ir(ir: EthereumBlockBuilderIR) -> Self {
+	pub fn from_ir(ir: EthereumBlockBuilderIR<T>) -> Self {
 		Self {
 			transaction_root_builder: IncrementalHashBuilder::from_ir(ir.transaction_root_builder),
 			receipts_root_builder: IncrementalHashBuilder::from_ir(ir.receipts_root_builder),
 			gas_used: ir.gas_used,
+			base_fee_per_gas: ir.base_fee_per_gas,
+			block_gas_limit: ir.block_gas_limit,
 			tx_hashes: ir.tx_hashes,
 			logs_bloom: LogsBloom { bloom: ir.logs_bloom },
 			gas_info: ir.gas_info,
@@ -145,14 +154,32 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 	}
 
 	/// Build the ethereum block from provided data.
-	pub fn build(
+	pub fn build_block(
+		&mut self,
+		block_number: crate::BlockNumberFor<T>,
+	) -> (Block, Vec<ReceiptGasInfo>) {
+		let parent_hash = if !Zero::is_zero(&block_number) {
+			let prev_block_num = block_number.saturating_sub(One::one());
+			crate::BlockHash::<T>::get(prev_block_num)
+		} else {
+			H256::default()
+		};
+		// Eth uses timestamps in seconds
+		let timestamp = (T::Time::now() / 1000u32.into()).into();
+		let block_author = Pallet::<T>::block_author();
+
+		let eth_block_num: U256 = block_number.into();
+		self.build_block_with_params(eth_block_num, parent_hash, timestamp, block_author)
+	}
+
+	/// Build the ethereum block from provided parameters.
+	/// This is useful for testing with custom block metadata.
+	fn build_block_with_params(
 		&mut self,
 		block_number: U256,
-		base_fee_per_gas: U256,
 		parent_hash: H256,
 		timestamp: U256,
 		block_author: H160,
-		gas_limit: U256,
 	) -> (Block, Vec<ReceiptGasInfo>) {
 		if self.transaction_root_builder.needs_first_value(BuilderPhase::Build) {
 			if let Some((first_tx, first_receipt)) = self.pallet_take_first_values() {
@@ -177,13 +204,13 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 			parent_hash,
 			timestamp,
 			miner: block_author,
-			gas_limit,
 
 			state_root: transactions_root,
 			transactions_root,
 			receipts_root,
 
-			base_fee_per_gas,
+			gas_limit: self.block_gas_limit,
+			base_fee_per_gas: self.base_fee_per_gas,
 			gas_used: self.gas_used,
 
 			logs_bloom: self.logs_bloom.bloom.into(),
@@ -220,17 +247,28 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 }
 
 /// The intermediate representation of the Ethereum block builder.
+///
+/// # Note
+///
+/// `base_fee_per_gas` and `block_gas_limit` are derived from the `NextFeeMultiplier`.
+/// We store these values instead of computing them in `on_finalize` to ensure
+/// they reflect the current block’s values, not those of the next block.
 #[derive(Encode, Decode, TypeInfo)]
-pub struct EthereumBlockBuilderIR {
+#[scale_info(skip_type_params(T))]
+pub struct EthereumBlockBuilderIR<T: Config> {
 	transaction_root_builder: IncrementalHashBuilderIR,
 	receipts_root_builder: IncrementalHashBuilderIR,
+
+	base_fee_per_gas: U256,
+	block_gas_limit: U256,
 	gas_used: U256,
 	logs_bloom: [u8; BLOOM_SIZE_BYTES],
 	pub(crate) tx_hashes: Vec<H256>,
 	pub(crate) gas_info: Vec<ReceiptGasInfo>,
+	_phantom: core::marker::PhantomData<T>,
 }
 
-impl Default for EthereumBlockBuilderIR {
+impl<T: Config> Default for EthereumBlockBuilderIR<T> {
 	fn default() -> Self {
 		Self {
 			// Default not implemented for [u8; BLOOM_SIZE_BYTES]
@@ -240,6 +278,9 @@ impl Default for EthereumBlockBuilderIR {
 			gas_used: U256::zero(),
 			tx_hashes: Vec::new(),
 			gas_info: Vec::new(),
+			base_fee_per_gas: Pallet::<T>::evm_base_fee(),
+			block_gas_limit: Pallet::<T>::evm_block_gas_limit(),
+			_phantom: core::marker::PhantomData,
 		}
 	}
 }
@@ -425,20 +466,18 @@ mod test {
 
 				let ir = incremental_block.to_ir();
 				incremental_block = EthereumBlockBuilder::from_ir(ir);
-				println!(" Log size {:?}", log_size);
+				log::debug!(target: LOG_TARGET, " Log size {log_size:?}");
 			}
 
 			// The block hash would differ here because we don't take into account
 			// the ommers and other fields from the substrate perspective.
 			// However, the state roots must be identical.
 			let built_block = incremental_block
-				.build(
+				.build_block_with_params(
 					block.number,
-					block.base_fee_per_gas,
 					block.parent_hash,
 					block.timestamp,
 					block.miner,
-					Default::default(),
 				)
 				.0;
 
@@ -456,7 +495,7 @@ mod test {
 			for enc in &encoded_tx {
 				total_size += enc.len();
 			}
-			println!("Total size used by transactions: {:?}", total_size);
+			log::debug!(target: LOG_TARGET, "Total size used by transactions: {:?}", total_size);
 
 			let mut builder = IncrementalHashBuilder::default();
 			let mut loaded = false;
@@ -478,10 +517,10 @@ mod test {
 
 			let incremental_hash = builder.finish();
 
-			println!("Incremental hash: {:?}", incremental_hash);
-			println!("Manual Hash: {:?}", manual_hash);
-			println!("Built block Hash: {:?}", built_block.transactions_root);
-			println!("Real Block Tx Hash: {:?}", block.transactions_root);
+			log::debug!(target: LOG_TARGET, "Incremental hash: {incremental_hash:?}");
+			log::debug!(target: LOG_TARGET, "Manual Hash: {manual_hash:?}");
+			log::debug!(target: LOG_TARGET, "Built block Hash: {:?}", built_block.transactions_root);
+			log::debug!(target: LOG_TARGET, "Real Block Tx Hash: {:?}", block.transactions_root);
 
 			assert_eq!(incremental_hash, block.transactions_root);
 

substrate/frame/revive/src/evm/block_storage.rs

@@ -22,11 +22,11 @@ use crate::{
 		fees::InfoT,
 	},
 	limits,
-	sp_runtime::traits::One,
+	sp_runtime::traits::{One, Zero},
 	weights::WeightInfo,
-	AccountIdOf, BalanceOf, BalanceWithDust, BlockHash, Config, ContractResult, Error,
-	EthBlockBuilderIR, EthereumBlock, Event, ExecReturnValue, Pallet, ReceiptGasInfo,
-	ReceiptInfoData, StorageDeposit, UniqueSaturatedInto, Weight, H160, H256, LOG_TARGET,
+	AccountIdOf, BalanceOf, BalanceWithDust, BlockHash, BlockNumberFor, Config, ContractResult,
+	Error, EthBlockBuilderIR, EthereumBlock, Event, ExecReturnValue, Pallet, ReceiptGasInfo,
+	ReceiptInfoData, StorageDeposit, Weight, H160, H256, LOG_TARGET,
 };
 use alloc::vec::Vec;
 use environmental::environmental;
@@ -202,43 +202,21 @@ pub fn on_initialize<T: Config>() {
 }
 
 /// Build the ethereum block and store it into the pallet storage.
-pub fn on_finalize_build_eth_block<T: Config>(
-	block_author: H160,
-	eth_block_num: U256,
-	eth_block_base_fee: U256,
-	gas_limit: U256,
-	timestamp: U256,
-) {
-	let parent_hash = if eth_block_num > U256::zero() {
-		BlockHash::<T>::get(eth_block_num - 1)
-	} else {
-		H256::default()
-	};
-
+pub fn on_finalize_build_eth_block<T: Config>(block_number: BlockNumberFor<T>) {
 	let block_builder_ir = EthBlockBuilderIR::<T>::get();
 	EthBlockBuilderIR::<T>::kill();
 
-	// Load the first values if not already loaded.
-	let (block, receipt_data) = EthereumBlockBuilder::<T>::from_ir(block_builder_ir).build(
-		eth_block_num,
-		eth_block_base_fee,
-		parent_hash,
-		timestamp,
-		block_author,
-		gas_limit,
-	);
+	let (block, receipt_data) =
+		EthereumBlockBuilder::<T>::from_ir(block_builder_ir).build_block(block_number);
 
 	// Put the block hash into storage.
-	BlockHash::<T>::insert(eth_block_num, block.hash);
+	BlockHash::<T>::insert(block_number, block.hash);
 
 	// Prune older block hashes.
 	let block_hash_count = BLOCK_HASH_COUNT;
-	let to_remove =
-		eth_block_num.saturating_sub(block_hash_count.into()).saturating_sub(One::one());
-	if !to_remove.is_zero() {
-		<BlockHash<T>>::remove(U256::from(UniqueSaturatedInto::<u32>::unique_saturated_into(
-			to_remove,
-		)));
+	let to_remove = block_number.saturating_sub(block_hash_count.into()).saturating_sub(One::one());
+	if !Zero::is_zero(&to_remove) {
+		<BlockHash<T>>::remove(to_remove);
 	}
 	// Store the ETH block into the last block.
 	EthereumBlock::<T>::put(block);

substrate/frame/revive/src/lib.rs

@@ -658,7 +658,8 @@ pub mod pallet {
 	///
 	/// The maximum number of elements stored is capped by the block hash count `BLOCK_HASH_COUNT`.
 	#[pallet::storage]
-	pub(crate) type BlockHash<T: Config> = StorageMap<_, Identity, U256, H256, ValueQuery>;
+	pub(crate) type BlockHash<T: Config> =
+		StorageMap<_, Identity, BlockNumberFor<T>, H256, ValueQuery>;
 
 	/// The details needed to reconstruct the receipt info offchain.
 	///
@@ -674,7 +675,7 @@ pub mod pallet {
 	#[pallet::storage]
 	#[pallet::unbounded]
 	pub(crate) type EthBlockBuilderIR<T: Config> =
-		StorageValue<_, EthereumBlockBuilderIR, ValueQuery>;
+		StorageValue<_, EthereumBlockBuilderIR<T>, ValueQuery>;
 
 	/// The first transaction and receipt of the ethereum block.
 	///
@@ -820,14 +821,7 @@ pub mod pallet {
 			}
 
 			// Build genesis block
-			block_storage::on_finalize_build_eth_block::<T>(
-				H160::zero(),
-				frame_system::Pallet::<T>::block_number().into(),
-				Pallet::<T>::evm_base_fee(),
-				Pallet::<T>::evm_block_gas_limit(),
-				// Eth uses timestamps in seconds
-				(T::Time::now() / 1000u32.into()).into(),
-			);
+			block_storage::on_finalize_build_eth_block::<T>(0u32.into());
 
 			// Set debug settings.
 			if let Some(settings) = self.debug_settings.as_ref() {
@@ -856,14 +850,7 @@ pub mod pallet {
 
 		fn on_finalize(block_number: BlockNumberFor<T>) {
 			// Build the ethereum block and place it in storage.
-			block_storage::on_finalize_build_eth_block::<T>(
-				Self::block_author(),
-				block_number.into(),
-				Self::evm_base_fee(),
-				Self::evm_block_gas_limit(),
-				// Eth uses timestamps in seconds
-				(T::Time::now() / 1000u32.into()).into(),
-			);
+			block_storage::on_finalize_build_eth_block::<T>(block_number);
 		}
 
 		fn integrity_test() {
@@ -1877,6 +1864,7 @@ impl<T: Config> Pallet<T> {
 	/// The Ethereum block number is identical to the Substrate block number.
 	/// If the provided block number is outside of the pruning None is returned.
 	pub fn eth_block_hash_from_number(number: U256) -> Option<H256> {
+		let number = BlockNumberFor::<T>::try_from(number).ok()?;
 		let hash = <BlockHash<T>>::get(number);
 		if hash == H256::zero() {
 			None

substrate/frame/revive/src/tests/pvm.rs

@@ -2965,7 +2965,10 @@ fn block_hash_works() {
 
 		// Store block hash in pallet-revive BlockHash mapping
 		let block_hash = H256::from([1; 32]);
-		crate::BlockHash::<Test>::insert(U256::from(0u32), H256::from(block_hash));
+		crate::BlockHash::<Test>::insert(
+			crate::BlockNumberFor::<Test>::from(0u32),
+			H256::from(block_hash),
+		);
 
 		assert_ok!(builder::call(addr)
 			.data((U256::zero(), H256::from(block_hash)).encode())