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

realbigsean · 2020-10-20T17:02:42Z

Issue Addressed

This comment: #1776 (comment)

Proposed Changes

Add quoted serde utils for FixedVector and VariableList
Had to remove the dependency that ssz_types has on serde_utils to avoid a circular dependency.

Additional Info

paulhauner

Nice, will be great to fix this up for users.

I had a few small comments, should be easy.

Had to remove the dependency that ssz_types has on serde_utils to avoid a circular dependency.

Perhaps we could remove this circular dep (and duplication of hex_decode and PrefixedHexVisitor) by defining the quoted_u64_fixed_vec and quoted_u64_var_list in the ssz_types crate, instead of serde_utils. I can't imagine these definitions being used anywhere else than in the ssz_types crate, so I think it makes sense structurally.

paulhauner · 2020-10-25T23:26:57Z

consensus/serde_utils/src/quoted_u64_fixed_vec.rs

+
+#[derive(Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct QuotedIntWrapper {


It's pretty minor, but perhaps we could import this from crate::quoted_u64_vec?

paulhauner · 2020-10-25T23:27:10Z

consensus/serde_utils/src/quoted_u64_var_list.rs

+
+#[derive(Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct QuotedIntWrapper {


As above, regarding importing existing definition.

paulhauner · 2020-10-25T23:31:28Z

consensus/serde_utils/src/quoted_u64_fixed_vec.rs

+            let val: QuotedIntWrapper = val;
+            vec.push(val.int);
+        }
+        let fix: FixedVector<u64, N> = FixedVector::from(vec);


I can see it's subjective, but I think it would be safest if we reject anything that doesn't match the N length. This is what we do in ssz, so it seems fitting here.

Ideally, we would want to avoid reading any more than N elements from seq (as to prevent unnecessary allocations).

Yes, I think you're totally right. It's even explicit in the API spec: https://github.com/ethereum/eth2.0-APIs/blob/master/types/state.yaml#L71

paulhauner · 2020-10-25T23:32:53Z

consensus/serde_utils/src/quoted_u64_fixed_vec.rs

+where
+    S: Serializer,
+{
+    let mut seq = serializer.serialize_seq(Some(value.len()))?;


IMO, no need to check the len against N here. We can rely on the upstream type (FixedVector).

No change requested.

paulhauner · 2020-10-25T23:33:19Z

consensus/serde_utils/src/quoted_u64_var_list.rs

+            let val: QuotedIntWrapper = val;
+            vec.push(val.int);
+        }
+        let fix: VariableList<u64, N> = VariableList::from(vec);


As per previous comment, I think it would be safest to check we don't exceed the N length.

paulhauner · 2020-10-25T23:35:10Z

consensus/ssz_types/src/bitfield.rs

    }
 }

+/// Encode `data` as a 0x-prefixed hex string.


See primary review comment regarding code duplication.

… lengths are incorrect

realbigsean · 2020-10-26T16:42:27Z

consensus/ssz_types/src/variable_list.rs

        let vec = vec![];
        let fixed: VariableList<u64, U4> = VariableList::from(vec);
-        assert_eq!(&fixed[..], &vec![][..]);
+        let expected: Vec<u64> = vec![];


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

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

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

It is weird that the modules caused the change tho.

updated 👍

paulhauner · 2020-10-29T03:54:41Z

consensus/ssz_types/src/serde_utils/quoted_u64_var_list.rs

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


Suggested change

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

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

paulhauner · 2020-10-29T03:55:02Z

consensus/ssz_types/src/serde_utils/quoted_u64_var_list.rs

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


Suggested change

Ok(fix)

Ok(list)

paulhauner

Only a couple of more suggestions, if you don't mind?

paulhauner · 2020-10-29T04:53:04Z

consensus/ssz_types/src/serde_utils/quoted_u64_fixed_vec.rs

+    {
+        let mut vec = vec![];
+
+        while let Some(val) = seq.next_element()? {


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) }

Yeah, makes sense. Updated 👍

paulhauner · 2020-10-29T04:53:54Z

consensus/ssz_types/src/serde_utils/quoted_u64_var_list.rs

+    {
+        let mut vec = vec![];
+
+        while let Some(val) = seq.next_element()? {


That fn deser_max function should work here too :)

paulhauner

Nice!

bors r+

…1794) ## Issue Addressed This comment: #1776 (comment) ## Proposed Changes - Add quoted serde utils for `FixedVector` and `VariableList` - Had to remove the dependency that `ssz_types` has on `serde_utils` to avoid a circular dependency. ## Additional Info Co-authored-by: realbigsean <seananderson33@gmail.com>

bors · 2020-10-30T00:15:01Z

Pull request successfully merged into master.

Build succeeded:

add quoted serialization util for FixedVector and VariableList

717a0d7

realbigsean added the ready-for-review The code is ready for review label Oct 21, 2020

paulhauner requested changes Oct 25, 2020

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 25, 2020

realbigsean added 3 commits October 26, 2020 08:59

move the ssz_type serde utils to the ssz_type crate

57bdc63

move imports to tests

f8fc03b

fail on quoted u64 deserialization if FixedVector or VariableList…

1d0a2d6

… lengths are incorrect

realbigsean commented Oct 26, 2020

View reviewed changes

update docs

a3fed2a

realbigsean added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 26, 2020

update vec to slice in var list test

16c94c4

paulhauner reviewed Oct 29, 2020

View reviewed changes

paulhauner requested changes Oct 29, 2020

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 29, 2020

ensure we don't deserialize more than N items in our max length types

02a60c8

realbigsean added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 29, 2020

paulhauner approved these changes Oct 29, 2020

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 29, 2020

bors bot changed the title ~~add quoted serialization util for FixedVector and VariableList~~ [Merged by Bors] - add quoted serialization util for FixedVector and VariableList Oct 30, 2020

bors bot closed this Oct 30, 2020

michaelsproul mentioned this pull request Sep 23, 2021

Remove duplication of serde_utils #2617

Closed

realbigsean deleted the fix-beacon-state-serde branch November 21, 2023 16:14

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

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

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

Uh oh!

Conversation

realbigsean commented Oct 20, 2020

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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