Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expand the protection interval for new connections to 3m #2096

Conversation

RubenKelevra
Copy link
Contributor

The protection interval was previously set to 1m, which was deemed too
low.

Rationale:
#2055 (comment)

The protection interval was previously set to 1m, which was deemed too 
low.

Rationale: 
ipfs#2055 (comment)
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

To be honest, I am not sold on changing this to 3m, given the average stay stat being under one minute 🙈

HTTP keep-alive does not apply here, we have N and not 1 connection to worry about.

My main concern right now is around how the Grace Period works: AFAIK it will protect connection no matter if it was used for bitswap or not all. Raising this to 3m means the user will quickly reach thousands of peers and in theory, kill their cheap router even faster. Browsing Wikipedia and some other IPFS websites will produce over 1k of peers, and these won't get closed fast enough.

I propose we park this until at least one of below is available and implemented in this PR

  • go-ipfs 0.13 ships Swarm.ResourceMgr – that will allow us to set hard connection limits
  • we are able to set separate Grace Period for peers that sent us some data over bitswap

@lidel lidel marked this pull request as draft April 8, 2022 17:15
@lidel lidel added the status/blocked Unable to be worked further until needs are met label Apr 8, 2022
@RubenKelevra
Copy link
Contributor Author

@lidel yeah I understand your concerns, but I got one of those pesky devices as my router and don't have issue's since I did this.

Connections stay somewhere around 400 max for me. But since they are only established and then terminated after a while instead of replaced by other new ones if they terminate, the router seems to be fine with that.

In my case it's a 4G router from TP-Link.

Browsing Wikipedia and some other IPFS websites will produce over 1k of peers, and these won't get closed fast enough.

Let me check that.

@RubenKelevra
Copy link
Contributor Author

Yeah I agree, it creates a lot of connections while loading Wikipedia. Not sure what's the issue there, as it is also extremely slow, with loading times between 10 seconds and one minute.

So I would agree that 1 minute is probably the better value and something like 3 minutes for a "Grace Period for peers that sent us some data over bitswap". :)

src/daemon/config.js Outdated Show resolved Hide resolved
@SgtPooki SgtPooki marked this pull request as ready for review September 13, 2022 19:03
@SgtPooki SgtPooki requested a review from a team as a code owner September 13, 2022 19:03
@SgtPooki
Copy link
Member

I propose we park this until at least one of below is available and implemented in this PR

  • go-ipfs 0.13 ships Swarm.ResourceMgr – that will allow us to set hard connection limits
  • we are able to set separate Grace Period for peers that sent us some data over bitswap

@lidel are there tracking issues for these? Kubo is on 0.15 now, so I assume that first item is done. What is needed to handle that second item?

@SgtPooki SgtPooki marked this pull request as draft September 13, 2022 19:05
@SgtPooki
Copy link
Member

Hey Ruben,

Thanks so much for this PR and for your patience with us. After talking with Lidel, we've agreed that the proper fix is to remember peers you connected to when viewing a DAG, and re-use them when attempting to revisit the DAG. This work would need to be done inside of Kubo. Would you mind opening an issue in the Kubo repo for this feature enhancement?

If we were to increase the GracePeriod blindly for all connections, there are a large number of potential issues that could arise with this. We think this should be a user-based decision to increase this value.

@SgtPooki SgtPooki closed this Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants