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

Return an error if we can't connect and limit the number of retries #813

Merged
merged 5 commits into from
Jan 29, 2018

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jan 27, 2018

Retrying to connect in eternity seems a bit unintuitive, so we limit to 10 attempts and keep track of when we started connecting. We could pass these parameters from lightningd in the future, but this is the simplest solution for now. Also this fixes the hanging when we do not know an address and don't provide a hint.

Fixes #681
Fixes #405
Fixes #390

@cdecker cdecker added this to the v0.6 milestone Jan 27, 2018
@cdecker cdecker self-assigned this Jan 27, 2018
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

Mostly minor nits, this seems good.

gossipd/gossip.c Outdated
/* How many times have we attempted to connect? */
u32 attempts;

/* How many timest to attempt */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: comment misspelling "timest"

@@ -43,6 +43,13 @@ gossip_peer_connected,,lfeatures,lflen*u8
gossip_peer_already_connected,3015
gossip_peer_already_connected,,id,struct pubkey

# gossipd -> master: attempted to connect, unsuccessful, gave up
gossip_peer_connection_failed,3020
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need some good way to allocate message numbers, now I need to bump the message number in #638 again...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, fortunately compile will fail with a clash, so whoever is applying it can rebase and fixup...

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Great work! Appended minor typo fixes for expedience...

@@ -43,6 +43,13 @@ gossip_peer_connected,,lfeatures,lflen*u8
gossip_peer_already_connected,3015
gossip_peer_already_connected,,id,struct pubkey

# gossipd -> master: attempted to connect, unsuccessful, gave up
gossip_peer_connection_failed,3020
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, fortunately compile will fail with a clash, so whoever is applying it can rebase and fixup...

gossipd/gossip.c Outdated
u32 max_attempts;

/* Timestamp of the first attempt */
u32 first_attempt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mildly prefer timestamps as struct timeabs, which is clearer, but not worth another round of feedback for.

{
struct pubkey id;
u32 attempts, timediff;
struct connect *i, *next;
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird indent?

i->cmd,
"Could not connect to %s after %d seconds and %d attempts",
type_to_string(msg, struct pubkey, &id), timediff,
attempts);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably standardize on indentation style for multiline calls sometime :)

}



Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a lot of vertical space :)

@rustyrussell rustyrussell merged commit 6daa770 into ElementsProject:master Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants