filecoin-project / venus

Filecoin Full Node Implementation in Go
https://venus.filecoin.io
Other
2.05k stars 459 forks source link

DHTs may be able to merge between networks #1700

Closed jhiesey closed 5 years ago

jhiesey commented 5 years ago

Description

As far as I (@jhiesey) am aware, there is nothing preventing filecoin network DHTs from merging with each other or the global IPFS DHT. For this to happen, one node would have to communicate with two networks.

Assuming no bugs or malicious nodes this shouldn't happen, but it's still a risk, especially after launch when people could run modified clients or misconfigure bootstrap nodes. This already happened when the test network for the rust implementation of libp2p-kad-dht accidentally merged with the live IPFS network.

This could have a few different consequences:

This came out of a discussion with @phritz @anacrolix @mgoelzer over this issue: https://github.com/filecoin-project/pm/issues/105

Acceptance criteria

Accidental DHT network merges either aren't possible or don't matter.

Risks + pitfalls

Even experimenting with this could result in networks unexpectedly merging, which might be very difficult to fix.

Where to begin

We need to discuss whether this is an issue, and if so where in the stack to fix it.

mishmosh commented 5 years ago

thanks for this writeup. would be really helpful to have libp2p friends pick this up, make a recommendation, and carry it through to implementation. does that sound right to you @jhiesey @mgoelzer?

ghost commented 5 years ago

Yes, I think it's fine to assign this to @jhiesey and/or @anacrolix. It started as a sincere question -- there are some legitimate reasons why FC might want them to merge. Since then we have discussed with @whyrusleeping, and he said no, don't let them merge. We can prevent that by adding a DHT ID field somewhere.

anacrolix commented 5 years ago

Yes, please assign this me. I'll do some checks to determine what is required in libp2p and lodge specific issues there.

phritz commented 5 years ago

see also https://github.com/filecoin-project/go-filecoin/pull/2042#discussion_r258935218

raulk commented 5 years ago

A simple and immediately applicable method to solve this is to pass in an option to override the default DHT protocol ID with a custom string: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/opts/options.go#L99.

This is what OpenBazaar does to segregate their DHT network and safeguard against accidental merges.

anacrolix commented 5 years ago

Yes that sounds like the ideal solution. It is also the most appropriate way to handle it throughout the codebase with regard to interacting with other libp2p components.

anacrolix commented 5 years ago

Some useful pointers to the API: DefaultProtocols and Protocols, and any of the IpfsDHT constructors. I'll try to post a PR in the next day or two.

anacrolix commented 5 years ago

It looks like this is already done here. It also looks like there's already a test in go-libp2p-kad-dht too. The only other case is the merging of test and production networks for Filecoin. It could be desirable to have these networks use the same protocol to test other sanity checking measures, and the resilience of the network to having bad nodes. If not we could allow specifying a custom protocol in the Filecoin code. @anorth

anorth commented 5 years ago

Thanks for investigating for us! So if we want to prevent the networks merging at that level, it's just a matter of runtime configuration now? Great, I think we should do it, but it's not urgent.

anacrolix commented 5 years ago

We should close this, referring back to it if we have an issue in the future. I think exposing a runtime option is a separate issue, and should be opened when the need arises.