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

Connection timeout #4039

Merged
merged 2 commits into from
Sep 11, 2020

Conversation

rustyrussell
Copy link
Contributor

Found this because multifundchannel hung. Want it for rc2.

@rustyrussell rustyrussell added this to the v0.9.1 milestone Sep 11, 2020
Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK 8d336e6

Nice ! I'd be in favor of a more realistic timeout but not urgent for this release i guess

@@ -694,6 +700,9 @@ static const struct config mainnet_config = {

/* Allow to define the default behavior of tor services calls*/
.use_v3_autotor = true,

/* 10 minutes should be enough for anyone! */
.connection_timeout_secs = 600,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very conservative ? I'd rather not start funding a channel with a peer who took 9 minutes to respond to init..

Comment on lines +2643 to +2645
opts=[{'dev-timeout-secs': 1,
'disconnect': ['0WIRE_INIT', '0WIRE_INIT']},
{}])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, i wondered why you disconnected l1 to test the timeout on l1 and started testing the other way around: this would not trigger a timeout but a connection reset error.

Why specifying two times though ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to test incoming timeout didn't corrupt or leak memory. But this doesn't actually do that (since the incoming fails with Connection reset as soon at it hits the dev-disconnect line.

Added some minor logging so we can make sure it actually fired.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

10 minutes sounds way too long, but better start with a relaxed default 👍

ACK 8d336e6

This is simple, and we now can multifundchannel to every node on testnet
(one simply hangs once we connect).

Changelog-Fixed: Protocol: We now hang up if peer doesn't respond to init message after 60 seconds.
@rustyrussell rustyrussell merged commit f658dd0 into ElementsProject:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants