Update `RuntimeVerison` type and use `system_version` to derive extrinsics root `StateVersion` instead of `V0` #4257

vedhavyas · 2024-04-23T11:09:08Z

This PR

Renames RuntimeVersion::state_version to system_version
Uses Runtime::system_version to derive extrinsics root StateVersion instead of default StateVersion::V0

This PR should not be breaking any existing chains so long as they use same RuntimeVersion::state_version for Runtime::system_version

Using RuntimeVersion::system_version = 2 will make the extrinsics root to use StateVersion::V1 instead of V0

RFC for this change - polkadot-fellows/RFCs#42

…meVersion instead

vedhavyas · 2024-04-25T09:49:40Z

@bkchr I had looked through the failed tests and not sure if the failures are related to changes in this PR. Can you confirm? If not where should I look at to fix these tests ?

vedhavyas · 2024-05-13T12:46:51Z

Hey @koute @bkchr would appreciate some thoughts on the failing issues.
Also, any chance you can do a first pass/review this PR ?

vedhavyas · 2024-06-19T16:56:58Z

Hey @bkchr @koute
Would be really great to this PR in. I will fix the conflicts in sometime but please have a look so that this is landed

vedhavyas · 2024-06-24T13:32:33Z

@bkchr @koute bumping this again

vedhavyas · 2024-06-27T07:05:52Z

@bkchr @koute bumping this again

vedhavyas · 2024-07-02T10:14:54Z

@bkchr @koute bumping this again :)

cumulus/parachains/runtimes/starters/seedling/src/lib.rs

cumulus/parachains/runtimes/starters/shell/src/lib.rs

bkchr · 2024-07-24T14:25:10Z

@vedhavyas sorry for the delay. Can you please fix the CI issues and merge master?

vedhavyas · 2024-07-24T14:29:03Z

@bkchr yes will do. Thank you!

Co-authored-by: Bastian Köcher <[email protected]>

prdoc/pr_4257.prdoc

koute · 2024-07-24T15:47:23Z

substrate/primitives/version/proc-macro/src/decl_runtime_version.rs

-		} else if field_name == "state_version" {
-			parse_once(&mut self.state_version, field_value, Self::parse_num_literal_u8)?;


Would be nice to still accept the old state_version for backwards compatibility and emit a deprecation message prompting the users to change to system_version.

Also, since this is a procedural macro it could be nice to allow people to specify the versions in a "nice" way instead of just having a hardcoded magic number, for example:

// instead of "system_version: 0": state_version: StateVersion::V0, extrinsics_root_state_version: StateVersion::V0, // instead of "system_version: 1": state_version: StateVersion::V1, extrinsics_root_state_version: StateVersion::V0, // instead of "system_version: 2": state_version: StateVersion::V1, extrinsics_root_state_version: StateVersion::V1,

This would be nicer from a users' point of view, but I suppose it makes the macro even more "magic", so *shrug*. I'll leave it up to whatever @bkchr prefers.

This is similar to my initial proposal but decided to not go that route since this adds another extra field to Runtimeversion. More here - polkadot-fellows/RFCs#42 (comment)

I'm not talking about adding/changing the physical fields - the RFC's already accepted so that train already left the station. I'm just talking about providing potential syntax sugar in the macro which would turn the state_version + extrinsics_root_state_version into system_version.

A less magic alternative would be something like a const fn system_version(state_version: StateVersion, extrinsics_root_state_version: StateVersion) -> u8 or SystemVersion { state_version: ..., extrinsics_root_state_version: ... }.into() helper or something like that.

(But, again, I don't have a particularly strong opinion here which way to go here. But at very least I'd like to not force additional churn on people by breaking backwards compatibility.)

state_version + extrinsics_root_state_version into system_version.

Not sure we should do this.

The general idea of still supporting state_version sounds like a good proposal. So, it prints some warning and internally converts it to system_version.

However, I would not separate the fields.

@vedhavyas and could you do this?

Co-authored-by: Koute <[email protected]>

vedhavyas · 2024-07-26T07:50:53Z

@bkchr seems like CI is stuck. Is there an way to restart it ?

koute · 2024-07-26T08:48:59Z

seems like CI is stuck. Is there an way to restart it ?

Done.

vedhavyas · 2024-07-26T09:39:59Z

@koute any idea why some tests failed. Looking at the CI logs of each failed test does not give much info. Any pointers pls ?

bkchr · 2024-07-28T21:09:29Z

I think the zombienet tests are just flaky. @vedhavyas please give us merge rights to your branch, then we can also merge master for example

…t the same time in serde serialized input

nazar-pc · 2024-08-29T07:03:01Z

Had to introduce custom deserialization as well in order to allow "duplicated" field and make serialization/deserialization symmetrical

bkchr · 2024-08-29T07:24:52Z

substrate/primitives/version/src/lib.rs

+						Field::TransactionVersion => {
+							if transaction_version.is_some() {
+								return Err(<A::Error as serde::de::Error>::duplicate_field(
+									"transactionVersion",
+								));
+							}
+							transaction_version = Some(map.next_value()?);
+						},
+						Field::SystemVersion => {


If both exists, they should be the same value

bkchr · 2024-08-29T07:25:26Z

substrate/primitives/version/src/lib.rs

+							transaction_version = Some(map.next_value()?);
+						},
+						Field::SystemVersion => {
+							system_version = Some(map.next_value()?);


Also you are missing here the check if this is already some.

bkchr · 2024-08-29T07:30:34Z

substrate/primitives/version/src/lib.rs

+				formatter.write_str("field identifier")
+			}
+
+			fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E>


Do we need this?

I didn't write it, I used macro expansion as the basis and this is what it spit out, I just made it a bit more readable

bkchr · 2024-08-29T07:30:52Z

substrate/primitives/version/src/lib.rs

+				}
+			}
+
+			fn visit_bytes<E>(self, value: &[u8]) -> Result<Self::Value, E>


Same here, this is from Deserialize derive

…rialization have the same value

paritytech-cicd-pr · 2024-08-30T00:21:55Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7190974

bkchr · 2024-09-02T19:09:28Z

substrate/primitives/version/src/lib.rs

+								if system_version != new_value {
+									return Err(<A::Error as serde::de::Error>::custom(
+										alloc::format!(
+											r#"Duplicated "stateVersion" and "systemVersion" \


You don't really know this. It could also be that there are more than one stateVersion.

You're right, but it is one of those two fields. And I decided to make this assumption in an error message since it is the most likely. Open for suggestions.

bkchr · 2024-09-02T19:09:55Z

substrate/primitives/version/src/lib.rs

+// Manual implementation in order to sprinkle `stateVersion` at the end for migration purposes
+// after the field was renamed from `state_version` to `system_version`
+#[cfg(feature = "serde")]
+impl serde::Serialize for RuntimeVersion {


Can we move all this code into a separate file? 400 lines is a lot.

Would serde module work? I generally try to keep impls next to the definition, but agree that in this case it is quite lengthy.

koute · 2024-09-03T09:06:41Z

substrate/primitives/version/src/lib.rs

+// Manual implementation in order to sprinkle `stateVersion` at the end for migration purposes
+// after the field was renamed from `state_version` to `system_version`
+#[cfg(feature = "serde")]
+impl serde::Serialize for RuntimeVersion {


Hmm.... I don't think we need this serde boilerplate? Something like this should work:

use serde::{Serialize, Deserialize}; #[derive(Clone)] pub struct RuntimeVersion { // ... pub system_version: u8 } #[derive(Clone, Serialize, Deserialize)] struct RuntimeVersionSerde { // ... system_version: Option<u8>, state_version: Option<u8>, } impl From<RuntimeVersion> for RuntimeVersionSerde { fn from(value: RuntimeVersion) -> Self { Self { // ... system_version: Some(value.system_version), state_version: Some(value.system_version), } } } impl From<RuntimeVersionSerde> for RuntimeVersion { fn from(value: RuntimeVersionSerde) -> Self { Self { // .. system_version: value.system_version.or(value.state_version) .unwrap() // TODO: Properly handle if both/none are there. } } } impl serde::Serialize for RuntimeVersion { fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer, { RuntimeVersionSerde::from(self.clone()).serialize(serializer) } } impl<'de> serde::Deserialize<'de> for RuntimeVersion { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { Ok(RuntimeVersionSerde::deserialize(deserializer)?.into()) } }

(The From impls are for illustrative purposes; it's probably better to just inline the impls into the Serialize/Deserialize.)

Basically - use an auxiliary struct to do the serialization/deserialization, and then just convert between the real RuntimeVersion and the auxiliary one. You still need some boilerplate to copy the fields, but it should be much less than the serde junk and be much easier to maintain.

This looks appealing, but Option and missing field are not 100% the same for serde. How big of an issue is this really?

Yeah, just let's take the current code.

This looks appealing, but Option and missing field are not 100% the same for serde. How big of an issue is this really?

@nazar-pc What do you mean are not the same? What exactly is the problem? You can implement strictly the same behavior as you've currently implemented, just with a lot less code (and more elegant) if you use a wrapper like I've suggested.

I'm fine with taking the existing code if it's truly necessary, but if not I'd rather have a simplified version that won't be pain to maintain and won't require you to read a ton of serde boilerplate to understand what it does.

Serde is not used just for JSON, so while Some(u8) and u8 encode identically in JSON, I do not know if it is actually the case for all formats that serde supports, so the change is not strictly equivalent in serialization case.

Same thing with deserialization: you can deserialize a number into both Option<u8> and u8 from JSON, but I do not think it is guaranteed to be symmetrical that way with other formats. If we do not care then fine, but I was trying to make it actually equivalent and didn't find a clean way to do so without manual implementation.

Well, as far as I know the only format we ever explicitly supported was JSON (we just used serde because that's the "standard" way to support JSON), and if we care about formats other than JSON then ideally we should have tests for serialization/deserialization of those formats, otherwise considering how flexible serde is we essentially always risk breaking some unknown code somewhere. And we're already partially breaking compatibility anyway by having it serialize an extra field!

So, to reiterate my policy here: if there's a good reason to keep things complex, sure, go for it, but if it's something that's "just in case" and we don't have any idea at all whether it's necessary, then I just prefer to keep things as simple as possible.

Anyway, honestly, taking a step back: the simplest thing would have been to leave the name state_version as state_version and just introduce the possibility of it being 2, and then all of this could have been avoided! We wouldn't have to worry whether this breaks anything, we wouldn't need custom serialization/deserialization, we wouldn't need deprecation warnings, consumers of RuntimeVersion that don't expect the extra field wouldn't break (if there are any then they will break now!), we wouldn't have to remove the state_version from the serialized RuntimeVersion in the future potentially breaking consumers yet again (or, alternatively, we wouldn't leave those two fields in there forever - do you have a plan for when to remove the state_version?), and this PR would have been already merged long ago. The only advantage of renaming it is, as far as I can see, a slightly nicer name, and a big list of pain and disadvantages to go with it.

The RFC was already accepted by the fellowship, so I won't block this, but my opinion is that I would highly prefer to just leave the old name as it was, delete all of the new extra code and call it a day.

Anyway, okay, sorry, rant over. If Big Boss @bkchr's fine with this then I'm also fine; I don't like it, but I don't see any hard blockers. :P

Although, now that I think about it again, isn't emitting both state_version and system_version non-compliant with the RFC? The RFC stated that only system_version should be there. For polkadot-sdk internally we're fine to do whatever we want, but on external community/Polkadot interfaces (like the serialized RuntimeVersion struct) what is in the Polkadot spec and in fellowship RFCs takes precedence over what we want, so shouldn't we only emit system_version or update the RFC?

For SCALE the encoding is as documented in the RFC. For others consuming the type via JSON (which is self descriptive and it is observable that the field name changed), they can continue to use their old code without it breaking.

(I'm also not 100% happy with the 400 lines of useless code, but there is no real better solution...)

nazar-pc · 2024-09-05T10:13:07Z

If there are no blockers here, I'd really appreciate finally merging this and be done with it 🙏

bkchr · 2024-09-06T07:01:33Z

@koute needs to approve :P

bkchr · 2024-09-08T20:54:05Z

@nazar-pc we need a master merge again 🙈

# Conflicts: # Cargo.lock

liuchengxu · 2024-09-12T02:56:17Z

There is a leftover in chain sync :(

polkadot-sdk/substrate/client/network/sync/src/strategy/chain_sync.rs

Lines 2290 to 2293 in ec9a734

    
           let got = HashingFor::<Block>::ordered_trie_root( 
        
           	body.iter().map(Encode::encode).collect(), 
        
           	sp_runtime::StateVersion::V0, 
        
           );

cc @nazar-pc

With the introduction of `system_version` in #4257 the extrinsic root may also use the `V1` layout. At this point in the sync code it would require some special handling to find out the `system_version`. So, this pull request is removing it. The extrinsics root is checked when executing the block later, so that at least no invalid block gets imported.

bkchr · 2024-09-12T08:54:17Z

#5686

…nsics root `StateVersion` instead of `V0` (paritytech#4257) This PR - Renames `RuntimeVersion::state_version` to `system_version` - Uses `Runtime::system_version` to derive extrinsics root `StateVersion` instead of default `StateVersion::V0` This PR should not be breaking any existing chains so long as they use same `RuntimeVersion::state_version` for `Runtime::system_version` Using `RuntimeVersion::system_version = 2` will make the extrinsics root to use `StateVersion::V1` instead of `V0` RFC for this change - polkadot-fellows/RFCs#42 --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Koute <[email protected]> Co-authored-by: Nazar Mokrynskyi <[email protected]> (cherry picked from commit 9930d21)

With the introduction of `system_version` in paritytech#4257 the extrinsic root may also use the `V1` layout. At this point in the sync code it would require some special handling to find out the `system_version`. So, this pull request is removing it. The extrinsics root is checked when executing the block later, so that at least no invalid block gets imported.

vedhavyas added 2 commits April 23, 2024 16:23

rename state_version to system_version in RuntimeVersion type

f72efc2

update extrinsic root to use the state_version dervied from the Runti…

17a857f

…meVersion instead

vedhavyas requested review from a team and koute as code owners April 23, 2024 11:09

vedhavyas mentioned this pull request Apr 23, 2024

add extrinsic_state_version to RuntimeVersion and bump CoreApi version to 5 #1968

Closed

4 tasks

update docs-tests and tests

f7293df

Merge branch 'master' into system_version

246d3f3

vedhavyas force-pushed the system_version branch from f97bcad to d06738b Compare June 24, 2024 13:31

add prdoc and some fixes

d497f25

vedhavyas force-pushed the system_version branch from d06738b to d497f25 Compare June 24, 2024 14:43

bkchr approved these changes Jul 24, 2024

View reviewed changes

cumulus/parachains/runtimes/starters/seedling/src/lib.rs Outdated Show resolved Hide resolved

cumulus/parachains/runtimes/starters/shell/src/lib.rs Outdated Show resolved Hide resolved

bkchr added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jul 24, 2024

vedhavyas and others added 2 commits July 24, 2024 20:32

Update cumulus/parachains/runtimes/starters/seedling/src/lib.rs

1ae816c

Co-authored-by: Bastian Köcher <[email protected]>

Update cumulus/parachains/runtimes/starters/shell/src/lib.rs

227d68d

Co-authored-by: Bastian Köcher <[email protected]>

koute reviewed Jul 24, 2024

View reviewed changes

vedhavyas and others added 2 commits July 24, 2024 21:31

Update prdoc/pr_4257.prdoc

a97a69c

Co-authored-by: Koute <[email protected]>

Merge branch 'master' into system_version

3f4c79a

Merge branch 'master' into system_version

2db4305

Allow both old stateVersion and new systemVersion to be present a…

4248931

…t the same time in serde serialized input

bkchr reviewed Aug 29, 2024

View reviewed changes

nazar-pc added 2 commits August 30, 2024 03:11

Check that both stateVersion and systemVersion when present in se…

abc1e5f

…rialization have the same value

Merge branch 'master' into system_version

7ad19de

Fix no-std compilation

2340a94

bkchr approved these changes Sep 2, 2024

View reviewed changes

koute reviewed Sep 3, 2024

View reviewed changes

koute approved these changes Sep 7, 2024

View reviewed changes

bkchr enabled auto-merge September 8, 2024 20:53

Merge remote-tracking branch 'upstream/master' into system_version

5b1e88e

# Conflicts: # Cargo.lock

auto-merge was automatically disabled September 9, 2024 11:15
Head branch was pushed to by a user without write access

bkchr added this pull request to the merge queue Sep 10, 2024

Merged via the queue into paritytech:master with commit 9930d21 Sep 10, 2024
200 of 203 checks passed

nazar-pc deleted the system_version branch September 10, 2024 07:58

bkchr mentioned this pull request Sep 12, 2024

sync: Remove checking of the extrinsics root #5686

Merged

bkchr mentioned this pull request Sep 16, 2024

Introduce System version to RuntimeVersion object polkadot-fellows/RFCs#42

Merged

This was referenced Sep 27, 2024

[Backport] Version bumps and prdocs reordering from stable2409 #5849

Merged

Upgrade Substrate to stable2409 autonomys/subspace#3078

Merged

		} else if field_name == "state_version" {
		parse_once(&mut self.state_version, field_value, Self::parse_num_literal_u8)?;

Update RuntimeVerison type and use system_version to derive extrinsics root StateVersion instead of V0 #4257

Update RuntimeVerison type and use system_version to derive extrinsics root StateVersion instead of V0 #4257

Conversation

vedhavyas commented Apr 23, 2024

vedhavyas commented Apr 25, 2024

vedhavyas commented May 13, 2024

vedhavyas commented Jun 19, 2024

vedhavyas commented Jun 24, 2024

vedhavyas commented Jun 27, 2024

vedhavyas commented Jul 2, 2024

bkchr commented Jul 24, 2024

vedhavyas commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas commented Jul 26, 2024

koute commented Jul 26, 2024

vedhavyas commented Jul 26, 2024

bkchr commented Jul 28, 2024

nazar-pc commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koute Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc commented Sep 5, 2024

bkchr commented Sep 6, 2024

bkchr commented Sep 8, 2024

liuchengxu commented Sep 12, 2024

bkchr commented Sep 12, 2024

Update `RuntimeVerison` type and use `system_version` to derive extrinsics root `StateVersion` instead of `V0` #4257

Update `RuntimeVerison` type and use `system_version` to derive extrinsics root `StateVersion` instead of `V0` #4257

koute Sep 3, 2024 •

edited

Loading