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

fix: interop dial blacklist #77

Merged
merged 1 commit into from
Jul 16, 2019
Merged

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jul 16, 2019

We introduced a dial queue on js-libp2p-switch, which intends to (among other factors) blacklist abusive peers. In this test, and differently, to the interop tests for pubsub, we had the default bootstrap list, as well as MDNS discovery enabled. This way, we were discovering the peer in several ways and trying to dial to it, while also dialing explicitly to it, in order to guarantee that each peer knows each other before the IPNS over pubsub tests.
With this into account, the libp2p-switch was marking the peer as blacklisted and we were not being able to dial with it.

I created this PR, which basically has an empty bootstrap list for the js node, and disables MDNS. This way, we will have the CI green in here and test the interop over the wire. Moreover, I will create an issue in js-libp2p-switch so that we rethink the way we blacklist nodes next week, once @jacobheun is back

More context: ipfs/interop#71#issuecomment-511833421

@vasco-santos vasco-santos marked this pull request as ready for review July 16, 2019 16:38
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

At least this way we'll be able to catch regressions. Thanks for finding this!

@Stebalien Stebalien merged commit 0de4d32 into master Jul 16, 2019
@vasco-santos vasco-santos deleted the fix/interop-dial-blacklist branch July 16, 2019 16:46
@jacobheun
Copy link
Contributor

We had this issue previously when go-ipfs was announcing itself over mdns before it was accepting connections, which causes the initial blacklisting. The number of dials to the peer shouldn't matter, as the js switch will queue them, but if the first dial is to a peer that's not ready they'll all fail, which is why we need the blacklisting improvement for js. We should make sure there is some level of regression testing for go, if there is not already, to make it's not announcing addresses before it's listening.

@Stebalien
Copy link
Member

Hm. So, there was a bug in go-ipfs 0.4.21 that has now been fixed in 0.4.22-rc1. Given that #81 fixed the issue, I wonder if we should revert this?

And no, there's no regression testing for MDNS (likely to prevent tests from interfering with each other). I've held off on doing any MDNS related work till we can rewrite the entire system. It's unsalvageable.

@vasco-santos
Copy link
Member Author

I think we can try that when 0.4.22 lands?

@hugomrdias and travis CI in this repo were able to reproduce this problem with blacklisting and the test broke as a result. So, if we take it out now, I think it will happen again.

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