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

Ferveo Variant docs - also removal of existing stale ferveo docs. #143

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre commented Jul 18, 2023

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Refactor
  • Other

Required reviews:

How many reviews does the PR author need?

  • 1
  • 2
  • 3

What this does:

  • Adds documentation for Ferveo variants and their characteristics
  • Re-use pre-existing ferveo docs set up, but also remove stale data that is no longer applicable based on our own changes.

Issues fixed/closed:
Fixes #140 .

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

I tried to keep some of the more relevant existing Ferveo docs. I have no problems with just deleting them if they are just no longer applicable.

Let me know what you think.

…longer applicable based on changes we made to the logic.
@derekpierre derekpierre changed the title [WIP] Ferveo Variant docs - also removal of existing stale ferveo docs. Ferveo Variant docs - also removal of existing stale ferveo docs. Jul 19, 2023
@derekpierre derekpierre marked this pull request as ready for review July 19, 2023 16:21
`n - m + 1` nodes would have to not respond for the failure case.

### Disadvantages
- Since the `m` nodes are arbitrary, combining the decryption shares requires the requester to do a more computationally intensive operation.

Choose a reason for hiding this comment

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

Should we mention the performance overhead in concrete numbers and/or link to benchmarks?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there is something to link to - yep, no problem, just point me to it.

Choose a reason for hiding this comment

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

In the PRs description: #32

Copy link
Member Author

Choose a reason for hiding this comment

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

Those look like scripts to do the benchmarking, is there an output file containing the benchmarks somewhere or no? If not, perhaps you can add instructions on running those scripts to the docs in a separate PR.

Choose a reason for hiding this comment

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

This is the output of cargo bench and these files are generated locally. We could add a section dedicated to benchmarking with these instructions and other information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that in a different PR.

Copy link

@piotr-roslaniec piotr-roslaniec left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

I think it's sufficiently descriptive at this time. Thank you for putting this to paper.

@derekpierre
Copy link
Member Author

Side note: there is a mention of simple and fast here - https://github.com/derekpierre/nucypher-ferveo/blob/variant-docs/book/src/tpke.md#threshold-decryption-fast-method - are these the same concepts or something separate?

@piotr-roslaniec
Copy link

@derekpierre Yes, we use the original simple variant. fast is also implemented (still present in the codebase), but remains unused.

@derekpierre derekpierre merged commit 12c8b5c into nucypher:main Jul 21, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Document simple and precomputed variants and their benefits/drawbacks in ferveo library documentation
3 participants