arkworks-rs / crypto-primitives

Interfaces and implementations of cryptographic primitives, along with R1CS constraints for them
https://www.arkworks.rs
Apache License 2.0
165 stars 79 forks source link

Pedersen hash test fails for MNT6-753 curve #109

Closed paberr closed 1 year ago

paberr commented 1 year ago

I noticed that the pedersen hash native equality test fails on the MNT6-753 curve. So far, I have not investigated it any further.

Here is a straightforward copy of the original test within the crate, just adapted to use the MNT6-753 curve. It fails due to the result not being equal:

thread 'test_native_equality' panicked at 'assertion failed: `(left == right)`
  left: `(30390519998062178694725475488940532302589938923242267558107260219929746235682202286671549394454858372824954717174269307126111696685337959911401422533740374719533611898999068655889367618546677228247302093533059382636731943987153, 31577653328290403174636224464923809536312721930568898260924681142803173090303008532122247136406726160984825509478364846586928834029472596529985379920851904757723573215025326910498114850509986137058967739174592705707005691872426)`,
 right: `(28859985354968639743946055458264681860931873493266425400581389206408943549804757046100063253039557629051321858869964295459866261787662758167419177045205076114605735178873939272389854650616996123629301617779573535732334814960566, 24451171218383564241303970355918892269988964476566513651408375954536170738339336517864976400937097228888940980774708934032138427601324417814051388979167673529040688562864423189542386386911374815819363810050003455919964690178629, 1)`', tests/test.rs:52:5

I compared the result of the off-circuit hash to our reference implementation and it seems correct. Thus, the problem must occur in the ZKP version of the hash.

paberr commented 1 year ago

Further investigating this, we found the root cause of the error.

The precomputed_base_multiscalar_mul_le function used looks as follows:

        for (bits, bases) in scalars.zip(bases) {
            let bases = bases.borrow();
            let bits = bits.to_bits_le()?;
            result.precomputed_base_scalar_mul_le(bits.iter().zip(bases))?;
        }

It seems to expect precomputed_base_scalar_mul_le to add the base scalar multiplication to the original value of result. However, both the generic implementation and the short weierstrass implementation simply calculate the base scalar mult. and set *self = result;. So, the variable is overwritten in every iteration of the loop.

So, the question is: what's the expected behaviour of precomputed_base_scalar_mul_le? Should it add to self? Or should precomputed_base_multiscalar_mul_le instead take care of the addition? EDIT: It seems like it is expected to add to self as done in the twisted edwards implementation.

I've tested and confirmed that this is the issue.