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

Versioned PVF execution APIs #645

Open
rphmeier opened this issue May 6, 2023 · 9 comments
Open

Versioned PVF execution APIs #645

rphmeier opened this issue May 6, 2023 · 9 comments
Assignees
Labels
I4-refactor Code needs refactoring.

Comments

@rphmeier
Copy link
Contributor

rphmeier commented May 6, 2023

We will need to support changes to the interfaces of PVFs over time, with both new inputs and outputs.

Here is a description of the status quo and a proposal for versioning the PVF Wasm API.

Status Quo

The ValidationParams are what are passed into the validate_block function of any particular PVF, and they currently look like this
https://github.com/paritytech/polkadot/blob/1203b2519fed1727256556fb879c6c03c27a830d/parachain/src/primitives.rs#L354-L363

The good news is that these are generated in the candidate-validation subsystem, which has access to the CandidateReceipt, and therefore the relay_parent. This means that PersistedValidationData doesn't need to be updated, only the executor interface.

https://github.com/paritytech/polkadot/blob/1203b2519fed1727256556fb879c6c03c27a830d/node/core/candidate-validation/src/lib.rs#L588-L593

Proposed Changeset

Summary of changes:

  • Expect PVFs to provide a range of supported API versions in some data section of the Wasm module. If this doesn't exist, assume the supported version is [1]. (Q: which section is appropriate?)
  • Adjust the ArtifactState::Prepared to store the supported versions
  • Add an ExecutorParam indicating the range of allowed API versions
  • All API versions other than 1 use a new entry-point versioned_validate_block, which expects to read a (version: u32, [params bytes]) tuple
  • The PVF must always be executed with the highest API version which is both in its supported list and the allowed list.

We'd also need to update Cumulus to compile Wasm blobs which include the API version and to implement the versioned_validate_block.

ValidationParams v2

struct ValidationParams {
  // all fields from v1
  relay_parent: Hash,
}
@s0me0ne-unkn0wn
Copy link
Contributor

s0me0ne-unkn0wn commented May 8, 2023

  • (Q: which section is appropriate?)

If I'm not missing anything, it seems like just two exported constant globals are enough.

  • Add an ExecutorParam indicating the range of allowed API versions

Why do you think it's needed? The node gets the version range from those globals, and it's either aware of those versions and is able to encode the parameters or not aware, and then it cannot execute that PVF. So an additional restriction in the ExecutorParams seems redundant, or maybe you had something different in mind?

  • All API versions other than 1 use a new entry-point versioned_validate_block, which expects to read a (version: u32, [params bytes]) tuple

The version could be put in the first field of ValidationParams, the PVF decodes the first field of known length and knows how to decode the rest so that we could get rid of the explicit version parameter.

General thought: aren't we overcomplicating things here? Could we escape with a single version supported by PVF? Exactly as with the range situation, the node is either aware of that version and can encode the parameters, or it's not aware (too old) and cannot execute the PVF. In what situations would we benefit from having a range instead?

@bkchr
Copy link
Member

bkchr commented May 8, 2023

I think the most simplest way would be to have versioning on the validate_block level as we do it for host functions. So, just introduce validate_block_version_2 that expects the new parameters and returns the new/old return value. The PVF executor can then find out about the version the PVF supports by checking which validate_block functions are exported.

General thought: aren't we overcomplicating things here? Could we escape with a single version supported by PVF? Exactly as with the range situation, the node is either aware of that version and can encode the parameters, or it's not aware (too old) and cannot execute the PVF. In what situations would we benefit from having a range instead?

We will need to support multiple versions. First point being that we first release the changes to the nodes and can also in parallel let Cumulus already export the new functions. Then, when we enable the new function there will maybe already be some Parachains supporting it, but there will be quite a lot that will only support the old validate_block. Maybe at some point in the future, like in one year, we can remove the support for the old validate_block.

@rphmeier
Copy link
Contributor Author

rphmeier commented May 8, 2023

Why do you think it's needed? The node gets the version range from those globals, and it's either aware of those versions and is able to encode the parameters or not aware

If two different entry-points / versions have two different behaviors, then it could lead to consensus splits when some validator nodes have updated to be able to validate with some version N and other nodes can only validate with some version less than N.

The entry-point/version that any particular candidate is executed with needs to be the exact same across all validators. Parachain runtimes may update their code to support the new entrypoint before all validators have upgraded, so the choice of which version to use cannot be safely determined by the validator node version and the exposed entry points.

Because of that, we clearly need a maximum version to be determined by the relay-chain runtime. Then as Basti mentions, we may want to set minimum versions to deprecate old logic, though this is probably years down the line. Still, seems like low-effort future proofing that we might as well knock out at the same time.

I think the most simplest way would be to have versioning on the validate_block level as we do it for host functions. So, just introduce validate_block_version_2 that expects the new parameters and returns the new/old return value

I'm not opinionated on the mechanics here because I'm no Wasm expert, but I am pretty sure we need the PVF to support some contiguous range of versions because of the maximum/minimum requirements being determined by the relay chain runtime. Doing a linear search to check for existence of N entry points is small, because N should be small and we only do it on preparation anyway.

@bkchr
Copy link
Member

bkchr commented May 8, 2023

Doing a linear search to check for existence of N entry points is small, because N should be small and we only do it on preparation anyway.

Should be fairly simple. We can just get all exports with: https://docs.rs/wasmtime/latest/wasmtime/struct.Module.html#method.exports and then filter for functions and check the names.

@slumber

This comment was marked as resolved.

@bkchr

This comment was marked as resolved.

@slumber

This comment was marked as resolved.

@bkchr

This comment was marked as resolved.

@slumber

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring.
Projects
Status: Backlog
Development

No branches or pull requests

5 participants