[revive] Receipts should include failed tx

.github/workflows/tests-evm.yml

@@ -43,7 +43,7 @@ jobs:
         uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
         with:
           repository: paritytech/evm-test-suite
-          ref: d20230d0a70ad1159ee75b4c56a47e85173f19e5
+          ref: c2422cace2fb8a4337fc1c704c49e458c8a79d6b
           path: evm-test-suite
 
       - uses: denoland/setup-deno@v1
@@ -68,13 +68,6 @@ jobs:
           echo "== Running evm tests =="
           START_REVIVE_DEV_NODE=true START_ETH_RPC=true deno task test:evm
 
-      - name: Collect tests results
-        if: always()
-        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
-        with:
-          name: evm-test-suite-${{ github.sha }}
-          path: evm-test-suite/test-logs/matter-labs-tests.log
-
   confirm-required-test-evm-jobs-passed:
     runs-on: ubuntu-latest
     name: All test misc tests passed

prdoc/pr_10120.prdoc

@@ -0,0 +1,11 @@
+title: '[revive] Receipts should include failed tx'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Fix Eth block receipt production, the current approach didn't work as failed extrinsic, revert all storage changes.
+    This PR updates eth transactions, so that they always succeed, the logic is wrapped into a top level `with_transaction` that will rollback all storage change in case of failure
+crates:
+- name: pallet-revive
+  bump: patch
+- name: pallet-revive-eth-rpc
+  bump: patch

substrate/frame/revive/rpc/src/receipt_extractor.rs

@@ -18,8 +18,10 @@ use crate::{
 	client::{runtime_api::RuntimeApi, SubstrateBlock, SubstrateBlockNumber},
 	subxt_client::{
 		self,
-		revive::{calls::types::EthTransact, events::ContractEmitted},
-		system::events::ExtrinsicSuccess,
+		revive::{
+			calls::types::EthTransact,
+			events::{ContractEmitted, EthExtrinsicRevert},
+		},
 		SrcChainConfig,
 	},
 	ClientError, H160, LOG_TARGET,
@@ -179,10 +181,10 @@ impl ReceiptExtractor {
 		let events = ext.events().await?;
 		let block_number: U256 = substrate_block.number().into();
 
-		let success = events.has::<ExtrinsicSuccess>().inspect_err(|err| {
+		let success = !events.has::<EthExtrinsicRevert>().inspect_err(|err| {
 			log::debug!(
 				target: LOG_TARGET,
-				"Failed to lookup for ExtrinsicSuccess event in block {block_number}: {err:?}"
+				"Failed to lookup for EthExtrinsicRevert event in block {block_number}: {err:?}"
 			);
 		})?;
 

substrate/frame/revive/src/benchmarking.rs

@@ -348,6 +348,16 @@ mod benchmarks {
 		assert_eq!(Pallet::<T>::evm_balance(&addr), evm_value);
 	}
 
+	#[benchmark(pov_mode = Measured)]
+	fn deposit_eth_extrinsic_revert_event() {
+		#[block]
+		{
+			Pallet::<T>::deposit_event(Event::<T>::EthExtrinsicRevert {
+				dispatch_error: crate::Error::<T>::BenchmarkingError.into(),
+			});
+		}
+	}
+
 	// `i`: Size of the input in bytes.
 	// `s`: Size of e salt in bytes.
 	#[benchmark(pov_mode = Measured)]
@@ -2743,7 +2753,7 @@ mod benchmarks {
 				);
 
 				// Store transaction
-				let _ = block_storage::with_ethereum_context(|| {
+				let _ = block_storage::bench_with_ethereum_context(|| {
 					let (encoded_logs, bloom) =
 						block_storage::get_receipt_details().unwrap_or_default();
 
@@ -2816,7 +2826,7 @@ mod benchmarks {
 			);
 
 			// Store transaction
-			let _ = block_storage::with_ethereum_context(|| {
+			let _ = block_storage::bench_with_ethereum_context(|| {
 				let (encoded_logs, bloom) =
 					block_storage::get_receipt_details().unwrap_or_default();
 
@@ -2881,7 +2891,7 @@ mod benchmarks {
 		let gas_used = Weight::from_parts(1_000_000, 1000);
 
 		// Store transaction
-		let _ = block_storage::with_ethereum_context(|| {
+		let _ = block_storage::bench_with_ethereum_context(|| {
 			let (encoded_logs, bloom) = block_storage::get_receipt_details().unwrap_or_default();
 
 			let block_builder_ir = EthBlockBuilderIR::<T>::get();
@@ -2943,7 +2953,7 @@ mod benchmarks {
 		let gas_used = Weight::from_parts(1_000_000, 1000);
 
 		// Store transaction
-		let _ = block_storage::with_ethereum_context(|| {
+		let _ = block_storage::bench_with_ethereum_context(|| {
 			let (encoded_logs, bloom) = block_storage::get_receipt_details().unwrap_or_default();
 
 			let block_builder_ir = EthBlockBuilderIR::<T>::get();

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

@@ -125,14 +125,14 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 		// The first transaction and receipt are returned to be stored in the pallet storage.
 		// The index of the incremental hash builders already expects the next items.
 		if self.tx_hashes.len() == 1 {
-			log::debug!(target: LOG_TARGET, "Storing first transaction and receipt in pallet storage");
+			log::trace!(target: LOG_TARGET, "Storing first transaction and receipt in pallet storage");
 			self.pallet_put_first_values((transaction_encoded, encoded_receipt));
 			return;
 		}
 
 		if self.transaction_root_builder.needs_first_value(BuilderPhase::ProcessingValue) {
 			if let Some((first_tx, first_receipt)) = self.pallet_take_first_values() {
-				log::debug!(target: LOG_TARGET, "Loaded first transaction and receipt from pallet storage");
+				log::trace!(target: LOG_TARGET, "Loaded first transaction and receipt from pallet storage");
 				self.transaction_root_builder.set_first_value(first_tx);
 				self.receipts_root_builder.set_first_value(first_receipt);
 			} else {
@@ -159,7 +159,7 @@ impl<T: crate::Config> EthereumBlockBuilder<T> {
 				self.transaction_root_builder.set_first_value(first_tx);
 				self.receipts_root_builder.set_first_value(first_receipt);
 			} else {
-				log::debug!(target: LOG_TARGET, "Building an empty block");
+				log::trace!(target: LOG_TARGET, "Building an empty block");
 			}
 		}
 

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

@@ -252,7 +252,7 @@ impl IncrementalHashBuilder {
 				.take()
 				.expect("First value must be set when processing index 127; qed");
 
-			log::debug!(target: LOG_TARGET, "Adding first value at index 0 while processing index 127");
+			log::trace!(target: LOG_TARGET, "Adding first value at index 0 while processing index 127");
 
 			let rlp_index = rlp::encode_fixed_size(&0usize);
 			self.hash_builder.add_leaf(Nibbles::unpack(&rlp_index), &encoded_value);
@@ -284,7 +284,7 @@ impl IncrementalHashBuilder {
 		// first value index is the last one in the sorted vector
 		// by rlp encoding of the index.
 		if let Some(encoded_value) = self.first_value.take() {
-			log::debug!(target: LOG_TARGET, "Adding first value at index 0 while building the trie");
+			log::trace!(target: LOG_TARGET, "Adding first value at index 0 while building the trie");
 
 			let rlp_index = rlp::encode_fixed_size(&0usize);
 			self.hash_builder.add_leaf(Nibbles::unpack(&rlp_index), &encoded_value);

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

@@ -14,20 +14,23 @@
 // 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.
-
 use crate::{
 	evm::block_hash::{AccumulateReceipt, EthereumBlockBuilder, LogsBloom},
 	limits,
 	sp_runtime::traits::One,
-	BlockHash, Config, EthBlockBuilderIR, EthereumBlock, ReceiptInfoData, UniqueSaturatedInto,
-	H160, H256,
+	weights::WeightInfo,
+	BlockHash, Config, EthBlockBuilderIR, EthereumBlock, Event, Pallet, ReceiptInfoData,
+	UniqueSaturatedInto, H160, H256,
 };
-
-use frame_support::weights::Weight;
-pub use sp_core::U256;
-
 use alloc::vec::Vec;
 use environmental::environmental;
+use frame_support::{
+	pallet_prelude::{DispatchError, DispatchResultWithPostInfo},
+	storage::with_transaction,
+	weights::Weight,
+};
+use sp_core::U256;
+use sp_runtime::TransactionOutcome;
 
 /// The maximum number of block hashes to keep in the history.
 ///
@@ -60,12 +63,62 @@ pub fn get_receipt_details() -> Option<(Vec<u8>, LogsBloom)> {
 }
 
 /// Capture the receipt events emitted from the current ethereum
-/// transaction. The transaction must be signed by an eth-compatible
-/// wallet.
-pub fn with_ethereum_context<R>(f: impl FnOnce() -> R) -> R {
+#[cfg(feature = "runtime-benchmarks")]
+pub fn bench_with_ethereum_context<R>(f: impl FnOnce() -> R) -> R {
 	receipt::using(&mut AccumulateReceipt::new(), f)
 }
 
+/// Execute the Ethereum call, and write the block storage transaction details.
+///
+/// # Parameters
+/// - transaction_encoded: The RLP encoded transaction bytes.
+/// - call: A closure that executes the transaction logic and returns the gas consumed and result.
+pub fn with_ethereum_context<T: Config>(
+	transaction_encoded: Vec<u8>,
+	call: impl FnOnce() -> (Weight, DispatchResultWithPostInfo),
+) -> DispatchResultWithPostInfo {
+	receipt::using(&mut AccumulateReceipt::new(), || {
+		let (err, gas_consumed, mut post_info) =
+			with_transaction(|| -> TransactionOutcome<Result<_, DispatchError>> {
+				let (gas_consumed, result) = call();
+				match result {
+					Ok(post_info) =>
+						TransactionOutcome::Commit(Ok((None, gas_consumed, post_info))),
+					Err(err) => TransactionOutcome::Rollback(Ok((
+						Some(err.error),
+						gas_consumed,
+						err.post_info,
+					))),
+				}
+			})?;
+
+		if let Some(dispatch_error) = err {
+			deposit_eth_extrinsic_revert_event::<T>(dispatch_error);
+			crate::block_storage::process_transaction::<T>(
+				transaction_encoded,
+				false,
+				gas_consumed,
+			);
+			Ok(post_info)
+		} else {
+			// deposit a dummy event in benchmark mode
+			#[cfg(feature = "runtime-benchmarks")]
+			deposit_eth_extrinsic_revert_event::<T>(crate::Error::<T>::BenchmarkingError.into());
+
+			crate::block_storage::process_transaction::<T>(transaction_encoded, true, gas_consumed);
+			post_info
+				.actual_weight
+				.as_mut()
+				.map(|w| w.saturating_reduce(T::WeightInfo::deposit_eth_extrinsic_revert_event()));
+			Ok(post_info)
+		}
+	})
+}
+
+fn deposit_eth_extrinsic_revert_event<T: Config>(dispatch_error: DispatchError) {
+	Pallet::<T>::deposit_event(Event::<T>::EthExtrinsicRevert { dispatch_error });
+}
+
 /// Clear the storage used to capture the block hash related data.
 pub fn on_initialize<T: Config>() {
 	ReceiptInfoData::<T>::kill();

substrate/frame/revive/src/lib.rs

@@ -433,6 +433,14 @@ pub mod pallet {
 
 		/// Contract deployed by deployer at the specified address.
 		Instantiated { deployer: H160, contract: H160 },
+
+		/// Emitted when an Ethereum transaction reverts.
+		///
+		/// Ethereum transactions always complete successfully at the extrinsic level,
+		/// as even reverted calls must store their `ReceiptInfo`.
+		/// To distinguish reverted calls from successful ones, this event is emitted
+		/// for failed Ethereum transactions.
+		EthExtrinsicRevert { dispatch_error: DispatchError },
 	}
 
 	#[pallet::error]
@@ -554,6 +562,10 @@ pub mod pallet {
 		///
 		/// This happens if the passed `gas` inside the ethereum transaction is too low.
 		TxFeeOverdraw = 0x35,
+
+		/// Benchmarking only error.
+		#[cfg(feature = "runtime-benchmarks")]
+		BenchmarkingError = 0xFF,
 	}
 
 	/// A reason for the pallet revive placing a hold on funds.
@@ -1194,6 +1206,7 @@ pub mod pallet {
 		#[pallet::call_index(10)]
 		#[pallet::weight(
 			<T as Config>::WeightInfo::eth_instantiate_with_code(code.len() as u32, data.len() as u32, Pallet::<T>::has_dust(*value).into())
+			.saturating_add(T::WeightInfo::on_finalize_block_per_tx(transaction_encoded.len() as u32))
 			.saturating_add(*gas_limit)
 		)]
 		pub fn eth_instantiate_with_code(
@@ -1221,7 +1234,7 @@ pub mod pallet {
 			let base_info = T::FeeInfo::base_dispatch_info(&mut call);
 			drop(call);
 
-			block_storage::with_ethereum_context(|| {
+			block_storage::with_ethereum_context::<T>(transaction_encoded, || {
 				let mut output = Self::bare_instantiate(
 					origin,
 					value,
@@ -1243,18 +1256,13 @@ pub mod pallet {
 					}
 				}
 
-				block_storage::process_transaction::<T>(
-					transaction_encoded,
-					output.result.is_ok(),
-					output.gas_consumed,
-				);
-
 				let result = dispatch_result(
 					output.result.map(|result| result.result),
 					output.gas_consumed,
 					base_info.call_weight,
 				);
-				T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result)
+				let result = T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result);
+				(output.gas_consumed, result)
 			})
 		}
 
@@ -1291,7 +1299,7 @@ pub mod pallet {
 			let base_info = T::FeeInfo::base_dispatch_info(&mut call);
 			drop(call);
 
-			block_storage::with_ethereum_context(|| {
+			block_storage::with_ethereum_context::<T>(transaction_encoded, || {
 				let mut output = Self::bare_call(
 					origin,
 					dest,
@@ -1312,22 +1320,10 @@ pub mod pallet {
 					}
 				}
 
-				let encoded_length = transaction_encoded.len() as u32;
-
-				block_storage::process_transaction::<T>(
-					transaction_encoded,
-					output.result.is_ok(),
-					output.gas_consumed,
-				);
-
-				let result = dispatch_result(
-					output.result,
-					output.gas_consumed,
-					base_info
-						.call_weight
-						.saturating_add(T::WeightInfo::on_finalize_block_per_tx(encoded_length)),
-				);
-				T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result)
+				let result =
+					dispatch_result(output.result, output.gas_consumed, base_info.call_weight);
+				let result = T::FeeInfo::ensure_not_overdrawn(encoded_len, &info, result);
+				(output.gas_consumed, result)
 			})
 		}