diff --git a/cumulus/client/collator/src/service.rs b/cumulus/client/collator/src/service.rs index c06be006fc17..1f08352e938a 100644 --- a/cumulus/client/collator/src/service.rs +++ b/cumulus/client/collator/src/service.rs @@ -262,6 +262,7 @@ where ) }) .ok()?; + let horizontal_messages = collation_info .horizontal_messages .try_into() diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs b/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs index ab78b31fbd80..a945a3608442 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs @@ -219,13 +219,20 @@ where { let runtime_api = para_client.runtime_api(); - if runtime_api.has_api::>(parent_hash)? { - Ok(runtime_api.core_selector(parent_hash)?) - } else { - let next_block_number: U256 = (parent_number + One::one()).into(); - - // If the runtime API does not support the core selector API, fallback to some default - // values. - Ok((CoreSelector(next_block_number.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET))) - } + let next_block_number: U256 = (parent_number + One::one()).into(); + // If the runtime API does not support the core selector API, fallback to some default + // values. + let fallback_core_selector = + (CoreSelector(next_block_number.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)); + + let maybe_api_version = + runtime_api.api_version::>(parent_hash)?; + + Ok(match maybe_api_version { + Some(api_version) if api_version >= 2 => + runtime_api.core_selector(parent_hash)?.unwrap_or(fallback_core_selector), + #[allow(deprecated)] + Some(_) => runtime_api.core_selector_before_version_2(parent_hash)?, + None => fallback_core_selector, + }) } diff --git a/cumulus/pallets/parachain-system/Cargo.toml b/cumulus/pallets/parachain-system/Cargo.toml index 6b6bc4fbcefe..16fe53983b9b 100644 --- a/cumulus/pallets/parachain-system/Cargo.toml +++ b/cumulus/pallets/parachain-system/Cargo.toml @@ -120,5 +120,3 @@ try-runtime = [ "polkadot-runtime-parachains/try-runtime", "sp-runtime/try-runtime", ] - -experimental-ump-signals = [] diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 0fa759357f65..f0615c5966dd 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -35,7 +35,9 @@ use core::{cmp, marker::PhantomData}; use cumulus_primitives_core::{ relay_chain::{ self, - vstaging::{ClaimQueueOffset, CoreSelector, DEFAULT_CLAIM_QUEUE_OFFSET}, + vstaging::{ + ClaimQueueOffset, CoreSelector, UMPSignal, DEFAULT_CLAIM_QUEUE_OFFSET, UMP_SEPARATOR, + }, }, AbridgedHostConfiguration, ChannelInfo, ChannelStatus, CollationInfo, GetChannelInfo, InboundDownwardMessage, InboundHrmpMessage, ListChannelInfos, MessageSendError, @@ -194,25 +196,34 @@ pub mod ump_constants { /// Trait for selecting the next core to build the candidate for. pub trait SelectCore { /// Core selector information for the current block. - fn selected_core() -> (CoreSelector, ClaimQueueOffset); + fn selected_core() -> Option<(CoreSelector, ClaimQueueOffset)>; /// Core selector information for the next block. - fn select_next_core() -> (CoreSelector, ClaimQueueOffset); + fn select_next_core() -> Option<(CoreSelector, ClaimQueueOffset)>; +} + +impl SelectCore for () { + fn selected_core() -> Option<(CoreSelector, ClaimQueueOffset)> { + None + } + fn select_next_core() -> Option<(CoreSelector, ClaimQueueOffset)> { + None + } } /// The default core selection policy. pub struct DefaultCoreSelector(PhantomData); impl SelectCore for DefaultCoreSelector { - fn selected_core() -> (CoreSelector, ClaimQueueOffset) { + fn selected_core() -> Option<(CoreSelector, ClaimQueueOffset)> { let core_selector: U256 = frame_system::Pallet::::block_number().into(); - (CoreSelector(core_selector.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)) + Some((CoreSelector(core_selector.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET))) } - fn select_next_core() -> (CoreSelector, ClaimQueueOffset) { + fn select_next_core() -> Option<(CoreSelector, ClaimQueueOffset)> { let core_selector: U256 = (frame_system::Pallet::::block_number() + One::one()).into(); - (CoreSelector(core_selector.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)) + Some((CoreSelector(core_selector.byte(0)), ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET))) } } @@ -220,16 +231,16 @@ impl SelectCore for DefaultCoreSelector { pub struct LookaheadCoreSelector(PhantomData); impl SelectCore for LookaheadCoreSelector { - fn selected_core() -> (CoreSelector, ClaimQueueOffset) { + fn selected_core() -> Option<(CoreSelector, ClaimQueueOffset)> { let core_selector: U256 = frame_system::Pallet::::block_number().into(); - (CoreSelector(core_selector.byte(0)), ClaimQueueOffset(1)) + Some((CoreSelector(core_selector.byte(0)), ClaimQueueOffset(1))) } - fn select_next_core() -> (CoreSelector, ClaimQueueOffset) { + fn select_next_core() -> Option<(CoreSelector, ClaimQueueOffset)> { let core_selector: U256 = (frame_system::Pallet::::block_number() + One::one()).into(); - (CoreSelector(core_selector.byte(0)), ClaimQueueOffset(1)) + Some((CoreSelector(core_selector.byte(0)), ClaimQueueOffset(1))) } } @@ -391,9 +402,7 @@ pub mod pallet { UpwardMessages::::put(&up[..num as usize]); *up = up.split_off(num as usize); - // Send the core selector UMP signal. This is experimental until relay chain - // validators are upgraded to handle ump signals. - #[cfg(feature = "experimental-ump-signals")] + // Send the core selector UMP signal. Self::send_ump_signal(); // If the total size of the pending messages is less than the threshold, @@ -1429,7 +1438,7 @@ impl Pallet { } /// Returns the core selector for the next block. - pub fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + pub fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { T::SelectCore::select_next_core() } @@ -1450,17 +1459,16 @@ impl Pallet { } /// Send the ump signals - #[cfg(feature = "experimental-ump-signals")] fn send_ump_signal() { - use cumulus_primitives_core::relay_chain::vstaging::{UMPSignal, UMP_SEPARATOR}; + // If the runtime is configured with a core selection policy, send the core selector signal. + let maybe_core_selector = T::SelectCore::selected_core(); - UpwardMessages::::mutate(|up| { - up.push(UMP_SEPARATOR); - - // Send the core selector signal. - let core_selector = T::SelectCore::selected_core(); - up.push(UMPSignal::SelectCore(core_selector.0, core_selector.1).encode()); - }); + if let Some(core_selector) = maybe_core_selector { + UpwardMessages::::mutate(|up| { + up.push(UMP_SEPARATOR); + up.push(UMPSignal::SelectCore(core_selector.0, core_selector.1).encode()); + }); + } } /// Open HRMP channel for using it in benchmarks or tests. diff --git a/cumulus/pallets/parachain-system/src/mock.rs b/cumulus/pallets/parachain-system/src/mock.rs index 5b59be0482e7..dfd5e889fa30 100644 --- a/cumulus/pallets/parachain-system/src/mock.rs +++ b/cumulus/pallets/parachain-system/src/mock.rs @@ -22,7 +22,7 @@ use super::*; use alloc::collections::vec_deque::VecDeque; use codec::Encode; -use core::num::NonZeroU32; +use core::{cell::Cell, num::NonZeroU32}; use cumulus_primitives_core::{ relay_chain::BlockNumber as RelayBlockNumber, AggregateMessageOrigin, InboundDownwardMessage, InboundHrmpMessage, PersistedValidationData, @@ -94,7 +94,31 @@ impl Config for Test { type CheckAssociatedRelayNumber = AnyRelayNumber; type ConsensusHook = TestConsensusHook; type WeightInfo = (); - type SelectCore = DefaultCoreSelector; + type SelectCore = TestCoreSelector>; +} + +std::thread_local! { + pub static USE_CORE_SELECTOR: Cell = Cell::new(true); +} + +pub struct TestCoreSelector(PhantomData); + +impl SelectCore for TestCoreSelector { + fn selected_core() -> Option<(CoreSelector, ClaimQueueOffset)> { + if USE_CORE_SELECTOR.get() { + Selector::selected_core() + } else { + None + } + } + + fn select_next_core() -> Option<(CoreSelector, ClaimQueueOffset)> { + if USE_CORE_SELECTOR.get() { + Selector::select_next_core() + } else { + None + } + } } std::thread_local! { diff --git a/cumulus/pallets/parachain-system/src/tests.rs b/cumulus/pallets/parachain-system/src/tests.rs index 2b65dd6a9216..0059b1e5c763 100755 --- a/cumulus/pallets/parachain-system/src/tests.rs +++ b/cumulus/pallets/parachain-system/src/tests.rs @@ -25,9 +25,10 @@ use frame_support::{assert_ok, parameter_types, weights::Weight}; use frame_system::RawOrigin; use hex_literal::hex; use rand::Rng; -#[cfg(feature = "experimental-ump-signals")] -use relay_chain::vstaging::{UMPSignal, UMP_SEPARATOR}; -use relay_chain::HrmpChannelId; +use relay_chain::{ + vstaging::{UMPSignal, UMP_SEPARATOR}, + HrmpChannelId, +}; use sp_core::H256; #[test] @@ -585,25 +586,18 @@ fn send_upward_message_num_per_candidate() { }, || { let v = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - v, - vec![ - b"Mr F was here".to_vec(), - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(1), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert_eq!(v, vec![b"Mr F was here".to_vec()]); - } + assert_eq!( + v, + vec![ + b"Mr F was here".to_vec(), + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(1), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); }, ) .add_with_post_test( @@ -614,25 +608,39 @@ fn send_upward_message_num_per_candidate() { }, || { let v = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - v, - vec![ - b"message 2".to_vec(), - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(2), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert_eq!(v, vec![b"message 2".to_vec()]); - } + assert_eq!( + v, + vec![ + b"message 2".to_vec(), + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(2), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); + }, + ); +} + +#[test] +fn no_ump_signal_if_no_core_selector() { + USE_CORE_SELECTOR.set(false); + + BlockTests::new() + .with_relay_sproof_builder(|_, _, sproof| { + sproof.relay_dispatch_queue_remaining_capacity = None; + }) + .add_with_post_test( + 1, + || { + ParachainSystem::send_upward_message(b"message 1".to_vec()).unwrap(); + ParachainSystem::send_upward_message(b"message 2".to_vec()).unwrap(); + }, + || { + let v = UpwardMessages::::get(); + assert_eq!(v, vec![b"message 1".to_vec(), b"message 2".to_vec()]); }, ); } @@ -658,24 +666,17 @@ fn send_upward_message_relay_bottleneck() { || { // The message won't be sent because there is already one message in queue. let v = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - v, - vec![ - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(1), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert!(v.is_empty()); - } + assert_eq!( + v, + vec![ + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(1), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); }, ) .add_with_post_test( @@ -683,25 +684,18 @@ fn send_upward_message_relay_bottleneck() { || { /* do nothing within block */ }, || { let v = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - v, - vec![ - vec![0u8; 8], - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(2), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert_eq!(v, vec![vec![0u8; 8]]); - } + assert_eq!( + v, + vec![ + vec![0u8; 8], + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(2), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); }, ); } @@ -1245,25 +1239,18 @@ fn ump_fee_factor_increases_and_decreases() { || { // Factor decreases in `on_finalize`, but only if we are below the threshold let messages = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - messages, - vec![ - b"Test".to_vec(), - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(1), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert_eq!(messages, vec![b"Test".to_vec()]); - } + assert_eq!( + messages, + vec![ + b"Test".to_vec(), + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(1), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); assert_eq!( UpwardDeliveryFeeFactor::::get(), FixedU128::from_rational(105, 100) @@ -1277,28 +1264,18 @@ fn ump_fee_factor_increases_and_decreases() { }, || { let messages = UpwardMessages::::get(); - #[cfg(feature = "experimental-ump-signals")] - { - assert_eq!( - messages, - vec![ - b"This message will be enough to increase the fee factor".to_vec(), - UMP_SEPARATOR, - UMPSignal::SelectCore( - CoreSelector(2), - ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) - ) - .encode() - ] - ); - } - #[cfg(not(feature = "experimental-ump-signals"))] - { - assert_eq!( - messages, - vec![b"This message will be enough to increase the fee factor".to_vec()] - ); - } + assert_eq!( + messages, + vec![ + b"This message will be enough to increase the fee factor".to_vec(), + UMP_SEPARATOR, + UMPSignal::SelectCore( + CoreSelector(2), + ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET) + ) + .encode() + ] + ); // Now the delivery fee factor is decreased, since we are below the threshold assert_eq!(UpwardDeliveryFeeFactor::::get(), FixedU128::from_u32(1)); }, diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs index dd1535826152..1e707432f3ed 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs @@ -1495,7 +1495,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs index 707d1c52f743..1749b76acc59 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -1671,7 +1671,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs index 492b731610ce..5b0fb88ad077 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs @@ -901,7 +901,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs index edf79ea0c315..06c2374d6d19 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs @@ -834,7 +834,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs index 5c2ba2e24c22..59590a79b687 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs @@ -1018,7 +1018,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs index 594c9b26f57e..c658c0d09d24 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs @@ -659,7 +659,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs index e8f6e6659e13..8f2ff383856a 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs @@ -890,7 +890,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs index ce965f0ad1ba..99171b66963f 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs @@ -882,7 +882,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/glutton/glutton-westend/src/lib.rs b/cumulus/parachains/runtimes/glutton/glutton-westend/src/lib.rs index 763f8abea34a..9a3024391dac 100644 --- a/cumulus/parachains/runtimes/glutton/glutton-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/glutton/glutton-westend/src/lib.rs @@ -428,7 +428,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs b/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs index b8db687da625..8db3ded1eeb2 100644 --- a/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs @@ -838,7 +838,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/people/people-westend/src/lib.rs b/cumulus/parachains/runtimes/people/people-westend/src/lib.rs index 620ec41c071c..658addb71b61 100644 --- a/cumulus/parachains/runtimes/people/people-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/people/people-westend/src/lib.rs @@ -836,7 +836,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/testing/penpal/src/lib.rs b/cumulus/parachains/runtimes/testing/penpal/src/lib.rs index b51670c792d6..e72530a6f5f5 100644 --- a/cumulus/parachains/runtimes/testing/penpal/src/lib.rs +++ b/cumulus/parachains/runtimes/testing/penpal/src/lib.rs @@ -985,7 +985,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/parachains/runtimes/testing/rococo-parachain/src/lib.rs b/cumulus/parachains/runtimes/testing/rococo-parachain/src/lib.rs index 42556e0b493c..ecfcd117a96b 100644 --- a/cumulus/parachains/runtimes/testing/rococo-parachain/src/lib.rs +++ b/cumulus/parachains/runtimes/testing/rococo-parachain/src/lib.rs @@ -859,7 +859,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs b/cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs index 6bfd5f4f4cbd..40bdcadb7524 100644 --- a/cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs +++ b/cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs @@ -158,7 +158,7 @@ macro_rules! impl_node_runtime_apis { } impl cumulus_primitives_core::GetCoreSelectorApi<$block> for $runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { unimplemented!() } } diff --git a/cumulus/primitives/core/src/lib.rs b/cumulus/primitives/core/src/lib.rs index f88e663db19e..27c61e2307a3 100644 --- a/cumulus/primitives/core/src/lib.rs +++ b/cumulus/primitives/core/src/lib.rs @@ -398,8 +398,12 @@ sp_api::decl_runtime_apis! { } /// Runtime api used to select the core for which the next block will be built. + #[api_version(2)] pub trait GetCoreSelectorApi { /// Retrieve core selector and claim queue offset for the next block. + #[changed_in(2)] fn core_selector() -> (CoreSelector, ClaimQueueOffset); + + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)>; } } diff --git a/cumulus/test/runtime/Cargo.toml b/cumulus/test/runtime/Cargo.toml index 150838e5e96e..cf514872cfa8 100644 --- a/cumulus/test/runtime/Cargo.toml +++ b/cumulus/test/runtime/Cargo.toml @@ -96,4 +96,4 @@ std = [ ] increment-spec-version = [] elastic-scaling = [] -experimental-ump-signals = ["cumulus-pallet-parachain-system/experimental-ump-signals"] +ump-signals = [] diff --git a/cumulus/test/runtime/build.rs b/cumulus/test/runtime/build.rs index 43e60c1074a0..cabac633db7e 100644 --- a/cumulus/test/runtime/build.rs +++ b/cumulus/test/runtime/build.rs @@ -35,7 +35,7 @@ fn main() { WasmBuilder::new() .with_current_project() .enable_feature("elastic-scaling") - .enable_feature("experimental-ump-signals") + .enable_feature("ump-signals") .import_memory() .set_file_name("wasm_binary_elastic_scaling.rs") .build(); diff --git a/cumulus/test/runtime/src/lib.rs b/cumulus/test/runtime/src/lib.rs index 4abc10276af1..0e925e367524 100644 --- a/cumulus/test/runtime/src/lib.rs +++ b/cumulus/test/runtime/src/lib.rs @@ -318,7 +318,10 @@ impl cumulus_pallet_parachain_system::Config for Runtime { type CheckAssociatedRelayNumber = cumulus_pallet_parachain_system::RelayNumberMonotonicallyIncreases; type ConsensusHook = ConsensusHook; + #[cfg(feature = "ump-signals")] type SelectCore = cumulus_pallet_parachain_system::DefaultCoreSelector; + #[cfg(not(feature = "ump-signals"))] + type SelectCore = (); } impl parachain_info::Config for Runtime {} @@ -537,7 +540,7 @@ impl_runtime_apis! { } impl cumulus_primitives_core::GetCoreSelectorApi for Runtime { - fn core_selector() -> (CoreSelector, ClaimQueueOffset) { + fn core_selector() -> Option<(CoreSelector, ClaimQueueOffset)> { ParachainSystem::core_selector() } } diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 25614349486e..2a4643031bf8 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -912,15 +912,10 @@ async fn validate_candidate_exhaustive( // invalid. Ok(ValidationResult::Invalid(InvalidCandidate::CommitmentsHashMismatch)) } else { - let core_index = candidate_receipt.descriptor.core_index(); - - match (core_index, exec_kind) { + match exec_kind { // Core selectors are optional for V2 descriptors, but we still check the // descriptor core index. - ( - Some(_core_index), - PvfExecKind::Backing(_) | PvfExecKind::BackingSystemParas(_), - ) => { + PvfExecKind::Backing(_) | PvfExecKind::BackingSystemParas(_) => { let Some(claim_queue) = maybe_claim_queue else { let error = "cannot fetch the claim queue from the runtime"; gum::warn!( @@ -937,9 +932,9 @@ async fn validate_candidate_exhaustive( { gum::warn!( target: LOG_TARGET, - ?err, candidate_hash = ?candidate_receipt.hash(), - "Candidate core index is invalid", + "Candidate core index is invalid: {}", + err ); return Ok(ValidationResult::Invalid( InvalidCandidate::InvalidCoreIndex, @@ -947,7 +942,7 @@ async fn validate_candidate_exhaustive( } }, // No checks for approvals and disputes - (_, _) => {}, + _ => {}, } Ok(ValidationResult::Valid( diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 98e34a1cb4c1..795d7c93f8a7 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -30,8 +30,8 @@ use polkadot_node_subsystem_util::reexports::SubsystemContext; use polkadot_overseer::ActivatedLeaf; use polkadot_primitives::{ vstaging::{ - CandidateDescriptorV2, ClaimQueueOffset, CoreSelector, MutateDescriptorV2, UMPSignal, - UMP_SEPARATOR, + CandidateDescriptorV2, CandidateDescriptorVersion, ClaimQueueOffset, CoreSelector, + MutateDescriptorV2, UMPSignal, UMP_SEPARATOR, }, CandidateDescriptor, CoreIndex, GroupIndex, HeadData, Id as ParaId, OccupiedCoreAssumption, SessionInfo, UpwardMessage, ValidatorId, @@ -851,7 +851,7 @@ fn invalid_session_or_core_index() { )) .unwrap(); - // Validation doesn't fail for approvals, core/session index is not checked. + // Validation doesn't fail for disputes, core/session index is not checked. assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); assert_eq!(outputs.upward_messages, commitments.upward_messages); @@ -911,6 +911,69 @@ fn invalid_session_or_core_index() { assert_eq!(outputs.hrmp_watermark, 0); assert_eq!(used_validation_data, validation_data); }); + + // Test that a v1 candidate that outputs the core selector UMP signal is invalid. + let descriptor_v1 = make_valid_candidate_descriptor( + ParaId::from(1_u32), + dummy_hash(), + dummy_hash(), + pov.hash(), + validation_code.hash(), + validation_result.head_data.hash(), + dummy_hash(), + sp_keyring::Sr25519Keyring::Ferdie, + ); + let descriptor: CandidateDescriptorV2 = descriptor_v1.into(); + + perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code.hash()) + .unwrap(); + assert_eq!(descriptor.version(), CandidateDescriptorVersion::V1); + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + + for exec_kind in + [PvfExecKind::Backing(dummy_hash()), PvfExecKind::BackingSystemParas(dummy_hash())] + { + let result = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + exec_kind, + &Default::default(), + Some(Default::default()), + )) + .unwrap(); + assert_matches!(result, ValidationResult::Invalid(InvalidCandidate::InvalidCoreIndex)); + } + + // Validation doesn't fail for approvals and disputes, core/session index is not checked. + for exec_kind in [PvfExecKind::Approval, PvfExecKind::Dispute] { + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + exec_kind, + &Default::default(), + Default::default(), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); + } } #[test] @@ -1407,7 +1470,7 @@ fn compressed_code_works() { ExecutorParams::default(), PvfExecKind::Backing(dummy_hash()), &Default::default(), - Default::default(), + Some(Default::default()), )); assert_matches!(v, Ok(ValidationResult::Valid(_, _))); diff --git a/polkadot/primitives/src/vstaging/mod.rs b/polkadot/primitives/src/vstaging/mod.rs index 271f78efe090..24036e865019 100644 --- a/polkadot/primitives/src/vstaging/mod.rs +++ b/polkadot/primitives/src/vstaging/mod.rs @@ -505,6 +505,13 @@ pub enum CommittedCandidateReceiptError { /// Currenly only one such message is allowed. #[cfg_attr(feature = "std", error("Too many UMP signals"))] TooManyUMPSignals, + /// If the parachain runtime started sending core selectors, v1 descriptors are no longer + /// allowed. + #[cfg_attr( + feature = "std", + error("Candidate contains core selector but descriptor version is v1") + )] + CoreSelectorWithV1Decriptor, } macro_rules! impl_getter { @@ -603,15 +610,25 @@ impl CommittedCandidateReceiptV2 { &self, cores_per_para: &TransposedClaimQueue, ) -> Result<(), CommittedCandidateReceiptError> { + let maybe_core_selector = self.commitments.core_selector()?; + match self.descriptor.version() { - // Don't check v1 descriptors. - CandidateDescriptorVersion::V1 => return Ok(()), + CandidateDescriptorVersion::V1 => { + // If the parachain runtime started sending core selectors, v1 descriptors are no + // longer allowed. + if maybe_core_selector.is_some() { + return Err(CommittedCandidateReceiptError::CoreSelectorWithV1Decriptor) + } else { + // Nothing else to check for v1 descriptors. + return Ok(()) + } + }, CandidateDescriptorVersion::V2 => {}, CandidateDescriptorVersion::Unknown => return Err(CommittedCandidateReceiptError::UnknownVersion(self.descriptor.version)), } - let (maybe_core_index_selector, cq_offset) = self.commitments.core_selector()?.map_or_else( + let (maybe_core_index_selector, cq_offset) = maybe_core_selector.map_or_else( || (None, ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)), |(sel, off)| (Some(sel), off), ); @@ -1207,8 +1224,7 @@ mod tests { assert_eq!(new_ccr.hash(), v2_ccr.hash()); } - // Only check descriptor `core_index` field of v2 descriptors. If it is v1, that field - // will be garbage. + // V1 descriptors are forbidden once the parachain runtime started sending UMP signals. #[test] fn test_v1_descriptors_with_ump_signal() { let mut ccr = dummy_old_committed_candidate_receipt(); @@ -1234,9 +1250,12 @@ mod tests { cq.insert(CoreIndex(0), vec![v1_ccr.descriptor.para_id()].into()); cq.insert(CoreIndex(1), vec![v1_ccr.descriptor.para_id()].into()); - assert!(v1_ccr.check_core_index(&transpose_claim_queue(cq)).is_ok()); - assert_eq!(v1_ccr.descriptor.core_index(), None); + + assert_eq!( + v1_ccr.check_core_index(&transpose_claim_queue(cq)), + Err(CommittedCandidateReceiptError::CoreSelectorWithV1Decriptor) + ); } #[test] diff --git a/polkadot/zombienet-sdk-tests/tests/elastic_scaling/slot_based_3cores.rs b/polkadot/zombienet-sdk-tests/tests/elastic_scaling/slot_based_3cores.rs index 41ec1250ecc4..d4331557994a 100644 --- a/polkadot/zombienet-sdk-tests/tests/elastic_scaling/slot_based_3cores.rs +++ b/polkadot/zombienet-sdk-tests/tests/elastic_scaling/slot_based_3cores.rs @@ -63,7 +63,6 @@ async fn slot_based_3cores_test() -> Result<(), anyhow::Error> { .with_default_command("test-parachain") .with_default_image(images.cumulus.as_str()) .with_chain("elastic-scaling-mvp") - .with_default_args(vec![("--experimental-use-slot-based").into()]) .with_default_args(vec![ ("--experimental-use-slot-based").into(), ("-lparachain=debug,aura=debug").into(), @@ -161,6 +160,5 @@ async fn slot_based_3cores_test() -> Result<(), anyhow::Error> { .await?; log::info!("Test finished successfully"); - Ok(()) }