A point $(x,y)$ on Secp256k1 curve is of 64 bytes in its uncompressed form (32 byte for each coordinate).

hash_to_curve.rs#L45-L62 uses 320 bytes for each coordinate. Arkworks uses 40 bytes to store a coordinate.

Recommendation:
Although the test passes, you can use 40 bytes instead of 320:

-let num_field_bytes = 320;
+let num_field_bytes = 40;

Circom and js use sha256.
Rust implementations use sha512.

So these implementations are not compatible.

I suggest passing all these implementations through a common test to ensure consistency.

At ark crate it's used quite suboptimal now, also see branch:43 for a further suggestion.

It might be a RustCrypto thing which just panics on certain methods when bytes yields the value larger than $p$, but it's still a worthy case to have in the suite across implementation to be sure that wrapping is done correctly, and that signatures equality can be facilitated.

Same values are named differently across implementations. While it's not a big deal for small code size, it helps the reader if they can be named same throughout.

For example: h[m,pk]^r is called:

• z in arkworks impl,
• hash_m_pk_pow_r in k-256 impl,
• hashMPkPowR in js and
• h_pow_r in circom

Differing upto casing is fine, but arkworks impl variable name deviates the most from others.

Two possible solutions were mentioned. Both require some experience/research to implement.

Replace Jest with cargo-test for ?

Is there any specific in that test suit which won't work better in Rust? I guess circom crate can be just a dev dependency there (instead of external call), which potentially might enable tests parallelization. Also there's small chance of running it in GitHub action that way (though I guess it would hit some bottleneck along the way).

circom-tester

#24 (comment)

I didn't check, but it seems to me that cases when c is zero, or EC points are at infinity aren't covered by tests, and in couple of recent PRs such tests would be helpful. I guess there also maybe such important inputs which would be good to test.

(Super cool would be to have a file of test vectors.)

https://github.com/plume-sig/zk-nullifier-sig/blob/main/rust-k256/src/lib.rs#L35 for easy readability.

          I totally trust you just trying to understand.

Deterministic as "to know c beforehand of signing"; or is it just the fact that V2 wouldn't work since it lacks the point $r$ value? Or something else?

I mean I kind of don't see yet the essential difference if signer adds to the message point $r$ instead of the info/data you mentioned. 🤔

Originally posted by @skaunov in #98 (comment)

Context: verify_nullifier.circom#L166

Description: The following comment is incorrect as what's being calculated is a * ((b^c)^-1)

// Calculates a^s * (b^c)-1

Recommendation: Correct the comment.

fn encode_pt(point: ProjectivePoint) -> Result<Vec<u8>, Error> always returns Ok(..) currently. Should be analyzed to either introduce error handling or made it infallible.

Move circom-helper to devDependencies in https://github.com/geometryresearch/secp256k1_hash_to_curve/blob/main/circuits/package.json

From #29

EDIT: Blocked on approval in geometry research repo.

https://github.com/plume-sig/zk-nullifier-sig/blob/main/javascript/src/signals.ts#L10

it's not used.

I am recently reading into the PLUME design and the use of Geometry's map. Note that Geometry's map was designed for a different purpose. If we target at making a nullifier, there is at least one trick that we can use.

Note that for provable indifferentiability, Geometry's map does hashing to curves twice and adds the results together. This is not necessary if we use the signature as the nullifier because you don't need to reveal the signature (see #11, I am referring to the GDH undeniable signature, the signature part), and to hide the signature, just hash the final result.

This ideally should be able to reduce the cost by half (given that clearing the cofactor is not necessary for secp256k1).

Circom 2.1 allows using <== operator for signal arrays (documentation). You can avoid writing loops just to assign inputs to a component.

For example:

-for (var i = 0; i < k; i++) {
-    g_pow_s.privkey[i] <== s[i];
-}
+g_pow_s.privkey <== s

https://github.com/plume-sig/zk-nullifier-sig/blob/main/rust-k256/src/lib.rs#L429-L435

encode_pt() is described as:

/// Format a ProjectivePoint to 64 bytes - the concatenation of the x and y values.  We use 64
/// bytes instead of SEC1 encoding as our arkworks secp256k1 implementation doesn't support SEC1
/// encoding yet.

However, it converts a ProjectivePoint to its compressed form of 33 bytes.

It's kind of #72 (comment), but letting moving forward meanwhile.

I added a test which should be showing that hashing to curve isn't correct (at least on signing; before #84 ). As there's many conversions and different formats/endings the test isn't super readable, so it's quite probable I just messed up along the way. This is quite easy to check if anybody would add a hash_to_curve test which a current implementation would pass.

Anyway this/such test is a good contribution to the code. Mine was born when my implementation yielded a signature (V1) different from the current one using the same inputs which correctness I double-checked. (Yep, it's quite probable I messed up implementation and that test; it's enough to have just two errors for this. On the other hand it's god enough signal to dig into this.)

Tests in rust_k256 use the same function while producing test data as the function they test. So actually they test wrapping (which is useful too), but c_sha256_vec_signal is left untested in fact.

Currently, running serially tends to fail. Run all the tests in parallel instead.

Both crates basically lacks documentation, and <./rust-k256> lacks API choices.

Also their name are ...temporary.

PS It would be nice to think about traits used and reexported so that it would not clash with versions that downstream use. Though most likely Cargo will do its job on it, just it's better to be sure.
This one is more about releasing the crates when things will come to it.

Current used DST is taken from the example, so it will collide with other projects which didn't introduce their own DST, which defies its purpose. I see it as quite a priority since if it won't be done until people start to use the thing it will be quite painful to change as it's not backward compatible in no way.

On the other hand there's quite an amount of tests alignment and small modifications due to change of the DST to proper one. Though not a horrific amount.

So the choice is better be done once and never changed, so it worth to put some effort in it. While it should be done in reasonable time. Obvious step is to start the string with "PLUME", and maybe it would be enough to be aligned with usual string, maybe some more information is nice to put there. Too long string bring small burden, btw.

PS Sorry for brevity, am writing in hurry; hope to edit for better readability, or clarify anything in discussion.

perhaps non trivial 😜

I'm not sure if this is the right repo for this issue, but in the demo app (https://ethbogota-2022.netlify.app/) I see the following error when trying to connect Metamask Flask.

"The method "wallet_enable" does not exist / is not available."

The crate is quite complex and I fail to see any reason behind it. It offers a complex trait, but its type system requires further development to be really useful. (And error handling isn't well-compatible with other crates.) It's very generic over curves and its generators, but the only curve other implementations support is deprecated, and I can't find a case when another curve would be plugged into it.

I well might be just fail to see the big idea though. @weijiekoh , could you explain these since I struggled to pick-up the reasons from code but it doesn't add up well in my mind. =(

My current proposal is to ditch the defined trait, simplify few places, and decide on the level of generality. If we don't expect no other curves for PLUME in foreseeable future then further simplify to make it concrete, if do expect then [try] to preserve this part.
[try]: deprecation of the curve definition makes this more a grounding and reference work (than just use) until the whole thing isn't maintained to be compatible with current arkworks version

@Divide-By-0 , how much care should nullifier receive compared with the secret key, and the random nonce (they receive different care, ofc)?

leave a note that secret values handling isn't appropriately implemented there

https://github.com/miracl/core/blob/master/javascript/examples/node/TestHTP.js#L37 for https://github.com/zk-nullifier-sig/zk-nullifier-sig/blob/main/javascript/src/utils/hashToCurve.ts#L33

hashToPairing() function is taken from htp() function in TestHTP.js. Best to add this information in comments. Just makes the life of the reader easier. You can add commit info to the source link.

In rust_k256 crate this fn seems to be fundamental. I'd make it pub, redo some internals (at least better separate computed and input values), and got rid of println! which are suitable for a bin crate, not for a lib.

And that in turn blocks #43 .
See geometryxyz/ark-secp256k1#1 (comment).

verify_nullifier.circom#L33

Prover is free to choose any value forsha256_preimage_bit_length to generate the sha256 hash for c. Since the length of the hash pre-image is fixed, it can be removed as an input signal. The TODO comment indicates this too:

// ...TODO: calculate internally in circom to simplify API
signal input sha256_preimage_bit_length;

Recommendation: sha256_preimage_bit_length should always be equal to message_bits (6*33*8).

As it's known to be leaking some information. At least was using it for the input on verification.

There's substantial number of different improvements since "0.3.0" which is used now. I guess it'd not only generally improve execution, but also might enable simplification of some current helpers introduced in the code.

It's yet named SecretKey, oncoming PR renames it to not give false impression of security.

here arkworks should be checked for proper methods for keys handling
at least zeroize should be applied
also would be nice to check that proper RNG is enforced

If c overflows the Curve order, the value returned by Rust impls differ from js and circom versions.

Rust impls return c % Curve.n, js and circom return the plain c.

I noticed that both c are produced with SHA-256 in <./javascript>. Is it ok to that crate to do it with SHA-512?

Context: verify_nullifier.circom#L214-L219, verify_nullifier.circom#L72

Public key is compressed twice which results in more constraints than required.

Recommendation: Use the compressed public key for calculating the hash for c, although it makes the code complicated.

Seems like <./rust-k256> designed only for verification of a signature, as there's no ready outlet to pub which could sign a message. Should it be so, or does it just lacking it?

It's important to have consistency across these impls. A few examples:

• sha256 in js, circom vs sha512 in rust.
• c is returned as a plain number in js, circom vs a field element in rust.
• More issues may be uncovered, like encoding/decoding across different formats.

You should run a differential test on random values for message and private key (maybe pre-generate and freeze them).

This would provide an expected level of interoperability.

Just to add, there is a quick way to prove the PLUME security, in case anyone asks.

This is the GDH undeniable signature with the confirmation protocol replaced with NIZK, and here the NIZK is a classical Chaum-Pedersen protocol.

The GDH undeniable signature can be found here: Tatsuaki Okamoto and David Pointcheval. The gap-problems: A new class of problems for the security of cryptographic schemes. In PKC ’01

This is also discussed in the BLS signature paper. Section 2.2.

Uniqueness comes from hashing to the curve.

Variant should be joint/embedded with struct PlumeSignature (aka Signature) to avoid failing verifications when with a correct signature instance a wrong variant indicator being input.