Skip to content

test: deduplicate scalar and field constants for n-1 and p-1#1820

Open
therohityadav wants to merge 1 commit intobitcoin-core:masterfrom
therohityadav:refactor-dedup-scalars
Open

test: deduplicate scalar and field constants for n-1 and p-1#1820
therohityadav wants to merge 1 commit intobitcoin-core:masterfrom
therohityadav:refactor-dedup-scalars

Conversation

@therohityadav
Copy link
Contributor

Following up on feedback from #1819, this PR deduplicates the hardcoded constants for the group order minus one (n-1) and the field prime minus one (p-1) within src/tests.c.

Changes:

  • Defined scalar_n_m1 and fe_p_m1 as global static constants at the top of the file.
  • Replaced all hardcoded hex occurrences of these values in:
    • test_scalar_check_overflow
    • run_scalar_tests (HALF_TESTS)
    • scalar_cmov_test
    • scalar_cases and fe_cases arrays
    • Local scalar_minus_one and fe_minus_one definitions.
  • Verified that make check passes all tests.

@therohityadav therohityadav force-pushed the refactor-dedup-scalars branch from ab653ab to d31d8c0 Compare February 5, 2026 05:05
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ~0

I'm not convinced this really improves readability or maintainability

@therohityadav
Copy link
Contributor Author

Concept ~0

I'm not convinced this really improves readability or maintainability

Thanks for the review @real-or-random.

I understand the concern about readability . having explicit values in tests is often preferred.

However, my idea here was to treat n-1 and p-1 as constants rather than arbitrary test vectors. By defining them in one place , we eliminate the risk of typos that could occur when these hex strings are repeated across the file. It ensures that every test checking these boundaries is mathematically guaranteed to be checking the same boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants