Skip to content

tests: add exhaustive extrakeys tweak coverage#1836

Open
mangoostaa wants to merge 1 commit intobitcoin-core:masterfrom
mangoostaa:add-exhaustive-tweak-tests-extrakeys
Open

tests: add exhaustive extrakeys tweak coverage#1836
mangoostaa wants to merge 1 commit intobitcoin-core:masterfrom
mangoostaa:add-exhaustive-tweak-tests-extrakeys

Conversation

@mangoostaa
Copy link
Copy Markdown

Motivation

  • Cover the previously-untested TODO in test_exhaustive_extrakeys to ensure x-only pubkey and keypair tweak APIs are exhaustively validated.
  • Validate success/failure behavior for every non-zero internal key and every tweak scalar in the exhaustive test group.
  • Ensure consistency between xonly_pubkey_tweak_add, xonly_pubkey_tweak_add_check, and keypair_xonly_tweak_add
    and detect edge cases (resulting infinity, parity/x-coordinate mismatches).

Description

Replaced the TODO in src/modules/extrakeys/tests_exhaustive_impl.h with nested loops that exercise every internal non-zero key and every tweak scalar and compute the expected resulting group element and parity.
For each case the test exercises secp256k1_xonly_pubkey_tweak_add, secp256k1_keypair_xonly_tweak_add, secp256k1_xonly_pubkey_tweak_add_check, and verifies serialized compressed pubkey bytes and x-only serialization against the expected group point.
Fixed test-side issues: call secp256k1_fe_get_b32 without wrapping in CHECK (avoids invalid use of EXPECT), normalize field elements before checking parity and exporting to bytes, reset serialized_pklen prior to reuse, and derive the keypair secret bytes via secp256k1_scalar_get_b32 for deterministic keypair creation in the exhaustive loop.
All changes are contained in src/modules/extrakeys/tests_exhaustive_impl.h and only affect the exhaustive test code

Copy link
Copy Markdown
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.

ACK 61796cb

Copy link
Copy Markdown
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 61796cb

}

/* TODO: keypair/xonly_pubkey tweak tests */
/* Check keypair/xonly_pubkey tweak behavior over all non-zero tweaks. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whitespace nit:

Suggested change
/* Check keypair/xonly_pubkey tweak behavior over all non-zero tweaks. */
/* Check keypair/xonly_pubkey tweak behavior over all non-zero tweaks. */

Comment on lines +116 to +120
CHECK(secp256k1_ec_pubkey_serialize(ctx, serialized_pk, &serialized_pklen, &tweaked_pk, SECP256K1_EC_COMPRESSED));
CHECK(serialized_pklen == sizeof(serialized_pk));
CHECK((serialized_pk[0] == SECP256K1_TAG_PUBKEY_EVEN) || (serialized_pk[0] == SECP256K1_TAG_PUBKEY_ODD));
CHECK(serialized_pk[0] == (expected_pk_parity ? SECP256K1_TAG_PUBKEY_ODD : SECP256K1_TAG_PUBKEY_EVEN));
CHECK(secp256k1_memcmp_var(&serialized_pk[1], expected_x, 32) == 0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit (here and below): serializing is not strictly needed I think, one could alternatively do these checks on the internal types, i.e. by loading into a group element instance (via secp256k1_pubkey_load) first and then check its x/y field elements.

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.

3 participants