Adding centralised alba (bounded scheme)

[rrtoledo]

Content

Adding Bounded scheme (section 3.2.2 ) implementation in Rust with randomized tests and benches.

This PR includes...

Pre-submit checklist

• Branch
  • Tests are provided (if possible)
  • Commit sequence broadly makes sense
  • Key commits have useful messages
• PR
  • No clippy warnings in the CI
  • Self-reviewed the diff
  • Useful pull request description
  • Reviewer requested
• Documentation
  • Update README file (if relevant)
  • Update documentation website (if relevant)

Comments

Repetition bench doesn't work so is disabled

Issue(s)

Issue #25

[curiecrypt]

First round of the review. I will do one more time for benches.

[tolikzinovyev]

I cannot comment on the lines that weren't changed (the gray lines). :( I really think we should remove all the code from the main branch and then review. Otherwise, reviewing is much harder.

[tolikzinovyev]

This is a nice first implementation, Raphael. I've looked over centralized.rs and I wonder if we should modify the approach a bit. If this library intends to eventually become production ready with high guarantees of security (and I really hope it does), it might make sense to make the first PR a very small simple implementation to make sure everyone can review it and ascertain correctness with ease. We should prioritize code readability and good interfaces and leave optimizations such as parallelization and reusing H_1 hashes for future, also short, PRs. In other words, making small steps that are easy to verify.

I admit that I'm generally very pedantic about code, but given that it's a cryptographic library, it makes sense to stay on the safe side. This doesn't mean, however, that we can't move fast, and I'm also happy to volunteer a tiny first implementation, if you allow me.

[rrtoledo]

I cannot comment on the lines that weren't changed (the gray lines). :( I really think we should remove all the code from the main branch and then review. Otherwise, reviewing is much harder.

@tolikzinovyev all of the old rust code has been removed in this PR. If you are experiencing difficulties reviewing the commits that have been added, it is part of the reviewing process to say why so that we can go through it together and improve in the future.

As for the untouched code, you can still select lines and comment on them when looking at the files though the "commits" or "file changed" tabs here. Were you not to be able to do so, you can still go through the code and write your points in a general comment here or on a paper and we would then be able to look at them together.

[tolikzinovyev]

I cannot comment on the lines that weren't changed (the gray lines). :( I really think we should remove all the code from the main branch and then review. Otherwise, reviewing is much harder.

@tolikzinovyev all of the old rust code has been removed in this PR. If you are experiencing difficulties reviewing the commits that have been added, it is part of the reviewing process to say why so that we can go through it together and improve in the future.

As for the untouched code, you can still select lines and comment on them when looking at the files though the "commits" or "file changed" tabs here. Were you not to be able to do so, you can still go through the code and write your points in a general comment here or on a paper and we would then be able to look at them together.

Never mind, I just checked that I can leave comments on unchanged lines. It's weird that I couldn't before.

[rrtoledo]

This is a nice first implementation, Raphael. I've looked over centralized.rs and I wonder if we should modify the approach a bit. If this library intends to eventually become production ready with high guarantees of security (and I really hope it does), it might make sense to make the first PR a very small simple implementation to make sure everyone can review it and ascertain correctness with ease. We should prioritize code readability and good interfaces and leave optimizations such as parallelization and reusing H_1 hashes for future, also short, PRs. In other words, making small steps that are easy to verify.

I admit that I'm generally very pedantic about code, but given that it's a cryptographic library, it makes sense to stay on the safe side. This doesn't mean, however, that we can't move fast, and I'm also happy to volunteer a tiny first implementation, if you allow me.

@tolikzinovyev thanks for looking at the implementation of the centralised scheme.

We will go through many iterations of the code indeed and having small PRs is easier to manage these, I agree, but we have to start from somewhere and do not want to re-invent the wheel. We have code, we should reuse it, review it and update it.
Alba centralised scheme is not a complexe nor big, we want here to assert that these 400 lines of code follow the paper's pseudocode, and check that the tests and benchmarks make sense. This should be quite straightforward. Further testing, including property testing, and optimization are taking into account in our roadmap and shall be dealt with other issues and PRs.

Please write down your issues on specific lines so that we can discuss each point separately. As for parallelization, I will remove it as indeed it is a bit early to have it here. There was anyway another PR schedule to deal with the limitation of the current one and I would be more than happy to work with you then on it as we already discussed on how to imrpove it. I am not sure what you are talking about "reusing H1", please give more details on the corresponding lines.

This is a cryptographic library and I agree we have to be careful about what we publish. We are however using versioning, and are not currently releasing version 1.0 of Alba. We can hence afford to go step by step and, to begin with, merge code that corresponds to the paper's pseudocode and again improve it later on. We work by proposing code, reviewing it and refining it.

We need to move forward, and cannot afford to have everyone work on their own implementation, however small it may be. Design choices need to be made, they can and should of course be discussed, but when working in a team we may not all agree and need to compromise. This is what the reviewing process is for.
I encourage you to write here how you would have done things differently, why and what benefits these changes would bring us. We can also go though the code together and have this discussion in a call as these tend to be more efficient.

[tolikzinovyev]

By reusing h1, I mean the way h1 hashes are computed. In the paper h1 has long inputs: h1(v, t, s1, ..., s_k), but you update h1 recursively: h2(h1(h1(v, t), s1), s2). This is the reason why your implementation doesn't correspond the paper's specification. This is indeed an important optimization, I'm just suggesting we do it in a separate PR after we are confident about the most basic implementation.

By the way, we never wrote a proof that this optimization works and there is no specification. This is a todo item on the research side.

[tolikzinovyev]

Found those gray lines. I cannot leave comments on lines 17-24.

[tolikzinovyev]

First set of comments. I'm going to review the math in parameter derivation next.

[tolikzinovyev]

Finished reviewing the math in parameter derivation expect for factorial_check(). Things mostly look good, left some comments.

[tolikzinovyev]

I think limit and sec_param should be renamed now since they are very confusing and it's no longer just a matter of style / preference, but I'm ok to leave it for #38 as long as it is addressed soon.

[curiecrypt]

LGTM 👍

[djetchev]

Great job, @rrtoledo ! I know it was a lot of work and I am sure we will further improve it but already quite happy with it.

LGTM!