feat: address management #612

vasco-santos · 2020-04-20T14:16:23Z

This PR adds an Address Management component. This component aims to allow users to add to the libp2p configuration { listen, announce, noAnnounce } addresses.

These addresses will be used by each subsystem according to their needs. The details for the reasoning for these addresses are detailed on /src/address-manager/README.md, which should be read in this branch (last commit)

Needs:

Address Manager tests
Remove multiaddrs from discovery
Consider Listen addresses update on noAnnounce addresses filtering
Improve configuration doc

Closes #202

Depends on:

vasco-santos · 2020-04-20T14:19:24Z

cc @jacobheun

Once you have time, please check this out and let me know your thoughs. Go to the feat: address manager commit and look into:

Address Manager Readme
API Readme
src/address-manager/index.js: there is one TODO comment

jacobheun

Added some thoughts on how advertised addresses could be handled.

src/address-manager/index.js

jacobheun · 2020-04-24T16:32:08Z

src/address-manager/index.js

+   * @param {Array<Multiaddr>} newMultiaddrs
+   * @return {Array<Multiaddr>}
+   */
+  _replaceListen (newMultiaddrs) {


I don't think we should do this, it makes tracking things more difficult and since we're not tracking everything in PeerInfo this is a bit easier to manage. We should keep the listen, announce, and noAnnounce untouched. They could be added to, but we shouldn't internally replace them, these should be user controlled.

We should have a method that computes everything for advertised addresses, this could live on libp2p itself. libp2p.getAddrs() or similar. This method would do something like:

Get the TransportManager addresses, libp2p.transportManager.getAddrs()

Add in the announce addresses, [...libp2p.transportManager.getAddrs(), ...libp2p.addressManager.announce]

And then filter out matching noAnnounce and any duplicate addresses

The big advantage of this is that we are able to update addresses over time and will always be able to reference our original addresses. Things like Identify will just be able to call this method when they need the latest addresses.

Also with this, I think we can simplify this class; get rid of the getters and just expose the properties.

This solution seems good to me and I will try it out. My main concern here is the same issue that I still have to solve: noAnnounce addresses using random ports for filtering. Unless the match criteria does not consider the port, we would need to make a comparison between the provided listenAddr and what is in libp2p.transportManager.getAddrs()

changed accordingly

This solution seems good to me and I will try it out. My main concern here is the same issue that I still have to solve: noAnnounce addresses using random ports for filtering

Not sure I follow. Are you concerned that a listening address of /ip4/0.0.0.0/tcp/0 can't be filtered with a noAnnounce of /ip4/0.0.0.0/tcp/0? If so, I don't think this matters and users shouldn't be trying to do this. While this is helpful for testing, listening on random ports is not recommended for production usage. The only noAnnounce "wildcard" I think would be nice to have is filtering loopback IP addresses, but that could be checked by the app before configuring libp2p.

Yes. I already added support for that now, but it is a bit messy 😅

The only noAnnounce "wildcard" I think would be nice to have is filtering loopback IP addresses, but that could be checked by the app before configuring libp2p.

👍

jacobheun

Mostly minor things and some language clarifications

jacobheun · 2020-04-28T13:06:25Z

doc/CONFIGURATION.md


 ### Examples

 #### Basic setup

+TODO: should we add to the basic setup the configuration of listen addresses? we should probably make it a required option?


It shouldn't be required. Creating a dial only node is a valid use case.

doc/CONFIGURATION.md

jacobheun · 2020-04-28T13:46:49Z

doc/API.md


-`libp2p.peerRouting.findPeer(peerId, options)`
+`libp2p.getAdvertisingMultiaddrs()`


We discussed offline making this a computed getter, libp2p.multiaddrs, to be similar to libp2p.peerInfo.multiaddrs.

jacobheun · 2020-04-28T13:47:48Z

doc/API.md

 ```

+### addressManager.getAnnounceMultiaddrs


Maybe truncate these methods to addressManager.get...Addrs?

jacobheun · 2020-04-28T13:49:15Z

doc/GETTING_STARTED.md

@@ -136,6 +136,8 @@ If you want to know more about libp2p stream multiplexing, you should read the f

 Now that you have configured a [**Transport**][transport], [**Crypto**][crypto] and [**Stream Multiplexer**](streamMuxer) module, you can start your libp2p node. We can start and stop libp2p using the [`libp2p.start()`](./API.md#start) and [`libp2p.stop()`](./API.md#stop) methods.

+TODO: add listen addresses here?


Yes, I would configure a websocket listen address (on a specific port).

src/address-manager/README.md

jacobheun · 2020-04-28T14:05:36Z

src/address-manager/README.md

+
+#### Modify Listen Addresses
+
+While adding new addresses to listen on runtime is a feasible operation, removing one listen address might have bad implications for the node, since all the connections using that listen address will be closed. With this in mind and taking also into consideration the lack of good use cases for removing listen addresses, the Address Manager API only allows libp2p users to add new Listen Addresses on runtime.


A future use case for removing listening addresses would be Relay and Rendezvous. If we asked used Rendezvous to find relays to listen on, we could add them to our list. If the relay we found was at its maximum listener capacity, it could potentially tell us to go away, at which point we would likely remove it from our listen addresses.

I don't think we need to speculate on the operations here though. Mentioning the potential of them changing in runtime and that it should provide a mechanism for listening to those changes would be sufficient here.

src/address-manager/index.js

jacobheun · 2020-04-28T14:08:57Z

src/identify/index.js

   * @param {Map<string, handler>} options.protocols A reference to the protocols we support
-   * @param {PeerId} options.peerId The peer running the identify service
-   * @param {{ listen: Array<Multiaddr>}} options.addresses The peer addresses
   */
  constructor (options) {


Not that there are fewer params this could help simplify the options accessor calls in the constructor

Suggested change

constructor (options) {

constructor ({ libp2p, protocols }) {

jacobheun · 2020-04-28T14:14:39Z

src/index.js

+          return false
+        }
+
+        // Filter out if in the special filter


I would remove support for this right now and we can look at doing a followup PR for this with verbose testing. I think this logic isn't quite right and non TCP addresses might behave unexpectedly here.

Co-Authored-By: Jacob Heun <[email protected]>

vasco-santos · 2020-04-28T15:01:06Z

@jacobheun everything should be addressed now, and the dependent PR, as well as the pubsub discovery module were updated according to these changes

jacobheun · 2020-04-29T12:13:29Z

src/circuit/index.js

@@ -122,7 +122,7 @@ class Circuit {
          type: CircuitPB.Type.HOP,
          srcPeer: {
            id: this.peerId.toBytes(),
-            addrs: this.addresses.listen.map(addr => addr.buffer)
+            addrs: this.addressManager.getListenAddrs().map(addr => addr.buffer)


This should be libp2p.multiaddrs not the listen addrs.

jacobheun · 2020-04-29T12:17:14Z

src/index.js

@@ -189,6 +188,8 @@ class Libp2p extends EventEmitter {
   */
  async start () {
    log('libp2p is starting')
+    // TODO: consider validate listen addresses on start?
+    // depend on transports?


What do you mean? When the TransportManager listens it results in each Transport filtering out all addresses that don't match it. We're not logging any information around "hey, you gave us an address and there is no transport that's valid for it", unless non of the addresses match any transport.

This is not valid anymore. Old thought

doc/API.md

Co-Authored-By: Jacob Heun <[email protected]>

jacobheun

This LGTM. CI is having issues, this should be good to merge.

vasco-santos mentioned this pull request Apr 21, 2020

Get announce addresses from AddressManager libp2p/js-libp2p-pubsub-peer-discovery#6

Closed

vasco-santos force-pushed the feat/address-management branch from 2fb9cc8 to 4f39f2e Compare April 22, 2020 13:05

vasco-santos mentioned this pull request Apr 23, 2020

chore: remove peer info usage #610

Merged

vasco-santos force-pushed the feat/address-management branch 3 times, most recently from 50e0b11 to 10a8142 Compare April 24, 2020 15:06

jacobheun reviewed Apr 24, 2020

View reviewed changes

vasco-santos force-pushed the feat/address-management branch 2 times, most recently from 1095875 to 8a86437 Compare April 27, 2020 07:48

This was referenced Apr 27, 2020

feat: peerstore persistence #619

Merged

chore: refactor conn mgr and registrar #611

Merged

feat: address manager

7e76d0f

vasco-santos force-pushed the feat/address-management branch 2 times, most recently from ef59d4a to 7d9a8d1 Compare April 27, 2020 16:33

chore: update api docs

4e4a998

vasco-santos force-pushed the feat/address-management branch from 7d9a8d1 to 4e4a998 Compare April 27, 2020 17:03

This was referenced Apr 28, 2020

chore: use address manager libp2p/js-libp2p-mdns#91

Merged

chore: use address manager libp2p/js-libp2p-pubsub-peer-discovery#7

Merged

vasco-santos marked this pull request as ready for review April 28, 2020 06:48

vasco-santos requested a review from jacobheun April 28, 2020 06:48

jacobheun suggested changes Apr 28, 2020

View reviewed changes

vasco-santos force-pushed the feat/address-management branch from a7c4dcd to 0b439fe Compare April 28, 2020 14:45

chore: rename address functions and apply suggestions from code review

21b39eb

Co-Authored-By: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/address-management branch from 0b439fe to 21b39eb Compare April 28, 2020 14:49

jacobheun suggested changes Apr 29, 2020

View reviewed changes

fix: use libp2p.multiaddrs instead of listen

7b8e5be

Co-Authored-By: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/address-management branch from c5764cf to 7b8e5be Compare April 29, 2020 14:24

jacobheun approved these changes Apr 29, 2020

View reviewed changes

vasco-santos merged commit e5592b0 into 0.28.x Apr 29, 2020

achingbrain deleted the feat/address-management branch May 18, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: address management #612

feat: address management #612

vasco-santos commented Apr 20, 2020 •

edited

Loading

vasco-santos commented Apr 20, 2020

jacobheun left a comment

jacobheun Apr 24, 2020

jacobheun Apr 24, 2020

vasco-santos Apr 24, 2020

vasco-santos Apr 25, 2020

jacobheun Apr 27, 2020 •

edited

Loading

vasco-santos Apr 27, 2020

jacobheun left a comment

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

jacobheun Apr 28, 2020

vasco-santos commented Apr 28, 2020 •

edited

Loading

jacobheun Apr 29, 2020

jacobheun Apr 29, 2020

vasco-santos Apr 29, 2020

jacobheun left a comment


		`libp2p.peerRouting.findPeer(peerId, options)`
		`libp2p.getAdvertisingMultiaddrs()`

		@@ -136,6 +136,8 @@ If you want to know more about libp2p stream multiplexing, you should read the f

		Now that you have configured a [Transport][transport], [Crypto][crypto] and [Stream Multiplexer](streamMuxer) module, you can start your libp2p node. We can start and stop libp2p using the [`libp2p.start()`](./API.md#start) and [`libp2p.stop()`](./API.md#stop) methods.

		TODO: add listen addresses here?


		#### Modify Listen Addresses

		While adding new addresses to listen on runtime is a feasible operation, removing one listen address might have bad implications for the node, since all the connections using that listen address will be closed. With this in mind and taking also into consideration the lack of good use cases for removing listen addresses, the Address Manager API only allows libp2p users to add new Listen Addresses on runtime.

	constructor (options) {
	constructor ({ libp2p, protocols }) {

feat: address management #612

feat: address management #612

Conversation

vasco-santos commented Apr 20, 2020 • edited Loading

vasco-santos commented Apr 20, 2020

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

vasco-santos commented Apr 20, 2020 •

edited

Loading

jacobheun Apr 27, 2020 •

edited

Loading

vasco-santos commented Apr 28, 2020 •

edited

Loading