Skip to content

Conversation

@zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Aug 24, 2021

Some large fragments (private VoteTally with lots of proposals/shares) can overflow the current fragment size (u16) when serializing. This PR encodes the size of a Fragment as a u32 to overcome such limitations.

In addition, add some tests that could help detect such issues in the future:

  • impl Arbitrary for private VoteTally, so that we can test its ser/de implementations and stress the behavior of other parts with very large payloads.
  • Add a ser/de test for Fragment

Copy link
Contributor

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@zeegomo
Copy link
Contributor Author

zeegomo commented Aug 24, 2021

I'm not actually too happy with Block::read implementation, as it uses more details about how the size of a fragment than what I would be comfortable with.
I'm not exactly sure of what the Readable trait tries to represent, but I think that implementing Readable for FragmentRaw and following the same structure as Block::deserialize would be the best option here (unless there's a reason why Readable was not implemented for FragmentRaw)

let mut inner = Vec::new();
let mut rng = ChaChaRng::seed_from_u64(u64::arbitrary(g));
let crs_seed = String::arbitrary(g).into_bytes();
let committee_size = 1; // very time consuming
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe try 1..=2 just to exercise the multi-member case?

@mzabaluev
Copy link
Contributor

I'm not actually too happy with Block::read implementation, as it uses more details about how the size of a fragment than what I would be comfortable with.

IIRC there was an idea to be able to iterate through raw fragments in a block without copying their bodies out of a raw block, and the read implementation for Block may reflect this.

inner.push(DecryptedPrivateTallyProposal {
tally_result: (0..n_options.get())
.map(|_| u64::arbitrary(g))
.collect::<Box<[_]>>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, didn't know that .collect<Box<[_]>> is a thing, cool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything that implements FromIterator is fair game for the collect combinator, so yeah.

@zeegomo
Copy link
Contributor Author

zeegomo commented Aug 25, 2021

I'm not actually too happy with Block::read implementation, as it uses more details about how the size of a fragment than what I would be comfortable with.

This was related to a temporary implementation that I coded before changing it in 03e27ba. I'm reasonably happy with the current one

@zeegomo
Copy link
Contributor Author

zeegomo commented Aug 25, 2021

I've moved the Arbitrary impl for VoteTally which was causing problems to #626 and retained here only the encoding fix and a general test for fragments ser/de

@mzabaluev mzabaluev merged commit 655ba68 into input-output-hk:master Aug 26, 2021
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.

4 participants