Increase idle connection timeout

[AgeManning]

Issue Addressed

It was reported that when Prysm connects to us, there is occasionally a small time in negotiating protocols before we set-up gossipsub that we have an idle connection. In this case Lighthouse immediately drops the connection.

This results in us preventing Prysm from connecting to us some fraction of the time. This PR increases the timeout from 0 seconds to 1 second to help correct this.

[AgeManning]

@jxs - Is probably not going to be awake early enough to approve this one, but I think it's not going to destroy the world if for some reason this turns out to be a mistake. We can revert after the RC

[michaelsproul]

I'll just roll it into the RC branch without merging it

[jxs]

I had a conversation with @dennis-tra some weeks ago, the data seemed inconclusive as AFAIR we were connected to Prysm clients, is there new data that backs this decision?

btw I am not sure if with_idle_connection_timeout() affects negotiation, confirm here

[AgeManning]

Yeah. The discussion I had was just that through experimentation they identified that this is the cause for connection drops. Changing this value is expected to resolve it.

I've not seen specific data on it, but I also didn't see much downside to having a 1 second buffer, just in case it does help.

Curious to know if there is some downside here (we shouldn't have idle connections so long as gossipsub is negotiated).

[AgeManning]

I guess. perhaps we maintain connections with crawlers and other clients that don't support gossipsub more easily with this PR.

Potentially, in a future PR we should only allow a percentage of our peer count to not support gossipsub. Last I checked there was a decent amount, like 10-20% of peers that didn't support it.

[dennis-tra]

Hi everyone,

yeah I talked to @AgeManning at DevCon about this issue. I also mentioned that we had a chat @jxs and that the data was inconclusive.

@jxs you mentioned that Lighthouse is indeed connected to Prysm peers but the crucial thing is in which direction these connections point. I presume most connections are pointing outwards from Lighthouse to Prysm peers. However, let me emphasize that this is just a hypothesis. I'm basing this on our observations from interacting with other rust-libp2p peers and a study of the relevant code.

In go-libp2p there's a brief moment in time when connecting to a peer before control is handed back to the user where all four conditions are true from your referenced code snippet. In go-libp2p the relevant part is here (which is called from the user-facing Connect call here) where we wait until the identify exchange has completed before we do anything else (like opening a new stream).

[AgeManning]

@dennis-tra - Cool. Wondering if we can confirm this before we merge this into our next release.

Is it possible for you to use this PR and see if it resolves the issues you have been seeing?

If they persist, then perhaps the problem lies elsewhere

[jxs]

Hi @dennis-tra! Thanks for your explanation, meanwhile I also found libp2p/rust-libp2p#4967 which I wasn't aware and makes sense to be merged, so I think we can leave it to 10 second here while a rust-libp2p release with 10 seconds as the default isn't released

[dennis-tra]

Great news, thanks @jxs! 10s sounds good.

[michaelsproul]

@mergify queue

[mergify]

queue

🛑 The pull request has been removed from the queue default

Details

Pull request #6604 has been dequeued by a dequeue command.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[michaelsproul]

@mergify dequeue

[mergify]

dequeue

✅ The pull request has been removed from the queue default

[michaelsproul]

@mergify queue

[mergify]

queue

🛑 The pull request has been removed from the queue default

Details

Pull request #6604 has been dequeued by a dequeue command.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[michaelsproul]

@mergify requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

[mergify]

queue

🛑 The pull request has been removed from the queue default

Details

Pull request #6604 has been dequeued by a dequeue command.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[michaelsproul]

@mergify dequeue

[mergify]

dequeue

✅ The pull request has been removed from the queue default

[michaelsproul]

@mergify requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

[mergify]

queue

🛑 The pull request has been removed from the queue default

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[michaelsproul]

@mergify requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

[mergify]

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 770d677