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

Feedback and issues with initial commit #2

Open
jot2re opened this issue Jul 8, 2022 · 0 comments
Open

Feedback and issues with initial commit #2

jot2re opened this issue Jul 8, 2022 · 0 comments

Comments

@jot2re
Copy link

jot2re commented Jul 8, 2022

In general everything looks nice and good. I found some things in specific files and some general things related to testing. All of which is listed below:

GWBuilder:

SecureRandom should be used instead of Random.

Method randomBit: If we can assume q mod 4 = 3 of the field, then simply taking the square of a random number and computing the sign of the squareroot will give a random bit. See protocol 2.1 in "Improved Primitives for Secure Multiparty Integer Computation" by Catrina and de Hoogh. This will only be one round, although it uses a multiplication it will also be directly malicious secure if the underlying multiplication protocol is.

BGWResourcePool:
Line 38 has a forgotten line of out-commented code.

BGWRoundSynchronization:
No code is actually executed in this class. It seem to still need to be implemented?

Util:
getRandomFieldElement. I would recommend instead sampling a random big integer between 0 and 2^s*q for the field with modulo q, then take the value mod q. This allows for constant time (if the underlying code is constant time).

I think the generation of Lagrange interpolation and share generation could be improved by using the Number Theoretic Transform instead of computing things using the straight forwards O(n^2) algorithm.
Another thing that could be optimized is to preprocess shares of random polynomial with constant 0.

BGWOutputToAllProtocol:
It seems to be needed that the t+1 shares received from other parties have to be from the first t+1 parties. In general, I think this is sort of a leftover from Spdz protocols where we need all shares. However, ideally this protocol should be able to complete even with missing shares. I think a good way of handling this is generally to store shares in maps, mapping party ID to share, rather than storing the shares in lists. (Although only some subprotocols can be completed with missing shares, multiplication cannot without adding extra spice.)

BGWOutputToAllParty:
This class does not seem to be covered by any test.

BGWTests:
It seems that TestKnown is not validated if computed correctly? I.e. the test does not have any asserts.

I would highly recommend also running tests with other parameters. Ie. EvaluationStrategy.SEQUENTIAL_BATCHED, more than 3 parties, and different bit lengths.

I would also recommend some negative tests. Although the current protocol is only semi-honestly secure, it would still be nice to validate that the protocol gives the expected wrong results in case of cheating. It will also make it easy to port tests to the malicious version when it is written.

For malicious security it is relatively simple, since it is mostly a question of adding verifiable secret sharing instead of regular secret sharing. In this case we can also achieve guaranteed output delivery as long as most n/3 servers are corrupt. See section 5 and 6 in this paper https://eprint.iacr.org/2011/136.pdf

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

No branches or pull requests

1 participant