Fix tests failing in release build by mrtcnk · Pull Request #11 · XRPLF/mpt-crypto

mrtcnk · 2026-02-10T02:19:15Z

Tests: Replaced assert() in all test files with a custom persistent EXPECT() macro. This guarantees that cryptographic setup logic executes correctly in both Debug and Release modes.

CMake: Moved architecture detection into CMakeLists.txt using CheckTypeSize. It now automatically detects 128-bit support and applies the optimized 64-bit flags (USE_SCALAR_4X64) globally.

Cleanup: Removed manual #define blocks from src/mpt_scalar.c to rely entirely on CMake, preventing "macro redefinition" warnings.

Verification I have verified that all tests now pass successfully in both Debug and Release modes on macOS.

…d improve CMake 64-bit detection

tests/test_bulletproof.c

tests/test_bulletproof_agg.c

tests/test_commitments.c

CMakeLists.txt

src/mpt_scalar.c

mathbunnyru · 2026-02-10T13:23:44Z

Also, the PR title doesn't represent the change, since it's not Conan-related at all, but tests failing in Release builds.
I updated the title

Copilot

Pull request overview

This PR aims to make the test suite reliable in Release builds (where assert() may be compiled out) and to centralize scalar/field backend selection in CMake to avoid macro redefinition warnings and mismatched build settings.

Changes:

Replaced assert() usage across tests with persistent EXPECT(...)-style macros that abort/exit on failure.
Moved scalar/field backend selection into top-level CMakeLists.txt and removed backend #define blocks from src/mpt_scalar.c.
Updated/cleaned several tests (including allocation strategies and proof generation/verification flows), and removed the old tests/test_bulletproof.c.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`CMakeLists.txt`	Adds global architecture/int128 detection and sets scalar/field backend compile definitions.
`src/mpt_scalar.c`	Removes manual backend `#define`s to rely on CMake-provided compile definitions.
`tests/test_same_plaintext_multi_shared_r.c`	Refactors shared-r equality test to use `EXPECT`, changes RNG seeding, and simplifies verification flow.
`tests/test_same_plaintext_multi.c`	Replaces `assert` with `EXPECT`, switches from VLAs to heap allocations for broader compiler compatibility.
`tests/test_same_plaintext.c`	Replaces `assert` with `EXPECT` and adds extra debugging helper (currently unused).
`tests/test_pok_sk.c`	Replaces `assert` with `EXPECT` and adds safer scalar generation helper.
`tests/test_link_proof.c`	Replaces `assert` with `EXPECT` and adds safer scalar generation helper.
`tests/test_ipa.c`	Replaces `assert` with `EXPECT` and hardens RNG checks.
`tests/test_equality_proof.c`	Replaces `assert` with `EXPECT` and consolidates test runner setup.
`tests/test_elgamal_verify.c`	Replaces `assert` with `EXPECT`; uses keypair-gen to obtain valid scalar for randomness.
`tests/test_elgamal.c`	Replaces `assert` with `EXPECT` and adds end-of-run success message.
`tests/test_commitments.c`	Replaces `assert` with `EXPECT` and hardens RNG failure handling.
`tests/test_bulletproof_agg.c`	Replaces `assert` with `EXPECT` and removes local prototypes in favor of header declarations.
`tests/test_bulletproof.c`	Removed.
`.gitignore`	Ignores IntelliJ and Python venv directories.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T13:32:24Z

CMakeLists.txt

+if (HAVE_INT128_T OR CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64"
+    OR CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
+    message(STATUS "Build: Detected 64-bit system. Enforcing 64-bit arithmetic globally.")
+    add_compile_definitions(USE_SCALAR_4X64 USE_FIELD_5X52 HAVE___INT128)
+else ()
+    message(STATUS "Build: Detected 32-bit system. Enforcing generic arithmetic globally.")


The architecture detection enables USE_SCALAR_4X64 and defines HAVE___INT128 whenever the CPU looks 64-bit, even if CheckTypeSize("unsigned __int128") fails. On toolchains without unsigned __int128 (e.g., MSVC x64), this can force the 4x64 backend and HAVE___INT128, likely breaking compilation or producing incorrect scalar math. Suggest gating USE_SCALAR_4X64/HAVE___INT128 strictly on HAVE_INT128_T (and otherwise fall back to USE_SCALAR_8X32), or adding a separate, verified MSVC-compatible 64-bit path.

Suggested change

if (HAVE_INT128_T OR CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64"

OR CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")

message(STATUS "Build: Detected 64-bit system. Enforcing 64-bit arithmetic globally.")

add_compile_definitions(USE_SCALAR_4X64 USE_FIELD_5X52 HAVE___INT128)

else ()

message(STATUS "Build: Detected 32-bit system. Enforcing generic arithmetic globally.")

if (HAVE_INT128_T)

message(STATUS "Build: Detected compiler support for unsigned __int128. Enforcing 64-bit arithmetic backend.")

add_compile_definitions(USE_SCALAR_4X64 USE_FIELD_5X52 HAVE___INT128)

else ()

message(STATUS "Build: No unsigned __int128 support detected. Using generic 32-bit arithmetic backend.")

I've updated CMakeLists.txt to use the definitions that actually exist in the source.

Copilot · 2026-02-10T13:32:24Z

tests/test_same_plaintext_multi_shared_r.c

    unsigned char seed[32];
-    if (RAND_bytes(seed, 32) == 1) {
-        secp256k1_context_randomize(ctx, seed);
-    }
+    FILE *fr = fopen("/dev/urandom", "r");
+    EXPECT(fr != NULL, "Failed to open /dev/urandom");
+    EXPECT(fread(seed, 1, 32, fr) == 32, "Failed to read random seed");
+    fclose(fr);


This test seeds randomness by reading from "/dev/urandom". That’s non-portable (fails on Windows and some restricted environments) and duplicates the project’s existing OpenSSL-based RNG usage elsewhere in the test suite. Prefer using RAND_bytes() (or a shared helper) for the seed/context bytes so tests remain cross-platform.

Fixed. I removed the non-portable /dev/urandom code and replaced it with the random_bytes() helper (wrapping RAND_bytes) to ensure cross-platform compatibility.

Copilot · 2026-02-10T13:32:24Z

tests/test_same_plaintext_multi_shared_r.c

+    // 4. Verify Proof
    res = secp256k1_mpt_verify_equality_shared_r(
            ctx, proof,
            N_RECIPIENTS,
-            &C1, C2_tampered, Pk, tx_id
+            &C1,
+            C2s,            // Pass the array directly
+            pks,            // Pass the array directly
+            tx_context
    );
-    assert(res == 0);
-    printf("Tamper detection: OK.\n");

-    /* 8. Negative Test: Tamper with Transaction Context */
-    printf("Verifying proof with wrong TxID (Expecting Failure)...\n");
-    unsigned char tx_id_fake[32];
-    memcpy(tx_id_fake, tx_id, 32);
-    tx_id_fake[0] ^= 0xFF; // Flip bits
+    EXPECT(res == 1, "Proof Verification Function returned failure");
+    printf("Proof verified successfully.\n");


The negative test coverage was removed: previously this test verified that tampering with ciphertexts and with the transaction context causes verification to fail. With only the positive-path verification remaining, regressions in binding / malleability checks may go undetected. Consider reintroducing at least one ciphertext-tamper case and one context-tamper case with EXPECT(res == 0).

Fixed. I have reintroduced negative test cases.

Copilot · 2026-02-10T13:32:25Z

tests/test_same_plaintext_multi_shared_r.c

-#include <openssl/rand.h>
-#include <stdlib.h> // Added for malloc/free
-
+#include <time.h>


#include <time.h> appears unused in this test after the refactor. If the project builds with warnings enabled, this can introduce noise; consider removing unused includes.

Suggested change

#include <time.h>

Copilot · 2026-02-10T13:32:25Z

tests/test_same_plaintext.c

+/* Helper to dump pubkey bytes for debugging */
+static void dump_pubkey_raw(const char* name, const secp256k1_pubkey* pk) {
+    const unsigned char* p = (const unsigned char*)pk;
+    printf("%s raw: ", name);
+    for (size_t i = 0; i < sizeof(*pk); i++) printf("%02x", p[i]);
+    printf("\n");
+}


dump_pubkey_raw is defined but not used. With common warning flags (e.g., -Wunused-function), this can fail CI if warnings are treated as errors. Consider removing it or wrapping it in a debug-only conditional (e.g., #ifdef DEBUG) and/or using it in an assertion failure path.

Fixed. Removed the unused dump_pubkey_raw function.

Copilot · 2026-02-10T13:32:25Z

tests/test_same_plaintext_multi_shared_r.c

+    // 3. Generate Proof
+    unsigned char proof[1024];

-    // UPDATED CALL: Removed 'proof_len'
-    res = secp256k1_mpt_verify_equality_shared_r(
+    // Passing flat arrays 'C2s' and 'pks' directly.
+    int res = secp256k1_mpt_prove_equality_shared_r(
            ctx, proof,
+            amount,          // correct arg order
+            r,
            N_RECIPIENTS,
-            &C1, C2, Pk, tx_id
+            &C1,
+            C2s,             // Pass the array directly
+            pks,             // Pass the array directly
+            tx_context


The proof buffer is hard-coded to unsigned char proof[1024];. For N=3 this happens to be larger than secp256k1_mpt_proof_equality_shared_r_size(N_RECIPIENTS), but it no longer documents/validates the required size and could silently become wrong if the proof format or recipient count changes. Prefer sizing the buffer using secp256k1_mpt_proof_equality_shared_r_size(N_RECIPIENTS) (stack VLA or malloc) to match the API contract.

Fixed. Updated proof buffers to use the _size() API helper instead of hard-coded values.

…pile_definitions

CMakeLists.txt

tests/test_link_proof.c

CMakeLists.txt

…nitions

mathbunnyru · 2026-02-11T15:52:54Z

I simplified cmake further - we don't need to have a separate variable, if move the section further down
And we can also make SECP256K1_WIDEMUL_INT128/SECP256K1_WIDEMUL_INT64 private definition, since we don't expose it as part of the library

mrtcnk · 2026-02-11T16:10:30Z

Thanks, that looks great! Moving the logic down and using PRIVATE definitions is much cleaner. I appreciate the simplification!

mrtcnk added 5 commits February 9, 2026 10:01

Igbore .idea files

abd681e

Merge remote-tracking branch 'upstream/main'

5ac0a66

Fix Release build crashes: Replace assert() with persistent checks an…

d716197

…d improve CMake 64-bit detection

Update tests/

ca9eeba

Fix CMake formatting

a60261f

mathbunnyru requested changes Feb 10, 2026

View reviewed changes

mathbunnyru reviewed Feb 10, 2026

View reviewed changes

src/mpt_scalar.c Show resolved Hide resolved

mathbunnyru changed the title ~~Fix/conan release build~~ Fix tests failing in release build Feb 10, 2026

mathbunnyru requested a review from Copilot February 10, 2026 13:26

Copilot started reviewing on behalf of mathbunnyru February 10, 2026 13:27 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

mrtcnk added 2 commits February 10, 2026 10:36

Refactor: Deduplicate EXPECT macro to test_utils.h and use target_com…

7833fa2

…pile_definitions

Refactor: Deduplicate EXPECT macro and use target_compile_definitions

a079363

mathbunnyru reviewed Feb 10, 2026

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mathbunnyru reviewed Feb 10, 2026

View reviewed changes

tests/test_link_proof.c Outdated Show resolved Hide resolved

Replaced with to match the macro checked in .

740a58a

mathbunnyru reviewed Feb 10, 2026

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mrtcnk added 4 commits February 10, 2026 23:16

Fix CMake scalar/field macros and standardize RNG usage in tests.

9f6155a

Merge upstream/main and resolve .gitignore conflict

58209fe

test: use API size helper for proof buffer allocation

c05da77

build: remove unused architecture flags, rely on WIDEMUL definition

166d37a

mathbunnyru reviewed Feb 11, 2026

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

mrtcnk and others added 2 commits February 11, 2026 10:32

build: clean up architecture flags and remove ineffective ECMULT defi…

530c239

…nitions

Refactor SECP256K1_WIDEMUL setting in CMakeLists.txt

3ecaae6

mathbunnyru approved these changes Feb 11, 2026

View reviewed changes

mathbunnyru merged commit 81e531e into XRPLF:main Feb 11, 2026
27 checks passed

Conversation

mrtcnk commented Feb 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathbunnyru commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

mrtcnk Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathbunnyru commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrtcnk commented Feb 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mathbunnyru commented Feb 10, 2026 •

edited

Loading

mathbunnyru commented Feb 11, 2026 •

edited

Loading