Implement HSM_CL #1
Conversation
There was a problem hiding this comment.
Nice work Lloyd!
I just left some comments, but I don't have much of an opinion on how to resolve them.
Happy to merge as is and iterate from there or work on this dedicated branch. Either way is fine!
It would be nice if we can resolve the flaky test issue before integrating it.
src/hsm_cl.rs
Outdated
| pub type Proof = CLDLProofPublicSetup; | ||
|
|
||
| lazy_static::lazy_static! { | ||
| pub static ref PUBLIC_SETUP: BigInt = BigInt::from(b"A2LPoC".as_ref()); |
There was a problem hiding this comment.
This lazy-static seems unnecessary to me.
It might actually be a problem because your functions depend on it, yet there are no compile-time errors if you pass a public key to it that was not constructed through KeyPair::gen.
You are using this in the functions encrypt and verify which also take a PublicKey.
I would suggest creating a newtype for their PublicKey (class_group::primitives::cl_dl_lcm::PK) and holding this state in there.
There was a problem hiding this comment.
Side note: lazy_static actually depends on the unmaintained primitive spin: rust-lang-nursery/lazy-static.rs#163
I would suggest using conquer_once if you need lazy computation. I think it is better because it doesn't need macros. In any case, this instance of global state seems to be unnecessary.
There was a problem hiding this comment.
Nod. The solution I went with is just passing it around. The public key shouldn't have state in it so I didn't want to put more stuff in there. So it's just another argument for now.
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct KeyPair(HSMCL); |
There was a problem hiding this comment.
There seems to be no use of the KeyPair itself, you only ever use the public key or the secret key. Do you mind adopting the interface we created where there is a free-function keygen that returns a tuple of two keys?
There was a problem hiding this comment.
I don't see what you mean. When the secret key is used (in decrypt) the public key is needed with it.
| (ciphertext, proof) | ||
| } | ||
|
|
||
| #[must_use] |
There was a problem hiding this comment.
I would suggest to just return a Result, then you get the must_use for free.
There was a problem hiding this comment.
As a consumer of APIs like this I weakly prefer bools over Results with a error newtype struct. If when consuming this you find you'd prefer a result then feel free to change it.
| scalar | ||
| } | ||
|
|
||
| pub fn multiply(ciphertext: &Ciphertext, sk: &secp256k1::SecretKey) -> Ciphertext { |
There was a problem hiding this comment.
We also need the following function:
pub fn multiply(publickey: &secp256k1::PublicKey, sk: &secp256k1::SecretKey) -> secp256k1::PublicKey {See line 22, figure 6, page 9 of the paper.
We thought that multiplication would require an instance of the public key but that doesn't seem to be the case so our (trait-based) model in dummy_hsm_cl is wrong. I think traits are still a good way of solving this though, but knowing that this is a globally available operation, I would probably implement Mul directly.
There was a problem hiding this comment.
I think we discussed this on call. it's just a usual ecmult and doesn't belong with hsm_cl. It is just a globally available operation but they didn't implement Mul for it yet.
| lazy_static = "1.4" | ||
|
|
||
| [dependencies.class_group] | ||
| git = "http://github.com/LLFourn/class" |
There was a problem hiding this comment.
Can we have a tag/commit dependency here please so we don't need to rely on the lockfile (which is not checked it atm).
luckysori
force-pushed
the
master
branch
2 times, most recently
from
8f3c0d7 to
d4e0a4b
Compare
thomaseizinger
force-pushed
the
master
branch
3 times, most recently
from
6097de4 to
add3cc7
Compare
Please review :). API is close to as originally discussed except as free functions (because the field definition is in public key).
The test fails like 1/20 or something. It fails on a sanity check I added which checks that the proof does not work on the ciphertext after it has been multiplied (something badly wrong here). I'll look into it. In general I'm not sure the proof implementation of the underlying library is is sound.