Framework for variable-size PRSS generation #903
Conversation
Implement proper random generation of BA256.
| ($modname:ident, $name:ident, $bits:tt) => { | ||
| boolean_array_impl!($modname, $name, $bits); | ||
|
|
||
| impl Field for $name { |
There was a problem hiding this comment.
should we add a todo here that we want to remove it later?
| @@ -32,7 +32,7 @@ pub async fn integer_add<C, XS, YS>( | |||
| where | |||
| C: Context, | |||
| YS: SharedValue + CustomArray<Element = XS::Element>, | |||
| XS: SharedValue + CustomArray + Field, | |||
| XS: SharedValue + CustomArray + SharedValue, | |||
There was a problem hiding this comment.
Not sure if this is why you left the comment, but I just noticed this now says SharedValue + CustomArray + SharedValue. Oops.
There was a problem hiding this comment.
yea I was impressed that Rust allows to specify that :)
| type SourceLength: ArrayLength; | ||
|
|
||
| /// Generate a random value of `Self` from `SourceLength` uniformly-distributed u128s. | ||
| fn from_random(src: GenericArray<u128, Self::SourceLength>) -> Self; |
There was a problem hiding this comment.
curious about the choice of u128 for container? why not u8?
There was a problem hiding this comment.
I used u128 because it matches what we get from PRSS. I'd be fine with using u8. The only hesitation I have about u8 is that it hides the fact that the randomness is generated in multiples of 128 bits, i.e., an implementation that takes a single u8 is much less efficient than it looks.
There was a problem hiding this comment.
agree, it is better to indicate that it is a multiple of 16 bytes
| // It would be nice for this to be TryFrom, but there's a lot of places where we use u128s as PRSS indexes. | ||
| impl From<u128> for PrssIndex { | ||
| fn from(value: u128) -> Self { | ||
| Self(value.try_into().unwrap()) |
There was a problem hiding this comment.
This would be a great place to use .expect() instead of .unwrap().
| impl PrssIndex { | ||
| fn offset(self, offset: usize) -> PrssIndex128 { | ||
| assert!(offset <= u16::MAX.into()); | ||
| PrssIndex128::from(u128::from((u64::from(self.0) << 16) + (offset as u64))) |
There was a problem hiding this comment.
Do you really want to only use 16 bits for the offset, given that this is a 32-bit value?
There was a problem hiding this comment.
Do you think 32 makes more sense? There was also @danielmasny's suggestion of 64 in #884 (comment).
Implementing Display will eliminate one reason for minimizing it, but it also seemed unlikely that any single invocation of PRSS would generate a value with more than 2^23 bits of entropy. I considered as little as 2^15. However, 2^15 bits is a 4,096-byte vector. That is the upper end of the range I foresee using, but I wanted to allow flexibility to go a bit bigger than that.
I don't have a strong opinion on this. 32 seems totally fine. I was prodded not to do the original suggestion of 64/64 when I saw that the existing RecordId and PRSS index are only 32 bits. I do wince a little at some of our use of u128, but this isn't a case of that since we're feeding it to AES anyways.
There was a problem hiding this comment.
Keeping the values within a 64-bit range is fine, possibly even a good idea.
| @@ -569,7 +638,7 @@ pub mod test { | |||
| #[test] | |||
| #[cfg(debug_assertions)] | |||
| #[should_panic( | |||
| expected = "Generated randomness for index '100' twice using the same key 'protocol/test'" | |||
| expected = "Generated randomness for index '6553600' twice using the same key 'protocol/test'" | |||
There was a problem hiding this comment.
any sense in implementing Display for PrssIndexU128 to make this less of a pain to use?
| for i in 0..N::USIZE { | ||
| self.used.insert(index.offset(i)); | ||
| } |
There was a problem hiding this comment.
I might suggest that we only need to track against PrssIndex for this. It will greatly reduce the tracking overhead.
There was a problem hiding this comment.
It's probably sufficient, but aren't we already planning to disable this tracking in production configurations? In which case, I'd rather be cautious and do the more extensive check, unless we determine it's a huge hit to our dev runtimes.
Vectorization will shift usage from the index bits to the offset bits, but the overall number of entries added to the map will stay the same. i.e. this change does not increase the size requirement for the tracking structure.
There was a problem hiding this comment.
Just a thought. The API doesn't let you take an index plus a range of offsets that don't start at zero, so I figured... why not.
You might get similar coverage by only checking 0 and N-1, FWIW.
There is some relevant background discussion on #884 and #852.
This addresses some, but not all, of the remaining cleanup for #812 -- this makes BA256 not a
Field, which happens to be doable because it's not used as one anywhere. We also need to make the smaller BAs notField, but that requires addressing places where they are used asFields.BA256 provides a sample of generating something with PRSS that needs more than 128 random bits, but this will also be needed for vectorization.
This doesn't address unnecessary generation of 128 random bits even when the value being generated is smaller. Once things are vectorized that will be less of an issue.