Skip to content

Conversation

@theStack
Copy link
Contributor

Following up on the recent decision that secp256k1lab should be vendored per-BIP if needed/useful (see #2004 (comment) ff. and #1855 (comment) ff.), this PR exercises this for BIP-374. The changes add secp256k1lab version 1.0.0 as subtree [1] within the bip-0374 folder and take use of it in the reference implementation. In particular, secp256k1.py is removed and the TaggedHash and xor_bytes routines are used from the secp256k1lab utils module, to simplify the code further. Usage of the library is mentioned in the BIP text to be license compliant (see #2004 (comment)).

Can be tested via:

$ ./bip-0374/run_test_vectors.py
.....
All test vectors passed.
$ ./bip-0374/gen_test_vectors.py
Generating /home/thestack/bips/bip-0374/test_vectors_generate_proof.csv...
Generating /home/thestack/bips/bip-0374/test_vectors_verify_proof.csv...
$ git diff
<shouldn't show any diffs, as generated .csv files should match the ones in the repository> 

[1] added via the command $ git subtree add --prefix=bip-0374/secp256k1lab --squash https://github.com/secp256k1lab/secp256k1lab v1.0.0

git-subtree-dir: bip-0374/secp256k1lab
git-subtree-split: 44dc4bd893b8f03e621585e3bf255253e0e0fbfb
This approach is incompatible with the sys.path extension approach
in the next commit which is used to to find the vendored copy of
secp256k1lab, so use __file__ instead which works as well.
Copy link
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

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

ACK 2b7f079

The changes are aligned with the referenced comments. I'm not reviewing the vendor library as the change is not about the qualities of it.

@macgyver13
Copy link
Contributor

ACK 2b7f079

Tested vector generation and confirmed no changes to generated .csv files.

Additionally tested the same technique in #2046. The next revision of #2046 will depend on the changes in this PR.

@murchandamus
Copy link
Contributor

cc: @andrewtoth, @RubenSomsen; perhaps also of interest to @real-or-random.

@murchandamus
Copy link
Contributor

Also, while I have you here, BIP 374 has been published for over a year, it appears to have a reference implementation and test vectors, do you still have planned work outstanding? Otherwise, you could perhaps consider whether it is time to advance it from Draft to Complete.

@stratospher
Copy link
Contributor

ACK 2b7f079.

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.

5 participants