Skip to content

Conversation

@nkohen
Copy link

@nkohen nkohen commented Aug 7, 2019

Adds functions secp256k1_schnorrsig_pubnonce and secp256k1_schnorrsig_sig_pubkey which are necessary for a DLC server and client respectively (see https://adiabat.github.io/dlc.pdf)

TODO:

  • Tests

Copy link
Owner

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Hi, this is really cool and I'd be interested to move forward with this. I have a few suggestions that would help to get going:

  1. add a pubnonce type because it's not a pubkey, needs its own serialization and deserialization functions
  2. change the api of the pubnonce function. Instead of accepting the secret nonce, make it similar to schnorrsig sign where it takes the seckey, msg, nonce function and nonce data. Then it's easily compatible with schnorrsig sign, we don't need the secret schnorrnonce type and we remove a giant footgun. If you want to work with specific nonces in order to instantiate single use signatures, you can do that by using a nonce function with a constant return value.
SECP256K1_API int secp256k1_schnorrsig_pubnonce(
    const secp256k1_context* ctx,
    secp256k1_schnorrsig_pubnonce* r,
    const unsigned char *msg32,
    const unsigned char *seckey,
    secp256k1_nonce_function noncefp,
    void *ndata
)
  1. "sig_pubkey" I don't know about this. On the other hand in DLCs it's used just like a pubkey, so the name is kind of appropriate.
  2. add tests
  3. make this its own experimental module
  4. open PR to libsecp-zkp, because it's way easier to get merged there. However the schnorr signature implementation is really outdated (no xonly keys yet) I'll update that as soon as I got more review comments on my schnorrsig PR.

@jonasnick jonasnick force-pushed the schnorrsig branch 2 times, most recently from f7661e9 to d727cb3 Compare November 14, 2019 13:54
@jonasnick jonasnick force-pushed the schnorrsig branch 3 times, most recently from 66a7315 to 1901f3b Compare December 3, 2019 21:33
@nkohen
Copy link
Author

nkohen commented Dec 5, 2019

Thanks for the review :) my working on this will probably be a little sparse as I'm deep in the weeds of implementing other DLC stuff on top of this for the time being, so sorry in advance :/

That being said I'd really love for this code to get into some place as stable as possible so that I don't have to maintain it, so it would be great to get this merged in libsecp-zkp when possible!

Now I consider myself a novice when it comes to C coding (I like my high level languages) but I'll do my best to follow all your instruction, but first, some likely silly comments/questions to make sure I have everything clear:

  1. I'm all for adding a pubnonce type, where should I put it? I'm sure there's some obvious place but I don't know my way around the codebase very well but I'm sure I can figure out how to do it right if you point me to where other similar stuff is. Also, while I agree it is not a pubkey and deserving of its own type, can I not reuse serialization/deserialization since it is the same at the bit-level?
  2. Sounds good, will do
  3. Do you think that perhaps "sig_point" is better?
  4. I'll likely have some questions when I get around to this
  5. This shouldn't be too hard, the only think I might not be clear on is how to handle the make file, I'll follow up
  6. sounds good, lmk when you have your newer work in there and I'll target them

@jonasnick
Copy link
Owner

my working on this will probably be a little sparse

If I have more time for this in the future I'd be happy to help more concretely, with code, if that's okay for you.

I'm all for adding a pubnonce type, where should I put it? I'm sure there's some obvious place

Actually, musig needs a nonce type too. I'll add it there and then this module can copy that.

do you think sig_point is better

Not ideal but better.

might not be clear on is how to handle the make file

Should be relatively easy, because you can just copy from other experimental modules (like the schnorrsig module).

lmk when you have your newer work

BlockstreamResearch/secp256k1-zkp#86 should bring everything up to date

Copy link
Owner

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Actually, musig needs a nonce type too. I'll add it there and then this module can copy that.

A nonce type that works in every crypto protocol wouldn't be much more helpful in preventing people from mixing things up than using the pubkey type for the nonce type. In the musig case I've proposed to just return a 32-byte array containing the nonce.

In the sig_point case, do we expect to do more crypto operations with the nonce? If not, we can just use bytes too. Otherwise, we may want to add a type (because converting from bytes to pubkey takes some time).

@nkohen
Copy link
Author

nkohen commented Dec 13, 2019

In the sig_point case, do we expect to do more crypto operations with the nonce? If not, we can just use bytes too. Otherwise, we may want to add a type (because converting from bytes to pubkey takes some time).

I think we expect to do some simple additions since you're supposed to add it to the user's pubkey to hide the fact that you are using the oracle should the point end up on-chain

@jonasnick
Copy link
Owner

Hm, the "sig_point" would be added to the user's pubkey, not the public nonce.

@nkohen
Copy link
Author

nkohen commented Dec 13, 2019

Oh, I see, I misunderstood. All the public nonce is used for is computing the sig_point which involves some crypto operations doesn't it?

@jonasnick
Copy link
Owner

I think so. And if that's indeed the case we can replace secp256k1_pubkey pubnonce with unsigned char* pubnonce in the public API.

@jonasnick jonasnick force-pushed the schnorrsig branch 2 times, most recently from 90b5915 to 23c3b00 Compare February 12, 2020 22:54
@nkohen
Copy link
Author

nkohen commented Apr 22, 2020

Replaced by #16

@nkohen nkohen closed this Apr 22, 2020
jonasnick pushed a commit that referenced this pull request Nov 4, 2024
… names

87384f5 cmake, test: Add `secp256k1_` prefix to test names (Hennadii Stepanov)

Pull request description:

  This PR improves regex matching options when using `ctest` in downstream projects, such as Bitcoin Core.

  For instance, a downstream project users can filter their tests like that:
  ```
  ctest --tests-regex "secp256k1"
  ```
  or
  ```
  ctest --exclude-regex "secp256k1"
  ```

  A `ctest` log with this PR:
  ```
  $ ctest --test-dir build -j 16
  Internal ctest changing into directory: /home/hebasto/git/secp256k1/secp256k1/build
  Test project /home/hebasto/git/secp256k1/secp256k1/build
      Start 1: secp256k1_noverify_tests
      Start 2: secp256k1_tests
      Start 3: secp256k1_exhaustive_tests
      Start 4: secp256k1_ecdsa_example
      Start 5: secp256k1_ecdh_example
      Start 6: secp256k1_schnorr_example
      Start 7: secp256k1_ellswift_example
      Start 8: secp256k1_musig_example
  1/8 Test #4: secp256k1_ecdsa_example ..........   Passed    0.00 sec
  2/8 Test #5: secp256k1_ecdh_example ...........   Passed    0.00 sec
  3/8 Test #6: secp256k1_schnorr_example ........   Passed    0.00 sec
  4/8 Test #7: secp256k1_ellswift_example .......   Passed    0.00 sec
  5/8 Test #8: secp256k1_musig_example ..........   Passed    0.00 sec
  6/8 Test #3: secp256k1_exhaustive_tests .......   Passed    6.19 sec
  7/8 Test #1: secp256k1_noverify_tests .........   Passed   38.83 sec
  8/8 Test #2: secp256k1_tests ..................   Passed   91.66 sec

  100% tests passed, 0 tests failed out of 8

  Total Test time (real) =  91.67 sec
  ```

ACKs for top commit:
  theuni:
    utACK 87384f5
  real-or-random:
    utACK 87384f5

Tree-SHA512: d8e46558cf58c9c660544b7bdfed24c991eb3e120b6511aa3968f509190130e498749a3c4dbabc87a7f22f0aa0056c6bcd3fc6c44f5eb131588945d593546840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants