Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Integrate the native LibP2P implementation #632

Merged
merged 4 commits into from Dec 10, 2019
Merged

Integrate the native LibP2P implementation #632

merged 4 commits into from Dec 10, 2019

Conversation

@zah
Copy link
Member

zah commented Dec 9, 2019

Local network sim (and testnet1) will now use the native libp2p by default.

@tersec

This comment has been minimized.

Copy link
Contributor

tersec commented Dec 9, 2019

Probably worth using this for #122 at this point, rather than implementing it twice.

@zah zah force-pushed the native-libp2p branch from 4274db0 to dc44e8f Dec 10, 2019
zah added 4 commits Nov 3, 2019
This hasn't been tested yet even in local sim.
@zah zah force-pushed the native-libp2p branch from dc44e8f to 012d2c2 Dec 10, 2019
template libp2pProtocol*(name: string, version: int) {.pragma.}
# TODO: This exists only as a compatibility layer between the daemon
# APIs and the native LibP2P ones. It won't be necessary once the
# daemon is removed.

This comment has been minimized.

Copy link
@arnetheduck

arnetheduck Dec 10, 2019

Member

or we can move some of it to the libp2p library - I believe there was interest in maintaining both daemon and non-daemon versions (though generally I'd prefer we drop the daemon api completely and focus our limited resources on making the native version excellent)

This comment has been minimized.

Copy link
@tersec

tersec Dec 10, 2019

Contributor

Supporting both also means keeping a significantly more complex build setup and build dependency documentation, due to the daemon being in Go. I'd prefer to drop all that when feasible.

This comment has been minimized.

Copy link
@zah

zah Dec 10, 2019

Author Member

I'm very eager to drop the daemon as soon as possible, because it's holding back the API in some sense (the daemon is not providing enough events to which you can hook up and it cannot be interrogated regarding the supported protocols of another peer).

To do this, we need to be more confident that we have working interop with all other clients though. Also, with the current state of nim-libp2p, we are back to the star topology that we managed to escape with the daemon. Discovery V5 should solve this.

This comment has been minimized.

Copy link
@stefantalpalaru

stefantalpalaru Dec 10, 2019

Contributor

Keeping the Go daemon wrapper helps with interoperability tests, unless you want to write half of them in Go.

This comment has been minimized.

Copy link
@zah

zah Dec 10, 2019

Author Member

Having the daemon wrapper as an optional module is perfectly fine. I was referring to the limitations imposed from trying to maintain the same facade API in front of the native implementation and the daemon.

if not stream.closed:
await close(stream)

include eth/p2p/p2p_backends_helpers

This comment has been minimized.

Copy link
@arnetheduck

arnetheduck Dec 10, 2019

Member

any way we can avoid the includes? they are difficult for tooling to analyze

This comment has been minimized.

Copy link
@arnetheduck

arnetheduck Dec 10, 2019

Member

as an alternative, let's at least start calling them something like .inc.nim so they're clearly marked / can be excluded

This comment has been minimized.

Copy link
@zah

zah Dec 10, 2019

Author Member

The purpose of these includes is to simulate "generic code" without introducing actual generics. In the included file, types such as P2PStream will be different depending on the backend file that uses it. If all the code was generic instead, this would have lead to a more baroque structure and slightly worse compilation error messages, so I think the current trade-off is worthwhile.

@zah zah merged commit 10e6d48 into devel Dec 10, 2019
7 checks passed
7 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jenkins/nim-beacon-chain/prs This commit looks good
Details
status-im.nim-beacon-chain Build #20191210.13 succeeded
Details
status-im.nim-beacon-chain #20191210.14 succeeded
Details
status-im.nim-beacon-chain (Windows 32-bit) Windows 32-bit succeeded
Details
status-im.nim-beacon-chain (Windows 64-bit) Windows 64-bit succeeded
Details
@delete-merged-branch delete-merged-branch bot deleted the native-libp2p branch Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.