Maintain permanent connections for active channels

[rustyrussell]

Fixes: #1281

[ZmnSCPxj]

Mostly OK, but this uses the same mechanism that connect command does.

Should we worry about the case:

1. An "important" peer with a channel disappears forever.
2. gossipd goes on a loop attempting to retry that peer forever.
3. User (or automated pilot) attempts a connect command.

It seems that 3 above will never return, as we never send a gossip_peer_connection_failed to trigger the connect command to return with failure.

An alternative is for connect JSON-RPC to return with a timeout separately from the timeout in gossipd.

This is worrisome as naive implementation of pilot program might just connect-fundchannel without bothering to check listpeers.

[ZmnSCPxj]

msg is parameter of this function and is also a local variable.

[cdecker]

Other than the msg param being shadowed, this is good to go :-)

Indeed clang seems to be a bit more pedantic in this case:

lightningd/closing_control.c:98:6: error: redefinition of 'msg' with a different type: 'u8 *' (aka 'unsigned char *') vs 'const u8 *'
      (aka 'const unsigned char *')
        u8 *msg;
            ^
lightningd/closing_control.c:96:70: note: previous definition is here
static void peer_closing_complete(struct channel *channel, const u8 *msg)
                                                                     ^
lightningd/closing_control.c:113:8: error: implicit declaration of function 'towire_gossipctl_peer_important' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
        msg = towire_gossipctl_peer_important(NULL, &channel->peer->id, false);
              ^
lightningd/closing_control.c:113:6: error: incompatible integer to pointer conversion assigning to 'const u8 *'
      (aka 'const unsigned char *') from 'int' [-Werror,-Wint-conversion]
        msg = towire_gossipctl_peer_important(NULL, &channel->peer->id, false);
            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.

[rustyrussell]

Should we worry about the case:

1. An "important" peer with a channel disappears forever.
2. gossipd goes on a loop attempting to retry that peer forever.
3. User (or automated pilot) attempts a connect command.

User will get an error:

if (find_reaching(daemon, id)) {
	/* FIXME: Perhaps kick timer in this case? */
	status_trace("try_reach_peer: already trying to reach %s",
		     type_to_string(tmpctx, struct pubkey, id));
	return false;
}

Perhaps we should probably stop the multiple connect attempts now for manual connects, which would allow us to fail faster.

But generally, I think trying forever is correct in this case: if we have an HTLC we'll eventually force close, and when we implement the json_close timeout logic that would do the same thing.

[rustyrussell]

Fixed shadowed variable, missing #include.

[ZmnSCPxj]

The code quoted is called by reach_peer, which only sends a message to masterd if the quoted code returns true. If it returns false, nothing is done and the connect command stalls.

[ZmnSCPxj]

Note that kicking the timer in the case "peer is important, peer is unavailable, user issued connect command to peer" is probably a Good Thing. Consider the case where the peer becomes unavailable for a while, thus triggering the exponential backoff until the 5-minute limit. Then the user learns that the peer is now available via some out-of-band method (phone call, IRC message, smoke signals, telepathy, psychohistorical trends of society). It would be better if the user at least has the ability to trigger connection with that peer immediately, than wait for the next 5-minute cycle.

So I propose that we split the reaching system into two parts:

1. A subsystem that makes a single attempt to try, and then trigger everyone interested in connecting to that peer, informing them of success or failure. It sends gossip_peer_connected gossip_peer_already_connected and gossip_peer_connection_failed to the master.
2. Another sub-system that handles the reconnect to "important" peers with exponential backoff limited to 5 minutes. After a timeout (struct oneshot instance) is triggered, it simply frees the timeout object and triggers the single-connect-try subsystem for the important peer if it is not connected. On any failure connection to an "important" peer it clears any timeout it has for that peer and creates a new timeout object.

A connect command would then simply trigger the single-connect-try subsystem, possibly doing its own retries independently of the auto-reconnect subsystem.

[rustyrussell]

Good point. I agree, let's make connect a one-shot, and have a separate system for important peers.

[ZmnSCPxj]

Should we not keep the returned struct oneshot in the struct important_peerid? And before making a new timer, we should free any existing timers? Otherwise connect commands may cause multiple overlapping timers to exist (I think?).

/* Kick the timer if present, or start a new one if not present. */
tal_free(imp->timer);
imp->timer = new_reltimer(&reach->daemon->timers, imp, time_from_sec(5), retry_important, imp);

The retry_important function also needs to clear this timer:

imp->timer = tal_free(imp->timer);

[rustyrussell]

Good point, we will end up with one timer per connect for every important peer. Will fix, though not here.

[rustyrussell]

OK, fixed python whitespace and invalid assertion.

And added a commit which prevents multiple timers running for multiple connect commands to the same (important) peer; thanks @ZmnSCPxj !

[ZmnSCPxj]

At first glance imp->reconnect_timer seems not initialized when new important_peerid is created. However important_peerid is created only one place, and, it calls retry_important immediately, which sets imp->reconnect_timer to NULL, so no issue here.

[ZmnSCPxj]

ACK f9f38b1

[ZmnSCPxj]

It is failing in CI, but I am uncertain why; the CI error pertains to openingd, not gossipd...? It seems the funder openingd died because the fundee openingd disconnected, while the fundee can see its own openingd disconnected from itself but did not die. Some edge case of connecting? Perhaps a reconnect try is still spuriously triggering?

[rustyrussell]

Fixing that turned out to be more surgery than I anticipated! Added WIP since I'm pushing an update to check CI...

[rustyrussell]

The fast reconnect seems to be triggering a heap of fun bugs. Will spend more time diagnosing tomorrow.

[ZmnSCPxj]

find_connect loops over the connect commands, and this loop also loops over find_connect. More efficient to just use list_safe_for_each and resolve each matching command. Acceptable for now though, we really need this merged.

[rustyrussell]

Agreed, it's N^2 but as you say, N small and we don't really care, so simplicity wins.

[rustyrussell]

Travis seems confused, didn't start? Will rebase to fix conflict...

[rustyrussell]

This time, fixed conflict properly...

[rustyrussell]

Holiday with kids means less time to code :)

Or, put another way: parenting is my chosen career, which demanded a full week on-site. Thus my coding hobby suffered a small setback.

Thanks for your patience, everyone!