polkadot-sdk icon indicating copy to clipboard operation
polkadot-sdk copied to clipboard

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

Open vedhavyas opened this issue 1 year ago • 19 comments

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 - https://github.com/polkadot-fellows/RFCs/pull/42

vedhavyas avatar Apr 23 '24 11:04 vedhavyas

@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 avatar Apr 25 '24 09:04 vedhavyas

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

vedhavyas avatar May 13 '24 12:05 vedhavyas

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 avatar Jun 19 '24 16:06 vedhavyas

@bkchr @koute bumping this again

vedhavyas avatar Jun 24 '24 13:06 vedhavyas

@bkchr @koute bumping this again

vedhavyas avatar Jun 27 '24 07:06 vedhavyas

@bkchr @koute bumping this again :)

vedhavyas avatar Jul 02 '24 10:07 vedhavyas

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

bkchr avatar Jul 24 '24 14:07 bkchr

@bkchr yes will do. Thank you!

vedhavyas avatar Jul 24 '24 14:07 vedhavyas

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

vedhavyas avatar Jul 26 '24 07:07 vedhavyas

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

Done.

koute avatar Jul 26 '24 08:07 koute

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

vedhavyas avatar Jul 26 '24 09:07 vedhavyas

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

bkchr avatar Jul 28 '24 21:07 bkchr

@vedhavyas please give us merge rights to your branch, then we can also merge master for example

I don't think that'll work because PR was created from an organization fork, not from a personal one.

I just merged master in though.

nazar-pc avatar Jul 28 '24 21:07 nazar-pc

Merged master again, hoping to push this across the finish line soon :crossed_fingers:

nazar-pc avatar Aug 06 '24 00:08 nazar-pc

@koute @bkchr how can I get you interested in reviewing this before cutting the next stable release?

nazar-pc avatar Aug 12 '24 09:08 nazar-pc

@koute how can I get you interested in reviewing this before cutting the next stable release?

Make it backwards compatible (have it emit a deprecation warning when the old field name is used) and you'll have a :+1: from me.

koute avatar Aug 12 '24 09:08 koute

What @koute is referring to is, this discussion: https://github.com/paritytech/polkadot-sdk/pull/4257#discussion_r1690053835

bkchr avatar Aug 12 '24 19:08 bkchr

@bkchr @koute Should be updated now. Can you take a look if I missed anything ?

vedhavyas avatar Aug 12 '24 19:08 vedhavyas

@bkchr @koute are we good to go here?

nazar-pc avatar Aug 22 '24 20:08 nazar-pc

Not sure what to do with zombienet checks (not clear what is wrong from CI logs alone), but the rest of the CI should be fixed assuming I didn't miss anything in prdoc

nazar-pc avatar Aug 23 '24 12:08 nazar-pc

The zombienet errors seem to be valid:

tokio-runtime-worker cumulus-pov-recovery: [Parachain] Failed to fetch relay chain runtime version. error=RpcCallError("state_getRuntimeVersion")

The bridges zombienet errors are maybe also related. Probably PJS tries to fetch the RuntimeVersion and it not having the state_version field is maybe the problem.

bkchr avatar Aug 27 '24 21:08 bkchr

I have added stateVersion alias to systemVersion field, which should fix the decoding issue. If desirable customized serialization can be implemented to produce both stateVersion and systemVersion in the serialized JSON for smoother migration process.

nazar-pc avatar Aug 27 '24 22:08 nazar-pc

If desirable customized serialization can be implemented to produce both stateVersion and systemVersion in the serialized JSON for smoother migration process.

Yeah I also thought about this.

bkchr avatar Aug 27 '24 22:08 bkchr

So do you want me to do that?

nazar-pc avatar Aug 27 '24 22:08 nazar-pc

So do you want me to do that?

Yeah please do it. Can you also try to merge master again? I hope that this fixes the bridges tests.

bkchr avatar Aug 28 '24 19:08 bkchr

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

nazar-pc avatar Aug 29 '24 07:08 nazar-pc

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

paritytech-cicd-pr avatar Aug 30 '24 00:08 paritytech-cicd-pr

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

nazar-pc avatar Sep 05 '24 10:09 nazar-pc

@koute needs to approve :P

bkchr avatar Sep 06 '24 07:09 bkchr

@nazar-pc we need a master merge again :see_no_evil:

bkchr avatar Sep 08 '24 20:09 bkchr