Staging Runtime API implementation

[tdimitrov]

This is a follow up from #11338, paritytech/polkadot#5048 and some offline discussions.

What we need

I write this from the point of view of a substrate user in polkadot. This problem might not be relevant for other projects.

At the moment we use the primitives provided by substrate to build versioned runtime API. We have got different runtimes (for polkadot, kusama, rococo, etc) but they implement the same runtime API interface. For specific runtimes (rococo to be concrete) we want to introduce breaking changes like:

• Modify the behaviour of a function already present in the stable API in sort of a breaking/incompatible way.
• Add completely new functions which we consider 'staging'. This means they are not production ready and we want to test them in safe but real environment. By 'safe' I mean 'not on a production network. By 'real' - actual network where we can see how runtime upgrade behaves, how version mismatch between the node and the runtime works and so on.

Additionally we need versioning independent from the one of the stable staging API. We expect the development around it to be dynamic so we should be able to create new versions easily.

A quote from @eskimor from the linked discussion:

So in a way we actually want two runtimes to be built .... One stable one and the staging one with an incremented version number. Stable will be the default (native) and we can upgrade test networks to the staging one via sudo and see whether it works.

Another one regarding the versioning:

The thing is if you are checking for the presence of the function, that is not quite the same as checking a version.

In particular: If the function was not yet added, (the case we are discussing) we just want to handle it by simply behaving in the old way. What could also happen, is that the function gets removed at a later point in time - in that case we would at least want an error message, so we will detect early in debugging that a needed function was accidentally removed.

That's actually the reason we bother about versioning at all, otherwise we could always just check whether the function is there or not and versioning becomes irrelevant (to the node).

And a final one from @rphmeier :

So in short: we want the node to be able to declare both the staging runtime API and the 'current' runtime API. The staging runtime API should have its own version so it can be detected at runtime. We should not have 2 types for this, because then you have to write a lot of dispatch logic based on the version and add a lot of trait bounds. We should have the ability to maintain multiple staging versions at once.

Things we want to avoid (if possible)

• We want to avoid adding new type for the staging API (see @rphmeier quote above) because native runtime will be gone and adding dispatch logic + new trait bounds may quickly turn into a mess.
• The staging API to be a first class concept in substrate so that it's straightforward to use. Currently we relay on hacks to achieve the same behaviour.

[bkchr]

Additionally we need versioning independent from the one of the stable staging API. We expect the development around it to be dynamic so we should be able to create new versions easily.

What is the stable staging api? The one that is "actually" stable but not yet enabled by the runtime?

In general I find this a good idea, what I did not yet understood. How many "different" versions do you need? Currently we have one version for a runtime api. I think we should keep this "main version" for when we need to iterate on the stable api, aka stuff that doesn't need interventions by the council. Then we would like to have one "staging version", which would be different to the "main version" as long as it wasn't yet enacted by the council? Meaning both can also be the same when the staging one is enacted. And then you also want to have versions around new functions that you added to the runtime api that are only used in test nets? Do we need there one version per "unstable staging api function" or would it be enough to have one version for the "unstable staging api"? So, that in the end we have in maximum 3 different versions per runtime api? "main version", "stable staging version" and "unstable staging version"?

Most of this stuff could already be done today, relative easily on the runtime side. We could add some kind of check if version < required_version { panic!() }. Then you have a staging api and this code would be really easy to write. If we want to make it nice and have the node side return a nice error, it would be more complicated.

There are multiple things to consider:

1. Currently the runtime api versions etc are part of the RuntimeVersion. This version is currently expected to not change as long as the wasm file doesn't change. When we make the runtime api version setable by the council, this wouldn't be true anymore. We also added the RuntimeVersion as its own wasm section to the wasm binary. The runtime api versions are also their own section in the wasm file. All this is something we need to consider and fix. We could for example remove the runtime api versions completely from the wasm sections and require calling Core_version to get the current runtime api versions. All of this is solvable. Maybe we should even introduce a runtime api call to only get the runtime api versions. This could help with 2.
2. The runtime api is currently identified with a [u8; 8] that is the blake2 hash of the runtime api name. Each ID has only one version assigned. If we would want to change this, we would need again a migration for RuntimeVersion. However, we could also work around this by giving each runtime api version a different ID. The main version is just the normal blake2(RuntimeApiName), "stable staging" would be blake2(RuntimeApiName++StableStaging) and "unstable staging" would be blake2(RuntimeApiName ++ UnstableStaging). Then we could communicate the version per ID to the client. The client could then check what is the current version and return a proper error, like "FunctionUnavailable" or something like this (instead of just panicking).
3. On the runtime side we would still need to check that if the calling of a particular runtime function is allowed and if not, we would need to panic. We could should probably be able to promote functions to "stable" thus we don't need to always check if they are allowed to be executed. This would for example be required for functions that are called on chain, like exeucte_block or apply_extrinsic to prevent the extra lookup of the stable runtime api version in the storage.

For me the main points that need to be cleared are around how many different versions we need per runtime api. I hope that the 3 above I outlined are enough. Then the rest should be relative simple.

[tdimitrov]

What is the stable staging api? The one that is "actually" stable but not yet enabled by the runtime?

Sorry about that. It was a typo. It should read as STABLE API.

How many "different" versions do you need?

Probably my typo lead to this question, but let's clear this. We need two versions of the runtime - a stable and a staging one. Both APIs can 'grow' linearly. I don't expect to have forks. I think this is similar to the current situation. We've got a stable api v2. At some point it will become v3. The staging one will be the same but the versions will go faster. You do a change - bump the version. Another change - another bump and so on.

This version is currently expected to not change as long as the wasm file doesn't change. When we make the runtime api version setable by the council, this wouldn't be true anymore.

I am very unfamiliar with the council part but I think we don't want to change this behaviour. The council might decide that a specific network can have the staging API enabled. Or it might be just hardcoded for Versi and that's all. @rphmeier could you please share your thoughts about this paragraph?
So if my assumption above is correct - the version of the runtime will change when new wasm is uploaded.

[rphmeier]

The most basic requirements of this feature at the Substrate level:

• The decl_runtime_apis should allow us to declare multiple versions of the runtime API trait.
• The impl_runtime_apis should allow us to choose which version of the runtime API trait to use.

In Polkadot, we will then create a special "vstaging" version of the parachain-host runtime API which will eventually become v3. We'll upgrade through normal runtime upgrades. Rococo/Westend will implement v3 before Kusama, which will implement v3 before Polkadot. Before any of that, we'd run 'vstaging' on Versi intensively.

^ This is the first thing that should be implemented and is probably not more than a day or two of work.

We discussed other stretch features which are not as necessary but will make things easier in the long run. These should not be implemented right away.

Experimental API bundles: Grouping new functions together, so they are 'bundled' and only enabled together as an experimental staging API. Runtime API version could be treated as a bitfield where bits indicate which experimental feature bundles are enabled.
Setting runtime API with Governance: Allow a single version of the runtime to support multiple versions of a runtime API at once and upgrade dynamically through governance proposals. It needs to be investigated if the overhead is too high, but this improves governance UX by leaving the choice of when to upgrade node logic up to very specific proposals.

[bkchr]

We could do the following:

decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[staging]
         fn new_cool_function() -> u32;
    }
}

With staging enabled:

impl_runtime_apis! {
    #[staging]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }
         
         fn new_cool_function() -> u32 {
             10
         }
    } 
}

This would return as runtime api version 11. Basically when you use staging the runtime api version is always the stable version + 1.

Without staging enabled:

impl_runtime_apis! {
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }
    } 
}

This would return as runtime api version 10. Calling a staging api for this runtime would return NotAvailable.

WDYT?

[rphmeier]

This suits our needs for the moment just fine.

[rphmeier]

However, shortly after that we will want the ability to choose the runtime API version we implement in a runtime beyond just staging. Otherwise, we will have to implement #[staging] on Kusama which feels wrong, as it is production.

[bkchr]

decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[api_version(11)]
         fn new_cool_function() -> u32;
    }
}

impl_runtime_apis! {
    #[api_version(11)]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }
         
         fn new_cool_function() -> u32 {
             10
         }
    } 
}

So something like this?

[rphmeier]

Yes, that'd definitely work. We don't need the 'staging' nomenclature in Substrate itself because we can make that distinction at a higher level.

[rphmeier]

btw I think @tdimitrov is happy to help as long as he has some guidance on the required code changes. @tdimitrov have you worked on proc-macros before?

[bkchr]

And I'm happy if I don't need to do it :D I can help as needed with instructions :)

[tdimitrov]

Yes I will help with the implementation. I've got basic knowledge about proc-macros but I'll ramp up. The task doesn't seem too complicated (I hope) :)

One question:

Basically when you use staging the runtime api version is always the stable version + 1.

What about the case when another staging function is added to the staging API? We will want to differentiate between the two versions, won't we?

[bkchr]

The latest "iteration" didn't included this staging notion anymore. So, you can just ignore it. We want to go the way with the manual versioning as highlighted in my second last post above.

[rphmeier]

What about the case when another staging function is added to the staging API? We will want to differentiate between the two versions, won't we?

Yep, that'll be for later.

For reference, we will make modifications for something like #11577 (comment) and then make the 'staging' distinction only at the higher level with Polkadot for the moment.

[tdimitrov]

@rphmeier @bkchr may I have some initial feedback on this: #11779 ? It needs more work, but I want to check if I'm in the right direction.

The biggest issue so far is how to implement safety check for an unimplemented method. E.g. in @bkchr 's example above - if fn new_cool_function() is not implemented we are in big trouble. I've got some ideas how this can be done but I need to test them.

EDIT: Please check the first commit only. The rest is still not worth watching.

[tdimitrov]

Let me restate the problem I spoke about in the last comment again:
If we have got this API:

decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[api_version(11)]
         fn new_cool_function() -> u32;
    }
}

it will generate a trait similar to:

trait Test {
    fn something() -> Vec<u8>;

    fn new_cool_function() -> u32 {
        unimplemented!();
    }
}

The important part here is that the staging function has got a default implementation.
Then let's implement the API like this:

impl_runtime_apis! {
    #[api_version(11)]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }
         
         // fn new_cool_function() -> u32 {
         //    10
         // }
    } 
}

We state that we implement version 11 but we don't provide implementation for new_cool_function. Code compiles fine and Runtime crashes when the function is called. This is not acceptable in my opinion. Correct me if I'm missing something.

What we can do?

With decl_ruintime_apis we can generate some 'dummy traits' like:

trait Test {
    fn something() -> Vec<u8>;
    fn new_cool_function() -> u32 {
        unimplemented!();
     }
}

trait TestV11 {
    fn something() -> Vec<u8>;
    fn new_cool_function();
}

Note that one more trait is generated - TestV11. It contains all methods which version 11 requires (no default implementations here).

Then impl_runtime_apis can generate code similar to:

impl Test for Runtime {
    // the method implementations provided with the macro
}

impl TestV11 for Runtime {
    // exactly the same implementations go here too
}

This way if a method mandatory for V11 is missed - the code won't compile. The error will be a bit weird, but at least the problem will be caught compile time.

[tdimitrov]

I did a quick and dirty implementation of the above idea in the PR. Then I've added the modified substrate version in Polkadot.
Here are some cases I've tested and the compiler errors generated:

You'll see a method with staging_ prefix. This is because I'm using the current code. It's not a requirement.

First I tried to add a method with a version older than the stable one. Note that the api is V2, while staging_get_disputes is V1:

sp_api::decl_runtime_apis! {
	/// The API for querying the state of parachains on-chain.
	#[api_version(2)]
	pub trait ParachainHost<H: Encode + Decode = pcp::v2::Hash, N: Encode + Decode = pcp::v2::BlockNumber> {
		/// Get the current validators.
		fn validators() -> Vec<v2::ValidatorId>;

		/// SKIPPED
		
		/***** STAGING *****/

		/// Returns all onchain disputes.
		/// This is a staging method! Do not use on production runtimes!
		#[api_version(1)]
		fn staging_get_disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState<v2::BlockNumber>)>;
	}
}

Compilation error:

error: Method version `1` is older than (or equal to) trait version `2`. Methods can't define versions older than the trait version.
     --> /home/ceco/src/polkadot/primitives/src/runtime_api.rs:155:3
      |
  155 | /         /// Returns all onchain disputes.
  156 | |         /// This is a staging method! Do not use on production runtimes!
  157 | |         #[api_version(1)]
  158 | |         fn staging_get_disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState<v2::BlockNumber>)>;
      | |___________________________________________________________________________________________________________________^

Then I declared I am implementing version 3 and didn't add a required method:

#[api_version(3)]
impl primitives::runtime_api::ParachainHost<Block, Hash, BlockNumber> for Runtime {
	// SKIPPED
	
	// fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState<BlockNumber>)> {
	// 	runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::<Runtime>()
	// }
}

The compilation error is:

 error[E0046]: not all trait items implemented, missing: `staging_get_disputes`
      --> /home/ceco/src/polkadot/runtime/westend/src/lib.rs:1288:2
       |
  1288 |     impl primitives::runtime_api::ParachainHost<Block, Hash, BlockNumber> for Runtime {
       |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `staging_get_disputes` in implementation
       |
       = help: implement the missing item: `fn staging_get_disputes() -> sp_std::prelude::Vec<(u32, CandidateHash, DisputeState)> { todo!() }`

  For more information about this error, try `rustc --explain E0046`.

They look almost like real to me :) What do you think?

This is the branch I used for testing: https://github.com/tdimitrov/polkadot/tree/substrate-local-test

[rphmeier]

The biggest issue so far is how to implement safety check for an unimplemented method. E.g. in @bkchr 's example #11577 (comment) - if fn new_cool_function() is not implemented we are in big trouble. I've got some ideas how this can be done but I need to test them.

Could you elaborate on why this is big trouble?

Code compiles fine and Runtime crashes when the function is called. This is not acceptable in my opinion. Correct me if I'm missing something.

I think this isn't substantially different from a user implementing the runtime API as an unimplemented!() or panic!() statement, which is more or less OK. The repercussions of the runtime crashing during block execution are high (i.e. the execute_block API), but when invoked outside of that, the calling code should be prepared to handle errors so a panic is OK.

The only issue is if not implementing the new runtime APIs does not issue a warning or error to the user, which is a developer experience issue. Less than ideal, and if so, it should be documented as a major caveat. But acceptable if there's no simple way to introduce that warning or error.

But the example you showed of implementing api_version(3) and not including staging_get_disputes failing looks like it covers that already, which is great.

In short, nice work!

[tdimitrov]

Could you elaborate on why this is big trouble?

I see it as a big trouble because the developer can implement a runtime version without providing all required method implementations. And when any such method is called - runtime will crash. In my eyes this was an apocalyptic outcome but you explained this is not the case :)

I think a found a nice 'hack' which yields compilation error if a required method implementation is missing. The idea came after I wrote the comment above.

I spoke with @bkchr about my idea. He has got some reservations about it so let's wait for his feedback too. Until then I'll refactor my changes a bit. I wanted to see how it will work out and I cut a lot of corners with the implementation.

[tdimitrov]

I've created a polkadot branch based on this PR. It does two things:

• switches the substrate version with one supporting the new syntax - this commit
• tags the new runtime method with version 3 - this commit

I've built a single binary from this branch and ran it with zombienet with polkadot and versi chainspec. The versioning works good. In versi the new method was executed. In polkadot the runtime version check here and here kicked in:

2022-07-11 11:59:18.001 DEBUG tokio-runtime-worker parachain::provisioner: Can't fetch onchain disputes, because runtime version is not recent enough. Will continue with empty onchain disputes set. runtime_api_err=NotSupported { runtime_api_name: "staging_get_disputes" } relay_parent=0x85172496c5dd9d3e742f05f84d6f278f27c8295d0072a7a9b97733193867a845

[tdimitrov]

Fixed with #11779