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

Use upstream rust-secp256k1 #616

Merged
merged 3 commits into from
Nov 9, 2018

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Nov 8, 2018

The question implied here is: why did we fork rust-secp256k1 and are those reasons still valid? Can we get back to the mainline crate? There is ongoing work on our fork to make it no_std but I'm not sure what the end-goal of that effort is.

Part of #593

@dvdplm dvdplm requested a review from arkpar November 8, 2018 09:10
@burdges
Copy link

burdges commented Nov 8, 2018

Anyone looked into https://docs.rs/libsecp256k1/0.1.14/secp256k1/ ? I have not done so yet.

@dvdplm dvdplm self-assigned this Nov 8, 2018
@tomaka
Copy link
Member

tomaka commented Nov 8, 2018

We need to make sure either that this doesn't break substrate, or that substrate can be upgraded as well.

@arkpar
Copy link
Contributor

arkpar commented Nov 8, 2018

The original reason to fork is this issue: bitcoin-core/secp256k1#352
After that additional changes were added to support secret store I believe.
cc @svyatonik

If libp2p does not need to support ECDH with anything else other than SHA256, or it does not use ECDH in the first place, then it's fine to use upstream I would guess.

@tomaka
Copy link
Member

tomaka commented Nov 8, 2018

We're not using ECDH with secp256k1.
I was mostly worried about symbols defined twice for example, that would trigger a compilation error in substrate.

@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 9, 2018

Substrate compiles and tests run green.

@tomaka tomaka merged commit 3e1eca1 into libp2p:master Nov 9, 2018
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Nov 14, 2018
…e-handled_node_tasks

* upstream/master:
  Tweaks, spelling and grammar (libp2p#629)
  Add a badge with a link to deps.rs (libp2p#630)
  Rewrite floodsub to use the ProtocolsHandler (libp2p#603)
  Add an IdentifyListen behaviour (libp2p#626)
  Add a custom derive for NetworkBehaviour (libp2p#619)
  Set the maximum size of Mplex messages to 1Mb (libp2p#622)
  Use expect rather than unwrap (libp2p#625)
  Make libp2p-websocket optional (libp2p#624)
  Add From<IpAddr> for Multiaddr (libp2p#623)
  Add implementations of NetworkBehaviour for ping (libp2p#618)
  Add a PeriodicIdentifyBehaviour (libp2p#617)
  Use upstream rust-secp256k1 (libp2p#616)
  Use yamux and aio-limited from crates.io (libp2p#621)
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Nov 14, 2018
…st-collection_stream

* dp/chore/test-core-handled_node_tasks: (24 commits)
  Revert changes to Debug impl for HandledNodesTasks
  whitespace
  Don't organise tests in submodules
  Rename test builders
  Tweaks, spelling and grammar (libp2p#629)
  Add a badge with a link to deps.rs (libp2p#630)
  Rewrite floodsub to use the ProtocolsHandler (libp2p#603)
  Add an IdentifyListen behaviour (libp2p#626)
  Add a custom derive for NetworkBehaviour (libp2p#619)
  Set the maximum size of Mplex messages to 1Mb (libp2p#622)
  Use expect rather than unwrap (libp2p#625)
  Make libp2p-websocket optional (libp2p#624)
  Add From<IpAddr> for Multiaddr (libp2p#623)
  Add implementations of NetworkBehaviour for ping (libp2p#618)
  Add a PeriodicIdentifyBehaviour (libp2p#617)
  Use upstream rust-secp256k1 (libp2p#616)
  Use yamux and aio-limited from crates.io (libp2p#621)
  Remove tests for Task we don't need Test Task.send_event() and id() using a HandledNodesTasks
  Better debug impl for HandledNodesTasks
  Address grumbles
  ...
@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 16, 2018

Anyone looked into https://docs.rs/libsecp256k1/0.1.14/secp256k1/ ? I have not done so yet.

@burdges I did, briefly, and spoke to the author. It is incomplete atm so it'd require some work to extend the API to work for secio, it is not audited and has not seen production use. It is definitely an interesting lib and over time might become a good replacement candidate but in its current state we can't use it.

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.

None yet

4 participants