Serialize and Deserialize implementations of block::header::Header do not match

[romac]

Problem

block::header::Header defines an app_hash field as follows:

    /// State after txs from the previous block
    #[serde(deserialize_with = "serializers::parse_hex")]
    pub app_hash: Vec<u8>,

This means that app_hash is serialized as a byte array, but serializers::parse_hex expects a string holding hex-encoded data, which results in the following error when attempting to do a serialize-then-deserialize round trip:

invalid type: sequence, expected a string", line: 26, column: 20

Proposed solutions

Solution 1

If I understand correctly, app_hash is not fixed-length hash (eg. SHA256) but variable-length hex-encoded data. I thus suggest keep the Vec<u8> type but add a serde annotation to serialize the data with serialize_hex:

    /// State after txs from the previous block
    #[serde(
        serialize_with = "serializers::serialize_hex",
        deserialize_with = "serializers::parse_hex"
    )]
    pub app_hash: Vec<u8>,

Pros

• Easy fix
• Keep a meaningful type for the field (ie. it's a byte vector).

Cons

• None w.r.t. to the current state of things

Solution 2

Alternatively, we could define a Hex struct with custom Serialize and Deserialize instances:

struct Hex(Vec<u8>);

impl Serialize for Hex {
    fn serialize<S: serde::ser::Serializer>(
        &self,
        serializer: S,
    ) -> std::result::Result<S::Ok, S::Error> {
        tendermint::serializers::serialize_hex(&self.0, serializer)
    }
}

impl<'de> Deserialize<'de> for Hex {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: serde::de::Deserializer<'de>,
    {
        tendermint::serializers::parse_hex(deserializer).map(|bytes| Self(bytes))
    }
}

Pros

• One cannot forget to specify a (de)serializer for field
• One cannot provide incompatible (de)serializers for a field

Cons

• Pollutes the interface of the struct with a type that is only meaningful for serialization, but has otherwise no semantic meaning
• Involves a bit more code and tests

Recommendation

I am partial to solution 1 as it preserves the semantic meaning of the type, but would be happy to implement solution 2 as well if anyone has a good argument for it.

[liamsi]

Good catch! And a sign we should better test even just for serde serialization and deserialization. I agree that solution 1 is the way to go.

[romac]

There is actually a third solution, that kinda combines the two others:

Solution 3

• Define a Hex struct as above
• Keep the Vec<u8> type on the app_hash field
• Add the following serde annoations on the app_hash field:

#[serde(from = "Hex")]

Deserialize this type (Vec<u8>) by deserializing into Hex, then converting. This type (Vec<u8>) must implement From<Hex>, and Hex must implement Deserialize.

#[serde(into = "Hex")]

Serialize this type (Vec<u8>) by converting it into the specified Hex and serializing that. This type must implement Clone and Into<Vec<u8>>, and Hex must implement Serialize.

---

To me it feels a nice tradeoff between the two approaches above, with a bit more safety as one just has to remember to put both into and from serde annotations but can use the same struct name, so there is less chance of picking the wrong (de)serialization function.

The main downside I see is that it introduces more code. On the other hand this struct could be re-used in other ways outside of serde serialization.

We could give Hex a Display instance, to enable one to easily print a byte array in hex format by just doing println!("{}", Hex(some_byte_vector)). Likewise for parsing, if we give it a FromString instance.

@liamsi What do you think?

[liamsi]

Introducing another struct to (de)serialize a field (app_hash) in a struct (Header) whose (current) main purpose itself is serialization feels a bit weird. But I agree, it looks like a clean solution too. As longs as the Hex struct isn't publicly accessible, I think we are good with your last proposed option as well 👍

[romac]

Alright, let's go with the leanest solution for now then :)

We can always revisit the 3rd solution as some later stage if we see the need for the Display and FromString instances as well.