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

[Merged by Bors] - add quoted serialization util for FixedVector and VariableList #1794

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions consensus/ssz_types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ typenum = "1.12.0"
arbitrary = { version = "0.4.6", features = ["derive"], optional = true }

[dev-dependencies]
serde_json = "1.0.58"
tree_hash_derive = "0.2.0"
1 change: 1 addition & 0 deletions consensus/ssz_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#[macro_use]
mod bitfield;
mod fixed_vector;
pub mod serde_utils;
mod tree_hash;
mod variable_list;

Expand Down
2 changes: 2 additions & 0 deletions consensus/ssz_types/src/serde_utils/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub mod quoted_u64_fixed_vec;
pub mod quoted_u64_var_list;
117 changes: 117 additions & 0 deletions consensus/ssz_types/src/serde_utils/quoted_u64_fixed_vec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
//! Formats `FixedVector<u64,N>` using quotes.
//!
//! E.g., `FixedVector::from(vec![0, 1, 2])` serializes as `["0", "1", "2"]`.
//!
//! Quotes can be optional during decoding. If `N` does not equal the length of the `Vec`, the `Vec` is truncated.

use crate::FixedVector;
use serde::ser::SerializeSeq;
use serde::{Deserializer, Serializer};
use serde_utils::quoted_u64_vec::QuotedIntWrapper;
use std::marker::PhantomData;
use typenum::Unsigned;

pub struct QuotedIntFixedVecVisitor<N> {
_phantom: PhantomData<N>,
}

impl<'a, N> serde::de::Visitor<'a> for QuotedIntFixedVecVisitor<N>
where
N: Unsigned,
{
type Value = FixedVector<u64, N>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "a list of quoted or unquoted integers")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: serde::de::SeqAccess<'a>,
{
let mut vec = vec![];

while let Some(val) = seq.next_element()? {
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't clear on this before, but I think we should add a safe-guard against growing vec greater than N::to_usize(). I was tempted to just leave it as is, but some of the vulnerabilities observed in ssz have made me weary of over-allocating.

E.g., (I'm not sure if this compiles)

/// Returns a `Vec` of no more than `max_items` length.
fn deser_max(mut seq: A, max_items: usize) -> Result<Vec<QuotedIntWrapper>, A::Error>
where
        A: serde::de::SeqAccess<'a>,
{
  let mut vec = vec![];
  let mut counter = 0;
  
  while let Some(val) = seq.next_element()? {
    counter += 1;
    if counter > max_items {
      return Err(Something)
    }

    vec.push(val.int);
  }

  Ok(vec)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense. Updated 👍

let val: QuotedIntWrapper = val;
vec.push(val.int);
}
let fix: FixedVector<u64, N> = FixedVector::new(vec)
.map_err(|e| serde::de::Error::custom(format!("FixedVector: {:?}", e)))?;
Ok(fix)
}
}

pub fn serialize<S>(value: &[u64], serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut seq = serializer.serialize_seq(Some(value.len()))?;
for &int in value {
seq.serialize_element(&QuotedIntWrapper { int })?;
}
seq.end()
}

pub fn deserialize<'de, D, N>(deserializer: D) -> Result<FixedVector<u64, N>, D::Error>
where
D: Deserializer<'de>,
N: Unsigned,
{
deserializer.deserialize_any(QuotedIntFixedVecVisitor {
_phantom: PhantomData,
})
}

#[cfg(test)]
mod test {
use super::*;
use serde_derive::{Deserialize, Serialize};
use typenum::U4;

#[derive(Debug, Serialize, Deserialize)]
struct Obj {
#[serde(with = "crate::serde_utils::quoted_u64_fixed_vec")]
values: FixedVector<u64, U4>,
}

#[test]
fn quoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", "2", "3", "4"] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn unquoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn mixed_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", 2, "3", "4"] }"#).unwrap();
let expected: FixedVector<u64, U4> = FixedVector::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn empty_list_err() {
serde_json::from_str::<Obj>(r#"{ "values": [] }"#).unwrap_err();
}

#[test]
fn short_list_err() {
serde_json::from_str::<Obj>(r#"{ "values": [1, 2] }"#).unwrap_err();
}

#[test]
fn long_list_err() {
serde_json::from_str::<Obj>(r#"{ "values": [1, 2, 3, 4, 5] }"#).unwrap_err();
}

#[test]
fn whole_list_quoted_err() {
serde_json::from_str::<Obj>(r#"{ "values": "[1, 2, 3, 4]" }"#).unwrap_err();
}
}
120 changes: 120 additions & 0 deletions consensus/ssz_types/src/serde_utils/quoted_u64_var_list.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
//! Formats `VariableList<u64,N>` using quotes.
//!
//! E.g., `VariableList::from(vec![0, 1, 2])` serializes as `["0", "1", "2"]`.
//!
//! Quotes can be optional during decoding. If `N` is greater than the length of the `Vec`, the `Vec` is truncated.

use crate::VariableList;
use serde::ser::SerializeSeq;
use serde::{Deserializer, Serializer};
use serde_utils::quoted_u64_vec::QuotedIntWrapper;
use std::marker::PhantomData;
use typenum::Unsigned;

pub struct QuotedIntVarListVisitor<N> {
_phantom: PhantomData<N>,
}

impl<'a, N> serde::de::Visitor<'a> for QuotedIntVarListVisitor<N>
where
N: Unsigned,
{
type Value = VariableList<u64, N>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "a list of quoted or unquoted integers")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: serde::de::SeqAccess<'a>,
{
let mut vec = vec![];

while let Some(val) = seq.next_element()? {
Copy link
Member

Choose a reason for hiding this comment

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

That fn deser_max function should work here too :)

let val: QuotedIntWrapper = val;
vec.push(val.int);
}
let fix: VariableList<u64, N> = VariableList::new(vec)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let fix: VariableList<u64, N> = VariableList::new(vec)
let list: VariableList<u64, N> = VariableList::new(vec)

.map_err(|e| serde::de::Error::custom(format!("VariableList: {:?}", e)))?;
Ok(fix)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ok(fix)
Ok(list)

}
}

pub fn serialize<S>(value: &[u64], serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut seq = serializer.serialize_seq(Some(value.len()))?;
for &int in value {
seq.serialize_element(&QuotedIntWrapper { int })?;
}
seq.end()
}

pub fn deserialize<'de, D, N>(deserializer: D) -> Result<VariableList<u64, N>, D::Error>
where
D: Deserializer<'de>,
N: Unsigned,
{
deserializer.deserialize_any(QuotedIntVarListVisitor {
_phantom: PhantomData,
})
}

#[cfg(test)]
mod test {
use super::*;
use serde_derive::{Deserialize, Serialize};
use typenum::U4;

#[derive(Debug, Serialize, Deserialize)]
struct Obj {
#[serde(with = "crate::serde_utils::quoted_u64_var_list")]
values: VariableList<u64, U4>,
}

#[test]
fn quoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", "2", "3", "4"] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn unquoted_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn mixed_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": ["1", 2, "3", "4"] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2, 3, 4]);
assert_eq!(obj.values, expected);
}

#[test]
fn empty_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [] }"#).unwrap();
assert!(obj.values.is_empty());
}

#[test]
fn short_list_success() {
let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2] }"#).unwrap();
let expected: VariableList<u64, U4> = VariableList::from(vec![1, 2]);
assert_eq!(obj.values, expected);
}

#[test]
fn long_list_err() {
serde_json::from_str::<Obj>(r#"{ "values": [1, 2, 3, 4, 5] }"#).unwrap_err();
}

#[test]
fn whole_list_quoted_err() {
serde_json::from_str::<Obj>(r#"{ "values": "[1, 2, 3, 4]" }"#).unwrap_err();
}
}
3 changes: 2 additions & 1 deletion consensus/ssz_types/src/variable_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,8 @@ mod test {

let vec = vec![];
let fixed: VariableList<u64, U4> = VariableList::from(vec);
assert_eq!(&fixed[..], &vec![][..]);
let expected: Vec<u64> = vec![];
Copy link
Member Author

Choose a reason for hiding this comment

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

Once I added the new modules, I was getting this error:

Screen Shot 2020-10-26 at 12 41 00 PM

Which is why I made this update. But I don't know why adding the new modules would cause it to pop up...

Copy link
Member

Choose a reason for hiding this comment

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

Can you use &[] instead of &vec![][..]?

Copy link
Member

Choose a reason for hiding this comment

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

It is weird that the modules caused the change tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated 👍

assert_eq!(&fixed[..], &expected[..]);
}

#[test]
Expand Down
2 changes: 2 additions & 0 deletions consensus/types/src/beacon_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,14 @@ where
#[compare_fields(as_slice)]
pub validators: VariableList<Validator, T::ValidatorRegistryLimit>,
#[compare_fields(as_slice)]
#[serde(with = "ssz_types::serde_utils::quoted_u64_var_list")]
pub balances: VariableList<u64, T::ValidatorRegistryLimit>,

// Randomness
pub randao_mixes: FixedVector<Hash256, T::EpochsPerHistoricalVector>,

// Slashings
#[serde(with = "ssz_types::serde_utils::quoted_u64_fixed_vec")]
pub slashings: FixedVector<u64, T::EpochsPerSlashingsVector>,

// Attestations
Expand Down