Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 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::ClientFeatures,
};
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 client_features(&self, at: Hash) -> Result<ClientFeatures, ApiError> {
Ok(self.rpc_client.parachain_host_client_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::ClientFeatures,
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_client_features(
&self,
at: RelayHash,
) -> Result<ClientFeatures, RelayChainError> {
self.call_remote_runtime_function("ParachainHost_client_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
13 changes: 12 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>,
client_features: LruMap<Hash, vstaging::ClientFeatures>,
}

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)),
client_features: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
}
}
}
Expand Down Expand Up @@ -446,6 +448,14 @@ impl RequestResultCache {
self.minimum_backing_votes.insert(session_index, minimum_backing_votes);
}

pub(crate) fn client_features(&mut self, key: &Hash) -> Option<vstaging::ClientFeatures> {
self.client_features.get(key).copied()
}

pub(crate) fn cache_client_features(&mut self, key: Hash, features: vstaging::ClientFeatures) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need those to be able to change on a per block basis. We should make ClientFeatures session buffered.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done. please have a look

self.client_features.insert(key, features);
}

pub(crate) fn disabled_validators(
&mut self,
relay_parent: &Hash,
Expand Down Expand Up @@ -540,4 +550,5 @@ pub(crate) enum RequestResult {
DisabledValidators(Hash, Vec<ValidatorIndex>),
ParaBackingState(Hash, ParaId, Option<async_backing::BackingState>),
AsyncBackingParams(Hash, async_backing::AsyncBackingParams),
ClientFeatures(Hash, vstaging::ClientFeatures),
}
10 changes: 10 additions & 0 deletions 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),
ClientFeatures(relay_parent, params) =>
self.requests_cache.cache_client_features(relay_parent, params),
}
}

Expand Down Expand Up @@ -313,6 +315,8 @@ where
Some(Request::MinimumBackingVotes(index, sender))
}
},
Request::ClientFeatures(sender) =>
query!(client_features(), sender).map(|sender| Request::ClientFeatures(sender)),
}
}

Expand Down Expand Up @@ -591,5 +595,11 @@ where
sender
)
},
Request::ClientFeatures(sender) => query!(
ClientFeatures,
client_features(),
ver = Request::CLIENT_FEATURES_RUNTIME_REQUIREMENT,
sender
),
}
}
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::ClientFeatures, 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 client_features(&self, _: Hash) -> Result<ClientFeatures, 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::ClientFeatures, 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 client features.
ClientFeatures(RuntimeApiSender<ClientFeatures>),
}

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

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

/// `Client features`
pub const CLIENT_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 client features.
async fn client_features(&self, at: Hash) -> Result<vstaging::ClientFeatures, 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 client_features(&self, at: Hash) -> Result<vstaging::ClientFeatures, ApiError> {
self.client.runtime_api().client_features(at)
}

async fn disabled_validators(&self, at: Hash) -> Result<Vec<ValidatorIndex>, ApiError> {
self.client.runtime_api().disabled_validators(at)
}
Expand Down
33 changes: 29 additions & 4 deletions polkadot/node/subsystem-util/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ 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,
ScrapedOnChainVotes, SessionIndex, SessionInfo, Signed, SigningContext, UncheckedSigned,
ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, LEGACY_MIN_BACKING_VOTES,
slashing, vstaging::ClientFeatures, 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,
};

use crate::{
Expand Down Expand Up @@ -507,3 +508,27 @@ pub async fn request_min_backing_votes(
min_backing_votes_res
}
}

/// Request the client features enabled in the runtime.
/// Prior to runtime API version 9, just return `None`.
pub async fn request_client_features(
parent: Hash,
sender: &mut impl overseer::SubsystemSender<RuntimeApiMessage>,
) -> Result<Option<ClientFeatures>> {
let res = recv_runtime(
request_from_runtime(parent, sender, |tx| RuntimeApiRequest::ClientFeatures(tx)).await,
)
.await;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 client features from the runtime is not supported by the current Runtime API",
);

Ok(None)
} else {
res.map(Some)
}
}
1 change: 1 addition & 0 deletions polkadot/primitives/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ license.workspace = true

[dependencies]
bitvec = { version = "1.0.0", default-features = false, features = ["alloc"] }
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 client features.
/// This is a staging method! Do not use on production runtimes!
#[api_version(9)]
fn client_features() -> vstaging::ClientFeatures;
}
}
10 changes: 10 additions & 0 deletions polkadot/primitives/src/vstaging/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,13 @@
//! Staging Primitives.

// Put any primitives used by staging APIs functions here

use parity_scale_codec::{Decode, Encode};
use scale_info::TypeInfo;
use serde::{Deserialize, Serialize};

bitflags::bitflags! {
#[derive(Default, TypeInfo, Encode, Decode, Serialize, Deserialize)]
/// Bit indices in the `HostCoonfiguration.client_features` that correspond to different client features.
pub struct ClientFeatures: u64 {}
}
Loading