[pallet-revive] tracing improvements (#7614)

Various pallet-revive improvements

- add check for precompiles addresses,
So we can easily identify which one are being called and not supported
yet

- fixes debug_call for revert call
If a call revert we still want to get the traces for that call, that
matches geth behaviors, diff tests will be added to the test suite for
this

- fixes traces for staticcall
The call type was not always being reported properly.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>

Author: 3 people

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -2368,11 +2368,15 @@ impl_runtime_apis! {
 		{
 			use pallet_revive::tracing::trace;
 			let mut tracer = config.build(Revive::evm_gas_from_weight);
-			trace(&mut tracer, || {
-				Self::eth_transact(tx)
-			})?;
-
-			Ok(tracer.collect_traces().pop().expect("eth_transact succeeded, trace must exist, qed"))
+			let result = trace(&mut tracer, || Self::eth_transact(tx));
+
+			if let Some(trace) = tracer.collect_traces().pop() {
+				Ok(trace)
+			} else if let Err(err) = result {
+				Err(err)
+			} else {
+				Ok(Default::default())
+			}
 		}
 	}
 }

prdoc/pr_7614.prdoc

@@ -0,0 +1,21 @@
+title: '[pallet-revive] tracing improvements'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Various pallet-revive improvements
+
+    - add check for precompiles addresses,
+    So we can easily identified which one are being called and not supported yet
+
+    - fixes debug_call for revert call
+    If a call revert we still want to get the traces for that call, that matches geth behaviors, diff tests will be added to the test suite for this
+
+    - fixes traces for staticcall
+    The call type was not always being reported properly.
+crates:
+- name: asset-hub-westend-runtime
+  bump: minor
+- name: pallet-revive-eth-rpc
+  bump: minor
+- name: pallet-revive
+  bump: minor

substrate/bin/node/runtime/src/lib.rs

@@ -3744,11 +3744,15 @@ impl_runtime_apis! {
 		{
 			use pallet_revive::tracing::trace;
 			let mut tracer = config.build(Revive::evm_gas_from_weight);
-			trace(&mut tracer, || {
-				Self::eth_transact(tx)
-			})?;
-
-			Ok(tracer.collect_traces().pop().expect("eth_transact succeeded, trace must exist, qed"))
+			let result = trace(&mut tracer, || Self::eth_transact(tx));
+
+			if let Some(trace) = tracer.collect_traces().pop() {
+				Ok(trace)
+			} else if let Err(err) = result {
+				Err(err)
+			} else {
+				Ok(Default::default())
+			}
 		}
 	}
 

substrate/frame/revive/rpc/revive_chain.metadata

@@ -181,7 +181,8 @@ pub struct CallTrace<Gas = U256> {
 	#[serde(skip_serializing_if = "Vec::is_empty")]
 	pub logs: Vec<CallLog>,
 	/// Amount of value transferred.
-	pub value: U256,
+	#[serde(skip_serializing_if = "Option::is_none")]
+	pub value: Option<U256>,
 	/// Type of call.
 	#[serde(rename = "type")]
 	pub call_type: CallType,

substrate/frame/revive/src/evm/api/debug_rpc_types.rs

@@ -70,7 +70,7 @@ impl<Gas: Default, GasMapper: Fn(Weight) -> Gas> Tracer for CallTracer<Gas, GasM
 		self.traces.push(CallTrace {
 			from,
 			to,
-			value,
+			value: if is_read_only { None } else { Some(value) },
 			call_type,
 			input: input.to_vec().into(),
 			gas: (self.gas_mapper)(gas_left),

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

@@ -1373,6 +1373,13 @@ where
 	}
 }
 
+/// Determine if the given address is a precompile.
+/// For now, we consider that all addresses between 0x1 and 0xff are reserved for precompiles.
+fn is_precompile(address: &H160) -> bool {
+	let bytes = address.as_bytes();
+	bytes.starts_with(&[0u8; 19]) && bytes[19] != 0
+}
+
 impl<'a, T, E> Ext for Stack<'a, T, E>
 where
 	T: Config,
@@ -1403,6 +1410,11 @@ where
 		*self.last_frame_output_mut() = Default::default();
 
 		let try_call = || {
+			if is_precompile(dest_addr) {
+				log::debug!(target: crate::LOG_TARGET, "Unsupported precompile address {dest_addr:?}");
+				return Err(Error::<T>::UnsupportedPrecompileAddress.into());
+			}
+
 			let dest = T::AddressMapper::to_account_id(dest_addr);
 			if !self.allows_reentry(&dest) {
 				return Err(<Error<T>>::ReentranceDenied.into());
@@ -1420,28 +1432,38 @@ where
 					CachedContract::Cached(contract) => Some(contract.clone()),
 					_ => None,
 				});
+
+			// Enable read-only access if requested; cannot disable it if already set.
+			let is_read_only = read_only || self.is_read_only();
+
 			if let Some(executable) = self.push_frame(
 				FrameArgs::Call { dest: dest.clone(), cached_info, delegated_call: None },
 				value,
 				gas_limit,
 				deposit_limit.saturated_into::<BalanceOf<T>>(),
-				// Enable read-only access if requested; cannot disable it if already set.
-				read_only || self.is_read_only(),
+				is_read_only,
 			)? {
 				self.run(executable, input_data)
 			} else {
-				let result = Self::transfer_from_origin(
-					&self.origin,
-					&Origin::from_account_id(self.account_id().clone()),
-					&dest,
-					value,
-				);
+				let result = if is_read_only && value.is_zero() {
+					Ok(Default::default())
+				} else if is_read_only {
+					Err(Error::<T>::StateChangeDenied.into())
+				} else {
+					Self::transfer_from_origin(
+						&self.origin,
+						&Origin::from_account_id(self.account_id().clone()),
+						&dest,
+						value,
+					)
+				};
+
 				if_tracing(|t| {
 					t.enter_child_span(
 						T::AddressMapper::to_address(self.account_id()),
 						T::AddressMapper::to_address(&dest),
 						false,
-						false,
+						is_read_only,
 						value,
 						&input_data,
 						Weight::zero(),

substrate/frame/revive/src/exec.rs

@@ -492,6 +492,8 @@ pub mod pallet {
 		InvalidGenericTransaction,
 		/// The refcount of a code either over or underflowed.
 		RefcountOverOrUnderflow,
+		/// Unsupported precompile address
+		UnsupportedPrecompileAddress,
 	}
 
 	/// A reason for the pallet contracts placing a hold on funds.

substrate/frame/revive/src/lib.rs

@@ -146,7 +146,10 @@ builder!(
 
 	/// Build the instantiate call and unwrap the account id.
 	pub fn build_and_unwrap_contract(self) -> Contract<T> {
-		let addr = self.build().result.unwrap().addr;
+		let result = self.build().result.unwrap();
+		assert!(!result.result.did_revert(), "instantiation did revert");
+
+		let addr = result.addr;
 		let account_id = T::AddressMapper::to_account_id(&addr);
 		Contract{ account_id,  addr }
 	}

substrate/frame/revive/src/test_utils/builder.rs

@@ -4441,7 +4441,7 @@ fn tracing_works_for_transfers() {
 			vec![CallTrace {
 				from: ALICE_ADDR,
 				to: BOB_ADDR,
-				value: U256::from(10_000_000),
+				value: Some(U256::from(10_000_000)),
 				call_type: CallType::Call,
 				..Default::default()
 			},]
@@ -4509,6 +4509,7 @@ fn tracing_works() {
 					input: (3u32, addr_callee).encode().into(),
 					call_type: Call,
 					logs: logs.clone(),
+					value: Some(U256::from(0)),
 					calls: vec![
 						CallTrace {
 							from: addr,
@@ -4520,6 +4521,7 @@ fn tracing_works() {
 							revert_reason: Some("revert: This function always fails".to_string()),
 							error: Some("execution reverted".to_string()),
 							call_type: Call,
+							value: Some(U256::from(0)),
 							..Default::default()
 						},
 						CallTrace {
@@ -4528,6 +4530,7 @@ fn tracing_works() {
 							input: (2u32, addr_callee).encode().into(),
 							call_type: Call,
 							logs: logs.clone(),
+							value: Some(U256::from(0)),
 							calls: vec![
 								CallTrace {
 									from: addr,
@@ -4536,6 +4539,7 @@ fn tracing_works() {
 									output: Default::default(),
 									error: Some("ContractTrapped".to_string()),
 									call_type: Call,
+									value: Some(U256::from(0)),
 									..Default::default()
 								},
 								CallTrace {
@@ -4544,25 +4548,28 @@ fn tracing_works() {
 									input: (1u32, addr_callee).encode().into(),
 									call_type: Call,
 									logs: logs.clone(),
+									value: Some(U256::from(0)),
 									calls: vec![
 										CallTrace {
 											from: addr,
 											to: addr_callee,
 											input: 0u32.encode().into(),
 											output: 0u32.to_le_bytes().to_vec().into(),
 											call_type: Call,
+											value: Some(U256::from(0)),
 											..Default::default()
 										},
 										CallTrace {
 											from: addr,
 											to: addr,
 											input: (0u32, addr_callee).encode().into(),
 											call_type: Call,
+											value: Some(U256::from(0)),
 											calls: vec![
 												CallTrace {
 													from: addr,
 													to: BOB_ADDR,
-													value: U256::from(100),
+													value: Some(U256::from(100)),
 													call_type: CallType::Call,
 													..Default::default()
 												}
@@ -4582,3 +4589,41 @@ fn tracing_works() {
 		}
 	});
 }
+
+#[test]
+fn unknown_precompiles_revert() {
+	let (code, _code_hash) = compile_module("read_only_call").unwrap();
+
+	ExtBuilder::default().build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
+		let Contract { addr, .. } =
+			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+
+		let cases: Vec<(H160, Box<dyn FnOnce(_)>)> = vec![
+			(
+				H160::from_low_u64_be(0x1),
+				Box::new(|result| {
+					assert_err!(result, <Error<Test>>::UnsupportedPrecompileAddress);
+				}),
+			),
+			(
+				H160::from_low_u64_be(0xff),
+				Box::new(|result| {
+					assert_err!(result, <Error<Test>>::UnsupportedPrecompileAddress);
+				}),
+			),
+			(
+				H160::from_low_u64_be(0x1ff),
+				Box::new(|result| {
+					assert_ok!(result);
+				}),
+			),
+		];
+
+		for (callee_addr, assert_result) in cases {
+			let result =
+				builder::bare_call(addr).data((callee_addr, [0u8; 0]).encode()).build().result;
+			assert_result(result);
+		}
+	});
+}