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.

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

Fix compatibility de/serialization of Policies #878

Merged
merged 15 commits into from
Dec 6, 2024

Conversation

AurelienFT
Copy link
Contributor

@AurelienFT AurelienFT commented Dec 4, 2024

Issue

We want to be able to execute transactions that were created with transactions builders using versions before 0.59.0 in the next VM versions.
The problem is that with the new policy (#871) the array of policies values is now of length 5 and before it was length 4. On 0.59.0 the serde de/serialization is taking 5 "Words" to decode the policies values array which cause transactions from 0.58.0 to not be able to be deserialized correctly.

Fix

We want in this PR to change the serde de/serialization so that we ensure that previous format is still supported and new policies too.

We first de/serialize the PoliciesBits this allow us to know if we have only policies set in the first 4 (old) policies then we just the backward compatibility mode otherwise we use a new mode with a flexible length.

Previous serde derive expanded (to compare with new implementation)

const _: () = {
            #[allow(unused_extern_crates, clippy::useless_attribute)]
            extern crate serde as _serde;
            #[automatically_derived]
            impl _serde::Serialize for Policies {
                fn serialize<__S>(
                    &self,
                    __serializer: __S,
                ) -> _serde::__private::Result<__S::Ok, __S::Error>
                where
                    __S: _serde::Serializer,
                {
                    let mut __serde_state = _serde::Serializer::serialize_struct(
                        __serializer,
                        "Policies",
                        false as usize + 1 + 1,
                    )?;
                    _serde::ser::SerializeStruct::serialize_field(
                        &mut __serde_state,
                        "bits",
                        &self.bits,
                    )?;
                    _serde::ser::SerializeStruct::serialize_field(
                        &mut __serde_state,
                        "values",
                        &self.values,
                    )?;
                    _serde::ser::SerializeStruct::end(__serde_state)
                }
            }
        };
        #[doc(hidden)]
        #[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
        const _: () = {
            #[allow(unused_extern_crates, clippy::useless_attribute)]
            extern crate serde as _serde;
            #[automatically_derived]
            impl<'de> _serde::Deserialize<'de> for Policies {
                fn deserialize<__D>(
                    __deserializer: __D,
                ) -> _serde::__private::Result<Self, __D::Error>
                where
                    __D: _serde::Deserializer<'de>,
                {
                    #[allow(non_camel_case_types)]
                    #[doc(hidden)]
                    enum __Field {
                        __field0,
                        __field1,
                        __ignore,
                    }
                    #[doc(hidden)]
                    struct __FieldVisitor;
                    impl<'de> _serde::de::Visitor<'de> for __FieldVisitor {
                        type Value = __Field;
                        fn expecting(
                            &self,
                            __formatter: &mut _serde::__private::Formatter,
                        ) -> _serde::__private::fmt::Result {
                            _serde::__private::Formatter::write_str(
                                __formatter,
                                "field identifier",
                            )
                        }
                        fn visit_u64<__E>(
                            self,
                            __value: u64,
                        ) -> _serde::__private::Result<Self::Value, __E>
                        where
                            __E: _serde::de::Error,
                        {
                            match __value {
                                0u64 => _serde::__private::Ok(__Field::__field0),
                                1u64 => _serde::__private::Ok(__Field::__field1),
                                _ => _serde::__private::Ok(__Field::__ignore),
                            }
                        }
                        fn visit_str<__E>(
                            self,
                            __value: &str,
                        ) -> _serde::__private::Result<Self::Value, __E>
                        where
                            __E: _serde::de::Error,
                        {
                            match __value {
                                "bits" => _serde::__private::Ok(__Field::__field0),
                                "values" => _serde::__private::Ok(__Field::__field1),
                                _ => _serde::__private::Ok(__Field::__ignore),
                            }
                        }
                        fn visit_bytes<__E>(
                            self,
                            __value: &[u8],
                        ) -> _serde::__private::Result<Self::Value, __E>
                        where
                            __E: _serde::de::Error,
                        {
                            match __value {
                                b"bits" => _serde::__private::Ok(__Field::__field0),
                                b"values" => _serde::__private::Ok(__Field::__field1),
                                _ => _serde::__private::Ok(__Field::__ignore),
                            }
                        }
                    }
                    impl<'de> _serde::Deserialize<'de> for __Field {
                        #[inline]
                        fn deserialize<__D>(
                            __deserializer: __D,
                        ) -> _serde::__private::Result<Self, __D::Error>
                        where
                            __D: _serde::Deserializer<'de>,
                        {
                            _serde::Deserializer::deserialize_identifier(
                                __deserializer,
                                __FieldVisitor,
                            )
                        }
                    }
                    #[doc(hidden)]
                    struct __Visitor<'de> {
                        marker: _serde::__private::PhantomData<Policies>,
                        lifetime: _serde::__private::PhantomData<&'de ()>,
                    }
                    impl<'de> _serde::de::Visitor<'de> for __Visitor<'de> {
                        type Value = Policies;
                        fn expecting(
                            &self,
                            __formatter: &mut _serde::__private::Formatter,
                        ) -> _serde::__private::fmt::Result {
                            _serde::__private::Formatter::write_str(
                                __formatter,
                                "struct Policies",
                            )
                        }
                        #[inline]
                        fn visit_seq<__A>(
                            self,
                            mut __seq: __A,
                        ) -> _serde::__private::Result<Self::Value, __A::Error>
                        where
                            __A: _serde::de::SeqAccess<'de>,
                        {
                            let __field0 = match _serde::de::SeqAccess::next_element::<
                                PoliciesBits,
                            >(&mut __seq)? {
                                _serde::__private::Some(__value) => __value,
                                _serde::__private::None => {
                                    return _serde::__private::Err(
                                        _serde::de::Error::invalid_length(
                                            0usize,
                                            &"struct Policies with 2 elements",
                                        ),
                                    );
                                }
                            };
                            let __field1 = match _serde::de::SeqAccess::next_element::<
                                [Word; POLICIES_NUMBER],
                            >(&mut __seq)? {
                                _serde::__private::Some(__value) => __value,
                                _serde::__private::None => {
                                    return _serde::__private::Err(
                                        _serde::de::Error::invalid_length(
                                            1usize,
                                            &"struct Policies with 2 elements",
                                        ),
                                    );
                                }
                            };
                            _serde::__private::Ok(Policies {
                                bits: __field0,
                                values: __field1,
                            })
                        }
                        #[inline]
                        fn visit_map<__A>(
                            self,
                            mut __map: __A,
                        ) -> _serde::__private::Result<Self::Value, __A::Error>
                        where
                            __A: _serde::de::MapAccess<'de>,
                        {
                            let mut __field0: _serde::__private::Option<PoliciesBits> = _serde::__private::None;
                            let mut __field1: _serde::__private::Option<
                                [Word; POLICIES_NUMBER],
                            > = _serde::__private::None;
                            while let _serde::__private::Some(__key) = _serde::de::MapAccess::next_key::<
                                __Field,
                            >(&mut __map)? {
                                match __key {
                                    __Field::__field0 => {
                                        if _serde::__private::Option::is_some(&__field0) {
                                            return _serde::__private::Err(
                                                <__A::Error as _serde::de::Error>::duplicate_field("bits"),
                                            );
                                        }
                                        __field0 = _serde::__private::Some(
                                            _serde::de::MapAccess::next_value::<
                                                PoliciesBits,
                                            >(&mut __map)?,
                                        );
                                    }
                                    __Field::__field1 => {
                                        if _serde::__private::Option::is_some(&__field1) {
                                            return _serde::__private::Err(
                                                <__A::Error as _serde::de::Error>::duplicate_field("values"),
                                            );
                                        }
                                        __field1 = _serde::__private::Some(
                                            _serde::de::MapAccess::next_value::<
                                                [Word; POLICIES_NUMBER],
                                            >(&mut __map)?,
                                        );
                                    }
                                    _ => {
                                        let _ = _serde::de::MapAccess::next_value::<
                                            _serde::de::IgnoredAny,
                                        >(&mut __map)?;
                                    }
                                }
                            }
                            let __field0 = match __field0 {
                                _serde::__private::Some(__field0) => __field0,
                                _serde::__private::None => {
                                    _serde::__private::de::missing_field("bits")?
                                }
                            };
                            let __field1 = match __field1 {
                                _serde::__private::Some(__field1) => __field1,
                                _serde::__private::None => {
                                    _serde::__private::de::missing_field("values")?
                                }
                            };
                            _serde::__private::Ok(Policies {
                                bits: __field0,
                                values: __field1,
                            })
                        }
                    }
                    #[doc(hidden)]
                    const FIELDS: &'static [&'static str] = &["bits", "values"];
                    _serde::Deserializer::deserialize_struct(
                        __deserializer,
                        "Policies",
                        FIELDS,
                        __Visitor {
                            marker: _serde::__private::PhantomData::<Policies>,
                            lifetime: _serde::__private::PhantomData,
                        },
                    )
                }
            }
        };

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@AurelienFT AurelienFT marked this pull request as ready for review December 4, 2024 23:13
@AurelienFT AurelienFT requested a review from a team December 4, 2024 23:14
@AurelienFT AurelienFT marked this pull request as draft December 4, 2024 23:18
@AurelienFT AurelienFT marked this pull request as ready for review December 4, 2024 23:55
xgreenx
xgreenx previously approved these changes Dec 6, 2024
@AurelienFT AurelienFT requested a review from a team December 6, 2024 03:34
Co-authored-by: Andrea Cerone <22031682+acerone85@users.noreply.github.com>
@acerone85
Copy link
Contributor

acerone85 commented Dec 6, 2024

General comment on serialization and deserialization: You can still derive most of it.

The main idea is that you can define your own structure PoliciesV1 and PoliciesV2, only for serialization/deserialization purposes, for which #[derive(Serialize, Deserialize)] are present. Then in Policies you only need to implement the logic for choosing which version to Serialize/Deserialize, and defer to the serializer/deserializer for the specific version.

This is what I have got locally:

pub struct Policies {
    /// A bitmask that indicates what policies are set.
    bits: PoliciesBits,
    /// The array of policy values.
    values: [Word; POLICIES_NUMBER],
}

#[derive(serde::Serialize, serde::Deserialize)]
#[serde(rename = "Policies")]
pub struct PoliciesV1 {
    bits: PoliciesBits,
    values: [Word; 4],
}

#[derive(serde::Serialize, serde::Deserialize)]
#[serde(rename = "Policies")]
pub struct PoliciesV2 {
    bits: PoliciesBits,
    values: Vec<Word>,
}

// This serde is manually implemented because of the `values` field format.
// Serialization of the `values` field :
// 1. Always write the 4 elements for the first 4 policies even if they are not set for
//    backward compatibility.
// 2. For the remaining, write the value only if the policy is set.
impl serde::Serialize for Policies {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        let Policies { bits, values } = self;

        if self.bits.intersection(PoliciesBits::all())
            == self.bits.intersection(
                PoliciesBits::Maturity
                    .union(PoliciesBits::MaxFee)
                    .union(PoliciesBits::Tip)
                    .union(PoliciesBits::WitnessLimit),
            )
        {
            PoliciesV1 {
                bits: *bits,
                values: values[..4].try_into().map_err(|_| {
                    serde::ser::Error::custom("The first 4 values should be present")
                })?,
            }
            .serialize(serializer)
        } else {
            PoliciesV2 {
                bits: *bits,
                values: values.to_vec(),
            }
            .serialize(serializer)
        }
    }
}

impl<'de> serde::Deserialize<'de> for Policies {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::Deserializer<'de>,
    {
        let policies_v2 = PoliciesV2::deserialize(deserializer)?;
        if policies_v2.bits.intersection(PoliciesBits::all())
            == policies_v2.bits.intersection(
                PoliciesBits::Maturity
                    .union(PoliciesBits::MaxFee)
                    .union(PoliciesBits::Tip)
                    .union(PoliciesBits::WitnessLimit),
            )
        {
            let mut values: [Word; POLICIES_NUMBER] = [0; POLICIES_NUMBER];
            values[..4].copy_from_slice(&policies_v2.values[..4]);
            Ok(Policies {
                bits: policies_v2.bits,
                values,
            })
        } else {
            let mut values: [Word; POLICIES_NUMBER] = [0; POLICIES_NUMBER];
            values[..POLICIES_NUMBER]
                .copy_from_slice(&policies_v2.values[..POLICIES_NUMBER]);
            Ok(Policies {
                bits: policies_v2.bits,
                values,
            })
        }
    }
}

acerone85
acerone85 previously approved these changes Dec 6, 2024
Copy link
Contributor

@acerone85 acerone85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have proposed a change but it can be implemented on top of your PR

@AurelienFT AurelienFT requested a review from xgreenx December 6, 2024 14:21
@xgreenx
Copy link
Collaborator

xgreenx commented Dec 6, 2024

General comment on serialization and deserialization: You can still derive most of it.

Maybe it is possible to deserialize bits first, and based on that, deserialize the remaining part?

Maybe the approach with PoliciesV2 also can work if serde serializes the size of the vector and a tuple.

@acerone85
Copy link
Contributor

General comment on serialization and deserialization: You can still derive most of it.

Maybe it is possible to deserialize bits first, and based on that, deserialize the remaining part?

Maybe the approach with PoliciesV2 also can work if serde serializes the size of the vector and a tuple.

Yep. Alternatively you can try to deserialize as V2, and if either the deserialization fails or only the first 4 policy bits are set, you try to deserialize as V1. But I'm not 100% happy with that, as it requires deserializing twice in the worst case

@acerone85 acerone85 added this pull request to the merge queue Dec 6, 2024
Merged via the queue into master with commit 4de6034 Dec 6, 2024
40 checks passed
@acerone85 acerone85 deleted the backward_compatible_deserialization branch December 6, 2024 22:24
@AurelienFT AurelienFT mentioned this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants