Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement PVF pre-checking using pending ExecutorParams #7139

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement PVF pre-checking using pending ExecutorParams #7139

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ea4f3c
Implement PVF pre-checking using pending ExecutorParams
s0me0ne-unkn0wn Apr 26, 2023
672ea6c
Add documentations
s0me0ne-unkn0wn Apr 27, 2023
51cf479
Add backward compatibility layer
s0me0ne-unkn0wn Apr 27, 2023
2752d6d
Fix tests
s0me0ne-unkn0wn May 1, 2023
d17b555
Merge remote-tracking branch 'origin/master' into s0me0ne/precheck-wi…
s0me0ne-unkn0wn May 1, 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
18 changes: 15 additions & 3 deletions node/core/candidate-validation/src/lib.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document this in the impl guide?

Also, will we eventually have some mechanism to re-do pre-checking when pending exec params are queued? Because say some params are queued one session after we do pre-checking for the first time - that only gives us a window of one session to do any re-checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some docs to the implementer's guide. Regarding the situation you're describing, I hope that's not a valid concern. Say we pre-check PVF in session N and configuration changes are scheduled in session N+1. So, the PVF will be enacted in session N+2, and the configuration will change in session N+3. That means we still need to check the PVF against the session N+2 configuration as we want it to be active during that session. Could the configuration changes in session N+3 render it unusable? I hope not. As @bkchr mentioned, we should carefully evaluate any executor parameters change against every PVF which is on-chain before applying them, and I believe we'll have some tooling for that in the near future. And the PVF in question is already on-chain at session N, although it's not enacted yet, and we still have to check the configuration changes against it, too.

Something that bothers me more than that is the race condition inside a single session. Say, PVF is submitted to upgrade at block 10, and validators start pre-check voting at block 11, and the configuration change is scheduled at block 15, everything is inside a single session. In that case, we really can end up with a PVF which is enacted in session N+2 but cannot be executed due to configuration changes. It's a super corner case, but nothing is impossible. That risk should be evaluated, and if proved to be a valid vector of attack we should do something to mitigate it, but I didn't think much about it yet. Purely intuitively, I don't like the idea of re-pre-checking. It seems to me the solution could be too heavyweight for such a corner case. Probably there are some simpler ways like prohibiting configuration changes in the session where any PVF upgrades are submitted or something like that (I'm just thinking aloud, not saying it's a valid solution).

Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,18 @@ where
.await
}

async fn request_pending_executor_params<Sender>(
sender: &mut Sender,
relay_parent: Hash,
) -> Result<ExecutorParams, RuntimeRequestFailed>
where
Sender: SubsystemSender<RuntimeApiMessage>,
{
let (tx, rx) = oneshot::channel();
runtime_api_request(sender, relay_parent, RuntimeApiRequest::PendingExecutorParams(tx), rx)
.await
}

async fn precheck_pvf<Sender>(
sender: &mut Sender,
mut validation_backend: impl ValidationBackend,
Expand Down Expand Up @@ -319,12 +331,12 @@ where
};

let executor_params =
if let Ok(executor_params) = executor_params_at_relay_parent(relay_parent, sender).await {
if let Ok(executor_params) = request_pending_executor_params(sender, relay_parent).await {
gum::debug!(
target: LOG_TARGET,
?relay_parent,
?validation_code_hash,
"precheck: acquired executor params for the session: {:?}",
"precheck: acquired pending executor params: {:?}",
executor_params,
);
executor_params
Expand All @@ -333,7 +345,7 @@ where
target: LOG_TARGET,
?relay_parent,
?validation_code_hash,
"precheck: failed to acquire executor params for the session, thus voting against.",
"precheck: failed to acquire pending executor params, thus voting against.",
);
return PreCheckOutcome::Invalid
};
Expand Down
18 changes: 18 additions & 0 deletions node/core/runtime-api/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub(crate) struct RequestResultCache {
LruCache<(Hash, ParaId, OccupiedCoreAssumption), Option<ValidationCodeHash>>,
version: LruCache<Hash, u32>,
disputes: LruCache<Hash, Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)>>,
pending_executor_params: LruCache<Hash, ExecutorParams>,
}

impl Default for RequestResultCache {
Expand Down Expand Up @@ -90,6 +91,7 @@ impl Default for RequestResultCache {
validation_code_hash: LruCache::new(DEFAULT_CACHE_CAP),
version: LruCache::new(DEFAULT_CACHE_CAP),
disputes: LruCache::new(DEFAULT_CACHE_CAP),
pending_executor_params: LruCache::new(DEFAULT_CACHE_CAP),
}
}
}
Expand Down Expand Up @@ -385,6 +387,21 @@ impl RequestResultCache {
) {
self.disputes.put(relay_parent, value);
}

pub(crate) fn pending_executor_params(
&mut self,
relay_parent: &Hash,
) -> Option<&ExecutorParams> {
self.pending_executor_params.get(relay_parent)
}

pub(crate) fn cache_pending_executor_params(
&mut self,
relay_parent: Hash,
value: ExecutorParams,
) {
self.pending_executor_params.put(relay_parent, value);
}
}

pub(crate) enum RequestResult {
Expand Down Expand Up @@ -422,4 +439,5 @@ pub(crate) enum RequestResult {
ValidationCodeHash(Hash, ParaId, OccupiedCoreAssumption, Option<ValidationCodeHash>),
Version(Hash, u32),
Disputes(Hash, Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)>),
PendingExecutorParams(Hash, ExecutorParams),
}
11 changes: 11 additions & 0 deletions node/core/runtime-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ where
self.requests_cache.cache_version(relay_parent, version),
Disputes(relay_parent, disputes) =>
self.requests_cache.cache_disputes(relay_parent, disputes),
PendingExecutorParams(relay_parent, pending_executor_params) => self
.requests_cache
.cache_pending_executor_params(relay_parent, pending_executor_params),
}
}

Expand Down Expand Up @@ -271,6 +274,8 @@ where
.map(|sender| Request::ValidationCodeHash(para, assumption, sender)),
Request::Disputes(sender) =>
query!(disputes(), sender).map(|sender| Request::Disputes(sender)),
Request::PendingExecutorParams(sender) => query!(pending_executor_params(), sender)
.map(|sender| Request::PendingExecutorParams(sender)),
}
}

Expand Down Expand Up @@ -490,5 +495,11 @@ where
query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender),
Request::Disputes(sender) =>
query!(Disputes, disputes(), ver = Request::DISPUTES_RUNTIME_REQUIREMENT, sender),
Request::PendingExecutorParams(sender) => query!(
PendingExecutorParams,
pending_executor_params(),
ver = Request::PENDING_EXECUTOR_PARAMS_RUNTIME_REQUIREMENT,
sender
),
}
}
6 changes: 6 additions & 0 deletions node/subsystem-types/src/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,9 @@ pub enum RuntimeApiRequest {
),
/// Returns all on-chain disputes at given block number. Available in `v3`.
Disputes(RuntimeApiSender<Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)>>),
/// Returns the latest pending executor parameter set, or the current set if no configuration
/// changes are pending
PendingExecutorParams(RuntimeApiSender<ExecutorParams>),
}

impl RuntimeApiRequest {
Expand All @@ -613,6 +616,9 @@ impl RuntimeApiRequest {

/// `ExecutorParams`
pub const EXECUTOR_PARAMS_RUNTIME_REQUIREMENT: u32 = 4;

/// Pending `ExecutorParams`
pub const PENDING_EXECUTOR_PARAMS_RUNTIME_REQUIREMENT: u32 = 5;
}

/// A message to the Runtime API subsystem.
Expand Down
10 changes: 10 additions & 0 deletions node/subsystem-types/src/runtime_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,12 @@ pub trait RuntimeApiSubsystemClient {
&self,
at: Hash,
) -> std::result::Result<Vec<sp_authority_discovery::AuthorityId>, ApiError>;

/***** Staging *****/

/// Returns the latest pending executor parameter set, or the current set if no configuration
/// changes are pending
async fn pending_executor_params(&self, at: Hash) -> Result<ExecutorParams, ApiError>;
}

#[async_trait]
Expand Down Expand Up @@ -374,4 +380,8 @@ where
) -> Result<Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)>, ApiError> {
self.runtime_api().disputes(at)
}

async fn pending_executor_params(&self, at: Hash) -> Result<ExecutorParams, ApiError> {
self.runtime_api().pending_executor_params(at)
}
}
6 changes: 6 additions & 0 deletions primitives/src/runtime_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,5 +218,11 @@ sp_api::decl_runtime_apis! {

/// Returns execution parameters for the session.
fn session_executor_params(session_index: SessionIndex) -> Option<ExecutorParams>;

/***** STAGING *****/
/// Returns the latest pending executor parameter set, or the current set if no configuration
/// changes are pending
#[api_version(5)]
fn pending_executor_params() -> ExecutorParams;
}
}
10 changes: 10 additions & 0 deletions runtime/parachains/src/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1374,4 +1374,14 @@ impl<T: Config> Pallet<T> {

Ok(())
}

/// Returns the latest pending executor parameter set, or the current set if no configuration
/// changes are pending
pub fn pending_executor_params() -> ExecutorParams {
<PendingConfigs<T>>::get()
.last()
.map(|(_, config)| config.clone())
.unwrap_or_else(Self::config)
.executor_params
}
}
9 changes: 9 additions & 0 deletions runtime/parachains/src/runtime_api_impl/vstaging.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,12 @@
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Put implementations of functions from staging APIs here.

use crate::configuration;
use primitives::ExecutorParams;

/// Returns the latest pending executor parameter set, or the current set if no configuration
/// changes are pending
pub fn pending_executor_params<T: configuration::Config>() -> ExecutorParams {
<configuration::Pallet<T>>::pending_executor_params()
}
5 changes: 5 additions & 0 deletions runtime/rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1645,6 +1645,7 @@ sp_api::impl_runtime_apis! {
}
}

#[api_version(5)]
impl primitives::runtime_api::ParachainHost<Block, Hash, BlockNumber> for Runtime {
fn validators() -> Vec<ValidatorId> {
parachains_runtime_api_impl::validators::<Runtime>()
Expand Down Expand Up @@ -1750,6 +1751,10 @@ sp_api::impl_runtime_apis! {
fn disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)> {
parachains_runtime_api_impl::get_session_disputes::<Runtime>()
}

fn pending_executor_params() -> ExecutorParams {
runtime_parachains::runtime_api_impl::vstaging::pending_executor_params::<Runtime>()
}
}

#[api_version(2)]
Expand Down
5 changes: 5 additions & 0 deletions runtime/westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,7 @@ sp_api::impl_runtime_apis! {
}
}

#[api_version(5)]
impl primitives::runtime_api::ParachainHost<Block, Hash, BlockNumber> for Runtime {
fn validators() -> Vec<ValidatorId> {
parachains_runtime_api_impl::validators::<Runtime>()
Expand Down Expand Up @@ -1466,6 +1467,10 @@ sp_api::impl_runtime_apis! {
fn disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)> {
parachains_runtime_api_impl::get_session_disputes::<Runtime>()
}

fn pending_executor_params() -> ExecutorParams {
runtime_parachains::runtime_api_impl::vstaging::pending_executor_params::<Runtime>()
}
}

impl beefy_primitives::BeefyApi<Block> for Runtime {
Expand Down