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
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
93 changes: 80 additions & 13 deletions node/core/candidate-validation/src/lib.rs
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 @@ -238,7 +238,11 @@ async fn run<Context>(
}
}

struct RuntimeRequestFailed;
enum RuntimeRequestFailed {
Dropped,
InternalError,
NotSupported,
}

async fn runtime_api_request<T, Sender>(
sender: &mut Sender,
Expand All @@ -258,7 +262,7 @@ where
.map_err(|_| {
gum::debug!(target: LOG_TARGET, ?relay_parent, "Runtime API request dropped");

RuntimeRequestFailed
RuntimeRequestFailed::Dropped
})
.and_then(|res| {
res.map_err(|e| {
Expand All @@ -269,7 +273,7 @@ where
"Runtime API request internal error"
);

RuntimeRequestFailed
RuntimeRequestFailed::InternalError
})
})
}
Expand All @@ -292,6 +296,43 @@ 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();
sender
.send_message(
RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::PendingExecutorParams(tx))
.into(),
)
.await;

rx.await
.map_err(|_| {
gum::debug!(target: LOG_TARGET, ?relay_parent, "Runtime API request dropped");

RuntimeRequestFailed::Dropped
})
.and_then(|res| {
res.map_err(|e| {
gum::debug!(
target: LOG_TARGET,
?relay_parent,
err = ?e,
"Runtime API request internal error"
);
match e {
RuntimeApiError::Execution { .. } => RuntimeRequestFailed::InternalError,
RuntimeApiError::NotSupported { .. } => RuntimeRequestFailed::NotSupported,
}
})
})
}

async fn precheck_pvf<Sender>(
sender: &mut Sender,
mut validation_backend: impl ValidationBackend,
Expand All @@ -318,25 +359,52 @@ where
},
};

let executor_params =
if let Ok(executor_params) = executor_params_at_relay_parent(relay_parent, sender).await {
let executor_params = match request_pending_executor_params(sender, relay_parent).await {
Ok(executor_params) => {
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
} else {
},
Err(RuntimeRequestFailed::NotSupported) => {
// Old runtime that doesn't support retrieving pending configuration. Use the
// current configuration as a backward compatibility measure.
// TODO: Remove this after all the networks are upgraded to runtimes supporting
Copy link
Member

Choose a reason for hiding this comment

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

Todos should reference a ticket.

// pending_executor_params()
if let Ok(executor_params) = executor_params_at_relay_parent(relay_parent, sender).await
{
gum::debug!(
target: LOG_TARGET,
?relay_parent,
?validation_code_hash,
"precheck: acquired current executor params: {:?}",
executor_params,
);
executor_params
} else {
eskimor marked this conversation as resolved.
Show resolved Hide resolved
gum::warn!(
target: LOG_TARGET,
?relay_parent,
?validation_code_hash,
"precheck: failed to acquire current executor params, thus voting against.",
);
return PreCheckOutcome::Invalid
}
},
Err(_) => {
gum::warn!(
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
};
},
};

let timeout = pvf_prep_timeout(&executor_params, PvfPrepTimeoutKind::Precheck);

Expand Down Expand Up @@ -388,7 +456,7 @@ where
.await;

match d {
Ok(None) | Err(RuntimeRequestFailed) => return AssumptionCheckOutcome::BadRequest,
Ok(None) | Err(_) => return AssumptionCheckOutcome::BadRequest,
Ok(Some(d)) => d,
}
};
Expand All @@ -406,7 +474,7 @@ where
.await;

match validation_code {
Ok(None) | Err(RuntimeRequestFailed) => AssumptionCheckOutcome::BadRequest,
Ok(None) | Err(_) => AssumptionCheckOutcome::BadRequest,
Ok(Some(v)) => AssumptionCheckOutcome::Matches(validation_data, v),
}
} else {
Expand Down Expand Up @@ -516,8 +584,7 @@ where
{
Ok(true) => {},
Ok(false) => return Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs)),
Err(RuntimeRequestFailed) =>
return Err(ValidationFailed("Check Validation Outputs: Bad request".into())),
Err(_) => return Err(ValidationFailed("Check Validation Outputs: Bad request".into())),
}
}

Expand Down
18 changes: 18 additions & 0 deletions node/core/runtime-api/src/cache.rs
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
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
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
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
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;
}
}
3 changes: 3 additions & 0 deletions roadmap/implementers-guide/src/pvf-prechecking.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ The logic described above is implemented by the [paras] module.

On the node-side, there is a PVF pre-checking [subsystem][pvf-prechecker-subsystem] that scans the chain for new PVFs via using [runtime APIs][pvf-runtime-api]. Upon finding a new PVF, the subsystem will initiate a PVF pre-checking request and wait for the result. Whenever the result is obtained, the subsystem will use the [runtime API][pvf-runtime-api] to submit a vote for the PVF. The vote is an unsigned transaction. The vote will be distributed via the gossip similarly to a normal transaction. Eventually a block producer will include the vote into the block where it will be handled by the [runtime][paras].

It is worth noting that executor environment parameters, which are part of [configuration], may affect the results of PVF pre-checking. For example, changing limits exposed by execution environment parameters may render valid a PVF that was invalid before the parameters changed. To perform pre-checking deterministically, the pre-checking subsystem uses pending execution environment parameters instead of the current ones. Pending configurations changes are postponed for two sessions, as well as the PVF upgrade, so a PVF pre-checked in session N is guaranteed to be valid and usable in session N+2 no matter if any configuration changes are pending or not.

## Summary

Parachains' and parathreads' validation function is described by a wasm module that we refer to as a PVF.
Expand All @@ -62,3 +64,4 @@ Besides pre-checking, preparation can also be triggered by execution, since a co
[paras]: runtime/paras.md
[pvf-runtime-api]: runtime-api/pvf-prechecking.md
[pvf-prechecker-subsystem]: node/utility/pvf-prechecker.md
[configuration]: runtime/configuration.md
10 changes: 10 additions & 0 deletions runtime/parachains/src/configuration.rs
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
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
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
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