ecdh: Add test computing shared_secret=basepoint with random inputs #1026

I was playing around with the ECDH code and wrote this test on way.

ACK 18fe75b19e5cf5e1d4c6192e09a0d6346566b098

Agreed that we should get ecdh(g^s, s_inv) == ecdh(g^s_inv, s) == ecdh(g, 1) for any s, s_inv pair.

Non-blocking suggestion, would the intent be more clear if you explicitly calculate and compare against the expected constant value ecdh(g, 1), instead of comparing out against the previous iteration's out_eq?

void test_result_basepoint(void) {
    secp256k1_pubkey point;
    secp256k1_scalar rand;
    unsigned char s[32];
    unsigned char s_inv[32];
    unsigned char out[32];
    unsigned char out_inv[32];
    unsigned char out_base[32];
    int i;

    unsigned char s_one[32] = { 0 };
    s_one[31] = 1;
    CHECK(secp256k1_ec_pubkey_create(ctx, &point, s_one) == 1);
    CHECK(secp256k1_ecdh(ctx, out_base, &point, s_one, NULL, NULL) == 1);

    for (i = 0; i < 64; i++) {
        random_scalar_order(&rand);
        secp256k1_scalar_get_b32(s, &rand);
        secp256k1_scalar_inverse(&rand, &rand);
        secp256k1_scalar_get_b32(s_inv, &rand);

        CHECK(secp256k1_ec_pubkey_create(ctx, &point, s) == 1);
        CHECK(secp256k1_ecdh(ctx, out, &point, s_inv, NULL, NULL) == 1);
        CHECK(secp256k1_memcmp_var(out, out_base, 32) == 0);

        CHECK(secp256k1_ec_pubkey_create(ctx, &point, s_inv) == 1);
        CHECK(secp256k1_ecdh(ctx, out_inv, &point, s, NULL, NULL) == 1);
        CHECK(secp256k1_memcmp_var(out_inv, out_base, 32) == 0);
    }
}

Non-blocking suggestion, would the intent be more clear if you explicitly calculate and compare against the expected constant value ecdh(g, 1), instead of comparing out against the previous iteration's out_eq?

Probably yes. I'll change this.

@uvhw @bitchaincoin @totongcute

Forcedpushed to rebase and add you as a coauthor.

ACK 6bc47a170319efb7bc6b07ce1eeba3dbbb5e6ca9

Test passes locally, first commit is directly from #1026#pullrequestreview-824152063 (only with iteration count changed).

Thanks for adding me, gotta pump those commit numbers up :)

Thanks for adding me, gotta pump those commit numbers up :)

I think it's good and right to acknowledge people. :)

But here's an off-topic rant about code stats, which I think are always misleading:

• Commit scope is very different among contributors and stages of a project.
• Line numbers are a stupid metric, too. Wanna have ten thousand lines? Change the format of the precomputed tables.
• Commits and line numbers say nothing about the time invested in a change or its value (whatever that even is).
• Often less is more. Except for docs where more is often better.
• Common stats ignore code review (which is by far the largest chunk of time spent here) and conceptual contributions.

So I think it's better for our wellbeing to ignore the numbers... At least that's why I decided to do.

Hopefully it's clear I was joking about pumping those rookie numbers up. I strongly agree with all of that! Goodhart's Law and etc. Especially about less is more:

If we wish to count lines of code, we should not regard them as "lines produced" but as "lines spent": the current conventional wisdom is so foolish as to book that count on the wrong side of the ledger. --Dijkstra

Hopefully it's clear I was joking about pumping those rookie numbers up.

Yes, I assumed you were joking. :) (Sorry if my post looked like the opposite... I was in the mood to write that rant down.)

Concept ACK

Would be helpful to add a comment describing what the test does, similar to what @robot-dreams says above:

[...] ecdh(g^s, s_inv) == ecdh(g^s_inv, s) == ecdh(g, 1) for any s, s_inv pair.

@jonasnick Indeed! Fixed.

ACK 3531a43b5bc739838f5634afcfd02bdbef71b1ef