Add basic support for multiaddr addresses and improvement around peer id #75

zaibon · 2018-11-26T11:59:59Z

relates to #65

I'm already creating this PR so my fork doesn't diverge too far away from here.

alexh · 2018-11-26T17:11:40Z

network/swarm.py

        # Get peer info from peer store
        addrs = self.peerstore.addrs(peer_id)

        if not addrs:
            raise SwarmException("No known addresses to peer")

+        # TODO: define logic to choose which address to use, or try them all ?


I believe we should try them all and use whichever address responds the fastest

This is what the go-libp2p repo is doing at the moment, and there are plans to upgrade it to a smarter dial. I forgot if it's called a traffic shaper or a dial manager. There is a bit of design freedom here. We should participate in the discussion with the rest of the community.

@zixuanzh do you have a link to the issue where they discuss this dial manager ?

@zaibon this is the thread libp2p/go-libp2p-swarm#88, dial manager might be a make-up name.

requirements.txt

stuckinaboot · 2018-11-26T17:18:33Z

Hi @zaibon, is this PR intented to be merged right now? Some of the tests are failing, please fix before merging

zaibon · 2018-11-26T17:49:04Z

@stuckinaboot
as explained at #65 (comment)
The multiaddr lib needs to be moved to the proper account and probably updated with my fixes too before we can merge this.

With the proper lib installed, I don't have any tests failing, can you point me to which test is not working with you ?

Resolved.

zixuanzh

@zaibon Makes sense, let us check with Raul and see if there are any updates there about ownership transfer of py-multiaddr to libp2p but I think it might still take some time. In the mean time, how can we point to your py-multiaddr fork? We don't see that in your py-libp2p fork either. If we can try this, then we can retest locally.

zaibon · 2018-11-26T18:14:08Z

What I did is locally install my fork: https://github.com/zaibon/py-multiaddr
then I develop the rest based on this multiaddr code

robzajac · 2018-11-26T18:24:43Z

@zaibon I'm trying to recreate this setup - what are the steps I should follow to locally install the fork? Thanks!

zaibon · 2018-11-26T18:39:30Z

@robzajac

cd py-libp2p
source venv/bin/activate
cd ..
git clone  https://github.com/zaibon/py-multiaddr
cd py-multiaddr
pip install .
cd ../py-libp2p
pytest -v

robzajac · 2018-11-28T19:50:24Z

Hi @zaibon apologies for the pause in communication. Recently we were focusing on pushing up big design improvements over our PoC (#82 and #83).

We have reached out to @raulk regarding the discussion in #65, particularly transferring ownership of sbuss/py-multiaddr to libp2p and accepting the changes in your fork. We haven't heard back yet, but I think we're ready to use your py-multiaddr fork in this repo.

Is there a way to integrate your py-multiaddr fork into our pip dependencies automatically, so that other contributors do not have to manually follow the steps above? Critically, we'd also like this fork to be pulled in as a dependency during CI. If we can have that set up and have this PR pass in CI I think we will be ready to merge.

Thanks for your great work!

zaibon · 2018-11-28T22:57:26Z

Hi @robzajac, no worries, I see you're making good progress here.

Regarding my fork. I can publish it to pypi so it can be easily integrated with pip and CI. But I guess this will be temporary until we move sbuss/py-multiaddr since I won't be able to use the same package name. But that can be done, no problem.

no need to cast from a string to an ID

- keep multiaddr object into peerstore instead of string - update network code to use new multiaddr lib - update tests and example

This has side effect where the same peerstore is used for different instance of Libp2p

codecov-io · 2018-11-29T00:52:50Z

Codecov Report

Merging #75 into master will increase coverage by 0.44%.
The diff coverage is 95.96%.

@@            Coverage Diff            @@
##           master     #75      +/-   ##
=========================================
+ Coverage   53.45%   53.9%   +0.44%     
=========================================
  Files          49      51       +2     
  Lines        1463    1551      +88     
=========================================
+ Hits          782     836      +54     
- Misses        681     715      +34

Impacted Files	Coverage Δ
libp2p/libp2p.py	`100% <100%> (ø)`	⬆️
network/network_interface.py	`100% <100%> (ø)`	⬆️
host/basic_host.py	`96.66% <100%> (+8.43%)`	⬆️
host/host_interface.py	`100% <100%> (ø)`	⬆️
tests/protocol_muxer/test_protocol_muxer.py	`100% <100%> (ø)`	⬆️
tests/peer/test_peerid.py	`100% <100%> (ø)`
tests/libp2p/test_libp2p.py	`100% <100%> (ø)`	⬆️
peer/peerstore.py	`98.24% <100%> (ø)`	⬆️
tests/peer/test_peerinfo.py	`100% <100%> (ø)`
transport/tcp/tcp.py	`78.37% <100%> (+0.87%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fa674d...0bd6e88. Read the comment docs.

zaibon · 2018-11-29T00:54:46Z

@robzajac ok I think it's ready now. Please have another look if I didn't mess up with the code. I had to rebase a few commits.

robzajac

LGTM! I will let one of @alexh @zixuanzh @stuckinaboot take a look as well.

robzajac · 2018-11-29T05:07:02Z

host/basic_host.py

@@ -56,3 +69,22 @@ def set_stream_handler(self, protocol_id, stream_handler):
        """
        stream = await self.network.new_stream(peer_id, protocol_ids)
        return stream
+
+    async def connect(self, peer_info):


Adding reference if anyone is wondering where this comes from.

zixuanzh

LGTM as well, thank you for the great work!

* new issue and pr templates

alexh reviewed Nov 26, 2018

View reviewed changes

robzajac previously requested changes Nov 26, 2018

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

stuckinaboot closed this Nov 26, 2018

stuckinaboot reopened this Nov 26, 2018

zixuanzh reviewed Nov 26, 2018

View reviewed changes

zaibon added 7 commits November 29, 2018 00:59

Improved peer ID construction and usage

6e209e5

peer id object is directly passed to the network

0bb3686

no need to cast from a string to an ID

don't base64 encode the peer id when loading from public key

3ee5fb4

use proper multiaddr address

5a46566

- keep multiaddr object into peerstore instead of string - update network code to use new multiaddr lib - update tests and example

don't instanciate peerstore object in constructor

37e862b

This has side effect where the same peerstore is used for different instance of Libp2p

add connect method to basic_host

3d43726

use zaibon's fork of sbuss/py-multiaddr

eebcdef

zaibon force-pushed the multiaddr branch from 175844c to 2322d99 Compare November 29, 2018 00:43

lint

0bd6e88

zaibon force-pushed the multiaddr branch from 2322d99 to 0bd6e88 Compare November 29, 2018 00:51

robzajac approved these changes Nov 29, 2018

View reviewed changes

zixuanzh approved these changes Nov 29, 2018

View reviewed changes

robzajac merged commit 611de28 into libp2p:master Nov 29, 2018

zaibon deleted the multiaddr branch September 18, 2019 18:05

pacrob added a commit to pacrob/py-libp2p that referenced this pull request Feb 19, 2024

new github issue and pr templates (libp2p#75)

89e5d8e

* new issue and pr templates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for multiaddr addresses and improvement around peer id #75

Add basic support for multiaddr addresses and improvement around peer id #75

zaibon commented Nov 26, 2018

alexh Nov 26, 2018

zixuanzh Nov 26, 2018

zaibon Nov 26, 2018

zixuanzh Nov 26, 2018 •

edited

Loading

stuckinaboot commented Nov 26, 2018

zaibon commented Nov 26, 2018

zixuanzh left a comment •

edited

Loading

zaibon commented Nov 26, 2018

robzajac commented Nov 26, 2018

zaibon commented Nov 26, 2018

robzajac commented Nov 28, 2018 •

edited

Loading

zaibon commented Nov 28, 2018

codecov-io commented Nov 29, 2018

zaibon commented Nov 29, 2018

robzajac left a comment

robzajac Nov 29, 2018

zixuanzh left a comment

Add basic support for multiaddr addresses and improvement around peer id #75

Add basic support for multiaddr addresses and improvement around peer id #75

Conversation

zaibon commented Nov 26, 2018

alexh Nov 26, 2018

Choose a reason for hiding this comment

zixuanzh Nov 26, 2018

Choose a reason for hiding this comment

zaibon Nov 26, 2018

Choose a reason for hiding this comment

zixuanzh Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

stuckinaboot commented Nov 26, 2018

zaibon commented Nov 26, 2018

zixuanzh left a comment • edited Loading

Choose a reason for hiding this comment

zaibon commented Nov 26, 2018

robzajac commented Nov 26, 2018

zaibon commented Nov 26, 2018

robzajac commented Nov 28, 2018 • edited Loading

zaibon commented Nov 28, 2018

codecov-io commented Nov 29, 2018

Codecov Report

zaibon commented Nov 29, 2018

robzajac left a comment

Choose a reason for hiding this comment

robzajac Nov 29, 2018

Choose a reason for hiding this comment

zixuanzh left a comment

Choose a reason for hiding this comment

zixuanzh Nov 26, 2018 •

edited

Loading

zixuanzh left a comment •

edited

Loading

robzajac commented Nov 28, 2018 •

edited

Loading