removes fallible ErasureMeta.fec_set_index type-casts #1841
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
behzadnouri
force-pushed
the
serde-compat-fec-set-index
branch
from
5a66107
to
067312a
Compare
behzadnouri
force-pushed
the
serde-compat-fec-set-index
branch
3 times, most recently
from
4b21f3e
to
5f35b48
Compare
fec_set_index is u32 by the definition in the shred struct. Blockstore however uses u64 for ErasureMeta.fec_set_index for backward compatibility reasons which results in weird typing and fallible conversions from u64 to u32. Any ErasureMeta with a fec_set_index which does not fit in u32 is invalid and should be discarded early. The commit changes ErasureMeta.fec_set_index type to u32 while using serde attributes to maintain backward compatibility for (de)serialization.
behzadnouri
force-pushed
the
serde-compat-fec-set-index
branch
from
5f35b48
to
463dd5c
Compare
behzadnouri
commented
Jul 18, 2024
Comment on lines
+143
to
+168
| // Serializes a value of type T by first type-casting to type R. | ||
| pub(super) fn serialize<S: Serializer, R, T: Copy>( | ||
| &val: &T, | ||
| serializer: S, | ||
| ) -> Result<S::Ok, S::Error> | ||
| where | ||
| R: TryFrom<T> + Serialize, | ||
| <R as TryFrom<T>>::Error: std::fmt::Display, | ||
| { | ||
| R::try_from(val) | ||
| .map_err(serde::ser::Error::custom)? | ||
| .serialize(serializer) | ||
| } | ||
|
|
||
| // Deserializes a value of type R and type-casts it to type T. | ||
| pub(super) fn deserialize<'de, D, R, T>(deserializer: D) -> Result<T, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| R: Deserialize<'de>, | ||
| T: TryFrom<R>, | ||
| <T as TryFrom<R>>::Error: std::fmt::Display, | ||
| { | ||
| R::deserialize(deserializer) | ||
| .map(T::try_from)? | ||
| .map_err(serde::de::Error::custom) | ||
| } |
There was a problem hiding this comment.
intentionally generic so that it can be used elsewhere for similar type discrepancies.
AshwinSekar
approved these changes
Jul 19, 2024
yeah, looking into other blockstore fields with mismatching types. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
fec_set_indexisu32by the definition in the shred struct. Blockstore however usesu64forErasureMeta.fec_set_indexfor backward compatibility reasons which results in weird typing and fallible conversions fromu64tou32.Any
ErasureMetawith afec_set_indexwhich does not fit inu32is invalid and should be discarded early.Summary of Changes
The commit changes
ErasureMeta.fec_set_indextype tou32while usingserdeattributes to maintain backward compatibility for (de)serialization.