-
Notifications
You must be signed in to change notification settings - Fork 721
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
Update RuntimeVerison
type and use system_version
to derive extrinsics root StateVersion
instead of V0
#4257
Conversation
@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 ? |
f97bcad
to
d06738b
Compare
d06738b
to
d497f25
Compare
@vedhavyas sorry for the delay. Can you please fix the CI issues and merge master? |
@bkchr yes will do. Thank you! |
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
} else if field_name == "state_version" { | ||
parse_once(&mut self.state_version, field_value, Self::parse_num_literal_u8)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state_version
+extrinsics_root_state_version
intosystem_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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I would not separate the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedhavyas and could you do this?
Co-authored-by: Koute <[email protected]>
@bkchr seems like CI is stuck. Is there an way to restart it ? |
Done. |
@koute any idea why some tests failed. Looking at the CI logs of each failed test does not give much info. Any pointers pls ? |
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
Had to introduce custom deserialization as well in order to allow "duplicated" field and make serialization/deserialization symmetrical |
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 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both exists, they should be the same value
transaction_version = Some(map.next_value()?); | ||
}, | ||
Field::SystemVersion => { | ||
system_version = Some(map.next_value()?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you are missing here the check if this is already some.
formatter.write_str("field identifier") | ||
} | ||
|
||
fn visit_u64<E>(self, value: u64) -> Result<Self::Value, E> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
} | ||
|
||
fn visit_bytes<E>(self, value: &[u8]) -> Result<Self::Value, E> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this is from Deserialize
derive
…rialization have the same value
The CI pipeline was cancelled due to failure one of the required jobs. |
if system_version != new_value { | ||
return Err(<A::Error as serde::de::Error>::custom( | ||
alloc::format!( | ||
r#"Duplicated "stateVersion" and "systemVersion" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't really know this. It could also be that there are more than one stateVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move all this code into a separate file? 400 lines is a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would serde
module work? I generally try to keep impls next to the definition, but agree that in this case it is quite lengthy.
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks appealing, but Option
and missing field are not 100% the same for serde
. How big of an issue is this really?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just let's take the current code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks appealing, but
Option
and missing field are not 100% the same forserde
. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm also not 100% happy with the 400 lines of useless code, but there is no real better solution...)
If there are no blockers here, I'd really appreciate finally merging this and be done with it 🙏 |
@koute needs to approve :P |
@nazar-pc we need a master merge again 🙈 |
# Conflicts: # Cargo.lock
Head branch was pushed to by a user without write access
9930d21
There is a leftover in chain sync :( polkadot-sdk/substrate/client/network/sync/src/strategy/chain_sync.rs Lines 2290 to 2293 in ec9a734
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.
…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.
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.
This PR
RuntimeVersion::state_version
tosystem_version
Runtime::system_version
to derive extrinsics rootStateVersion
instead of defaultStateVersion::V0
This PR should not be breaking any existing chains so long as they use same
RuntimeVersion::state_version
forRuntime::system_version
Using
RuntimeVersion::system_version = 2
will make the extrinsics root to useStateVersion::V1
instead ofV0
RFC for this change - polkadot-fellows/RFCs#42