diff --git a/polkadot/primitives/src/v1.rs b/polkadot/primitives/src/v1.rs index 5a3d25efee..fd320dfcb2 100644 --- a/polkadot/primitives/src/v1.rs +++ b/polkadot/primitives/src/v1.rs @@ -473,6 +473,7 @@ pub type SignedAvailabilityBitfields = Vec; /// A backed (or backable, depending on context) candidate. #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug)] +#[cfg_attr(feature = "std", derive(Default))] pub struct BackedCandidate { /// The candidate referred to. pub candidate: CommittedCandidateReceipt, diff --git a/polkadot/runtime/parachains/src/inclusion_inherent.rs b/polkadot/runtime/parachains/src/inclusion_inherent.rs index 27a319dced..3747f33fda 100644 --- a/polkadot/runtime/parachains/src/inclusion_inherent.rs +++ b/polkadot/runtime/parachains/src/inclusion_inherent.rs @@ -25,10 +25,9 @@ use sp_std::prelude::*; use primitives::v1::{ BackedCandidate, SignedAvailabilityBitfields, INCLUSION_INHERENT_IDENTIFIER, Header, }; -use sp_runtime::traits::One; use frame_support::{ decl_error, decl_module, decl_storage, ensure, - dispatch::DispatchResult, + dispatch::DispatchResultWithPostInfo, weights::{DispatchClass, Weight}, traits::Get, }; @@ -40,6 +39,12 @@ use crate::{ }; use inherents::{InherentIdentifier, InherentData, MakeFatalError, ProvideInherent}; +// In the future, we should benchmark these consts; these are all untested assumptions for now. +const BACKED_CANDIDATE_WEIGHT: Weight = 100_000; +const INCLUSION_INHERENT_CLAIMED_WEIGHT: Weight = 1_000_000_000; +// we assume that 75% of an inclusion inherent's weight is used processing backed candidates +const MINIMAL_INCLUSION_INHERENT_WEIGHT: Weight = INCLUSION_INHERENT_CLAIMED_WEIGHT / 4; + pub trait Config: inclusion::Config + scheduler::Config {} decl_storage! { @@ -80,19 +85,21 @@ decl_module! { } /// Include backed candidates and bitfields. - #[weight = (1_000_000_000, DispatchClass::Mandatory)] + #[weight = ( + MINIMAL_INCLUSION_INHERENT_WEIGHT + backed_candidates.len() as Weight * BACKED_CANDIDATE_WEIGHT, + DispatchClass::Mandatory, + )] pub fn inclusion( origin, signed_bitfields: SignedAvailabilityBitfields, backed_candidates: Vec>, parent_header: Header, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { ensure_none(origin)?; ensure!(!::exists(), Error::::TooManyInclusionInherents); // Check that the submitted parent header indeed corresponds to the previous block hash. - let now = >::block_number(); - let parent_hash = >::block_hash(now - One::one()); + let parent_hash = >::parent_hash(); ensure!( parent_header.hash().as_ref() == parent_hash.as_ref(), Error::::InvalidParentHeader, @@ -119,6 +126,9 @@ decl_module! { >::schedule(freed); + let backed_candidates = limit_backed_candidates::(backed_candidates); + let backed_candidates_len = backed_candidates.len() as Weight; + // Process backed candidates according to scheduled cores. let parent_storage_root = parent_header.state_root; let occupied = >::process_candidates( @@ -137,11 +147,35 @@ decl_module! { // And track that we've finished processing the inherent for this block. Included::set(Some(())); - Ok(()) + Ok(Some( + MINIMAL_INCLUSION_INHERENT_WEIGHT + + (backed_candidates_len * BACKED_CANDIDATE_WEIGHT) + ).into()) } } } +/// Limit the number of backed candidates processed in order to stay within block weight limits. +/// +/// Use a configured assumption about the weight required to process a backed candidate and the +/// current block weight as of the execution of this function to ensure that we don't overload +/// the block with candidate processing. +/// +/// If the backed candidates exceed the available block weight remaining, then skips all of them. +/// This is somewhat less desirable than attempting to fit some of them, but is more fair in the +/// even that we can't trust the provisioner to provide a fair / random ordering of candidates. +fn limit_backed_candidates( + backed_candidates: Vec>, +) -> Vec> { + // the weight of the inclusion inherent is already included in the current block weight, + // so our operation is simple: if the block is currently overloaded, make this intrinsic smaller + if frame_system::Module::::block_weight().total() > ::BlockWeights::get().max_block { + Vec::new() + } else { + backed_candidates + } +} + impl ProvideInherent for Module { type Call = Call; type Error = MakeFatalError<()>; @@ -174,3 +208,156 @@ impl ProvideInherent for Module { ) } } + +#[cfg(test)] +mod tests { + use super::*; + + use crate::mock::{ + new_test_ext, System, GenesisConfig as MockGenesisConfig, Test + }; + + mod limit_backed_candidates { + use super::*; + + #[test] + fn does_not_truncate_on_empty_block() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let backed_candidates = vec![BackedCandidate::default()]; + System::set_block_consumed_resources(0, 0); + assert_eq!(limit_backed_candidates::(backed_candidates).len(), 1); + }); + } + + #[test] + fn does_not_truncate_on_exactly_full_block() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let backed_candidates = vec![BackedCandidate::default()]; + let max_block_weight = ::BlockWeights::get().max_block; + // if the consumed resources are precisely equal to the max block weight, we do not truncate. + System::set_block_consumed_resources(max_block_weight, 0); + assert_eq!(limit_backed_candidates::(backed_candidates).len(), 1); + }); + } + + #[test] + fn truncates_on_over_full_block() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let backed_candidates = vec![BackedCandidate::default()]; + let max_block_weight = ::BlockWeights::get().max_block; + // if the consumed resources are precisely equal to the max block weight, we do not truncate. + System::set_block_consumed_resources(max_block_weight + 1, 0); + assert_eq!(limit_backed_candidates::(backed_candidates).len(), 0); + }); + } + + #[test] + fn all_backed_candidates_get_truncated() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let backed_candidates = vec![BackedCandidate::default(); 10]; + let max_block_weight = ::BlockWeights::get().max_block; + // if the consumed resources are precisely equal to the max block weight, we do not truncate. + System::set_block_consumed_resources(max_block_weight + 1, 0); + assert_eq!(limit_backed_candidates::(backed_candidates).len(), 0); + }); + } + } + + mod inclusion_inherent_weight { + use super::*; + + use crate::mock::{ + new_test_ext, System, GenesisConfig as MockGenesisConfig, Test + }; + + use frame_support::traits::UnfilteredDispatchable; + + fn default_header() -> Header { + Header { + parent_hash: Default::default(), + number: 0, + state_root: Default::default(), + extrinsics_root: Default::default(), + digest: Default::default(), + } + } + + /// We expect the weight of the inclusion inherent not to change when no truncation occurs: + /// its weight is dynamically computed from the size of the backed candidates list, and is + /// already incorporated into the current block weight when it is selected by the provisioner. + #[test] + fn weight_does_not_change_on_happy_path() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let header = default_header(); + System::set_block_number(1); + System::set_parent_hash(header.hash()); + + // number of bitfields doesn't affect the inclusion inherent weight, so we can mock it with an empty one + let signed_bitfields = Vec::new(); + // backed candidates must not be empty, so we can demonstrate that the weight has not changed + let backed_candidates = vec![BackedCandidate::default(); 10]; + + // the expected weight can always be computed by this formula + let expected_weight = MINIMAL_INCLUSION_INHERENT_WEIGHT + + (backed_candidates.len() as Weight * BACKED_CANDIDATE_WEIGHT); + + // we've used half the block weight; there's plenty of margin + let max_block_weight = ::BlockWeights::get().max_block; + let used_block_weight = max_block_weight / 2; + System::set_block_consumed_resources(used_block_weight, 0); + + // execute the inclusion inherent + let post_info = Call::::inclusion(signed_bitfields, backed_candidates, default_header()) + .dispatch_bypass_filter(None.into()).unwrap_err().post_info; + + // we don't directly check the block's weight post-call. Instead, we check that the + // call has returned the appropriate post-dispatch weight for refund, and trust + // Substrate to do the right thing with that information. + // + // In this case, the weight system can update the actual weight with the same amount, + // or return `None` to indicate that the pre-computed weight should not change. + // Either option is acceptable for our purposes. + if let Some(actual_weight) = post_info.actual_weight { + assert_eq!(actual_weight, expected_weight); + } + }); + } + + /// We expect the weight of the inclusion inherent to change when truncation occurs: its + /// weight was initially dynamically computed from the size of the backed candidates list, + /// but was reduced by truncation. + #[test] + fn weight_changes_when_backed_candidates_are_truncated() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let header = default_header(); + System::set_block_number(1); + System::set_parent_hash(header.hash()); + + // number of bitfields doesn't affect the inclusion inherent weight, so we can mock it with an empty one + let signed_bitfields = Vec::new(); + // backed candidates must not be empty, so we can demonstrate that the weight has not changed + let backed_candidates = vec![BackedCandidate::default(); 10]; + + // the expected weight with no blocks is just the minimum weight + let expected_weight = MINIMAL_INCLUSION_INHERENT_WEIGHT; + + // oops, looks like this mandatory call pushed the block weight over the limit + let max_block_weight = ::BlockWeights::get().max_block; + let used_block_weight = max_block_weight + 1; + System::set_block_consumed_resources(used_block_weight, 0); + + // execute the inclusion inherent + let post_info = Call::::inclusion(signed_bitfields, backed_candidates, header) + .dispatch_bypass_filter(None.into()).unwrap(); + + // we don't directly check the block's weight post-call. Instead, we check that the + // call has returned the appropriate post-dispatch weight for refund, and trust + // Substrate to do the right thing with that information. + assert_eq!( + post_info.actual_weight.unwrap(), + expected_weight, + ); + }); + } + } +} diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 7d5e0c380b..7ae33c01b3 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -120,6 +120,8 @@ impl crate::inclusion::Config for Test { type RewardValidators = TestRewardValidators; } +impl crate::inclusion_inherent::Config for Test { } + impl crate::session_info::Config for Test { } impl crate::session_info::AuthorityDiscoveryConfig for Test {