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

Feature: adds custom multicodec protocol option #206

Merged
merged 6 commits into from
Dec 9, 2020

Conversation

HexaField
Copy link
Contributor

@HexaField HexaField commented Dec 5, 2020

This PR adds the ability to use unique multicodec protocols for the DHT, which enables multiple DHTs to operate separately in a single libp2p instance, as well as specifying they are unique from the IPFS ecosystem.

Includes a test to ensure peers with separate protocols do not find each other. I have also run external integration tests with a libp2p instance with no problems. I am open to adding more tests if there are more cases to test for.

The protocol to use can be specified by passing the protocol argument in the constructor argument, which defaults to what currently is in use: "/ipfs/kad/1.0.0"

The multicodec module export used in other js-libp2p packages still exports the same default value, so all external references to it should remain unaffected.

Please note, this may not a complete solution, since each DHT instance will have it's own peer topology while using the same peerStore. My knowledge of libp2p is limited in this capacity, so more work may need to be done to enable full functionality with multiple topologies.

For those interested, the code necessary to add a custom secondary DHT to a libp2p instance is as follows:

/**
 * @param {Libp2p} libp2p
 */
function addDHT(libp2p) {
    const customDHT = new KadDHT({
        libp2p,
        dialer: libp2p.dialer,
        peerId: libp2p.peerId,
        peerStore: libp2p.peerStore,
        registrar: libp2p.registrar,
        protocol: '/custom/kad/1.0.0'
    })
    customDHT.start()
    customDHT.on('peer', libp2p._onDiscoveryPeer) // DISCLAIMER: You may want to implement your own discovery event function
    return customDHT
}

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This LGTM. The code snippet in the description would be great to have in the readme as well.

This is necessary and useful for running an isolated DHT and I think it's a reasonable first step for running multiple DHTs, although we're likely losing some efficiencies with this approach. This is also inline with how the go-libp2p DHT is configured and I believe it's currently using this approach to run DHTs for lan/wan networks.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Can you please add @jacobheun recommendation as well?

src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks, just some minor adjustments in the README left

README.md Outdated
@@ -42,6 +42,29 @@
const KadDHT = require('libp2p-kad-dht')
```

### Usage
Copy link
Member

Choose a reason for hiding this comment

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

This would feel like the regular usage of the DHT

Suggested change
### Usage
### Custom secondary DHT in libp2p

README.md Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Member

@HexaField apologies, but we should do another change to be consistent with go-libp2p. In go-libp2p users can specify a protocol prefix (e.g. /myapp that then gets prepended to /kad/1.0.0).

This may help with network upgrades where we might want to be checking both /myapp/kad/1.0.0 and /myapp/kad/2.0.0 without asking users to understand what's happening with the upgrade.

So, my suggestion here would be to rename the protocol option to protocolPrefix (default to /ipfs, removing this part from the constant) like https://github.com/libp2p/go-libp2p-kad-dht/blob/7db4172fea89359edcbb7624c25529bd7c27304f/dht_options.go#L38.

Other than this, go-libp2p currently supports a legacy mode where it is possible to custom the entire protocol, removing the kad/1.0.0. While we hope that no one is using legacy protocols, with would be good to support this if go users are leveraging such protocols. This is the go escape hatch: https://github.com/libp2p/go-libp2p-kad-dht/blob/7db4172fea89359edcbb7624c25529bd7c27304f/dht_options.go#L279-L289

I would probably simplify this to another option parameter in create, such as forceProtocolLegacy as a boolean. This should have a warning description saying that this should only be used for legacy purposes.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This looks good! Just missing the below suggestion for the legacy type

src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

@vasco-santos vasco-santos merged commit 20d57b5 into libp2p:master Dec 9, 2020
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.

3 participants