Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add NodeFeatures field to HostConfiguration and runtime API #2177

Merged
merged 20 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8aa7f3a
add ClientFeatures field to HostConfiguration and runtime API
alindima Nov 6, 2023
d88b791
".git/.scripts/commands/fmt/fmt.sh"
Nov 6, 2023
61d5ad0
cache client_features by session index
alindima Nov 7, 2023
b4fcb1c
rename ClientFeatures to NodeFeatures
alindima Nov 9, 2023
79bfc97
replace bitflags with bitvec
alindima Nov 9, 2023
42c00b8
add `toggle_node_feature` config pallet extrinsic
alindima Nov 9, 2023
6e8e33d
Merge remote-tracking branch 'origin/master' into alindima/add-client…
alindima Nov 9, 2023
4872169
also use BitVec in the HostConfig
alindima Nov 9, 2023
4e2f764
replace toggle with explicit set
alindima Nov 9, 2023
72a71ff
add separate weight for set_node_feature
alindima Nov 10, 2023
b1abeb4
fix benchmark
alindima Nov 10, 2023
db20f6e
remove bitflags dependency and update comment
alindima Nov 10, 2023
fd393d0
fix benchmark again
alindima Nov 10, 2023
da7ce3a
Merge remote-tracking branch 'origin/master' into alindima/add-client…
alindima Nov 10, 2023
6413b80
".git/.scripts/commands/bench/bench.sh" --subcommand=runtime --runtim…
Nov 10, 2023
55adc2f
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
Nov 10, 2023
3d8d30f
".git/.scripts/commands/bench/bench.sh" --subcommand=runtime --runtim…
Nov 10, 2023
d3b06b8
Merge remote-tracking branch 'origin/master' into alindima/add-client…
alindima Nov 13, 2023
42cff85
Merge remote-tracking branch 'origin/master' into alindima/add-client…
alindima Nov 14, 2023
9889c2f
Merge branch 'master' into alindima/add-client-features-to-runtime
alindima Nov 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use polkadot_overseer::RuntimeApiSubsystemClient;
use polkadot_primitives::{
async_backing::{AsyncBackingParams, BackingState},
slashing,
vstaging::NodeFeatures,
};
use sc_authority_discovery::{AuthorityDiscovery, Error as AuthorityDiscoveryError};
use sp_api::{ApiError, RuntimeApiInfo};
Expand Down Expand Up @@ -364,6 +365,10 @@ impl RuntimeApiSubsystemClient for BlockChainRpcClient {
) -> Result<Option<BackingState>, ApiError> {
Ok(self.rpc_client.parachain_host_para_backing_state(at, para_id).await?)
}

async fn node_features(&self, at: Hash) -> Result<NodeFeatures, ApiError> {
Ok(self.rpc_client.parachain_host_node_features(at).await?)
}
}

#[async_trait::async_trait]
Expand Down
12 changes: 11 additions & 1 deletion cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ use parity_scale_codec::{Decode, Encode};
use cumulus_primitives_core::{
relay_chain::{
async_backing::{AsyncBackingParams, BackingState},
slashing, BlockNumber, CandidateCommitments, CandidateEvent, CandidateHash,
slashing,
vstaging::NodeFeatures,
BlockNumber, CandidateCommitments, CandidateEvent, CandidateHash,
CommittedCandidateReceipt, CoreState, DisputeState, ExecutorParams, GroupRotationInfo,
Hash as RelayHash, Header as RelayHeader, InboundHrmpMessage, OccupiedCoreAssumption,
PvfCheckStatement, ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidationCode,
Expand Down Expand Up @@ -597,6 +599,14 @@ impl RelayChainRpcClient {
.await
}

pub async fn parachain_host_node_features(
&self,
at: RelayHash,
) -> Result<NodeFeatures, RelayChainError> {
self.call_remote_runtime_function("ParachainHost_node_features", at, None::<()>)
.await
}

pub async fn parachain_host_disabled_validators(
&self,
at: RelayHash,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub use pallet_xcm;
pub use xcm::prelude::{AccountId32, WeightLimit};

decl_test_relay_chains! {
#[api_version(8)]
#[api_version(9)]
pub struct Westend {
genesis = westend::genesis(),
on_init = (),
Expand All @@ -62,7 +62,7 @@ decl_test_relay_chains! {
AssetRate: westend_runtime::AssetRate,
}
},
#[api_version(8)]
#[api_version(9)]
pub struct Rococo {
genesis = rococo::genesis(),
on_init = (),
Expand All @@ -78,7 +78,7 @@ decl_test_relay_chains! {
Hrmp: rococo_runtime::Hrmp,
}
},
#[api_version(8)]
#[api_version(9)]
pub struct Wococo {
genesis = rococo::genesis(),
on_init = (),
Expand Down
20 changes: 19 additions & 1 deletion polkadot/node/core/runtime-api/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use schnellru::{ByLength, LruMap};
use sp_consensus_babe::Epoch;

use polkadot_primitives::{
async_backing, slashing, AuthorityDiscoveryId, BlockNumber, CandidateCommitments,
async_backing, slashing, vstaging, AuthorityDiscoveryId, BlockNumber, CandidateCommitments,
CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState, DisputeState,
ExecutorParams, GroupRotationInfo, Hash, Id as ParaId, InboundDownwardMessage,
InboundHrmpMessage, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
Expand Down Expand Up @@ -67,6 +67,7 @@ pub(crate) struct RequestResultCache {
disabled_validators: LruMap<Hash, Vec<ValidatorIndex>>,
para_backing_state: LruMap<(Hash, ParaId), Option<async_backing::BackingState>>,
async_backing_params: LruMap<Hash, async_backing::AsyncBackingParams>,
node_features: LruMap<SessionIndex, vstaging::NodeFeatures>,
}

impl Default for RequestResultCache {
Expand Down Expand Up @@ -100,6 +101,7 @@ impl Default for RequestResultCache {
disabled_validators: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
para_backing_state: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
async_backing_params: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
node_features: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
}
}
}
Expand Down Expand Up @@ -446,6 +448,21 @@ impl RequestResultCache {
self.minimum_backing_votes.insert(session_index, minimum_backing_votes);
}

pub(crate) fn node_features(
&mut self,
session_index: SessionIndex,
) -> Option<&vstaging::NodeFeatures> {
self.node_features.get(&session_index).map(|f| &*f)
}

pub(crate) fn cache_node_features(
&mut self,
session_index: SessionIndex,
features: vstaging::NodeFeatures,
) {
self.node_features.insert(session_index, features);
}

pub(crate) fn disabled_validators(
&mut self,
relay_parent: &Hash,
Expand Down Expand Up @@ -540,4 +557,5 @@ pub(crate) enum RequestResult {
DisabledValidators(Hash, Vec<ValidatorIndex>),
ParaBackingState(Hash, ParaId, Option<async_backing::BackingState>),
AsyncBackingParams(Hash, async_backing::AsyncBackingParams),
NodeFeatures(SessionIndex, vstaging::NodeFeatures),
}
23 changes: 22 additions & 1 deletion polkadot/node/core/runtime-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ where
.cache_para_backing_state((relay_parent, para_id), constraints),
AsyncBackingParams(relay_parent, params) =>
self.requests_cache.cache_async_backing_params(relay_parent, params),
NodeFeatures(session_index, params) =>
self.requests_cache.cache_node_features(session_index, params),
}
}

Expand Down Expand Up @@ -313,6 +315,15 @@ where
Some(Request::MinimumBackingVotes(index, sender))
}
},
Request::NodeFeatures(index, sender) => {
if let Some(value) = self.requests_cache.node_features(index) {
self.metrics.on_cached_request();
let _ = sender.send(Ok(value.clone()));
None
} else {
Some(Request::NodeFeatures(index, sender))
}
},
}
}

Expand Down Expand Up @@ -408,6 +419,9 @@ where

macro_rules! query {
($req_variant:ident, $api_name:ident ($($param:expr),*), ver = $version:expr, $sender:expr) => {{
query!($req_variant, $api_name($($param),*), ver = $version, $sender, result = ( relay_parent $(, $param )* ) )
}};
($req_variant:ident, $api_name:ident ($($param:expr),*), ver = $version:expr, $sender:expr, result = ( $($results:expr),* ) ) => {{
let sender = $sender;
let version: u32 = $version; // enforce type for the version expression
let runtime_version = client.api_version_parachain_host(relay_parent).await
Expand Down Expand Up @@ -441,7 +455,7 @@ where
metrics.on_request(res.is_ok());
let _ = sender.send(res.clone());

res.ok().map(|res| RequestResult::$req_variant(relay_parent, $( $param, )* res))
res.ok().map(|res| RequestResult::$req_variant($( $results, )* res))
}}
}

Expand Down Expand Up @@ -591,5 +605,12 @@ where
sender
)
},
Request::NodeFeatures(index, sender) => query!(
NodeFeatures,
node_features(),
ver = Request::NODE_FEATURES_RUNTIME_REQUIREMENT,
sender,
result = (index)
),
}
}
16 changes: 10 additions & 6 deletions polkadot/node/core/runtime-api/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ use polkadot_node_primitives::{BabeAllowedSlots, BabeEpoch, BabeEpochConfigurati
use polkadot_node_subsystem::SpawnGlue;
use polkadot_node_subsystem_test_helpers::make_subsystem_context;
use polkadot_primitives::{
async_backing, slashing, AuthorityDiscoveryId, BlockNumber, CandidateCommitments,
CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState, DisputeState,
ExecutorParams, GroupRotationInfo, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage,
OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, ScrapedOnChainVotes,
SessionIndex, SessionInfo, Slot, ValidationCode, ValidationCodeHash, ValidatorId,
ValidatorIndex, ValidatorSignature,
async_backing, slashing, vstaging::NodeFeatures, AuthorityDiscoveryId, BlockNumber,
CandidateCommitments, CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState,
DisputeState, ExecutorParams, GroupRotationInfo, Id as ParaId, InboundDownwardMessage,
InboundHrmpMessage, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
ScrapedOnChainVotes, SessionIndex, SessionInfo, Slot, ValidationCode, ValidationCodeHash,
ValidatorId, ValidatorIndex, ValidatorSignature,
};
use sp_api::ApiError;
use sp_core::testing::TaskExecutor;
Expand Down Expand Up @@ -269,6 +269,10 @@ impl RuntimeApiSubsystemClient for MockSubsystemClient {
todo!("Not required for tests")
}

async fn node_features(&self, _: Hash) -> Result<NodeFeatures, ApiError> {
todo!("Not required for tests")
}

async fn disabled_validators(&self, _: Hash) -> Result<Vec<ValidatorIndex>, ApiError> {
todo!("Not required for tests")
}
Expand Down
17 changes: 11 additions & 6 deletions polkadot/node/subsystem-types/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ use polkadot_node_primitives::{
ValidationResult,
};
use polkadot_primitives::{
async_backing, slashing, AuthorityDiscoveryId, BackedCandidate, BlockNumber, CandidateEvent,
CandidateHash, CandidateIndex, CandidateReceipt, CollatorId, CommittedCandidateReceipt,
CoreState, DisputeState, ExecutorParams, GroupIndex, GroupRotationInfo, Hash,
Header as BlockHeader, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage,
MultiDisputeStatementSet, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
PvfExecTimeoutKind, SessionIndex, SessionInfo, SignedAvailabilityBitfield,
async_backing, slashing, vstaging::NodeFeatures, AuthorityDiscoveryId, BackedCandidate,
BlockNumber, CandidateEvent, CandidateHash, CandidateIndex, CandidateReceipt, CollatorId,
CommittedCandidateReceipt, CoreState, DisputeState, ExecutorParams, GroupIndex,
GroupRotationInfo, Hash, Header as BlockHeader, Id as ParaId, InboundDownwardMessage,
InboundHrmpMessage, MultiDisputeStatementSet, OccupiedCoreAssumption, PersistedValidationData,
PvfCheckStatement, PvfExecTimeoutKind, SessionIndex, SessionInfo, SignedAvailabilityBitfield,
SignedAvailabilityBitfields, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex,
ValidatorSignature,
};
Expand Down Expand Up @@ -706,6 +706,8 @@ pub enum RuntimeApiRequest {
///
/// If it's not supported by the Runtime, the async backing is said to be disabled.
AsyncBackingParams(RuntimeApiSender<async_backing::AsyncBackingParams>),
/// Get the node features.
NodeFeatures(SessionIndex, RuntimeApiSender<NodeFeatures>),
}

impl RuntimeApiRequest {
Expand Down Expand Up @@ -734,6 +736,9 @@ impl RuntimeApiRequest {

/// `DisabledValidators`
pub const DISABLED_VALIDATORS_RUNTIME_REQUIREMENT: u32 = 8;

/// `Node features`
pub const NODE_FEATURES_RUNTIME_REQUIREMENT: u32 = 9;
}

/// A message to the Runtime API subsystem.
Expand Down
22 changes: 16 additions & 6 deletions polkadot/node/subsystem-types/src/runtime_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@

use async_trait::async_trait;
use polkadot_primitives::{
async_backing, runtime_api::ParachainHost, slashing, Block, BlockNumber, CandidateCommitments,
CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState, DisputeState,
ExecutorParams, GroupRotationInfo, Hash, Id, InboundDownwardMessage, InboundHrmpMessage,
OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, ScrapedOnChainVotes,
SessionIndex, SessionInfo, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex,
ValidatorSignature,
async_backing, runtime_api::ParachainHost, slashing, vstaging, Block, BlockNumber,
CandidateCommitments, CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState,
DisputeState, ExecutorParams, GroupRotationInfo, Hash, Id, InboundDownwardMessage,
InboundHrmpMessage, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidationCode, ValidationCodeHash,
ValidatorId, ValidatorIndex, ValidatorSignature,
};
use sc_transaction_pool_api::OffchainTransactionPoolFactory;
use sp_api::{ApiError, ApiExt, ProvideRuntimeApi};
Expand Down Expand Up @@ -257,8 +257,14 @@ pub trait RuntimeApiSubsystemClient {
) -> Result<Option<async_backing::BackingState>, ApiError>;

// === v8 ===

/// Gets the disabled validators at a specific block height
async fn disabled_validators(&self, at: Hash) -> Result<Vec<ValidatorIndex>, ApiError>;

// === v9 ===

/// Get the node features.
async fn node_features(&self, at: Hash) -> Result<vstaging::NodeFeatures, ApiError>;
}

/// Default implementation of [`RuntimeApiSubsystemClient`] using the client.
Expand Down Expand Up @@ -508,6 +514,10 @@ where
self.client.runtime_api().async_backing_params(at)
}

async fn node_features(&self, at: Hash) -> Result<vstaging::NodeFeatures, ApiError> {
self.client.runtime_api().node_features(at)
}

async fn disabled_validators(&self, at: Hash) -> Result<Vec<ValidatorIndex>, ApiError> {
self.client.runtime_api().disabled_validators(at)
}
Expand Down
33 changes: 31 additions & 2 deletions polkadot/node/subsystem-util/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ use polkadot_node_subsystem::{
};
use polkadot_node_subsystem_types::UnpinHandle;
use polkadot_primitives::{
slashing, AsyncBackingParams, CandidateEvent, CandidateHash, CoreState, EncodeAs,
ExecutorParams, GroupIndex, GroupRotationInfo, Hash, IndexedVec, OccupiedCore,
slashing, vstaging::NodeFeatures, AsyncBackingParams, CandidateEvent, CandidateHash, CoreState,
EncodeAs, ExecutorParams, GroupIndex, GroupRotationInfo, Hash, IndexedVec, OccupiedCore,
ScrapedOnChainVotes, SessionIndex, SessionInfo, Signed, SigningContext, UncheckedSigned,
ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, LEGACY_MIN_BACKING_VOTES,
};
Expand Down Expand Up @@ -507,3 +507,32 @@ pub async fn request_min_backing_votes(
min_backing_votes_res
}
}

/// Request the node features enabled in the runtime.
/// Pass in the session index for caching purposes, as it should only change on session boundaries.
/// Prior to runtime API version 9, just return `None`.
pub async fn request_node_features(
parent: Hash,
session_index: SessionIndex,
sender: &mut impl overseer::SubsystemSender<RuntimeApiMessage>,
) -> Result<Option<NodeFeatures>> {
let res = recv_runtime(
request_from_runtime(parent, sender, |tx| {
RuntimeApiRequest::NodeFeatures(session_index, tx)
})
.await,
)
.await;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On session changes it would be a good idea to emit an warning to upgrade the node if not all bits make sense(at least a required feature we don't have)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's doable if we want to be able to add new features without a runtime upgrade.
The runtime does not have any idea of what the feature bits mean. They only make sense on the node-side.

If we want to emit warnings for new features that are added, they have to come via a runtime upgrade (which would defeat the purpose of what I've done with this PR so far in order to have seamless feature addition/enabling). I think it's best to leave it as it is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runtime does not have any idea of what the feature bits mean. They only make sense on the node-side.

Yes exactly, those are bits that only the client understands. This is just to inform the operator, that currently a client requirement is not fulfilled and a node upgrade is required.

If we want to emit warnings for new features that are added, they have to come via a runtime upgrade (which would defeat the purpose of what I've done with this PR so far in order to have seamless feature addition/enabling). I think it's best to leave it as it is

No runtime upgrade is needed as the client will see a new 1 bit when the feature gets enabled. Even if it doesn't know what it means it can still print the warning saying to upgrade the node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, I didn't fully get this from the beginning.
I can make the RuntimeApiSubsystem track the last session and when seeing a new session check the highest bit set returned from the node_features runtime API. If that's higher than some LAST_NODE_FEATURE constant on the node side, issue a warning.

As spoken on element, I'll add this on the next PR that uses this runtime API, as it'd be redundant right now.
Good suggestion 👍🏻

if let Err(Error::RuntimeRequest(RuntimeApiError::NotSupported { .. })) = res {
gum::trace!(
target: LOG_TARGET,
?parent,
"Querying the node features from the runtime is not supported by the current Runtime API",
);

Ok(None)
} else {
res.map(Some)
}
}
3 changes: 2 additions & 1 deletion polkadot/primitives/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ edition.workspace = true
license.workspace = true

[dependencies]
bitvec = { version = "1.0.0", default-features = false, features = ["alloc"] }
bitvec = { version = "1.0.0", default-features = false, features = ["alloc", "serde"] }
bitflags = "1.3.2"
hex-literal = "0.4.1"
parity-scale-codec = { version = "3.6.1", default-features = false, features = ["bit-vec", "derive"] }
scale-info = { version = "2.10.0", default-features = false, features = ["bit-vec", "derive", "serde"] }
Expand Down
15 changes: 11 additions & 4 deletions polkadot/primitives/src/runtime_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,10 @@
//! separated from the stable primitives.

use crate::{
async_backing, slashing, AsyncBackingParams, BlockNumber, CandidateCommitments, CandidateEvent,
CandidateHash, CommittedCandidateReceipt, CoreState, DisputeState, ExecutorParams,
GroupRotationInfo, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex,
async_backing, slashing, vstaging, AsyncBackingParams, BlockNumber, CandidateCommitments,
CandidateEvent, CandidateHash, CommittedCandidateReceipt, CoreState, DisputeState,
ExecutorParams, GroupRotationInfo, OccupiedCoreAssumption, PersistedValidationData,
PvfCheckStatement, ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex,
ValidatorSignature,
};
use parity_scale_codec::{Decode, Encode};
Expand Down Expand Up @@ -264,5 +264,12 @@ sp_api::decl_runtime_apis! {
/// Returns a list of all disabled validators at the given block.
#[api_version(8)]
fn disabled_validators() -> Vec<ValidatorIndex>;

/***** Added in v9 *****/

/// Get node features.
/// This is a staging method! Do not use on production runtimes!
#[api_version(9)]
fn node_features() -> vstaging::NodeFeatures;
}
}
Loading