Add crypto jets for secp256k1 operations. #34

roconnor-blockstream · 2019-12-18T19:41:54Z

A set of crypto jets cloned from secp256k1.

roconnor-blockstream · 2020-04-09T17:05:24Z

@ysangkok You may want to follow this PR. We will have Schnorr jets in master when this is merged.

roconnor-blockstream · 2021-01-26T18:13:35Z

Updated to recent libsecp256k1, which includes the latest BIP-0340 schnorr signature standard.

roconnor-blockstream · 2021-08-31T21:51:28Z

@apoelstra This PR is ready for your review. You need only review the C code. Please have a look at C/secp256k1/README.md first.

apoelstra · 2021-09-27T20:02:18Z

C/jets-secp256k1.c

+static inline void write_gej(frameItem* dst, secp256k1_gej* r) {
+  write_fe(dst, &r->x);
+  write_fe(dst, &r->y);
+  write_fe(dst, &r->z);


In efbe85f

Does this function need a check for if (gej->infinity), like write_ge?

If not, why not?

if you grep infinity = across the codebase and ignore all the = 0 cases, you will find a short list of places that the infinity flag might be set. Most are ok. One that jumps out at me though is secp256k1_gej_set_ge which sets the output infintiy flag equal to the input infinity flag, but uncontditionally sets the output z coordinate to 1.

Similar code exists in secp256k1_gej_add_zinv_var

In fact, I think are the only two offending lines, and that they should be fixed inline. In addition to breaking write_gej, they violate the claim in your second commit (19d3074) message that "Our specification has no infinity flag, and treats the infinity flag as a cache of a z=0 test."

Given this analysis I think you can probably remove the r->infinity check from write_ge as well.

I also checked every instance of secp256k1_fe_add to see if they might change the coordinates of an infinite point away from zero, and none of them do. Meanwhile secp256k1_fe_mul and secp256k1_fe_neg cannot change a 0 value to a nonzero one and secp256k1_fe_inv_var I assume is guarded against being passed zero.

Since bitcoin-core/secp256k1@dd6c3de it does seem that the r->infinity check is no longer needed since secp256k1_ge_set_gej_var now explicitly calls secp256k1_ge_set_infinity.

I believe a very detailed analysis would show that secp256k1_gej_set_ge and especially secp256k1_gej_add_zinv_var are never called in such a way to set the infinity flag in this way.

But, for the same reason, there is very little reason not to amend the code to make the z-coordinate match the flag even in these cases. The diff will be small.

I will make that ammendment.

I believe these concerns have now been addressed.

C/jets-secp256k1.c

C/primitive/elements/primitive.c

roconnor-blockstream · 2021-10-29T14:41:58Z

This should be ready again.

apoelstra

ACK bdc0ce1

Did not review the Haskell code; did not review the Simplicity bytecode; did not attempt to regenerate the autogenerated C code. But I did read the rest of the code (or at least used range-diff to confirm that it was substantially similar to what I had reviewed before) and checked the encoding-infinity changes, which I believe are ok now.

cc @sanket1729 this PR changes the jet encoding, you may want to check it out.

ProofOfKeags · 2021-11-03T18:14:24Z

I could give a go at reviewing the Haskell code. So far what I gather is that the Haskell code is doing C source generation for test cases. But it also looks like there are jets that are being called from the Haskell code. Is the purpose for the Haskell code to be an entire test runner for the C tests (generation, running, and eval) or am I missing the broader purpose here?

roconnor-blockstream · 2021-11-03T19:32:55Z

The Haskell code has an bunch of aspects to it.

A Haskell implementation of secp256k1 jet functionality in Haskell/Core/Simplicity/LibSecp256k1/Spec.hs
A Simplicity implementation (in Haskell) of secp256k1 jet functionality in Haskell/Core/Simplicity/Programs/LibSecp256k1.hs.
Haskell bindings to the C implementation of secp256k1 (C/jets-secp256k1.c) jet functionality in Haskell/Core/Simplicity/FFI/Jets.hs (new in this PR).
Testing that compares the Simplicity implementation to the C implementation in Haskell/Tests/Simplicity/FFI/Tests.hs
Testing that compares the Simplicity implementation to the Haskell implementaion in Haskell/Tests/Simplicity/Programs/Tests.hs.
Simplicity's jet encodings for the secp256k1 jet functionality in Haskell/Core/Simplicity/CoreJets.hs (new in this PR).

The testing code uses a "fast-evaluation" which ends up runs the C code for evaluation of any (proper!) sub-expressions matching that matches jets. The testing therefore, in a sense, operates in layers, where the testing of the field operations implies the reliability of using those field operation's C implementation for testing the group operations, and so forth. This keeps the test time manageable, because running the whole Schnorr signature verification directly in Simplicity is exhausting.

Somewhat separately we have in Haskell-GenerateC some Haskell programs that uses the Haskell Simplicity library to generated the .inc files for C/primitive/elements/primtive.c covers the type inference for jets and binding the jet enumeration to the jet fuctionality. There is also a program to generate some of the test data by encoding some simple simplicity programs and generating signatures that validate.

I hope that this is a helpful guide.

FWIW, review of the Haskell code is not blocking for this PR. The Haskell code is technically informative at not normative. Only the C code will form the consensus rules when incorporated into Elements or Bitcoin. Thus I may merge this PR prior to any Haskell review. That said, even post-merge reviews are helpful and appreciated if you are willing to perform them.

P.S. You may stumble over Haskell/Core/Simplicity/LibSecp256k1/FFI.hs which is obsolete C bindings that are slated to be removed (#64). This file is not part of the cabal project and isn't part of the build. You can safely ignore it.

sanket1729

I need more time to review this. The reasoning about infinity is confusing

C/jets-secp256k1.c

sanket1729 · 2021-11-04T15:26:42Z

C/jets-secp256k1.c

+
+  unsigned char buf[64];
+  secp256k1_xonly_pubkey pubkey;
+  unsigned char msg[32];


If and when we support variable len messages, how would it work?

One has to use some combination of secp256k1-pubkey-unpack, bip0340-challenge-midstate, and secp256k1-linear-verify. These have been sketched out in the Tech Report, but not all of them have made it into this PR.

We need our own copy of libsecp256k1 because we are breaking the API by directly creating Simplicity bindings for low-level elliptic curve operations.

Fixups of secp256k1 to make it compatible with our prefered specifications. (1) Our specification has no infinity flag, and treats the infinity flag as a cache of a z=0 test. This view holds everywhere except when doubling a point of order 2. We fix this instance. We also adjust secp256k1_gej_set_ge and secp256k1_gej_add_zinv_var to ensure that the z-value is set to zero when infinity is returned. (2) gej_add_ge leaves the rzr value uninitialized when the a input is infinity. This can occur when ecmult builds a scalar table for very low order points, i.e. odd order points of order less than 15 or so. While this uninitialized value is read and then eventually multiplied by 0, this is techincally unspecified behaviour. Thus we prefer to explicity clear the rzr in this case. (3) Our specification of GE cannot include points at infinity. However secp256k1_ge_globalz_set_table_gej may produce GE points at the infinity when ecmult builds a scalar table for very low order points, i.e. odd order points of order less than 15 or so. For compatability with our specification, we clear the infinity flag, yeilding the cusp point of (0,0).

With secp256k1 jets available the compile-test cycle seems faster without optimizations than with optimizations.

Now that tests use jets for evaluation, they are not as long running, (though obviously now assume that the jet implementations are faithful).

sanket1729

Looks good to me. I would want to cross-test this with rust-simplicity, but it is far behind in implementation. I think we can go ahead with this for now.

I checked that jets do the correct thing for points on secp256k1, but still am not sure if operations on out of curve points can never produce a point that is on curve

roconnor-blockstream · 2021-11-22T16:03:52Z

I have little doubt that running "addition" on points that are off curve, especially on inputs that are not even on the same curve, can result in points on the secp256k1 curve. (in particular all the curves share the same representation for points at infinity.) But there is nothing wrong with that.

roconnor-blockstream mentioned this pull request Dec 18, 2019

secp256k1 jets #18

Closed

apoelstra self-requested a review December 18, 2019 22:44

roconnor-blockstream force-pushed the C-secp256k1 branch from 8804eaa to f76406a Compare April 9, 2020 14:51

roconnor-blockstream marked this pull request as draft May 4, 2020 19:46

roconnor-blockstream force-pushed the C-secp256k1 branch 2 times, most recently from 072801b to db16b46 Compare September 7, 2020 16:28

roconnor-blockstream force-pushed the C-secp256k1 branch from db16b46 to ccd93b3 Compare September 16, 2020 12:46

roconnor-blockstream mentioned this pull request Oct 6, 2020

Remove obsolete secp256k1git / mysecp256k1 / Simplicity.Libsecp256k1.FFI #64

Closed

roconnor-blockstream force-pushed the C-secp256k1 branch from ccd93b3 to b2fe26d Compare January 26, 2021 18:11

roconnor-blockstream changed the base branch from master to haskell-dev January 26, 2021 18:12

roconnor-blockstream force-pushed the C-secp256k1 branch from b2fe26d to 5d7a997 Compare January 31, 2021 02:10

roconnor-blockstream force-pushed the C-secp256k1 branch from 5d7a997 to 6419a43 Compare May 19, 2021 20:11

roconnor-blockstream changed the base branch from haskell-dev to master May 19, 2021 21:21

roconnor-blockstream force-pushed the C-secp256k1 branch from 6419a43 to 8765456 Compare June 4, 2021 17:52

roconnor-blockstream force-pushed the C-secp256k1 branch 2 times, most recently from 23f0b5a to 403e49c Compare June 19, 2021 17:51

roconnor-blockstream force-pushed the C-secp256k1 branch from 403e49c to 1790810 Compare July 25, 2021 01:10

roconnor-blockstream force-pushed the C-secp256k1 branch from 1790810 to f89cc92 Compare August 31, 2021 21:47

roconnor-blockstream marked this pull request as ready for review August 31, 2021 21:48

roconnor-blockstream changed the title ~~Add a sample of jets for secp256k1 operations.~~ Add crypto jets for secp256k1 operations. Aug 31, 2021

roconnor-blockstream force-pushed the C-secp256k1 branch 2 times, most recently from 71040b9 to 07bd9ae Compare September 8, 2021 17:07

apoelstra reviewed Sep 27, 2021

View reviewed changes

C/jets-secp256k1.c Outdated Show resolved Hide resolved

apoelstra reviewed Sep 27, 2021

View reviewed changes

C/primitive/elements/primitive.c Outdated Show resolved Hide resolved

apoelstra reviewed Sep 27, 2021

View reviewed changes

C/primitive/elements/primitive.c Outdated Show resolved Hide resolved

apoelstra reviewed Sep 27, 2021

View reviewed changes

C/primitive/elements/primitive.c Outdated Show resolved Hide resolved

roconnor-blockstream changed the base branch from master to pdf October 29, 2021 14:44

roconnor-blockstream changed the base branch from pdf to master October 29, 2021 14:44

roconnor-blockstream force-pushed the C-secp256k1 branch from 5c8fb3b to bdc0ce1 Compare October 29, 2021 15:17

apoelstra approved these changes Nov 2, 2021

View reviewed changes

roconnor-blockstream force-pushed the C-secp256k1 branch from bdc0ce1 to c84e74e Compare November 3, 2021 19:43

sanket1729 reviewed Nov 3, 2021

View reviewed changes

C/jets-secp256k1.c Outdated Show resolved Hide resolved

C/jets-secp256k1.c Show resolved Hide resolved

sanket1729 reviewed Nov 4, 2021

View reviewed changes

roconnor-blockstream force-pushed the C-secp256k1 branch from c84e74e to f4b709e Compare November 8, 2021 15:07

roconnor-blockstream added 12 commits November 19, 2021 13:33

Conistently use _'s before digits in snake_case.

be67436

Modify gej_normalize to return Nothing in case of infinity.

15058b9

Carve out a copy of libsecp256k1 verification routines.

37ac6b2

We need our own copy of libsecp256k1 because we are breaking the API by directly creating Simplicity bindings for low-level elliptic curve operations.

Add Haskell bindings to new C crypto jets.

f33fd53

Add CoreJets for Secp256k1 operations.

8ff9922

Set -O0 for tests.

b0a5d8e

With secp256k1 jets available the compile-test cycle seems faster without optimizations than with optimizations.

Add in tests that are no longer expensive.

92e0656

Now that tests use jets for evaluation, they are not as long running, (though obviously now assume that the jet implementations are faithful).

Add Tests for secp256k1 jets.

3ff9ebe

Decode secp256k1 jets in C.

c309bf5

Update C test cases to use BIP0340 signature jets.

d31496c

A Haskell program to generate the test cases used in the C project.

424411b

roconnor-blockstream force-pushed the C-secp256k1 branch from f4b709e to 424411b Compare November 19, 2021 18:34

sanket1729 approved these changes Nov 21, 2021

View reviewed changes

roconnor-blockstream merged commit 424411b into master Nov 23, 2021

roconnor-blockstream deleted the C-secp256k1 branch November 23, 2021 16:07

roconnor-blockstream mentioned this pull request Mar 15, 2023

Quickcheck Infrastructure: Haskell <> C (Jet) #32

Closed

roconnor-blockstream mentioned this pull request May 3, 2023

Remove obsolete libsecp256k1 bindings #138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add crypto jets for secp256k1 operations. #34

Add crypto jets for secp256k1 operations. #34

roconnor-blockstream commented Dec 18, 2019 •

edited

Loading

roconnor-blockstream commented Apr 9, 2020

roconnor-blockstream commented Jan 26, 2021

roconnor-blockstream commented Aug 31, 2021

apoelstra Sep 27, 2021 •

edited

Loading

apoelstra Sep 27, 2021

apoelstra Sep 27, 2021 •

edited

Loading

roconnor-blockstream Oct 15, 2021

roconnor-blockstream Oct 15, 2021

roconnor-blockstream Oct 16, 2021

roconnor-blockstream commented Oct 29, 2021

apoelstra left a comment

ProofOfKeags commented Nov 3, 2021

roconnor-blockstream commented Nov 3, 2021

sanket1729 left a comment

sanket1729 Nov 4, 2021

roconnor-blockstream Nov 4, 2021

sanket1729 left a comment

roconnor-blockstream commented Nov 22, 2021

Add crypto jets for secp256k1 operations. #34

Add crypto jets for secp256k1 operations. #34

Conversation

roconnor-blockstream commented Dec 18, 2019 • edited Loading

roconnor-blockstream commented Apr 9, 2020

roconnor-blockstream commented Jan 26, 2021

roconnor-blockstream commented Aug 31, 2021

apoelstra Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

apoelstra Sep 27, 2021

Choose a reason for hiding this comment

apoelstra Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

roconnor-blockstream Oct 15, 2021

Choose a reason for hiding this comment

roconnor-blockstream Oct 15, 2021

Choose a reason for hiding this comment

roconnor-blockstream Oct 16, 2021

Choose a reason for hiding this comment

roconnor-blockstream commented Oct 29, 2021

apoelstra left a comment

Choose a reason for hiding this comment

ProofOfKeags commented Nov 3, 2021

roconnor-blockstream commented Nov 3, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Nov 4, 2021

Choose a reason for hiding this comment

roconnor-blockstream Nov 4, 2021

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

roconnor-blockstream commented Nov 22, 2021

roconnor-blockstream commented Dec 18, 2019 •

edited

Loading

apoelstra Sep 27, 2021 •

edited

Loading

apoelstra Sep 27, 2021 •

edited

Loading