revive: Precompiles should return dummy code when queried

.github/workflows/tests-evm.yml

@@ -37,13 +37,13 @@ jobs:
       - name: script
         run: |
           forklift cargo build --locked --profile production -p pallet-revive-eth-rpc --bin eth-rpc
-          forklift cargo build --bin substrate-node
+          forklift cargo build -p staging-node-cli --bin substrate-node
 
       - name: Checkout evm-tests
         uses: actions/checkout@v4
         with:
           repository: paritytech/evm-test-suite
-          ref: 6ffa85740a82d9cdb90629f9e3dbca5dc175c5e9
+          ref: 72d1dace20c8fea4c2404383cc422299b26f1961
           path: evm-test-suite
 
       - uses: actions/setup-node@v4
@@ -58,7 +58,7 @@ jobs:
           echo "Change to the evm-test-suite directory"
           cd evm-test-suite
           echo "Download the resolc binary"
-          wget https://github.com/paritytech/revive/releases/download/v0.1.0-dev.9/resolc -q
+          wget -O resolc https://github.com/paritytech/revive/releases/download/v0.2.0/resolc-x86_64-unknown-linux-musl -q
           chmod +x resolc
           mv resolc /usr/local/bin
           resolc --version
@@ -74,12 +74,14 @@ jobs:
           # cat matter-labs-tests/hardhat.config.ts | grep batchSize
 
           echo "Installing solc"
-          wget https://github.com/ethereum/solidity/releases/download/v0.8.28/solc-static-linux -q
+          wget https://github.com/ethereum/solidity/releases/download/v0.8.30/solc-static-linux -q
           chmod +x solc-static-linux
           mv solc-static-linux /usr/local/bin/solc
           echo "Run the tests"
           echo "bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH"
           bash init.sh --kitchensink -- --matter-labs -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH
+          echo "Run eth-rpc tests"
+          bash init.sh --kitchensink http://localhost:9944 --eth-rpc -- $NODE_BIN_PATH $ETH_RPC_PATH $RESOLC_PATH
 
       - name: Collect tests results
         if: always()

prdoc/pr_9001.prdoc

@@ -0,0 +1,30 @@
+title: 'revive: Precompiles should return dummy code when queried'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Fixes https://github.com/paritytech/contract-issues/issues/111
+
+    This fixes both the RPC and the opcodes `EXTCODESIZE` and `EXTCODEHASH`.
+
+    Also removed the disabled host function `is_contract`. Contracts do use `EXTCODESIZE` to determine if something is a contract exclusively.
+
+    Need to add some differential tests to our test suite to make sure that the RPC matches geth behaviour:
+
+    On kitchensink:
+
+    ```shell
+    # primitive precompiles should not return error but 0x
+    $ cast code 0x0000000000000000000000000000000000000001
+    0x
+
+    # this is the erc pre-compile
+    $ cast code 0x0000000000000000000000000000000000010000
+    0x60006000fd
+    ```
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-uapi
+  bump: major
+- name: pallet-revive-eth-rpc
+  bump: major

substrate/frame/revive/rpc/src/client/runtime_api.rs

@@ -154,4 +154,11 @@ impl RuntimeApi {
 		let trace = self.0.call(payload).await?.map_err(|err| ClientError::TransactError(err.0))?;
 		Ok(trace.0)
 	}
+
+	/// Get the code of the given address.
+	pub async fn code(&self, address: H160) -> Result<Vec<u8>, ClientError> {
+		let payload = subxt_client::apis().revive_api().code(address);
+		let code = self.0.call(payload).await?;
+		Ok(code)
+	}
 }

substrate/frame/revive/rpc/src/client/storage_api.rs

@@ -47,17 +47,6 @@ impl StorageApi {
 		Ok(info)
 	}
 
-	/// Get the contract code for the given contract address.
-	pub async fn get_contract_code(
-		&self,
-		contract_address: &H160,
-	) -> Result<Option<Vec<u8>>, ClientError> {
-		let ContractInfo { code_hash, .. } = self.get_contract_info(contract_address).await?;
-		let query = subxt_client::storage().revive().pristine_code(code_hash);
-		let result = self.0.fetch(&query).await?.map(|v| v.0);
-		Ok(result)
-	}
-
 	/// Get the contract trie id for the given contract address.
 	pub async fn get_contract_trie_id(&self, address: &H160) -> Result<Vec<u8>, ClientError> {
 		let ContractInfo { trie_id, .. } = self.get_contract_info(address).await?;

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

@@ -246,8 +246,8 @@ impl EthRpcServer for EthRpcServerImpl {
 
 	async fn get_code(&self, address: H160, block: BlockNumberOrTagOrHash) -> RpcResult<Bytes> {
 		let hash = self.client.block_hash_for_tag(block).await?;
-		let code = self.client.storage_api(hash).get_contract_code(&address).await?;
-		Ok(code.unwrap_or_default().into())
+		let code = self.client.runtime_api(hash).code(address).await?;
+		Ok(code.into())
 	}
 
 	async fn accounts(&self) -> RpcResult<Vec<H160>> {

substrate/frame/revive/src/benchmarking.rs

@@ -445,22 +445,6 @@ mod benchmarks {
 		);
 	}
 
-	#[benchmark(pov_mode = Measured)]
-	fn seal_is_contract() {
-		let Contract { account_id, .. } =
-			Contract::<T>::with_index(1, VmBinaryModule::dummy(), vec![]).unwrap();
-
-		build_runtime!(runtime, memory: [account_id.encode(), ]);
-
-		let result;
-		#[block]
-		{
-			result = runtime.bench_is_contract(memory.as_mut_slice(), 0);
-		}
-
-		assert_eq!(result.unwrap(), 1);
-	}
-
 	#[benchmark(pov_mode = Measured)]
 	fn seal_to_account_id() {
 		// use a mapped address for the benchmark, to ensure that we bench the worst

substrate/frame/revive/src/exec.rs

@@ -323,9 +323,6 @@ pub trait PrecompileExt: sealing::Sealed {
 	/// Return the origin of the whole call stack.
 	fn origin(&self) -> &Origin<Self::T>;
 
-	/// Check if a contract lives at the specified `address`.
-	fn is_contract(&self, address: &H160) -> bool;
-
 	/// Returns the account id for the given `address`.
 	fn to_account_id(&self, address: &H160) -> AccountIdOf<Self::T>;
 
@@ -337,9 +334,6 @@ pub trait PrecompileExt: sealing::Sealed {
 	fn code_size(&self, address: &H160) -> u64;
 
 	/// Check if the caller of the current contract is the origin of the whole call stack.
-	///
-	/// This can be checked with `is_contract(self.caller())` as well.
-	/// However, this function does not require any storage lookup and therefore uses less weight.
 	fn caller_is_origin(&self) -> bool;
 
 	/// Check if the caller is origin, and this origin is root.
@@ -1854,15 +1848,14 @@ where
 		&self.origin
 	}
 
-	fn is_contract(&self, address: &H160) -> bool {
-		ContractInfoOf::<T>::contains_key(&address)
-	}
-
 	fn to_account_id(&self, address: &H160) -> T::AccountId {
 		T::AddressMapper::to_account_id(address)
 	}
 
 	fn code_hash(&self, address: &H160) -> H256 {
+		if let Some(code) = <AllPrecompiles<T>>::code(address.as_fixed_bytes()) {
+			return sp_io::hashing::keccak_256(code).into()
+		}
 		<ContractInfoOf<T>>::get(&address)
 			.map(|contract| contract.code_hash)
 			.unwrap_or_else(|| {
@@ -1874,6 +1867,9 @@ where
 	}
 
 	fn code_size(&self, address: &H160) -> u64 {
+		if let Some(code) = <AllPrecompiles<T>>::code(address.as_fixed_bytes()) {
+			return code.len() as u64
+		}
 		<ContractInfoOf<T>>::get(&address)
 			.and_then(|contract| CodeInfoOf::<T>::get(contract.code_hash))
 			.map(|info| info.code_len())

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

@@ -782,34 +782,6 @@ fn origin_returns_proper_values() {
 	assert_eq!(WitnessedCallerCharlie::get(), Some(ALICE_ADDR));
 }
 
-#[test]
-fn is_contract_returns_proper_values() {
-	let bob_ch = MockLoader::insert(Call, |ctx, _| {
-		// Verify that BOB is a contract
-		assert!(ctx.ext.is_contract(&BOB_ADDR));
-		// Verify that ALICE is not a contract
-		assert!(!ctx.ext.is_contract(&ALICE_ADDR));
-		exec_success()
-	});
-
-	ExtBuilder::default().build().execute_with(|| {
-		place_contract(&BOB, bob_ch);
-
-		let origin = Origin::from_account_id(ALICE);
-		let mut storage_meter = storage::meter::Meter::new(0);
-		let result = MockStack::run_call(
-			origin,
-			BOB_ADDR,
-			&mut GasMeter::<Test>::new(GAS_LIMIT),
-			&mut storage_meter,
-			U256::zero(),
-			vec![],
-			false,
-		);
-		assert_matches!(result, Ok(_));
-	});
-}
-
 #[test]
 fn to_account_id_returns_proper_values() {
 	let bob_code_hash = MockLoader::insert(Call, |ctx, _| {

substrate/frame/revive/src/lib.rs

@@ -1603,6 +1603,20 @@ impl<T: Config> Pallet<T> {
 		let account_id = T::FindAuthor::find_author(pre_runtime_digests)?;
 		Some(T::AddressMapper::to_address(&account_id))
 	}
+
+	/// Returns the code at `address`.
+	///
+	/// This takes pre-compiles into account.
+	pub fn code(address: &H160) -> Vec<u8> {
+		use precompiles::{All, Precompiles};
+		if let Some(code) = <All<T>>::code(address.as_fixed_bytes()) {
+			return code.into()
+		}
+		<ContractInfoOf<T>>::get(&address)
+			.and_then(|contract| <PristineCode<T>>::get(contract.code_hash))
+			.map(|code| code.into())
+			.unwrap_or_default()
+	}
 }
 
 /// The address used to call the runtime's pallets dispatchables
@@ -1729,6 +1743,9 @@ sp_api::decl_runtime_apis! {
 
 		/// The address used to call the runtime's pallets dispatchables
 		fn runtime_pallets_address() -> H160;
+
+		/// The code at the specified address taking pre-compiles into account.
+		fn code(address: H160) -> Vec<u8>;
 	}
 }
 
@@ -1947,6 +1964,9 @@ macro_rules! impl_runtime_apis_plus_revive {
 					$crate::RUNTIME_PALLETS_ADDR
 				}
 
+				fn code(address: $crate::H160) -> Vec<u8> {
+					$crate::Pallet::<Self>::code(&address)
+				}
 			}
 		}
 	};

substrate/frame/revive/src/precompiles.rs

@@ -54,6 +54,9 @@ pub(crate) use builtin::{IBenchmarking, NoInfo as BenchmarkNoInfo, WithInfo as B
 
 const UNIMPLEMENTED: &str = "A precompile must either implement `call` or `call_with_info`";
 
+/// A minimal EVM bytecode to be returned when a pre-compile is queried for its code.
+pub(crate) const EVM_REVERT: [u8; 5] = sp_core::hex2array!("60006000fd");
+
 /// The composition of all available pre-compiles.
 ///
 /// This is how the rest of the pallet discovers and calls pre-compiles.
@@ -227,6 +230,7 @@ pub(crate) trait BuiltinPrecompile {
 	type Interface: SolInterface;
 	const MATCHER: BuiltinAddressMatcher;
 	const HAS_CONTRACT_INFO: bool;
+	const CODE: &[u8] = &EVM_REVERT;
 
 	fn call(
 		_address: &[u8; 20],
@@ -245,7 +249,7 @@ pub(crate) trait BuiltinPrecompile {
 	}
 }
 
-/// A low level pre-compile that does use Solidity ABI.
+/// A low level pre-compile that does not use Solidity ABI.
 ///
 /// It is used to implement the original Ethereum pre-compiles which do not
 /// use Solidity ABI but just encode inputs and outputs packed in memory.
@@ -256,6 +260,7 @@ pub(crate) trait PrimitivePrecompile {
 	type T: Config;
 	const MATCHER: BuiltinAddressMatcher;
 	const HAS_CONTRACT_INFO: bool;
+	const CODE: &[u8] = &[];
 
 	fn call(
 		_address: &[u8; 20],
@@ -315,6 +320,13 @@ pub(crate) trait Precompiles<T: Config> {
 	/// range by accident.
 	const USES_EXTERNAL_RANGE: bool;
 
+	/// Returns the code of the pre-compile.
+	///
+	/// Just used when queried by `EXTCODESIZE` or the RPC. It is just
+	/// a bogus code that is never executed. Returns None if no pre-compile
+	/// exists at the specified address.
+	fn code(address: &[u8; 20]) -> Option<&'static [u8]>;
+
 	/// Get a reference to a specific pre-compile.
 	///
 	/// Returns `None` if no pre-compile exists at `address`.
@@ -348,6 +360,7 @@ impl<P: BuiltinPrecompile> PrimitivePrecompile for P {
 	type T = <Self as BuiltinPrecompile>::T;
 	const MATCHER: BuiltinAddressMatcher = P::MATCHER;
 	const HAS_CONTRACT_INFO: bool = P::HAS_CONTRACT_INFO;
+	const CODE: &[u8] = P::CODE;
 
 	fn call(
 		address: &[u8; 20],
@@ -398,24 +411,40 @@ impl<T: Config> Precompiles<T> for Tuple {
 		uses_external
 	};
 
+	fn code(address: &[u8; 20]) -> Option<&'static [u8]> {
+		for_tuples!(
+			#(
+				if Tuple::MATCHER.matches(address) {
+					return Some(Tuple::CODE)
+				}
+			)*
+		);
+		None
+	}
+
 	fn get<E: ExtWithInfo<T = T>>(address: &[u8; 20]) -> Option<Instance<E>> {
 		let _ = <Self as Precompiles<T>>::CHECK_COLLISION;
-		let mut has_contract_info = false;
-		let mut function: Option<fn(&[u8; 20], Vec<u8>, &mut E) -> Result<Vec<u8>, Error>> = None;
+		let mut instance: Option<Instance<E>> = None;
 		for_tuples!(
 			#(
 				if Tuple::MATCHER.matches(address) {
 					if Tuple::HAS_CONTRACT_INFO {
-						has_contract_info = true;
-						function = Some(Tuple::call_with_info);
+						instance = Some(Instance {
+							address: *address,
+							has_contract_info: true,
+							function: Tuple::call_with_info,
+						})
 					} else {
-						has_contract_info = false;
-						function = Some(Tuple::call);
+						instance = Some(Instance {
+							address: *address,
+							has_contract_info: false,
+							function: Tuple::call,
+						})
 					}
 				}
 			)*
 		);
-		function.map(|function| Instance { has_contract_info, address: *address, function })
+		instance
 	}
 }
 
@@ -428,6 +457,10 @@ impl<T: Config> Precompiles<T> for (Builtin<T>, <T as Config>::Precompiles) {
 	};
 	const USES_EXTERNAL_RANGE: bool = { <T as Config>::Precompiles::USES_EXTERNAL_RANGE };
 
+	fn code(address: &[u8; 20]) -> Option<&'static [u8]> {
+		<Builtin<T>>::code(address).or_else(|| <T as Config>::Precompiles::code(address))
+	}
+
 	fn get<E: ExtWithInfo<T = T>>(address: &[u8; 20]) -> Option<Instance<E>> {
 		let _ = <Self as Precompiles<T>>::CHECK_COLLISION;
 		<Builtin<T>>::get(address).or_else(|| <T as Config>::Precompiles::get(address))

substrate/frame/revive/src/precompiles/builtin.rs

@@ -64,6 +64,10 @@ impl<T: Config> Precompiles<T> for (Production<T>, Benchmarking<T>) {
 	const USES_EXTERNAL_RANGE: bool =
 		Production::<T>::USES_EXTERNAL_RANGE || Benchmarking::<T>::USES_EXTERNAL_RANGE;
 
+	fn code(address: &[u8; 20]) -> Option<&'static [u8]> {
+		<Production<T>>::code(address).or_else(|| Benchmarking::<T>::code(address))
+	}
+
 	fn get<E: ExtWithInfo<T = T>>(address: &[u8; 20]) -> Option<Instance<E>> {
 		let _ = <Self as Precompiles<T>>::CHECK_COLLISION;
 		<Production<T>>::get(address).or_else(|| Benchmarking::<T>::get(address))