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

+-x into 2x #6

Merged
merged 14 commits into from
Jan 27, 2023
Merged

+-x into 2x #6

merged 14 commits into from
Jan 27, 2023

Conversation

maurges
Copy link
Contributor

@maurges maurges commented Dec 20, 2022

In the initial implementation I forgot that the notation for y <- +-a means that y <- [0; 2a]. This patch changes the generation functions to account for that

While making this change, I also refactored tests and added a new category of tests: check that the probablity to reject a correct proof is similar to the one in paper. This helps to show that the change above didn't break security (at least completely)

src/group_element_vs_paillier_encryption_in_range.rs Outdated Show resolved Hide resolved
BigNumber::from_slice(x.to_be_bytes().as_bytes())
// double the range to account for +-
let m = BigNumber::from(2) * &security.q;
BigNumber::from_rng(&m, rng)
Copy link
Contributor

Choose a reason for hiding this comment

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

Openssl (and maybe some other) backends of unknown_order ignore provided source of randomness. I just can imagine someone choosing openssl backend and, as a result, getting unintuitive errors and unexpected behavior.

Can we detect somehow which backend is chosen and produce compiler_error! (if it's possible to detect on compile-time) or panic! (if we can determine backend only in runtime)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's easier to forbid openssl as a feature, since a backend is explicitly selected with this crate. I'll disable it in the next commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

You can do that, but one can bypass that by setting pailler-zk.deafult-features = false, and turning openssl feature on directly on unknown-order. If we can't check which backend is used, we should at least add a note, that this backend is not supported

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have any root-level docs or readme where we could mention that, I'm fine if you open an issue to remember mention it in the docs in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. Let's make this and the discussion below into a separate documentation MR

src/group_element_vs_paillier_encryption_in_range.rs Outdated Show resolved Hide resolved
src/paillier_encryption_in_range.rs Show resolved Hide resolved
@survived survived changed the title Draft: +-x into 2x +-x into 2x Jan 26, 2023
@survived
Copy link
Contributor

I see you added a lot of docs. Is it a right time to add #[forbid(missing_docs)] to the top of lib.rs or not yet?

Otherwise the performance is cripplingly bad
src/lib.rs Outdated Show resolved Hide resolved
@maurges
Copy link
Contributor Author

maurges commented Jan 26, 2023

Found another case of non-determinism that slipped me by, and fixed it. Should be completely ready now

@maurges
Copy link
Contributor Author

maurges commented Jan 26, 2023

Oh god, and now CI fails with that exact test, what happened

@maurges
Copy link
Contributor Author

maurges commented Jan 26, 2023

I'm not sure what happened there with CI. I debugged it locally, and the proofs are computed deterministically

@survived
Copy link
Contributor

I recommend you to use rand_dev in tests rather than OsRng. It prints seed to stdout when constructed, so you can locally reproduce whatever issue you have on CI (assuming tests are 100% deterministic with fixed randomness source)

@maurges
Copy link
Contributor Author

maurges commented Jan 26, 2023

Owners: survived

=)) Good crate though, will use it

The problem with determinism here is that it's easy to accidentally use functions from unknown_order that use OsRng implicitly.

@survived
Copy link
Contributor

it's easy to accidentally use functions from unknown_order that use OsRng

Should we then add a script that greps BigNumber::prime and others bad functions and fails CI if it found any of them in the project?

@survived
Copy link
Contributor

So still unresolved things in this PR

  • Document crate, enforce #![forbid(missing_docs)]
    • Add a warning that openssl backend of unknown order is not supported
  • Deterministic reproducible tests (i.e. rand_dev)
    • Detect invocations of functions like BigNumber::prime in CI and recommend using their appropriate alternatives like BigNumber::prime_from_rng

If you don't want to resolve any of them within this PR, please just open an issue so we could keep track of them.

@maurges
Copy link
Contributor Author

maurges commented Jan 27, 2023

I think all those 4 changes are out of scope of this MR at least, yes. Let me make the issues for them

@maurges
Copy link
Contributor Author

maurges commented Jan 27, 2023

See #9, #10, #11

@survived
Copy link
Contributor

Great, thanks!

@maurges maurges merged commit f41a525 into m Jan 27, 2023
@maurges maurges deleted the d branch May 4, 2023 16:42
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.

2 participants