Fix commitment tests

[jaspervdm]

It turned out that the Pedersen commitment tests were disabled. I re-enabled them in https://github.com/mimblewimble/secp256k1-zkp/pull/34 but some of the tests are broken.

[garyyu]

That's weird, or perhaps it's never enabled:) I would propose to give a fix on another PR, and don't mix it with the important #34 . I will take a look now.

[jaspervdm]

Yes, for now I commented the broken tests out in #34 but we should provide a separate PR that fixes and re-enables them

[garyyu]

The root cause is: The implementation of secp256k1_pedersen_commit never use its 1st parameter const secp256k1_context* ctx, but the tests would like to see the different behavior for different ctx:

    CHECK(secp256k1_pedersen_commit(none, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) == 0);
    CHECK(ecount == 1);
    CHECK(secp256k1_pedersen_commit(vrfy, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) == 0);
    CHECK(ecount == 2);
    CHECK(secp256k1_pedersen_commit(sign, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) != 0);
    CHECK(ecount == 2);

Actually, all above 3 functions call will return 1 (i.e. success!), no matter what ctx is passed.

And another point, I'm wondering why we don't use this secp256k1_context* ctx to achieve an optimized ecmult (i.e. secp256k1_ecmult_gen), since we always use the GENERATOR_G for our pedersen commitment. The current implementation use a generic ecmult to be compatible with any generator.

secp256k1_ecmult_const(&bj, blind_gen, sec, 256);

Since this is the upstream bug (on test_commitment_api), I gave the comments on ElementsProject/secp256k1-zkp#23, and hope it's fixed on there, and then we just merge it.

So, no need a PR here in this repo. But we can keep this issue open until we saw it is fixed on upstream.

And one important concern here: the current Travis-CI test script doesn't have the test step (i.e. run ./test), that's a major risk on our secp256k1-zkp lib. We'd better take a test to enable it and ensure each test has been passed for any future PR. It deserves a new issue and a dedicated PR for that.

[garyyu]

BTW, if you really want a fix here, then you can modify it as following:

in commitment/tests_impl.h:

static void test_commitment_api(void) {
...
    int32_t ecount = 2;
    ...
    secp256k1_rand256(blind);
    CHECK(secp256k1_pedersen_commit(none, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) != 0);
    CHECK(ecount == 2);
    CHECK(secp256k1_pedersen_commit(vrfy, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) != 0);
    CHECK(ecount == 2);
    CHECK(secp256k1_pedersen_commit(sign, &commit, blind, val, &secp256k1_generator_const_h, &secp256k1_generator_const_g) != 0);
    CHECK(ecount == 2);

Then the tests should be OK.

[jaspervdm]

Thanks for investigating. Also the test_multiple_generators test is failing, namely at secp256k1_pedersen_verify_tally

[garyyu]

Fixed in https://github.com/mimblewimble/secp256k1-zkp/pull/44