Mascot preprocessing #214

n1v0lg · 2017-12-21T15:40:11Z

Introduces MASCOT (https://eprint.iacr.org/2016/505.pdf) protocol for the SPDZ offline phase and an OT-extension protocol (https://eprint.iacr.org/2015/546). Also implements several useful sub-protocols, including base OT (www.pinkas.net/PAPERS/effot.ps), actively-secure broadcast, coin-tossing, and commitments.

…n Fresco

…t-preprocessing

pffrandsen

Just the first run, I have fixed a few comments myself.

I have not gone through all files yet.

pffrandsen · 2017-12-21T19:17:16Z

tools/mascot/pom.xml

+          <descriptorRefs>
+        <descriptorRef>jar-with-dependencies</descriptorRef>
+          </descriptorRefs>
+          <finalName>fresco-tools-mascot</finalName>


Do we package mascot - why?

Whoops that was a copy and paste. Gone now.

pffrandsen · 2017-12-21T19:27:50Z

tools/mascot/src/main/java/dk/alexandra/fresco/tools/mascot/arithm/CollectionUtils.java

+//import java.util.List;
+//import java.util.stream.Stream;
+//
+//// TODO this class is sad


pffrandsen · 2017-12-21T19:30:32Z

...t/src/main/java/dk/alexandra/fresco/tools/mascot/broadcast/BroadcastingNetworkDecorator.java

+   */
+  @Override
+  public void send(int partyId, byte[] data) {
+    throw new UnsupportedOperationException("Broadcast network can only send to all");


This looks more like a proxy than a decorator

Just rename the class

pffrandsen · 2017-12-21T19:50:06Z

tools/mascot/src/main/java/dk/alexandra/fresco/tools/mascot/triple/TripleGeneration.java

+    // for each value we will have two sub-factors for each other party
+    List<List<FieldElement>> subFactors = new ArrayList<>();
+
+    // TODO parallelize


Is this still a TODO or irellevant now?

This is a performance improvement for the > 2 party case. I will remove the TODO and make an improve performance issue instead where this can go as a subtask.

KasperDamgaard

In general: really cool!!! :D
I would like to see mascot removed from spdz though, as it is really just a different data supplier. Also, it would be awesome if mascot could store the preprocessed data on file. If we only have the option of generating new data on the fly, then we kinda loose the whole idea of the preprocessing concept. So creating an option/class for serializing to disk and either use the same format as the current static approach (SpdzStorageDataSupplier) or change that supplier to accomodate a faster/better way (optimal solution - since we currently are using crappy IO and inefficient ObjectStreams - we want NIO instead I think).

KasperDamgaard · 2017-12-22T08:33:30Z

core/src/main/java/dk/alexandra/fresco/commitment/HashBasedCommitment.java

+ * digest serves as the commitment itself and the opening is the randomness and
+ * the message committed to.
+ *
+ * @author jot2re


We decided at some point that all author tags be removed. Some are still remaining, but I guess we should be consistent on new files

I have removed all author tags from the OT package.

KasperDamgaard · 2017-12-22T08:38:02Z

core/src/main/java/dk/alexandra/fresco/framework/builder/numeric/NumericResourcePool.java

+import java.math.BigInteger;
+import java.security.MessageDigest;
+
+public interface NumericResourcePool extends ResourcePool {


Interface missing javadoc. Which might help for me in understanding exactly what it is. In my world, resource pools are specific to protocol suites, and I don't think all numeric protocol suites needs access to e.g. a hash algorithm?

KasperDamgaard · 2017-12-22T08:52:55Z

...e/spdz/src/main/java/dk/alexandra/fresco/suite/spdz/configuration/PreprocessingStrategy.java

-	STATIC; // Use data already present on the machine it's running on. 
+  DUMMY, // Use a dummy approach (e.g. always the same data)
+  MASCOT, // Use the Mascot preprocessing
+  STATIC // Use data already present on the machine it's running on.


We should not have Mascot as a possibility here. It belongs within the mascot project only.

KasperDamgaard · 2017-12-22T08:54:55Z

suite/spdz/src/main/java/dk/alexandra/fresco/suite/spdz/gates/SpdzMacCheckProtocol.java

@@ -111,7 +112,7 @@ public SpdzMacCheckProtocol(SecureRandom rand, MessageDigest digest, SpdzStorage
        }
        deltaSum = deltaSum.mod(modulus);
        if (!deltaSum.equals(BigInteger.ZERO)) {
-          throw new MPCException(
+          throw new MaliciousException(


Great! - while I remember: The other runtime exceptions in this class should also be malicious. This requires a few tests to be altered from what they expect.

Wait, the other exceptions I'm talking about are not present until the spdz-test-coverage PR is merged in. Whichever PR get's merged last, should correct that then :)

KasperDamgaard · 2017-12-22T09:00:04Z

suite/spdz/src/main/java/dk/alexandra/fresco/suite/spdz/storage/SpdzMascotDataSupplier.java

+  private int maxBitLength;
+  private int batchSize;
+
+  public static SpdzMascotDataSupplier createTestSupplier(


Do we want this in the "production" code?

KasperDamgaard · 2017-12-22T09:01:06Z

suite/spdz/src/test/java/dk/alexandra/fresco/suite/spdz/AbstractSpdzTest.java

    List<Integer> ports = new ArrayList<>(noOfParties);
    for (int i = 1; i <= noOfParties; i++) {
      ports.add(9000 + i * (noOfParties - 1));
    }

+    int maxBitLength;


The 150 doesn't matter for other preprocessing types. Just make it 128 if this is required by MASCOT

The bitlength has relation to the size of modulus but also needs to be aligned to 8.

Let us fix the mim test and then see

KasperDamgaard · 2017-12-22T09:12:35Z

suite/spdz/src/test/java/dk/alexandra/fresco/suite/spdz/TestSpdzMiMC.java

+
+  @Test
+  public void testMimcWithMascot() {
+    // TODO This fails (most likely because


Is this todo still valid?

KasperDamgaard · 2017-12-22T09:27:18Z

tools/mascot/src/main/java/dk/alexandra/fresco/tools/mascot/maccheck/MacCheck.java

+   * @param macShare this party's share of the mac
+   * @throws MaliciousException if mac-check fails
+   */
+  public void check(FieldElement opened, FieldElement macKeyShare, FieldElement macShare) {


Mostly out of curiosity: do we do MAC checks for just a single field element? Seems like it should be possible to pool multiple together and do the check on a sum.

Yep, but the batching happens outside of this class (in ElementGeneration.check for instance).

KasperDamgaard · 2017-12-22T09:36:16Z

suite/spdz/pom.xml

+
+    <dependency>
+      <groupId>dk.alexandra.fresco</groupId>
+      <artifactId>mascot</artifactId>


I do not agree that the online spdz should depend on mascot. Cause it doesn't :) - mascot effectively just provides a different SpdzDataSupplier, and all tests on that should be done within mascot. The same was done with the (now removed) fuelstation where a REST data supplier was created and used only within that project. So I suggest moving the mascot supplier to mascot.

There are three options:

Mascot depending on spdz

Spdz depending on mascot

new spdz offline maven project

1 is your suggestion, and implies that mascot should be seen as a subprotocol of spdz
2 is the current solution
3 is the correct, but cumbersome solution

The plan is to keep mascot pure, therefore we went with 2. Which also eliminates the potential jar hell at the end of that choice.

KasperDamgaard · 2017-12-22T09:39:38Z

suite/spdz/src/main/java/dk/alexandra/fresco/suite/spdz/storage/SpdzMascotDataSupplier.java

@@ -0,0 +1,161 @@
+package dk.alexandra.fresco.suite.spdz.storage;


As mentioned in the spdz POM file comment, this class should be located within the mascot project.

…t-preprocessing

pffrandsen · 2017-12-22T15:05:33Z

@Rhaea I definitely think we should aim for MVP, and not extend this PR to include offline storage possibilities. And I fail to see the reason to put implementations of offline storage into the framework. There are serialization strategies available so anyone can do their own storage.

GuutBoy · 2018-01-23T10:37:27Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/BristolOtReceiver.java

+    // Retrieve the random preprocessed message
+    byte[] randomMessage = randomMessages.get(offset).toByteArray();
+    // Use the random message as the seed to a PRG
+    Drbg currentPrg = new AesCtrDrbg(randomMessage);


See comment on optimization on the Sender side.

See comment on Sender side.

GuutBoy · 2018-01-23T10:42:18Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/BristolOtReceiver.java

+   * @param choiceBit
+   *          The actual choice bit of the receiver
+   */
+  private void sendSwitchBit(Boolean choiceBit) {


Is there a reason choiceBit should not be a primitive type boolean instead of an object Boolean?

Changed in c901c59

GuutBoy · 2018-01-23T10:43:07Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/BristolOtReceiver.java

+   *          Choice-bit. False for message 0, true for message 1.
+   * @return The serialized message from the OT
+   */
+  public byte[] receive(Boolean choiceBit) {


Is there a reason choiceBit should not be a primitive type boolean instead of an object Boolean?

Changed in 6bd02da

GuutBoy · 2018-01-23T10:58:05Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/BristolOt.java

+  private final int batchSize;
+
+  /**
+   * Constructs a new OT protocol and constructs the internal sender and receiver


This is not right, these objects are created in the send and receive methods. Possibly the java doc is updated?

Updated in 6bd02da

…t-preprocessing

GuutBoy · 2018-01-24T10:36:43Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/RotSharedImpl.java

+    // the value itself.
+    // Size of an int is always 4 bytes in java
+    ByteBuffer indexBuffer = ByteBuffer
+        .allocate(4 + input.get(0).getSize() * 8);


I think this should be .allocate(4 + input.get(0).getSize() / 8);

... possibly even .allocate((Integer.SIZE + input.get(0).getSize()) / 8); to make the code more readable (and make the comment above redundant).

Fixed in commit 0515efa

Actually, I think you missed some parenthesis. Nevermind, I will fix it before merge.

GuutBoy · 2018-01-24T10:42:01Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/BristolRotBatch.java

+   * @return A random messages generated using a PRG
+   */
+  private StrictBitVector computeRandomMessage(StrictBitVector seed, int sizeOfMessage) {
+    Drbg rand = new AesCtrDrbg(seed.toByteArray());


As mentioned other places this should just be taken directly from the seed if the length allows it. And otherwise should be the result of applying a hash function not a freshly seeded DRBG.

GuutBoy · 2018-01-24T10:52:47Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/RotList.java

+   */
+  public void send(Ot ot) {
+    if (sent == true) {
+      throw new IllegalStateException("Seed OTs have already been sent.");


Would it be possible/reasonable to just hand the ot object to the constructor and do the sending/receiving at construction? This way there would be no reason to handle these exceptions.

Maybe. The problem is that OT is asymmetric and thus one party must call send when the other calls receive and vice versa. Thus the constructor would also need the IDs from the OT functionality to do this properly. Furthermore this also requires the RotList to ALWAYS do OT in both directions. Something we might not want to do in general (MASCOT does it, but if we make 2PC protocols, we don't need it).

Also I am not too keen on doing too much network interaction in constructors, both in regards to testing, but also in regards to usage.

Ok, I guess you know better than I what problems it would cause :). However, at least then we should add to the documentation of these methods that they should only be called once/only called after send/recieve. Also, it would be nice to have getter methods for the sent/receive flags so you can test if the methods are safe to call.

Btw, we do network stuff in many constructors in, e.g., MASCOT, I do not see that as a problem.

Added documentation in commit 0515efa
I have not added gettter and setter methods since they are currently not needed and thus adding these would mean that we also had to test methods. In general I don't think it is a good idea to add dead code.
Regarding the network stuff: I personally prefer light constructors, in particular when we are at the primitive level (which I consider OT to be). The reason being that otherwise we must ensure that both parties are at the same place in the protocol at the same time, at the specific place where an object is constructed, not when it is necessary used.

Just the docs changed is fine by me.

About the constructor, I think in this case this implementation does make sense to me. As you say, you may not always both send and receive. So you cant just do both at construction (unless you pass extra parameters to the constructor or something, which would probably just be a mess anyway).

GuutBoy · 2018-01-24T12:47:27Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/Transpose.java

+ * vectors.
+ */
+public class Transpose {
+


For a utility class like this that is only intended to have static methods it is a good idea to add an empty private contructor so it is not instantiated by mistake. Like so:

private Tranpose() { }

Added in commit faa7e12

GuutBoy · 2018-01-24T12:53:09Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/otextension/CoteShared.java

+ * Superclass containing the common methods for the sender and
+ * receiver parties of correlated OT with errors.
+ */
+public class CoteShared {


I am not a huge fan of these *Shared super classes. It seems to me they are mainly used to hold common parameters and utility methods which could just as well be held in a separate helper class (basically the resourcepool).

At least, I think they should be made abstract as only their subclasses are intended to be instantiated.
(This comment goes for essentially all the classes with the Shared suffix).

I have already removed the getter methods so that they now only contain common methods in order to avoid code-copy between the sender and receiver, while still keeping an object oriented approach.
I have now also made them abstract in commit faa7e12

…act) Factory it is, see https://en.wikipedia.org/wiki/Abstract_factory_pattern

GuutBoy · 2018-01-24T13:03:49Z

I think I have been through everything, and will not be adding more comments. I think I am pretty much ready to merge once the last few bits are done. Good job guys 👍

GuutBoy · 2018-01-24T13:09:53Z

Btw. Note to self: let's try not to do a PR this huge ever again.

Merging fun

475d7e6

n1v0lg requested review from KasperDamgaard, GuutBoy and pffrandsen December 21, 2017 15:40

n1v0lg and others added 7 commits December 21, 2017 10:43

Use new network constructor in demo

ef1ae3b

Changed names so it is not implied to be secure, rather a subsystem i…

80fea70

…n Fresco

Merge remote-tracking branch 'origin/mascot-preprocessing' into masco…

6410b47

…t-preprocessing

Fixed visibilty

5a75335

Removed unneeded getmod

0fb5f37

Renamed FrescoSerializer to ByteSerializer

0b0ef78

Encapsulated fields

f6d6130

pffrandsen reviewed Dec 21, 2017

View reviewed changes

n1v0lg and others added 4 commits December 21, 2017 15:22

Remove dead code and outdated TODOs

d1ef72c

Don't package mascot

2628bad

Killed some unused imports

cddf71b

Merge branch 'develop' into mascot-preprocessing

77293d3

KasperDamgaard requested changes Dec 22, 2017

View reviewed changes

pffrandsen added 2 commits December 22, 2017 15:51

Better naming

441faf1

Merge remote-tracking branch 'origin/mascot-preprocessing' into masco…

a4b0f7f

…t-preprocessing

n1v0lg and others added 10 commits December 22, 2017 13:13

Smaller, co-prime modulus for spdz-mascot tests

fee5111

Nice prime

d29319b

Mascot input mask gen

c885ef0

Spdz input mask tests

ab27b7e

Use random element gen in supplier

cde82cd

Fixed class cast exception

521683f

Mindre netværk i test

3017f70

Removed dummy override version

2be3d88

Spdz is always spdz

eba224d

Spdz is always spdz

a33e9d0

GuutBoy reviewed Jan 23, 2018

View reviewed changes

jot2re and others added 9 commits January 23, 2018 12:47

Minor changed to Bit vector classes according to review comments

dddb173

Fixed merge conflict

5451ba6

Dead code

c13540a

Merge remote-tracking branch 'origin/mascot-preprocessing' into masco…

5f83849

…t-preprocessing

Dead code

2a93dfd

final field

d258a94

Renamed things and added comments to Naor Pinkas OT

06f4731

Changed Boolean to boolean

c901c59

Minor changes according to review

6bd02da

GuutBoy reviewed Jan 24, 2018

View reviewed changes

Implement mod finder and misc tests

8ad218e

GuutBoy reviewed Jan 24, 2018

View reviewed changes

Removed redundant interface and made the RotFactory the actual (Abstr…

3e61953

…act) Factory it is, see https://en.wikipedia.org/wiki/Abstract_factory_pattern

GuutBoy approved these changes Jan 24, 2018

View reviewed changes

pffrandsen approved these changes Jan 24, 2018

View reviewed changes

jot2re added 2 commits January 24, 2018 15:40

Addresed minor review comments

0515efa

Handled minor review comments

faa7e12

jot2re approved these changes Jan 24, 2018

View reviewed changes

Added missing parens.

fcc0a55

GuutBoy merged commit 328b893 into develop Jan 25, 2018

GuutBoy deleted the mascot-preprocessing branch January 25, 2018 12:07

		@@ -0,0 +1,161 @@
		package dk.alexandra.fresco.suite.spdz.storage;

Mascot preprocessing #214

Mascot preprocessing #214

Conversation

n1v0lg commented Dec 21, 2017

pffrandsen 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

KasperDamgaard 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pffrandsen commented Dec 22, 2017

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

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

GuutBoy Jan 24, 2018 • edited

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

GuutBoy Jan 24, 2018 • edited

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

GuutBoy commented Jan 24, 2018 • edited

GuutBoy commented Jan 24, 2018

GuutBoy Jan 24, 2018 •

edited

GuutBoy Jan 24, 2018 •

edited

GuutBoy commented Jan 24, 2018 •

edited