Naor-Pinkas Base OT optimization with elliptic curves #378

Gugi264 · 2021-03-29T19:36:01Z

Closes #375

This PR optimizes the base Naor-Pinkas OT through the use of elliptic curves.
For this the curve p-256 is used (but can be changed easily).

The PR adds 3 new optional dependencies to the ot submodule.
1 For bouncy castle,
2 for the use of the commercial ECCelerate Library.
These dependencies are optional, the dependency and the corresponding files can be deleted if they are not wanted.

But at least one of those two is needed to use the optimization.

jonas-lj

Thanks for your PR! It looks great but we have a few comments - both on smaller issues and a few larger ones. My main concern is that the old OT seems to disappear - do you think it's possible to retain the original OT and add yours as a new one to optionally use?

core/src/main/java/dk/alexandra/fresco/framework/util/ByteAndBitConverter.java

tools/ot/pom.xml

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/base/BigIntElement.java

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/base/BouncyCastleECCElement.java

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/base/ECCelerateElement.java

jonas-lj · 2021-04-06T09:23:09Z

...tinytables/src/main/java/dk/alexandra/fresco/suite/tinytables/ot/TinyTablesNaorPinkasOt.java

-  }
-
-}
+package dk.alexandra.fresco.suite.tinytables.ot;


Maybe you have committed files with a different line? It's odd that the diff insists that you changed all lines in the files

I think that was a problem with the line seperators, should be fixed now

jonas-lj · 2021-04-06T09:23:53Z

...tinytables/src/main/java/dk/alexandra/fresco/suite/tinytables/ot/TinyTablesNaorPinkasOt.java

+import dk.alexandra.fresco.tools.ot.base.AbstractNaorPinkasOT;
+import dk.alexandra.fresco.tools.ot.base.BouncyCastleNaorPinkas;
+
+public class TinyTablesNaorPinkasOt implements TinyTablesOt {


Could you implement your OT as a new OT and keep the old TinyTablesNaorPinkasOt?

TinyTablesNaorPinkasOt uses stillt the old Naor Pinkas Implementation, just with the new name (BigIntNaorPinkas)

jonas-lj · 2021-04-06T10:25:16Z

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/base/BigIntElement.java

+  }
+
+  @Override
+  public InterfaceNaorPinkasElement groupOp(InterfaceNaorPinkasElement other) {


I think this could be done more elegantly by adding a generic parameter to InterfaceNaorPinkasElement to avoid type casting

Now uses templates, no cast needed anymore

Gugi264 · 2021-04-11T17:53:23Z

demos/common/src/main/java/dk/alexandra/fresco/demo/cli/CmdLineProtocolSuite.java

@@ -229,8 +226,7 @@ private Drbg getDrbg(int myId, int prgSeedLength) {
    Map<Integer, RotList> seedOts = new HashMap<>();
    for (int otherId = 1; otherId <= parties; otherId++) {
      if (myId != otherId) {
-        DHParameterSpec dhSpec = DhParameters.getStaticDhParams();
-        Ot ot = new NaorPinkasOt(otherId, drbg, network, dhSpec);
+        AbstractNaorPinkasOT ot = new BigIntNaorPinkas(otherId, drbg, network);


BigIntNaorPinkas has the same functionality as the old NaorPinkasOt.
The name is just different to differentiate between different NaorPinkas implementations.

Gugi264 · 2021-04-11T17:56:30Z

tools/ot/src/test/java/dk/alexandra/fresco/tools/ot/base/TestFunctionalNaorPinkas.java

+      Class clazz = this.testClass;
+      Constructor[] constructors = clazz.getConstructors();
+      Ot otSender = (AbstractNaorPinkasOT) constructors[0]
+          .newInstance(2, rand, network);


Used to get the constructor via reflection. Is not the most beautiful solution, but the best I could come up with without changing too much of the existing code

codecov · 2021-04-11T17:58:12Z

Codecov Report

Merging #378 (16cda54) into master (60c2345) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              master      #378   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      3598      3618   +20     
===========================================
  Files            404       408    +4     
  Lines          10177     10209   +32     
  Branches         776       776           
===========================================
+ Hits           10177     10209   +32

Flag	Coverage Δ
unittests	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lexandra/fresco/demo/cli/CmdLineProtocolSuite.java	`100.00% <100.00%> (ø)`
...co/suite/tinytables/ot/TinyTablesNaorPinkasOt.java	`100.00% <100.00%> (ø)`
...co/tools/bitTriples/BitTripleResourcePoolImpl.java	`100.00% <100.00%> (ø)`
...dra/fresco/tools/ot/base/AbstractNaorPinkasOT.java	`100.00% <100.00%> (ø)`
.../alexandra/fresco/tools/ot/base/BigIntElement.java	`100.00% <100.00%> (ø)`
...exandra/fresco/tools/ot/base/BigIntNaorPinkas.java	`100.00% <100.00%> (ø)`
...a/fresco/tools/ot/base/BouncyCastleECCElement.java	`100.00% <100.00%> (ø)`
...a/fresco/tools/ot/base/BouncyCastleNaorPinkas.java	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60c2345...16cda54. Read the comment docs.

Gugi264 · 2021-04-11T18:01:29Z

Thanks for the review!
I changed the files you highlighted.

My main concern is that the old OT seems to disappear - do you think it's possible to retain the original OT and add yours as a new one to optionally use?

The old one doesn't disappear (at least not the functionality), I just refactored it and it is now just called BigIntNaorPinkas. If you don't want the version using elliptic curves, just use that one.

jot2re

Looks good, but I think in general it is better to switch to Curve25519 as it is generally regarded as safer.

jot2re · 2022-02-16T10:41:31Z

...va/dk/alexandra/fresco/framework/builder/numeric/field/MersennePrimeFieldDefinitionTest.java

+  public void createElement() {
+      Long baseNumber = 2L;
+      MersennePrimeFieldDefinition fieldDefintion = new MersennePrimeFieldDefinition(4, 2);
+      MersennePrimeFieldElement elementFromString = (MersennePrimeFieldElement) fieldDefintion.createElement(baseNumber.toString());


I think something should be checked here since the code coverage says that not all versions FieldDefintion.createElement gets called.

No idea why that is, as highlighted from you, it is called and should therefore be in the code coverage

tools/ot/src/main/java/dk/alexandra/fresco/tools/ot/base/BouncyCastleNaorPinkas.java

…tleNaorPinkas to only use one ECPoint import

jot2re

Looks good! Thanks a lot for your contribution!

jot2re

Okay, I was a bit too quick approving. I see the build service has an issue building the project. It gi ved the following error /home/runner/work/fresco/fresco/core/src/test/java/dk/alexandra/fresco/framework/builder/numeric/field/MersennePrimeFieldDefinitionTest.java:[36,39] error: cannot find symbol
However, I did not have that issue when running the branch locally.
Comparing it with what is shown here at GitHub it seems that there is an a weird whitespace symbol on this line. I tries to fix it but I am not authorised to push to your branch.

Gugi264 · 2022-02-23T11:05:34Z

I also didnt have the problem locally. I also dont see the whitespace problem, but I hope I fixed it.

Gugi264 · 2022-02-23T13:40:35Z

I tried running the github actions on a different branch on my repository, and there I didn't get the error
See https://github.com/Gugi264/fresco/runs/5304055567?check_suite_focus=true

jonas-lj · 2022-02-23T20:35:59Z

I tried running the github actions on a different branch on my repository, and there I didn't get the error See https://github.com/Gugi264/fresco/runs/5304055567?check_suite_focus=true

I haven't run the code myself, but I think it's because the extractValue method on FieldElement was renamed to toBigInteger in september last year: f128d39

jonas-lj · 2022-02-24T13:42:47Z

Looks good. Thanks for your contribution :)

jonas-lj · 2022-02-24T13:43:04Z

Looks good. Thanks for your contribution :)

Gugi264 force-pushed the PRNaorPinkasOTwithEcc branch 2 times, most recently from acdb912 to e4423cb Compare March 29, 2021 19:46

Gugi264 closed this Mar 29, 2021

Gugi264 force-pushed the PRNaorPinkasOTwithEcc branch from e4423cb to 63ddfd8 Compare March 29, 2021 19:49

squased commits

fd6e370

Gugi264 reopened this Mar 29, 2021

fixed pom

ab88cf0

Gugi264 marked this pull request as ready for review March 29, 2021 19:56

jonas-lj requested changes Apr 6, 2021

View reviewed changes

Gugi264 added 4 commits April 11, 2021 17:26

fixed line seperators

8c36635

cleanup

5a784dc

removed ECCelerate

a24a34b

changed to default BigIntNaorPinkas

a41df54

Gugi264 commented Apr 11, 2021

View reviewed changes

added createElement Test for MersenPrimeFieldDefintion

a7f6c38

jonas-lj requested a review from jot2re February 16, 2022 09:28

jot2re requested changes Feb 16, 2022

View reviewed changes

Simon Guggi added 3 commits February 20, 2022 15:29

changed curve to Curve25519 and refactored Constructor from BouncyCas…

48b7e9b

…tleNaorPinkas to only use one ECPoint import

removed unnecessary imports

21f0ba5

QoL change

0b30dd9

jot2re approved these changes Feb 23, 2022

View reviewed changes

jot2re self-requested a review February 23, 2022 09:21

jot2re requested changes Feb 23, 2022

View reviewed changes

hopefully fixed whitespace error

94cf52e

Simon Guggi and others added 2 commits February 23, 2022 14:21

added qa

0cb4b6e

Merge branch 'master' into PRNaorPinkasOTwithEcc

9ee4069

fixed errors

16cda54

jonas-lj merged commit fa35303 into aicis:master Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Naor-Pinkas Base OT optimization with elliptic curves #378

Naor-Pinkas Base OT optimization with elliptic curves #378

Gugi264 commented Mar 29, 2021 •

edited

Loading

jonas-lj left a comment

jonas-lj Apr 6, 2021

Gugi264 Apr 11, 2021

jonas-lj Apr 6, 2021

Gugi264 Apr 11, 2021

jonas-lj Apr 6, 2021

Gugi264 Apr 11, 2021

Gugi264 Apr 11, 2021

Gugi264 Apr 11, 2021

codecov bot commented Apr 11, 2021 •

edited

Loading

Gugi264 commented Apr 11, 2021 •

edited

Loading

jot2re left a comment

jot2re Feb 16, 2022

Gugi264 Feb 20, 2022 •

edited

Loading

jot2re left a comment

jot2re left a comment •

edited

Loading

Gugi264 commented Feb 23, 2022

Gugi264 commented Feb 23, 2022 •

edited

Loading

jonas-lj commented Feb 23, 2022

jonas-lj commented Feb 24, 2022

jonas-lj commented Feb 24, 2022

Naor-Pinkas Base OT optimization with elliptic curves #378

Naor-Pinkas Base OT optimization with elliptic curves #378

Conversation

Gugi264 commented Mar 29, 2021 • edited Loading

jonas-lj 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

codecov bot commented Apr 11, 2021 • edited Loading

Codecov Report

Gugi264 commented Apr 11, 2021 • edited Loading

jot2re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gugi264 Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

jot2re left a comment

Choose a reason for hiding this comment

jot2re left a comment • edited Loading

Choose a reason for hiding this comment

Gugi264 commented Feb 23, 2022

Gugi264 commented Feb 23, 2022 • edited Loading

jonas-lj commented Feb 23, 2022

jonas-lj commented Feb 24, 2022

jonas-lj commented Feb 24, 2022

Gugi264 commented Mar 29, 2021 •

edited

Loading

codecov bot commented Apr 11, 2021 •

edited

Loading

Gugi264 commented Apr 11, 2021 •

edited

Loading

Gugi264 Feb 20, 2022 •

edited

Loading

jot2re left a comment •

edited

Loading

Gugi264 commented Feb 23, 2022 •

edited

Loading