[pallet-revive] fix empty transactions array in eth_getBlockBy*

prdoc/pr_12243.prdoc

@@ -0,0 +1,50 @@
+title: "revive eth-rpc: recover transactions for pre-upgrade blocks in eth_getBlockBy* (#6790)"
+doc:
+- audience: Node Operator
+  description: |-
+    For blocks whose `EthereumBlock` runtime storage is unset (produced before
+    pallet-revive shipped the ETH-block storage value — runtime API returns
+    `EthBlock::default()`, identified by `eth_block.hash.is_zero()`),
+    `eth_getBlockByNumber` / `eth_getBlockByHash` previously returned an empty
+    `transactions` array even though the block had ETH transactions.
+
+    `evm_block` now walks the substrate block's extrinsics for those blocks. Each
+    `EthTransact` extrinsic carries the RLP-encoded transaction payload, from
+    which we can derive:
+    * The Ethereum tx hash (`keccak256(payload)`) — recovers the non-hydrated
+      `Hashes(...)` array.
+    * The full `TransactionInfo` (RLP-decode → `recover_eth_address()` for `from`,
+      plus block number and substrate extrinsic index) — recovers the hydrated
+      `TransactionInfos(...)` array.
+
+    Recovery is independent of `EthereumBlock`/`ReceiptInfoData` and of the eth-rpc
+    sqlite cache, all of which lack data for pre-upgrade blocks under any current
+    writer.
+
+    Known limitations of recovered pre-upgrade blocks:
+    * `block_hash` on inner `TransactionInfo`s stays `H256::zero()` — the
+      pre-upgrade header has no real ethereum hash to stamp.
+    * Other top-level header fields (`parent_hash`, `state_root`, `gas_limit`,
+      `timestamp`, `miner`, etc.) remain default. The response is internally
+      inconsistent but at least the `transactions` array is recovered.
+    * `transactionIndex` on inner objects is the substrate extrinsic index
+      (typically `2..` after the timestamp + parachain-system inherents), not the
+      0-based EVM index. This is pre-existing behaviour (#6790 point 3) — not
+      introduced or fixed here.
+    * `eth_getTransactionReceipt` for transactions in pre-upgrade blocks remains
+      bugged (would need gas-usage data from `ReceiptInfoData`). Separate follow-up.
+
+    Observable API changes:
+    * `ReceiptProvider::block_transaction_hashes` return type changed from
+      `Option<HashMap<usize, H256>>` to `HashMap<usize, H256>`. DB errors are now
+      logged and collapsed into an empty map instead of `None`; callers that
+      previously distinguished a DB error from "block has no ETH transactions"
+      no longer can.
+    * `eth_debug_traceBlockByNumber` (`Client::trace_block_by_number`) now returns
+      `EthExtrinsicNotFound` when the runtime emits traces but the receipt
+      provider has no rows for the block. Previously this silently produced an
+      empty list.
+
+crates:
+- name: pallet-revive-eth-rpc
+  bump: minor

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

@@ -38,6 +38,7 @@ use pallet_revive::{
 	},
 };
 use runtime_api::RuntimeApi;
+use sp_crypto_hashing::keccak_256;
 use sp_runtime::traits::Block as BlockT;
 use sp_weights::Weight;
 use std::{
@@ -1003,11 +1004,10 @@ impl Client {
 		let runtime_api = RuntimeApi::new(self.api.runtime_api().at(parent_hash));
 		let traces = runtime_api.trace_block(block, config.clone()).await?;
 
-		let mut hashes = self
-			.receipt_provider
-			.block_transaction_hashes(&block_hash)
-			.await
-			.ok_or(ClientError::EthExtrinsicNotFound)?;
+		let mut hashes = self.receipt_provider.block_transaction_hashes(&block_hash).await;
+		if !traces.is_empty() && hashes.is_empty() {
+			return Err(ClientError::EthExtrinsicNotFound);
+		}
 
 		let traces = traces.into_iter().filter_map(|(index, trace)| {
 			Some(TransactionTrace { tx_hash: hashes.remove(&(index as usize))?, trace })
@@ -1070,36 +1070,48 @@ impl Client {
 		//  - the block author cannot be obtained from the digest logs (highly unlikely)
 		//  - the node we are targeting has an outdated revive pallet (or ETH block functionality is
 		//    disabled)
-		match self.runtime_api(block.hash()).eth_block().await {
-			Ok(mut eth_block) => {
-				log::trace!(target: LOG_TARGET, "Ethereum block from runtime API hash {:?}", eth_block.hash);
-
-				if hydrated_transactions {
-					// Hydrate the block.
-					let tx_infos = self
-						.receipt_provider
-						.receipts_from_block(&block, eth_block.hash)
-						.await
-						.inspect_err(|err| {
-							log::trace!(target: LOG_TARGET,
-								"Failed to extract receipts for block #{}: {err:?}",
-								block.number());
-						})
-						.unwrap_or_default()
-						.into_iter()
-						.map(|(signed_tx, receipt)| TransactionInfo::new(&receipt, signed_tx))
-						.collect::<Vec<_>>();
-
-					eth_block.transactions = HashesOrTransactionInfos::TransactionInfos(tx_infos);
-				}
-
-				Some(eth_block)
-			},
+		let mut eth_block = match self.runtime_api(block.hash()).eth_block().await {
+			Ok(b) => b,
 			Err(err) => {
 				log::error!(target: LOG_TARGET, "Failed to get Ethereum block for hash {:?}: {err:?}", block.hash());
-				None
+				return None;
 			},
+		};
+		log::trace!(target: LOG_TARGET, "Ethereum block from runtime API hash {:?}", eth_block.hash);
+
+		// Pre-upgrade blocks (`hash.is_zero()` ⇒ runtime returned `EthBlock::default()`)
+		// have no `EthereumBlock`/`ReceiptInfoData` storage and no sqlite mapping. Recover
+		// `transactions` by walking the substrate extrinsics directly. Inner `block_hash`
+		// stays zero. See #6790.
+		if hydrated_transactions {
+			let tx_infos = if eth_block.hash.is_zero() {
+				// `receipts_from_block` errors on these (no `ReceiptInfoData`) after
+				// issuing 3 RPCs: extrinsics(), events(), eth_receipt_data runtime API.
+				// Skipping it saves 2 (the extrinsics-walk fallback still does extrinsics()).
+				pre_upgrade_tx_infos_from_extrinsics(&block).await
+			} else {
+				self.receipt_provider
+					.receipts_from_block(&block, eth_block.hash)
+					.await
+					.inspect_err(|err| {
+						log::trace!(target: LOG_TARGET,
+							"Failed to extract receipts for block #{}: {err:?}",
+							block.number());
+					})
+					.unwrap_or_default()
+					.into_iter()
+					.map(|(signed_tx, receipt)| TransactionInfo::new(&receipt, signed_tx))
+					.collect::<Vec<_>>()
+			};
+			eth_block.transactions = HashesOrTransactionInfos::TransactionInfos(tx_infos);
+		} else if eth_block.hash.is_zero() {
+			let hashes = pre_upgrade_tx_hashes_from_extrinsics(&block).await;
+			if !hashes.is_empty() {
+				eth_block.transactions = HashesOrTransactionInfos::Hashes(hashes);
+			}
 		}
+
+		Some(eth_block)
 	}
 
 	/// Get the chain ID.
@@ -1176,3 +1188,165 @@ impl Client {
 fn to_hex(bytes: impl AsRef<[u8]>) -> String {
 	format!("0x{}", hex::encode(bytes.as_ref()))
 }
+
+/// Decode an `EthTransact` RLP payload into a `TransactionInfo` with `block_hash = 0`.
+/// Returns `None` on RLP-decode or `recover_eth_address` failure (both logged at WARN —
+/// they should be infallible for any payload the chain itself accepted).
+fn eth_transact_payload_to_info(
+	payload: &[u8],
+	block_number: U256,
+	transaction_index: usize,
+) -> Option<TransactionInfo> {
+	let signed_tx = TransactionSigned::decode(payload)
+		.inspect_err(|err| {
+			log::warn!(target: LOG_TARGET,
+				"Failed to RLP-decode EthTransact payload at tx_index {transaction_index}: \
+				 {err:?}");
+		})
+		.ok()?;
+	let from = signed_tx
+		.recover_eth_address()
+		.inspect_err(|_| {
+			log::warn!(target: LOG_TARGET,
+				"Failed to recover sender for EthTransact payload at tx_index {transaction_index}");
+		})
+		.ok()?;
+	Some(TransactionInfo {
+		block_hash: H256::zero(),
+		block_number,
+		from,
+		hash: H256(keccak_256(payload)),
+		transaction_index: U256::from(transaction_index),
+		transaction_signed: signed_tx,
+	})
+}
+
+/// Tx hash for every `EthTransact` call in `block`, in block order. Cannot fail
+/// per-entry; the hydrated counterpart may return fewer entries on decode failure.
+async fn pre_upgrade_tx_hashes_from_extrinsics(block: &SubstrateBlock) -> Vec<H256> {
+	let extrinsics = match block.extrinsics().await {
+		Ok(extrinsics) => extrinsics,
+		Err(err) => {
+			log::warn!(target: LOG_TARGET,
+				"Failed to fetch extrinsics for pre-upgrade block #{}: {err:?}",
+				block.number());
+			return Vec::new();
+		},
+	};
+
+	extrinsics
+		.iter()
+		.filter_map(|ext| {
+			ext.as_extrinsic::<EthTransact>()
+				.inspect_err(|err| {
+					log::debug!(target: LOG_TARGET,
+						"as_extrinsic::<EthTransact> failed (likely metadata/codec drift): \
+						 {err:?}");
+				})
+				.ok()
+				.flatten()
+		})
+		.map(|call| H256(keccak_256(&call.payload)))
+		.collect()
+}
+
+/// `TransactionInfo` for every `EthTransact` call in `block`, in block order. Drops
+/// payloads that fail to RLP-decode or whose sender can't be recovered (logged at
+/// WARN); the hash-only counterpart returns one entry per extrinsic regardless.
+async fn pre_upgrade_tx_infos_from_extrinsics(block: &SubstrateBlock) -> Vec<TransactionInfo> {
+	let extrinsics = match block.extrinsics().await {
+		Ok(extrinsics) => extrinsics,
+		Err(err) => {
+			log::warn!(target: LOG_TARGET,
+				"Failed to fetch extrinsics for pre-upgrade block #{}: {err:?}",
+				block.number());
+			return Vec::new();
+		},
+	};
+	let block_number = U256::from(block.number());
+
+	extrinsics
+		.iter()
+		.enumerate()
+		.filter_map(|(ext_idx, ext)| {
+			let call = ext
+				.as_extrinsic::<EthTransact>()
+				.inspect_err(|err| {
+					log::debug!(target: LOG_TARGET,
+						"as_extrinsic::<EthTransact> failed at ext_idx {ext_idx} \
+						 (likely metadata/codec drift): {err:?}");
+				})
+				.ok()
+				.flatten()?;
+			eth_transact_payload_to_info(&call.payload, block_number, ext_idx)
+		})
+		.collect()
+}
+
+#[cfg(test)]
+mod helpers_tests {
+	use super::*;
+	use pallet_revive::evm::{Account, TransactionLegacyUnsigned, TransactionUnsigned};
+
+	/// Well-formed payload → `from`/`hash`/indices match the signing account.
+	#[test]
+	fn eth_transact_payload_to_info_happy_path() {
+		let signer = Account::default();
+		let unsigned = TransactionUnsigned::from(TransactionLegacyUnsigned {
+			chain_id: Some(U256::from(42)),
+			to: Some(signer.address()),
+			gas: U256::from(21_000),
+			gas_price: U256::from(1),
+			nonce: U256::from(7),
+			value: U256::from(1_000_000),
+			..Default::default()
+		});
+		let payload = signer.sign_transaction(unsigned).signed_payload();
+		let expected_hash = H256(keccak_256(&payload));
+
+		let info = eth_transact_payload_to_info(&payload, U256::from(123u64), 2)
+			.expect("well-formed payload must decode");
+
+		assert_eq!(info.hash, expected_hash);
+		assert_eq!(info.from, signer.address());
+		assert_eq!(info.block_number, U256::from(123u64));
+		assert_eq!(info.transaction_index, U256::from(2u64));
+		assert_eq!(info.block_hash, H256::zero(), "block_hash stays zero on pre-upgrade");
+	}
+
+	/// Garbage that fails RLP decode → `None` (no panic).
+	#[test]
+	fn eth_transact_payload_to_info_decode_failure() {
+		let garbage = [0xFFu8; 32];
+		let info = eth_transact_payload_to_info(&garbage, U256::from(1u64), 2);
+		assert!(info.is_none(), "garbage payload must yield None");
+	}
+
+	/// Valid RLP but unrecoverable signature (`r = s = 0`) → `None`. Covers the
+	/// `recover_eth_address` branch, distinct from the RLP-decode branch above.
+	#[test]
+	fn eth_transact_payload_to_info_recovery_failure() {
+		let signer = Account::default();
+		let unsigned = TransactionUnsigned::from(TransactionLegacyUnsigned {
+			chain_id: Some(U256::from(42)),
+			to: Some(signer.address()),
+			gas: U256::from(21_000),
+			gas_price: U256::from(1),
+			nonce: U256::from(7),
+			value: U256::from(1_000_000),
+			..Default::default()
+		});
+		let mut signed = signer.sign_transaction(unsigned);
+		match signed {
+			TransactionSigned::TransactionLegacySigned(ref mut tx) => {
+				tx.r = U256::zero();
+				tx.s = U256::zero();
+			},
+			_ => panic!("test built a legacy tx"),
+		}
+		let payload = signed.signed_payload();
+
+		let info = eth_transact_payload_to_info(&payload, U256::from(1u64), 0);
+		assert!(info.is_none(), "payload with unrecoverable signature must yield None");
+	}
+}