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.

Sign up for GitHub

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
Merged

Connection timeout #4039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
357e8d5
connectd: implement connection timeout (60 seconds).
rustyrussell Sep 11, 2020
f939622
pytest: test connection timeout.
rustyrussell Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions connectd/connectd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <ccan/str/str.h>
#include <ccan/take/take.h>
#include <ccan/tal/str/str.h>
#include <ccan/timer/timer.h>
#include <common/bech32.h>
#include <common/bech32_util.h>
#include <common/cryptomsg.h>
Expand All @@ -38,6 +39,7 @@
#include <common/pseudorand.h>
#include <common/status.h>
#include <common/subdaemon.h>
#include <common/timeout.h>
#include <common/type_to_string.h>
#include <common/utils.h>
#include <common/version.h>
Expand Down Expand Up @@ -121,6 +123,10 @@ struct daemon {
/* pubkey equivalent. */
struct pubkey mykey;

/* Base for timeout timers, and how long to wait for init msg */
struct timers timers;
u32 timeout_secs;

/* Peers that we've handed to `lightningd`, which it hasn't told us
* have disconnected. */
struct node_set peers;
Expand Down Expand Up @@ -509,6 +515,14 @@ static struct io_plan *handshake_in_success(struct io_conn *conn,
cs, &id, addr);
}

/*~ If the timer goes off, we simply free everything, which hangs up. */
static void conn_timeout(struct io_conn *conn)
{
status_debug("conn timed out");
errno = ETIMEDOUT;
io_close(conn);
}

/*~ When we get a connection in we set up its network address then call
* handshake.c to set up the crypto state. */
static struct io_plan *connection_in(struct io_conn *conn, struct daemon *daemon)
Expand Down Expand Up @@ -544,7 +558,11 @@ static struct io_plan *connection_in(struct io_conn *conn, struct daemon *daemon
return io_close(conn);
}

/* FIXME: Timeout */
/* If they don't complete handshake in reasonable time, hang up */
notleak(new_reltimer(&daemon->timers, conn,
time_from_sec(daemon->timeout_secs),
conn_timeout, conn));

/*~ The crypto handshake differs depending on whether you received or
* initiated the socket connection, so there are two entry points.
* Note, again, the notleak() to avoid our simplistic leak detection
Expand Down Expand Up @@ -583,7 +601,10 @@ struct io_plan *connection_out(struct io_conn *conn, struct connecting *connect)
return io_close(conn);
}

/* FIXME: Timeout */
/* If they don't complete handshake in reasonable time, hang up */
notleak(new_reltimer(&connect->daemon->timers, conn,
time_from_sec(connect->daemon->timeout_secs),
conn_timeout, conn));
status_peer_debug(&connect->id, "Connected out, starting crypto");

connect->connstate = "Cryptographic handshake";
Expand Down Expand Up @@ -1243,7 +1264,8 @@ static struct io_plan *connect_init(struct io_conn *conn,
&proxyaddr, &daemon->use_proxy_always,
&daemon->dev_allow_localhost, &daemon->use_dns,
&tor_password,
&daemon->use_v3_autotor)) {
&daemon->use_v3_autotor,
&daemon->timeout_secs)) {
/* This is a helper which prints the type expected and the actual
* message, then exits (it should never be called!). */
master_badmsg(WIRE_CONNECTD_INIT, msg);
Expand Down Expand Up @@ -1638,6 +1660,7 @@ int main(int argc, char *argv[])
memleak_add_helper(daemon, memleak_daemon_cb);
list_head_init(&daemon->connecting);
daemon->listen_fds = tal_arr(daemon, struct listen_fd, 0);
timers_init(&daemon->timers, time_mono());
/* stdin == control */
daemon->master = daemon_conn_new(daemon, STDIN_FILENO, recv_req, NULL,
daemon);
Expand All @@ -1651,9 +1674,11 @@ int main(int argc, char *argv[])
* status_failed on error. */
ecdh_hsmd_setup(HSM_FD, status_failed);

/* Should never exit. */
io_loop(NULL, NULL);
abort();
for (;;) {
struct timer *expired;
io_loop(&daemon->timers, &expired);
timer_expired(daemon, expired);
}
}

/*~ Getting bored? This was a pretty simple daemon!
Expand Down
1 change: 1 addition & 0 deletions connectd/connectd_wire.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ msgdata,connectd_init,dev_allow_localhost,bool,
msgdata,connectd_init,use_dns,bool,
msgdata,connectd_init,tor_password,wirestring,
msgdata,connectd_init,use_v3_autotor,bool,
msgdata,connectd_init,timeout_secs,u32,

# Connectd->master, here are the addresses I bound, can announce.
msgtype,connectd_init_reply,2100
Expand Down
8 changes: 5 additions & 3 deletions connectd/connectd_wiregen.c
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions connectd/connectd_wiregen.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion lightningd/connect_control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,8 @@ int connectd_init(struct lightningd *ld)
ld->proxyaddr, ld->use_proxy_always || ld->pure_tor_setup,
IFDEV(ld->dev_allow_localhost, false), ld->config.use_dns,
ld->tor_service_password ? ld->tor_service_password : "",
ld->config.use_v3_autotor);
ld->config.use_v3_autotor,
ld->config.connection_timeout_secs);

subd_req(ld->connectd, ld->connectd, take(msg), -1, 0,
connect_init_done, NULL);
Expand Down
3 changes: 3 additions & 0 deletions lightningd/lightningd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ struct config {

/* This is the key we use to encrypt `hsm_secret`. */
struct secret *keypass;

/* How long before we give up waiting for INIT msg */
u32 connection_timeout_secs;
};

typedef STRMAP(const char *) alt_subdaemon_map;
Expand Down
9 changes: 9 additions & 0 deletions lightningd/options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,9 @@ static void dev_register_opts(struct lightningd *ld)
"Make builtin plugins unimportant so you can plugin stop them.");
opt_register_arg("--dev-force-features", opt_force_featureset, NULL, ld,
"Force the init/globalinit/node_announce/channel/bolt11 features, each comma-separated bitnumbers");
opt_register_arg("--dev-timeout-secs", opt_set_u32, opt_show_u32,
&ld->config.connection_timeout_secs,
"Seconds to timeout if we don't receive INIT from peer");
}
#endif /* DEVELOPER */

Expand Down Expand Up @@ -638,6 +641,9 @@ static const struct config testnet_config = {
.min_capacity_sat = 10000,

.use_v3_autotor = true,

/* 1 minute should be enough for anyone! */
.connection_timeout_secs = 60,
};

/* aka. "Dude, where's my coins?" */
Expand Down Expand Up @@ -694,6 +700,9 @@ static const struct config mainnet_config = {

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

/* 1 minute should be enough for anyone! */
.connection_timeout_secs = 60,
};

static void check_config(struct lightningd *ld)
Expand Down
17 changes: 17 additions & 0 deletions tests/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2634,3 +2634,20 @@ def test_nonstatic_channel(node_factory, bitcoind):

l1.pay(l2, 1000)
l1.rpc.close(l2.info['id'])


@unittest.skipIf(not DEVELOPER, "needs --dev-timeout-secs")
def test_connection_timeout(node_factory):
# l1 hears nothing back after sending INIT, should time out.
l1, l2 = node_factory.get_nodes(2,
opts=[{'dev-timeout-secs': 1,
'disconnect': ['0WIRE_INIT', '0WIRE_INIT']},
{}])
Comment on lines +2643 to +2645
Copy link
Contributor

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.


with pytest.raises(RpcError, match='timed out'):
l1.rpc.connect(l2.info['id'], 'localhost', port=l2.port)
l1.daemon.wait_for_log('conn timed out')

with pytest.raises(RpcError, match='reset by peer'):
l2.rpc.connect(l1.info['id'], 'localhost', port=l1.port)
l1.daemon.wait_for_log('conn timed out')