-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: range proof sum optimization and cleanup #101
feat: range proof sum optimization and cleanup #101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Nice, the addition explains a really nice maths trick and makes the implementation more readily understandable. I have some comments below to make the explanation more palatable.
- I think we need to retain the previous
## Comparative performance
section but remove all the tables and discuss relative performance in general terms.
What kind of comparative performance would you be looking for here? And should updated performance data be a separate PR? I removed it here only because it was very out of date. It's not clear to me if an RFC is the right place for that. In my opinion, if the comparative performance is for functions within this repository, those should be included in benchmarks that the user can run themselves to get accurate absolute numbers for their system. Comparative performance between protocols using operation counts is fine, since those typically scale independently of the target system. Comparative performance between implementations of different protocols seems sketchier to me, since different libraries or algorithmic optimizations might not make it a fair fight. That being said, I think limited informal comparisons to give the user a broad overview of implementations can be fine if presented as such (this is done in the Bulletproofs+ documentation to a give a very rough comparison to the |
I think we could include what the maths predict, for example,
|
Should that go into the RFC, or into the repository documentation? The RFC's "goals" section states the current intention is to describe Tari-specific implementation details. |
I would change the goal to accommodate discussing |
Does this commit Bulletproofs+ repository contributors also to keep RFC benchmarks up to date? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merging to assist auditors |
Description --- Updates range proof [RFC-0181](https://rfc.tari.com/RFC-0181_BulletproofsPlus.html) to add comparative performance data and improve notation. Closes #102. Motivation and Context --- An [earlier PR](#101) removed outdated benchmark data, but it was [suggested](#101 (comment)) to include reasonable comparative performance data against Bulletproofs. This data has been added for both proof size and verification complexity. Some notation was incomplete or incorrect. In particular, sum and index bounds were left implicit in many places, and indexing relating to mask recovery was incorrect. Both of these have been addressed. How Has This Been Tested? --- The changes build and appear to render correctly.
Description
Adds details of a Bulletproofs+ optimization to RFC-0181. Removes old benchmark data.
Motivation and Context
The Bulletproofs+ implementation contains a particular optimization used when computing a vector sum. This optimization is not particularly obvious, as it uses some fun arithmetic and a fact about geometric series partial sums.
This PR adds details about the math behind the optimization to RFC-0181. It also removes outdated benchmark data, similarly to earlier removal in the Bulletproofs+ repository.
How Has This Been Tested?
The changes build and render properly. The underlying mathematics should be manually checked for correctness.