From dd4d8e24e03b99e9a827bee32d22178168621ff8 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 11 Nov 2020 11:34:52 +0100 Subject: [PATCH] Improve diagnostics for the ValidationOutputs checker / inclusion (#1926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Missed one suggestion Co-authored-by: Bastian Köcher Co-authored-by: Bastian Köcher --- polkadot/Cargo.lock | 1 + polkadot/runtime/parachains/Cargo.toml | 1 + polkadot/runtime/parachains/src/inclusion.rs | 135 +++++---- polkadot/runtime/parachains/src/router.rs | 5 +- polkadot/runtime/parachains/src/router/dmp.rs | 64 +++-- .../runtime/parachains/src/router/hrmp.rs | 271 ++++++++++++------ polkadot/runtime/parachains/src/router/ump.rs | 115 +++++--- .../parachains/src/runtime_api_impl/v1.rs | 16 +- 8 files changed, 402 insertions(+), 206 deletions(-) diff --git a/polkadot/Cargo.lock b/polkadot/Cargo.lock index d571ac2fd0..d121c5811b 100644 --- a/polkadot/Cargo.lock +++ b/polkadot/Cargo.lock @@ -5445,6 +5445,7 @@ name = "polkadot-runtime-parachains" version = "0.8.0" dependencies = [ "bitvec", + "derive_more", "frame-benchmarking", "frame-support", "frame-system", diff --git a/polkadot/runtime/parachains/Cargo.toml b/polkadot/runtime/parachains/Cargo.toml index fac83aa381..98c047d1e9 100644 --- a/polkadot/runtime/parachains/Cargo.toml +++ b/polkadot/runtime/parachains/Cargo.toml @@ -10,6 +10,7 @@ codec = { package = "parity-scale-codec", version = "1.3.4", default-features = log = "0.4.11" rustc-hex = { version = "2.0.1", default-features = false } serde = { version = "1.0.102", features = [ "derive" ], optional = true } +derive_more = { version = "0.99.11" } sp-api = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } inherents = { package = "sp-inherents", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } diff --git a/polkadot/runtime/parachains/src/inclusion.rs b/polkadot/runtime/parachains/src/inclusion.rs index b685be1bf3..572a426e3a 100644 --- a/polkadot/runtime/parachains/src/inclusion.rs +++ b/polkadot/runtime/parachains/src/inclusion.rs @@ -186,8 +186,9 @@ decl_module! { } } -impl Module { +const LOG_TARGET: &str = "parachains_runtime_inclusion"; +impl Module { /// Block initialization logic, called by initializer. pub(crate) fn initializer_initialize(_now: T::BlockNumber) -> Weight { 0 } @@ -400,7 +401,7 @@ impl Module { // 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 Module { Error::::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::())?; + }; for (i, assignment) in scheduled[skip..].iter().enumerate() { check_assignment_in_order(assignment)?; @@ -542,13 +555,11 @@ impl Module { } /// 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::::new().check_validation_outputs( + ) -> bool { + if let Err(err) = CandidateCheckContext::::new().check_validation_outputs( para_id, &validation_outputs.head_data, &validation_outputs.new_validation_code, @@ -556,7 +567,18 @@ impl Module { &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 { + HeadDataTooLarge, + PrematureCodeUpgrade, + NewCodeTooLarge, + ProcessedDownwardMessages(router::ProcessedDownwardMessagesAcceptanceErr), + UpwardMessages(router::UpwardMessagesAcceptanceCheckErr), + HrmpWatermark(router::HrmpWatermarkAcceptanceErr), + OutboundHrmp(router::OutboundHrmpAcceptanceErr), +} + +impl AcceptanceCheckErr { + /// 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(self) -> Error { + use AcceptanceCheckErr::*; + match self { + HeadDataTooLarge => Error::::HeadDataTooLarge, + PrematureCodeUpgrade => Error::::PrematureCodeUpgrade, + NewCodeTooLarge => Error::::NewCodeTooLarge, + ProcessedDownwardMessages(_) => Error::::IncorrectDownwardMessageHandling, + UpwardMessages(_) => Error::::InvalidUpwardMessages, + HrmpWatermark(_) => Error::::HrmpWatermarkMishandling, + OutboundHrmp(_) => Error::::InvalidOutboundHrmp, + } + } +} + /// A collection of data required for checking a candidate. struct CandidateCheckContext { config: configuration::HostConfiguration, @@ -720,10 +770,10 @@ impl CandidateCheckContext { upward_messages: &[primitives::v1::UpwardMessage], hrmp_watermark: T::BlockNumber, horizontal_messages: &[primitives::v1::OutboundHrmpMessage], - ) -> Result<(), DispatchError> { + ) -> Result<(), AcceptanceCheckErr> { ensure!( head_data.0.len() <= self.config.max_head_data_size as _, - Error::::HeadDataTooLarge + AcceptanceCheckErr::HeadDataTooLarge, ); // if any, the code upgrade attempt is allowed. @@ -734,45 +784,28 @@ impl CandidateCheckContext { && self.relay_parent_number.saturating_sub(last) >= self.config.validation_upgrade_frequency }); - ensure!(valid_upgrade_attempt, Error::::PrematureCodeUpgrade); + ensure!( + valid_upgrade_attempt, + AcceptanceCheckErr::PrematureCodeUpgrade, + ); ensure!( new_validation_code.0.len() <= self.config.max_code_size as _, - Error::::NewCodeTooLarge + AcceptanceCheckErr::NewCodeTooLarge, ); } // check if the candidate passes the messaging acceptance criteria - ensure!( - >::check_processed_downward_messages( - para_id, - processed_downward_messages, - ), - Error::::IncorrectDownwardMessageHandling, - ); - ensure!( - >::check_upward_messages( - &self.config, - para_id, - upward_messages, - ), - Error::::InvalidUpwardMessages, - ); - ensure!( - >::check_hrmp_watermark( - para_id, - self.relay_parent_number, - hrmp_watermark, - ), - Error::::HrmpWatermarkMishandling, - ); - ensure!( - >::check_outbound_hrmp( - &self.config, - para_id, - horizontal_messages, - ), - Error::::InvalidOutboundHrmp, - ); + >::check_processed_downward_messages( + para_id, + processed_downward_messages, + )?; + >::check_upward_messages(&self.config, para_id, upward_messages)?; + >::check_hrmp_watermark( + para_id, + self.relay_parent_number, + hrmp_watermark, + )?; + >::check_outbound_hrmp(&self.config, para_id, horizontal_messages)?; Ok(()) } diff --git a/polkadot/runtime/parachains/src/router.rs b/polkadot/runtime/parachains/src/router.rs index 508e4da590..eefc6900b8 100644 --- a/polkadot/runtime/parachains/src/router.rs +++ b/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; diff --git a/polkadot/runtime/parachains/src/router/dmp.rs b/polkadot/runtime/parachains/src/router/dmp.rs index fa0d057c01..cc3163e543 100644 --- a/polkadot/runtime/parachains/src/router/dmp.rs +++ b/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 Module { pub(crate) fn clean_dmp_after_outgoing(outgoing_para: ParaId) { @@ -72,26 +104,24 @@ impl Module { 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()); }); } diff --git a/polkadot/runtime/parachains/src/router/hrmp.rs b/polkadot/runtime/parachains/src/router/hrmp.rs index 4b56af297f..3bdd895cea 100644 --- a/polkadot/runtime/parachains/src/router/hrmp.rs +++ b/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 . -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, } -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 { + 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 fmt::Debug for HrmpWatermarkAcceptanceErr +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 Module { @@ -125,10 +255,9 @@ impl Module { idx -= 1; let channel_id = open_req_channels[idx].clone(); - let mut request = ::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 = ::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 >::is_valid_para(channel_id.sender) @@ -243,7 +372,7 @@ impl Module { recipient: ParaId, relay_chain_parent_number: T::BlockNumber, new_hrmp_watermark: T::BlockNumber, - ) -> bool { + ) -> Result<(), HrmpWatermarkAcceptanceErr> { // 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 Module { // not be greater than the relay-chain context block which the parablock refers to. if let Some(last_watermark) = ::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 Module { // (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 = ::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 Module { config: &HostConfiguration, sender: ParaId, out_hrmp_msgs: &[OutboundHrmpMessage], - ) -> 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::; - 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 Module { let channel = match ::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 Module { ::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 Module { let config = >::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 Module { #[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. diff --git a/polkadot/runtime/parachains/src/router/ump.rs b/polkadot/runtime/parachains/src/router/ump.rs index 63d0726175..2bfdafbb6c 100644 --- a/polkadot/runtime/parachains/src/router/ump.rs +++ b/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 Module { @@ -79,15 +135,12 @@ impl Module { config: &HostConfiguration, 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 Module { 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 Module { // 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 { } }); } - } diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs b/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs index 1f7050661f..48e21bf2bf 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs @@ -223,21 +223,7 @@ pub fn check_validation_outputs( para_id: ParaId, outputs: primitives::v1::ValidationOutputs, ) -> bool { - match >::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 - } - } + >::check_validation_outputs(para_id, outputs) } /// Implementation for the `session_index_for_child` function of the runtime API.