Make upgrade take an iterator #724

martinthomson · 2023-06-23T06:36:14Z

The cost of this is that some of the type safety we got from the traits had to be cut (maybe we can restore them, but I'm not smart enough today).

martinthomson · 2023-06-23T06:37:01Z

src/protocol/context/upgrade.rs

-/// };
-/// // This can't be upgraded with a record-bound context because the record ID
-/// // is used internally for vector indexing.
-/// let _ = <UpgradeContext<C<'_, F>, F, RecordId> as UpgradeToMalicious<Vec<Replicated<F>>, _>>::upgrade;


I couldn't work out how to make this keep failing. The way it failed before was that we only implemented for Vec<Vec<T>>, but now that there is a generic IntoIterator version, I'm not sure what we would do to keep this.

Using a non-Vec type that is IntoIterator (like [Replicated<F>; 3]) would work, but I'm not sure how much value that has. I might be inclined to delete all of these doc tests -- without the negative case, I don't think the positive cases have much value.

Test restoration incoming. I forgot to revert this part of the change.

martinthomson · 2023-06-23T06:37:53Z

src/protocol/context/upgrade.rs

@@ -431,15 +407,14 @@ where
 // context. This is only used for tests where the protocol takes a single `Replicated<F>` input.
 #[cfg(test)]
 #[async_trait]
-impl<'a, C, F, T, M> UpgradeToMalicious<'a, T, M> for UpgradeContext<'a, C, F, NoRecord>
+impl<'a, C, F, M> UpgradeToMalicious<'a, Replicated<F>, M> for UpgradeContext<'a, C, F, NoRecord>


A smaller implementation worked fine, for all but the modulus conversion tests. This seemed like a good trade.

We might also need/want a corresponding non-record-bound implementation for BitDecomposed at some point. Right now I think malicious tests are upgrading BitDecomposed<T> to Vec<M> (which is harmless, just something I noticed while looking at the changes).

It might be worth a TODO mentioning this, since the error messages when the compiler doesn't find the upgrade implementation it needs can be a bit hard to interpret.

andyleiserson

It makes me a little nervous to have an impl for Vec<T> that panics for large vectors -- it appears at the surface like it will work for any Vec, but it won't. Vec<Vec<T>> seemed a little safer since that's a less common type to have around.

One easy improvement might be to return an error on attempt to upgrade a long vector rather than panic. Then at least if we end up reaching this case at runtime, the helper doesn't crash.

A fix that I do like but is much more work, is to use a dedicated type for "bit-decomposition of a value that is at most 64 bits". That would also help us get the last bit of storage efficiency for Vec<MaliciousReplicated<Gf2>> (although for that case, I'm also hopeful that we can use a larger field).

andyleiserson · 2023-06-23T19:58:09Z

src/protocol/context/upgrade.rs

@@ -431,15 +407,14 @@ where
 // context. This is only used for tests where the protocol takes a single `Replicated<F>` input.


Suggest changing this comment to:

// Impl to upgrade a single `Replicated<F>` using a non-record-bound context. Used for tests.

andyleiserson · 2023-06-23T20:27:52Z

src/protocol/context/upgrade.rs

 /// It assumes the inner vector is much smaller (e.g. multiple bits per record) than the outer vector (e.g. records)
 /// Each inner vector element uses a different context and outer vector shares a context for the same inner vector index


Suggested change

/// It assumes the inner vector is much smaller (e.g. multiple bits per record) than the outer vector (e.g. records)

/// Each inner vector element uses a different context and outer vector shares a context for the same inner vector index

andyleiserson · 2023-06-23T20:37:37Z

src/protocol/context/upgrade.rs

-/// };
-/// // This can't be upgraded with a record-bound context because the record ID
-/// // is used internally for vector indexing.
-/// let _ = <UpgradeContext<C<'_, F>, F, RecordId> as UpgradeToMalicious<Vec<Replicated<F>>, _>>::upgrade;


Using a non-Vec type that is IntoIterator (like [Replicated<F>; 3]) would work, but I'm not sure how much value that has. I might be inclined to delete all of these doc tests -- without the negative case, I don't think the positive cases have much value.

martinthomson · 2023-06-26T07:57:48Z

@andyleiserson I took a swing at doing the hard thing you mentioned. It wasn't easy, as you said, but I think that there are real improvements to be had from it.

The main drawback is that I had to move IntoShares to take Iterator rather than IntoIterator. I'm in two minds about that, but implementing IntoIterator for BitDecomposed seemed like it was more useful.

andyleiserson

The main drawback is that I had to move IntoShares to take Iterator rather than IntoIterator. I'm in two minds about that, but implementing IntoIterator for BitDecomposed seemed like it was more useful.

Yeah, I agree this is a tough call. If we had both owned and borrowed versions of BitDecomposed, then we might not need BitDecomposed to deref to a slice, and then maybe we could do without the IntoIterator implementation for BitDecomposed. But that's a whole additional set of changes to make.

It might be possible to avoid the into_iter() calls in the common case of passing a Vec to a test, in exchange for more verbosity in the less common case of passing an iterator to a test, by implementing IntoShares for Vec<U> and Box<dyn Iterator<Item = U>>. (Or for a newtype wrapper around an iterator.) However, the changes to call into_iter() are already done, so I don't see a compelling reason to pursue this.

andyleiserson · 2023-06-28T19:14:10Z

src/secret_sharing/decomposed.rs

+    /// # Panics
+    /// If the iterator produces more than `Self::MAX` items.
+    pub fn new<I: IntoIterator<Item = S>>(bits: I) -> Self {
+        let bits = bits.into_iter().collect::<Vec<_>>();


Do you know if the compiler optimizes this in the case where I is a Vec?

If it doesn't, it might make sense to expose separate interfaces for "from vec" and "from iterator".

https://gcc.godbolt.org/z/x9d7We759 seems to support the view that it optimizes out.

It's nice being able to use unstable features :)

andyleiserson · 2023-06-28T23:33:44Z

src/protocol/context/upgrade.rs

@@ -431,15 +407,14 @@ where
 // context. This is only used for tests where the protocol takes a single `Replicated<F>` input.
 #[cfg(test)]
 #[async_trait]
-impl<'a, C, F, T, M> UpgradeToMalicious<'a, T, M> for UpgradeContext<'a, C, F, NoRecord>
+impl<'a, C, F, M> UpgradeToMalicious<'a, Replicated<F>, M> for UpgradeContext<'a, C, F, NoRecord>


We might also need/want a corresponding non-record-bound implementation for BitDecomposed at some point. Right now I think malicious tests are upgrading BitDecomposed<T> to Vec<M> (which is harmless, just something I noticed while looking at the changes).

It might be worth a TODO mentioning this, since the error messages when the compiler doesn't find the upgrade implementation it needs can be a bit hard to interpret.

andyleiserson · 2023-06-28T23:47:23Z

src/protocol/sort/secureapplyinv.rs

@@ -67,10 +95,14 @@ mod tests {
                .map(u128::from)
                .map(Fp31::truncate_from);

+            // Flatten the input so that it can implement `IntoShares`.


Is it possible flattening is no longer necessary with the impls in the final version? I tried removing it, and it seemed to work.

Nice, I had forgotten that part.

andyleiserson · 2023-06-29T00:07:19Z

src/secret_sharing/replicated/malicious/additive_share.rs

    async fn downgrade(self) -> UnauthorizedDowngradeWrapper<Self::Target> {
-        // TODO: validate that the vector here is truly small.
        #[allow(clippy::disallowed_methods)]
        let result = join_all(


This is exactly the kind of thing I was hoping to catch with this change, nice!

Make upgrade take an iterator

ad9c720

The cost of this is that some of the type safety we got from the traits had to be cut (maybe we can restore them, but I'm not smart enough today).

martinthomson requested a review from andyleiserson June 23, 2023 06:36

martinthomson commented Jun 23, 2023

View reviewed changes

martinthomson mentioned this pull request Jun 23, 2023

Breakdown key simplification cleanups #725

Merged

andyleiserson approved these changes Jun 23, 2023

View reviewed changes

Do the harder work of creating a BitDecomposed wrapper

d72a84d

martinthomson requested a review from andyleiserson June 27, 2023 01:42

Restore doc test

8986d60

andyleiserson approved these changes Jun 29, 2023

View reviewed changes

martinthomson added 2 commits June 29, 2023 16:27

Remove flatten/unflatten mess

de4fb51

Merge branch 'main' into upgrade-iterator

3a70ee0

martinthomson merged commit 53d2b78 into private-attribution:main Jun 29, 2023

martinthomson deleted the upgrade-iterator branch June 29, 2023 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make upgrade take an iterator #724

Make upgrade take an iterator #724

martinthomson commented Jun 23, 2023

martinthomson Jun 23, 2023

andyleiserson Jun 23, 2023

martinthomson Jun 28, 2023

martinthomson Jun 23, 2023

andyleiserson Jun 28, 2023

andyleiserson left a comment

andyleiserson Jun 23, 2023

andyleiserson Jun 23, 2023

andyleiserson Jun 23, 2023

martinthomson commented Jun 26, 2023

andyleiserson left a comment

andyleiserson Jun 28, 2023

martinthomson Jun 29, 2023

andyleiserson Jun 29, 2023

andyleiserson Jun 28, 2023

andyleiserson Jun 28, 2023

martinthomson Jun 29, 2023

andyleiserson Jun 29, 2023

		@@ -431,15 +407,14 @@ where
		// context. This is only used for tests where the protocol takes a single `Replicated<F>` input.

		/// It assumes the inner vector is much smaller (e.g. multiple bits per record) than the outer vector (e.g. records)
		/// Each inner vector element uses a different context and outer vector shares a context for the same inner vector index

Make upgrade take an iterator #724

Make upgrade take an iterator #724

Conversation

martinthomson commented Jun 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleiserson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson commented Jun 26, 2023

andyleiserson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment