Upgradeable validation functions

[rphmeier]

cc @bkchr @coriolinus

Closes #825

This uses a time-limit approach to allowing parachains to upgrade their validation code, as opposed to a fees-based approach as outlined in the issue. The most important changes are in polkadot-runtime-common and summarized here:

• Track times in history when code was replaced for parachains and use that to determine the correct code
• Replaced code is kept in the relay-chain state for a configured time-period.
• Clean up replaced code after a configured delay
• Parachains, when cleaned up, have their validation code placed into the state and cleaned up after the same delay.
• Parachains cannot apply code changes more than once per configured delay.
• Pending code is not applied until after a configured delay

Other miscellaneous changes:

• Extracted certain types to the parachain crate as mentioned in Add current relay block to ValidationParams #879 . These types are re-exported via primitives/parachain.
• Update the parachain-wasm interface to allow for code changes
• Implement a test parachain which does code changes and test it. This example logic can be used as a basis for Substrate implementation

[rphmeier]

cc @arkpar Any idea why the wasm-executor test seems to be failing? I haven't changed this logic but did shuffle some crates around

[arkpar]

Not sure how the test failure could have happened. Unless CI machine was paused for a few seconds in the middle of the the test execution, it should timeout external validation function execution. That timeout was triggered too late apparently. It worked after restart. Please let me know if this happens again.

[coriolinus]

I don't feel competent to review this PR as a whole; there's a lot of changes in here about which I just don't yet know enough to predict the resulting changes in behavior. However, from the narrow perspective of enabling me to handle parachain validation function changes within cumulus, this definitely appears to work.

Just ensuring that I understand the intention behind this design, here's the happy path as I understand it:

• a superuser (or, todo, referendum) approves a new validation function
• a support pallet (in progress) looks at validation_params.code_upgrade_allowed and keeps track of the relay chain block number at which an upgrade would be legal
• The first time that validation_params.relay_chain_height >= upgrade_block, it returns the new function in ValidationResult, which notifies polkadot that the new function is ready to adopt. At all other times, ValidationResult.new_validation_code == None.

Is that correct?

[coriolinus]

Suggested change

	#[derive(PartialEq, Eq, Decode)]
	#[cfg_attr(feature = "std", derive(Debug, Encode))]
	pub struct ValidationParams {
	#[derive(PartialEq, Eq, Encode, Decode)]
	#[cfg_attr(feature = "std", derive(Debug))]
	pub struct ValidationParams {

It would be helpful if coding on this struct were always bidirectional: that would let me encode it into storage (under a constant key) so that pallets could publish things like the max code size and whether or not code upgrades are currently allowed.

Alternate suggestion, in case BlockData or HeadData are large: factor out something like

#[derive(PartialEq, Eq, Encode, Decode, Clone, Copy)]
#[cfg_attr(feature = "std", derive(Debug))]
pub struct ValidationFunctionParams {
	/// The maximum code size permitted, in bytes.
	pub max_code_size: u32,
	/// The current relay-chain block number.
	pub relay_chain_height: RelayChainBlockNumber,
	/// Whether a code upgrade is allowed or not, and at which height the upgrade
	/// would be applied after, if so. The parachain logic should apply any upgrade
	/// issued in this block after the first block
	/// with `relay_chain_height` at least this value, if `Some`. if `None`, issue
	/// no upgrade.
	pub code_upgrade_allowed: Option<NonZero<RelayChainBlockNumber>>,
}

That's all the data I'm really interested in forwarding for pallet use; this struct could then become a public member of ValidationParams.

[rphmeier]

It would be helpful if coding on this struct were always bidirectional: that would let me encode it into storage (under a constant key) so that pallets could publish things like the max code size and whether or not code upgrades are currently allowed.

Yes, but at the cost of larger runtime code size. You can build it with the std feature to get Encode functionality.

[coriolinus]

Would you be open to the alternate suggestion, breaking out ValidationFunctionParams? I'm already doing that in the Cumulus branch; it's not hard, but it would be easier to just copy out the struct directly.

https://github.com/paritytech/cumulus/blob/97b96a9802d290dadb5b3e7fe7c4a7daab0bb627/runtime/src/lib.rs#L95-L122

[bkchr]

I think this stuff should be kept locally to Cumulus.

[rphmeier]

Can you elaborate on why a Cumulus chain would want to put this in storage? These parameters will change for the execution of every block and cannot be relied on beyond a single block's execution.

The only thing a Cumulus should need to store to process a pending upgrade is what the inner value of code_upgrade_allowed: Some was when the upgrade was initiated. Then the next time you get a ValidationFunctionParams with relay_chain_height >= scheduled_at, you apply the code change at the end of the block by writing to :code storage.

Is it a question of putting it in the ephemeral storage, which is wiped at the end of the block, so other modules can query this value? I think you only need to put code_upgrade_allowed in such ephemeral storage.

And I'll go a little off topic here, but I wrote this code with the understanding that Cumulus would have an inherent type with the ValidationFunctionParams which is required in each block. Then the validation function is checking that the passed function params are equal to the ones in the inherent. This means that you can get this data into the ephemeral store by implementing a normal ensure_none Call. Is this in line with your thinking?

[coriolinus]

Can you elaborate on why a Cumulus chain would want to put this in storage?

The main goal is to propagate the values beyond Cumulus, so that a pallet can use them. It enables the pallet to publish methods like can_set_code() -> bool or max_code_size() -> u32, but more importantly, it lets the pallet generate nice errors like this:

pub fn set_code(origin, validation_function: ValidationFunction) {
    let vfp = Self::validation_function_params();
    ensure!(validation_function.len() <= vfp.max_code_size as usize, Error::<T>::TooBig);
    let apply_block = vfp.code_upgrade_allowed.ok_or(Error::<T>::ProhibitedByPolkadot)?;
    ...
}

To a user, that's a much nicer way to know that this is the wrong time or the wrong size for a validation function upgrade than panicing during the Cumulus validate_block function.

I spoke with @bkchr earlier this afternoon, and he explained why literally putting it into storage is a bad idea (it messes with the storage root). We came up with workaround which still ends up going through the storage interface, even though it doesn't actually modify the storage; for that reason, it would be still good to derive Encode, Decode on the struct.

Is it a question of putting it in the ephemeral storage... I think you only need to put code_upgrade_allowed in such ephemeral storage.

Essentially, yes, that's why we're using it. We do use all three fields, though:

• max_code_size: so that we can return an error instead of panicing if submitted code is too big
• relay_chain_height: so we know when to set :code given a previously scheduled validation function upgrade
• code_upgrade_allowed: so we know when to schedule a validation function upgrade

I wrote this code with the understanding that Cumulus would have an inherent type with the ValidationFunctionParams

I'm also looking into how to implement that. However, even if the upgrade pallet gets the validation function params from an inherent instead of by directly querying a special storage key, it's not yet clear to me how the inherent would get access to them except via the magic storage key.

[coriolinus]

Additionally: it would be helpful to add a field to ValidationParams / ValidationFunctionParams. It could be as simple as

expect_upgrade_this_block: bool,

Rationale: we can't use code_upgrade_allowed to validate all inserts to :code. Obviously the upgrade pallet will be well-behaved in this regard, but not all users will discover and use that pallet, so we should guard against errors in DIY user code.

Scenario:

• Block 1000: can_set_code == Some(1500). User sets some code, which is returned in the ValidationResult.
• Blocks 1001-1499: can_set_code == None
• Block 1500: can_set_code == None || can_set_code == Some(2000). I don't know which condition actually applies, but neither value indicates that it is legal to set :code on this block, right now.

Given expect_upgrade_this_block, we can inspect all writes to :code and ensure that they are only performed when legal. Without it, we have to either store the scheduled upgrade block within validate_block (which breaks the storage root), or skip the check entirely.

[rphmeier]

Ok, I will try to address these points one-by-one.

Essentially, yes, that's why we're using it. We do use all three fields, though:

• max_code_size: so that we can return an error instead of panicing if submitted code is too big
• relay_chain_height: so we know when to set :code given a previously scheduled validation function upgrade
• code_upgrade_allowed: so we know when to schedule a validation function upgrade

I would still recommend to create a separate type for these fields, as we will add more stuff to ValidationFunctionParams including incoming messages and other fields you would not want to necessarily include in the ephemeral storage.

Additionally: it would be helpful to add a field to ValidationParams / ValidationFunctionParams

expect_upgrade_this_block is redundant which could be a cause of confusion or bugs later on. I would prefer to avoid it. Specifically, this statement above is not correct:

Block 1500: can_set_code == None || can_set_code == Some(2000). I don't know which condition actually applies, but neither value indicates that it is legal to set :code on this block, right now.

When can_set_code is Some, you can schedule an upgrade to be applied at/after that block. When an upgrade is scheduled, you should put it into code as soon as relay_chain_height >= scheduled_at, regardless of value of can_set_code. Happy to bikeshed on names for can_set_code as I see where the confusion stems from. The implementation accounts for the case where an upgrade is applied and a new one is signaled at the same time here: https://github.com/paritytech/polkadot/pull/918/files#diff-e21f8096708b22fcdbd66ebc74f0821aR1038

I wrote this code with the understanding that Cumulus would have an inherent type with the ValidationFunctionParams

I'm also looking into how to implement that. However, even if the upgrade pallet gets the validation function params from an inherent instead of by directly querying a special storage key, it's not yet clear to me how the inherent would get access to them except via the magic storage key.

The inherent should provide the value for the ephemeral magic storage key, right? And then the inherent is constructed via a ProvideInherent implementation, whose InherentData contains a ValidationFunctionParams which is provided by the polkadot-collator pipeline to Cumulus' authorship process.

[coriolinus]

When can_set_code is Some, you can schedule an upgrade to be applied at/after that block. When an upgrade is scheduled, you should put it into code as soon as relay_chain_height >= scheduled_at, regardless of value of can_set_code.

Yes, I understand that; that's what the pallet does. However, there are three areas in the code in play:

• Polkadot
• cumulus runtime (validate_block)
• pallet-parachain-upgrade

When an upgrade is scheduled, both polkadot and the pallet store the current value of can_set_code, and apply the code change at the appropriate time. However, the cumulus runtime is stateless; it doesn't get to add anything persistent to storage.

That's all fine, assuming that the only way that users ever attempt parachain upgrades is via our pallet. However, that's not an entirely reasonable assumption. Someone out there, eventually, is likely to want to do it themselves for some reason.

The purpose of expect_upgrade_this_block is to enable validate_block to know when an upgrade is legal, given that it can't store its own data in storage, and shouldn't peek into the pallet's storage (because we can't assume that our pallet will be used). If it exists, then validate_block can fail when a user attempts to set :code at the wrong time. If it doesn't exist, then it cannot perform that validation. If that validation is already performed by polkadot, then we can skip this. However, if Polkadot is not performing that validation check, then it is possible for an end user to write a pallet which just updates :code whenever it feels like.

And then the inherent is constructed via a ProvideInherent implementation, whose InherentData contains a ValidationFunctionParams which is provided by the polkadot-collator pipeline to Cumulus' authorship process.

Thanks! This gives me a lot of entrypoints to research to figure out how to do this.

[rphmeier]

There are a lot of ways someone can write an incorrect parachain or runtime. Even if we added this expect_upgrade_this_block, there is still a possibility that someone writes a pallet that doesn't care about can_upgrade_at and writes to :code anyway. Or writes the wrong code to :code. Or doesn't write anything to:code when it's supposed to. There are a million ways an end user can mess things up, which we cannot prevent. We can't address these issues, so are we supposed to throw away this PR?

This PR sets forth a protocol that chains can follow. Cumulus is one way for them to follow the protocol. If someone decides to register a parachain diverging from the protocol, then their parachain will not work.

As long as Cumulus can expose a reasonable entry-point for users to perform parachain code upgrades, then responsible parachain authors and auditors will (need to) ensure that it is used correctly.

[rphmeier]

I don't feel competent to review this PR as a whole; there's a lot of changes in here about which I just don't yet know enough to predict the resulting changes in behavior. However, from the narrow perspective of enabling me to handle parachain validation function changes within cumulus, this definitely appears to work.

Just ensuring that I understand the intention behind this design, here's the happy path as I understand it:

• a superuser (or, todo, referendum) approves a new validation function
• a support pallet (in progress) looks at validation_params.code_upgrade_allowed and keeps track of the relay chain block number at which an upgrade would be legal
• The first time that validation_params.relay_chain_height >= upgrade_block, it returns the new function in ValidationResult, which notifies polkadot that the new function is ready to adopt. At all other times, ValidationResult.new_validation_code == None.

Is that correct?

The process is more like this:

• The validation function can return new validation code if allowed.
• This code will be applied according to the rules described in the PR here: https://github.com/paritytech/polkadot/pull/918/files#diff-5653780a760f53aeb6fa37d2ba8b8894R205
• The parachain is not notified after-the-fact because it was sufficiently notified before the code upgrade was signalled. Rephrasing: we tell parachains when they can upgrade their code, and if they want to signal an upgrade now, at what time to apply that upgrade.

[rphmeier]

cc @arkpar I've encountered the same spurious test failure of parallel_execution again.

[rphmeier]

If #973 passes CI then we need to merge #972 ASAP and then merge this on top of it.

[montekki]

TODOs should be fixed before merging or have an issue opened?