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

Hotfix the large number of connections #5071

Closed
wants to merge 2 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Feb 26, 2020

Re-purposes discovery_only_if_under_num into allow_out_only_if_under_num, which blocks all new outgoing connection attempts after the limit has been reached.

The limit right now is set to value-of---out-peers + 15. Also note that it is possible that we go slightly higher than the limit, as we don't cancel pending connections. Only new connections are forbidden when above the limit.

Also enables the incoming_limit feature of the SwarmBuilder to be set to the value of --in-peers.

This is obviously not great, and will possibly make discovery work way less greatly, but it should hotfix the issue with the large number of connections which we've been seeing.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Feb 26, 2020
@tomaka tomaka requested review from romanb and arkpar February 26, 2020 20:02
@tomaka
Copy link
Contributor Author

tomaka commented Feb 26, 2020

I reverted the addition of incoming_limit, as I realized that this was limiting only the number of simultaneous non-negotiated connections, which is irrelevant for this issue.

@romanb
Copy link
Contributor

romanb commented Feb 27, 2020

So, for my understanding, in contrast to the existing limit which affects only when a new DHT random lookup is started, this change is supposed to even cut short such a query via returning early from addresses_of_peer? Note though that I think this affects all Kademlia queries, not just these random lookups, i.e. the actual DHT put/get operations that substrate uses will also be affected.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 27, 2020

I think this affects all Kademlia queries, not just these random lookups, i.e. the actual DHT put/get operations that substrate uses will also be affected.

Normally it shouldn't, as the response is fetched only from the k-buckets:
https://github.com/libp2p/rust-libp2p/blob/b6a93b3c5e6242683d745e067f55f5702692a769/protocols/kad/src/behaviour.rs#L551-L562

@romanb
Copy link
Contributor

romanb commented Feb 27, 2020

I think this affects all Kademlia queries, not just these random lookups, i.e. the actual DHT put/get operations that substrate uses will also be affected.

Normally it shouldn't, as the response is fetched only from the k-buckets:
https://github.com/libp2p/rust-libp2p/blob/b6a93b3c5e6242683d745e067f55f5702692a769/protocols/kad/src/behaviour.rs#L551-L562

That is how a request is answered, but if a node performs a get or put operation on a Kademlia DHT, the same iterative query towards the k closest nodes to the key happens as during a get_closest_peers with a random ID. These queries will then also be cut short. But maybe that is what you want here and why this change seems "effective", I don't know.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 27, 2020

But maybe that is what you want here and why this change seems "effective"

Ah, yes that's what I want here.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 27, 2020

(note: this PR is being put on ice while we try another fix)

@bkchr bkchr added A1-onice and removed A0-please_review Pull request needs code review. labels Feb 27, 2020
@tomaka
Copy link
Contributor Author

tomaka commented Mar 3, 2020

Let's close this, since the origin issue has been fixed by #5086

@tomaka tomaka closed this Mar 3, 2020
@tomaka tomaka deleted the limit-connections branch March 3, 2020 12:58
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