From 8fdc8cf9d52f6798c7c2d1578f773ea0f675a446 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 1 May 2023 19:15:51 -0500 Subject: [PATCH 01/19] refactor unincluded segment length into a ConsensusHook --- .../parachain-system/src/consensus_hook.rs | 105 ++++++++++ pallets/parachain-system/src/lib.rs | 194 +++++++++++------- pallets/parachain-system/src/tests.rs | 56 +++-- .../src/unincluded_segment.rs | 68 ++++-- .../src/validate_block/tests.rs | 9 +- test/runtime/src/lib.rs | 6 +- 6 files changed, 320 insertions(+), 118 deletions(-) create mode 100644 pallets/parachain-system/src/consensus_hook.rs diff --git a/pallets/parachain-system/src/consensus_hook.rs b/pallets/parachain-system/src/consensus_hook.rs new file mode 100644 index 00000000000..d9f49a69434 --- /dev/null +++ b/pallets/parachain-system/src/consensus_hook.rs @@ -0,0 +1,105 @@ +// Copyright 2023 Parity Technologies (UK) Ltd. +// This file is part of Cumulus. + +// Cumulus is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Cumulus is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Cumulus. If not, see . + +//! The definition of a [`ConsensusHook`] trait for consensus logic to manage the backlog +//! of parachain blocks ready to submit to the relay chain, as well as some basic implementations. + +use super::relay_state_snapshot::RelayChainStateProof; +use sp_std::num::NonZeroU32; + +/// The possible capacity of the unincluded segment. +#[derive(Clone)] +pub struct UnincludedSegmentCapacity(UnincludedSegmentCapacityInner); + +impl UnincludedSegmentCapacity { + pub(crate) fn get(&self) -> u32 { + match self.0 { + UnincludedSegmentCapacityInner::ExpectParentIncluded => 1, + UnincludedSegmentCapacityInner::Value(v) => v.get(), + } + } + + pub(crate) fn is_expecting_included_parent(&self) -> bool { + match self.0 { + UnincludedSegmentCapacityInner::ExpectParentIncluded => true, + UnincludedSegmentCapacityInner::Value(_) => false, + } + } +} + +#[derive(Clone)] +pub(crate) enum UnincludedSegmentCapacityInner { + ExpectParentIncluded, + Value(NonZeroU32), +} + +impl From for UnincludedSegmentCapacity { + fn from(value: NonZeroU32) -> Self { + UnincludedSegmentCapacity(UnincludedSegmentCapacityInner::Value(value)) + } +} + +/// The consensus hook for dealing with the unincluded segment. +/// +/// Higher-level and user-configurable consensus logic is more informed about the +/// desired unincluded segment length, as well as any rules for adapting it dynamically +/// according to the relay-chain state. +pub trait ConsensusHook { + // TODO [now]: docs. cover where this is called, invariants, etc + fn on_state_proof( + state_proof: &RelayChainStateProof, + ) -> UnincludedSegmentCapacity; +} + +/// A special consensus hook for handling the migration to asynchronous backing gracefully, +/// even if collators haven't been updated to provide the last included parent in the state +/// proof yet. +/// +/// This behaves as though the parent is included, even if the relay chain state proof doesn't contain +/// the included para head. If the para head is present in the state proof, this does ensure the +/// parent is included. +pub struct ExpectParentIncluded; + +impl ConsensusHook for ExpectParentIncluded { + fn on_state_proof( + _state_proof: &RelayChainStateProof, + ) -> UnincludedSegmentCapacity { + UnincludedSegmentCapacity(UnincludedSegmentCapacityInner::ExpectParentIncluded) + } +} + +/// A consensus hook for a fixed unincluded segment length. This hook does nothing but +/// set the capacity of the unincluded segment to the constant N. +/// +/// Since it is illegal to provide an unincluded segment length of 0, this sets a minimum of +/// 1. +pub struct FixedCapacityUnincludedSegment; + +impl ConsensusHook for FixedCapacityUnincludedSegment { + fn on_state_proof( + _state_proof: &RelayChainStateProof, + ) -> UnincludedSegmentCapacity { + NonZeroU32::new(sp_std::cmp::max(N, 1)) + .expect("1 is the minimum value and non-zero; qed") + .into() + } +} + +/// A fixed-capacity unincluded segment hook, which requires that the parent block is +/// included prior to the current block being authored. +/// +/// This is a simple type alias around a fixed-capacity unincluded segment with a size of 1. +pub type RequireParentIncluded = FixedCapacityUnincludedSegment<1>; diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 3edcc6dfb14..3e76cf583da 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -48,7 +48,7 @@ use frame_system::{ensure_none, ensure_root}; use polkadot_parachain::primitives::RelayChainBlockNumber; use scale_info::TypeInfo; use sp_runtime::{ - traits::{Block as BlockT, BlockNumberProvider, Hash, Zero}, + traits::{Block as BlockT, BlockNumberProvider, Hash}, transaction_validity::{ InvalidTransaction, TransactionLongevity, TransactionSource, TransactionValidity, ValidTransaction, @@ -60,13 +60,15 @@ use xcm::latest::XcmHash; mod migration; mod relay_state_snapshot; mod unincluded_segment; -#[macro_use] -pub mod validate_block; #[cfg(test)] mod tests; +pub mod consensus_hook; +#[macro_use] +pub mod validate_block; + use unincluded_segment::{ - Ancestor, HrmpChannelUpdate, SegmentTracker, TotalBandwidthLimits, UsedBandwidth, + Ancestor, HrmpChannelUpdate, OutboundBandwidthLimits, SegmentTracker, UsedBandwidth, }; /// Register the `validate_block` function that is used by parachains to validate blocks on a @@ -92,6 +94,7 @@ use unincluded_segment::{ /// # fn main() {} /// ``` pub use cumulus_pallet_parachain_system_proc_macro::register_validate_block; +pub use consensus_hook::ConsensusHook; pub use relay_state_snapshot::{MessagingStateSnapshot, RelayChainStateProof}; pub use pallet::*; @@ -198,6 +201,13 @@ pub mod pallet { /// Something that can check the associated relay parent block number. type CheckAssociatedRelayNumber: CheckAssociatedRelayNumber; + + /// An entry-point for higher-level logic to manage the backlog of unincluded parachain blocks + /// and authorship rights for those blocks. + /// + /// To maintain the same behavior as prior to asynchronous backing, provide the + /// [`ExpectParentIncludedHook`] here. + type ConsensusHook: ConsensusHook; } #[pallet::hooks] @@ -237,14 +247,27 @@ pub mod pallet { }, }; - let (ump_msg_count, ump_total_bytes) = >::mutate(|up| { - let (count, size) = relevant_messaging_state.relay_dispatch_queue_size; + let total_bandwidth_out = OutboundBandwidthLimits::from_relay_chain_state( + &relevant_messaging_state, + host_config.max_upward_queue_count, + host_config.max_upward_queue_size, + ); + let mut bandwidth_out = None; + if let Some(segment) = AggregatedUnincludedSegment::::get() { + let mut b = total_bandwidth_out.clone(); + b.subtract(segment.used_bandwidth()); + bandwidth_out = Some(b); + } + + let (ump_msg_count, ump_total_bytes) = >::mutate(|up| { + let bandwidth_out = bandwidth_out.as_ref().unwrap_or(&total_bandwidth_out); let available_capacity = cmp::min( - host_config.max_upward_queue_count.saturating_sub(count), + bandwidth_out.ump_messages_remaining, host_config.max_upward_message_num_per_candidate, ); - let available_size = host_config.max_upward_queue_size.saturating_sub(size); + + let available_size = bandwidth_out.ump_bytes_remaining; // Count the number of messages we can possibly fit in the given constraints, i.e. // available_capacity and available_size. @@ -289,23 +312,17 @@ pub mod pallet { .hrmp_max_message_num_per_candidate .min(>::take()) as usize; + // TODO [now]: the `ChannelInfo` implementation for this pallet is what's + // important here for proper limiting. let outbound_messages = T::OutboundXcmpMessageSource::take_outbound_messages(maximum_channels) .into_iter() .map(|(recipient, data)| OutboundHrmpMessage { recipient, data }) .collect::>(); - if MaxUnincludedLen::::get().map_or(false, |max_len| !max_len.is_zero()) { - // NOTE: these limits don't account for the amount of processed messages from - // downward and horizontal queues. - // - // This is correct because: - // - inherent never contains messages that were previously processed. - // - current implementation always attempts to exhaust each message queue. - // - // - let limits = TotalBandwidthLimits::new(&relevant_messaging_state); - + // Update the unincluded segment length; capacity checks were done previously in + // `set_validation_data`, so this can be done unconditionally. + { let hrmp_outgoing = outbound_messages .iter() .map(|msg| { @@ -326,7 +343,7 @@ pub mod pallet { // TODO: In order of this panic to be correct, outbound message source should // respect bandwidth limits as well. // - agg.append(&ancestor, watermark, &limits) + agg.append(&ancestor, watermark, &total_bandwidth_out) .expect("unincluded segment limits exceeded"); }); // Check in `on_initialize` guarantees there's space for this block. @@ -346,8 +363,8 @@ pub mod pallet { weight += T::DbWeight::get().writes(1); } - // New para head was unknown during block finalization, update it. - if MaxUnincludedLen::::get().map_or(false, |max_len| !max_len.is_zero()) { + // The parent hash was unknown during block finalization. Update it here. + { >::mutate(|chain| { if let Some(ancestor) = chain.last_mut() { let parent = frame_system::Pallet::::parent_hash(); @@ -361,7 +378,6 @@ pub mod pallet { // Weight used during finalization. weight += T::DbWeight::get().reads_writes(2, 2); } - weight += T::DbWeight::get().reads(1); // Remove the validation from the old block. ValidationData::::kill(); @@ -461,6 +477,9 @@ pub mod pallet { ) .expect("Invalid relay chain state proof"); + // Update the desired maximum capacity according to the consensus hook. + let capacity = T::ConsensusHook::on_state_proof(&relay_state_proof); + // initialization logic: we know that this runs exactly once every block, // which means we can put the initialization logic here to remove the // sequencing problem. @@ -508,6 +527,16 @@ pub mod pallet { ::on_validation_data(&vfp); // TODO: This is more than zero, but will need benchmarking to figure out what. + // NOTE: We don't account for the amount of processed messages from + // downward and horizontal channels in the unincluded segment. + // + // This is correct only because the current implementation always attempts + // to exhaust each message queue and panics if the DMQ head doesn't match. + // + // If one or more messages were ever "re-processed" in a parachain block before its + // ancestor was included, the MQC heads wouldn't match and the block would be invalid. + // + // let mut total_weight = Weight::zero(); total_weight += Self::process_inbound_downward_messages( relevant_messaging_state.dmq_mqc_head, @@ -518,7 +547,7 @@ pub mod pallet { horizontal_messages, vfp.relay_parent_number, ); - total_weight += Self::maybe_drop_included_ancestors(&relay_state_proof); + total_weight += Self::maybe_drop_included_ancestors(&relay_state_proof, capacity); Ok(PostDispatchInfo { actual_weight: Some(total_weight), pays_fee: Pays::No }) } @@ -621,18 +650,12 @@ pub mod pallet { Unauthorized, } - /// Maximum number of latest included block descendants the runtime is allowed to accept. In other words, - /// these are ancestor of the block being currently executed, not yet sent to the relay chain runtime. - /// - /// This value is optional, but once set to `Some` by the governance, should never go back to `None`. - /// Requires latest included para head to be present in the relay chain storage proof. - #[pallet::storage] - pub(super) type MaxUnincludedLen = StorageValue<_, T::BlockNumber, OptionQuery>; - /// Latest included block descendants the runtime accepted. In other words, these are - /// ancestors of the block being currently executed, not yet sent to the relay chain runtime. + /// ancestors of the currently executing block which have not been included in the observed + /// relay-chain state. /// - /// The segment length is limited by [`MaxUnincludedLen`]. + /// The segment length is limited by the capacity returned from the [`ConsensusHook`] configured + /// in the pallet. #[pallet::storage] pub(super) type UnincludedSegment = StorageValue<_, Vec>, ValueQuery>; @@ -1061,53 +1084,59 @@ impl Pallet { } /// Drop blocks from the unincluded segment with respect to the latest parachain head. - /// - /// No-op if [`MaxUnincludedLen`] is not set. - fn maybe_drop_included_ancestors(relay_state_proof: &RelayChainStateProof) -> Weight { + fn maybe_drop_included_ancestors( + relay_state_proof: &RelayChainStateProof, + capacity: consensus_hook::UnincludedSegmentCapacity, + ) -> Weight { let mut weight_used = Weight::zero(); - // If `MaxUnincludedLen` is present in the storage, parachain head - // is always expected to be included into the relay storage proof. - let para_head_with_len = >::get().map(|max_len| { - ( - relay_state_proof - .read_included_para_head() - .expect("Invalid para head in relay chain state proof"), - max_len, - ) - }); + // If the unincluded segment length is nonzero, then the parachain head must be present. + let para_head = relay_state_proof + .read_included_para_head() + .ok() + .map(|h| T::Hashing::hash(&h.0)); + + let unincluded_segment_len = >::decode_len().unwrap_or(0); weight_used += T::DbWeight::get().reads(1); - let Some((para_head, max_len)) = para_head_with_len else { return weight_used }; - let para_head_hash = T::Hashing::hash(¶_head.0); - if !max_len.is_zero() { - let (dropped, left_count): (Vec>, u32) = - >::mutate(|chain| { - // Drop everything up to the block with an included para head, if present. - let idx = chain - .iter() - .position(|block| { - let head_hash = block.para_head_hash().expect( - "para head hash is updated during block initialization; qed", - ); - head_hash == ¶_head_hash - }) - .map_or(0, |idx| idx + 1); // inclusive. - - let left_count = (idx..chain.len()).count() as u32; - let dropped = chain.drain(..idx).collect(); - (dropped, left_count) - }); - weight_used += T::DbWeight::get().reads_writes(1, 1); + // Clean up unincluded segment if nonempty. + let included_head = match (para_head, capacity.is_expecting_included_parent()) { + (Some(h), true) => { + assert_eq!( + h, + frame_system::Pallet::::parent_hash(), + "expected parent to be included" + ); - // sanity-check there's place for the block at finalization phase. - // - // If this fails, the max segment len is reached and parachain should wait - // for ancestor's inclusion. - assert!( - max_len > left_count.into(), - "no space left for the block in the unincluded segment" - ); + h + } + (Some(h), false) => h, + (None, true) => { + // All this logic is essentially a workaround to support collators which + // might still not provide the included block with the state proof. + frame_system::Pallet::::parent_hash() + }, + (None, false) => panic!("included head not present in relay storage proof"), + }; + + let new_len = { + let para_head_hash = included_head; + let dropped: Vec> = >::mutate(|chain| { + // Drop everything up to (inclusive) the block with an included para head, if present. + let idx = chain + .iter() + .position(|block| { + let head_hash = block.para_head_hash().expect( + "para head hash is updated during block initialization; qed", + ); + head_hash == ¶_head_hash + }) + .map_or(0, |idx| idx + 1); // inclusive. + + chain.drain(..idx).collect() + }); + weight_used += T::DbWeight::get().reads_writes(1, 1); + let new_len = unincluded_segment_len - dropped.len(); if !dropped.is_empty() { >::mutate(|agg| { let agg = agg.as_mut().expect( @@ -1119,7 +1148,18 @@ impl Pallet { }); weight_used += T::DbWeight::get().reads_writes(1, 1); } - } + + new_len as u32 + }; + + // Current block validity check: ensure there is space in the unincluded segment. + // + // If this fails, the parachain needs to wait for ancestors to be included before + // a new block is allowed. + assert!( + new_len < capacity.get(), + "no space left for the block in the unincluded segment" + ); weight_used } diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 8edbabaf5a7..410e5c21ad2 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -38,11 +38,12 @@ use sp_runtime::{ traits::{BlakeTwo256, IdentityLookup}, DispatchErrorWithPostInfo, }; -use sp_std::collections::vec_deque::VecDeque; +use sp_std::{collections::vec_deque::VecDeque, num::NonZeroU32}; use sp_version::RuntimeVersion; use std::cell::RefCell; use crate as parachain_system; +use crate::consensus_hook::UnincludedSegmentCapacity; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -110,6 +111,7 @@ impl Config for Test { type XcmpMessageHandler = SaveIntoThreadLocal; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = TestConsensusHook; } pub struct FromThreadLocal; @@ -119,6 +121,16 @@ std::thread_local! { static HANDLED_DMP_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); static HANDLED_XCMP_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); static SENT_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); + static CONSENSUS_HOOK: RefCell UnincludedSegmentCapacity>> + = RefCell::new(Box::new(|_| NonZeroU32::new(1).unwrap().into())); +} + +pub struct TestConsensusHook; + +impl ConsensusHook for TestConsensusHook { + fn on_state_proof(s: &RelayChainStateProof) -> UnincludedSegmentCapacity { + CONSENSUS_HOOK.with(|f| f.borrow_mut()(s)) + } } fn send_message(dest: ParaId, message: Vec) { @@ -233,7 +245,6 @@ struct BlockTests { inherent_data_hook: Option>, inclusion_delay: Option, - max_unincluded_len: Option, included_para_head: Option, pending_blocks: VecDeque, @@ -297,9 +308,8 @@ impl BlockTests { self } - fn with_unincluded_segment(mut self, inclusion_delay: usize, max_unincluded_len: u64) -> Self { + fn with_inclusion_delay(mut self, inclusion_delay: usize) -> Self { self.inclusion_delay.replace(inclusion_delay); - self.max_unincluded_len.replace(max_unincluded_len); self } @@ -311,11 +321,8 @@ impl BlockTests { relay_chain::HeadData(header.encode()) }; - if let Some(max_unincluded_len) = self.max_unincluded_len { - // Initialize included head if the segment is enabled. - self.included_para_head.replace(parent_head_data.clone()); - >::put(max_unincluded_len); - } + self.included_para_head = Some(parent_head_data.clone()); + for BlockTest { n, within_block, after_block } in self.tests.iter() { // clear pending updates, as applicable if let Some(upgrade_block) = self.pending_upgrade { @@ -433,8 +440,10 @@ fn block_tests_run_on_drop() { #[test] fn unincluded_segment_works() { + CONSENSUS_HOOK.with(|c| *c.borrow_mut() = Box::new(|_| NonZeroU32::new(10).unwrap().into())); + BlockTests::new() - .with_unincluded_segment(1, 10) + .with_inclusion_delay(1) .add_with_post_test( 123, || {}, @@ -457,17 +466,19 @@ fn unincluded_segment_works() { || {}, || { let segment = >::get(); - // Block 123 was popped from the segment, the len is still 2. + // Block 123 was popped from the segment, the len is still 1. assert_eq!(segment.len(), 2); }, ); } #[test] -#[should_panic] +#[should_panic = "no space left for the block in the unincluded segment"] fn unincluded_segment_is_limited() { + CONSENSUS_HOOK.with(|c| *c.borrow_mut() = Box::new(|_| NonZeroU32::new(1).unwrap().into())); + BlockTests::new() - .with_unincluded_segment(10, 1) + .with_inclusion_delay(2) .add_with_post_test( 123, || {}, @@ -621,15 +632,18 @@ fn send_upward_message_num_per_candidate() { let v = UpwardMessages::::get(); assert_eq!(v, vec![b"Mr F was here".to_vec()]); }, - ) - .add_with_post_test( - 2, - || { /* do nothing within block */ }, - || { - let v = UpwardMessages::::get(); - assert_eq!(v, vec![b"message 2".to_vec()]); - }, ); + // .add_with_post_test( + // 2, + // || { + // assert_eq!(UnincludedSegment::::get().len(), 0); + // /* do nothing within block */ + // }, + // || { + // let v = UpwardMessages::::get(); + // assert_eq!(v, vec![b"message 2".to_vec()]); + // }, + // ); } #[test] diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index d0e5dd47f61..dea24638bce 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -27,6 +27,7 @@ use scale_info::TypeInfo; use sp_std::{collections::btree_map::BTreeMap, marker::PhantomData}; /// Constraints on outbound HRMP channel. +#[derive(Clone)] pub struct HrmpOutboundLimits { /// The maximum bytes that can be written to the channel. pub bytes_remaining: u32, @@ -34,8 +35,9 @@ pub struct HrmpOutboundLimits { pub messages_remaining: u32, } -/// Constraints imposed on the entire segment, i.e. based on the latest included parablock. -pub struct TotalBandwidthLimits { +/// Limits on outbound message bandwidth. +#[derive(Clone)] +pub struct OutboundBandwidthLimits { /// The amount of UMP messages remaining. pub ump_messages_remaining: u32, /// The amount of UMP bytes remaining. @@ -44,11 +46,24 @@ pub struct TotalBandwidthLimits { pub hrmp_outgoing: BTreeMap, } -impl TotalBandwidthLimits { - /// Creates new limits from the messaging state. - pub fn new(messaging_state: &MessagingStateSnapshot) -> Self { - let (ump_messages_remaining, ump_bytes_remaining) = +impl OutboundBandwidthLimits { + /// Creates new limits from the messaging state and upward message queue maximums fetched + /// from the host configuration. + /// + /// These will be the total bandwidth limits across the entire unincluded segment. + pub fn from_relay_chain_state( + messaging_state: &MessagingStateSnapshot, + max_upward_queue_count: u32, + max_upward_queue_size: u32, + ) -> Self { + let (ump_messages_in_relay, ump_bytes_in_relay) = messaging_state.relay_dispatch_queue_size; + + let (ump_messages_remaining, ump_bytes_remaining) = ( + max_upward_queue_count.saturating_sub(ump_messages_in_relay), + max_upward_queue_size.saturating_sub(ump_bytes_in_relay), + ); + let hrmp_outgoing = messaging_state .egress_channels .iter() @@ -56,8 +71,8 @@ impl TotalBandwidthLimits { ( *id, HrmpOutboundLimits { - bytes_remaining: channel.max_total_size, - messages_remaining: channel.max_capacity, + bytes_remaining: channel.max_total_size.saturating_sub(channel.total_size), + messages_remaining: channel.max_capacity.saturating_sub(channel.msg_count), }, ) }) @@ -65,6 +80,18 @@ impl TotalBandwidthLimits { Self { ump_messages_remaining, ump_bytes_remaining, hrmp_outgoing } } + + /// Compute the remaining bandwidth when accounting for the used amounts provided. + pub fn subtract(&mut self, used: &UsedBandwidth) { + self.ump_messages_remaining = self.ump_messages_remaining.saturating_sub(used.ump_msg_count); + self.ump_bytes_remaining = self.ump_bytes_remaining.saturating_sub(used.ump_total_bytes); + for (para_id, channel_limits) in self.hrmp_outgoing.iter_mut() { + if let Some(update) = used.hrmp_outgoing.get(para_id) { + channel_limits.bytes_remaining = channel_limits.bytes_remaining.saturating_sub(update.total_bytes); + channel_limits.messages_remaining = channel_limits.messages_remaining.saturating_sub(update.msg_count); + } + } + } } /// The error type for updating bandwidth used by a segment. @@ -131,7 +158,7 @@ impl HrmpChannelUpdate { &self, other: &Self, recipient: ParaId, - limits: &TotalBandwidthLimits, + limits: &OutboundBandwidthLimits, ) -> Result { let limits = limits .hrmp_outgoing @@ -186,7 +213,7 @@ impl UsedBandwidth { fn append( &self, other: &Self, - limits: &TotalBandwidthLimits, + limits: &OutboundBandwidthLimits, ) -> Result { let mut new = self.clone(); @@ -277,11 +304,13 @@ pub struct SegmentTracker { impl SegmentTracker { /// Tries to append another block to the tracker, respecting given bandwidth limits. + /// In practice, the bandwidth limits supplied should be the total allowed within the + /// block. pub fn append( &mut self, block: &Ancestor, hrmp_watermark: relay_chain::BlockNumber, - limits: &TotalBandwidthLimits, + limits: &OutboundBandwidthLimits, ) -> Result<(), BandwidthUpdateError> { if let Some(watermark) = self.hrmp_watermark.as_ref() { if &hrmp_watermark <= watermark { @@ -304,6 +333,11 @@ impl SegmentTracker { // Watermark doesn't need to be updated since the is always dropped // from the tail of the segment. } + + /// Return a reference to the used bandwidth across the entire segment. + pub fn used_bandwidth(&self) -> &UsedBandwidth { + &self.used_bandwidth + } } #[cfg(test)] @@ -319,7 +353,7 @@ mod tests { let para_1 = ParaId::from(1); let para_1_limits = HrmpOutboundLimits { bytes_remaining: u32::MAX, messages_remaining: 3 }; let hrmp_outgoing = [(para_0, para_0_limits), (para_1, para_1_limits)].into(); - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 0, ump_bytes_remaining: 0, hrmp_outgoing, @@ -371,7 +405,7 @@ mod tests { HrmpOutboundLimits { bytes_remaining: 25, messages_remaining: u32::MAX }; let hrmp_outgoing = [(para_0, para_0_limits)].into(); - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 0, ump_bytes_remaining: 0, hrmp_outgoing, @@ -410,7 +444,7 @@ mod tests { let para_1 = ParaId::from(1); let para_1_limits = HrmpOutboundLimits { bytes_remaining: 20, messages_remaining: 3 }; let hrmp_outgoing = [(para_0, para_0_limits), (para_1, para_1_limits)].into(); - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 0, ump_bytes_remaining: 0, hrmp_outgoing, @@ -477,7 +511,7 @@ mod tests { hrmp_outgoing: BTreeMap::default(), }; - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 5, ump_bytes_remaining: 50, hrmp_outgoing: BTreeMap::default(), @@ -535,7 +569,7 @@ mod tests { used_bandwidth: UsedBandwidth::default(), para_head_hash: None::, }; - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 0, ump_bytes_remaining: 0, hrmp_outgoing: BTreeMap::default(), @@ -579,7 +613,7 @@ mod tests { let para_1_limits = HrmpOutboundLimits { bytes_remaining: u32::MAX, messages_remaining: u32::MAX }; let hrmp_outgoing = [(para_0, para_0_limits), (para_1, para_1_limits)].into(); - let limits = TotalBandwidthLimits { + let limits = OutboundBandwidthLimits { ump_messages_remaining: 0, ump_bytes_remaining: 0, hrmp_outgoing, diff --git a/pallets/parachain-system/src/validate_block/tests.rs b/pallets/parachain-system/src/validate_block/tests.rs index 2c39188a085..9cb2f011b55 100644 --- a/pallets/parachain-system/src/validate_block/tests.rs +++ b/pallets/parachain-system/src/validate_block/tests.rs @@ -18,7 +18,10 @@ use codec::{Decode, DecodeAll, Encode}; use cumulus_primitives_core::{ParachainBlockData, PersistedValidationData}; use cumulus_test_client::{ generate_extrinsic, - runtime::{Block, Hash, Header, TestPalletCall, UncheckedExtrinsic, WASM_BINARY}, + runtime::{ + self as test_runtime, Block, Hash, Header, TestPalletCall, UncheckedExtrinsic, + WASM_BINARY, + }, transfer, BlockData, BuildParachainBlockData, Client, DefaultTestClientBuilderExt, HeadData, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt, ValidationParams, }; @@ -79,8 +82,10 @@ fn build_block_with_witness( client: &Client, extra_extrinsics: Vec, parent_head: Header, - sproof_builder: RelayStateSproofBuilder, + mut sproof_builder: RelayStateSproofBuilder, ) -> TestBlockData { + sproof_builder.para_id = test_runtime::PARACHAIN_ID.into(); + sproof_builder.included_para_head = Some(HeadData(parent_head.encode())); let (relay_parent_storage_root, _) = sproof_builder.clone().into_state_root_and_proof(); let mut validation_data = PersistedValidationData { relay_parent_number: 1, diff --git a/test/runtime/src/lib.rs b/test/runtime/src/lib.rs index 9d7e9f078da..0886f528905 100644 --- a/test/runtime/src/lib.rs +++ b/test/runtime/src/lib.rs @@ -76,6 +76,9 @@ impl_opaque_keys! { /// [`OnRuntimeUpgrade`] works as expected. pub const TEST_RUNTIME_UPGRADE_KEY: &[u8] = b"+test_runtime_upgrade_key+"; +/// The para-id used in this runtime. +pub const PARACHAIN_ID: u32 = 100; + // The only difference between the two declarations below is the `spec_version`. With the // `increment-spec-version` feature enabled `spec_version` should be greater than the one of without the // `increment-spec-version` feature. @@ -274,10 +277,11 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = (); type ReservedXcmpWeight = (); type CheckAssociatedRelayNumber = cumulus_pallet_parachain_system::AnyRelayNumber; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::RequireParentIncluded; } parameter_types! { - pub storage ParachainId: cumulus_primitives_core::ParaId = 100.into(); + pub storage ParachainId: cumulus_primitives_core::ParaId = PARACHAIN_ID.into(); } impl test_pallet::Config for Runtime {} From cc0ff1c8f2ff6b24422d6fbaeabea864d6e4613e Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 1 May 2023 19:23:53 -0500 Subject: [PATCH 02/19] add docs --- pallets/parachain-system/src/consensus_hook.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pallets/parachain-system/src/consensus_hook.rs b/pallets/parachain-system/src/consensus_hook.rs index d9f49a69434..850e2182aa0 100644 --- a/pallets/parachain-system/src/consensus_hook.rs +++ b/pallets/parachain-system/src/consensus_hook.rs @@ -58,7 +58,10 @@ impl From for UnincludedSegmentCapacity { /// desired unincluded segment length, as well as any rules for adapting it dynamically /// according to the relay-chain state. pub trait ConsensusHook { - // TODO [now]: docs. cover where this is called, invariants, etc + /// This hook is called partway through the `set_validation_data` inherent in parachain-system. + /// + /// The hook is allowed to panic if customized consensus rules aren't met and is required + /// to return a maximum capacity for the unincluded segment. fn on_state_proof( state_proof: &RelayChainStateProof, ) -> UnincludedSegmentCapacity; From 3ce29a617866622e86fdcc4d11b72dc4bf83645e Mon Sep 17 00:00:00 2001 From: asynchronous rob Date: Tue, 2 May 2023 16:47:11 -0500 Subject: [PATCH 03/19] refactor bandwidth_out calculation Co-authored-by: Chris Sosnin <48099298+slumber@users.noreply.github.com> --- pallets/parachain-system/src/lib.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 3e76cf583da..c79346ce4c1 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -253,12 +253,11 @@ pub mod pallet { host_config.max_upward_queue_count, host_config.max_upward_queue_size, ); - let mut bandwidth_out = None; - if let Some(segment) = AggregatedUnincludedSegment::::get() { - let mut b = total_bandwidth_out.clone(); - b.subtract(segment.used_bandwidth()); - bandwidth_out = Some(b); - } + let bandwidth_out = AggregatedUnincludedSegment::::get().map(|segment| { + let mut bandwidth_out = total_bandwidth_out.clone(); + bandwidth_out.subtract(segment.used_bandwidth()); + bandwidth_out + }); let (ump_msg_count, ump_total_bytes) = >::mutate(|up| { let bandwidth_out = bandwidth_out.as_ref().unwrap_or(&total_bandwidth_out); From f7da3a9f5fc6fea84ad47bffab160e40b5caa0b3 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Fri, 5 May 2023 17:54:14 +0400 Subject: [PATCH 04/19] test for limits from impl --- .../src/unincluded_segment.rs | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index dea24638bce..d13ebf1ab50 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -345,6 +345,71 @@ mod tests { use super::*; use assert_matches::assert_matches; + #[test] + fn outbound_limits_constructed_correctly() { + let para_a = ParaId::from(0); + let para_a_channel = relay_chain::AbridgedHrmpChannel { + max_message_size: 15, + + // Msg count capacity left is 2. + msg_count: 5, + max_capacity: 7, + + // Bytes capacity left is 10. + total_size: 50, + max_total_size: 60, + mqc_head: None, + }; + + let para_b = ParaId::from(1); + let para_b_channel = relay_chain::AbridgedHrmpChannel { + max_message_size: 15, + + // Msg count capacity left is 10. + msg_count: 40, + max_capacity: 50, + + // Bytes capacity left is 0. + total_size: 500, + max_total_size: 500, + mqc_head: None, + }; + let messaging_state = MessagingStateSnapshot { + dmq_mqc_head: relay_chain::Hash::zero(), + // (msg_count, bytes) + relay_dispatch_queue_size: (10, 100), + ingress_channels: Vec::new(), + + egress_channels: vec![(para_a, para_a_channel), (para_b, para_b_channel)], + }; + + let max_upward_queue_count = 11; + let max_upward_queue_size = 150; + let limits = OutboundBandwidthLimits::from_relay_chain_state( + &messaging_state, max_upward_queue_count, max_upward_queue_size + ); + + // UMP. + assert_eq!(limits.ump_messages_remaining, 1); + assert_eq!(limits.ump_bytes_remaining, 50); + + // HRMP. + let para_a_limits = limits.hrmp_outgoing.get(¶_a).expect("channel must be present"); + let para_b_limits = limits.hrmp_outgoing.get(¶_b).expect("channel must be present"); + assert_eq!( + para_a_limits.bytes_remaining, 10 + ); + assert_eq!( + para_a_limits.messages_remaining, 2 + ); + assert_eq!( + para_b_limits.bytes_remaining, 0 + ); + assert_eq!( + para_b_limits.messages_remaining, 10 + ); + } + #[test] fn hrmp_msg_count_limits() { let para_0 = ParaId::from(0); From 857961255f87b191f7983e0f2c1df082f6e4a80c Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Fri, 5 May 2023 17:54:34 +0400 Subject: [PATCH 05/19] fmt --- .../parachain-system/src/consensus_hook.rs | 12 ++----- pallets/parachain-system/src/lib.rs | 24 ++++++-------- pallets/parachain-system/src/tests.rs | 22 ++++++------- .../src/unincluded_segment.rs | 32 ++++++++----------- .../src/validate_block/tests.rs | 3 +- 5 files changed, 38 insertions(+), 55 deletions(-) diff --git a/pallets/parachain-system/src/consensus_hook.rs b/pallets/parachain-system/src/consensus_hook.rs index 850e2182aa0..8d3606d14b8 100644 --- a/pallets/parachain-system/src/consensus_hook.rs +++ b/pallets/parachain-system/src/consensus_hook.rs @@ -62,9 +62,7 @@ pub trait ConsensusHook { /// /// The hook is allowed to panic if customized consensus rules aren't met and is required /// to return a maximum capacity for the unincluded segment. - fn on_state_proof( - state_proof: &RelayChainStateProof, - ) -> UnincludedSegmentCapacity; + fn on_state_proof(state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity; } /// A special consensus hook for handling the migration to asynchronous backing gracefully, @@ -77,9 +75,7 @@ pub trait ConsensusHook { pub struct ExpectParentIncluded; impl ConsensusHook for ExpectParentIncluded { - fn on_state_proof( - _state_proof: &RelayChainStateProof, - ) -> UnincludedSegmentCapacity { + fn on_state_proof(_state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity { UnincludedSegmentCapacity(UnincludedSegmentCapacityInner::ExpectParentIncluded) } } @@ -92,9 +88,7 @@ impl ConsensusHook for ExpectParentIncluded { pub struct FixedCapacityUnincludedSegment; impl ConsensusHook for FixedCapacityUnincludedSegment { - fn on_state_proof( - _state_proof: &RelayChainStateProof, - ) -> UnincludedSegmentCapacity { + fn on_state_proof(_state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity { NonZeroU32::new(sp_std::cmp::max(N, 1)) .expect("1 is the minimum value and non-zero; qed") .into() diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index c79346ce4c1..9332c6c2a0c 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -59,9 +59,9 @@ use xcm::latest::XcmHash; mod migration; mod relay_state_snapshot; -mod unincluded_segment; #[cfg(test)] mod tests; +mod unincluded_segment; pub mod consensus_hook; #[macro_use] @@ -71,6 +71,7 @@ use unincluded_segment::{ Ancestor, HrmpChannelUpdate, OutboundBandwidthLimits, SegmentTracker, UsedBandwidth, }; +pub use consensus_hook::ConsensusHook; /// Register the `validate_block` function that is used by parachains to validate blocks on a /// validator. /// @@ -94,7 +95,6 @@ use unincluded_segment::{ /// # fn main() {} /// ``` pub use cumulus_pallet_parachain_system_proc_macro::register_validate_block; -pub use consensus_hook::ConsensusHook; pub use relay_state_snapshot::{MessagingStateSnapshot, RelayChainStateProof}; pub use pallet::*; @@ -247,7 +247,6 @@ pub mod pallet { }, }; - let total_bandwidth_out = OutboundBandwidthLimits::from_relay_chain_state( &relevant_messaging_state, host_config.max_upward_queue_count, @@ -1089,10 +1088,8 @@ impl Pallet { ) -> Weight { let mut weight_used = Weight::zero(); // If the unincluded segment length is nonzero, then the parachain head must be present. - let para_head = relay_state_proof - .read_included_para_head() - .ok() - .map(|h| T::Hashing::hash(&h.0)); + let para_head = + relay_state_proof.read_included_para_head().ok().map(|h| T::Hashing::hash(&h.0)); let unincluded_segment_len = >::decode_len().unwrap_or(0); weight_used += T::DbWeight::get().reads(1); @@ -1107,7 +1104,7 @@ impl Pallet { ); h - } + }, (Some(h), false) => h, (None, true) => { // All this logic is essentially a workaround to support collators which @@ -1124,9 +1121,9 @@ impl Pallet { let idx = chain .iter() .position(|block| { - let head_hash = block.para_head_hash().expect( - "para head hash is updated during block initialization; qed", - ); + let head_hash = block + .para_head_hash() + .expect("para head hash is updated during block initialization; qed"); head_hash == ¶_head_hash }) .map_or(0, |idx| idx + 1); // inclusive. @@ -1155,10 +1152,7 @@ impl Pallet { // // If this fails, the parachain needs to wait for ancestors to be included before // a new block is allowed. - assert!( - new_len < capacity.get(), - "no space left for the block in the unincluded segment" - ); + assert!(new_len < capacity.get(), "no space left for the block in the unincluded segment"); weight_used } diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 410e5c21ad2..658d09d7316 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -633,17 +633,17 @@ fn send_upward_message_num_per_candidate() { assert_eq!(v, vec![b"Mr F was here".to_vec()]); }, ); - // .add_with_post_test( - // 2, - // || { - // assert_eq!(UnincludedSegment::::get().len(), 0); - // /* do nothing within block */ - // }, - // || { - // let v = UpwardMessages::::get(); - // assert_eq!(v, vec![b"message 2".to_vec()]); - // }, - // ); + // .add_with_post_test( + // 2, + // || { + // assert_eq!(UnincludedSegment::::get().len(), 0); + // /* do nothing within block */ + // }, + // || { + // let v = UpwardMessages::::get(); + // assert_eq!(v, vec![b"message 2".to_vec()]); + // }, + // ); } #[test] diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index d13ebf1ab50..6e4b9ebe856 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -56,8 +56,7 @@ impl OutboundBandwidthLimits { max_upward_queue_count: u32, max_upward_queue_size: u32, ) -> Self { - let (ump_messages_in_relay, ump_bytes_in_relay) = - messaging_state.relay_dispatch_queue_size; + let (ump_messages_in_relay, ump_bytes_in_relay) = messaging_state.relay_dispatch_queue_size; let (ump_messages_remaining, ump_bytes_remaining) = ( max_upward_queue_count.saturating_sub(ump_messages_in_relay), @@ -83,12 +82,15 @@ impl OutboundBandwidthLimits { /// Compute the remaining bandwidth when accounting for the used amounts provided. pub fn subtract(&mut self, used: &UsedBandwidth) { - self.ump_messages_remaining = self.ump_messages_remaining.saturating_sub(used.ump_msg_count); + self.ump_messages_remaining = + self.ump_messages_remaining.saturating_sub(used.ump_msg_count); self.ump_bytes_remaining = self.ump_bytes_remaining.saturating_sub(used.ump_total_bytes); for (para_id, channel_limits) in self.hrmp_outgoing.iter_mut() { if let Some(update) = used.hrmp_outgoing.get(para_id) { - channel_limits.bytes_remaining = channel_limits.bytes_remaining.saturating_sub(update.total_bytes); - channel_limits.messages_remaining = channel_limits.messages_remaining.saturating_sub(update.msg_count); + channel_limits.bytes_remaining = + channel_limits.bytes_remaining.saturating_sub(update.total_bytes); + channel_limits.messages_remaining = + channel_limits.messages_remaining.saturating_sub(update.msg_count); } } } @@ -386,7 +388,9 @@ mod tests { let max_upward_queue_count = 11; let max_upward_queue_size = 150; let limits = OutboundBandwidthLimits::from_relay_chain_state( - &messaging_state, max_upward_queue_count, max_upward_queue_size + &messaging_state, + max_upward_queue_count, + max_upward_queue_size, ); // UMP. @@ -396,18 +400,10 @@ mod tests { // HRMP. let para_a_limits = limits.hrmp_outgoing.get(¶_a).expect("channel must be present"); let para_b_limits = limits.hrmp_outgoing.get(¶_b).expect("channel must be present"); - assert_eq!( - para_a_limits.bytes_remaining, 10 - ); - assert_eq!( - para_a_limits.messages_remaining, 2 - ); - assert_eq!( - para_b_limits.bytes_remaining, 0 - ); - assert_eq!( - para_b_limits.messages_remaining, 10 - ); + assert_eq!(para_a_limits.bytes_remaining, 10); + assert_eq!(para_a_limits.messages_remaining, 2); + assert_eq!(para_b_limits.bytes_remaining, 0); + assert_eq!(para_b_limits.messages_remaining, 10); } #[test] diff --git a/pallets/parachain-system/src/validate_block/tests.rs b/pallets/parachain-system/src/validate_block/tests.rs index 9cb2f011b55..8e61481193a 100644 --- a/pallets/parachain-system/src/validate_block/tests.rs +++ b/pallets/parachain-system/src/validate_block/tests.rs @@ -19,8 +19,7 @@ use cumulus_primitives_core::{ParachainBlockData, PersistedValidationData}; use cumulus_test_client::{ generate_extrinsic, runtime::{ - self as test_runtime, Block, Hash, Header, TestPalletCall, UncheckedExtrinsic, - WASM_BINARY, + self as test_runtime, Block, Hash, Header, TestPalletCall, UncheckedExtrinsic, WASM_BINARY, }, transfer, BlockData, BuildParachainBlockData, Client, DefaultTestClientBuilderExt, HeadData, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt, ValidationParams, From 83a25ed20e7e5771c436a714ae0d84b332597169 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Fri, 5 May 2023 20:09:47 +0400 Subject: [PATCH 06/19] make tests compile --- pallets/xcmp-queue/src/mock.rs | 1 + parachain-template/runtime/src/lib.rs | 1 + parachains/runtimes/assets/statemine/src/lib.rs | 1 + parachains/runtimes/assets/statemint/src/lib.rs | 1 + parachains/runtimes/assets/westmint/src/lib.rs | 1 + parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs | 1 + parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs | 1 + parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs | 1 + parachains/runtimes/collectives/collectives-polkadot/src/lib.rs | 1 + parachains/runtimes/contracts/contracts-rococo/src/lib.rs | 1 + parachains/runtimes/starters/seedling/src/lib.rs | 1 + parachains/runtimes/starters/shell/src/lib.rs | 1 + parachains/runtimes/testing/penpal/src/lib.rs | 1 + parachains/runtimes/testing/rococo-parachain/src/lib.rs | 1 + 14 files changed, 14 insertions(+) diff --git a/pallets/xcmp-queue/src/mock.rs b/pallets/xcmp-queue/src/mock.rs index 0d7d6eda00b..084ed98f372 100644 --- a/pallets/xcmp-queue/src/mock.rs +++ b/pallets/xcmp-queue/src/mock.rs @@ -116,6 +116,7 @@ impl cumulus_pallet_parachain_system::Config for Test { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = (); type CheckAssociatedRelayNumber = AnyRelayNumber; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } parameter_types! { diff --git a/parachain-template/runtime/src/lib.rs b/parachain-template/runtime/src/lib.rs index 61839e0a621..c45401f2a05 100644 --- a/parachain-template/runtime/src/lib.rs +++ b/parachain-template/runtime/src/lib.rs @@ -375,6 +375,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/assets/statemine/src/lib.rs b/parachains/runtimes/assets/statemine/src/lib.rs index 5f987851a18..06aed9d59b1 100644 --- a/parachains/runtimes/assets/statemine/src/lib.rs +++ b/parachains/runtimes/assets/statemine/src/lib.rs @@ -521,6 +521,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/assets/statemint/src/lib.rs b/parachains/runtimes/assets/statemint/src/lib.rs index aa90ca7a157..ac4a369b3ca 100644 --- a/parachains/runtimes/assets/statemint/src/lib.rs +++ b/parachains/runtimes/assets/statemint/src/lib.rs @@ -469,6 +469,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/assets/westmint/src/lib.rs b/parachains/runtimes/assets/westmint/src/lib.rs index c237c8dc5cf..6e9c0e83f29 100644 --- a/parachains/runtimes/assets/westmint/src/lib.rs +++ b/parachains/runtimes/assets/westmint/src/lib.rs @@ -506,6 +506,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs b/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs index 4ebf760e849..a7ece2d4045 100644 --- a/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs +++ b/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs @@ -286,6 +286,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs b/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs index 109cd2434b3..50d987ae1ec 100644 --- a/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs +++ b/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs @@ -286,6 +286,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs b/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs index 04eddf59e3a..ac6b0117334 100644 --- a/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs +++ b/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs @@ -353,6 +353,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/collectives/collectives-polkadot/src/lib.rs b/parachains/runtimes/collectives/collectives-polkadot/src/lib.rs index e0f95cb052b..6ab122c2bc3 100644 --- a/parachains/runtimes/collectives/collectives-polkadot/src/lib.rs +++ b/parachains/runtimes/collectives/collectives-polkadot/src/lib.rs @@ -370,6 +370,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/contracts/contracts-rococo/src/lib.rs b/parachains/runtimes/contracts/contracts-rococo/src/lib.rs index 819bb7ba537..57a85f20b64 100644 --- a/parachains/runtimes/contracts/contracts-rococo/src/lib.rs +++ b/parachains/runtimes/contracts/contracts-rococo/src/lib.rs @@ -266,6 +266,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl pallet_insecure_randomness_collective_flip::Config for Runtime {} diff --git a/parachains/runtimes/starters/seedling/src/lib.rs b/parachains/runtimes/starters/seedling/src/lib.rs index 2861cac1fe4..97b18cd6f3f 100644 --- a/parachains/runtimes/starters/seedling/src/lib.rs +++ b/parachains/runtimes/starters/seedling/src/lib.rs @@ -176,6 +176,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = (); type ReservedXcmpWeight = (); type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/starters/shell/src/lib.rs b/parachains/runtimes/starters/shell/src/lib.rs index a05a78863c4..a896611bf46 100644 --- a/parachains/runtimes/starters/shell/src/lib.rs +++ b/parachains/runtimes/starters/shell/src/lib.rs @@ -180,6 +180,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = (); type ReservedXcmpWeight = (); type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/testing/penpal/src/lib.rs b/parachains/runtimes/testing/penpal/src/lib.rs index 7ade3bd2f63..0ecb0c6794d 100644 --- a/parachains/runtimes/testing/penpal/src/lib.rs +++ b/parachains/runtimes/testing/penpal/src/lib.rs @@ -458,6 +458,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} diff --git a/parachains/runtimes/testing/rococo-parachain/src/lib.rs b/parachains/runtimes/testing/rococo-parachain/src/lib.rs index 1a26290f2a3..4feb14bb4fd 100644 --- a/parachains/runtimes/testing/rococo-parachain/src/lib.rs +++ b/parachains/runtimes/testing/rococo-parachain/src/lib.rs @@ -272,6 +272,7 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type XcmpMessageHandler = XcmpQueue; type ReservedXcmpWeight = ReservedXcmpWeight; type CheckAssociatedRelayNumber = RelayNumberStrictlyIncreases; + type ConsensusHook = cumulus_pallet_parachain_system::consensus_hook::ExpectParentIncluded; } impl parachain_info::Config for Runtime {} From 0930d4e7aa0eb16b2067d34e291adbfcfe36e151 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 11:55:19 -0500 Subject: [PATCH 07/19] update comment --- pallets/parachain-system/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 658d09d7316..7415139af5a 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -466,7 +466,7 @@ fn unincluded_segment_works() { || {}, || { let segment = >::get(); - // Block 123 was popped from the segment, the len is still 1. + // Block 123 was popped from the segment, the len is still 2. assert_eq!(segment.len(), 2); }, ); From 5d5d165f009161c944e9f017302927b052211b8a Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 12:00:48 -0500 Subject: [PATCH 08/19] uncomment test --- pallets/parachain-system/src/tests.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 7415139af5a..e1010f89be0 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -632,18 +632,18 @@ fn send_upward_message_num_per_candidate() { let v = UpwardMessages::::get(); assert_eq!(v, vec![b"Mr F was here".to_vec()]); }, + ) + .add_with_post_test( + 2, + || { + assert_eq!(UnincludedSegment::::get().len(), 0); + /* do nothing within block */ + }, + || { + let v = UpwardMessages::::get(); + assert_eq!(v, vec![b"message 2".to_vec()]); + }, ); - // .add_with_post_test( - // 2, - // || { - // assert_eq!(UnincludedSegment::::get().len(), 0); - // /* do nothing within block */ - // }, - // || { - // let v = UpwardMessages::::get(); - // assert_eq!(v, vec![b"message 2".to_vec()]); - // }, - // ); } #[test] From b67b0d6bbc310b8e84d9981bf228b35fc1e8552d Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 12:10:08 -0500 Subject: [PATCH 09/19] fix collator test by adding parent to state proof --- Cargo.lock | 1 + client/collator/Cargo.toml | 1 + client/collator/src/lib.rs | 8 +++++++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 10b520c0ad7..d6570175d5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1920,6 +1920,7 @@ dependencies = [ "cumulus-client-network", "cumulus-primitives-core", "cumulus-test-client", + "cumulus-test-relay-sproof-builder", "cumulus-test-runtime", "futures", "parity-scale-codec 3.4.0", diff --git a/client/collator/Cargo.toml b/client/collator/Cargo.toml index 19e3578d153..95ecf67acac 100644 --- a/client/collator/Cargo.toml +++ b/client/collator/Cargo.toml @@ -42,3 +42,4 @@ polkadot-node-subsystem-test-helpers = { git = "https://github.com/paritytech/po # Cumulus cumulus-test-client = { path = "../../test/client" } cumulus-test-runtime = { path = "../../test/runtime" } +cumulus-test-relay-sproof-builder = { path = "../../test/relay-sproof-builder" } diff --git a/client/collator/src/lib.rs b/client/collator/src/lib.rs index a931201f6cc..5ef2230129f 100644 --- a/client/collator/src/lib.rs +++ b/client/collator/src/lib.rs @@ -385,9 +385,11 @@ mod tests { TestClientBuilder, TestClientBuilderExt, }; use cumulus_test_runtime::{Block, Header}; + use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder; use futures::{channel::mpsc, executor::block_on, StreamExt}; use polkadot_node_subsystem_test_helpers::ForwardSubsystem; use polkadot_overseer::{dummy::dummy_overseer_builder, HeadSupportsParachains}; + use polkadot_primitives::HeadData; use sp_consensus::BlockOrigin; use sp_core::{testing::TaskExecutor, Pair}; use sp_runtime::traits::BlakeTwo256; @@ -415,10 +417,14 @@ mod tests { _: PHash, validation_data: &PersistedValidationData, ) -> Option> { + let mut sproof = RelayStateSproofBuilder::default(); + sproof.included_para_head = Some(HeadData(parent.encode())); + sproof.para_id = cumulus_test_runtime::PARACHAIN_ID.into(); + let builder = self.client.init_block_builder_at( parent.hash(), Some(validation_data.clone()), - Default::default(), + sproof, ); let (block, _, proof) = builder.build().expect("Creates block").into_inner(); From c0c0edc2793b3255c710b54f5a005d334032fd26 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 15:06:23 -0500 Subject: [PATCH 10/19] patch HRMP watermark rules for unincluded segment --- pallets/parachain-system/src/lib.rs | 11 +++-- .../src/unincluded_segment.rs | 46 ++++++++++++++++--- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 9332c6c2a0c..dbcb5048b18 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -68,7 +68,8 @@ pub mod consensus_hook; pub mod validate_block; use unincluded_segment::{ - Ancestor, HrmpChannelUpdate, OutboundBandwidthLimits, SegmentTracker, UsedBandwidth, + Ancestor, HrmpChannelUpdate, HrmpWatermarkUpdate, OutboundBandwidthLimits, SegmentTracker, + UsedBandwidth, }; pub use consensus_hook::ConsensusHook; @@ -336,12 +337,16 @@ pub mod pallet { let ancestor = Ancestor::new_unchecked(used_bandwidth); let watermark = HrmpWatermark::::get(); + let watermark_update = HrmpWatermarkUpdate::new( + watermark, + LastRelayChainBlockNumber::::get(), + ); AggregatedUnincludedSegment::::mutate(|agg| { let agg = agg.get_or_insert_with(SegmentTracker::default); // TODO: In order of this panic to be correct, outbound message source should // respect bandwidth limits as well. // - agg.append(&ancestor, watermark, &total_bandwidth_out) + agg.append(&ancestor, watermark_update, &total_bandwidth_out) .expect("unincluded segment limits exceeded"); }); // Check in `on_initialize` guarantees there's space for this block. @@ -374,7 +379,7 @@ pub mod pallet { weight += T::DbWeight::get().reads_writes(1, 1); // Weight used during finalization. - weight += T::DbWeight::get().reads_writes(2, 2); + weight += T::DbWeight::get().reads_writes(3, 2); } // Remove the validation from the old block. diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index 6e4b9ebe856..610b68118d2 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -292,6 +292,34 @@ impl Ancestor { } } +/// An update to the HRMP watermark. This is always a relay-chain block number, +/// but the two variants have different semantic meanings. +pub enum HrmpWatermarkUpdate { + /// An update to the HRMP watermark where the new value is set to be equal to the + /// relay-parent's block number, i.e. the "head" of the relay chain. + /// This is always legal. + Head(relay_chain::BlockNumber), + /// An update to the HRMP watermark where the new value falls into the "trunk" of the + /// relay-chain. In this case, the watermark must be greater than the previous value. + Trunk(relay_chain::BlockNumber), +} + +impl HrmpWatermarkUpdate { + /// Create a new update based on the desired watermark value and the current + /// relay-parent number. + pub fn new( + watermark: relay_chain::BlockNumber, + relay_parent_number: relay_chain::BlockNumber, + ) -> Self { + // Hard constrain the watermark to the relay-parent number. + if watermark >= relay_parent_number { + HrmpWatermarkUpdate::Head(relay_parent_number) + } else { + HrmpWatermarkUpdate::Trunk(watermark) + } + } +} + /// Struct that keeps track of bandwidth used by the unincluded part of the chain /// along with the latest HRMP watermark. #[derive(Default, Encode, Decode, TypeInfo)] @@ -311,20 +339,24 @@ impl SegmentTracker { pub fn append( &mut self, block: &Ancestor, - hrmp_watermark: relay_chain::BlockNumber, + new_watermark: HrmpWatermarkUpdate, limits: &OutboundBandwidthLimits, ) -> Result<(), BandwidthUpdateError> { if let Some(watermark) = self.hrmp_watermark.as_ref() { - if &hrmp_watermark <= watermark { - return Err(BandwidthUpdateError::InvalidHrmpWatermark { - submitted: hrmp_watermark, - latest: *watermark, - }) + if let HrmpWatermarkUpdate::Trunk(new) = new_watermark { + if &new <= watermark { + return Err(BandwidthUpdateError::InvalidHrmpWatermark { + submitted: new, + latest: *watermark, + }) + } } } self.used_bandwidth = self.used_bandwidth.append(block.used_bandwidth(), limits)?; - self.hrmp_watermark.replace(hrmp_watermark); + self.hrmp_watermark.replace(match new_watermark { + HrmpWatermarkUpdate::Trunk(w) | HrmpWatermarkUpdate::Head(w) => w, + }); Ok(()) } From 9b4b2f7ac224a40afd0f3f6f2eb61c2af1e22968 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 15:15:38 -0500 Subject: [PATCH 11/19] get consensus-common tests to pass, using unincluded segment --- Cargo.lock | 1 + client/consensus/common/Cargo.toml | 1 + client/consensus/common/src/tests.rs | 56 +++++++++++++++++++++++----- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d6570175d5b..93b69a38be1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1978,6 +1978,7 @@ dependencies = [ "cumulus-primitives-core", "cumulus-relay-chain-interface", "cumulus-test-client", + "cumulus-test-relay-sproof-builder", "dyn-clone", "futures", "futures-timer", diff --git a/client/consensus/common/Cargo.toml b/client/consensus/common/Cargo.toml index 12fa2099a84..c606ff27417 100644 --- a/client/consensus/common/Cargo.toml +++ b/client/consensus/common/Cargo.toml @@ -38,3 +38,4 @@ sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master # Cumulus cumulus-test-client = { path = "../../../test/client" } +cumulus-test-relay-sproof-builder = { path = "../../../test/relay-sproof-builder" } diff --git a/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index 23516d96388..b73f79ee40c 100644 --- a/client/consensus/common/src/tests.rs +++ b/client/consensus/common/src/tests.rs @@ -28,11 +28,13 @@ use cumulus_test_client::{ runtime::{Block, Hash, Header}, Backend, Client, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt, }; +use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder; use futures::{channel::mpsc, executor::block_on, select, FutureExt, Stream, StreamExt}; use futures_timer::Delay; use sc_client_api::{blockchain::Backend as _, Backend as _, UsageProvider}; use sc_consensus::{BlockImport, BlockImportParams, ForkChoiceStrategy}; use sp_consensus::{BlockOrigin, BlockStatus}; +use polkadot_primitives::HeadData; use std::{ collections::{BTreeMap, HashMap}, pin::Pin, @@ -209,17 +211,36 @@ impl RelayChainInterface for Relaychain { } } +fn sproof_with_best_parent(client: &Client) -> RelayStateSproofBuilder { + let best_hash = client.chain_info().best_hash; + sproof_with_parent_by_hash(client, best_hash) +} + +fn sproof_with_parent_by_hash(client: &Client, hash: PHash) -> RelayStateSproofBuilder { + let header = client.header(hash).ok().flatten().expect("No header for parent block"); + sproof_with_parent(HeadData(header.encode())) +} + +fn sproof_with_parent(parent: HeadData) -> RelayStateSproofBuilder { + let mut x = RelayStateSproofBuilder::default(); + x.para_id = cumulus_test_client::runtime::PARACHAIN_ID.into(); + x.included_para_head = Some(parent); + + x +} + fn build_block( builder: &B, + sproof: RelayStateSproofBuilder, at: Option, timestamp: Option, ) -> Block { let builder = match at { Some(at) => match timestamp { - Some(ts) => builder.init_block_builder_with_timestamp(at, None, Default::default(), ts), - None => builder.init_block_builder_at(at, None, Default::default()), + Some(ts) => builder.init_block_builder_with_timestamp(at, None, sproof, ts), + None => builder.init_block_builder_at(at, None, sproof), }, - None => builder.init_block_builder(None, Default::default()), + None => builder.init_block_builder(None, sproof), }; let mut block = builder.build().unwrap().block; @@ -260,15 +281,20 @@ fn import_block_sync>( block_on(import_block(importer, block, origin, import_as_best)); } -fn build_and_import_block_ext>( - builder: &B, +fn build_and_import_block_ext>( + client: &Client, origin: BlockOrigin, import_as_best: bool, importer: &mut I, at: Option, timestamp: Option, ) -> Block { - let block = build_block(builder, at, timestamp); + let sproof = match at { + None => sproof_with_best_parent(client), + Some(at) => sproof_with_parent_by_hash(client, at), + }; + + let block = build_block(client, sproof, at, timestamp); import_block_sync(importer, block.clone(), origin, import_as_best); block } @@ -337,7 +363,12 @@ fn follow_new_best_with_dummy_recovery_works() { Some(recovery_chan_tx), ); - let block = build_block(&*client.clone(), None, None); + let sproof = { + let best = client.chain_info().best_hash; + let header = client.header(best).ok().flatten().expect("No header for best"); + sproof_with_parent(HeadData(header.encode())) + }; + let block = build_block(&*client.clone(), sproof, None, None); let block_clone = block.clone(); let client_clone = client.clone(); @@ -423,7 +454,8 @@ fn follow_finalized_does_not_stop_on_unknown_block() { let block = build_and_import_block(client.clone(), false); let unknown_block = { - let block_builder = client.init_block_builder_at(block.hash(), None, Default::default()); + let sproof = sproof_with_parent_by_hash(&*client, block.hash()); + let block_builder = client.init_block_builder_at(block.hash(), None, sproof); block_builder.build().unwrap().block }; @@ -472,7 +504,8 @@ fn follow_new_best_sets_best_after_it_is_imported() { let block = build_and_import_block(client.clone(), false); let unknown_block = { - let block_builder = client.init_block_builder_at(block.hash(), None, Default::default()); + let sproof = sproof_with_parent_by_hash(&*client, block.hash()); + let block_builder = client.init_block_builder_at(block.hash(), None, sproof); block_builder.build().unwrap().block }; @@ -548,7 +581,10 @@ fn do_not_set_best_block_to_older_block() { let blocks = (0..NUM_BLOCKS) .into_iter() - .map(|_| build_and_import_block(client.clone(), true)) + .map(|i| { + println!("{}", i); + build_and_import_block(client.clone(), true) + }) .collect::>(); assert_eq!(NUM_BLOCKS as u32, client.usage_info().chain.best_number); From fdf948b28262daafadfe02d19ba9e79e718227c3 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 15:32:26 -0500 Subject: [PATCH 12/19] fix unincluded segment tests --- .../src/unincluded_segment.rs | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index 610b68118d2..e73d43fb52e 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -458,7 +458,7 @@ mod tests { for _ in 0..5 { hrmp_update = hrmp_update .append(&HrmpChannelUpdate { msg_count: 1, total_bytes: 10 }, para_0, &limits) - .expect("update is withing the limits"); + .expect("update is within the limits"); } assert_matches!( hrmp_update.append( @@ -476,7 +476,7 @@ mod tests { let mut hrmp_update = HrmpChannelUpdate::default(); hrmp_update = hrmp_update .append(&HrmpChannelUpdate { msg_count: 2, total_bytes: 10 }, para_1, &limits) - .expect("update is withing the limits"); + .expect("update is within the limits"); assert_matches!( hrmp_update.append( &HrmpChannelUpdate { msg_count: 3, total_bytes: 10 }, @@ -510,7 +510,7 @@ mod tests { for _ in 0..5 { hrmp_update = hrmp_update .append(&HrmpChannelUpdate { msg_count: 1, total_bytes: 4 }, para_0, &limits) - .expect("update is withing the limits"); + .expect("update is within the limits"); } assert_matches!( hrmp_update.append( @@ -550,7 +550,8 @@ mod tests { used_bandwidth: create_used_hrmp([(para_0, para_0_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_0, 0, &limits).expect("update is withing the limits"); + segment.append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) + .expect("update is within the limits"); for watermark in 1..5 { let ancestor = Ancestor { @@ -558,8 +559,8 @@ mod tests { para_head_hash: None::, }; segment - .append(&ancestor, watermark, &limits) - .expect("update is withing the limits"); + .append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits) + .expect("update is within the limits"); } let para_0_update = HrmpChannelUpdate { msg_count: 1, total_bytes: 1 }; @@ -568,7 +569,7 @@ mod tests { para_head_hash: None::, }; assert_matches!( - segment.append(&ancestor_5, 5, &limits), + segment.append(&ancestor_5, HrmpWatermarkUpdate::Trunk(5), &limits), Err(BandwidthUpdateError::HrmpBytesOverflow { recipient, bytes_remaining, @@ -577,17 +578,18 @@ mod tests { ); // Remove the first ancestor from the segment to make space. segment.subtract(&ancestor_0); - segment.append(&ancestor_5, 5, &limits).expect("update is withing the limits"); + segment.append(&ancestor_5, HrmpWatermarkUpdate::Trunk(5), &limits).expect("update is within the limits"); let para_1_update = HrmpChannelUpdate { msg_count: 3, total_bytes: 10 }; let ancestor = Ancestor { used_bandwidth: create_used_hrmp([(para_1, para_1_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor, 6, &limits).expect("update is withing the limits"); + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(6), &limits) + .expect("update is within the limits"); assert_matches!( - segment.append(&ancestor, 7, &limits), + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(7), &limits), Err(BandwidthUpdateError::HrmpMessagesOverflow { recipient, messages_remaining, @@ -616,7 +618,8 @@ mod tests { used_bandwidth: create_used_ump((1, 10)), para_head_hash: None::, }; - segment.append(&ancestor_0, 0, &limits).expect("update is withing the limits"); + segment.append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) + .expect("update is within the limits"); for watermark in 1..4 { let ancestor = Ancestor { @@ -624,8 +627,8 @@ mod tests { para_head_hash: None::, }; segment - .append(&ancestor, watermark, &limits) - .expect("update is withing the limits"); + .append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits) + .expect("update is within the limits"); } let ancestor_4 = Ancestor { @@ -633,7 +636,7 @@ mod tests { para_head_hash: None::, }; assert_matches!( - segment.append(&ancestor_4, 4, &limits), + segment.append(&ancestor_4, HrmpWatermarkUpdate::Trunk(4), &limits), Err(BandwidthUpdateError::UmpBytesOverflow { bytes_remaining, bytes_submitted, @@ -644,9 +647,10 @@ mod tests { used_bandwidth: create_used_ump((1, 5)), para_head_hash: None::, }; - segment.append(&ancestor, 4, &limits).expect("update is withing the limits"); + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(4), &limits) + .expect("update is within the limits"); assert_matches!( - segment.append(&ancestor, 5, &limits), + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(5), &limits), Err(BandwidthUpdateError::UmpMessagesOverflow { messages_remaining, messages_submitted, @@ -669,10 +673,10 @@ mod tests { }; segment - .append(&ancestor, 0, &limits) + .append(&ancestor, HrmpWatermarkUpdate::Head(0), &limits) .expect("nothing to compare the watermark with in default segment"); assert_matches!( - segment.append(&ancestor, 0, &limits), + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(0), &limits), Err(BandwidthUpdateError::InvalidHrmpWatermark { submitted, latest, @@ -680,17 +684,19 @@ mod tests { ); for watermark in 1..5 { - segment.append(&ancestor, watermark, &limits).expect("hrmp watermark is valid"); + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits).expect("hrmp watermark is valid"); } for watermark in 0..5 { assert_matches!( - segment.append(&ancestor, watermark, &limits), + segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits), Err(BandwidthUpdateError::InvalidHrmpWatermark { submitted, latest, }) if submitted == watermark && latest == 4 ); } + + segment.append(&ancestor, HrmpWatermarkUpdate::Head(4), &limits).expect("head updates always valid"); } #[test] @@ -719,13 +725,15 @@ mod tests { used_bandwidth: create_used_hrmp([(para_0, para_0_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_0, 0, &limits).expect("update is withing the limits"); + segment.append(&ancestor_0, HrmpWatermarkUpdate::Head(0), &limits) + .expect("update is within the limits"); let para_1_update = HrmpChannelUpdate { msg_count: 3, total_bytes: 10 }; let ancestor_1 = Ancestor { used_bandwidth: create_used_hrmp([(para_1, para_1_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_1, 1, &limits).expect("update is withing the limits"); + segment.append(&ancestor_1, HrmpWatermarkUpdate::Head(1), &limits) + .expect("update is within the limits"); assert_eq!(segment.used_bandwidth.hrmp_outgoing.len(), 2); From 79c56500c649fe48985dcf48c5d7ee4c4299b194 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 18:36:25 -0500 Subject: [PATCH 13/19] get all tests passing --- primitives/timestamp/src/lib.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index ddc2fe340dd..9e9cb755ed4 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -110,11 +110,15 @@ mod tests { timestamp: u64, relay_chain_slot: Slot, ) -> (ParachainBlockData, PHash) { - let sproof_builder = - RelayStateSproofBuilder { current_slot: relay_chain_slot, ..Default::default() }; - let parent_header = client.header(hash).ok().flatten().expect("Genesis header exists"); + let sproof_builder = RelayStateSproofBuilder { + para_id: cumulus_test_client::runtime::PARACHAIN_ID.into(), + included_para_head: Some(HeadData(parent_header.encode())), + current_slot: relay_chain_slot, + ..Default::default() + }; + let relay_parent_storage_root = sproof_builder.clone().into_state_root_and_proof().0; let validation_data = PersistedValidationData { From 8c363b50f7363072411e5e06fd65a3c1e927b6ea Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 18:40:07 -0500 Subject: [PATCH 14/19] fmt --- client/collator/src/lib.rs | 2 +- client/consensus/common/src/tests.rs | 2 +- pallets/parachain-system/src/lib.rs | 6 ++-- .../src/unincluded_segment.rs | 30 +++++++++++++------ primitives/timestamp/src/lib.rs | 2 +- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/client/collator/src/lib.rs b/client/collator/src/lib.rs index 5ef2230129f..13708c4f617 100644 --- a/client/collator/src/lib.rs +++ b/client/collator/src/lib.rs @@ -384,8 +384,8 @@ mod tests { Client, ClientBlockImportExt, DefaultTestClientBuilderExt, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt, }; - use cumulus_test_runtime::{Block, Header}; use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder; + use cumulus_test_runtime::{Block, Header}; use futures::{channel::mpsc, executor::block_on, StreamExt}; use polkadot_node_subsystem_test_helpers::ForwardSubsystem; use polkadot_overseer::{dummy::dummy_overseer_builder, HeadSupportsParachains}; diff --git a/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index b73f79ee40c..98113a73a61 100644 --- a/client/consensus/common/src/tests.rs +++ b/client/consensus/common/src/tests.rs @@ -31,10 +31,10 @@ use cumulus_test_client::{ use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder; use futures::{channel::mpsc, executor::block_on, select, FutureExt, Stream, StreamExt}; use futures_timer::Delay; +use polkadot_primitives::HeadData; use sc_client_api::{blockchain::Backend as _, Backend as _, UsageProvider}; use sc_consensus::{BlockImport, BlockImportParams, ForkChoiceStrategy}; use sp_consensus::{BlockOrigin, BlockStatus}; -use polkadot_primitives::HeadData; use std::{ collections::{BTreeMap, HashMap}, pin::Pin, diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index dbcb5048b18..a51d7c09b08 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -337,10 +337,8 @@ pub mod pallet { let ancestor = Ancestor::new_unchecked(used_bandwidth); let watermark = HrmpWatermark::::get(); - let watermark_update = HrmpWatermarkUpdate::new( - watermark, - LastRelayChainBlockNumber::::get(), - ); + let watermark_update = + HrmpWatermarkUpdate::new(watermark, LastRelayChainBlockNumber::::get()); AggregatedUnincludedSegment::::mutate(|agg| { let agg = agg.get_or_insert_with(SegmentTracker::default); // TODO: In order of this panic to be correct, outbound message source should diff --git a/pallets/parachain-system/src/unincluded_segment.rs b/pallets/parachain-system/src/unincluded_segment.rs index e73d43fb52e..a61c5725556 100644 --- a/pallets/parachain-system/src/unincluded_segment.rs +++ b/pallets/parachain-system/src/unincluded_segment.rs @@ -550,7 +550,8 @@ mod tests { used_bandwidth: create_used_hrmp([(para_0, para_0_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) + segment + .append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) .expect("update is within the limits"); for watermark in 1..5 { @@ -578,14 +579,17 @@ mod tests { ); // Remove the first ancestor from the segment to make space. segment.subtract(&ancestor_0); - segment.append(&ancestor_5, HrmpWatermarkUpdate::Trunk(5), &limits).expect("update is within the limits"); + segment + .append(&ancestor_5, HrmpWatermarkUpdate::Trunk(5), &limits) + .expect("update is within the limits"); let para_1_update = HrmpChannelUpdate { msg_count: 3, total_bytes: 10 }; let ancestor = Ancestor { used_bandwidth: create_used_hrmp([(para_1, para_1_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(6), &limits) + segment + .append(&ancestor, HrmpWatermarkUpdate::Trunk(6), &limits) .expect("update is within the limits"); assert_matches!( @@ -618,7 +622,8 @@ mod tests { used_bandwidth: create_used_ump((1, 10)), para_head_hash: None::, }; - segment.append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) + segment + .append(&ancestor_0, HrmpWatermarkUpdate::Trunk(0), &limits) .expect("update is within the limits"); for watermark in 1..4 { @@ -647,7 +652,8 @@ mod tests { used_bandwidth: create_used_ump((1, 5)), para_head_hash: None::, }; - segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(4), &limits) + segment + .append(&ancestor, HrmpWatermarkUpdate::Trunk(4), &limits) .expect("update is within the limits"); assert_matches!( segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(5), &limits), @@ -684,7 +690,9 @@ mod tests { ); for watermark in 1..5 { - segment.append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits).expect("hrmp watermark is valid"); + segment + .append(&ancestor, HrmpWatermarkUpdate::Trunk(watermark), &limits) + .expect("hrmp watermark is valid"); } for watermark in 0..5 { assert_matches!( @@ -696,7 +704,9 @@ mod tests { ); } - segment.append(&ancestor, HrmpWatermarkUpdate::Head(4), &limits).expect("head updates always valid"); + segment + .append(&ancestor, HrmpWatermarkUpdate::Head(4), &limits) + .expect("head updates always valid"); } #[test] @@ -725,14 +735,16 @@ mod tests { used_bandwidth: create_used_hrmp([(para_0, para_0_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_0, HrmpWatermarkUpdate::Head(0), &limits) + segment + .append(&ancestor_0, HrmpWatermarkUpdate::Head(0), &limits) .expect("update is within the limits"); let para_1_update = HrmpChannelUpdate { msg_count: 3, total_bytes: 10 }; let ancestor_1 = Ancestor { used_bandwidth: create_used_hrmp([(para_1, para_1_update)].into()), para_head_hash: None::, }; - segment.append(&ancestor_1, HrmpWatermarkUpdate::Head(1), &limits) + segment + .append(&ancestor_1, HrmpWatermarkUpdate::Head(1), &limits) .expect("update is within the limits"); assert_eq!(segment.used_bandwidth.hrmp_outgoing.len(), 2); diff --git a/primitives/timestamp/src/lib.rs b/primitives/timestamp/src/lib.rs index 9e9cb755ed4..97fd3128ca2 100644 --- a/primitives/timestamp/src/lib.rs +++ b/primitives/timestamp/src/lib.rs @@ -117,7 +117,7 @@ mod tests { included_para_head: Some(HeadData(parent_header.encode())), current_slot: relay_chain_slot, ..Default::default() - }; + }; let relay_parent_storage_root = sproof_builder.clone().into_state_root_and_proof().0; From 352d556b09d2faadd877fde888fafad705f853c7 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 5 May 2023 19:09:37 -0500 Subject: [PATCH 15/19] rustdoc CI --- pallets/parachain-system/src/lib.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index a51d7c09b08..50df5e8ba59 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -206,8 +206,13 @@ pub mod pallet { /// An entry-point for higher-level logic to manage the backlog of unincluded parachain blocks /// and authorship rights for those blocks. /// - /// To maintain the same behavior as prior to asynchronous backing, provide the - /// [`ExpectParentIncludedHook`] here. + /// Typically, this should be a hook tailored to the collator-selection/consensus mechanism + /// that is used for this chain. + /// + /// However, to maintain the same behavior as prior to asynchronous backing, provide the + /// [`consensus_hook::ExpectParentIncludedHook`] here. This is only necessary in the case + /// that collators aren't expected to have node versions that supply the included block + /// in the relay-chain state proof. type ConsensusHook: ConsensusHook; } From 18184269572ce5cb0413d8d5c6c3f595932971a2 Mon Sep 17 00:00:00 2001 From: Chris Sosnin <48099298+slumber@users.noreply.github.com> Date: Thu, 11 May 2023 20:24:09 +0300 Subject: [PATCH 16/19] aura-ext: limit the number of authored blocks per slot (#2551) * aura_ext consensus hook * reverse dependency * include weight into hook * fix tests --- pallets/aura-ext/Cargo.toml | 4 ++ pallets/aura-ext/src/consensus_hook.rs | 56 +++++++++++++++++++ pallets/aura-ext/src/lib.rs | 25 ++++++++- .../parachain-system/src/consensus_hook.rs | 23 +++++--- pallets/parachain-system/src/lib.rs | 7 ++- .../src/relay_state_snapshot.rs | 2 + pallets/parachain-system/src/tests.rs | 14 +++-- 7 files changed, 114 insertions(+), 17 deletions(-) create mode 100644 pallets/aura-ext/src/consensus_hook.rs diff --git a/pallets/aura-ext/Cargo.toml b/pallets/aura-ext/Cargo.toml index 1db43697511..df145aad522 100644 --- a/pallets/aura-ext/Cargo.toml +++ b/pallets/aura-ext/Cargo.toml @@ -18,6 +18,9 @@ sp-consensus-aura = { git = "https://github.com/paritytech/substrate", default-f sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } sp-std = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } +# Cumulus +cumulus-pallet-parachain-system = { path = "../parachain-system", default-features = false } + [dev-dependencies] # Cumulus @@ -35,5 +38,6 @@ std = [ "sp-consensus-aura/std", "sp-runtime/std", "sp-std/std", + "cumulus-pallet-parachain-system/std", ] try-runtime = ["frame-support/try-runtime"] diff --git a/pallets/aura-ext/src/consensus_hook.rs b/pallets/aura-ext/src/consensus_hook.rs new file mode 100644 index 00000000000..c8806b1f4cb --- /dev/null +++ b/pallets/aura-ext/src/consensus_hook.rs @@ -0,0 +1,56 @@ +// Copyright 2023 Parity Technologies (UK) Ltd. +// This file is part of Cumulus. + +// Cumulus is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Cumulus is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Cumulus. If not, see . + +//! The definition of a [`FixedVelocityConsensusHook`] for consensus logic to manage +//! block velocity. +//! +//! The velocity `V` refers to the rate of block processing by the relay chain. + +use super::pallet; +use cumulus_pallet_parachain_system::{ + consensus_hook::{ConsensusHook, UnincludedSegmentCapacity}, + relay_state_snapshot::RelayChainStateProof, +}; +use frame_support::pallet_prelude::*; +use sp_std::{marker::PhantomData, num::NonZeroU32}; + +/// A consensus hook for a fixed block processing velocity and unincluded segment capacity. +pub struct FixedVelocityConsensusHook(PhantomData); + +impl ConsensusHook + for FixedVelocityConsensusHook +{ + // Validates the number of authored blocks within the slot with respect to the `V + 1` limit. + fn on_state_proof(_state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) { + // Ensure velocity is non-zero. + let velocity = V.max(1); + + let authored = pallet::Pallet::::slot_info() + .map(|(_slot, authored)| authored) + .expect("slot info is inserted on block initialization"); + if authored > velocity + 1 { + panic!("authored blocks limit is reached for the slot") + } + let weight = T::DbWeight::get().reads(1); + + ( + weight, + NonZeroU32::new(sp_std::cmp::max(C, 1)) + .expect("1 is the minimum value and non-zero; qed") + .into(), + ) + } +} diff --git a/pallets/aura-ext/src/lib.rs b/pallets/aura-ext/src/lib.rs index 15e82edeefe..5605e2f2ac5 100644 --- a/pallets/aura-ext/src/lib.rs +++ b/pallets/aura-ext/src/lib.rs @@ -37,9 +37,12 @@ use frame_support::traits::{ExecuteBlock, FindAuthor}; use sp_application_crypto::RuntimeAppPublic; -use sp_consensus_aura::digests::CompatibleDigestItem; +use sp_consensus_aura::{digests::CompatibleDigestItem, Slot}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; +pub mod consensus_hook; +pub use consensus_hook::FixedVelocityConsensusHook; + type Aura = pallet_aura::Pallet; pub use pallet::*; @@ -68,6 +71,19 @@ pub mod pallet { // Fetch the authorities once to get them into the storage proof of the PoV. Authorities::::get(); + let new_slot = Aura::::current_slot(); + + let (new_slot, authored) = match SlotInfo::::get() { + Some((slot, authored)) if slot == new_slot => (slot, authored + 1), + Some((slot, _)) if slot < new_slot => (new_slot, 1), + Some(..) => { + panic!("slot moved backwards") + }, + None => (new_slot, 1), + }; + + SlotInfo::::put((new_slot, authored)); + T::DbWeight::get().reads_writes(2, 1) } } @@ -84,6 +100,13 @@ pub mod pallet { ValueQuery, >; + /// Current slot paired with a number of authored blocks. + /// + /// Updated on each block initialization. + #[pallet::storage] + #[pallet::getter(fn slot_info)] + pub(crate) type SlotInfo = StorageValue<_, (Slot, u32), OptionQuery>; + #[pallet::genesis_config] #[derive(Default)] pub struct GenesisConfig; diff --git a/pallets/parachain-system/src/consensus_hook.rs b/pallets/parachain-system/src/consensus_hook.rs index 8d3606d14b8..bdf590a0fd5 100644 --- a/pallets/parachain-system/src/consensus_hook.rs +++ b/pallets/parachain-system/src/consensus_hook.rs @@ -18,6 +18,7 @@ //! of parachain blocks ready to submit to the relay chain, as well as some basic implementations. use super::relay_state_snapshot::RelayChainStateProof; +use frame_support::weights::Weight; use sp_std::num::NonZeroU32; /// The possible capacity of the unincluded segment. @@ -61,8 +62,8 @@ pub trait ConsensusHook { /// This hook is called partway through the `set_validation_data` inherent in parachain-system. /// /// The hook is allowed to panic if customized consensus rules aren't met and is required - /// to return a maximum capacity for the unincluded segment. - fn on_state_proof(state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity; + /// to return a maximum capacity for the unincluded segment with weight consumed. + fn on_state_proof(state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity); } /// A special consensus hook for handling the migration to asynchronous backing gracefully, @@ -75,8 +76,11 @@ pub trait ConsensusHook { pub struct ExpectParentIncluded; impl ConsensusHook for ExpectParentIncluded { - fn on_state_proof(_state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity { - UnincludedSegmentCapacity(UnincludedSegmentCapacityInner::ExpectParentIncluded) + fn on_state_proof(_state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) { + ( + Weight::zero(), + UnincludedSegmentCapacity(UnincludedSegmentCapacityInner::ExpectParentIncluded), + ) } } @@ -88,10 +92,13 @@ impl ConsensusHook for ExpectParentIncluded { pub struct FixedCapacityUnincludedSegment; impl ConsensusHook for FixedCapacityUnincludedSegment { - fn on_state_proof(_state_proof: &RelayChainStateProof) -> UnincludedSegmentCapacity { - NonZeroU32::new(sp_std::cmp::max(N, 1)) - .expect("1 is the minimum value and non-zero; qed") - .into() + fn on_state_proof(_state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) { + ( + Weight::zero(), + NonZeroU32::new(sp_std::cmp::max(N, 1)) + .expect("1 is the minimum value and non-zero; qed") + .into(), + ) } } diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 50df5e8ba59..5601bcdd17d 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -58,12 +58,12 @@ use sp_std::{cmp, collections::btree_map::BTreeMap, prelude::*}; use xcm::latest::XcmHash; mod migration; -mod relay_state_snapshot; #[cfg(test)] mod tests; mod unincluded_segment; pub mod consensus_hook; +pub mod relay_state_snapshot; #[macro_use] pub mod validate_block; @@ -484,7 +484,8 @@ pub mod pallet { .expect("Invalid relay chain state proof"); // Update the desired maximum capacity according to the consensus hook. - let capacity = T::ConsensusHook::on_state_proof(&relay_state_proof); + let (consensus_hook_weight, capacity) = + T::ConsensusHook::on_state_proof(&relay_state_proof); // initialization logic: we know that this runs exactly once every block, // which means we can put the initialization logic here to remove the @@ -543,7 +544,7 @@ pub mod pallet { // ancestor was included, the MQC heads wouldn't match and the block would be invalid. // // - let mut total_weight = Weight::zero(); + let mut total_weight = consensus_hook_weight; total_weight += Self::process_inbound_downward_messages( relevant_messaging_state.dmq_mqc_head, downward_messages, diff --git a/pallets/parachain-system/src/relay_state_snapshot.rs b/pallets/parachain-system/src/relay_state_snapshot.rs index 9da5a03ce83..ead077f527d 100644 --- a/pallets/parachain-system/src/relay_state_snapshot.rs +++ b/pallets/parachain-system/src/relay_state_snapshot.rs @@ -14,6 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Cumulus. If not, see . +//! Relay chain state proof provides means for accessing part of relay chain storage for reads. + use codec::{Decode, Encode}; use cumulus_primitives_core::{ relay_chain, AbridgedHostConfiguration, AbridgedHrmpChannel, ParaId, diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index e1010f89be0..574ab43078d 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -121,14 +121,14 @@ std::thread_local! { static HANDLED_DMP_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); static HANDLED_XCMP_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); static SENT_MESSAGES: RefCell)>> = RefCell::new(Vec::new()); - static CONSENSUS_HOOK: RefCell UnincludedSegmentCapacity>> - = RefCell::new(Box::new(|_| NonZeroU32::new(1).unwrap().into())); + static CONSENSUS_HOOK: RefCell (Weight, UnincludedSegmentCapacity)>> + = RefCell::new(Box::new(|_| (Weight::zero(), NonZeroU32::new(1).unwrap().into()))); } pub struct TestConsensusHook; impl ConsensusHook for TestConsensusHook { - fn on_state_proof(s: &RelayChainStateProof) -> UnincludedSegmentCapacity { + fn on_state_proof(s: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) { CONSENSUS_HOOK.with(|f| f.borrow_mut()(s)) } } @@ -440,7 +440,9 @@ fn block_tests_run_on_drop() { #[test] fn unincluded_segment_works() { - CONSENSUS_HOOK.with(|c| *c.borrow_mut() = Box::new(|_| NonZeroU32::new(10).unwrap().into())); + CONSENSUS_HOOK.with(|c| { + *c.borrow_mut() = Box::new(|_| (Weight::zero(), NonZeroU32::new(10).unwrap().into())) + }); BlockTests::new() .with_inclusion_delay(1) @@ -475,7 +477,9 @@ fn unincluded_segment_works() { #[test] #[should_panic = "no space left for the block in the unincluded segment"] fn unincluded_segment_is_limited() { - CONSENSUS_HOOK.with(|c| *c.borrow_mut() = Box::new(|_| NonZeroU32::new(1).unwrap().into())); + CONSENSUS_HOOK.with(|c| { + *c.borrow_mut() = Box::new(|_| (Weight::zero(), NonZeroU32::new(1).unwrap().into())) + }); BlockTests::new() .with_inclusion_delay(2) From b9431fe05698a39de4c4c4d419fa1c4b2ada8cf0 Mon Sep 17 00:00:00 2001 From: asynchronous rob Date: Sat, 13 May 2023 00:42:46 -0500 Subject: [PATCH 17/19] remove stray println Co-authored-by: Chris Sosnin <48099298+slumber@users.noreply.github.com> --- client/consensus/common/src/tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index 98113a73a61..37cec2c94be 100644 --- a/client/consensus/common/src/tests.rs +++ b/client/consensus/common/src/tests.rs @@ -582,7 +582,6 @@ fn do_not_set_best_block_to_older_block() { let blocks = (0..NUM_BLOCKS) .into_iter() .map(|i| { - println!("{}", i); build_and_import_block(client.clone(), true) }) .collect::>(); From 4115b0ca22b2d10239b95a91570d912c562c0c8d Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 16 May 2023 15:51:43 -0400 Subject: [PATCH 18/19] fix test warning --- client/consensus/common/src/tests.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index 37cec2c94be..5e65ffc53d1 100644 --- a/client/consensus/common/src/tests.rs +++ b/client/consensus/common/src/tests.rs @@ -581,9 +581,7 @@ fn do_not_set_best_block_to_older_block() { let blocks = (0..NUM_BLOCKS) .into_iter() - .map(|i| { - build_and_import_block(client.clone(), true) - }) + .map(|_| build_and_import_block(client.clone(), true)) .collect::>(); assert_eq!(NUM_BLOCKS as u32, client.usage_info().chain.best_number); From 54646aef1a8d8b0702ab8d6ce77e6e9460140206 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 16 May 2023 17:43:49 -0400 Subject: [PATCH 19/19] fix doc link --- pallets/parachain-system/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 5601bcdd17d..8c8a57107dd 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -210,7 +210,7 @@ pub mod pallet { /// that is used for this chain. /// /// However, to maintain the same behavior as prior to asynchronous backing, provide the - /// [`consensus_hook::ExpectParentIncludedHook`] here. This is only necessary in the case + /// [`consensus_hook::ExpectParentIncluded`] here. This is only necessary in the case /// that collators aren't expected to have node versions that supply the included block /// in the relay-chain state proof. type ConsensusHook: ConsensusHook;