Skip to content
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

Secp256k1 support #13

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

ysangkok
Copy link

This needs to be squashed, and there are some minors issues, but I thought you might want to comment on the approach.

@ysangkok
Copy link
Author

I am stuck on this because there seems to be no way to tell hpack that secp256k1-haskell needs to be built with the ECDH build flag, see sol/hpack#372

Copy link
Collaborator

@centromere centromere left a comment

Choose a reason for hiding this comment

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

I would prefer to not have Lightning network specific logic in the main code base. Shouldn't it be as simple as dropping in a new module, as you've done in src/Crypto/Noise/DH/Secp256k1.hs?

@ysangkok
Copy link
Author

@centromere Thanks for taking a look Alex! I would be happy to change the code, but I don't know how to accomplish this, since the rekeying scheme of Lightning makes the chaining keys diverge between sending/receiving. The codebase, as it is, only has one chaining key ssck. Do you have any tips on how you'd imagine it working? Of course the chaining keys only diverge after the first Lightning rekeying (which happens independently for sending/receiving), so the changes to SymmetricState.hs are only necessary because the types do not reflect whether the chaining keys have diverged yet. But it would be a pretty big change to create separate types to have the methods in SymmetricState.hs only work on objects that contain a single chaining key. Furthermore, the system, as it is now, allows you to combine the Noise spec rekeying and the Lightning rekeying. It would require a lot more code to forbid this (which would make sense, but I don't think it is worth the complexity).

@ysangkok
Copy link
Author

@centromere Any suggestions how I can proceed?

@centromere
Copy link
Collaborator

I am trying to understand the Lightning spec a bit more, in relation to the proposed change. Is this what I should be reading?

@ysangkok
Copy link
Author

Yep, that is the spec and the relevant section.

@ysangkok
Copy link
Author

@centromere If there is any way I can help, please let me know.

@centromere
Copy link
Collaborator

I see two separate issues to be considered. The first is whether to include secp256k1. The second is to have independent chaining keys for sending and receiving. I am okay with adding secp256k1. I am not yet sold on the idea of changing something this fundamental (a single CK) about Noise's design, but I am willing to listen.

Will you be at RWC2020?

@ysangkok
Copy link
Author

@centromere I asked on #lightning-dev @ Freenode (IRC) and Roasbeef replied (he is one of the spec authors): http://gnusha.org/lightning-dev/2019-12-23.log (see around 11:25) I'll copy it here:

11:25 <+roasbeef> ysangkok: I brought it up on the noise mailing list back when i was drafting brontide, trevor seemed to be ok w/ it, can prob search the list (was 2 yrs ago or so) to find his original post 
11:26 <+roasbeef> i think since then they might've made some chnages (evolved pretty quickly) to specify a diff key rotation (or maybe the same, haven't been keeping up with it too closely lately) 
11:29 <+roasbeef> ysangkok: here's the post https://moderncrypto.org/mail-archive/noise/2017/000869.html
11:29 <+roasbeef> hmm actually don't think that's the right one 
13:35 <+roasbeef> ysangkok: oh reading your msg again, that section was added _after_ we adopted rekying into brontide, the section was actually inspired by our work as the noise spec previously didn't cover it 
13:37 <+roasbeef> even that section is just advisory, it advises use a one way func, we use hdkf w/ the prior state, nothing super glamarous or hard to reason about

Also note that this protocol is being used on a lot of public nodes on the internet, that are indexed by sites such as https://1ml.com/

@centromere
Copy link
Collaborator

@ysangkok Would you make a dedicated post on the mailing list regarding having separate chaining keys for sending and receiving? I do not feel qualified to evaluate the soundness of this approach.

@ysangkok
Copy link
Author

ysangkok commented Jan 9, 2020

@centromere I have sent a mail to the list, but it may take a few days to appear because of greylisting.

@ysangkok
Copy link
Author

ysangkok commented Mar 3, 2020

@centromere as always, please let me know if you have any questions

@Kleidukos
Copy link
Member

Regarding new functions and datatypes, I would encourage you to add @since annotations as Haddock comments @ysangkok :)

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