Clean up Field trait
#958
Clean up Field trait
#958
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #958 +/- ##
==========================================
+ Coverage 88.43% 88.47% +0.04%
==========================================
Files 158 158
Lines 21472 21515 +43
==========================================
+ Hits 18988 19035 +47
+ Misses 2484 2480 -4 ☔ View full report in Codecov by Sentry. |
| boolean_vector!(3, BA3); | ||
| boolean_vector!(5, BA5); | ||
| boolean_vector!(8, BA8); | ||
| boolean_vector!(20, BA20); | ||
| boolean_vector!(32, BA32); | ||
| boolean_vector!(64, BA64); | ||
| boolean_vector!(256, BA256); |
There was a problem hiding this comment.
The set of sizes we generate values for isn't consistent. I hope that is OK. Will someone be surprised if they try to invoke something for which there is one implementation and not another?
There was a problem hiding this comment.
For vectorization in general, because rust's const generic support is limited, the supported vectorizations need to be enumerated explicitly in the source. I did the same thing here (listing only the ones that we use). Also, arbitrary vectorization dimensions are only needed where we're using the record-wise vectorization support to implement bit-wise vectorization within a record. Implementing only the necessary vectorizations helps to discourage that.
* Remove `Field` trait impl from Boolean arrays. * Remove u128 conversions from `Fp25519`. * Add `select` variant of `if_else`. `select` is bus multiplexer with a single-bit control input. `if_else` is a vectorizable multiplexer, with condition input the same width as the data inputs. * Remove `Field` (or replace with `SharedValue`) in various trait bounds. * Implement `Vectorizable` and related traits for more boolean arrays. Fixes private-attribution#812
andyleiserson
force-pushed
the
field-cleanup
branch
from
247b53b to
38e2461
Compare
| /// This macro uses a bit of recursive repetition to produce those zeros. | ||
| /// | ||
| /// The longest call is 8 bits, which involves `2(n+1)` macro expansions in addition to `bitarr!`. | ||
| macro_rules! bitarr_one { |
There was a problem hiding this comment.
so we don't need this awesome macro anymore? :)
ipa-core/src/ff/boolean_array.rs
Outdated
| let mut max_value = $name::ZERO.0; | ||
| max_value[..$bits].fill(true); | ||
| deserialize(max_value).unwrap(); | ||
|
|
||
| let mut rnd_value = $name::ZERO.0; |
There was a problem hiding this comment.
does it check something that this test doesn't already?
There was a problem hiding this comment.
No, I don't think it does. I added this one to replace the test case in this function that deserialized the ONE value, but I can drop it in favor of the other test.
| let false_value = false_value.clone().into(); | ||
| let true_value = true_value.clone().into(); | ||
| let condition = B::expand(condition).into(); |
There was a problem hiding this comment.
maybe use From so the types for these values become obvious?
| record_id: RecordId, | ||
| a: &'fut Self::Vectorized, | ||
| b: &'fut Self::Vectorized, | ||
| ) -> impl Future<Output = Result<Self::Vectorized, Error>> + Send + 'fut |
| TV: SharedValue + U128Conversions + CustomArray<Element = Boolean>, | ||
| TS: SharedValue + U128Conversions + CustomArray<Element = Boolean>, | ||
| SS: SharedValue + U128Conversions + CustomArray<Element = Boolean>, | ||
| Replicated<BK>: BooleanArrayMul, |
There was a problem hiding this comment.
I guess you tried it already, but I am going to ask anyway - have you tried to provide blanket implementation BooleanArrayMul for all boolean shares?
There was a problem hiding this comment.
It's another consequence of const generic limitations -- see this comment.
Part of why things get weird here is that we're using the record-wise vectorization support for bit-wise vectorization. To make a fully vectorized version of these protocols we'd need to change that, and could probably get rid of BooleanArrayMul in the process.
| let f: Fp25519 = Fp25519::ONE; | ||
| f.serialize((&mut bits).into()); | ||
| Ok(f) | ||
| fn from_random(src: GenericArray<u128, Self::SourceLength>) -> Self { |
There was a problem hiding this comment.
Would it make sense to use a u8 as a base and increase the SourceLength? I think even Aes uses a GenericArray over u8
There was a problem hiding this comment.
yea we have #951 for that
Fieldtrait impl from Boolean arrays.Fp25519.selectvariant ofif_else.selectis bus multiplexer with a single-bit control input.if_elseis a vectorizable multiplexer, with condition input the same width as the data inputs.Field(or replace withSharedValue) in various trait bounds.Vectorizableand related traits for more boolean arrays.