Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Introducing Node for py-libp2p #795

Merged
merged 13 commits into from
Jul 21, 2019
Merged

Conversation

mhchia
Copy link
Contributor

@mhchia mhchia commented Jul 11, 2019

What was wrong?

  • The first step of migrating from the existing p2p code to py-libp2p.
  • The local package name libp2p conflicts with the dependency libp2p/py-libp2p.

How was it fixed?

  • Add Node to contain the components in py-libp2p we need.
  • Add tests.
  • Remove libp2p_daemon_bindings and its tests.

To-Do

  • Add needed API and components in Node.
  • Test, test, test.

Cute Animal Picture

put a cute animal picture link inside the parentheses

tests/libp2p/bcc/conftest.py Outdated Show resolved Hide resolved
tests/libp2p/bcc/conftest.py Outdated Show resolved Hide resolved
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Yes, your factory approach looks like what I was suggesting. 👍

tests/libp2p/bcc/conftest.py Outdated Show resolved Hide resolved
@mhchia
Copy link
Contributor Author

mhchia commented Jul 16, 2019

Will fix the CI tomorrow and this PR should be ready by the time. 🙏

@mhchia mhchia changed the title [WIP] Introducing Node for py-libp2p Introducing Node for py-libp2p Jul 17, 2019
@mhchia
Copy link
Contributor Author

mhchia commented Jul 17, 2019

Should be ready now.

mhchia added 11 commits July 19, 2019 13:59
To avoid package name conflicts with py-libp2p
Along with
- configs.py: configs for libp2p protocols standard
- utils.py: identity generation and protocol id helpers for now
- And the incomplete tests
- Add `Node` service.
- Add tests for `dial_peer`.
Generate the testing nodes with the factory.
- Add `NodeFactory.create_batch`
- Add `try/finally` to the fixture `nodes` to ensure nodes are close even when
the generator is closed
And modify tox.ini
Along with the deps in setup.py
Copy link
Contributor

@NIC619 NIC619 left a comment

Choose a reason for hiding this comment

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

LGTM. Just some questions and nit.

muxer_opt=muxer_protocol_ids,
sec_opt=security_protocol_ops,
peerstore_opt=None, # let the function initialize it
disc_opt=None, # no routing required here
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is this a temporary thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will be used for a while because we will use discv5 instead of the Kademlia in libp2p for the discovery.


MULTIPLEXING_PROTOCOL_ID = "/mplex/6.7.0"

PUBSUB_PROTOCOL_ID = "/eth/serenity/gossipsub/1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be GOSSIPSUB_PROTOCOL_ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually mean "the protocol id of pubsub we use here". Given this context, does PUBSUB_PROTOCOL_ID make sense?

)
assert nodes[0].peer_id in nodes[1].host.get_network().connections
assert nodes[1].peer_id in nodes[0].host.get_network().connections
# Test: Reuse the old connection when connecting again
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The wording here sounds like it is reconnecting after disconnecting. What about
# Test: Second dial to a connected peer does not open a new connection

- Change `PUBSUB_PROTOCOL_ID` to `GOSSIPSUB_PROTOCOL_ID`
- Reword the description of a test
@mhchia mhchia merged commit 3a5b9ef into ethereum:master Jul 21, 2019
@mhchia mhchia deleted the feature/pylibp2p-node branch July 21, 2019 10:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants