Minimize drbg initialization in ot extension, small optimizations and streams #249
Conversation
Simple functionality to adjust the length of a candidate byte array by either truncating it, or streching it using some secure but deterministic strategy (e.g., in this implementation SHA-256 and a counter).
A simple implementation of OTP using LengthAdjustment.java to adjust the length of keys to the length of the messages to be en/decrypted.
- This makes is to make it very easy to switch to parallel processing.
- Copies arrays bytewise instead of bitwise
- In the MultiplyWithoutReduction method we optimize by pre-computing each of the eight possible rotations of bytes in the b-vector.
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
============================================
+ Coverage 98.87% 98.89% +0.02%
- Complexity 2812 2827 +15
============================================
Files 318 320 +2
Lines 8441 8446 +5
Branches 695 691 -4
============================================
+ Hits 8346 8353 +7
Misses 85 85
+ Partials 10 8 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Two things:
- It is very annoying that you bundled serveral changes in one branch - it is quite hard to see the minimization for all the streaming and reformating. Please create seperate branches in the future.
- Make sure the last lines are covered, there should not be a reason for not reaching 100 %
Otherwise fine. I had hoped for more gains actually, it is still slow sadly
|
@pffrandsen I totally agree, this branch got a little out of hand. I will do better in the furture. I also agree the performance gain is a little disappointing. One way to improve might be to use more parallelization. The addition of streams should make this quite easy to add. The coverage is at 100% already though. Not sure what else to test. |
| } | ||
| return res; | ||
| int amountToPreprocess = computeExtensionSize(choiceBits.getSize(), comSecParam, statSecParam); | ||
| byte[] extraByteChoices = Arrays.copyOf(choiceBits.toByteArray(), amountToPreprocess / 8); |
There was a problem hiding this comment.
Shouldn't it be Byte.SIZE instead of 8, just to be consistent in the usage of constants :)
| * @param i an integer | ||
| * @return a corresponding byte array | ||
| */ | ||
| private static byte[] intToBytes(int i) { |
There was a problem hiding this comment.
I guess that for (rather pedantic) consistency we should do a loop from 0 to Integer.BYTES and shift based on a multiple of Byte.BIT, rather than having hardcoded 4 bytes for an int and 8 bits per byte. But I think that might be a bit too much to do, because we already have the necessary guarantees from Java that what is below will work.
There was a problem hiding this comment.
I considered it! But I felt it would be simpler this way.
There was a problem hiding this comment.
I must admit that I agree :)
| byte[] adjusted1 = LengthAdjustment.adjust(candidate, adjustedLength); | ||
| assertEquals(adjustedLength, adjusted1.length); | ||
| byte[] adjusted2 = LengthAdjustment.adjust(candidate, adjustedLength); | ||
| assertArrayEquals(adjusted1, adjusted2); |
There was a problem hiding this comment.
Perhaps add sanity test that random output looks random, e.g. not the zero-vector.
| assertFalse(Arrays.equals(Arrays.copyOf(candidate, cipherLength), cipherText)); | ||
| } | ||
| byte[] decryptedMessage = PseudoOtp.decrypt(cipherText, candidate, cipherLength); | ||
| assertArrayEquals(Arrays.copyOf(message, cipherLength), decryptedMessage); |
There was a problem hiding this comment.
Perhaps we should also check that no zero-vectors have sneaked in here?
There was a problem hiding this comment.
I think that is already covered by testing for equality of the original message.
There was a problem hiding this comment.
I agree, handled with the extra check in the lengthAdjustmentTest
| try { | ||
| Drbg rand = new AesCtrDrbg(HelperForTests.seedOne); | ||
| DhParameters params = new DhParameters(); | ||
| Ot otSender = new NaorPinkasOt(2, rand, network, params | ||
| .computeSecureDhParams(1, 2, rand, network)); |
There was a problem hiding this comment.
Do we want to remove the option of computing computeSecureDhParams from DhParameters? Because if not, then the testing using this method should not have been removed from all the tests in this class. On the other hand if we do want to remove the computeSecureDhParams option then the code for this method should be removed from DhParameters.
There was a problem hiding this comment.
Good point, I will go with just removing the computeSecureDhParams method, as we are never really using it anyway.
There was a problem hiding this comment.
I think you have made a really nice refactorization, making the OT extension much more dynamic, and in many cases easier to read. However, I do have a couple of questions/comments I would like some clarification on before final approval:
Why did you remove clarifying comments in NaorPinkasOt? In particular comments clarifying exactly what math is going on. They were meant to make the code easier to read if read alongside the paper.
Regarding speed improvements; I assume Java is very good at optimising things, but in general changing a lot of the code from imperative to lambda expressions might result in some overhead? I am also a bit unsure why exactly you rewrote most of the transpositioning code into lambda expression. Was it for purely cosmetic/coding style reasons?
Also, depending on where the bottleneck is, an alternative could be to not do Eklundh, but simply the trivial approach for matrix transposition.
As Peter says, the decrease in coverage is a not so nice feature of the pull-request, is it possible to add a few more tests to prevent this decrease in coverage?
Finally, I think your auto formatting has been set 100 char lines instead of 80?
|
@jot2re I wrote some point by point comments below: Removing comments in NaorPinkasOtI felt that the code was already nicely documented by having code factored into well named local methods, which were all well documented. So the comments did not seem to be needed. My personal taste is that in this case the code can actually be made harder to read by being cluttered by too many inline comments. I guess it is a matter of taste. Is there anything in particular where you feel the comments should be added back? Stream overheadI think you are right that using streams can have bit of overhead, but as far as I can see it is not significant here. I really like the new stream API, and to me it makes the code very easy to understand, which is part of the reason why I rewrote it in that style. An other reason is that it makes it very easy to parallelize the computation (basically just add TransposeYes, I mainly rewrote this to streams to make it more clear to myself. Once I reduced the number of DRBG initializations, it turned out transpose was the new bottleneck. To fix that I first "cleaned up" the code a bit to get a better idea of what was going on. As far as I can see transposing is no longer a bottleneck btw., so I think we should keep Eklundh. The new bottleneck appears to be CoverageI agree. I guess I just focused on the patch having 100% coverage. I think, however, that I may have decreased DhParameters coverage by using static parameters in the tests (which I would like to keep doing because it really speeds up the tests though.). Linelength100 is the correct length https://google.github.io/styleguide/javaguide.html#s4.4-column-limit |
| List<StrictBitVector> products = IntStream.range(0, alist.size()) | ||
| .mapToObj(i -> multiplyWithoutReduction(alist.get(i), blist.get(i))) | ||
| .collect(Collectors.toList()); | ||
| res = products.stream().reduce(res, (a, b) -> { |
There was a problem hiding this comment.
I'd remove the extra collect step, i.e., call reduce on the stream resulting from mapToObj.
- Tests of not zero in LengthAdjustmentTest - Removed redundant collect
|
Having realized the dh creation just dropped out, I still think this creation should be present somewhere so we are not forced to reinvent how to create if we want to run on different parameters. It could be in a test class, in a comment or as a main - just make it discorable within this repo |
|
@pffrandsen The test in TestDhParameters generates DhParameters in order to test against the static parameters. Is that all you are asking for? |
|
@GuutBoy Some feedback below: Removing comments in NaorPinkasOt It is fine. No need to put things back. I was just curious about the motivation :) Stream overhead If it doesn't have a significant impact then everything is fine. I have not used the streams in java before, but if it is that easy to parallelise using them, then it sounds like a great tool I/we should use much more in the future. Transpose Ok. I don't have any optimisation suggestions for RotSharedImpl.multiplyWithoutReduction(...) unfortunately. Coverage Looks good now :) Linelength |
GuutBoy
deleted the
Minimize-DRBG-Initialization-in-OTExtension-and-MASCOT
branch
This pull request:
BristolRotBatchclass in order to optimize OT extension performance. This is done by introducing a utility class to adjust the length a random string to match a desired length. This works by, if necessary, stretching the string using a hash function but otherwise using the provided string. An other utility uses this to implement form of OTP encryption. (fixes Minimize DRBG initialization in MASCOT/OTE #248 )Transposeutility class by copying byte arrays bytewise instead of bitwiseMultiplyWithoutReductionmethod in theRotSharedImplclass by pre-computing all possible rotations of thebvecinput.