Add exhaustive test for ECDH module #1852

theStack commented at 4:53 PM on May 2, 2026: contributor

This PR adds an exhaustive test for the ECDH module, looping over all key combinations and verifying the commutativity property (ECDH(i*G, j) == ECDH(j*G, i)) and checking against a recalculated ECDH result (by manually invoking the default ECDH hash function on the precalculated group element group[i * j]'s coordinates). The existing test coverage is already solid (including Wycheproof test vectors), but I figured it likely wouldn't hurt to add this as well.

real-or-random added the label assurance on May 2, 2026

real-or-random added the label feature on May 2, 2026

real-or-random approved

real-or-random commented at 7:56 PM on May 2, 2026: contributor

utACK https://github.com/bitcoin-core/secp256k1/pull/1852/changes/6b9e4b88dc0f76fdeeaa73de7294b0b82e755ced nice!

real-or-random requested review from Copilot on Jun 2, 2026

?

copilot_work_started real-or-random

real-or-random commented at 7:18 AM on June 2, 2026: contributor

This is a test-only PR, so I'd be happy to merge without a further ACK, but let Copilot have another look to make sure we didn't miss anything obvious.

Copilot commented at 7:23 AM on June 2, 2026: none

Pull request overview

This PR adds ECDH coverage to the exhaustive test suite by iterating over all secret/public key pairs in the small exhaustive-test group and checking ECDH commutativity plus an independently recomputed expected result.

Changes:

Adds an ECDH exhaustive test implementation (test_exhaustive_ecdh) under ENABLE_MODULE_ECDH.
Hooks the ECDH exhaustive test into src/tests_exhaustive.c’s include list and main test runner.
Updates the ECDH module automake include to ship the new noinst header.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/tests_exhaustive.c	Includes and runs the new ECDH exhaustive test when the module is enabled.
src/modules/ecdh/tests_exhaustive_impl.h	Implements exhaustive ECDH commutativity + expected-hash verification.
src/modules/ecdh/Makefile.am.include	Adds the new exhaustive test header to `noinst_HEADERS`.

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

Add exhaustive test for ECDH module 5698e66c64

in src/modules/ecdh/tests_exhaustive_impl.h:49 in 6b9e4b88dc outdated

  44 | +
  45 | +                secp256k1_fe_normalize_var(&ecdh_ge_expected.x);
  46 | +                secp256k1_fe_normalize_var(&ecdh_ge_expected.y);
  47 | +                secp256k1_fe_get_b32(x, &ecdh_ge_expected.x);
  48 | +                secp256k1_fe_get_b32(y, &ecdh_ge_expected.y);
  49 | +                CHECK(secp256k1_ecdh_hash_function_default(ecdh_result_expected, x, y, NULL));

Copilot commented at 7:23 AM on June 2, 2026:

The manual recomputation uses secp256k1_ecdh_hash_function_default, which hashes using secp256k1_context_static (see modules/ecdh/main_impl.h). However, secp256k1_ecdh(..., hashfp=NULL, ...) hashes using the context’s hash backend (secp256k1_get_hash_context(ctx)), and the output can differ when the context SHA256 compression function is overridden. To make this exhaustive test match the actual default code path, use the ctx-aware implementation when hashing the expected point.

real-or-random commented at 7:28 AM on June 2, 2026:

the output can differ when the context SHA256 compression function is overridden.

No.

in src/modules/ecdh/tests_exhaustive_impl.h:38 in 6b9e4b88dc outdated

  33 | +            CHECK(secp256k1_ecdh(ctx, ecdh_result_ji, &pubkeys[j - 1], seckeys[i - 1], NULL, NULL));
  34 | +            CHECK(secp256k1_memcmp_var(ecdh_result_ij, ecdh_result_ji, 32) == 0);
  35 | +
  36 | +            /* Recalculate the expected ECDH result manually by invoking the default ECDH hash
  37 | +             * function on the precomputed group element (groups[i * j]) coordinates, and verify
  38 | +             * that it matches the previously calculated public API results. */

Copilot commented at 7:23 AM on June 2, 2026:

Comment refers to groups[i * j], but the variable is group and the code uses a modulus when indexing. Updating the comment to match the actual expression will avoid confusion when reading or modifying this test.

real-or-random commented at 7:30 AM on June 2, 2026:

The % EXHAUSTIVE_TEST_ORDER may be overkill but I agree that with the group typo.

theStack commented at 12:26 PM on June 2, 2026:

Fixed the typo, thanks.

theStack force-pushed on Jun 2, 2026

sipa commented at 2:03 PM on June 2, 2026: contributor

ACK 5698e66c64c6c3b7eef1e17c1fb33b003871eba3

real-or-random approved

real-or-random commented at 11:38 AM on June 7, 2026: contributor

utACK 5698e66c64c6c3b7eef1e17c1fb33b003871eba3

real-or-random merged this on Jun 7, 2026

real-or-random closed this on Jun 7, 2026

theStack deleted the branch on Jun 7, 2026