Skip to content
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.

Sign up for GitHub

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

Jump to bottom

serde for RuntimeString is broken #4001

Closed
2 tasks done
nazar-pc opened this issue Apr 5, 2024 · 13 comments · Fixed by #5693
Closed
2 tasks done

serde for RuntimeString is broken #4001

nazar-pc opened this issue Apr 5, 2024 · 13 comments · Fixed by #5693
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@nazar-pc
Copy link
Contributor

nazar-pc commented Apr 5, 2024 •
edited
Loading

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

I just had a strange issue when I tried to serialize RuntimeString on the client into JSON and then deserialize in the runtime. Turned out serde implementation for RuntimeString is broken because between client and runtime owned value will be String or Vec<u8> and as the result one end will serialize a String and another will try to deserialize it as a vector of bytes, resulting in confusing errors.

I have no idea what problem RuntimeString is even solving, looks like what you want is just Cow<'static, str> unless I miss something.

I hit this due to sp_genesis_builder::GenesisBuilder trying to decode JSON.

Steps to reproduce

No response
@nazar-pc nazar-pc added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Apr 5, 2024
@ggwpez
Copy link
Member

ggwpez commented Apr 5, 2024 •
edited
Loading

Maybe it would work to give the variants explicit indices?
Currently it looks like the String and Vector variant share the index depending on the std feature flag. But it seems hacky...

(yes could be that the whole type is a bit useless though cc @michalkucharczyk)

@bkchr
Copy link
Member

bkchr commented Apr 5, 2024

I have no idea what problem RuntimeString is even solving, looks like what you want is just Cow<'static, str> unless I miss something.

This comes from a time when we did not had Encode and Decode enabled in SCALE for no_std 🙈 Now, this is probably a good idea to change this.

@michalkucharczyk
Copy link
Contributor

Does it mean you have RuntimeString field somewhere in your RuntimeGenesisConfig?

We could use serde variant attributes to deserialize Owned as String in nostd.

@bkchr
Copy link
Member

bkchr commented Apr 6, 2024

I think the better solution is just to change RuntimeString to Cow as @nazar-pc said. Otherwise I would propose that we just write some custom Serialize/Deserialize.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Apr 6, 2024

Does it mean you have RuntimeString field somewhere in your RuntimeGenesisConfig?

Yes

I think the better solution is just to change RuntimeString to Cow as @nazar-pc said. Otherwise I would propose that we just write some custom Serialize/Deserialize.

The only reason for RuntimeString to exist that I see is that it allows to bypass UTF-8 verification in runtime in some cases. Not sure how critical it is though.

@bkchr
Copy link
Member

bkchr commented Apr 8, 2024

This comes from a time when we did not had Encode and Decode enabled in SCALE for no_std 🙈

I mean this is the reason why this exists, because I know the guy that added the RuntimeString :P

@nazar-pc nazar-pc mentioned this issue Apr 9, 2024
Merged
1 task
@nazar-pc
Copy link
Contributor Author

Recommended course of action to fix this?

@bkchr
Copy link
Member

bkchr commented Aug 27, 2024

If moving it to Cow<'static, str> works, I would propose we do it this way.

@nazar-pc
Copy link
Contributor Author

My only concern is that it'll be a breaking change for JSON serialization, hence the question. If someone stored serialized RuntimeString in actual runtime, they'll be surprised to see it not working anymore (I guess we can work around that by implementing custom deserializer that will support decoding bytes as well, but that feels ugly).

@bkchr
Copy link
Member

bkchr commented Sep 3, 2024

Can we even differentiate between bytes or string encoded?

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Sep 3, 2024

I think the answer very much depends on the serialization format. We can definitely tell that in JSON and probably some other self-describing formats, but I don't think it is universally true.

@bkchr
Copy link
Member

bkchr commented Sep 4, 2024

If someone stored serialized RuntimeString in actual runtime, they'll be surprised to see it not working anymore (I guess we can work around that by

Just thought about this again. Actually if they stored it as SCALE encoded, the encoding is the same and decoding should not fail between Vec<u8> and String.

@nazar-pc nazar-pc mentioned this issue Sep 12, 2024
Merged
3 tasks
@nazar-pc
Copy link
Contributor Author

I went a bit further and removed the data structure as redundant in #5693

github-merge-queue bot pushed a commit that referenced this issue Nov 5, 2024
@nazar-pc @actions-user
@gui1117 @bkchr
Remove sp_runtime::RuntimeString and replace with `Cow<'static, str…
c5444f3 
…>` or `String` depending on use case (#5693)

# Description

As described in #4001
`RuntimeVersion` was not encoded consistently using serde. Turned out it
was a remnant of old times and no longer actually needed. As such I
removed it completely in this PR and replaced with `Cow<'static, str>`
for spec/impl names and `String` for error cases.

Fixes #4001.

## Integration

For downstream projects the upgrade will primarily consist of following
two changes:
```diff
#[sp_version::runtime_version]
pub const VERSION: RuntimeVersion = RuntimeVersion {
-	spec_name: create_runtime_str!("statemine"),
-	impl_name: create_runtime_str!("statemine"),
+	spec_name: alloc::borrow::Cow::Borrowed("statemine"),
+	impl_name: alloc::borrow::Cow::Borrowed("statemine"),
```
```diff
		fn dispatch_benchmark(
			config: frame_benchmarking::BenchmarkConfig
-		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
+		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {
```

SCALE encoding/decoding remains the same as before, but serde encoding
in runtime has changed from bytes to string (it was like this in `std`
environment already), which most projects shouldn't have issues with. I
consider the impact of serde encoding here low due to the type only
being used in runtime version struct and mostly limited to runtime
internals, where serde encoding/decoding of this data structure is quite
unlikely (though we did hit exactly this edge-case ourselves
:sweat_smile:).

## Review Notes

Most of the changes are trivial and mechanical, the only non-trivial
change is in
`substrate/primitives/version/proc-macro/src/decl_runtime_version.rs`
where macro call expectation in `sp_version::runtime_version`
implementation was replaced with function call expectation.

# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [ ] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [ ] I have made corresponding changes to the documentation (if
applicable)

---------

Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Milestone
No milestone
4 participants
@nazar-pc @michalkucharczyk @bkchr @ggwpez