RPITIT and address a TODO for reveal #965
Conversation
| @@ -206,6 +210,20 @@ impl<'a, F: ExtendableField> Reveal<UpgradedMaliciousContext<'a, F>, 1> for Mali | |||
| } | |||
| } | |||
|
|
|||
| // Workaround for https://github.com/rust-lang/rust/issues/100013. Calling this wrapper function | |||
| // instead of `Reveal::reveal` seems to hide the `impl Future` GAT. | |||
| pub fn reveal<'fut, C, S>( | |||
There was a problem hiding this comment.
just curious if using pub async fn reveal(...) -> Result<S::Output,..> works too
There was a problem hiding this comment.
It does not... my hypothesis is it's related to this issue noted in my comment on #909:
The compiler is smart enough to see that if you have &local in the async move future, it can make that work by moving local into the future. However, this often results in the HRTB errors when the compiler needs to prove a statement of the form "for any lifetime 'local describing &local in the anonymous future, some other anonymous lifetime like 'ctx must outlive 'local.", which looks like
for <'local> 'ctx: 'local, which the compiler can only reduce to'ctx: 'static.
if that's easy, you could post a link to this PR and we can see if it ends up being not that hard. I agree that it is probably an overkill, but we could leave it here for educational purposes |
| .then(|| left_sender.send(record_id, right)); | ||
| let send_right_fut = (Some(ctx.role().peer(Direction::Right)) != left_out) | ||
| .then(|| right_sender.send(record_id, left)); | ||
| ctx.parallel_join(send_left_fut.into_iter().chain(send_right_fut)) |
There was a problem hiding this comment.
A parallel join involves a bunch of machinery. Is there a cheaper way to join two Option<impl Future> ?
There was a problem hiding this comment.
What I really wanted here is the native try_join_all. To avoid the parallel join overhead without trying to use the disallowed method, I added a MaybeFuture helper so that we can treat each of the left and right actions as futures whether or not they're actually sending.
| ctx: UpgradedMaliciousContext<'a, F>, | ||
| record_id: RecordId, | ||
| left_out: Role, | ||
| left_out: Option<Role>, |
There was a problem hiding this comment.
An observation, unrelated to this change:
"left" out is a poor name in this context because of the use of "left" and "right" as subjective labels for helpers. "skipped" or "excluded" perhaps?
There was a problem hiding this comment.
I changed it to excluded.
| ) -> Result<Option<<V as Vectorizable<N>>::Array>, Error> | ||
| where | ||
| C: 'fut, | ||
| { | ||
| let left = self.left_arr(); | ||
| let right = self.right_arr(); | ||
|
|
||
| // send except to left_out | ||
| if ctx.role().peer(Direction::Right) != left_out { | ||
| // send except to excluded helper (if any) |
There was a problem hiding this comment.
| // send except to excluded helper (if any) | |
| // Send shares, unless the target helper is excluded |
Here it is: 56b3de6 Or as a diff vs. the version in this PR: https://github.com/andyleiserson/ipa/compare/e7793d60bd22d6c5298ee54cb020378d06cd84af..reveal-alt?expand=1 |
I kinda like it - it does look heavy on the trait implementation side, but using it is quite easy. Not insisting on using it though |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
==========================================
- Coverage 89.24% 89.24% -0.01%
==========================================
Files 158 159 +1
Lines 21718 21735 +17
==========================================
+ Hits 19383 19398 +15
- Misses 2335 2337 +2 ☔ View full report in Codecov by Sentry. |
Two changes here (separated into two commits) related only in that they both affect reveal.
The first change eliminates async_trait for the
Revealtrait, using the same wrapper function workaround for HRTB issues that I used forBooleanArrayMul.The second change uses a single generic reveal implementation to support both full and partial reveals, reducing duplication.
I have a different version of the second change that specifies whether a reveal is full or partial (and if partial, which helper is excluded) as a type parameter to the generic implementation. It would offer the following:
unwrapin the trait-provided implementation ofreveal. (The unwrap is perfectly safe as long as the code stays as-is, just unsightly.)However, it ends up being more than a few lines of type-generic (i.e. hard to read) code, so I think it's probably not worth it. But I can share it if there's interest.