Deduplicate client-side inherents checking logic

cumulus/client/consensus/aura/src/equivocation_import_queue.rs

@@ -36,7 +36,7 @@ use sp_block_builder::BlockBuilder as BlockBuilderApi;
 use sp_consensus::{error::Error as ConsensusError, BlockOrigin};
 use sp_consensus_aura::{AuraApi, Slot, SlotDuration};
 use sp_core::crypto::Pair;
-use sp_inherents::{CreateInherentDataProviders, InherentDataProvider};
+use sp_inherents::CreateInherentDataProviders;
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor};
 use std::{fmt::Debug, sync::Arc};
 
@@ -230,29 +230,14 @@ where
 				.await
 				.map_err(|e| format!("Could not create inherent data {:?}", e))?;
 
-			let inherent_data = create_inherent_data_providers
-				.create_inherent_data()
-				.await
-				.map_err(|e| format!("Could not create inherent data {:?}", e))?;
-
-			let inherent_res = self
-				.client
-				.runtime_api()
-				.check_inherents(parent_hash, block, inherent_data)
-				.map_err(|e| format!("Unable to check block inherents {:?}", e))?;
-
-			if !inherent_res.ok() {
-				for (i, e) in inherent_res.into_errors() {
-					match create_inherent_data_providers.try_handle_error(&i, &e).await {
-						Some(res) => res.map_err(|e| format!("Inherent Error {:?}", e))?,
-						None =>
-							return Err(format!(
-								"Unknown inherent error, source {:?}",
-								String::from_utf8_lossy(&i[..])
-							)),
-					}
-				}
-			}
+			sp_block_builder::check_inherents(
+				self.client.clone(),
+				parent_hash,
+				block,
+				&create_inherent_data_providers,
+			)
+			.await
+			.map_err(|e| format!("Error checking block inherents {:?}", e))?;
 		}
 
 		Ok(block_params)

cumulus/client/consensus/relay-chain/src/import_queue.rs

@@ -27,7 +27,7 @@ use sp_api::ProvideRuntimeApi;
 use sp_block_builder::BlockBuilder as BlockBuilderApi;
 use sp_blockchain::Result as ClientResult;
 use sp_consensus::error::Error as ConsensusError;
-use sp_inherents::{CreateInherentDataProviders, InherentDataProvider};
+use sp_inherents::CreateInherentDataProviders;
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
 /// A verifier that just checks the inherents.
@@ -75,30 +75,15 @@ where
 				.await
 				.map_err(|e| e.to_string())?;
 
-			let inherent_data = inherent_data_providers
-				.create_inherent_data()
-				.await
-				.map_err(|e| format!("{:?}", e))?;
-
 			let block = Block::new(block_params.header.clone(), inner_body);
-
-			let inherent_res = self
-				.client
-				.runtime_api()
-				.check_inherents(*block.header().parent_hash(), block.clone(), inherent_data)
-				.map_err(|e| format!("{:?}", e))?;
-
-			if !inherent_res.ok() {
-				for (i, e) in inherent_res.into_errors() {
-					match inherent_data_providers.try_handle_error(&i, &e).await {
-						Some(r) => r.map_err(|e| format!("{:?}", e))?,
-						None => Err(format!(
-							"Unhandled inherent error from `{}`.",
-							String::from_utf8_lossy(&i)
-						))?,
-					}
-				}
-			}
+			sp_block_builder::check_inherents(
+				self.client.clone(),
+				*block.header().parent_hash(),
+				block.clone(),
+				&inherent_data_providers,
+			)
+			.await
+			.map_err(|e| format!("Error checking block inherents {:?}", e))?;
 
 			let (_, inner_body) = block.deconstruct();
 			block_params.body = Some(inner_body);

prdoc/pr_9175.prdoc

@@ -0,0 +1,22 @@
+title: Deduplicate client-side inherents checking logic
+doc:
+- audience: Node Dev
+  description: Stumbled upon this while working on other issue (https://github.com/paritytech/polkadot-sdk/pull/7902).
+    I thought I might need to change the `CheckInherentsResult` and this deduplication
+    would have made everything easier. Probably changing `CheckInherentsResult` won't
+    be needed in the end, but even so it would be nice to reduce the duplication.
+crates:
+- name: cumulus-client-consensus-aura
+  bump: patch
+- name: cumulus-client-consensus-relay-chain
+  bump: patch
+- name: sc-consensus-aura
+  bump: major
+- name: sc-consensus-babe
+  bump: patch
+- name: sc-consensus-pow
+  bump: patch
+- name: sp-block-builder
+  bump: minor
+- name: sp-inherents
+  bump: patch

substrate/client/consensus/aura/src/import_queue.rs

@@ -127,41 +127,6 @@ impl<C, P, CIDP, N> AuraVerifier<C, P, CIDP, N> {
 	}
 }
 
-impl<C, P, CIDP, N> AuraVerifier<C, P, CIDP, N>
-where
-	CIDP: Send,
-{
-	async fn check_inherents<B: BlockT>(
-		&self,
-		block: B,
-		at_hash: B::Hash,
-		inherent_data: sp_inherents::InherentData,
-		create_inherent_data_providers: CIDP::InherentDataProviders,
-	) -> Result<(), Error<B>>
-	where
-		C: ProvideRuntimeApi<B>,
-		C::Api: BlockBuilderApi<B>,
-		CIDP: CreateInherentDataProviders<B, ()>,
-	{
-		let inherent_res = self
-			.client
-			.runtime_api()
-			.check_inherents(at_hash, block, inherent_data)
-			.map_err(|e| Error::Client(e.into()))?;
-
-		if !inherent_res.ok() {
-			for (i, e) in inherent_res.into_errors() {
-				match create_inherent_data_providers.try_handle_error(&i, &e).await {
-					Some(res) => res.map_err(Error::Inherent)?,
-					None => return Err(Error::UnknownInherentError(i)),
-				}
-			}
-		}
-
-		Ok(())
-	}
-}
-
 #[async_trait::async_trait]
 impl<B: BlockT, C, P, CIDP> Verifier<B> for AuraVerifier<C, P, CIDP, NumberFor<B>>
 where
@@ -242,14 +207,15 @@ where
 						.has_api_with::<dyn BlockBuilderApi<B>, _>(parent_hash, |v| v >= 2)
 						.map_err(|e| e.to_string())?
 					{
-						self.check_inherents(
-							new_block.clone(),
+						sp_block_builder::check_inherents_with_data(
+							self.client.clone(),
 							parent_hash,
+							new_block.clone(),
+							&create_inherent_data_providers,
 							inherent_data,
-							create_inherent_data_providers,
 						)
 						.await
-						.map_err(|e| e.to_string())?;
+						.map_err(|e| format!("Error checking block inherents {:?}", e))?;
 					}
 
 					let (_, inner_body) = new_block.deconstruct();

substrate/client/consensus/aura/src/lib.rs

@@ -481,9 +481,6 @@ pub enum Error<B: BlockT> {
 	/// Client Error
 	#[error(transparent)]
 	Client(sp_blockchain::Error),
-	/// Unknown inherent error for identifier
-	#[error("Unknown inherent error for identifier: {}", String::from_utf8_lossy(.0))]
-	UnknownInherentError(sp_inherents::InherentIdentifier),
 	/// Inherents Error
 	#[error("Inherent error: {0}")]
 	Inherent(sp_inherents::Error),

substrate/client/consensus/babe/src/lib.rs

@@ -1003,20 +1003,23 @@ where
 		inherent_data: InherentData,
 		create_inherent_data_providers: CIDP::InherentDataProviders,
 	) -> Result<(), Error<Block>> {
-		let inherent_res = self
-			.client
-			.runtime_api()
-			.check_inherents(at_hash, block, inherent_data)
-			.map_err(Error::RuntimeApi)?;
-
-		if !inherent_res.ok() {
-			for (i, e) in inherent_res.into_errors() {
-				match create_inherent_data_providers.try_handle_error(&i, &e).await {
-					Some(res) => res.map_err(|e| Error::CheckInherents(e))?,
-					None => return Err(Error::CheckInherentsUnhandled(i)),
-				}
-			}
-		}
+		use sp_block_builder::CheckInherentsError;
+
+		sp_block_builder::check_inherents_with_data(
+			self.client.clone(),
+			at_hash,
+			block,
+			&create_inherent_data_providers,
+			inherent_data,
+		)
+		.await
+		.map_err(|e| match e {
+			CheckInherentsError::CreateInherentData(e) => Error::CreateInherents(e),
+			CheckInherentsError::Client(e) => Error::RuntimeApi(e),
+			CheckInherentsError::CheckInherents(e) => Error::CheckInherents(e),
+			CheckInherentsError::CheckInherentsUnknownError(id) =>
+				Error::CheckInherentsUnhandled(id),
+		})?;
 
 		Ok(())
 	}

substrate/client/consensus/pow/src/lib.rs

@@ -268,29 +268,26 @@ where
 		at_hash: B::Hash,
 		inherent_data_providers: CIDP::InherentDataProviders,
 	) -> Result<(), Error<B>> {
+		use sp_block_builder::CheckInherentsError;
+
 		if *block.header().number() < self.check_inherents_after {
 			return Ok(())
 		}
 
-		let inherent_data = inherent_data_providers
-			.create_inherent_data()
-			.await
-			.map_err(|e| Error::CreateInherents(e))?;
-
-		let inherent_res = self
-			.client
-			.runtime_api()
-			.check_inherents(at_hash, block, inherent_data)
-			.map_err(|e| Error::Client(e.into()))?;
-
-		if !inherent_res.ok() {
-			for (identifier, error) in inherent_res.into_errors() {
-				match inherent_data_providers.try_handle_error(&identifier, &error).await {
-					Some(res) => res.map_err(Error::CheckInherents)?,
-					None => return Err(Error::CheckInherentsUnknownError(identifier)),
-				}
-			}
-		}
+		sp_block_builder::check_inherents(
+			self.client.clone(),
+			at_hash,
+			block,
+			&inherent_data_providers,
+		)
+		.await
+		.map_err(|e| match e {
+			CheckInherentsError::CreateInherentData(e) => Error::CreateInherents(e),
+			CheckInherentsError::Client(e) => Error::Client(e.into()),
+			CheckInherentsError::CheckInherents(e) => Error::CheckInherents(e),
+			CheckInherentsError::CheckInherentsUnknownError(id) =>
+				Error::CheckInherentsUnknownError(id),
+		})?;
 
 		Ok(())
 	}

substrate/primitives/block-builder/src/client_side.rs

@@ -0,0 +1,80 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// 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::BlockBuilder;
+
+use sp_inherents::{InherentData, InherentDataProvider, InherentIdentifier};
+use sp_runtime::traits::Block as BlockT;
+
+/// Errors that occur when creating and checking on the client side.
+#[derive(Debug)]
+pub enum CheckInherentsError {
+	/// Create inherents error.
+	CreateInherentData(sp_inherents::Error),
+	/// Client Error
+	Client(sp_api::ApiError),
+	/// Check inherents error
+	CheckInherents(sp_inherents::Error),
+	/// Unknown inherent error for identifier
+	CheckInherentsUnknownError(InherentIdentifier),
+}
+
+/// Create inherent data and check that the inherents are valid.
+pub async fn check_inherents<Block: BlockT, Client: sp_api::ProvideRuntimeApi<Block>>(
+	client: std::sync::Arc<Client>,
+	at_hash: Block::Hash,
+	block: Block,
+	inherent_data_providers: &impl InherentDataProvider,
+) -> Result<(), CheckInherentsError>
+where
+	Client::Api: BlockBuilder<Block>,
+{
+	let inherent_data = inherent_data_providers
+		.create_inherent_data()
+		.await
+		.map_err(CheckInherentsError::CreateInherentData)?;
+
+	check_inherents_with_data(client, at_hash, block, inherent_data_providers, inherent_data).await
+}
+
+/// Check that the inherents are valid.
+pub async fn check_inherents_with_data<Block: BlockT, Client: sp_api::ProvideRuntimeApi<Block>>(
+	client: std::sync::Arc<Client>,
+	at_hash: Block::Hash,
+	block: Block,
+	inherent_data_provider: &impl InherentDataProvider,
+	inherent_data: InherentData,
+) -> Result<(), CheckInherentsError>
+where
+	Client::Api: BlockBuilder<Block>,
+{
+	let res = client
+		.runtime_api()
+		.check_inherents(at_hash, block, inherent_data)
+		.map_err(CheckInherentsError::Client)?;
+
+	if !res.ok() {
+		for (id, error) in res.into_errors() {
+			match inherent_data_provider.try_handle_error(&id, &error).await {
+				Some(res) => res.map_err(CheckInherentsError::CheckInherents)?,
+				None => return Err(CheckInherentsError::CheckInherentsUnknownError(id)),
+			}
+		}
+	}
+
+	Ok(())
+}

substrate/primitives/block-builder/src/lib.rs

@@ -21,6 +21,12 @@
 
 extern crate alloc;
 
+#[cfg(feature = "std")]
+mod client_side;
+
+#[cfg(feature = "std")]
+pub use client_side::*;
+
 use sp_inherents::{CheckInherentsResult, InherentData};
 use sp_runtime::{traits::Block as BlockT, ApplyExtrinsicResult};
 

substrate/primitives/inherents/src/lib.rs

@@ -318,15 +318,14 @@ impl CheckInherentsResult {
 			return Err(Error::FatalErrorReported)
 		}
 
+		self.okay = false;
 		if error.is_fatal_error() {
+			self.fatal_error = true;
 			// remove the other errors.
 			self.errors.data.clear();
 		}
-
 		self.errors.put_data(identifier, error)?;
 
-		self.okay = false;
-		self.fatal_error = error.is_fatal_error();
 		Ok(())
 	}