-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add DLEQ proof implementing BIP 374 #1802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Have you talked with @stratospher before opening this PR? If you have contacted stratospher, please ignore this comment. It's just to avoid putting the original author in an unfair or difficult position if still working on the PR. |
Yes, stratospher and I had discussed who should submit this new PR. I drew the short straw :) |
3c538a6 to
30d6f5e
Compare
stratospher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the confusion and thank you @macgyver13 for the PR!
did an initial pass and it looks good! mostly left style nits.
1 API design question I had is whether it would be better for the proof generation API to also accept C as an argument - that is GenerateProof(a, B, r, G, m, C) kind of API instead of GenerateProof(a, B, r, G, m).
When computing silent payment output points, we anyways compute shared secret/C for each output - so there's a possibility to avoid recomputation again if we can pass C. But current approach might be safer since callers could accidentally provide incorrect C and errors could happen.
Did you check other use cases of DLEQ for whether they might prefer passing C or computing C internally in the proof generation function?
src/modules/dleq/main_impl.h
Outdated
| static int secp256k1_dleq_hash_point(secp256k1_sha256 *sha, secp256k1_ge *p) { | ||
| unsigned char buf[33]; | ||
| size_t size = 33; | ||
| /* Reject infinity point */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2457f89: I guess this comment might be applicable now since the public API takes in secp256k1_pubkey and the ge we get is now from there - so unlikely for it to be infinity and VERIFY_CHECK might be better. though if we think about a function in isolation - it makes sense to reject infinity point.
cc @theStack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
45911b2 attempts to handle the 'invalid proof (e.g., all zeros)' case more clearly by moving the infinity check from secp256k1_dleq_hash_point to secp256k1_dleq_verify_internal, where R1 and R2 are computed. The check now occurs before calling secp256k1_dleq_challenge.
A check is required: invalid proofs (e.g., all zeros) will generate R1 and R2 as infinity points, which would cause verification to fail at VERIFY_CHECK(!secp256k1_ge_is_infinity(elem)); during serialization.
Open to a better solution.
src/modules/dleq/main_impl.h
Outdated
| } | ||
|
|
||
| secp256k1_nonce_function_bip374_sha256_tagged(&sha); | ||
| /* Hash masked-key||msg||m using the tagged hash as per BIP-374 v0.2.0 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2457f89: you could clarify that msg contains A and C. and that's different from m.
src/modules/dleq/main_impl.h
Outdated
| secp256k1_memclear_explicit(masked_key, sizeof(masked_key)); | ||
| } | ||
|
|
||
| /* Generates a nonce as defined in BIP0374 v0.2.0 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2457f89: not sure if we should mention the exact version - we don't mention the version for other modules like musig for example. only difference in newer version is the randomness computation in proof generation anyways.
src/modules/dleq/main_impl.h
Outdated
|
|
||
| ret = secp256k1_dleq_prove_internal(ctx, &s, &e, &a, &B, &A, &C, aux_rand32, msg); | ||
| if (!ret) { | ||
| secp256k1_scalar_clear(&a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
078d2da: since we always clear a - we can move it up and out of the if condition.
src/modules/dleq/tests_impl.h
Outdated
| if (i > 2 && i < 6) { | ||
| /* Skip tests indices 3-5: proof generation failure cases (a=0, a=N, B=infinity). | ||
| * These contain placeholder data from test_vectors_generate_proof.csv that would | ||
| * fail to parse. Only indices 0-2 and 6-12 have valid test data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2614b2a: extra white space
src/modules/dleq/tests_impl.h
Outdated
| CHECK_ILLEGAL(CTX, secp256k1_dleq_verify(CTX, proof, NULL, &B, &C, msg)); | ||
| CHECK_ILLEGAL(CTX, secp256k1_dleq_verify(CTX, proof, &A, NULL, &C, msg)); | ||
| CHECK_ILLEGAL(CTX, secp256k1_dleq_verify(CTX, proof, &A, &B, NULL, msg)); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 9149b57: extra white space in blank line
src/modules/dleq/Makefile.am.include
Outdated
| @@ -0,0 +1,3 @@ | |||
| include_HEADERS += include/secp256k1_dleq.h | |||
| noinst_HEADERS += src/modules/dleq/dleq_vectors.h | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2614b2a: tests also need to be added. nit: slight ordering preference for main_impl before test vectors - you can refer some other makefile.
tools/test_vectors_dleq_generate.py
Outdated
| for _ in range(5): # Skip the first 5 rows since those test vectors don't use secp's generator point | ||
| next(reader, None) | ||
|
|
||
| for i in range(3): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2614b2a: extra white space
|
|
||
| #include "../../../include/secp256k1.h" | ||
| #include "../../../include/secp256k1_dleq.h" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2457f89: #include "../../hash.h"
| #ifndef SECP256K1_MODULE_DLEQ_TESTS_H | ||
| #define SECP256K1_MODULE_DLEQ_TESTS_H | ||
|
|
||
| #include "dleq_vectors.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2614b2a: #include "../../unit_test.h"
|
I have a question regarding the choice of parameters passed to this API, and I may be missing some design context here — so I’d appreciate clarification. Other constructions with two generators on the same curve (e.g., Pedersen commitments, bulletproofs) typically derive the second generator using a hash‑to‑curve procedure, ensuring that no participant knows the discrete‑log ratio between |
Implements BIP-374 Discrete Log Equality (DLEQ) proofs as a new module. - Build system integration (CMake, autotools) - Configure dleq module as "optional" - Public API header declarations - Internal cryptographic implementation (prove_internal, verify_internal) - Tagged SHA256 functions per BIP-374 specification - Nonce generation following BIP-374 v0.2.0 Co-authored-by: stratospher <[email protected]>
- Adds test coverage for DLEQ internal functions - BIP-374 official test vectors (6 generation + 13 verification cases) - Includes Python script for generating test vectors matching BIP-374 specification. Tests call *_internal functions directly. Public API tests will be added in a subsequent commit. Co-authored-by: stratospher <[email protected]>
Adds public API functions that wrap the internal DLEQ implementation: - secp256k1_dleq_prove(): Generate DLEQ proof from secret key and base point B. Computes A = a*G and C = a*B internally, then generates proof. - secp256k1_dleq_verify(): Verify DLEQ proof given A, B, C public keys.
Adds comprehensive tests for the public DLEQ API: - secp256k1_dleq_prove(): Tests valid proof generation, NULL parameter detection, invalid secret key handling, context validation - secp256k1_dleq_verify(): Tests valid verification, NULL parameter detection for all required inputs (proof, A, B, C)
Reject proofs that produce infinity points during verification Add public API test case for infinity point scenario Add note to clarify contents of msg to reduce confusion with m Add missing includes Format tools/test_vectors_dleq_generate.py
30d6f5e to
45911b2
Compare
You probably mean soundness instead of correctness? If I'm not mistaken, this is simply not true. See for instance Nigel Smart's Cryptography: An Introduction (3rd Edition), Section 25.3.1, page 377, PDF page 389. This rather accessible write-up shows that the underlying Sigma protocol has correctness, 2-special soundness, and zero-knowledge, all with probability 1 and without any computational assumption. Technically, one doesn't even need the assumption that the discrete logarithm problem is hard in the group. Note in particular that the proof of special soundness extracts the witness and not the discrete logarithm between g and h (or G and B in our notation); the latter doesn't even show up in the proof.
Can you elaborate and also explain why you believe that this requires the assumption that the prover doesn't know the discrete logarithm between G and B? |
This PR adds a DLEQ (Discrete Logarithm Equality) proof module as specified in BIP 374.
Based on PR #1651 by @stratospher and the secp256k1-zkp implementation.
Public API
Exposes two functions for proof generation and verification:
secp256k1_dleq_provesecp256k1_dleq_verifyThese are designed to support rust FFI bindings and follow the API patterns established in similar modules.
Questions for Reviewers
Notes
Addressed outstanding comments from PR #1651: