Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: libp2p noise as fallback for secio #3074

Merged
merged 3 commits into from
Jun 22, 2020
Merged

Conversation

vasco-santos
Copy link
Member

This PR adds libp2p-noise encryption protocol as a fallback for libp2p-secio. This replicates what happened in ipfs/go-ipfs/releases/v0.5.0.

SECIO Deprecation Notice
SECIO should be considered to be well on the way to deprecation and will be
completely disabled in either the next release (0.6.0, ~mid May) or the one
following that (0.7.0, ~end of June). Before SECIO is disabled, support will be
added for the NOISE transport for compatibility with other IPFS implementations.

From what I know, this is likely to happen for 0.7.0.

We currently have interop tests for using noise in libp2p/interop/test/connect. I think there are enough for now, but when we use noise as default in JS and GO, we will definitely want to make sure we have more verbose interop before we switch to default. AFAIK, with the current setup of ipfsd-ctl we cannot specify what libp2p modules to use.

@@ -13,6 +13,7 @@
"author": "",
"license": "ISC",
"devDependencies": {
"@babel/plugin-transform-runtime": "^7.10.1",
Copy link
Member Author

@vasco-santos vasco-santos Jun 9, 2020

Choose a reason for hiding this comment

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

I was getting regeneratorRuntime is not defined from the browser bundle from libp2p-noise. This solves the issue here. Is this somethig that we want here?
Is there any other place that we should document this?

Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed, what is libp2p-noise doing to force this?

Copy link
Member

Choose a reason for hiding this comment

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

libp2p-noise is running babel in published code they really should not!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will open an issue on libp2p-noise

Copy link
Member

@hugomrdias hugomrdias Jun 9, 2020

Choose a reason for hiding this comment

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

If they use aegir they can avoid all the tooling besides typescript.
So if they run ts to just compile to js into a src folder they can then run aegir the same way all others repos do.

Choose a reason for hiding this comment

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

libp2p-noise is running babel in published code they really should not!

We use babel to transpile from ts to js so we really have to run it. Seems like my targets in browserlist in babel includes browser that doesn't support async generator so babel relied on global regeneratorRuntime. Ref: babel/babel#9849 (comment)

I will disable this, which will probably result in slightly larger generated codesize but it should work without hacks here.

I would really like to use aegir but I don't wan't to generate ts into src directory, because I would have to commit src directory with generated code to satisfy aegir. We will integrate aegir once there is first class support for typescript codebase. Currently from src (typescript) we generate using babel build directory with node optimized code and lib directory with esmodules to enable tree shaking in web apps.

Copy link
Member

Choose a reason for hiding this comment

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

You really shouldn't run babel, you are making decisions for your users and publishing non optimal code. Only the users know their targets.

IMO you should run TS to compile to JS without any transformation except types obviously. You can still tell TS to output esm and cjs. If your only concern about esm is tree shaking you probably can just skip it, i doubt it makes any difference.

Regarding aegir, it doesn't care if the code is committed or not. The only problem that I see is that you are already using a folder called src for TS and that's kinda hard coded in aegir. I can make it configurable for you, that should solve your problem.

Choose a reason for hiding this comment

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

You really shouldn't run babel, you are making decisions for your users and publishing non optimal code. Only the users know their targets.

Aegir is doing same thing, it's using babel to target some browsers and nodejs and then using webpack creating umd build that can run in both. I generally don't like to do this because it's replacing some js features with ugly (and slower) hacks to accomodate browsers which doesn't support that (async generators for example).

IMO you should run TS to compile to JS without any transformation except types obviously. You can still tell TS to output esm and cjs. If your only concern about esm is tree shaking you probably can just skip it, i doubt it makes any difference.

That won't work in nodejs since it doesn't support ECMAScript Modules

Regarding aegir, it doesn't care if the code is committed or not. The only problem that I see is that you are already using a folder called src for TS and that's kinda hard coded in aegir. I can make it configurable for you, that should solve your problem.

If you enable configurable source we will enable aegir and switch to umd build.

FYI I've set aegir babel browsers targets in libp2p noise and it seems to pass all example tests locally: ChainSafe/js-libp2p-noise#62

Copy link
Member

Choose a reason for hiding this comment

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

Aegir is doing same thing, it's using babel to target some browsers and nodejs and then using webpack creating umd build that can run in both. I generally don't like to do this because it's replacing some js features with ugly (and slower) hacks to accomodate browsers which doesn't support that (async generators for example).

Yes aegir runs babel with browserlist, but this is only because we publish a special browser bundle to be used with script tags directly using js cdns like unpkg or jsdelivr, NOT to be bundled again by users!
The main prop in package.json always points to src/index.js

That won't work in nodejs since it doesn't support ECMAScript Modules

Yes, thats why i said to run TS twice one for esm and another for cjs, package.json main would point to src-cjs/index.js and module to src-esm/index.js. Or just have cjs cause esm in your case likely adds 0 value.

If you enable configurable source we will enable aegir and switch to umd build.

I will make this happen asap.

@vasco-santos vasco-santos force-pushed the feat/libp2p-noise branch 2 times, most recently from cf58962 to 1748b8b Compare June 9, 2020 12:34
@@ -1,3 +1,4 @@
module.exports = {
presets: ['@vue/app'],
plugins: ['@babel/plugin-transform-runtime']
Copy link
Member Author

Choose a reason for hiding this comment

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

While this fixed the issue in the remaining examples, I could not make this work in Vue. It seems that this plugin is not correctly getting to the babel config

@@ -15,7 +15,7 @@ let sigServerB
let ipfsdServer

module.exports = {
bundlesize: { maxSize: '446kB' },
bundlesize: { maxSize: '530kB' },
Copy link
Member

Choose a reason for hiding this comment

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

This is almost a 20% increase in bundle size. Is there any way this can be made smaller?

Copy link

@mpetrunic mpetrunic Jun 16, 2020

Choose a reason for hiding this comment

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

@jacobheun said, he's gonna improve libp2p-crypto with some ciphers we need for noise. We are currently using bcrypto but it's really heavy

Copy link
Member Author

Choose a reason for hiding this comment

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

We will need to get this into js-ipfs before go-ipfs removes secio entirely.
The plan is to work on improving the bundle size with a better solution for libp2p crypto, kind of bring what you need like the rest of libp2p. However, I think that this is something that me or @jacobheun will not be able to get soon.
Some context: libp2p/js-libp2p#586

Copy link
Member Author

Choose a reason for hiding this comment

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

ETAs for secio: libp2p/js-libp2p#615

@achingbrain achingbrain merged commit 660d3db into master Jun 22, 2020
@achingbrain achingbrain deleted the feat/libp2p-noise branch June 22, 2020 12:20
jacobheun pushed a commit that referenced this pull request Aug 5, 2020
Adds `libp2p-noise` encryption protocol as a fallback for `libp2p-secio`. This replicates what happened in [ipfs/go-ipfs/releases/v0.5.0](https://github.com/ipfs/go-ipfs/releases/tag/v0.5.0).

>SECIO Deprecation Notice
>SECIO should be considered to be well on the way to deprecation and will be
>completely disabled in either the next release (0.6.0, ~mid May) or the one
>following that (0.7.0, ~end of June). Before SECIO is disabled, support will be
>added for the NOISE transport for compatibility with other IPFS implementations.

From what I know, this is likely to happen for `0.7.0`.

We currently have interop tests for using noise in [libp2p/interop/test/connect](https://github.com/libp2p/interop/tree/master/test/connect). I think there are enough for now, but when we use noise as default in JS and GO, we will definitely want to make sure we have more verbose interop before we switch to default. AFAIK, with the current setup of `ipfsd-ctl` we cannot specify what libp2p modules to use.

Co-authored-by: Alex Potsides <alex@achingbrain.net>
achingbrain added a commit that referenced this pull request Aug 11, 2020
Adds `libp2p-noise` encryption protocol as a fallback for `libp2p-secio`. This replicates what happened in [ipfs/go-ipfs/releases/v0.5.0](https://github.com/ipfs/go-ipfs/releases/tag/v0.5.0).

>SECIO Deprecation Notice
>SECIO should be considered to be well on the way to deprecation and will be
>completely disabled in either the next release (0.6.0, ~mid May) or the one
>following that (0.7.0, ~end of June). Before SECIO is disabled, support will be
>added for the NOISE transport for compatibility with other IPFS implementations.

From what I know, this is likely to happen for `0.7.0`.

We currently have interop tests for using noise in [libp2p/interop/test/connect](https://github.com/libp2p/interop/tree/master/test/connect). I think there are enough for now, but when we use noise as default in JS and GO, we will definitely want to make sure we have more verbose interop before we switch to default. AFAIK, with the current setup of `ipfsd-ctl` we cannot specify what libp2p modules to use.

Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
achingbrain added a commit that referenced this pull request Aug 12, 2020
Adds `libp2p-noise` encryption protocol as a fallback for `libp2p-secio`. This replicates what happened in [ipfs/go-ipfs/releases/v0.5.0](https://github.com/ipfs/go-ipfs/releases/tag/v0.5.0).

>SECIO Deprecation Notice
>SECIO should be considered to be well on the way to deprecation and will be
>completely disabled in either the next release (0.6.0, ~mid May) or the one
>following that (0.7.0, ~end of June). Before SECIO is disabled, support will be
>added for the NOISE transport for compatibility with other IPFS implementations.

From what I know, this is likely to happen for `0.7.0`.

We currently have interop tests for using noise in [libp2p/interop/test/connect](https://github.com/libp2p/interop/tree/master/test/connect). I think there are enough for now, but when we use noise as default in JS and GO, we will definitely want to make sure we have more verbose interop before we switch to default. AFAIK, with the current setup of `ipfsd-ctl` we cannot specify what libp2p modules to use.

Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants