Skip to content

bip374: add new test vectors checking for boundaries - #2335

Open
ViniciusCestarii wants to merge 1 commit into
bitcoin:masterfrom
ViniciusCestarii:bip-374-new-test-vectors
Open

ViniciusCestarii wants to merge 1 commit into
bitcoin:masterfrom
ViniciusCestarii:bip-374-new-test-vectors

Conversation

@ViniciusCestarii

Copy link
Copy Markdown
Contributor

Add 3 new generate proof test vectors for testing boundaries:

Success: a = 1, a = N-1
Failure: a = N+1 (catches implementation that does mod n on secret instead of first rejecting)

@jonatack jonatack added Proposed BIP modification PR by non-owner to update BIP content Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified labels Oct 20, 2026
@jonatack

Copy link
Copy Markdown
Member

cc BIP authors @andrewtoth, @RubenSomsen, and @theStack

@fametrano fametrano left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ACK 60b4c75, tested: the new vectors agree with an independent implementation.

The PR adds success cases for a=1 and a=N-1 and a failure case for a=N+1 to the proof-generation vectors. With the existing a=0 and a=N failures, both ends of the valid range are now pinned from both sides.

I ran all 14 rows of the CSV through btclib-ecc's BIP374 code, with and without libsecp256k1. Every success proof matched byte for byte and verified. The four failure rows (a=0, a=N, a=N+1, B at infinity) were refused. Re-running gen_test_vectors.py reproduced both CSV files exactly. The verification file is unchanged and already covers e=N and s=N, so I see no boundary missing from this change. Script and logs on request.

@theStack

Copy link
Copy Markdown
Contributor

LGTM, can you add a small changelog entry with bumped PATCH version?

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

Labels

Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified Proposed BIP modification PR by non-owner to update BIP content

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants