Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

324 mac check spdz #343

Merged
merged 28 commits into from Jun 19, 2019
Merged

324 mac check spdz #343

merged 28 commits into from Jun 19, 2019

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented Jun 7, 2019

This PR addresses #324 which is a bug in our SPDZ mac-checking protocol.

In particular, we need to run a fresh coin tossing each time we run a MAC check. To implement this, the PR moves several generic computations (for instance BroadcastComputation) out of SPDZ2k and into core, so these can be re-used across protocol suites.

Re-using these generic computations also allows us to remove some classes that were previously SPDZ-specific (e.g., SpdzCommitProtocol) and use the same commitment functionality as is currently used in MASCOT.

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #343 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #343      +/-   ##
============================================
- Coverage       100%   99.97%   -0.03%     
+ Complexity     3316     3292      -24     
============================================
  Files           369      366       -3     
  Lines          9181     9108      -73     
  Branches        689      680       -9     
============================================
- Hits           9181     9106      -75     
- Misses            0        1       +1     
- Partials          0        1       +1
Impacted Files Coverage Δ Complexity Δ
...protocols/computations/Spdz2kInputComputation.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...va/dk/alexandra/fresco/suite/spdz/SpdzBuilder.java 100% <ø> (ø) 8 <0> (ø) ⬇️
...resco/lib/generic/BroadcastValidationProtocol.java 85.71% <ø> (ø) 5 <0> (?)
...xandra/fresco/lib/generic/SecureBroadcastUtil.java 100% <ø> (ø) 9 <0> (?)
...lexandra/fresco/demo/cli/CmdLineProtocolSuite.java 100% <ø> (ø) 17 <0> (ø) ⬇️
.../fresco/lib/generic/InsecureBroadcastProtocol.java 100% <ø> (ø) 4 <0> (?)
...ra/fresco/suite/spdz/gates/SpdzNativeProtocol.java 100% <ø> (ø) 7 <0> (-2) ⬇️
...tocols/computations/Spdz2kMacCheckComputation.java 100% <100%> (ø) 17 <0> (ø) ⬇️
...dra/fresco/lib/generic/CoinTossingComputation.java 100% <100%> (ø) 7 <3> (?)
...ra/fresco/suite/spdz/SpdzRoundSynchronization.java 100% <100%> (ø) 13 <0> (ø) ⬇️
... and 7 more

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 8115293...5cf79a1. Read the comment docs.

@n1v0lg n1v0lg requested review from pffrandsen and jot2re June 11, 2019 06:56
Copy link
Contributor

@rubensayshi rubensayshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reviewing the PR out of curiosity of the changes and figured I might as well leave my review comments if you care for them ;)

@n1v0lg
Copy link
Contributor Author

n1v0lg commented Jun 13, 2019

@rubensayshi awesome, thanks for the review! All good points. Addressed in 5f9eb04.

Copy link
Collaborator

@jot2re jot2re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the inline comments; but it seems that there is a bit of a consistency issue of the changes in SPDZ compared to SPDZ2k. Furthermore it seems that the MAC check issue might remain in SPDZ2k.

Copy link
Collaborator

@pffrandsen pffrandsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing much to note from my part


BroadcastComputation(List<byte[]> input) {
public BroadcastComputation(List<byte[]> input, boolean runValidation) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable to document this class and the constructor - especially when it it resonable to use one or the other

final AesCtrDrbg localDrbg = new AesCtrDrbg();
final HashBasedCommitmentSerializer commitmentSerializer = new HashBasedCommitmentSerializer();

return builder.seq(new CoinTossingComputation(drbgSeedBitLength / 8,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reconsider the alignment here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually check the entire file :)

Copy link
Collaborator

@jot2re jot2re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@jot2re jot2re merged commit ecbda1b into master Jun 19, 2019
@jonas-lj jonas-lj deleted the 324-mac-check-spdz branch January 20, 2021 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants