Improve diagnostics for the ValidationOutputs checker / inclusion (#1926)

* Improve diagnostics for acceptance criteria failures during inclusion

* Initialize the runtime logger just before logging during inclusion

* Formatting suggestions

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Missed one suggestion

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

polkadot/runtime/parachains/src/inclusion.rs

@@ -186,8 +186,9 @@ decl_module! {
 	}
 }
 
-impl<T: Trait> Module<T> {
+const LOG_TARGET: &str = "parachains_runtime_inclusion";
 
+impl<T: Trait> Module<T> {
 	/// Block initialization logic, called by initializer.
 	pub(crate) fn initializer_initialize(_now: T::BlockNumber) -> Weight { 0 }
 
@@ -400,7 +401,7 @@ impl<T: Trait> Module<T> {
 			// In the meantime, we do certain sanity checks on the candidates and on the scheduled
 			// list.
 			'a:
-			for candidate in &candidates {
+			for (candidate_idx, candidate) in candidates.iter().enumerate() {
 				let para_id = candidate.descriptor().para_id;
 
 				// we require that the candidate is in the context of the parent block.
@@ -413,15 +414,27 @@ impl<T: Trait> Module<T> {
 					Error::<T>::NotCollatorSigned,
 				);
 
-				check_cx.check_validation_outputs(
-					para_id,
-					&candidate.candidate.commitments.head_data,
-					&candidate.candidate.commitments.new_validation_code,
-					candidate.candidate.commitments.processed_downward_messages,
-					&candidate.candidate.commitments.upward_messages,
-					T::BlockNumber::from(candidate.candidate.commitments.hrmp_watermark),
-					&candidate.candidate.commitments.horizontal_messages,
-				)?;
+				if let Err(err) = check_cx
+					.check_validation_outputs(
+						para_id,
+						&candidate.candidate.commitments.head_data,
+						&candidate.candidate.commitments.new_validation_code,
+						candidate.candidate.commitments.processed_downward_messages,
+						&candidate.candidate.commitments.upward_messages,
+						T::BlockNumber::from(candidate.candidate.commitments.hrmp_watermark),
+						&candidate.candidate.commitments.horizontal_messages,
+					)
+				{
+					frame_support::debug::RuntimeLogger::init();
+					log::debug!(
+						target: LOG_TARGET,
+						"Validation outputs checking during inclusion of a candidate {} for parachain `{}` failed: {:?}",
+						candidate_idx,
+						u32::from(para_id),
+						err,
+					);
+					Err(err.strip_into_dispatch_err::<T>())?;
+				};
 
 				for (i, assignment) in scheduled[skip..].iter().enumerate() {
 					check_assignment_in_order(assignment)?;
@@ -542,13 +555,11 @@ impl<T: Trait> Module<T> {
 	}
 
 	/// Run the acceptance criteria checks on the given candidate commitments.
-	///
-	/// Returns an 'Err` if any of the checks doesn't pass.
 	pub(crate) fn check_validation_outputs(
 		para_id: ParaId,
 		validation_outputs: primitives::v1::ValidationOutputs,
-	) -> Result<(), DispatchError> {
-		CandidateCheckContext::<T>::new().check_validation_outputs(
+	) -> bool {
+		if let Err(err) = CandidateCheckContext::<T>::new().check_validation_outputs(
 			para_id,
 			&validation_outputs.head_data,
 			&validation_outputs.new_validation_code,
@@ -556,7 +567,18 @@ impl<T: Trait> Module<T> {
 			&validation_outputs.upward_messages,
 			T::BlockNumber::from(validation_outputs.hrmp_watermark),
 			&validation_outputs.horizontal_messages,
-		)
+		) {
+			frame_support::debug::RuntimeLogger::init();
+			log::debug!(
+				target: LOG_TARGET,
+				"Validation outputs checking for parachain `{}` failed: {:?}",
+				u32::from(para_id),
+				err,
+			);
+			false
+		} else {
+			true
+		}
 	}
 
 	fn enact_candidate(
@@ -692,6 +714,34 @@ const fn availability_threshold(n_validators: usize) -> usize {
 	threshold
 }
 
+#[derive(derive_more::From, Debug)]
+enum AcceptanceCheckErr<BlockNumber> {
+	HeadDataTooLarge,
+	PrematureCodeUpgrade,
+	NewCodeTooLarge,
+	ProcessedDownwardMessages(router::ProcessedDownwardMessagesAcceptanceErr),
+	UpwardMessages(router::UpwardMessagesAcceptanceCheckErr),
+	HrmpWatermark(router::HrmpWatermarkAcceptanceErr<BlockNumber>),
+	OutboundHrmp(router::OutboundHrmpAcceptanceErr),
+}
+
+impl<BlockNumber> AcceptanceCheckErr<BlockNumber> {
+	/// Returns the same error so that it can be threaded through a needle of `DispatchError` and
+	/// ultimately returned from a `Dispatchable`.
+	fn strip_into_dispatch_err<T: Trait>(self) -> Error<T> {
+		use AcceptanceCheckErr::*;
+		match self {
+			HeadDataTooLarge => Error::<T>::HeadDataTooLarge,
+			PrematureCodeUpgrade => Error::<T>::PrematureCodeUpgrade,
+			NewCodeTooLarge => Error::<T>::NewCodeTooLarge,
+			ProcessedDownwardMessages(_) => Error::<T>::IncorrectDownwardMessageHandling,
+			UpwardMessages(_) => Error::<T>::InvalidUpwardMessages,
+			HrmpWatermark(_) => Error::<T>::HrmpWatermarkMishandling,
+			OutboundHrmp(_) => Error::<T>::InvalidOutboundHrmp,
+		}
+	}
+}
+
 /// A collection of data required for checking a candidate.
 struct CandidateCheckContext<T: Trait> {
 	config: configuration::HostConfiguration<T::BlockNumber>,
@@ -720,10 +770,10 @@ impl<T: Trait> CandidateCheckContext<T> {
 		upward_messages: &[primitives::v1::UpwardMessage],
 		hrmp_watermark: T::BlockNumber,
 		horizontal_messages: &[primitives::v1::OutboundHrmpMessage<ParaId>],
-	) -> Result<(), DispatchError> {
+	) -> Result<(), AcceptanceCheckErr<T::BlockNumber>> {
 		ensure!(
 			head_data.0.len() <= self.config.max_head_data_size as _,
-			Error::<T>::HeadDataTooLarge
+			AcceptanceCheckErr::HeadDataTooLarge,
 		);
 
 		// if any, the code upgrade attempt is allowed.
@@ -734,45 +784,28 @@ impl<T: Trait> CandidateCheckContext<T> {
 						&& self.relay_parent_number.saturating_sub(last)
 							>= self.config.validation_upgrade_frequency
 				});
-			ensure!(valid_upgrade_attempt, Error::<T>::PrematureCodeUpgrade);
+			ensure!(
+				valid_upgrade_attempt,
+				AcceptanceCheckErr::PrematureCodeUpgrade,
+			);
 			ensure!(
 				new_validation_code.0.len() <= self.config.max_code_size as _,
-				Error::<T>::NewCodeTooLarge
+				AcceptanceCheckErr::NewCodeTooLarge,
 			);
 		}
 
 		// check if the candidate passes the messaging acceptance criteria
-		ensure!(
-			<router::Module<T>>::check_processed_downward_messages(
-				para_id,
-				processed_downward_messages,
-			),
-			Error::<T>::IncorrectDownwardMessageHandling,
-		);
-		ensure!(
-			<router::Module<T>>::check_upward_messages(
-				&self.config,
-				para_id,
-				upward_messages,
-			),
-			Error::<T>::InvalidUpwardMessages,
-		);
-		ensure!(
-			<router::Module<T>>::check_hrmp_watermark(
-				para_id,
-				self.relay_parent_number,
-				hrmp_watermark,
-			),
-			Error::<T>::HrmpWatermarkMishandling,
-		);
-		ensure!(
-			<router::Module<T>>::check_outbound_hrmp(
-				&self.config,
-				para_id,
-				horizontal_messages,
-			),
-			Error::<T>::InvalidOutboundHrmp,
-		);
+		<router::Module<T>>::check_processed_downward_messages(
+			para_id,
+			processed_downward_messages,
+		)?;
+		<router::Module<T>>::check_upward_messages(&self.config, para_id, upward_messages)?;
+		<router::Module<T>>::check_hrmp_watermark(
+			para_id,
+			self.relay_parent_number,
+			hrmp_watermark,
+		)?;
+		<router::Module<T>>::check_outbound_hrmp(&self.config, para_id, horizontal_messages)?;
 
 		Ok(())
 	}

polkadot/runtime/parachains/src/router.rs

@@ -33,8 +33,9 @@ mod hrmp;
 mod ump;
 
 use hrmp::{HrmpOpenChannelRequest, HrmpChannel};
-pub use dmp::QueueDownwardMessageError;
-pub use ump::UmpSink;
+pub use dmp::{QueueDownwardMessageError, ProcessedDownwardMessagesAcceptanceErr};
+pub use ump::{UmpSink, AcceptanceCheckErr as UpwardMessagesAcceptanceCheckErr};
+pub use hrmp::{HrmpWatermarkAcceptanceErr, OutboundHrmpAcceptanceErr};
 
 #[cfg(test)]
 pub use ump::mock_sink::MockUmpSink;

polkadot/runtime/parachains/src/router/dmp.rs

@@ -17,7 +17,7 @@
 use super::{Trait, Module, Store};
 use crate::configuration::HostConfiguration;
 use frame_support::{StorageMap, weights::Weight, traits::Get};
-use sp_std::prelude::*;
+use sp_std::{fmt, prelude::*};
 use sp_runtime::traits::{BlakeTwo256, Hash as HashT, SaturatedConversion};
 use primitives::v1::{Id as ParaId, DownwardMessage, InboundDownwardMessage, Hash};
 
@@ -28,6 +28,38 @@ pub enum QueueDownwardMessageError {
 	ExceedsMaxMessageSize,
 }
 
+/// An error returned by [`check_processed_downward_messages`] that indicates an acceptance check
+/// didn't pass.
+pub enum ProcessedDownwardMessagesAcceptanceErr {
+	/// If there are pending messages then `processed_downward_messages` should be at least 1,
+	AdvancementRule,
+	/// `processed_downward_messages` should not be greater than the number of pending messages.
+	Underflow {
+		processed_downward_messages: u32,
+		dmq_length: u32,
+	},
+}
+
+impl fmt::Debug for ProcessedDownwardMessagesAcceptanceErr {
+	fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+		use ProcessedDownwardMessagesAcceptanceErr::*;
+		match *self {
+			AdvancementRule => write!(
+				fmt,
+				"DMQ is not empty, but processed_downward_messages is 0",
+			),
+			Underflow {
+				processed_downward_messages,
+				dmq_length,
+			} => write!(
+				fmt,
+				"processed_downward_messages = {}, but dmq_length is only {}",
+				processed_downward_messages, dmq_length,
+			),
+		}
+	}
+}
+
 /// Routines and getters related to downward message passing.
 impl<T: Trait> Module<T> {
 	pub(crate) fn clean_dmp_after_outgoing(outgoing_para: ParaId) {
@@ -72,26 +104,24 @@ impl<T: Trait> Module<T> {
 		Ok(())
 	}
 
-	/// Checks if the number of processed downward messages is valid, i.e.:
-	///
-	/// - if there are pending messages then `processed_downward_messages` should be at least 1,
-	/// - `processed_downward_messages` should not be greater than the number of pending messages.
-	///
-	/// Returns true if all checks have been passed.
+	/// Checks if the number of processed downward messages is valid.
 	pub(crate) fn check_processed_downward_messages(
 		para: ParaId,
 		processed_downward_messages: u32,
-	) -> bool {
+	) -> Result<(), ProcessedDownwardMessagesAcceptanceErr> {
 		let dmq_length = Self::dmq_length(para);
 
 		if dmq_length > 0 && processed_downward_messages == 0 {
-			return false;
+			return Err(ProcessedDownwardMessagesAcceptanceErr::AdvancementRule);
 		}
 		if dmq_length < processed_downward_messages {
-			return false;
+			return Err(ProcessedDownwardMessagesAcceptanceErr::Underflow {
+				processed_downward_messages,
+				dmq_length,
+			});
 		}
 
-		true
+		Ok(())
 	}
 
 	/// Prunes the specified number of messages from the downward message queue of the given para.
@@ -211,20 +241,20 @@ mod tests {
 
 		new_test_ext(default_genesis_config()).execute_with(|| {
 			// processed_downward_messages=0 is allowed when the DMQ is empty.
-			assert!(Router::check_processed_downward_messages(a, 0));
+			assert!(Router::check_processed_downward_messages(a, 0).is_ok());
 
 			queue_downward_message(a, vec![1, 2, 3]).unwrap();
 			queue_downward_message(a, vec![4, 5, 6]).unwrap();
 			queue_downward_message(a, vec![7, 8, 9]).unwrap();
 
 			// 0 doesn't pass if the DMQ has msgs.
-			assert!(!Router::check_processed_downward_messages(a, 0));
+			assert!(!Router::check_processed_downward_messages(a, 0).is_ok());
 			// a candidate can consume up to 3 messages
-			assert!(Router::check_processed_downward_messages(a, 1));
-			assert!(Router::check_processed_downward_messages(a, 2));
-			assert!(Router::check_processed_downward_messages(a, 3));
+			assert!(Router::check_processed_downward_messages(a, 1).is_ok());
+			assert!(Router::check_processed_downward_messages(a, 2).is_ok());
+			assert!(Router::check_processed_downward_messages(a, 3).is_ok());
 			// there is no 4 messages in the queue
-			assert!(!Router::check_processed_downward_messages(a, 4));
+			assert!(!Router::check_processed_downward_messages(a, 4).is_ok());
 		});
 	}
 

polkadot/runtime/parachains/src/router/hrmp.rs

@@ -14,23 +14,19 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use super::{Module, Store, Trait, Error as DispatchError, dmp};
+use super::{dmp, Error as DispatchError, Module, Store, Trait};
 use crate::{
 	configuration::{self, HostConfiguration},
 	paras,
 };
 use codec::{Decode, Encode};
-use frame_support::{
-	traits::Get, weights::Weight, StorageMap, StorageValue, ensure, debug::native as log,
-};
+use frame_support::{ensure, traits::Get, weights::Weight, StorageMap, StorageValue};
 use primitives::v1::{
 	Balance, Hash, HrmpChannelId, Id as ParaId, InboundHrmpMessage, OutboundHrmpMessage,
 	SessionIndex,
 };
 use sp_runtime::traits::{BlakeTwo256, Hash as HashT};
-use sp_std::collections::{btree_set::BTreeSet, btree_map::BTreeMap};
-use sp_std::mem;
-use sp_std::prelude::*;
+use sp_std::{mem, fmt, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, prelude::*};
 
 /// A description of a request to open an HRMP channel.
 #[derive(Encode, Decode)]
@@ -79,7 +75,141 @@ pub struct HrmpChannel {
 	pub mqc_head: Option<Hash>,
 }
 
-const LOG_TARGET: &str = "runtime-parachains::hrmp";
+/// An error returned by [`check_hrmp_watermark`] that indicates an acceptance criteria check
+/// didn't pass.
+pub enum HrmpWatermarkAcceptanceErr<BlockNumber> {
+	AdvancementRule {
+		new_watermark: BlockNumber,
+		last_watermark: BlockNumber,
+	},
+	AheadRelayParent {
+		new_watermark: BlockNumber,
+		relay_chain_parent_number: BlockNumber,
+	},
+	LandsOnBlockWithNoMessages {
+		new_watermark: BlockNumber,
+	},
+}
+
+/// An error returned by [`check_outbound_hrmp`] that indicates an acceptance criteria check
+/// didn't pass.
+pub enum OutboundHrmpAcceptanceErr {
+	MoreMessagesThanPermitted {
+		sent: u32,
+		permitted: u32,
+	},
+	NotSorted {
+		idx: u32,
+	},
+	NoSuchChannel {
+		idx: u32,
+		channel_id: HrmpChannelId,
+	},
+	MaxMessageSizeExceeded {
+		idx: u32,
+		msg_size: u32,
+		max_size: u32,
+	},
+	TotalSizeExceeded {
+		idx: u32,
+		total_size: u32,
+		limit: u32,
+	},
+	CapacityExceeded {
+		idx: u32,
+		count: u32,
+		limit: u32,
+	},
+}
+
+impl<BlockNumber> fmt::Debug for HrmpWatermarkAcceptanceErr<BlockNumber>
+where
+	BlockNumber: fmt::Debug,
+{
+	fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+		use HrmpWatermarkAcceptanceErr::*;
+		match self {
+			AdvancementRule {
+				new_watermark,
+				last_watermark,
+			} => write!(
+				fmt,
+				"the HRMP watermark is not advanced relative to the last watermark ({:?} > {:?})",
+				new_watermark,
+				last_watermark,
+			),
+			AheadRelayParent {
+				new_watermark,
+				relay_chain_parent_number,
+			} => write!(
+				fmt,
+				"the HRMP watermark is ahead the relay-parent ({:?} > {:?})",
+				new_watermark,
+				relay_chain_parent_number,
+			),
+			LandsOnBlockWithNoMessages { new_watermark } => write!(
+				fmt,
+				"the HRMP watermark ({:?}) doesn't land on a block with messages received",
+				new_watermark,
+			),
+		}
+	}
+}
+
+impl fmt::Debug for OutboundHrmpAcceptanceErr {
+	fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+		use OutboundHrmpAcceptanceErr::*;
+		match self {
+			MoreMessagesThanPermitted { sent, permitted } => write!(
+				fmt,
+				"more HRMP messages than permitted by config ({} > {})",
+				sent,
+				permitted,
+			),
+			NotSorted { idx } => write!(
+				fmt,
+				"the HRMP messages are not sorted (first unsorted is at index {})",
+				idx,
+			),
+			NoSuchChannel { idx, channel_id } => write!(
+				fmt,
+				"the HRMP message at index {} is sent to a non existent channel {:?}->{:?}",
+				idx,
+				channel_id.sender,
+				channel_id.recipient,
+			),
+			MaxMessageSizeExceeded {
+				idx,
+				msg_size,
+				max_size,
+			} => write!(
+				fmt,
+				"the HRMP message at index {} exceeds the negotiated channel maximum message size ({} > {})",
+				idx,
+				msg_size,
+				max_size,
+			),
+			TotalSizeExceeded {
+				idx,
+				total_size,
+				limit,
+			} => write!(
+				fmt,
+				"sending the HRMP message at index {} would exceed the neogitiated channel total size  ({} > {})",
+				idx,
+				total_size,
+				limit,
+			),
+			CapacityExceeded { idx, count, limit } => write!(
+				fmt,
+				"sending the HRMP message at index {} would exceed the neogitiated channel capacity  ({} > {})",
+				idx,
+				count,
+				limit,
+			),
+		}
+	}
+}
 
 /// Routines and getters related to HRMP.
 impl<T: Trait> Module<T> {
@@ -125,10 +255,9 @@ impl<T: Trait> Module<T> {
 
 			idx -= 1;
 			let channel_id = open_req_channels[idx].clone();
-			let mut request = <Self as Store>::HrmpOpenChannelRequests::get(&channel_id)
-				.expect(
-					"can't be `None` due to the invariant that the list contains the same items as the set; qed"
-				);
+			let mut request = <Self as Store>::HrmpOpenChannelRequests::get(&channel_id).expect(
+				"can't be `None` due to the invariant that the list contains the same items as the set; qed",
+			);
 
 			if request.confirmed {
 				if <paras::Module<T>>::is_valid_para(channel_id.sender)
@@ -243,7 +372,7 @@ impl<T: Trait> Module<T> {
 		recipient: ParaId,
 		relay_chain_parent_number: T::BlockNumber,
 		new_hrmp_watermark: T::BlockNumber,
-	) -> bool {
+	) -> Result<(), HrmpWatermarkAcceptanceErr<T::BlockNumber>> {
 		// First, check where the watermark CANNOT legally land.
 		//
 		// (a) For ensuring that messages are eventually, a rule requires each parablock new
@@ -253,23 +382,17 @@ impl<T: Trait> Module<T> {
 		//     not be greater than the relay-chain context block which the parablock refers to.
 		if let Some(last_watermark) = <Self as Store>::HrmpWatermarks::get(&recipient) {
 			if new_hrmp_watermark <= last_watermark {
-				log::warn!(
-					target: LOG_TARGET,
-					"the HRMP watermark is not advanced relative to the last watermark  ({} > {})",
-					new_hrmp_watermark,
+				return Err(HrmpWatermarkAcceptanceErr::AdvancementRule {
+					new_watermark: new_hrmp_watermark,
 					last_watermark,
-				);
-				return false;
+				});
 			}
 		}
 		if new_hrmp_watermark > relay_chain_parent_number {
-			log::warn!(
-				target: LOG_TARGET,
-				"the HRMP watermark is ahead the relay-parent ({} > {})",
-				new_hrmp_watermark,
+			return Err(HrmpWatermarkAcceptanceErr::AheadRelayParent {
+				new_watermark: new_hrmp_watermark,
 				relay_chain_parent_number,
-			);
-			return false;
+			});
 		}
 
 		// Second, check where the watermark CAN land. It's one of the following:
@@ -277,21 +400,18 @@ impl<T: Trait> Module<T> {
 		// (a) The relay parent block number.
 		// (b) A relay-chain block in which this para received at least one message.
 		if new_hrmp_watermark == relay_chain_parent_number {
-			true
+			Ok(())
 		} else {
 			let digest = <Self as Store>::HrmpChannelDigests::get(&recipient);
 			if !digest
 				.binary_search_by_key(&new_hrmp_watermark, |(block_no, _)| *block_no)
 				.is_ok()
 			{
-				log::warn!(
-					target: LOG_TARGET,
-					"the HRMP watermark ({}) doesn't land on a block with messages received",
-					new_hrmp_watermark,
-				);
-				return false;
+				return Err(HrmpWatermarkAcceptanceErr::LandsOnBlockWithNoMessages {
+					new_watermark: new_hrmp_watermark,
+				});
 			}
-			true
+			Ok(())
 		}
 	}
 
@@ -299,31 +419,27 @@ impl<T: Trait> Module<T> {
 		config: &HostConfiguration<T::BlockNumber>,
 		sender: ParaId,
 		out_hrmp_msgs: &[OutboundHrmpMessage<ParaId>],
-	) -> bool {
+	) -> Result<(), OutboundHrmpAcceptanceErr> {
 		if out_hrmp_msgs.len() as u32 > config.hrmp_max_message_num_per_candidate {
-			log::warn!(
-				target: LOG_TARGET,
-				"more HRMP messages than permitted by config ({} > {})",
-				out_hrmp_msgs.len(),
-				config.hrmp_max_message_num_per_candidate,
-			);
-			return false;
+			return Err(OutboundHrmpAcceptanceErr::MoreMessagesThanPermitted {
+				sent: out_hrmp_msgs.len() as u32,
+				permitted: config.hrmp_max_message_num_per_candidate,
+			});
 		}
 
 		let mut last_recipient = None::<ParaId>;
 
-		for (idx, out_msg) in out_hrmp_msgs.iter().enumerate() {
+		for (idx, out_msg) in out_hrmp_msgs
+			.iter()
+			.enumerate()
+			.map(|(idx, out_msg)| (idx as u32, out_msg))
+		{
 			match last_recipient {
 				// the messages must be sorted in ascending order and there must be no two messages sent
 				// to the same recipient. Thus we can check that every recipient is strictly greater than
 				// the previous one.
 				Some(last_recipient) if out_msg.recipient <= last_recipient => {
-					log::warn!(
-						target: LOG_TARGET,
-						"the HRMP messages are not sorted (at index {})",
-						idx,
-					);
-					return false;
+					return Err(OutboundHrmpAcceptanceErr::NotSorted { idx });
 				}
 				_ => last_recipient = Some(out_msg.recipient),
 			}
@@ -336,54 +452,39 @@ impl<T: Trait> Module<T> {
 			let channel = match <Self as Store>::HrmpChannels::get(&channel_id) {
 				Some(channel) => channel,
 				None => {
-					log::warn!(
-						target: LOG_TARGET,
-						"the HRMP message at index {} is sent to a non existent channel {}->{}",
-						idx,
-						channel_id.sender,
-						channel_id.recipient,
-					);
-					return false;
+					return Err(OutboundHrmpAcceptanceErr::NoSuchChannel { channel_id, idx });
 				}
 			};
 
-			if out_msg.data.len() as u32 > channel.max_message_size {
-				log::warn!(
-					target: LOG_TARGET,
-					"the HRMP message at index {} exceeds the negotiated channel maximum message size ({} > {})",
+			let msg_size = out_msg.data.len() as u32;
+			if msg_size > channel.max_message_size {
+				return Err(OutboundHrmpAcceptanceErr::MaxMessageSizeExceeded {
 					idx,
-					out_msg.data.len(),
-					channel.max_message_size,
-				);
-				return false;
+					msg_size,
+					max_size: channel.max_message_size,
+				});
 			}
 
 			let new_total_size = channel.total_size + out_msg.data.len() as u32;
 			if new_total_size > channel.max_total_size {
-				log::warn!(
-					target: LOG_TARGET,
-					"sending the HRMP message at index {} would exceed the neogitiated channel total size  ({} > {})",
+				return Err(OutboundHrmpAcceptanceErr::TotalSizeExceeded {
 					idx,
-					new_total_size,
-					channel.max_total_size,
-				);
-				return false;
+					total_size: new_total_size,
+					limit: channel.max_total_size,
+				});
 			}
 
 			let new_msg_count = channel.msg_count + 1;
 			if new_msg_count > channel.max_capacity {
-				log::warn!(
-					target: LOG_TARGET,
-					"sending the HRMP message at index {} would exceed the neogitiated channel capacity  ({} > {})",
+				return Err(OutboundHrmpAcceptanceErr::CapacityExceeded {
 					idx,
-					new_msg_count,
-					channel.max_capacity,
-				);
-				return false;
+					count: new_msg_count,
+					limit: channel.max_capacity,
+				});
 			}
 		}
 
-		true
+		Ok(())
 	}
 
 	pub(crate) fn prune_hrmp(recipient: ParaId, new_hrmp_watermark: T::BlockNumber) -> Weight {
@@ -662,8 +763,8 @@ impl<T: Trait> Module<T> {
 		<Self as Store>::HrmpAcceptedChannelRequestCount::insert(&origin, accepted_cnt + 1);
 
 		let notification_bytes = {
-			use xcm::v0::Xcm;
 			use codec::Encode as _;
+			use xcm::v0::Xcm;
 
 			Xcm::HrmpChannelAccepted {
 				recipient: u32::from(origin),
@@ -708,8 +809,8 @@ impl<T: Trait> Module<T> {
 
 		let config = <configuration::Module<T>>::config();
 		let notification_bytes = {
-			use xcm::v0::Xcm;
 			use codec::Encode as _;
+			use xcm::v0::Xcm;
 
 			Xcm::HrmpChannelClosing {
 				initiator: u32::from(origin),
@@ -775,8 +876,8 @@ impl<T: Trait> Module<T> {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use crate::mock::{new_test_ext, Configuration, Paras, Router, System};
 	use crate::router::tests::default_genesis_config;
-	use crate::mock::{Configuration, System, Paras, Router, new_test_ext};
 	use primitives::v1::BlockNumber;
 	use std::collections::{BTreeMap, HashSet};
 
@@ -1139,14 +1240,14 @@ mod tests {
 				data: b"this is an emergency".to_vec(),
 			}];
 			let config = Configuration::config();
-			assert!(Router::check_outbound_hrmp(&config, para_a, &msgs));
+			assert!(Router::check_outbound_hrmp(&config, para_a, &msgs).is_ok());
 			let _ = Router::queue_outbound_hrmp(para_a, msgs);
 			assert_storage_consistency_exhaustive();
 
 			// On Block 7:
 			// B receives the message sent by A. B sets the watermark to 6.
 			run_to_block(7, None);
-			assert!(Router::check_hrmp_watermark(para_b, 7, 6));
+			assert!(Router::check_hrmp_watermark(para_b, 7, 6).is_ok());
 			let _ = Router::prune_hrmp(para_b, 6);
 			assert_storage_consistency_exhaustive();
 		});
@@ -1203,7 +1304,7 @@ mod tests {
 				data: b"knock".to_vec(),
 			}];
 			let config = Configuration::config();
-			assert!(Router::check_outbound_hrmp(&config, para_a, &msgs));
+			assert!(Router::check_outbound_hrmp(&config, para_a, &msgs).is_ok());
 			let _ = Router::queue_outbound_hrmp(para_a, msgs.clone());
 
 			// Verify that the sent messages are there and that also the empty channels are present.

polkadot/runtime/parachains/src/router/ump.rs

@@ -16,9 +16,9 @@
 
 use super::{Trait, Module, Store};
 use crate::configuration::{self, HostConfiguration};
-use sp_std::prelude::*;
+use sp_std::{fmt, prelude::*};
 use sp_std::collections::{btree_map::BTreeMap, vec_deque::VecDeque};
-use frame_support::{StorageMap, StorageValue, weights::Weight, traits::Get, debug::native as log};
+use frame_support::{StorageMap, StorageValue, weights::Weight, traits::Get};
 use primitives::v1::{Id as ParaId, UpwardMessage};
 
 /// All upward messages coming from parachains will be funneled into an implementation of this trait.
@@ -50,7 +50,63 @@ impl UmpSink for () {
 	}
 }
 
-const LOG_TARGET: &str = "runtime-parachains::upward-messages";
+/// An error returned by [`check_upward_messages`] that indicates a violation of one of acceptance
+/// criteria rules.
+pub enum AcceptanceCheckErr {
+	MoreMessagesThanPermitted {
+		sent: u32,
+		permitted: u32,
+	},
+	MessageSize {
+		idx: u32,
+		msg_size: u32,
+		max_size: u32,
+	},
+	CapacityExceeded {
+		count: u32,
+		limit: u32,
+	},
+	TotalSizeExceeded {
+		total_size: u32,
+		limit: u32,
+	},
+}
+
+impl fmt::Debug for AcceptanceCheckErr {
+	fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+		match *self {
+			AcceptanceCheckErr::MoreMessagesThanPermitted { sent, permitted } => write!(
+				fmt,
+				"more upward messages than permitted by config ({} > {})",
+				sent,
+				permitted,
+			),
+			AcceptanceCheckErr::MessageSize {
+				idx,
+				msg_size,
+				max_size,
+			} => write!(
+				fmt,
+				"upward message idx {} larger than permitted by config ({} > {})",
+				idx,
+				msg_size,
+				max_size,
+			),
+			AcceptanceCheckErr::CapacityExceeded { count, limit } => write!(
+				fmt,
+				"the ump queue would have more items than permitted by config ({} > {})",
+				count,
+				limit,
+			),
+			AcceptanceCheckErr::TotalSizeExceeded { total_size, limit } => write!(
+				fmt,
+				"the ump queue would have grown past the max size permitted by config ({} > {})",
+				total_size, 
+				limit,
+			),
+		}
+	}
+}
 
 /// Routines related to the upward message passing.
 impl<T: Trait> Module<T> {
@@ -79,15 +135,12 @@ impl<T: Trait> Module<T> {
 		config: &HostConfiguration<T::BlockNumber>,
 		para: ParaId,
 		upward_messages: &[UpwardMessage],
-	) -> bool {
+	) -> Result<(), AcceptanceCheckErr> {
 		if upward_messages.len() as u32 > config.max_upward_message_num_per_candidate {
-			log::warn!(
-				target: LOG_TARGET,
-				"more upward messages than permitted by config ({} > {})",
-				upward_messages.len(),
-				config.max_upward_message_num_per_candidate,
-			);
-			return false;
+			return Err(AcceptanceCheckErr::MoreMessagesThanPermitted {
+				sent: upward_messages.len() as u32,
+				permitted: config.max_upward_message_num_per_candidate,
+			});
 		}
 
 		let (mut para_queue_count, mut para_queue_size) =
@@ -96,14 +149,11 @@ impl<T: Trait> Module<T> {
 		for (idx, msg) in upward_messages.into_iter().enumerate() {
 			let msg_size = msg.len() as u32;
 			if msg_size > config.max_upward_message_size {
-				log::warn!(
-					target: LOG_TARGET,
-					"upward message idx {} larger than permitted by config ({} > {})",
-					idx,
+				return Err(AcceptanceCheckErr::MessageSize {
+					idx: idx as u32,
 					msg_size,
-					config.max_upward_message_size,
-				);
-				return false;
+					max_size: config.max_upward_message_size,
+				});
 			}
 			para_queue_count += 1;
 			para_queue_size += msg_size;
@@ -112,21 +162,19 @@ impl<T: Trait> Module<T> {
 		// make sure that the queue is not overfilled.
 		// we do it here only once since returning false invalidates the whole relay-chain block.
 		if para_queue_count > config.max_upward_queue_count {
-			log::warn!(
-				target: LOG_TARGET,
-				"the ump queue would have more items than permitted by config ({} > {})",
-				para_queue_count, config.max_upward_queue_count,
-			);
+			return Err(AcceptanceCheckErr::CapacityExceeded {
+				count: para_queue_count,
+				limit: config.max_upward_queue_count,
+			});
 		}
 		if para_queue_size > config.max_upward_queue_size {
-			log::warn!(
-				target: LOG_TARGET,
-				"the ump queue would have grown past the max size permitted by config ({} > {})",
-				para_queue_size, config.max_upward_queue_size,
-			);
+			return Err(AcceptanceCheckErr::TotalSizeExceeded {
+				total_size: para_queue_size,
+				limit: config.max_upward_queue_size,
+			});
 		}
-		para_queue_count <= config.max_upward_queue_count
-			&& para_queue_size <= config.max_upward_queue_size
+
+		Ok(())
 	}
 
 	/// Enacts all the upward messages sent by a candidate.
@@ -539,11 +587,7 @@ mod tests {
 
 	fn queue_upward_msg(para: ParaId, msg: UpwardMessage) {
 		let msgs = vec![msg];
-		assert!(Router::check_upward_messages(
-			&Configuration::config(),
-			para,
-			&msgs,
-		));
+		assert!(Router::check_upward_messages(&Configuration::config(), para, &msgs).is_ok());
 		let _ = Router::enact_upward_messages(para, msgs);
 	}
 
@@ -737,5 +781,4 @@ mod tests {
 			}
 		});
 	}
-
 }

polkadot/runtime/parachains/src/runtime_api_impl/v1.rs

@@ -223,21 +223,7 @@ pub fn check_validation_outputs<T: initializer::Trait>(
 	para_id: ParaId,
 	outputs: primitives::v1::ValidationOutputs,
 ) -> bool {
-	match <inclusion::Module<T>>::check_validation_outputs(para_id, outputs) {
-		Ok(()) => true,
-		Err(e) => {
-			frame_support::debug::RuntimeLogger::init();
-			let err: &'static str = e.into();
-			log::debug!(
-				target: "candidate_validation",
-				"Validation outputs checking for parachain `{}` failed: {}",
-				u32::from(para_id),
-				err,
-			);
-
-			false
-		}
-	}
+	<inclusion::Module<T>>::check_validation_outputs(para_id, outputs)
 }
 
 /// Implementation for the `session_index_for_child` function of the runtime API.