Impl (Add|Sub)(Assign) for ValueCommitment #1105
Conversation
dconnolly
added
NU-1 Sapling
dconnolly
force-pushed
the
value-commitment-ops
branch
from
f9bd380 to
44594c1
Compare
| ]), | ||
| ); | ||
|
|
||
| let identity = ValueCommitment(jubjub::AffinePoint::identity()); |
There was a problem hiding this comment.
Maybe move the identity to the top to be consistent with the other tests.
| 0x0000_0000_0000_0000, | ||
| 0x0000_0000_0000_0000, | ||
| ]), | ||
| )); |
There was a problem hiding this comment.
Any reason to pick this point or it is just a random point in the curve? Maybe some comment explaining why it was selected if there is any reason.
There was a problem hiding this comment.
This is a known point on the curve, I just needed any known point, this happens to be the generator of the curve because it's hardcoded into jubjub.
There was a problem hiding this comment.
What about making the point a constant outside the individual tests but inside mod tests { so we can add a comment explaining this is a known point that it is also the generator ?
| @@ -200,13 +241,22 @@ impl ValueCommitment { | |||
| /// | |||
| /// https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit | |||
| #[allow(non_snake_case)] | |||
| pub fn new<T>(csprng: &mut T, value: Amount<NonNegative>) -> Self | |||
| pub fn randomized<T>(csprng: &mut T, value: Amount) -> Self | |||
There was a problem hiding this comment.
What is the reason for the new function in the context of this PR ?
There was a problem hiding this comment.
This snuck over into this PR from the WIP over here: #1100
| // TODO: These generator points can be generated once somewhere else to | ||
| // avoid having to recompute them on every new commitment. | ||
| let V = find_group_hash(*b"Zcash_cv", b"v"); |
There was a problem hiding this comment.
Should we make these into a lazy_static?
There was a problem hiding this comment.
Unfortunately lazy_static doesn't quite do what we want:
There was a problem hiding this comment.
Ah, so there's a trick here… you have to Deref the struct created by the lazy_static macro. (Confusingly, the struct has the same name as the variable it replaces, but is its own unique type.)
Sometimes, taking the struct as a reference using &V works. But it seems like V.as_ref() is more reliable:
https://github.com/ZcashFoundation/zebra/pull/1116/files#diff-bc19effbc9086e335d897caab9487b76R58
There was a problem hiding this comment.
But it seems like V.as_ref() is more reliable:
This is because it is using the method resolution rules which are closely interwined with Deref in rust which we could leverage this directly by using V.mul(v) (I thinkkkk)
Co-authored-by: Jane Lusby <jlusby42@gmail.com>
Needed for computing the transaction binding validating key