Schnorr Rebase #1
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Found thanks to the developer checks from the pkgsrc software distribution (for NetBSD, SmartOS, Minix, MacOS X, Linux, and more).
in case this code should ever be used as an example, a warning is a nice way of helping ensure insecure keys are not generated
the two middle arguments to fread() are easily confused, and cause the checking of return value to fail incorrectly (and possibly succeed incorrectly.)
95e99f1 fix tests.c in the count == 0 case (Andrew Poelstra) Pull request description: Fixes bitcoin-core#528 Tree-SHA512: 8b28d84f95bcd1337fbd7fb187dee2a9bad2b6b595eaf42a2d855e5784f48a1f3ad5739881b22eea115d32c4525feb69b41958699a165c847fcfb8096cc4903a
Move secp256k1_fe_normalize_weak calls out of ECMULT_TABLE_GET_GE and ECMULT_TABLE_GET_GE_STORAGE and into secp256k1_ge_globalz_set_table_gej instead.
static const secp256k1_ge secp256k1_ge_const_g; static const int CURVE_B;
This results in more self-documenting code.
c8fbc3c [ECDH API change] Allow pass arbitrary data to hash function (Kirill Fomichev) b00be65 [ECDH API change] Support custom hash function (Kirill Fomichev) Pull request description: Solve bitcoin-core#352 Tree-SHA512: f5985874d03e976cdb3d59036af7720636ad1488da40fd3bd7881b1fb71b05036a952013d519baa84c4ce4b558bdef25c4ce76b384b297e4d0aece9e37e78a01
…lities 40fde61 prevent attempts to modify `secp256k1_context_no_precomp` (Andrew Poelstra) ed7c084 add static context object which has no capabilities (Andrew Poelstra) Pull request description: Tree-SHA512: a843ed7ba00a00a46eec3146ce428d4b49eb440af766f44d731b1f51553d08de8cc9a0af5ed114d0dfdca6f4bf4a2ede4dbd6a37d6bd818b81630089424a0ba5
… contexts b3bf5f9 ecmult_impl: expand comment to explain how effective affine interacts with everything (Andrew Poelstra) efa783f Store z-ratios in the 'x' coord they'll recover (Peter Dettman) ffd3b34 add `secp256k1_ge_set_all_gej_var` test which deals with many infinite points (Andrew Poelstra) 84740ac ecmult_impl: save one fe_inv_var (Andrew Poelstra) 4704527 ecmult_impl: eliminate scratch memory used when generating context (Andrew Poelstra) 7f7a2ed ecmult_gen_impl: eliminate scratch memory used when generating context (Andrew Poelstra) Pull request description: Builds on bitcoin-core#553 Tree-SHA512: 6031a601a4a476c1d21fc8db219383e7930434d2f199543c61aca0118412322dd814a0109c385ff1f83d16897170dd0c25051697b0f88f15234b0059b661af41
If we’re in the last loop iteration, then `lenleft == 1` and it could be the case that `ret == MAX_SIZE`, and so `ret + lenleft` will overflow to 0 and the sanity check will not catch it. Then we will return `(int) MAX_SIZE`, which should be avoided because this value is implementation-defined. (However, this is harmless because `(int) MAX_SIZE == -1` on all supported platforms.)
0522caa Explain caller's obligations for preallocated memory (Tim Ruffing) 238305f Move _preallocated functions to separate header (Tim Ruffing) 695feb6 Export _preallocated functions (Tim Ruffing) 814cc78 Add tests for contexts in preallocated memory (Tim Ruffing) ba12dd0 Check arguments of _preallocated functions (Tim Ruffing) 5feadde Support cloning a context into preallocated memory (Tim Ruffing) c4fd5da Switch to a single malloc call (Tim Ruffing) ef020de Add size constants for preallocated memory (Tim Ruffing) 1bf7c05 Prepare for manual memory management in preallocated memory (Tim Ruffing) Pull request description: @apoelstra This builds on bitcoin-core#557. Manually managing memory is always a pain in the ass in some way. I tried to keep the pain manageable. I'm open to suggestions to make this less ugly or error-prone. to do: * tests * export functions ACKs for commit 0522ca: Tree-SHA512: 8ddb5b70219b6f095e780a9812d2387ab2a7f399803ce4101e27da504b479a61ebe08b6380568c7ba6f1e73d7d0b1f58a3c0a66fa0fdec7a64cd0740e156ce38
Broken by 3f3964e. It's important that the tests are also run without -DVERIFY due to the possibility that side-effects of a VERIFY_CHECK fix a bug that would otherwise be detected. Use of the verify_check macro in tests isn't sufficient.
…tch space is small 9ab96f7 Use trivial algorithm in ecmult_multi if scratch space is small (Jonas Nick) Pull request description: `ecmult_multi` already selects the trivial algorithm if the scratch space is NULL. With this PR the trivial algorithm is also selected if the scratch space is too small to use pippenger or strauss instead of returning 0. That makes it more easier to avoid consensus relevant inconsistencies just because scratch space construction was messed up. ACKs for commit 9ab96f: real-or-random: utACK 9ab96f7 Tree-SHA512: aa451adf8880af15cf167a59cb07fc411edc43f26c8eb0873bdae2774382ba182e2a1c54487912f8f2999cb0402d554b9d293e2fb9483234471348a1f43c6653
Use it when checking magic bytes
98836b1 scratch: replace frames with "checkpoint" system (Andrew Poelstra) 7623cf2 scratch: save a couple bytes of unnecessarily-allocated memory (Andrew Poelstra) a7a164f scratch: rename `max_size` to `size`, document that extra will actually be allocated (Andrew Poelstra) 5a4bc0b scratch: unify allocations (Andrew Poelstra) c2b028a scratch space: thread `error_callback` into all scratch space functions (Andrew Poelstra) 0be1a4a scratch: add magic bytes to beginning of structure (Andrew Poelstra) 92a48a7 scratch space: use single allocation (Andrew Poelstra) Pull request description: ACKs for commit 98836b: Tree-SHA512: 6e251f704644a5f61b24aa05c6f7a31ad8c58d147195079d52fe45daacd28a9fd2f4aaf71273183b99b3795a01a88f8389170d4280489b2a28a14a56e03153d7
Most of the codebase correctly used short-cutting to avoid calling _is_zero on possibly incompletely initialized elements, but a few places were missed.
e49f799 Add missing #(un)defines to base-config.h (Tim Ruffing) 77defd2 Add secp256k1_ prefix to default callback functions (Tim Ruffing) 908bdce Include stdio.h and stdlib.h explicitly in secp256k1.c (Tim Ruffing) 5db782e Allow usage of external default callbacks (Tim Ruffing) 6095a86 Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return (Tim Ruffing) Pull request description: This is intended for environments without implementations for `abort()`, `fprintf()`, and `stderr`. e.g., embedded systems. Those can provide their own implementations of `default_illegal_callback_fn` and `default_error_callback_fn` at compile time. If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data `extern` too, because then the initialization lists for `default_illegal_callback` won't contain only constants. (`const` variables are not compile-time constants). So you cannot take callback data in your own default callback function. As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss. Note that `abort()`, `fprintf()`, and `stderr` are also used in `CHECK`, which is still used in production code if we rely on gmp for scalar and field inversions (e.g., https://github.com/bitcoin-core/secp256k1/blob/master/src/scalar_impl.h#L240). This is not an issue for embedded system which probably don't want to use gmp anyway, but it is probably an issue for the reasons explained in bitcoin-core#566 (comment). (related downstream: rust-bitcoin/rust-secp256k1#100 @elichai) ACKs for commit e49f79: Tree-SHA512: 4dec0821eef4156cbe162bd8cdf0531c1fae8c98cd9db8438170ff1aa0e59b199739eeab293695bb582246812bea5309959f02f1fb74bb57872da54ebc52313f
…havior when dealing with sizes 14c7dbd Simplify control flow in DER parsing (Tim Ruffing) ec8f20b Avoid out-of-bound pointers and integer overflows in size comparisons (Tim Ruffing) 01ee1b3 Parse DER-enconded length into a size_t instead of an int (Tim Ruffing) 3cb057f Fix possible integer overflow in DER parsing (Tim Ruffing) Pull request description: This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed. I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code. Best reviewed in individual commits. ACKs for commit 14c7db: Tree-SHA512: 312dd3f961739752e1a861e75bd755920f634f87ee9668793e102c224434e8d21367452e114de729322c71a89f4fa82126aa5d32742f2bbbc091777c99515e10
248bffb Guard memcmp in tests against mixed size inputs. (Gregory Maxwell) Pull request description: Reported by real-or-random. Fixes bitcoin-core#623. ACKs for commit 248bff: practicalswift: utACK 248bffb Tree-SHA512: 29867c79d2d6852f495334a5a9129c7feac2df639dd7f752067380689b0ce9f9b35e94524834c01e698df5c0b83dc9855204ec09f5dfe488a388b509c9b861d9
dcf3920 Fix ability to compile tests without -DVERIFY. (Gregory Maxwell) Pull request description: Broken by 3f3964e. It's important that the tests are also run without -DVERIFY due to the possibility that side-effects of a VERIFY_CHECK fix a bug that would otherwise be detected. Use of the verify_check macro in tests isn't sufficient. ACKs for commit dcf392: Tree-SHA512: ff7ca0e89e33f845656a4d7d18c0195d1378b020d67f89e900b18cf3d702aa81dd91ffd05a98953a481b83e4247eaf0c484bea12eab020efb3c966a456e8129f
8d1563b Note intention of timing sidechannel freeness. (Gregory Maxwell) Pull request description: Resolves bitcoin-core#238 ACKs for commit 8d1563: Tree-SHA512: 2b0ca945d70e5975291ed9a0884eddfd771fd06dfed37c9711f8b57d431c28b974e5a5d86ae6e70e5e37c5f208bcb74e9ab18fcf9d7b78849fcf3cff9ba7623b
cd473e0 Avoid calling secp256k1_*_is_zero when secp256k1_*_set_b32 fails. (Gregory Maxwell) Pull request description: Most of the codebase correctly used short-cutting to avoid calling _is_zero on possibly incompletely initialized elements, but a few places were missed. ACKs for commit cd473e: sipa: utACK cd473e0 jonasnick: utACK cd473e0 Tree-SHA512: d6af2863f6795d2df26f2bd05a4e33085e88c45f7794601ea57e67238a2073ef1ee3ba0feab62a7fcbc0636c48dfd80eea07d0ca4f194414127f914b0478c732
…ning, verification and batch verification. [0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki
This function was already exported but not implemented. renamed `parse` to `decompress` a little more consistency in method comments removed dependency on google guava library It seemed a little overkill given that we only use one trivial function from this library. fixed github link bitcoin->bitcoin-core tests: removed guava dependency + cleanup added pub key parsing tests add `compressed` arg to methods returning a pubkey removed unreachable null check fixup: typo in tests (squashed commits 287634c to 2e16ac7) from https://github.com/bitcoin-s/secp256k1/commits/2e16ac7d6ca09f9e06be740c39e3009aeb85324c (rebased on top of https://github.com/jonasnick/secp256k1/tree/schnorrsig)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
21 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR squashes then rebases the work from https://github.com/bitcoin-s/secp256k1/commits/287634c955e703704a1ec7030ed7d41a145a84db to https://github.com/bitcoin-s/secp256k1/commits/2e16ac7d6ca09f9e06be740c39e3009aeb85324c on top of https://github.com/jonasnick/secp256k1/tree/schnorrsig
It then adds
schnorrsig_signandschnorrsig_verifyto the jni in the last commit, which is the only commit I have authored.