From 98b1a665f7281fd845c9522783b11dbcc0860296 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 24 Nov 2017 19:28:49 +0100 Subject: [PATCH 01/25] gossip: Remove HSM_FD from handshake --- gossipd/handshake.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/gossipd/handshake.c b/gossipd/handshake.c index 0fa8738adf1b..02f9207c0a62 100644 --- a/gossipd/handshake.c +++ b/gossipd/handshake.c @@ -22,8 +22,6 @@ #include #include -#define HSM_FD 3 - #ifndef SUPERVERBOSE #define SUPERVERBOSE(...) #endif From 37e9d1497008e2017e8cb016297621d36b142b13 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 2 Dec 2017 23:28:19 +0100 Subject: [PATCH 02/25] routing: Make routing_state aware of its own ID This is used to identify our own announcements. Signed-off-by: Christian Decker --- gossipd/gossip.c | 2 +- gossipd/routing.c | 4 +++- gossipd/routing.h | 3 ++- gossipd/test/run-find_route-specific.c | 3 +-- gossipd/test/run-find_route.c | 4 ++-- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index 89ae6262110a..197db965cc67 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -1114,7 +1114,7 @@ static struct io_plan *gossip_init(struct daemon_conn *master, &daemon->localfeatures)) { master_badmsg(WIRE_GOSSIPCTL_INIT, msg); } - daemon->rstate = new_routing_state(daemon, &chain_hash); + daemon->rstate = new_routing_state(daemon, &chain_hash, &daemon->id); setup_listeners(daemon, port); return daemon_conn_read_next(master->conn, master); diff --git a/gossipd/routing.c b/gossipd/routing.c index 0182d87a7331..736563ff21f7 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -26,12 +26,14 @@ static struct node_map *empty_node_map(const tal_t *ctx) } struct routing_state *new_routing_state(const tal_t *ctx, - const struct sha256_double *chain_hash) + const struct sha256_double *chain_hash, + const struct pubkey *local_id) { struct routing_state *rstate = tal(ctx, struct routing_state); rstate->nodes = empty_node_map(rstate); rstate->broadcasts = new_broadcast_state(rstate); rstate->chain_hash = *chain_hash; + rstate->local_id = *local_id; return rstate; } diff --git a/gossipd/routing.h b/gossipd/routing.h index 658ebfd4476a..9c51f951240d 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -93,7 +93,8 @@ struct route_hop { }; struct routing_state *new_routing_state(const tal_t *ctx, - const struct sha256_double *chain_hash); + const struct sha256_double *chain_hash, + const struct pubkey *local_id); /* msatoshi must be possible (< 21 million BTC), ie < 2^60. * If it returns more than msatoshi, it overflowed. */ diff --git a/gossipd/test/run-find_route-specific.c b/gossipd/test/run-find_route-specific.c index 3581a543a913..908bfa6b6279 100644 --- a/gossipd/test/run-find_route-specific.c +++ b/gossipd/test/run-find_route-specific.c @@ -67,8 +67,6 @@ int main(void) secp256k1_ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN); - rstate = new_routing_state(ctx, &zerohash); - pubkey_from_hexstr("03c173897878996287a8100469f954dd820fcd8941daed91c327f168f3329be0bf", strlen("03c173897878996287a8100469f954dd820fcd8941daed91c327f168f3329be0bf"), &a); @@ -79,6 +77,7 @@ int main(void) strlen("02ea622d5c8d6143f15ed3ce1d501dd0d3d09d3b1c83a44d0034949f8a9ab60f06"), &c); + rstate = new_routing_state(ctx, &zerohash, &a); /* [{'active': True, 'short_id': '6990:2:1/1', 'fee_per_kw': 10, 'delay': 5, 'flags': 1, 'destination': '0230ad0e74ea03976b28fda587bb75bdd357a1938af4424156a18265167f5e40ae', 'source': '02ea622d5c8d6143f15ed3ce1d501dd0d3d09d3b1c83a44d0034949f8a9ab60f06', 'last_update': 1504064344}, */ nc = get_or_make_connection(rstate, &c, &b); diff --git a/gossipd/test/run-find_route.c b/gossipd/test/run-find_route.c index 236f3e8e2aa8..21785cee0fdc 100644 --- a/gossipd/test/run-find_route.c +++ b/gossipd/test/run-find_route.c @@ -78,9 +78,9 @@ int main(void) secp256k1_ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN); - rstate = new_routing_state(ctx, &zerohash); - memset(&tmp, 'a', sizeof(tmp)); + rstate = new_routing_state(ctx, &zerohash, &a); + pubkey_from_privkey(&tmp, &a); new_node(rstate, &a); From 4c13032ce6216acbf0e2a6999ccf8ec85fea7f7f Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 24 Nov 2017 14:43:31 +0100 Subject: [PATCH 03/25] opts: Change alias to be u8*, better matches the unicode nature We are still generating only char* style aliases, but the field is defined to be unicode, which doesn't mix too well with char. Signed-off-by: Christian Decker --- lightningd/lightningd.h | 2 +- lightningd/options.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index ba53ed767958..fd161f7c43fa 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -89,7 +89,7 @@ struct lightningd { struct pubkey id; /* My name is... my favorite color is... */ - char *alias; /* At least 32 bytes (zero-filled) */ + u8 *alias; /* At least 32 bytes (zero-filled) */ u8 *rgb; /* tal_len() == 3. */ /* Any pending timers. */ diff --git a/lightningd/options.c b/lightningd/options.c index e98d0adffc87..37e440fed639 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -168,8 +168,8 @@ static char *opt_set_alias(const char *arg, struct lightningd *ld) */ if (strlen(arg) > 32) return tal_fmt(NULL, "Alias '%s' is over 32 characters", arg); - ld->alias = tal_arrz(ld, char, 33); - strncpy(ld->alias, arg, 32); + ld->alias = tal_arrz(ld, u8, 33); + strncpy((char*)ld->alias, arg, 32); return NULL; } @@ -572,11 +572,11 @@ void setup_color_and_alias(struct lightningd *ld) memcpy(&noun, der+3+sizeof(adjective), sizeof(noun)); noun %= ARRAY_SIZE(codename_noun); adjective %= ARRAY_SIZE(codename_adjective); - ld->alias = tal_arrz(ld, char, 33); + ld->alias = tal_arrz(ld, u8, 33); assert(strlen(codename_adjective[adjective]) + strlen(codename_noun[noun]) < 33); - strcpy(ld->alias, codename_adjective[adjective]); - strcat(ld->alias, codename_noun[noun]); + strcpy((char*)ld->alias, codename_adjective[adjective]); + strcat((char*)ld->alias, codename_noun[noun]); } } From 7427c8a22bcc89f02ae31335a94b26ce7b31c190 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 24 Nov 2017 15:03:22 +0100 Subject: [PATCH 04/25] gossip: Passing alias, color and wireaddrs through to gossipd First step towards `gossipd` managing the `node_announcement`. Signed-off-by: Christian Decker --- gossipd/gossip.c | 8 +++++++- gossipd/gossip_wire.csv | 4 ++++ lightningd/gossip_control.c | 3 ++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index 197db965cc67..b8629c8e834e 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -71,6 +71,10 @@ struct daemon { /* Local and global features to offer to peers. */ u8 *localfeatures, *globalfeatures; + + u8 alias[33]; + u8 rgb[3]; + struct wireaddr *wireaddrs; }; /* Peers we're trying to reach. */ @@ -1111,7 +1115,9 @@ static struct io_plan *gossip_init(struct daemon_conn *master, &daemon->broadcast_interval, &chain_hash, &daemon->id, &port, &daemon->globalfeatures, - &daemon->localfeatures)) { + &daemon->localfeatures, + &daemon->wireaddrs, + daemon->rgb, daemon->alias)) { master_badmsg(WIRE_GOSSIPCTL_INIT, msg); } daemon->rstate = new_routing_state(daemon, &chain_hash, &daemon->id); diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index a0c59a66fb1b..02f89903fc9d 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -12,6 +12,10 @@ gossipctl_init,,gflen,u16 gossipctl_init,,gfeatures,gflen*u8 gossipctl_init,,lflen,u16 gossipctl_init,,lfeatures,lflen*u8 +gossipctl_init,,num_wireaddrs,u16 +gossipctl_init,,wireaddrs,num_wireaddrs*struct wireaddr +gossipctl_init,,rgb,3*u8 +gossipctl_init,,alias,32*u8 # Master -> gossipd: Optional hint for where to find peer. gossipctl_peer_addrhint,3014 diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 0d7255ec69a2..2bc60420194e 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -126,7 +126,8 @@ void gossip_init(struct lightningd *ld) &get_chainparams(ld)->genesis_blockhash, &ld->id, ld->portnum, get_supported_global_features(tmpctx), - get_supported_local_features(tmpctx)); + get_supported_local_features(tmpctx), + ld->wireaddrs, ld->rgb, ld->alias); subd_send_msg(ld->gossip, msg); tal_free(tmpctx); } From abe919287c3c887ba1b62c0b52dfcffdcff7c75b Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 24 Nov 2017 15:47:14 +0100 Subject: [PATCH 05/25] routing: Return boolean from handle_channel_announcement This will later be used to determine whether or not we should announce ourselves as a node. Signed-off-by: Christian Decker --- gossipd/gossip.c | 69 +++++++++++++++++++++++++++++++++++++++++++---- gossipd/routing.c | 28 ++++++++++++------- gossipd/routing.h | 13 ++++++++- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index b8629c8e834e..e8b852236edb 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -43,6 +44,7 @@ #include #include #include +#include #define HSM_FD 3 @@ -371,12 +373,69 @@ static struct io_plan *owner_msg_in(struct io_conn *conn, static struct io_plan *nonlocal_dump_gossip(struct io_conn *conn, struct daemon_conn *dc); -static void handle_gossip_msg(struct routing_state *rstate, u8 *msg) +/* Create a node_announcement with the given signature. It may be NULL + * in the case we need to create a provisional announcement for the + * HSM to sign. This is typically called twice: once with the dummy + * signature to get it signed and a second time to build the full + * packet with the signature. The timestamp is handed in since that is + * the only thing that may change between the dummy creation and the + * call with a signature.*/ +static u8 *create_node_announcement(const tal_t *ctx, struct daemon *daemon, + secp256k1_ecdsa_signature *sig, + u32 timestamp) { + u8 *features = NULL; + u8 *addresses = tal_arr(ctx, u8, 0); + u8 *announcement; + size_t i; + if (!sig) { + sig = tal(ctx, secp256k1_ecdsa_signature); + memset(sig, 0, sizeof(*sig)); + } + for (i = 0; i < tal_count(daemon->wireaddrs); i++) + towire_wireaddr(&addresses, daemon->wireaddrs+i); + + announcement = + towire_node_announcement(ctx, sig, features, timestamp, + &daemon->id, daemon->rgb, daemon->alias, + addresses); + return announcement; +} + +static void send_node_announcement(struct daemon *daemon) +{ + tal_t *tmpctx = tal_tmpctx(daemon); + u32 timestamp = time_now().ts.tv_sec; + secp256k1_ecdsa_signature sig; + u8 *msg, *nannounce = create_node_announcement(tmpctx, daemon, NULL, timestamp); + + if (!wire_sync_write(HSM_FD, take(towire_hsm_node_announcement_sig_req(tmpctx, nannounce)))) + status_failed(STATUS_FAIL_MASTER_IO, "Could not write to HSM: %s", strerror(errno)); + + msg = wire_sync_read(tmpctx, HSM_FD); + if (!fromwire_hsm_node_announcement_sig_reply(msg, NULL, &sig)) + status_failed(STATUS_FAIL_MASTER_IO, "HSM returned an invalid node_announcement sig"); + + /* We got the signature for out provisional node_announcement back + * from the HSM, create the real announcement and forward it to + * gossipd so it can take care of forwarding it. */ + nannounce = create_node_announcement(tmpctx, daemon, &sig, timestamp); + handle_node_announcement(daemon->rstate, take(nannounce), tal_len(nannounce)); + tal_free(tmpctx); +} + +static void handle_gossip_msg(struct daemon *daemon, u8 *msg) +{ + struct routing_state *rstate = daemon->rstate; int t = fromwire_peektype(msg); switch(t) { case WIRE_CHANNEL_ANNOUNCEMENT: - handle_channel_announcement(rstate, msg, tal_count(msg)); + /* Add the channel_announcement to the routing state, + * it'll tell us whether this is local and signed, so + * we can hand in a node_announcement as well. */ + if(handle_channel_announcement(rstate, msg, tal_count(msg))) { + send_node_announcement(daemon); + } break; case WIRE_NODE_ANNOUNCEMENT: @@ -489,7 +548,7 @@ static struct io_plan *peer_msgin(struct io_conn *conn, case WIRE_CHANNEL_ANNOUNCEMENT: case WIRE_NODE_ANNOUNCEMENT: case WIRE_CHANNEL_UPDATE: - handle_gossip_msg(peer->daemon->rstate, msg); + handle_gossip_msg(peer->daemon, msg); return peer_next_in(conn, peer); case WIRE_PING: @@ -661,7 +720,7 @@ static struct io_plan *owner_msg_in(struct io_conn *conn, int type = fromwire_peektype(msg); if (type == WIRE_CHANNEL_ANNOUNCEMENT || type == WIRE_CHANNEL_UPDATE || type == WIRE_NODE_ANNOUNCEMENT) { - handle_gossip_msg(peer->daemon->rstate, dc->msg_in); + handle_gossip_msg(peer->daemon, dc->msg_in); } else if (type == WIRE_GOSSIP_GET_UPDATE) { handle_get_update(peer, dc->msg_in); } @@ -1161,7 +1220,7 @@ static void handle_forwarded_msg(struct io_conn *conn, struct daemon *daemon, co if (!fromwire_gossip_forwarded_msg(msg, msg, NULL, &payload)) master_badmsg(WIRE_GOSSIP_FORWARDED_MSG, msg); - handle_gossip_msg(daemon->rstate, payload); + handle_gossip_msg(daemon, payload); } static struct io_plan *handshake_out_success(struct io_conn *conn, diff --git a/gossipd/routing.c b/gossipd/routing.c index 736563ff21f7..1d4dcb812308 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -463,12 +463,12 @@ static bool check_channel_announcement( check_signed_hash(&hash, bitcoin2_sig, bitcoin2_key); } -void handle_channel_announcement( +bool handle_channel_announcement( struct routing_state *rstate, const u8 *announce, size_t len) { u8 *serialized; - bool forward = false; + bool forward = false, local, sigfail; secp256k1_ecdsa_signature node_signature_1; secp256k1_ecdsa_signature node_signature_2; struct short_channel_id short_channel_id; @@ -493,7 +493,7 @@ void handle_channel_announcement( &node_id_1, &node_id_2, &bitcoin_key_1, &bitcoin_key_2)) { tal_free(tmpctx); - return; + return false; } /* BOLT #7: @@ -507,7 +507,7 @@ void handle_channel_announcement( type_to_string(tmpctx, struct sha256_double, &chain_hash)); tal_free(tmpctx); - return; + return false; } // FIXME: Check features! @@ -517,24 +517,31 @@ void handle_channel_announcement( type_to_string(trc, struct short_channel_id, &short_channel_id)); - if (!check_channel_announcement(&node_id_1, &node_id_2, &bitcoin_key_1, - &bitcoin_key_2, &node_signature_1, - &node_signature_2, &bitcoin_signature_1, - &bitcoin_signature_2, serialized)) { + local = pubkey_eq(&node_id_1, &rstate->local_id) || + pubkey_eq(&node_id_2, &rstate->local_id); + sigfail = !check_channel_announcement( + &node_id_1, &node_id_2, &bitcoin_key_1, &bitcoin_key_2, + &node_signature_1, &node_signature_2, &bitcoin_signature_1, + &bitcoin_signature_2, serialized); + + if (sigfail && !local) { status_trace( "Signature verification of channel announcement failed"); tal_free(tmpctx); - return; + return false; } forward |= add_channel_direction(rstate, &node_id_1, &node_id_2, &short_channel_id, serialized); forward |= add_channel_direction(rstate, &node_id_2, &node_id_1, &short_channel_id, serialized); + if (!forward) { status_trace("Not forwarding channel_announcement"); tal_free(tmpctx); - return; + /* This will not be forwarded so we do not want to + * announce the node either, others might drop it. */ + return false; } u8 *tag = tal_arr(tmpctx, u8, 0); @@ -543,6 +550,7 @@ void handle_channel_announcement( tag, serialized); tal_free(tmpctx); + return local; } void handle_channel_update(struct routing_state *rstate, const u8 *update, size_t len) diff --git a/gossipd/routing.h b/gossipd/routing.h index 9c51f951240d..8a931333a314 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -83,6 +83,9 @@ struct routing_state { struct broadcast_state *broadcasts; struct sha256_double chain_hash; + + /* Our own ID so we can identify local channels */ + struct pubkey local_id; }; struct route_hop { @@ -117,7 +120,15 @@ struct node_connection *get_connection_by_scid(const struct routing_state *rstat const u8 direction); /* Handlers for incoming messages */ -void handle_channel_announcement(struct routing_state *rstate, const u8 *announce, size_t len); + +/** + * handle_channel_announcement -- Add channel announcement to state + * + * Returns true if the channel was fully signed and is local. This + * means that if we haven't sent a node_announcement just yet, now + * would be a good time. + */ +bool handle_channel_announcement(struct routing_state *rstate, const u8 *announce, size_t len); void handle_channel_update(struct routing_state *rstate, const u8 *update, size_t len); void handle_node_announcement(struct routing_state *rstate, const u8 *node, size_t len); From dfc5a6ed35c775dbfd4fda2d2259aba279c07baf Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 2 Dec 2017 23:38:43 +0100 Subject: [PATCH 06/25] routing: Add local and sigfail to trace when receiving cannounce Signed-off-by: Christian Decker --- gossipd/routing.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index 1d4dcb812308..be5396dcc74d 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -513,9 +513,6 @@ bool handle_channel_announcement( // FIXME: Check features! //FIXME(cdecker) Check chain topology for the anchor TX - status_trace("Received channel_announcement for channel %s", - type_to_string(trc, struct short_channel_id, - &short_channel_id)); local = pubkey_eq(&node_id_1, &rstate->local_id) || pubkey_eq(&node_id_2, &rstate->local_id); @@ -524,6 +521,11 @@ bool handle_channel_announcement( &node_signature_1, &node_signature_2, &bitcoin_signature_1, &bitcoin_signature_2, serialized); + status_trace("Received channel_announcement for channel %s, local=%d, sigfail=%d", + type_to_string(trc, struct short_channel_id, + &short_channel_id), local, sigfail); + + if (sigfail && !local) { status_trace( "Signature verification of channel announcement failed"); From aa67236b1c75cda566deb039c71b56f20b4d87db Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 4 Dec 2017 13:40:45 +0100 Subject: [PATCH 07/25] gossip: Forward when we don't have a valid node_announcement yet Signed-off-by: Christian Decker --- gossipd/routing.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index be5396dcc74d..15deb76bc786 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -479,6 +479,7 @@ bool handle_channel_announcement( struct pubkey bitcoin_key_1; struct pubkey bitcoin_key_2; struct sha256_double chain_hash; + struct node_connection *c0, *c1; const tal_t *tmpctx = tal_tmpctx(rstate); u8 *features; @@ -528,18 +529,23 @@ bool handle_channel_announcement( if (sigfail && !local) { status_trace( - "Signature verification of channel announcement failed"); + "Signature verification of non-local channel announcement failed"); tal_free(tmpctx); return false; } - forward |= add_channel_direction(rstate, &node_id_1, &node_id_2, - &short_channel_id, serialized); - forward |= add_channel_direction(rstate, &node_id_2, &node_id_1, - &short_channel_id, serialized); + /* Is this a new connection? */ + c0 = get_connection_by_scid(rstate, &short_channel_id, 0); + c1 = get_connection_by_scid(rstate, &short_channel_id, 1); + forward = !c0 || !c1 || !c0->channel_announcement || !c1->channel_announcement; - if (!forward) { - status_trace("Not forwarding channel_announcement"); + add_channel_direction(rstate, &node_id_1, &node_id_2, &short_channel_id, + sigfail ? NULL : serialized); + add_channel_direction(rstate, &node_id_2, &node_id_1, &short_channel_id, + sigfail ? NULL : serialized); + + if (!forward || sigfail) { + status_trace("Not forwarding channel_announcement, forward=%d, sigfail=%d", forward, sigfail); tal_free(tmpctx); /* This will not be forwarded so we do not want to * announce the node either, others might drop it. */ From 88218a055f2bcff2a4ec9c85ea0e7303eddcad1e Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 4 Dec 2017 17:46:50 +0100 Subject: [PATCH 08/25] routing: Do not set an empty channel_announcement if none is given Signed-off-by: Christian Decker --- gossipd/routing.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index 15deb76bc786..3911569ca33f 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -424,9 +424,11 @@ static bool add_channel_direction(struct routing_state *rstate, c = half_add_connection(rstate, from, to, short_channel_id, direction); /* Remember the announcement so we can forward it to new peers */ - tal_free(c->channel_announcement); - c->channel_announcement = tal_dup_arr(c, u8, announcement, - tal_count(announcement), 0); + if (announcement) { + tal_free(c->channel_announcement); + c->channel_announcement = tal_dup_arr(c, u8, announcement, + tal_count(announcement), 0); + } return true; } From c1b4f418a4ebe5df587ea223d1e38bd439525841 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 10:04:27 +1030 Subject: [PATCH 09/25] json_fund_channel: give more details than "peer died". Rather than using the destructor, hook up the cmd so we can close it. peers are allocated off ld, so they are only destroyed explicitly. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 50 ++++++++++++++++++++++----------------- lightningd/peer_control.h | 3 +++ 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 26b88068e032..06bb0310d0b8 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -175,6 +175,16 @@ static void drop_to_chain(struct peer *peer) broadcast_tx(peer->ld->topology, peer, peer->last_tx, NULL); } +/* This lets us give a more detailed error than just a destructor. */ +static void free_peer(struct peer *peer, const char *msg) +{ + if (peer->opening_cmd) { + command_fail(peer->opening_cmd, "%s", msg); + peer->opening_cmd = NULL; + } + tal_free(peer); +} + void peer_fail_permanent(struct peer *peer, const u8 *msg TAKES) { /* BOLT #1: @@ -183,10 +193,10 @@ void peer_fail_permanent(struct peer *peer, const u8 *msg TAKES) * zero (ie. all bytes zero), in which case it refers to all * channels. */ static const struct channel_id all_channels; + const char *why = tal_strndup(NULL, (char *)msg, tal_len(msg)); - log_unusual(peer->log, "Peer permanent failure in %s: %.*s", - peer_state_name(peer->state), - (int)tal_len(msg), (char *)msg); + log_unusual(peer->log, "Peer permanent failure in %s: %s", + peer_state_name(peer->state), why); /* We can have multiple errors, eg. onchaind failures. */ if (!peer->error) @@ -199,7 +209,8 @@ void peer_fail_permanent(struct peer *peer, const u8 *msg TAKES) if (peer_persists(peer)) drop_to_chain(peer); else - tal_free(peer); + free_peer(peer, why); + tal_free(why); return; } @@ -228,12 +239,13 @@ void peer_internal_error(struct peer *peer, const char *fmt, ...) void peer_fail_transient(struct peer *peer, const char *fmt, ...) { va_list ap; + const char *why; va_start(ap, fmt); - log_info(peer->log, "Peer transient failure in %s: ", - peer_state_name(peer->state)); - logv_add(peer->log, fmt, ap); + why = tal_vfmt(peer, fmt, ap); va_end(ap); + log_info(peer->log, "Peer transient failure in %s: %s", + peer_state_name(peer->state), why); #if DEVELOPER if (dev_disconnect_permanent(peer->ld)) { @@ -248,9 +260,10 @@ void peer_fail_transient(struct peer *peer, const char *fmt, ...) if (!peer_persists(peer)) { log_info(peer->log, "Only reached state %s: forgetting", peer_state_name(peer->state)); - tal_free(peer); + free_peer(peer, why); return; } + tal_free(why); /* Reconnect unless we've dropped/are dropping to chain. */ if (!peer_on_chain(peer) && peer->state != CLOSINGD_COMPLETE) { @@ -350,6 +363,7 @@ static struct peer *new_peer(struct lightningd *ld, peer->seed = NULL; peer->our_msatoshi = NULL; peer->state = UNINITIALIZED; + peer->opening_cmd = NULL; peer->channel_info = NULL; peer->last_tx = NULL; peer->last_sig = NULL; @@ -581,7 +595,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg, /* Reconnect: discard old one. */ case OPENINGD: - tal_free(peer); + free_peer(peer, "peer reconnected"); + peer = NULL; goto return_to_gossipd; case ONCHAIND_CHEATED: @@ -981,12 +996,6 @@ struct funding_channel { struct bitcoin_tx *funding_tx; }; -static void fail_fundchannel_command(struct funding_channel *fc) -{ - /* FIXME: More details? */ - command_fail(fc->cmd, "Peer died"); -} - static void funding_broadcast_failed(struct peer *peer, int exitstatus, const char *err) { @@ -1250,7 +1259,7 @@ static void handle_irrevocably_resolved(struct peer *peer, const u8 *msg) log_info(peer->log, "onchaind complete, forgetting peer"); /* This will also free onchaind. */ - tal_free(peer); + free_peer(peer, "onchaind complete, forgetting peer"); } static unsigned int onchain_msg(struct subd *sd, const u8 *msg, const int *fds) @@ -1562,15 +1571,12 @@ static void opening_got_hsm_funding_sig(struct funding_channel *fc, fc->peer->funding_txid, fc->peer->funding_outnum, funding_spent, NULL); - /* We could defer until after funding locked, but makes testing - * harder. */ - tal_del_destructor(fc, fail_fundchannel_command); - json_object_start(response, NULL); linear = linearize_tx(response, tx); json_add_hex(response, "tx", linear, tal_len(linear)); json_object_end(response); - command_success(fc->cmd, response); + command_success(fc->peer->opening_cmd, response); + fc->peer->opening_cmd = NULL; /* Start normal channel daemon. */ peer_start_channeld(fc->peer, cs, peer_fd, gossip_fd, NULL, false); @@ -2522,7 +2528,7 @@ static void peer_offer_channel(struct lightningd *ld, /* Peer now owns fc; if it dies, we fail fc. */ tal_steal(fc->peer, fc); - tal_add_destructor(fc, fail_fundchannel_command); + fc->peer->opening_cmd = fc->cmd; subd_req(fc, fc->peer->owner, take(msg), -1, 2, opening_funder_finished, fc); diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 53c7c8c6d65b..335a1022d545 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -41,6 +41,9 @@ struct peer { /* Which side offered channel? */ enum side funder; + /* Command which ordered us to open channel, if any. */ + struct command *opening_cmd; + /* Inside ld->peers. */ struct list_node list; From e99f69e7b74d4c67b42ee08cf1d46e4d1cbda687 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 10:05:46 +1030 Subject: [PATCH 10/25] subd: add transaction to subd exit corner case. As demonstrated in the test at the end of this series, openingd dying spontaneously causes the conn to be freed which causes the subd to be destroyed, which fails the peer, which hits the db. Signed-off-by: Rusty Russell --- lightningd/subd.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lightningd/subd.c b/lightningd/subd.c index 95d6703357cf..317c8eb69248 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -548,10 +548,20 @@ static void destroy_subd(struct subd *sd) if (sd->peer) { /* Don't loop back when we fail it. */ struct peer *peer = sd->peer; + struct db *db = sd->ld->wallet->db; + bool outer_transaction; + sd->peer = NULL; + + /* We can be freed both inside msg handling, or spontaneously. */ + outer_transaction = db->in_transaction; + if (!outer_transaction) + db_begin_transaction(db); peer_fail_transient(peer, "Owning subdaemon %s died (%i)", sd->name, status); + if (!outer_transaction) + db_commit_transaction(db); } if (sd->must_not_exit) { From 6793efe5cb06b43bc24d9982952aa389d29297d3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 10:06:32 +1030 Subject: [PATCH 11/25] wireaddr: marshal empty address properly. On unmarshal, we stop unmarshaling on a 0 (ADDR_TYPE_PADDING) type. So we should also stop marshaling in that case. Signed-off-by: Rusty Russell --- common/wireaddr.c | 4 ++++ common/wireaddr.h | 1 + 2 files changed, 5 insertions(+) diff --git a/common/wireaddr.c b/common/wireaddr.c index ce4bea332121..9d529bacb8c2 100644 --- a/common/wireaddr.c +++ b/common/wireaddr.c @@ -28,6 +28,10 @@ bool fromwire_wireaddr(const u8 **cursor, size_t *max, struct wireaddr *addr) void towire_wireaddr(u8 **pptr, const struct wireaddr *addr) { + if (!addr || addr->type == ADDR_TYPE_PADDING) { + towire_u8(pptr, ADDR_TYPE_PADDING); + return; + } towire_u8(pptr, addr->type); towire(pptr, addr->addr, addr->addrlen); towire_u16(pptr, addr->port); diff --git a/common/wireaddr.h b/common/wireaddr.h index ae196b9ccaf4..ab51b3c98dfc 100644 --- a/common/wireaddr.h +++ b/common/wireaddr.h @@ -36,6 +36,7 @@ struct wireaddr { u16 port; }; +/* Inserts a single ADDR_TYPE_PADDING if addr is NULL */ void towire_wireaddr(u8 **pptr, const struct wireaddr *addr); bool fromwire_wireaddr(const u8 **cursor, size_t *max, struct wireaddr *addr); #endif /* LIGHTNING_COMMON_WIREADDR_H */ From 139774c96d9b807986815db70a756a50428b742c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 13:52:35 +1030 Subject: [PATCH 12/25] openingd: return to master for more gossip when negotiation fails. We can open other channels, if we want. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 48 +++++++++++++++- lightningd/peer_control.h | 3 + lightningd/subd.c | 1 - openingd/Makefile | 3 +- openingd/opening.c | 118 ++++++++++++++++++++++++-------------- openingd/opening_wire.csv | 8 +++ 6 files changed, 133 insertions(+), 48 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 06bb0310d0b8..a04501b6a185 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -193,7 +193,11 @@ void peer_fail_permanent(struct peer *peer, const u8 *msg TAKES) * zero (ie. all bytes zero), in which case it refers to all * channels. */ static const struct channel_id all_channels; - const char *why = tal_strndup(NULL, (char *)msg, tal_len(msg)); + char *why; + + /* Subtle: we don't want tal_strndup here, it will take() msg! */ + why = tal_arrz(NULL, char, tal_len(msg) + 1); + memcpy(why, msg, tal_len(msg)); log_unusual(peer->log, "Peer permanent failure in %s: %s", peer_state_name(peer->state), why); @@ -2379,6 +2383,42 @@ static void opening_fundee_finished(struct subd *opening, peer_set_condition(peer, OPENINGD, CHANNELD_AWAITING_LOCKIN); } +/* Negotiation failed, but we can keep gossipping */ +static unsigned int opening_negotiation_failed(struct subd *openingd, + const u8 *msg, + const int *fds) +{ + struct crypto_state cs; + struct peer *peer = openingd->peer; + u8 *err; + const char *why; + + /* We need the peer fd. */ + if (tal_count(fds) == 0) + return 1; + + if (!fromwire_opening_negotiation_failed(msg, msg, NULL, &cs, &err)) { + peer_internal_error(peer, + "bad OPENING_NEGOTIATION_FAILED %s", + tal_hex(msg, msg)); + return 0; + } + + /* FIXME: Should we save addr in peer, or should gossipd remember it? */ + msg = towire_gossipctl_handle_peer(msg, &peer->id, NULL, &cs, + peer->gfeatures, peer->lfeatures, + NULL); + subd_send_msg(openingd->ld->gossip, take(msg)); + subd_send_fd(openingd->ld->gossip, fds[0]); + + why = tal_strndup(peer, (const char *)err, tal_len(err)); + log_unusual(peer->log, "Opening negotiation failed: %s", why); + + /* This will free openingd, since that's peer->owner */ + free_peer(peer, why); + return 0; +} + /* Peer has spontaneously exited from gossip due to open msg */ static void peer_accept_channel(struct lightningd *ld, const struct pubkey *peer_id, @@ -2400,7 +2440,8 @@ static void peer_accept_channel(struct lightningd *ld, peer_set_condition(peer, UNINITIALIZED, OPENINGD); peer_set_owner(peer, new_peer_subd(ld, "lightning_openingd", peer, - opening_wire_type_name, NULL, + opening_wire_type_name, + opening_negotiation_failed, take(&peer_fd), take(&gossip_fd), NULL)); if (!peer->owner) { peer_fail_transient(peer, "Failed to subdaemon opening: %s", @@ -2475,7 +2516,8 @@ static void peer_offer_channel(struct lightningd *ld, peer_set_owner(fc->peer, new_peer_subd(ld, "lightning_openingd", fc->peer, - opening_wire_type_name, NULL, + opening_wire_type_name, + opening_negotiation_failed, take(&peer_fd), take(&gossip_fd), NULL)); if (!fc->peer->owner) { fc->peer = tal_free(fc->peer); diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 335a1022d545..cda4610aaf55 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -215,6 +215,9 @@ void peer_fail_permanent_str(struct peer *peer, const char *str TAKES); /* Permanent error, but due to internal problems, not peer. */ void peer_internal_error(struct peer *peer, const char *fmt, ...); +/* Peer has failed to open; return to gossipd. */ +void opening_failed(struct peer *peer, const u8 *msg TAKES); + const char *peer_state_name(enum peer_state state); void peer_set_condition(struct peer *peer, enum peer_state oldstate, enum peer_state state); diff --git a/lightningd/subd.c b/lightningd/subd.c index 317c8eb69248..74610c9e1a51 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -377,7 +377,6 @@ static bool log_status_fail(struct subd *sd, case STATUS_FAIL_INTERNAL_ERROR: name = "STATUS_FAIL_INTERNAL_ERROR"; goto log_str_broken; - /* * These errors happen when the other peer misbehaves: */ diff --git a/openingd/Makefile b/openingd/Makefile index d112d7c41c8c..6575478c85a1 100644 --- a/openingd/Makefile +++ b/openingd/Makefile @@ -59,7 +59,8 @@ OPENINGD_COMMON_OBJS := \ common/type_to_string.o \ common/utils.o \ common/utxo.o \ - common/version.o + common/version.o \ + common/wire_error.o $(LIGHTNINGD_OPENING_OBJS): $(LIGHTNINGD_HEADERS) diff --git a/openingd/opening.c b/openingd/opening.c index 002c7268029b..aee1581f43d7 100644 --- a/openingd/opening.c +++ b/openingd/opening.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -17,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -63,6 +65,34 @@ struct state { const struct chainparams *chainparams; }; +/* For negotiation failures: we can still gossip with client. */ +static void negotiation_failed(struct state *state, const char *fmt, ...) +{ + va_list ap; + const char *errmsg; + u8 *msg; + + va_start(ap, fmt); + errmsg = tal_vfmt(state, fmt, ap); + va_end(ap); + + /* Make sure it's correct length for towire_. */ + tal_resize(&errmsg, strlen(errmsg)+1); + + /* Tell peer we're bailing on this channel. */ + msg = towire_errorfmt(errmsg, &state->channel_id, "%s", errmsg); + sync_crypto_write(&state->cs, PEER_FD, take(msg)); + + /* Tell master we should return to gossiping. */ + msg = towire_opening_negotiation_failed(state, &state->cs, + (const u8 *)errmsg); + wire_sync_write(REQ_FD, msg); + fdpass_send(REQ_FD, PEER_FD); + + tal_free(state); + exit(0); +} + static void check_config_bounds(struct state *state, const struct channel_config *remoteconf) { @@ -75,9 +105,10 @@ static void check_config_bounds(struct state *state, * unreasonably large. */ if (remoteconf->to_self_delay > state->max_to_self_delay) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "to_self_delay %u larger than %u", - remoteconf->to_self_delay, state->max_to_self_delay); + negotiation_failed(state, + "to_self_delay %u larger than %u", + remoteconf->to_self_delay, + state->max_to_self_delay); /* BOLT #2: * @@ -92,11 +123,11 @@ static void check_config_bounds(struct state *state, /* Overflow check before capacity calc. */ if (remoteconf->channel_reserve_satoshis > state->funding_satoshis) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "Invalid channel_reserve_satoshis %"PRIu64 - " for funding_satoshis %"PRIu64, - remoteconf->channel_reserve_satoshis, - state->funding_satoshis); + negotiation_failed(state, + "Invalid channel_reserve_satoshis %"PRIu64 + " for funding_satoshis %"PRIu64, + remoteconf->channel_reserve_satoshis, + state->funding_satoshis); /* Consider highest reserve. */ reserve_msat = remoteconf->channel_reserve_satoshis * 1000; @@ -109,32 +140,32 @@ static void check_config_bounds(struct state *state, capacity_msat = remoteconf->max_htlc_value_in_flight_msat; if (remoteconf->htlc_minimum_msat * (u64)1000 > capacity_msat) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "Invalid htlc_minimum_msat %"PRIu64 - " for funding_satoshis %"PRIu64 - " capacity_msat %"PRIu64, - remoteconf->htlc_minimum_msat, - state->funding_satoshis, - capacity_msat); + negotiation_failed(state, + "Invalid htlc_minimum_msat %"PRIu64 + " for funding_satoshis %"PRIu64 + " capacity_msat %"PRIu64, + remoteconf->htlc_minimum_msat, + state->funding_satoshis, + capacity_msat); if (capacity_msat < state->min_effective_htlc_capacity_msat) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "Channel capacity with funding %"PRIu64" msat," - " reserves %"PRIu64"/%"PRIu64" msat," - " max_htlc_value_in_flight_msat %"PRIu64 - " is %"PRIu64" msat, which is below %"PRIu64" msat", - state->funding_satoshis * 1000, - remoteconf->channel_reserve_satoshis * 1000, - state->localconf.channel_reserve_satoshis * 1000, - remoteconf->max_htlc_value_in_flight_msat, - capacity_msat, - state->min_effective_htlc_capacity_msat); + negotiation_failed(state, + "Channel capacity with funding %"PRIu64" msat," + " reserves %"PRIu64"/%"PRIu64" msat," + " max_htlc_value_in_flight_msat %"PRIu64 + " is %"PRIu64" msat, which is below %"PRIu64" msat", + state->funding_satoshis * 1000, + remoteconf->channel_reserve_satoshis * 1000, + state->localconf.channel_reserve_satoshis * 1000, + remoteconf->max_htlc_value_in_flight_msat, + capacity_msat, + state->min_effective_htlc_capacity_msat); /* We don't worry about how many HTLCs they accept, as long as > 0! */ if (remoteconf->max_accepted_htlcs == 0) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "max_accepted_htlcs %u invalid", - remoteconf->max_accepted_htlcs); + negotiation_failed(state, + "max_accepted_htlcs %u invalid", + remoteconf->max_accepted_htlcs); /* BOLT #2: * @@ -298,7 +329,7 @@ static u8 *funder_channel(struct state *state, * The `temporary_channel_id` MUST be the same as the * `temporary_channel_id` in the `open_channel` message. */ if (!structeq(&id_in, &state->channel_id)) - peer_failed(PEER_FD, &state->cs, &id_in, + peer_failed(PEER_FD, &state->cs, &state->channel_id, "accept_channel ids don't match: sent %s got %s", type_to_string(msg, struct channel_id, &id_in), type_to_string(msg, struct channel_id, @@ -313,9 +344,9 @@ static u8 *funder_channel(struct state *state, * `open_channel`. */ if (minimum_depth > max_minimum_depth) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "minimum_depth %u larger than %u", - minimum_depth, max_minimum_depth); + negotiation_failed(state, + "minimum_depth %u larger than %u", + minimum_depth, max_minimum_depth); check_config_bounds(state, state->remoteconf); /* Now, ask create funding transaction to pay those two addresses. */ @@ -506,10 +537,11 @@ static u8 *fundee_channel(struct state *state, * unknown to the receiver. */ if (!structeq(&chain_hash, &state->chainparams->genesis_blockhash)) { - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "Unknown chain-hash %s", - type_to_string(peer_msg, struct sha256_double, - &chain_hash)); + negotiation_failed(state, + "Unknown chain-hash %s", + type_to_string(peer_msg, + struct sha256_double, + &chain_hash)); } /* BOLT #2 FIXME: @@ -538,14 +570,14 @@ static u8 *fundee_channel(struct state *state, * too small for timely processing, or unreasonably large. */ if (state->feerate_per_kw < min_feerate) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "feerate_per_kw %u below minimum %u", - state->feerate_per_kw, min_feerate); + negotiation_failed(state, + "feerate_per_kw %u below minimum %u", + state->feerate_per_kw, min_feerate); if (state->feerate_per_kw > max_feerate) - peer_failed(PEER_FD, &state->cs, &state->channel_id, - "feerate_per_kw %u above maximum %u", - state->feerate_per_kw, max_feerate); + negotiation_failed(state, + "feerate_per_kw %u above maximum %u", + state->feerate_per_kw, max_feerate); set_reserve(&state->localconf.channel_reserve_satoshis, state->funding_satoshis); diff --git a/openingd/opening_wire.csv b/openingd/opening_wire.csv index 3ebaa8dc7faa..2f3d71fbd7d4 100644 --- a/openingd/opening_wire.csv +++ b/openingd/opening_wire.csv @@ -73,3 +73,11 @@ opening_fundee_reply,,feerate_per_kw,u32 # The (encrypted) funding signed message: send this and we're committed. opening_fundee_reply,,msglen,u16 opening_fundee_reply,,funding_signed_msg,msglen*u8 + +# We disagreed with opening parameters, but peer is ok for gossip (+ peerfd) +opening_negotiation_failed,6010 +opening_negotiation_failed,,crypto_state,struct crypto_state +opening_negotiation_failed,,len,u16 +# FIXME: string support! +opening_negotiation_failed,,msg,len*u8 + From c984fb20d861c97ea4ef526587940137f703fd7b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 13:52:40 +1030 Subject: [PATCH 13/25] openingd: handle ERROR packets (if other end fails negotiation). Signed-off-by: Rusty Russell --- openingd/opening.c | 57 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/openingd/opening.c b/openingd/opening.c index aee1581f43d7..9cf087c48a4e 100644 --- a/openingd/opening.c +++ b/openingd/opening.c @@ -66,7 +66,8 @@ struct state { }; /* For negotiation failures: we can still gossip with client. */ -static void negotiation_failed(struct state *state, const char *fmt, ...) +static void negotiation_failed(struct state *state, bool send_error, + const char *fmt, ...) { va_list ap; const char *errmsg; @@ -79,9 +80,12 @@ static void negotiation_failed(struct state *state, const char *fmt, ...) /* Make sure it's correct length for towire_. */ tal_resize(&errmsg, strlen(errmsg)+1); - /* Tell peer we're bailing on this channel. */ - msg = towire_errorfmt(errmsg, &state->channel_id, "%s", errmsg); - sync_crypto_write(&state->cs, PEER_FD, take(msg)); + /* We don't send error in response to their error packet. */ + if (send_error) { + /* Tell peer we're bailing on this channel. */ + msg = towire_errorfmt(errmsg, &state->channel_id, "%s", errmsg); + sync_crypto_write(&state->cs, PEER_FD, take(msg)); + } /* Tell master we should return to gossiping. */ msg = towire_opening_negotiation_failed(state, &state->cs, @@ -105,7 +109,7 @@ static void check_config_bounds(struct state *state, * unreasonably large. */ if (remoteconf->to_self_delay > state->max_to_self_delay) - negotiation_failed(state, + negotiation_failed(state, true, "to_self_delay %u larger than %u", remoteconf->to_self_delay, state->max_to_self_delay); @@ -123,7 +127,7 @@ static void check_config_bounds(struct state *state, /* Overflow check before capacity calc. */ if (remoteconf->channel_reserve_satoshis > state->funding_satoshis) - negotiation_failed(state, + negotiation_failed(state, true, "Invalid channel_reserve_satoshis %"PRIu64 " for funding_satoshis %"PRIu64, remoteconf->channel_reserve_satoshis, @@ -140,7 +144,7 @@ static void check_config_bounds(struct state *state, capacity_msat = remoteconf->max_htlc_value_in_flight_msat; if (remoteconf->htlc_minimum_msat * (u64)1000 > capacity_msat) - negotiation_failed(state, + negotiation_failed(state, true, "Invalid htlc_minimum_msat %"PRIu64 " for funding_satoshis %"PRIu64 " capacity_msat %"PRIu64, @@ -149,7 +153,7 @@ static void check_config_bounds(struct state *state, capacity_msat); if (capacity_msat < state->min_effective_htlc_capacity_msat) - negotiation_failed(state, + negotiation_failed(state, true, "Channel capacity with funding %"PRIu64" msat," " reserves %"PRIu64"/%"PRIu64" msat," " max_htlc_value_in_flight_msat %"PRIu64 @@ -163,7 +167,7 @@ static void check_config_bounds(struct state *state, /* We don't worry about how many HTLCs they accept, as long as > 0! */ if (remoteconf->max_accepted_htlcs == 0) - negotiation_failed(state, + negotiation_failed(state, true, "max_accepted_htlcs %u invalid", remoteconf->max_accepted_htlcs); @@ -220,6 +224,33 @@ static u8 *read_next_peer_msg(struct state *state, const tal_t *ctx) if (!wire_sync_write(GOSSIP_FD, take(msg))) status_failed(STATUS_FAIL_PEER_IO, "Relaying gossip message"); + } else if (fromwire_peektype(msg) == WIRE_ERROR) { + struct channel_id chanid; + char *err = sanitize_error(msg, msg, &chanid); + + /* BOLT #1: + * + * The channel is referred to by `channel_id`, unless + * `channel_id` is 0 (i.e. all bytes are 0), in which + * case it refers to all channels. + * ... + + * The receiving node: + * - upon receiving `error`: + * - MUST fail the channel referred to by the error + * message. + * - if no existing channel is referred to by the + * message: + * - MUST ignore the message. + */ + if (channel_id_is_all(&chanid)) + peer_failed(PEER_FD, &state->cs, + &state->channel_id, + "Error packet: %s", err); + + if (structeq(&chanid, &state->channel_id)) + negotiation_failed(state, false, + "Error packet: %s", err); } else { return msg; } @@ -344,7 +375,7 @@ static u8 *funder_channel(struct state *state, * `open_channel`. */ if (minimum_depth > max_minimum_depth) - negotiation_failed(state, + negotiation_failed(state, true, "minimum_depth %u larger than %u", minimum_depth, max_minimum_depth); check_config_bounds(state, state->remoteconf); @@ -537,7 +568,7 @@ static u8 *fundee_channel(struct state *state, * unknown to the receiver. */ if (!structeq(&chain_hash, &state->chainparams->genesis_blockhash)) { - negotiation_failed(state, + negotiation_failed(state, true, "Unknown chain-hash %s", type_to_string(peer_msg, struct sha256_double, @@ -570,12 +601,12 @@ static u8 *fundee_channel(struct state *state, * too small for timely processing, or unreasonably large. */ if (state->feerate_per_kw < min_feerate) - negotiation_failed(state, + negotiation_failed(state, true, "feerate_per_kw %u below minimum %u", state->feerate_per_kw, min_feerate); if (state->feerate_per_kw > max_feerate) - negotiation_failed(state, + negotiation_failed(state, true, "feerate_per_kw %u above maximum %u", state->feerate_per_kw, max_feerate); From 6889fe7c53791431f16b807cfa1b5326f0d8ad48 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 14:19:50 +1030 Subject: [PATCH 14/25] test_lightningd: add test for funding failures. We should not disconnect from a peer just because it fails opening; we should return it to gossipd, and give a meaningful error. Closes: #401 Signed-off-by: Rusty Russell --- tests/test_lightningd.py | 55 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index adc05f16a751..961a0f47add7 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -2116,6 +2116,61 @@ def test_funding_change(self): assert outputs[0] > 8990000 assert outputs[2] == 10000000 + def test_funding_fail(self): + """Add some funds, fund a channel without enough funds""" + # Previous runs with same bitcoind can leave funds! + l1 = self.node_factory.get_node(random_hsm=True) + max_locktime = 3 * 6 * 24 + l2 = self.node_factory.get_node(options=['--locktime-blocks={}'.format(max_locktime + 1)]) + l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + + addr = l1.rpc.newaddr()['address'] + txid = l1.bitcoin.rpc.sendtoaddress(addr, 0.01) + bitcoind.generate_block(1) + + # Wait for it to arrive. + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) > 0) + + # Fail because l1 dislikes l2's huge locktime. + try: + l1.rpc.fundchannel(l2.info['id'], 100000) + except ValueError as verr: + str(verr).index('to_self_delay {} larger than {}' + .format(max_locktime+1, max_locktime)) + except Exception as err: + self.fail("Unexpected exception {}".format(err)) + else: + self.fail("huge locktime ignored?") + + # We don't have enough left to cover fees if we try to spend it all. + try: + l1.rpc.fundchannel(l2.info['id'], 1000000) + except ValueError as verr: + str(verr).index('Cannot afford funding transaction') + except Exception as err: + self.fail("Unexpected exception {}".format(err)) + else: + self.fail("We somehow covered fees?") + + # Should still be connected. + assert l1.rpc.getpeers()['peers'][0]['connected'] + assert l2.rpc.getpeers()['peers'][0]['connected'] + + # Restart l2 without ridiculous locktime. + l2.daemon.proc.terminate() + + l2.daemon.cmd_line.remove('--locktime-blocks={}'.format(max_locktime + 1)) + + # Wait for l1 to notice + wait_for(lambda: len(l1.rpc.getpeers()['peers']) == 0) + + # Now restart l2, reconnect. + l2.daemon.start() + l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + + # This works. + l1.rpc.fundchannel(l2.info['id'], int(0.01 * 10**8 / 2)) + def test_addfunds_from_block(self): """Send funds to the daemon without telling it explicitly """ From 616bc26158e4dd6e01a8ee28629cf746bb0943aa Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 16:43:56 +1030 Subject: [PATCH 15/25] daemon_conn: helper to release daemon_conn. We'll want this for the next change, where gossipd migrates remote peers back to local ones. Signed-off-by: Rusty Russell --- common/daemon_conn.c | 6 ++++++ common/daemon_conn.h | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/common/daemon_conn.c b/common/daemon_conn.c index 899a497e8aa1..0928b84b61c1 100644 --- a/common/daemon_conn.c +++ b/common/daemon_conn.c @@ -87,6 +87,12 @@ void daemon_conn_init(tal_t *ctx, struct daemon_conn *dc, int fd, io_set_finish(conn, finish, dc); } +void daemon_conn_clear(struct daemon_conn *dc) +{ + io_set_finish(dc->conn, NULL, NULL); + io_close(dc->conn); +} + void daemon_conn_send(struct daemon_conn *dc, const u8 *msg) { msg_enqueue(&dc->out, msg); diff --git a/common/daemon_conn.h b/common/daemon_conn.h index 456b1af67e5c..fec23720ce4d 100644 --- a/common/daemon_conn.h +++ b/common/daemon_conn.h @@ -43,6 +43,16 @@ void daemon_conn_init(tal_t *ctx, struct daemon_conn *dc, int fd, struct io_plan *(*daemon_conn_recv)( struct io_conn *, struct daemon_conn *), void (*finish)(struct io_conn *, struct daemon_conn *)); + +/** + * daemon_conn_clear - discard a daemon conn without triggering finish. + * @dc: the daemon_conn to clean up. + * + * This is used by gossipd when a peer is handed back, and we no longer + * want to deal with it via daemon_conn. @dc must not be used after this! + */ +void daemon_conn_clear(struct daemon_conn *dc); + /** * daemon_conn_send - Enqueue an outgoing message to be sent */ From 06a01884bc95da1ec4ecdbb5744fd4de4d14f839 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 6 Dec 2017 16:45:06 +1030 Subject: [PATCH 16/25] gossipd: split peer structure to clearly separate local and remote fields. We should also go through and use consistent nomenclature on functions which are used with a local peer ("lpeer_xxx"?) and those with a remote peer ("rpeer_xxx"?) but this is minimal. Signed-off-by: Rusty Russell --- gossipd/gossip.c | 191 +++++++++++++++++++++++++---------------------- 1 file changed, 102 insertions(+), 89 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index e8b852236edb..11fc69317cbb 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -96,6 +96,30 @@ struct reaching { bool succeeded; }; +/* Things we need when we're talking direct to the peer. */ +struct local_peer_state { + /* Cryptostate */ + struct peer_crypto_state pcs; + + /* File descriptor corresponding to conn. */ + int fd; + + /* Our connection (and owner) */ + struct io_conn *conn; + + /* Waiting to send_peer_with_fds to master? */ + bool return_to_master; + + /* If we're exiting due to non-gossip msg, otherwise release */ + u8 *nongossip_msg; + + /* How many pongs are we expecting? */ + size_t num_pings_outstanding; + + /* Message queue for outgoing. */ + struct msg_queue peer_out; +}; + struct peer { struct daemon *daemon; @@ -111,41 +135,18 @@ struct peer { /* Feature bitmaps. */ u8 *gfeatures, *lfeatures; - /* Cryptostate */ - struct peer_crypto_state pcs; - - /* File descriptor corresponding to conn. */ - int fd; - - /* Our connection (and owner) */ - struct io_conn *conn; - /* High water mark for the staggered broadcast */ u64 broadcast_index; - /* Message queue for outgoing. */ - struct msg_queue peer_out; - /* Is it time to continue the staggered broadcast? */ bool gossip_sync; - /* The peer owner will use this to talk to gossipd */ - struct daemon_conn owner_conn; - - /* How many pongs are we expecting? */ - size_t num_pings_outstanding; - - /* Are we the owner of the peer? */ - bool local; - /* If we die, should we reach again? */ bool reach_again; - /* Waiting to send_peer_with_fds to master? */ - bool return_to_master; - - /* If we're exiting due to non-gossip msg, otherwise release */ - u8 *nongossip_msg; + /* Only one of these is set: */ + struct local_peer_state *local; + struct daemon_conn *remote; }; struct addrhint { @@ -198,6 +199,20 @@ static struct addrhint *find_addrhint(struct daemon *daemon, return NULL; } +static struct local_peer_state * +new_local_peer_state(struct peer *peer, const struct crypto_state *cs) +{ + struct local_peer_state *lps = tal(peer, struct local_peer_state); + + init_peer_crypto_state(peer, &lps->pcs); + lps->pcs.cs = *cs; + lps->return_to_master = false; + lps->num_pings_outstanding = 0; + msg_queue_init(&lps->peer_out, peer); + + return lps; +} + static struct peer *new_peer(const tal_t *ctx, struct daemon *daemon, const struct pubkey *their_id, @@ -206,17 +221,13 @@ static struct peer *new_peer(const tal_t *ctx, { struct peer *peer = tal(ctx, struct peer); - init_peer_crypto_state(peer, &peer->pcs); - peer->pcs.cs = *cs; peer->id = *their_id; peer->addr = *addr; peer->daemon = daemon; - peer->local = true; + peer->local = new_local_peer_state(peer, cs); + peer->remote = NULL; peer->reach_again = false; - peer->return_to_master = false; - peer->num_pings_outstanding = 0; peer->broadcast_index = 0; - msg_queue_init(&peer->peer_out, peer); return peer; } @@ -277,7 +288,7 @@ static void peer_error(struct peer *peer, const char *fmt, ...) /* Send error: we'll close after writing this. */ va_start(ap, fmt); - msg_enqueue(&peer->peer_out, + msg_enqueue(&peer->local->peer_out, take(towire_errorfmtv(peer, NULL, fmt, ap))); va_end(ap); } @@ -320,7 +331,7 @@ static struct io_plan *peer_init_received(struct io_conn *conn, /* We will not have anything queued, since we're not duplex. */ msg = towire_gossip_peer_connected(peer, &peer->id, &peer->addr, - &peer->pcs.cs, + &peer->local->pcs.cs, peer->gfeatures, peer->lfeatures); if (!send_peer_with_fds(peer, msg)) return io_close(conn); @@ -341,7 +352,7 @@ static struct io_plan *read_init(struct io_conn *conn, struct peer *peer) * Each node MUST wait to receive `init` before sending any other * messages. */ - return peer_read_message(conn, &peer->pcs, peer_init_received); + return peer_read_message(conn, &peer->local->pcs, peer_init_received); } /* This creates a temporary peer which is not in the list and is owner @@ -356,7 +367,7 @@ static struct io_plan *init_new_peer(struct io_conn *conn, struct peer *peer = new_peer(conn, daemon, their_id, addr, cs); u8 *initmsg; - peer->fd = io_conn_fd(conn); + peer->local->fd = io_conn_fd(conn); /* BOLT #1: * @@ -365,7 +376,8 @@ static struct io_plan *init_new_peer(struct io_conn *conn, */ initmsg = towire_init(peer, daemon->globalfeatures, daemon->localfeatures); - return peer_write_message(conn, &peer->pcs, take(initmsg), read_init); + return peer_write_message(conn, &peer->local->pcs, + take(initmsg), read_init); } static struct io_plan *owner_msg_in(struct io_conn *conn, @@ -458,7 +470,7 @@ static void handle_ping(struct peer *peer, u8 *ping) } if (pong) - msg_enqueue(&peer->peer_out, take(pong)); + msg_enqueue(&peer->local->peer_out, take(pong)); } static void handle_pong(struct peer *peer, const u8 *pong) @@ -471,12 +483,12 @@ static void handle_pong(struct peer *peer, const u8 *pong) return; } - if (!peer->num_pings_outstanding) { + if (!peer->local->num_pings_outstanding) { peer_error(peer, "Unexpected pong"); return; } - peer->num_pings_outstanding--; + peer->local->num_pings_outstanding--; daemon_conn_send(&peer->daemon->master, take(towire_gossip_ping_reply(pong, true, tal_len(pong)))); @@ -493,24 +505,23 @@ static void fail_release(struct peer *peer) static struct io_plan *ready_for_master(struct io_conn *conn, struct peer *peer) { u8 *msg; - if (peer->nongossip_msg) + if (peer->local->nongossip_msg) msg = towire_gossip_peer_nongossip(peer, &peer->id, &peer->addr, - &peer->pcs.cs, + &peer->local->pcs.cs, peer->gfeatures, peer->lfeatures, - peer->nongossip_msg); + peer->local->nongossip_msg); else msg = towire_gossipctl_release_peer_reply(peer, &peer->addr, - &peer->pcs.cs, + &peer->local->pcs.cs, peer->gfeatures, peer->lfeatures); if (send_peer_with_fds(peer, take(msg))) { /* In case we set this earlier. */ tal_del_destructor(peer, fail_release); - peer->return_to_master = false; return io_close_taken_fd(conn); } else return io_close(conn); @@ -523,14 +534,14 @@ static struct io_plan *peer_msgin(struct io_conn *conn, * pass up to master */ static struct io_plan *peer_next_in(struct io_conn *conn, struct peer *peer) { - if (peer->return_to_master) { - assert(!peer_in_started(conn, &peer->pcs)); - if (!peer_out_started(conn, &peer->pcs)) + if (peer->local->return_to_master) { + assert(!peer_in_started(conn, &peer->local->pcs)); + if (!peer_out_started(conn, &peer->local->pcs)) return ready_for_master(conn, peer); return io_wait(conn, peer, peer_next_in, peer); } - return peer_read_message(conn, &peer->pcs, peer_msgin); + return peer_read_message(conn, &peer->local->pcs, peer_msgin); } static struct io_plan *peer_msgin(struct io_conn *conn, @@ -577,8 +588,8 @@ static struct io_plan *peer_msgin(struct io_conn *conn, case WIRE_REVOKE_AND_ACK: case WIRE_INIT: /* Not our place to handle this, so we punt */ - peer->return_to_master = true; - peer->nongossip_msg = tal_steal(peer, msg); + peer->local->return_to_master = true; + peer->local->nongossip_msg = tal_steal(peer, msg); /* This will wait. */ return peer_next_in(conn, peer); @@ -609,28 +620,32 @@ static void wake_pkt_out(struct peer *peer) new_reltimer(&peer->daemon->timers, peer, time_from_msec(peer->daemon->broadcast_interval), wake_pkt_out, peer); - /* Notify the peer-write loop */ - msg_wake(&peer->peer_out); - /* Notify the daemon_conn-write loop */ - msg_wake(&peer->owner_conn.out); + + if (peer->local) + /* Notify the peer-write loop */ + msg_wake(&peer->local->peer_out); + else + /* Notify the daemon_conn-write loop */ + msg_wake(&peer->remote->out); } static struct io_plan *peer_pkt_out(struct io_conn *conn, struct peer *peer) { /* First priority is queued packets, if any */ - const u8 *out = msg_dequeue(&peer->peer_out); + const u8 *out = msg_dequeue(&peer->local->peer_out); if (out) { if (is_all_channel_error(out)) - return peer_write_message(conn, &peer->pcs, take(out), + return peer_write_message(conn, &peer->local->pcs, + take(out), peer_close_after_error); - return peer_write_message(conn, &peer->pcs, take(out), + return peer_write_message(conn, &peer->local->pcs, take(out), peer_pkt_out); } /* Do we want to send this peer to the master daemon? */ - if (peer->return_to_master) { - assert(!peer_out_started(conn, &peer->pcs)); - if (!peer_in_started(conn, &peer->pcs)) + if (peer->local->return_to_master) { + assert(!peer_out_started(conn, &peer->local->pcs)); + if (!peer_in_started(conn, &peer->local->pcs)) return ready_for_master(conn, peer); return io_out_wait(conn, peer, peer_pkt_out, peer); } @@ -643,14 +658,14 @@ static struct io_plan *peer_pkt_out(struct io_conn *conn, struct peer *peer) &peer->broadcast_index); if (next) - return peer_write_message(conn, &peer->pcs, + return peer_write_message(conn, &peer->local->pcs, next->payload, peer_pkt_out); /* Gossip is drained. Wait for next timer. */ peer->gossip_sync = false; } - return msg_queue_wait(conn, &peer->peer_out, peer_pkt_out, peer); + return msg_queue_wait(conn, &peer->local->peer_out, peer_pkt_out, peer); } /* Now we're a fully-fledged peer. */ @@ -704,7 +719,7 @@ static void handle_get_update(struct peer *peer, const u8 *msg) reply: msg = towire_gossip_get_update_reply(msg, update); - daemon_conn_send(&peer->owner_conn, take(msg)); + daemon_conn_send(peer->remote, take(msg)); } /** @@ -714,7 +729,7 @@ static void handle_get_update(struct peer *peer, const u8 *msg) static struct io_plan *owner_msg_in(struct io_conn *conn, struct daemon_conn *dc) { - struct peer *peer = container_of(dc, struct peer, owner_conn); + struct peer *peer = dc->ctx; u8 *msg = dc->msg_in; int type = fromwire_peektype(msg); @@ -744,6 +759,7 @@ static void forget_peer(struct io_conn *conn, struct daemon_conn *dc) static bool send_peer_with_fds(struct peer *peer, const u8 *msg) { int fds[2]; + int peer_fd = peer->local->fd; if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) { status_trace("Failed to create socketpair: %s", @@ -755,21 +771,19 @@ static bool send_peer_with_fds(struct peer *peer, const u8 *msg) } /* Now we talk to socket to get to peer's owner daemon. */ - peer->local = false; - - daemon_conn_init(peer, &peer->owner_conn, fds[0], + peer->local = tal_free(peer->local); + peer->remote = tal(peer, struct daemon_conn); + daemon_conn_init(peer, peer->remote, fds[0], owner_msg_in, forget_peer); - peer->owner_conn.msg_queue_cleared_cb = nonlocal_dump_gossip; + peer->remote->msg_queue_cleared_cb = nonlocal_dump_gossip; /* Peer stays around, even though caller will close conn. */ tal_steal(peer->daemon, peer); daemon_conn_send(&peer->daemon->master, msg); - daemon_conn_send_fd(&peer->daemon->master, peer->fd); + daemon_conn_send_fd(&peer->daemon->master, peer_fd); daemon_conn_send_fd(&peer->daemon->master, fds[1]); - /* Don't get confused: we can't use this any more. */ - peer->fd = -1; return true; } @@ -781,19 +795,17 @@ static bool send_peer_with_fds(struct peer *peer, const u8 *msg) static struct io_plan *nonlocal_dump_gossip(struct io_conn *conn, struct daemon_conn *dc) { struct queued_message *next; - struct peer *peer = container_of(dc, struct peer, owner_conn); + struct peer *peer = dc->ctx; /* Make sure we are not connected directly */ - if (peer->local) - return msg_queue_wait(conn, &peer->owner_conn.out, - daemon_conn_write_next, dc); + assert(!peer->local); next = next_broadcast_message(peer->daemon->rstate->broadcasts, &peer->broadcast_index); if (!next) { - return msg_queue_wait(conn, &peer->owner_conn.out, + return msg_queue_wait(conn, &peer->remote->out, daemon_conn_write_next, dc); } else { return io_write_wire(conn, next->payload, nonlocal_dump_gossip, dc); @@ -802,14 +814,15 @@ static struct io_plan *nonlocal_dump_gossip(struct io_conn *conn, struct daemon_ static struct io_plan *new_peer_got_fd(struct io_conn *conn, struct peer *peer) { - peer->conn = io_new_conn(conn, peer->fd, peer_start_gossip, peer); - if (!peer->conn) { + peer->local->conn = io_new_conn(conn, peer->local->fd, + peer_start_gossip, peer); + if (!peer->local->conn) { status_trace("Could not create connection for peer: %s", strerror(errno)); tal_free(peer); } else { /* If conn dies, we forget peer. */ - tal_steal(peer->conn, peer); + tal_steal(peer->local->conn, peer); } return daemon_conn_read_next(conn, &peer->daemon->master); } @@ -860,9 +873,9 @@ static struct io_plan *handle_peer(struct io_conn *conn, struct daemon *daemon, peer_finalized(peer); if (tal_len(inner_msg)) - msg_enqueue(&peer->peer_out, take(inner_msg)); + msg_enqueue(&peer->local->peer_out, take(inner_msg)); - return io_recv_fd(conn, &peer->fd, new_peer_got_fd, peer); + return io_recv_fd(conn, &peer->local->fd, new_peer_got_fd, peer); } static struct io_plan *release_peer(struct io_conn *conn, struct daemon *daemon, @@ -875,21 +888,21 @@ static struct io_plan *release_peer(struct io_conn *conn, struct daemon *daemon, master_badmsg(WIRE_GOSSIPCTL_RELEASE_PEER, msg); peer = find_peer(daemon, &id); - if (!peer || !peer->local || peer->return_to_master) { + if (!peer || !peer->local || peer->local->return_to_master) { /* This can happen with dying peers, or reconnect */ status_trace("release_peer: peer %s %s", type_to_string(trc, struct pubkey, &id), !peer ? "not found" - : peer->return_to_master ? "already releasing" + : peer->local ? "already releasing" : "not local"); msg = towire_gossipctl_release_peer_replyfail(msg); daemon_conn_send(&daemon->master, take(msg)); } else { - peer->return_to_master = true; - peer->nongossip_msg = NULL; + peer->local->return_to_master = true; + peer->local->nongossip_msg = NULL; /* Wake output, in case it's idle. */ - msg_wake(&peer->peer_out); + msg_wake(&peer->local->peer_out); } return daemon_conn_read_next(conn, &daemon->master); } @@ -1002,7 +1015,7 @@ static struct io_plan *ping_req(struct io_conn *conn, struct daemon *daemon, if (tal_len(ping) > 65535) status_failed(STATUS_FAIL_MASTER_IO, "Oversize ping"); - msg_enqueue(&peer->peer_out, take(ping)); + msg_enqueue(&peer->local->peer_out, take(ping)); status_trace("sending ping expecting %sresponse", num_pong_bytes >= 65532 ? "no " : ""); @@ -1016,7 +1029,7 @@ static struct io_plan *ping_req(struct io_conn *conn, struct daemon *daemon, daemon_conn_send(&daemon->master, take(towire_gossip_ping_reply(peer, true, 0))); else - peer->num_pings_outstanding++; + peer->local->num_pings_outstanding++; out: return daemon_conn_read_next(conn, &daemon->master); From 62634e9d3b55fa55b0826ef2bd247ad81187a096 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 13:22:57 +1030 Subject: [PATCH 17/25] Makefile: make gossipd objects depend correctly on its own headers. Signed-off-by: Rusty Russell --- gossipd/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gossipd/Makefile b/gossipd/Makefile index 71fcc9b54c32..55afc3100e94 100644 --- a/gossipd/Makefile +++ b/gossipd/Makefile @@ -57,7 +57,7 @@ GOSSIPD_COMMON_OBJS := \ hsmd/gen_hsm_client_wire.o \ lightningd/gossip_msg.o -$(LIGHTNINGD_GOSSIP_OBJS) $(LIGHTNINGD_GOSSIP_CLIENT_OBJS): $(LIGHTNINGD_HEADERS) +$(LIGHTNINGD_GOSSIP_OBJS) $(LIGHTNINGD_GOSSIP_CLIENT_OBJS): $(LIGHTNINGD_HEADERS) $(LIGHTNINGD_GOSSIP_HEADERS) $(LIGHTNINGD_GOSSIP_CONTROL_OBJS) : $(LIGHTNINGD_GOSSIP_CONTROL_HEADERS) From c552fa8ff26b5b6fc64a05c47763cdabe7030e06 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 13:23:28 +1030 Subject: [PATCH 18/25] subd: if a required daemon exits, wait instead of killing it. Otherwise we always say it died because we killed it, so we don't get the exit status. Signed-off-by: Rusty Russell --- lightningd/subd.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lightningd/subd.c b/lightningd/subd.c index 74610c9e1a51..a1d79c4608fd 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -523,8 +523,13 @@ static void destroy_subd(struct subd *sd) switch (waitpid(sd->pid, &status, WNOHANG)) { case 0: - log_debug(sd->log, "Status closed, but not exited. Killing"); - kill(sd->pid, SIGKILL); + /* If it's an essential daemon, don't kill: we want the + * exit status */ + if (!sd->must_not_exit) { + log_debug(sd->log, + "Status closed, but not exited. Killing"); + kill(sd->pid, SIGKILL); + } waitpid(sd->pid, &status, 0); fail_if_subd_fails = false; break; From b9eddbe00e1e87dde6c8bd2837ebb26a222e01ab Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 13:46:50 +1030 Subject: [PATCH 19/25] gossipd: don't hand length to route code, it's implied. Signed-off-by: Rusty Russell --- gossipd/gossip.c | 8 ++++---- gossipd/routing.c | 9 ++++++--- gossipd/routing.h | 6 +++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index 11fc69317cbb..f68d5c00bba5 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -432,7 +432,7 @@ static void send_node_announcement(struct daemon *daemon) * from the HSM, create the real announcement and forward it to * gossipd so it can take care of forwarding it. */ nannounce = create_node_announcement(tmpctx, daemon, &sig, timestamp); - handle_node_announcement(daemon->rstate, take(nannounce), tal_len(nannounce)); + handle_node_announcement(daemon->rstate, take(nannounce)); tal_free(tmpctx); } @@ -445,17 +445,17 @@ static void handle_gossip_msg(struct daemon *daemon, u8 *msg) /* Add the channel_announcement to the routing state, * it'll tell us whether this is local and signed, so * we can hand in a node_announcement as well. */ - if(handle_channel_announcement(rstate, msg, tal_count(msg))) { + if(handle_channel_announcement(rstate, msg)) { send_node_announcement(daemon); } break; case WIRE_NODE_ANNOUNCEMENT: - handle_node_announcement(rstate, msg, tal_count(msg)); + handle_node_announcement(rstate, msg); break; case WIRE_CHANNEL_UPDATE: - handle_channel_update(rstate, msg, tal_count(msg)); + handle_channel_update(rstate, msg); break; } } diff --git a/gossipd/routing.c b/gossipd/routing.c index 3911569ca33f..a02361f481bb 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -467,7 +467,7 @@ static bool check_channel_announcement( bool handle_channel_announcement( struct routing_state *rstate, - const u8 *announce, size_t len) + const u8 *announce) { u8 *serialized; bool forward = false, local, sigfail; @@ -484,6 +484,7 @@ bool handle_channel_announcement( struct node_connection *c0, *c1; const tal_t *tmpctx = tal_tmpctx(rstate); u8 *features; + size_t len = tal_len(announce); serialized = tal_dup_arr(tmpctx, u8, announce, len, 0); if (!fromwire_channel_announcement(tmpctx, serialized, NULL, @@ -563,7 +564,7 @@ bool handle_channel_announcement( return local; } -void handle_channel_update(struct routing_state *rstate, const u8 *update, size_t len) +void handle_channel_update(struct routing_state *rstate, const u8 *update) { u8 *serialized; struct node_connection *c; @@ -577,6 +578,7 @@ void handle_channel_update(struct routing_state *rstate, const u8 *update, size_ u32 fee_proportional_millionths; const tal_t *tmpctx = tal_tmpctx(rstate); struct sha256_double chain_hash; + size_t len = tal_len(update); serialized = tal_dup_arr(tmpctx, u8, update, len, 0); if (!fromwire_channel_update(serialized, NULL, &signature, @@ -684,7 +686,7 @@ static struct wireaddr *read_addresses(const tal_t *ctx, const u8 *ser) } void handle_node_announcement( - struct routing_state *rstate, const u8 *node_ann, size_t len) + struct routing_state *rstate, const u8 *node_ann) { u8 *serialized; struct sha256_double hash; @@ -697,6 +699,7 @@ void handle_node_announcement( u8 *features, *addresses; const tal_t *tmpctx = tal_tmpctx(rstate); struct wireaddr *wireaddrs; + size_t len = tal_len(node_ann); serialized = tal_dup_arr(tmpctx, u8, node_ann, len, 0); if (!fromwire_node_announcement(tmpctx, serialized, NULL, diff --git a/gossipd/routing.h b/gossipd/routing.h index 8a931333a314..c35741a94365 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -128,9 +128,9 @@ struct node_connection *get_connection_by_scid(const struct routing_state *rstat * means that if we haven't sent a node_announcement just yet, now * would be a good time. */ -bool handle_channel_announcement(struct routing_state *rstate, const u8 *announce, size_t len); -void handle_channel_update(struct routing_state *rstate, const u8 *update, size_t len); -void handle_node_announcement(struct routing_state *rstate, const u8 *node, size_t len); +bool handle_channel_announcement(struct routing_state *rstate, const u8 *announce); +void handle_channel_update(struct routing_state *rstate, const u8 *update); +void handle_node_announcement(struct routing_state *rstate, const u8 *node); /* Compute a route to a destination, for a given amount and riskfactor. */ struct route_hop *get_route(tal_t *ctx, struct routing_state *rstate, From 09151fd95eccb3c00f9dfe755f9ffc104492145d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 13:46:50 +1030 Subject: [PATCH 20/25] gossipd: hand back peer, don't hand a new peer. All peers come from gossipd, and maintain an fd to talk to it. Sometimes we hand the peer back, but to avoid a race, we always recreated it. The race was that a daemon closed the gossip_fd, which made gossipd forget the peer, then master handed the peer back to gossipd. We stop the race by never closing the gossipfd, but hand it back to gossipd for closing. Now gossipd has to accept two fds, but the handling of peers is far clearer. Signed-off-by: Rusty Russell --- gossipd/gossip.c | 109 ++++++++++++++++++++++-------------- gossipd/gossip_wire.csv | 17 ++---- lightningd/gossip_control.c | 2 +- lightningd/peer_control.c | 25 ++++----- lightningd/peer_control.h | 3 - openingd/opening.c | 1 + tests/test_lightningd.py | 30 +++++----- 7 files changed, 100 insertions(+), 87 deletions(-) diff --git a/gossipd/gossip.c b/gossipd/gossip.c index f68d5c00bba5..ab38d880ba5b 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -827,55 +827,80 @@ static struct io_plan *new_peer_got_fd(struct io_conn *conn, struct peer *peer) return daemon_conn_read_next(conn, &peer->daemon->master); } -/* Read and close fd */ -static struct io_plan *discard_peer_fd(struct io_conn *conn, int *fd) -{ - struct daemon *daemon = tal_parent(fd); - close(*fd); - tal_free(fd); - return daemon_conn_read_next(conn, &daemon->master); -} +/* This lets us read the fds in before handling anything. */ +struct returning_peer { + struct daemon *daemon; + struct pubkey id; + struct crypto_state cs; + u8 *inner_msg; + int peer_fd, gossip_fd; +}; -static struct io_plan *handle_peer(struct io_conn *conn, struct daemon *daemon, - const u8 *msg) +static struct io_plan *handle_returning_peer(struct io_conn *conn, + struct returning_peer *rpeer) { + struct daemon *daemon = rpeer->daemon; struct peer *peer; - struct crypto_state cs; - struct pubkey id; - struct wireaddr addr; - u8 *gfeatures, *lfeatures; - u8 *inner_msg; - if (!fromwire_gossipctl_handle_peer(msg, msg, NULL, &id, &addr, &cs, - &gfeatures, &lfeatures, &inner_msg)) - master_badmsg(WIRE_GOSSIPCTL_HANDLE_PEER, msg); + peer = find_peer(daemon, &rpeer->id); + if (!peer) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "hand_back_peer unknown peer: %s", + type_to_string(trc, struct pubkey, &rpeer->id)); - /* If it already exists locally, that's probably a reconnect: - * drop this one. If it exists as remote, replace with this.*/ - peer = find_peer(daemon, &id); - if (peer) { - if (peer->local) { - int *fd = tal(daemon, int); - status_trace("handle_peer %s: duplicate, dropping", - type_to_string(trc, struct pubkey, &id)); - return io_recv_fd(conn, fd, discard_peer_fd, fd); - } - status_trace("handle_peer %s: found remote duplicate, dropping", - type_to_string(trc, struct pubkey, &id)); - tal_free(peer); + /* We don't need the gossip_fd. We could drain it, so no gossip msgs + * are missed, but that seems overkill. */ + close(rpeer->gossip_fd); + + /* Possible if there's a reconnect: ignore handed back. */ + if (peer->local) { + status_trace("hand_back_peer %s: reconnected, dropping handback", + type_to_string(trc, struct pubkey, &rpeer->id)); + + close(rpeer->peer_fd); + tal_free(rpeer); + return daemon_conn_read_next(conn, &daemon->master); } - status_trace("handle_peer %s: new peer", - type_to_string(trc, struct pubkey, &id)); - peer = new_peer(daemon, daemon, &id, &addr, &cs); - peer->gfeatures = tal_steal(peer, gfeatures); - peer->lfeatures = tal_steal(peer, lfeatures); - peer_finalized(peer); + status_trace("hand_back_peer %s: now local again", + type_to_string(trc, struct pubkey, &rpeer->id)); + + /* Now we talk to peer directly again. */ + daemon_conn_clear(peer->remote); + peer->remote = tal_free(peer->remote); + + peer->local = new_local_peer_state(peer, &rpeer->cs); + peer->local->fd = rpeer->peer_fd; + + /* If they told us to send a message, queue it now */ + if (tal_len(rpeer->inner_msg)) + msg_enqueue(&peer->local->peer_out, take(rpeer->inner_msg)); + tal_free(rpeer); + + return new_peer_got_fd(conn, peer); +} + +static struct io_plan *read_returning_gossipfd(struct io_conn *conn, + struct returning_peer *rpeer) +{ + return io_recv_fd(conn, &rpeer->gossip_fd, + handle_returning_peer, rpeer); +} + +static struct io_plan *hand_back_peer(struct io_conn *conn, + struct daemon *daemon, + const u8 *msg) +{ + struct returning_peer *rpeer = tal(daemon, struct returning_peer); - if (tal_len(inner_msg)) - msg_enqueue(&peer->local->peer_out, take(inner_msg)); + rpeer->daemon = daemon; + if (!fromwire_gossipctl_hand_back_peer(msg, msg, NULL, + &rpeer->id, &rpeer->cs, + &rpeer->inner_msg)) + master_badmsg(WIRE_GOSSIPCTL_HAND_BACK_PEER, msg); - return io_recv_fd(conn, &peer->local->fd, new_peer_got_fd, peer); + return io_recv_fd(conn, &rpeer->peer_fd, + read_returning_gossipfd, rpeer); } static struct io_plan *release_peer(struct io_conn *conn, struct daemon *daemon, @@ -1483,8 +1508,8 @@ static struct io_plan *recv_req(struct io_conn *conn, struct daemon_conn *master handle_forwarded_msg(conn, daemon, daemon->master.msg_in); return daemon_conn_read_next(conn, &daemon->master); - case WIRE_GOSSIPCTL_HANDLE_PEER: - return handle_peer(conn, daemon, master->msg_in); + case WIRE_GOSSIPCTL_HAND_BACK_PEER: + return hand_back_peer(conn, daemon, master->msg_in); case WIRE_GOSSIPCTL_REACH_PEER: return reach_peer(conn, daemon, master->msg_in); diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index 02f89903fc9d..9c04108fb838 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -64,17 +64,12 @@ gossipctl_release_peer_reply,,lfeatures,lflen*u8 # Gossipd -> master: reply to gossip_release_peer if we couldn't find the peer. gossipctl_release_peer_replyfail,3204 -# Gossipd -> master: take over peer, with optional msg. (+peer fd) -gossipctl_handle_peer,3013 -gossipctl_handle_peer,,id,struct pubkey -gossipctl_handle_peer,,addr,struct wireaddr -gossipctl_handle_peer,,crypto_state,struct crypto_state -gossipctl_handle_peer,,gflen,u16 -gossipctl_handle_peer,,gfeatures,gflen*u8 -gossipctl_handle_peer,,lflen,u16 -gossipctl_handle_peer,,lfeatures,lflen*u8 -gossipctl_handle_peer,,len,u16 -gossipctl_handle_peer,,msg,len*u8 +# Gossipd -> master: take back peer, with optional msg. (+peer fd, +gossip fd) +gossipctl_hand_back_peer,3013 +gossipctl_hand_back_peer,,id,struct pubkey +gossipctl_hand_back_peer,,crypto_state,struct crypto_state +gossipctl_hand_back_peer,,len,u16 +gossipctl_hand_back_peer,,msg,len*u8 # Pass JSON-RPC getnodes call through gossip_getnodes_request,3005 diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 2bc60420194e..db78963d4f1c 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -66,7 +66,7 @@ static unsigned gossip_msg(struct subd *gossip, const u8 *msg, const int *fds) case WIRE_GOSSIP_RESOLVE_CHANNEL_REQUEST: case WIRE_GOSSIP_FORWARDED_MSG: case WIRE_GOSSIPCTL_REACH_PEER: - case WIRE_GOSSIPCTL_HANDLE_PEER: + case WIRE_GOSSIPCTL_HAND_BACK_PEER: case WIRE_GOSSIPCTL_RELEASE_PEER: case WIRE_GOSSIPCTL_PEER_ADDRHINT: case WIRE_GOSSIP_GET_UPDATE: diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index a04501b6a185..f91f3a5a952c 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -640,11 +640,10 @@ void peer_connected(struct lightningd *ld, const u8 *msg, return_to_gossipd: /* Otherwise, we hand back to gossipd, to continue. */ - msg = towire_gossipctl_handle_peer(msg, &id, &addr, &cs, - gfeatures, lfeatures, NULL); + msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, NULL); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); - close(gossip_fd); + subd_send_fd(ld->gossip, gossip_fd); /* If we were waiting for connection, we succeeded. */ connect_succeeded(ld, &id); @@ -653,11 +652,10 @@ void peer_connected(struct lightningd *ld, const u8 *msg, send_error: /* Hand back to gossipd, with an error packet. */ connect_failed(ld, &id, sanitize_error(msg, error, NULL)); - msg = towire_gossipctl_handle_peer(msg, &id, &addr, &cs, - gfeatures, lfeatures, error); + msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, error); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); - close(gossip_fd); + subd_send_fd(ld->gossip, gossip_fd); } void peer_sent_nongossip(struct lightningd *ld, @@ -704,11 +702,10 @@ void peer_sent_nongossip(struct lightningd *ld, send_error: /* Hand back to gossipd, with an error packet. */ connect_failed(ld, id, sanitize_error(error, error, NULL)); - msg = towire_gossipctl_handle_peer(error, id, addr, cs, - gfeatures, lfeatures, error); + msg = towire_gossipctl_hand_back_peer(ld, id, cs, error); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); - close(gossip_fd); + subd_send_fd(ld->gossip, gossip_fd); tal_free(error); } @@ -2393,9 +2390,9 @@ static unsigned int opening_negotiation_failed(struct subd *openingd, u8 *err; const char *why; - /* We need the peer fd. */ + /* We need the peer fd and gossip fd. */ if (tal_count(fds) == 0) - return 1; + return 2; if (!fromwire_opening_negotiation_failed(msg, msg, NULL, &cs, &err)) { peer_internal_error(peer, @@ -2404,12 +2401,10 @@ static unsigned int opening_negotiation_failed(struct subd *openingd, return 0; } - /* FIXME: Should we save addr in peer, or should gossipd remember it? */ - msg = towire_gossipctl_handle_peer(msg, &peer->id, NULL, &cs, - peer->gfeatures, peer->lfeatures, - NULL); + msg = towire_gossipctl_hand_back_peer(msg, &peer->id, &cs, NULL); subd_send_msg(openingd->ld->gossip, take(msg)); subd_send_fd(openingd->ld->gossip, fds[0]); + subd_send_fd(openingd->ld->gossip, fds[1]); why = tal_strndup(peer, (const char *)err, tal_len(err)); log_unusual(peer->log, "Opening negotiation failed: %s", why); diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index cda4610aaf55..20f2e1453c38 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -26,9 +26,6 @@ struct peer { /* ID of peer */ struct pubkey id; - /* Global and local features bitfields. */ - const u8 *gfeatures, *lfeatures; - /* Error message (iff in error state) */ u8 *error; diff --git a/openingd/opening.c b/openingd/opening.c index 9cf087c48a4e..f10e3dd7d320 100644 --- a/openingd/opening.c +++ b/openingd/opening.c @@ -92,6 +92,7 @@ static void negotiation_failed(struct state *state, bool send_error, (const u8 *)errmsg); wire_sync_write(REQ_FD, msg); fdpass_send(REQ_FD, PEER_FD); + fdpass_send(REQ_FD, GOSSIP_FD); tal_free(state); exit(0); diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 961a0f47add7..8073ac2d42ce 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -221,8 +221,8 @@ def connect(self): assert ret['id'] == l2.info['id'] - l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') return l1,l2 # Returns the short channel-id: :: @@ -342,8 +342,8 @@ def test_connect(self): assert l2.rpc.getpeer(l1.info['id'])['state'] == 'GOSSIPING' # Both gossipds will have them as new peers once handed back. - l1.daemon.wait_for_log('handle_peer {}: new peer'.format(l2.info['id'])) - l2.daemon.wait_for_log('handle_peer {}: new peer'.format(l1.info['id'])) + l1.daemon.wait_for_log('hand_back_peer {}: now local again'.format(l2.info['id'])) + l2.daemon.wait_for_log('hand_back_peer {}: now local again'.format(l1.info['id'])) def test_balance(self): l1,l2 = self.connect() @@ -709,8 +709,8 @@ def test_bad_opening(self): assert ret['id'] == l2.info['id'] - l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') addr = l1.rpc.newaddr()['address'] txid = l1.bitcoin.rpc.sendtoaddress(addr, 10**6 / 10**8 + 0.01) @@ -1406,7 +1406,7 @@ def test_forward(self): assert ret['id'] == l3.info['id'] - l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') self.fund_channel(l1, l2, 10**6) self.fund_channel(l2, l3, 10**6) @@ -1497,14 +1497,14 @@ def test_forward_different_fees_and_cltv(self): ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) assert ret['id'] == l2.info['id'] - l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') ret = l2.rpc.connect(l3.info['id'], 'localhost', l3.info['port']) assert ret['id'] == l3.info['id'] - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') c1 = self.fund_channel(l1, l2, 10**6) c2 = self.fund_channel(l2, l3, 10**6) @@ -1596,14 +1596,14 @@ def test_forward_pad_fees_and_cltv(self): ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) assert ret['id'] == l2.info['id'] - l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l1.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') ret = l2.rpc.connect(l3.info['id'], 'localhost', l3.info['port']) assert ret['id'] == l3.info['id'] - l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') - l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HANDLE_PEER') + l2.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') + l3.daemon.wait_for_log('WIRE_GOSSIPCTL_HAND_BACK_PEER') c1 = self.fund_channel(l1, l2, 10**6) c2 = self.fund_channel(l2, l3, 10**6) From a4512ead9839b51d156cbe7fb66d96e313b9bf19 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 13:46:50 +1030 Subject: [PATCH 21/25] gossipd: don't increment broadcast_index until *after* message sent. If the peer is moved from remote to local, this may be lost; it's more secure to increment after we've sent the broadcast. --- gossipd/broadcast.c | 16 ++++++++-------- gossipd/broadcast.h | 2 +- gossipd/gossip.c | 31 +++++++++++++++++++++++++++---- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/gossipd/broadcast.c b/gossipd/broadcast.c index c5d118243219..8fc02eefb6c5 100644 --- a/gossipd/broadcast.c +++ b/gossipd/broadcast.c @@ -27,13 +27,13 @@ void queue_broadcast(struct broadcast_state *bstate, const u8 *payload) { struct queued_message *msg; - u64 index = 0; + u64 index; + /* Remove any tag&type collisions */ - while (true) { - msg = next_broadcast_message(bstate, &index); - if (msg == NULL) - break; - else if (msg->type == type && memcmp(msg->tag, tag, tal_count(tag)) == 0) { + for (msg = uintmap_first(&bstate->broadcasts, &index); + msg; + msg = uintmap_after(&bstate->broadcasts, &index)) { + if (msg->type == type && memcmp(msg->tag, tag, tal_count(tag)) == 0) { uintmap_del(&bstate->broadcasts, index); tal_free(msg); } @@ -45,7 +45,7 @@ void queue_broadcast(struct broadcast_state *bstate, bstate->next_index += 1; } -struct queued_message *next_broadcast_message(struct broadcast_state *bstate, u64 *last_index) +struct queued_message *next_broadcast_message(struct broadcast_state *bstate, u64 last_index) { - return uintmap_after(&bstate->broadcasts, last_index); + return uintmap_after(&bstate->broadcasts, &last_index); } diff --git a/gossipd/broadcast.h b/gossipd/broadcast.h index 80545c4f64d7..baa561d4f747 100644 --- a/gossipd/broadcast.h +++ b/gossipd/broadcast.h @@ -35,6 +35,6 @@ void queue_broadcast(struct broadcast_state *bstate, const u8 *tag, const u8 *payload); -struct queued_message *next_broadcast_message(struct broadcast_state *bstate, u64 *last_index); +struct queued_message *next_broadcast_message(struct broadcast_state *bstate, u64 last_index); #endif /* LIGHTNING_LIGHTNINGD_GOSSIP_BROADCAST_H */ diff --git a/gossipd/gossip.c b/gossipd/gossip.c index ab38d880ba5b..e51532e4fe74 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -629,6 +629,17 @@ static void wake_pkt_out(struct peer *peer) msg_wake(&peer->remote->out); } +/* Mutual recursion. */ +static struct io_plan *peer_pkt_out(struct io_conn *conn, struct peer *peer); + +static struct io_plan *local_gossip_broadcast_done(struct io_conn *conn, + struct peer *peer) +{ + status_trace("%s", __func__); + peer->broadcast_index++; + return peer_pkt_out(conn, peer); +} + static struct io_plan *peer_pkt_out(struct io_conn *conn, struct peer *peer) { /* First priority is queued packets, if any */ @@ -655,11 +666,12 @@ static struct io_plan *peer_pkt_out(struct io_conn *conn, struct peer *peer) struct queued_message *next; next = next_broadcast_message(peer->daemon->rstate->broadcasts, - &peer->broadcast_index); + peer->broadcast_index); if (next) return peer_write_message(conn, &peer->local->pcs, - next->payload, peer_pkt_out); + next->payload, + local_gossip_broadcast_done); /* Gossip is drained. Wait for next timer. */ peer->gossip_sync = false; @@ -787,6 +799,16 @@ static bool send_peer_with_fds(struct peer *peer, const u8 *msg) return true; } +static struct io_plan *nonlocal_gossip_broadcast_done(struct io_conn *conn, + struct daemon_conn *dc) +{ + struct peer *peer = dc->ctx; + + status_trace("%s", __func__); + peer->broadcast_index++; + return nonlocal_dump_gossip(conn, dc); +} + /** * nonlocal_dump_gossip - catch the nonlocal peer up with the latest gossip. * @@ -802,13 +824,14 @@ static struct io_plan *nonlocal_dump_gossip(struct io_conn *conn, struct daemon_ assert(!peer->local); next = next_broadcast_message(peer->daemon->rstate->broadcasts, - &peer->broadcast_index); + peer->broadcast_index); if (!next) { return msg_queue_wait(conn, &peer->remote->out, daemon_conn_write_next, dc); } else { - return io_write_wire(conn, next->payload, nonlocal_dump_gossip, dc); + return io_write_wire(conn, next->payload, + nonlocal_gossip_broadcast_done, dc); } } From 2a2902bb1e8a156b3e03041ccbbffd0b195a57e2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 11 Dec 2017 14:03:16 +1030 Subject: [PATCH 22/25] gossipd: hand out gossip_index to other daemons. When gossipd sends a message, have a gossip_index. When it gets back a peer, the current gossip_index is included, so it can know exactly where it's up to. Most of this is mechanical plumbing through openingd, channeld and closingd, even though openingd and closingd don't (currently) read gossip, so their gossip_index will be unchanged. Signed-off-by: Rusty Russell --- channeld/channel.c | 23 +++++++++--- channeld/channel_wire.csv | 2 ++ closingd/closing.c | 6 ++-- closingd/closing_wire.csv | 2 ++ gossipd/gossip.c | 16 +++++++-- gossipd/gossip_wire.csv | 11 +++++- lightningd/gossip_control.c | 7 ++-- lightningd/peer_control.c | 72 ++++++++++++++++++++++++++----------- lightningd/peer_control.h | 1 + openingd/opening.c | 6 +++- openingd/opening_wire.csv | 4 +++ 11 files changed, 116 insertions(+), 34 deletions(-) diff --git a/channeld/channel.c b/channeld/channel.c index 4d0cbbb70f54..33e873099ebe 100644 --- a/channeld/channel.c +++ b/channeld/channel.c @@ -160,6 +160,9 @@ struct peer { u8 channel_flags; bool announce_depth_reached; + + /* Where we got up to in gossip broadcasts. */ + u64 gossip_index; }; static u8 *create_channel_announcement(const tal_t *ctx, struct peer *peer); @@ -176,15 +179,23 @@ static void *tal_arr_append_(void **p, size_t size) static void gossip_in(struct peer *peer, const u8 *msg) { - u16 type = fromwire_peektype(msg); + u8 *gossip; + u16 type; + + if (!fromwire_gossip_send_gossip(msg, msg, NULL, + &peer->gossip_index, &gossip)) + status_failed(STATUS_FAIL_GOSSIP_IO, + "Got bad message from gossipd: %s", + tal_hex(msg, msg)); + type = fromwire_peektype(gossip); if (type == WIRE_CHANNEL_ANNOUNCEMENT || type == WIRE_CHANNEL_UPDATE || type == WIRE_NODE_ANNOUNCEMENT) - msg_enqueue(&peer->peer_out, msg); + msg_enqueue(&peer->peer_out, gossip); else status_failed(STATUS_FAIL_GOSSIP_IO, - "Got bad message from gossipd: %s", - tal_hex(msg, msg)); + "Got bad message type %s from gossipd: %s", + wire_type_name(type), tal_hex(msg, msg)); } static void send_announcement_signatures(struct peer *peer) @@ -2256,6 +2267,7 @@ static void init_channel(struct peer *peer) &peer->feerate_min, &peer->feerate_max, &peer->their_commit_sig, &peer->cs, + &peer->gossip_index, &funding_pubkey[REMOTE], &points[REMOTE].revocation, &points[REMOTE].payment, @@ -2427,7 +2439,8 @@ static void send_shutdown_complete(struct peer *peer) /* Now we can tell master shutdown is complete. */ wire_sync_write(MASTER_FD, take(towire_channel_shutdown_complete(peer, - &peer->cs))); + &peer->cs, + peer->gossip_index))); fdpass_send(MASTER_FD, PEER_FD); fdpass_send(MASTER_FD, GOSSIP_FD); close(MASTER_FD); diff --git a/channeld/channel_wire.csv b/channeld/channel_wire.csv index 2a328102d970..bf49cdf0f618 100644 --- a/channeld/channel_wire.csv +++ b/channeld/channel_wire.csv @@ -18,6 +18,7 @@ channel_init,,feerate_min,u32 channel_init,,feerate_max,u32 channel_init,,first_commit_sig,secp256k1_ecdsa_signature channel_init,,crypto_state,struct crypto_state +channel_init,,gossip_index,u64 channel_init,,remote_fundingkey,struct pubkey channel_init,,remote_revocation_basepoint,struct pubkey channel_init,,remote_payment_basepoint,struct pubkey @@ -183,6 +184,7 @@ channel_got_shutdown,,scriptpubkey,scriptpubkey_len*u8 # Shutdown is complete, ready for closing negotiation. + peer_fd & gossip_fd. channel_shutdown_complete,1025 channel_shutdown_complete,,crypto_state,struct crypto_state +channel_shutdown_complete,,gossip_index,u64 # Re-enable commit timer. channel_dev_reenable_commit,1026 diff --git a/closingd/closing.c b/closingd/closing.c index 0a1b5df3c27a..98ba6423dfe0 100644 --- a/closingd/closing.c +++ b/closingd/closing.c @@ -168,6 +168,7 @@ int main(int argc, char *argv[]) secp256k1_ecdsa_signature sig; bool reconnected; u64 next_index[NUM_SIDES], revocations_received; + u64 gossip_index; if (argc == 2 && streq(argv[1], "--version")) { printf("%s\n", version()); @@ -184,7 +185,7 @@ int main(int argc, char *argv[]) msg = wire_sync_read(ctx, REQ_FD); if (!fromwire_closing_init(ctx, msg, NULL, - &cs, &seed, + &cs, &gossip_index, &seed, &funding_txid, &funding_txout, &funding_satoshi, &funding_pubkey[REMOTE], @@ -473,7 +474,8 @@ int main(int argc, char *argv[]) } /* We're done! */ - wire_sync_write(REQ_FD, take(towire_closing_complete(ctx))); + wire_sync_write(REQ_FD, + take(towire_closing_complete(ctx, gossip_index))); tal_free(ctx); return 0; diff --git a/closingd/closing_wire.csv b/closingd/closing_wire.csv index 12ef79312161..662e6c620a0d 100644 --- a/closingd/closing_wire.csv +++ b/closingd/closing_wire.csv @@ -3,6 +3,7 @@ # Begin! (passes peer fd, gossipd-client fd) closing_init,2001 closing_init,,crypto_state,struct crypto_state +closing_init,,gossip_index,u64 closing_init,,seed,struct privkey closing_init,,funding_txid,struct sha256_double closing_init,,funding_txout,u16 @@ -33,3 +34,4 @@ closing_received_signature_reply,2102 # Negotiations complete, we're exiting. closing_complete,2004 +closing_complete,,gossip_index,u64 diff --git a/gossipd/gossip.c b/gossipd/gossip.c index e51532e4fe74..709105b2be2e 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -332,6 +332,7 @@ static struct io_plan *peer_init_received(struct io_conn *conn, /* We will not have anything queued, since we're not duplex. */ msg = towire_gossip_peer_connected(peer, &peer->id, &peer->addr, &peer->local->pcs.cs, + peer->broadcast_index, peer->gfeatures, peer->lfeatures); if (!send_peer_with_fds(peer, msg)) return io_close(conn); @@ -509,6 +510,7 @@ static struct io_plan *ready_for_master(struct io_conn *conn, struct peer *peer) msg = towire_gossip_peer_nongossip(peer, &peer->id, &peer->addr, &peer->local->pcs.cs, + peer->broadcast_index, peer->gfeatures, peer->lfeatures, peer->local->nongossip_msg); @@ -516,6 +518,7 @@ static struct io_plan *ready_for_master(struct io_conn *conn, struct peer *peer) msg = towire_gossipctl_release_peer_reply(peer, &peer->addr, &peer->local->pcs.cs, + peer->broadcast_index, peer->gfeatures, peer->lfeatures); @@ -830,7 +833,10 @@ static struct io_plan *nonlocal_dump_gossip(struct io_conn *conn, struct daemon_ return msg_queue_wait(conn, &peer->remote->out, daemon_conn_write_next, dc); } else { - return io_write_wire(conn, next->payload, + u8 *msg = towire_gossip_send_gossip(conn, + peer->broadcast_index, + next->payload); + return io_write_wire(conn, take(msg), nonlocal_gossip_broadcast_done, dc); } } @@ -855,6 +861,7 @@ struct returning_peer { struct daemon *daemon; struct pubkey id; struct crypto_state cs; + u64 gossip_index; u8 *inner_msg; int peer_fd, gossip_fd; }; @@ -871,8 +878,8 @@ static struct io_plan *handle_returning_peer(struct io_conn *conn, "hand_back_peer unknown peer: %s", type_to_string(trc, struct pubkey, &rpeer->id)); - /* We don't need the gossip_fd. We could drain it, so no gossip msgs - * are missed, but that seems overkill. */ + /* We don't need the gossip_fd; we know what gossip it got + * from gossip_index */ close(rpeer->gossip_fd); /* Possible if there's a reconnect: ignore handed back. */ @@ -894,6 +901,7 @@ static struct io_plan *handle_returning_peer(struct io_conn *conn, peer->local = new_local_peer_state(peer, &rpeer->cs); peer->local->fd = rpeer->peer_fd; + peer->broadcast_index = rpeer->gossip_index; /* If they told us to send a message, queue it now */ if (tal_len(rpeer->inner_msg)) @@ -919,6 +927,7 @@ static struct io_plan *hand_back_peer(struct io_conn *conn, rpeer->daemon = daemon; if (!fromwire_gossipctl_hand_back_peer(msg, msg, NULL, &rpeer->id, &rpeer->cs, + &rpeer->gossip_index, &rpeer->inner_msg)) master_badmsg(WIRE_GOSSIPCTL_HAND_BACK_PEER, msg); @@ -1555,6 +1564,7 @@ static struct io_plan *recv_req(struct io_conn *conn, struct daemon_conn *master case WIRE_GOSSIP_PEER_NONGOSSIP: case WIRE_GOSSIP_GET_UPDATE: case WIRE_GOSSIP_GET_UPDATE_REPLY: + case WIRE_GOSSIP_SEND_GOSSIP: break; } diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index 9c04108fb838..2f4651663d9a 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -31,6 +31,7 @@ gossip_peer_connected,3002 gossip_peer_connected,,id,struct pubkey gossip_peer_connected,,addr,struct wireaddr gossip_peer_connected,,crypto_state,struct crypto_state +gossip_peer_connected,,gossip_index,u64 gossip_peer_connected,,gflen,u16 gossip_peer_connected,,gfeatures,gflen*u8 gossip_peer_connected,,lflen,u16 @@ -41,6 +42,7 @@ gossip_peer_nongossip,3003 gossip_peer_nongossip,,id,struct pubkey gossip_peer_nongossip,,addr,struct wireaddr gossip_peer_nongossip,,crypto_state,struct crypto_state +gossip_peer_nongossip,,gossip_index,u64 gossip_peer_nongossip,,gflen,u16 gossip_peer_nongossip,,gfeatures,gflen*u8 gossip_peer_nongossip,,lflen,u16 @@ -56,6 +58,7 @@ gossipctl_release_peer,,id,struct pubkey gossipctl_release_peer_reply,3104 gossipctl_release_peer_reply,,addr,struct wireaddr gossipctl_release_peer_reply,,crypto_state,struct crypto_state +gossipctl_release_peer_reply,,gossip_index,u64 gossipctl_release_peer_reply,,gflen,u16 gossipctl_release_peer_reply,,gfeatures,gflen*u8 gossipctl_release_peer_reply,,lflen,u16 @@ -64,10 +67,11 @@ gossipctl_release_peer_reply,,lfeatures,lflen*u8 # Gossipd -> master: reply to gossip_release_peer if we couldn't find the peer. gossipctl_release_peer_replyfail,3204 -# Gossipd -> master: take back peer, with optional msg. (+peer fd, +gossip fd) +# master -> gossipd: take back peer, with optional msg. (+peer fd, +gossip fd) gossipctl_hand_back_peer,3013 gossipctl_hand_back_peer,,id,struct pubkey gossipctl_hand_back_peer,,crypto_state,struct crypto_state +gossipctl_hand_back_peer,,gossip_index,u64 gossipctl_hand_back_peer,,len,u16 gossipctl_hand_back_peer,,msg,len*u8 @@ -141,3 +145,8 @@ gossip_get_update_reply,3112 gossip_get_update_reply,,len,u16 gossip_get_update_reply,,update,len*u8 +# Gossipd can tell channeld etc about gossip to fwd. +gossip_send_gossip,3016 +gossip_send_gossip,,gossip_index,u64 +gossip_send_gossip,,len,u16 +gossip_send_gossip,,gossip,len*u8 diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index db78963d4f1c..2e9a66d62538 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -27,9 +27,10 @@ static void peer_nongossip(struct subd *gossip, const u8 *msg, struct crypto_state cs; struct wireaddr addr; u8 *gfeatures, *lfeatures, *in_pkt; + u64 gossip_index; if (!fromwire_gossip_peer_nongossip(msg, msg, NULL, - &id, &addr, &cs, + &id, &addr, &cs, &gossip_index, &gfeatures, &lfeatures, &in_pkt)) @@ -47,7 +48,8 @@ static void peer_nongossip(struct subd *gossip, const u8 *msg, return; } - peer_sent_nongossip(gossip->ld, &id, &addr, &cs, gfeatures, lfeatures, + peer_sent_nongossip(gossip->ld, &id, &addr, &cs, gossip_index, + gfeatures, lfeatures, peer_fd, gossip_fd, in_pkt); } @@ -70,6 +72,7 @@ static unsigned gossip_msg(struct subd *gossip, const u8 *msg, const int *fds) case WIRE_GOSSIPCTL_RELEASE_PEER: case WIRE_GOSSIPCTL_PEER_ADDRHINT: case WIRE_GOSSIP_GET_UPDATE: + case WIRE_GOSSIP_SEND_GOSSIP: /* This is a reply, so never gets through to here. */ case WIRE_GOSSIP_GET_UPDATE_REPLY: case WIRE_GOSSIP_GETNODES_REPLY: diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f91f3a5a952c..c32dcd634c9e 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -61,21 +61,25 @@ static void peer_offer_channel(struct lightningd *ld, struct funding_channel *fc, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd); static bool peer_start_channeld(struct peer *peer, const struct crypto_state *cs, + u64 gossip_index, int peer_fd, int gossip_fd, const u8 *funding_signed, bool reconnected); static void peer_start_closingd(struct peer *peer, struct crypto_state *cs, + u64 gossip_index, int peer_fd, int gossip_fd, bool reconnected); static void peer_accept_channel(struct lightningd *ld, const struct pubkey *peer_id, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd, const u8 *open_msg); @@ -548,9 +552,10 @@ void peer_connected(struct lightningd *ld, const u8 *msg, u8 *error; struct peer *peer; struct wireaddr addr; + u64 gossip_index; if (!fromwire_gossip_peer_connected(msg, msg, NULL, - &id, &addr, &cs, + &id, &addr, &cs, &gossip_index, &gfeatures, &lfeatures)) fatal("Gossip gave bad GOSSIP_PEER_CONNECTED message %s", tal_hex(msg, msg)); @@ -620,7 +625,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg, peer_set_owner(peer, NULL); peer->addr = addr; - peer_start_channeld(peer, &cs, peer_fd, gossip_fd, NULL, + peer_start_channeld(peer, &cs, gossip_index, + peer_fd, gossip_fd, NULL, true); return; @@ -631,7 +637,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg, peer_set_owner(peer, NULL); peer->addr = addr; - peer_start_closingd(peer, &cs, peer_fd, gossip_fd, + peer_start_closingd(peer, &cs, gossip_index, + peer_fd, gossip_fd, true); return; } @@ -640,7 +647,7 @@ void peer_connected(struct lightningd *ld, const u8 *msg, return_to_gossipd: /* Otherwise, we hand back to gossipd, to continue. */ - msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, NULL); + msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, gossip_index, NULL); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); subd_send_fd(ld->gossip, gossip_fd); @@ -652,7 +659,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg, send_error: /* Hand back to gossipd, with an error packet. */ connect_failed(ld, &id, sanitize_error(msg, error, NULL)); - msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, error); + msg = towire_gossipctl_hand_back_peer(msg, &id, &cs, gossip_index, + error); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); subd_send_fd(ld->gossip, gossip_fd); @@ -662,6 +670,7 @@ void peer_sent_nongossip(struct lightningd *ld, const struct pubkey *id, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd, @@ -689,7 +698,8 @@ void peer_sent_nongossip(struct lightningd *ld, /* Open request? */ if (fromwire_peektype(in_msg) == WIRE_OPEN_CHANNEL) { - peer_accept_channel(ld, id, addr, cs, gfeatures, lfeatures, + peer_accept_channel(ld, id, addr, cs, gossip_index, + gfeatures, lfeatures, peer_fd, gossip_fd, in_msg); return; } @@ -702,7 +712,7 @@ void peer_sent_nongossip(struct lightningd *ld, send_error: /* Hand back to gossipd, with an error packet. */ connect_failed(ld, id, sanitize_error(error, error, NULL)); - msg = towire_gossipctl_hand_back_peer(ld, id, cs, error); + msg = towire_gossipctl_hand_back_peer(ld, id, cs, gossip_index, error); subd_send_msg(ld->gossip, take(msg)); subd_send_fd(ld->gossip, peer_fd); subd_send_fd(ld->gossip, gossip_fd); @@ -1528,7 +1538,8 @@ static enum watch_result funding_lockin_cb(struct peer *peer, static void opening_got_hsm_funding_sig(struct funding_channel *fc, int peer_fd, int gossip_fd, const u8 *resp, - const struct crypto_state *cs) + const struct crypto_state *cs, + u64 gossip_index) { secp256k1_ecdsa_signature *sigs; struct bitcoin_tx *tx = fc->funding_tx; @@ -1580,7 +1591,8 @@ static void opening_got_hsm_funding_sig(struct funding_channel *fc, fc->peer->opening_cmd = NULL; /* Start normal channel daemon. */ - peer_start_channeld(fc->peer, cs, peer_fd, gossip_fd, NULL, false); + peer_start_channeld(fc->peer, cs, gossip_index, + peer_fd, gossip_fd, NULL, false); peer_set_condition(fc->peer, OPENINGD, CHANNELD_AWAITING_LOCKIN); wallet_confirm_utxos(fc->peer->ld->wallet, fc->utxomap); @@ -1829,7 +1841,10 @@ static void peer_received_closing_signature(struct peer *peer, const u8 *msg) static void peer_closing_complete(struct peer *peer, const u8 *msg) { - if (!fromwire_closing_complete(msg, NULL)) { + /* FIXME: We should save this, to return to gossipd */ + u64 gossip_index; + + if (!fromwire_closing_complete(msg, NULL, &gossip_index)) { peer_internal_error(peer, "Bad closing_complete %s", tal_hex(peer, msg)); return; @@ -1867,6 +1882,7 @@ static unsigned closing_msg(struct subd *sd, const u8 *msg, const int *fds) static void peer_start_closingd(struct peer *peer, struct crypto_state *cs, + u64 gossip_index, int peer_fd, int gossip_fd, bool reconnected) { @@ -1942,6 +1958,7 @@ static void peer_start_closingd(struct peer *peer, */ initmsg = towire_closing_init(tmpctx, cs, + gossip_index, peer->seed, peer->funding_txid, peer->funding_outnum, @@ -1970,18 +1987,19 @@ static void peer_start_closingd_after_shutdown(struct peer *peer, const u8 *msg, const int *fds) { struct crypto_state cs; + u64 gossip_index; /* We expect 2 fds. */ assert(tal_count(fds) == 2); - if (!fromwire_channel_shutdown_complete(msg, NULL, &cs)) { + if (!fromwire_channel_shutdown_complete(msg, NULL, &cs, &gossip_index)) { peer_internal_error(peer, "bad shutdown_complete: %s", tal_hex(peer, msg)); return; } /* This sets peer->owner, closes down channeld. */ - peer_start_closingd(peer, &cs, fds[0], fds[1], false); + peer_start_closingd(peer, &cs, gossip_index, fds[0], fds[1], false); peer_set_condition(peer, CHANNELD_SHUTTING_DOWN, CLOSINGD_SIGEXCHANGE); } @@ -2045,6 +2063,7 @@ static unsigned channel_msg(struct subd *sd, const u8 *msg, const int *fds) static bool peer_start_channeld(struct peer *peer, const struct crypto_state *cs, + u64 gossip_index, int peer_fd, int gossip_fd, const u8 *funding_signed, bool reconnected) @@ -2135,7 +2154,7 @@ static bool peer_start_channeld(struct peer *peer, get_feerate(peer->ld->topology, FEERATE_NORMAL), get_feerate(peer->ld->topology, FEERATE_IMMEDIATE) * 5, peer->last_sig, - cs, + cs, gossip_index, &peer->channel_info->remote_fundingkey, &peer->channel_info->theirbase.revocation, &peer->channel_info->theirbase.payment, @@ -2196,6 +2215,7 @@ static void opening_funder_finished(struct subd *opening, const u8 *resp, struct crypto_state cs; secp256k1_ecdsa_signature remote_commit_sig; struct bitcoin_tx *remote_commit; + u64 gossip_index; assert(tal_count(fds) == 2); @@ -2212,6 +2232,7 @@ static void opening_funder_finished(struct subd *opening, const u8 *resp, remote_commit, &remote_commit_sig, &cs, + &gossip_index, &channel_info->theirbase.revocation, &channel_info->theirbase.payment, &channel_info->theirbase.htlc, @@ -2301,7 +2322,7 @@ static void opening_funder_finished(struct subd *opening, const u8 *resp, fatal("Could not write to HSM: %s", strerror(errno)); msg = hsm_sync_read(fc, fc->peer->ld); - opening_got_hsm_funding_sig(fc, fds[0], fds[1], msg, &cs); + opening_got_hsm_funding_sig(fc, fds[0], fds[1], msg, &cs, gossip_index); } static void opening_fundee_finished(struct subd *opening, @@ -2312,6 +2333,7 @@ static void opening_fundee_finished(struct subd *opening, u8 *funding_signed; struct channel_info *channel_info; struct crypto_state cs; + u64 gossip_index; secp256k1_ecdsa_signature remote_commit_sig; struct bitcoin_tx *remote_commit; @@ -2331,6 +2353,7 @@ static void opening_fundee_finished(struct subd *opening, remote_commit, &remote_commit_sig, &cs, + &gossip_index, &channel_info->theirbase.revocation, &channel_info->theirbase.payment, &channel_info->theirbase.htlc, @@ -2376,7 +2399,8 @@ static void opening_fundee_finished(struct subd *opening, peer_set_owner(peer, NULL); /* On to normal operation! */ - peer_start_channeld(peer, &cs, fds[0], fds[1], funding_signed, false); + peer_start_channeld(peer, &cs, gossip_index, + fds[0], fds[1], funding_signed, false); peer_set_condition(peer, OPENINGD, CHANNELD_AWAITING_LOCKIN); } @@ -2386,6 +2410,7 @@ static unsigned int opening_negotiation_failed(struct subd *openingd, const int *fds) { struct crypto_state cs; + u64 gossip_index; struct peer *peer = openingd->peer; u8 *err; const char *why; @@ -2394,14 +2419,16 @@ static unsigned int opening_negotiation_failed(struct subd *openingd, if (tal_count(fds) == 0) return 2; - if (!fromwire_opening_negotiation_failed(msg, msg, NULL, &cs, &err)) { + if (!fromwire_opening_negotiation_failed(msg, msg, NULL, + &cs, &gossip_index, &err)) { peer_internal_error(peer, "bad OPENING_NEGOTIATION_FAILED %s", tal_hex(msg, msg)); return 0; } - msg = towire_gossipctl_hand_back_peer(msg, &peer->id, &cs, NULL); + msg = towire_gossipctl_hand_back_peer(msg, &peer->id, &cs, gossip_index, + NULL); subd_send_msg(openingd->ld->gossip, take(msg)); subd_send_fd(openingd->ld->gossip, fds[0]); subd_send_fd(openingd->ld->gossip, fds[1]); @@ -2419,6 +2446,7 @@ static void peer_accept_channel(struct lightningd *ld, const struct pubkey *peer_id, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd, const u8 *open_msg) @@ -2470,7 +2498,7 @@ static void peer_accept_channel(struct lightningd *ld, &peer->our_config, max_to_self_delay, min_effective_htlc_capacity_msat, - cs, peer->seed); + cs, gossip_index, peer->seed); subd_send_msg(peer->owner, take(msg)); @@ -2493,6 +2521,7 @@ static void peer_offer_channel(struct lightningd *ld, struct funding_channel *fc, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd) { @@ -2550,7 +2579,7 @@ static void peer_offer_channel(struct lightningd *ld, &fc->peer->our_config, max_to_self_delay, min_effective_htlc_capacity_msat, - cs, fc->peer->seed); + cs, gossip_index, fc->peer->seed); subd_send_msg(fc->peer->owner, take(msg)); utxos = from_utxoptr_arr(fc, fc->utxomap); @@ -2579,6 +2608,7 @@ static void gossip_peer_released(struct subd *gossip, { struct lightningd *ld = gossip->ld; struct crypto_state cs; + u64 gossip_index; u8 *gfeatures, *lfeatures; struct wireaddr addr; @@ -2586,6 +2616,7 @@ static void gossip_peer_released(struct subd *gossip, fc->peer = peer_by_id(ld, &fc->peerid); if (!fromwire_gossipctl_release_peer_reply(fc, resp, NULL, &addr, &cs, + &gossip_index, &gfeatures, &lfeatures)) { if (!fromwire_gossipctl_release_peer_replyfail(resp, NULL)) { fatal("Gossip daemon gave invalid reply %s", @@ -2611,7 +2642,8 @@ static void gossip_peer_released(struct subd *gossip, } /* OK, offer peer a channel. */ - peer_offer_channel(ld, fc, &addr, &cs, gfeatures, lfeatures, + peer_offer_channel(ld, fc, &addr, &cs, gossip_index, + gfeatures, lfeatures, fds[0], fds[1]); } diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 20f2e1453c38..aee2378a9d6e 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -176,6 +176,7 @@ void peer_sent_nongossip(struct lightningd *ld, const struct pubkey *id, const struct wireaddr *addr, const struct crypto_state *cs, + u64 gossip_index, const u8 *gfeatures, const u8 *lfeatures, int peer_fd, int gossip_fd, diff --git a/openingd/opening.c b/openingd/opening.c index f10e3dd7d320..3357c3779e37 100644 --- a/openingd/opening.c +++ b/openingd/opening.c @@ -38,6 +38,7 @@ struct state { struct crypto_state cs; + u64 gossip_index; struct pubkey next_per_commit[NUM_SIDES]; /* Initially temporary, then final channel id. */ @@ -89,6 +90,7 @@ static void negotiation_failed(struct state *state, bool send_error, /* Tell master we should return to gossiping. */ msg = towire_opening_negotiation_failed(state, &state->cs, + state->gossip_index, (const u8 *)errmsg); wire_sync_write(REQ_FD, msg); fdpass_send(REQ_FD, PEER_FD); @@ -502,7 +504,7 @@ static u8 *funder_channel(struct state *state, state->remoteconf, tx, &sig, - &state->cs, + &state->cs, state->gossip_index, &theirs.revocation, &theirs.payment, &theirs.htlc, @@ -726,6 +728,7 @@ static u8 *fundee_channel(struct state *state, their_commit, &theirsig, &state->cs, + state->gossip_index, &theirs.revocation, &theirs.payment, &theirs.htlc, @@ -778,6 +781,7 @@ int main(int argc, char *argv[]) &state->max_to_self_delay, &state->min_effective_htlc_capacity_msat, &state->cs, + &state->gossip_index, &seed)) master_badmsg(WIRE_OPENING_INIT, msg); diff --git a/openingd/opening_wire.csv b/openingd/opening_wire.csv index 2f3d71fbd7d4..598336869c5c 100644 --- a/openingd/opening_wire.csv +++ b/openingd/opening_wire.csv @@ -9,6 +9,7 @@ opening_init,,our_config,struct channel_config opening_init,,max_to_self_delay,u32 opening_init,,min_effective_htlc_capacity_msat,u64 opening_init,,crypto_state,struct crypto_state +opening_init,,gossip_index,u64 # Seed to generate all the keys from opening_init,,seed,struct privkey @@ -34,6 +35,7 @@ opening_funder_reply,,their_config,struct channel_config opening_funder_reply,,first_commit,struct bitcoin_tx opening_funder_reply,,first_commit_sig,secp256k1_ecdsa_signature opening_funder_reply,,crypto_state,struct crypto_state +opening_funder_reply,,gossip_index,u64 opening_funder_reply,,revocation_basepoint,struct pubkey opening_funder_reply,,payment_basepoint,struct pubkey opening_funder_reply,,htlc_basepoint,struct pubkey @@ -58,6 +60,7 @@ opening_fundee_reply,,their_config,struct channel_config opening_fundee_reply,,first_commit,struct bitcoin_tx opening_fundee_reply,,first_commit_sig,secp256k1_ecdsa_signature opening_fundee_reply,,crypto_state,struct crypto_state +opening_fundee_reply,,gossip_index,u64 opening_fundee_reply,,revocation_basepoint,struct pubkey opening_fundee_reply,,payment_basepoint,struct pubkey opening_fundee_reply,,htlc_basepoint,struct pubkey @@ -77,6 +80,7 @@ opening_fundee_reply,,funding_signed_msg,msglen*u8 # We disagreed with opening parameters, but peer is ok for gossip (+ peerfd) opening_negotiation_failed,6010 opening_negotiation_failed,,crypto_state,struct crypto_state +opening_negotiation_failed,,gossip_index,u64 opening_negotiation_failed,,len,u16 # FIXME: string support! opening_negotiation_failed,,msg,len*u8 From a22d341aeee401270303e59374f85e2e0d370036 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 12 Dec 2017 22:45:07 +0100 Subject: [PATCH 23/25] pytest: Fix a flaky channel_reenable test It was relying on the message order instead of waiting the desired state. Signed-off-by: Christian Decker --- tests/test_lightningd.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 8073ac2d42ce..c28a5c46028a 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -2322,7 +2322,7 @@ def test_channel_reenable(self): l2.daemon.wait_for_log('Received node_announcement for node {}'.format(l1.info['id'])) # Both directions should be active before the restart - assert [c['active'] for c in l1.rpc.getchannels()['channels']] == [True, True] + wait_for(lambda: [c['active'] for c in l1.rpc.getchannels()['channels']] == [True, True]) # Restart l2, will cause l1 to reconnect l2.stop() @@ -2331,7 +2331,7 @@ def test_channel_reenable(self): # Now they should sync and re-establish again l1.daemon.wait_for_log('Received node_announcement for node {}'.format(l2.info['id'])) l2.daemon.wait_for_log('Received node_announcement for node {}'.format(l1.info['id'])) - assert [c['active'] for c in l1.rpc.getchannels()['channels']] == [True, True] + wait_for(lambda: [c['active'] for c in l1.rpc.getchannels()['channels']] == [True, True]) @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_update_fee(self): From c787e5198b2aa6e9fb38eaa5c5582bfbd950b179 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 12 Dec 2017 23:25:50 +0100 Subject: [PATCH 24/25] channel: Directly send announcements and updates to gossipd Signed-off-by: Christian Decker --- channeld/channel.c | 15 ++++---- channeld/channel_wire.csv | 7 ---- gossipd/gossip.c | 13 ------- gossipd/gossip_wire.csv | 6 --- lightningd/gossip_control.c | 1 - lightningd/peer_control.c | 76 +------------------------------------ 6 files changed, 8 insertions(+), 110 deletions(-) diff --git a/channeld/channel.c b/channeld/channel.c index 33e873099ebe..cf0e8324b746 100644 --- a/channeld/channel.c +++ b/channeld/channel.c @@ -367,16 +367,16 @@ static void handle_peer_funding_locked(struct peer *peer, const u8 *msg) static void announce_channel(struct peer *peer) { + tal_t *tmpctx = tal_tmpctx(peer); u8 *cannounce, *cupdate; - cannounce = create_channel_announcement(peer, peer); - cupdate = create_channel_update(cannounce, peer, false); + cannounce = create_channel_announcement(tmpctx, peer); + cupdate = create_channel_update(tmpctx, peer, false); + + wire_sync_write(GOSSIP_FD, cannounce); + wire_sync_write(GOSSIP_FD, cupdate); - /* Tell the master that we to announce channel (it does node) */ - wire_sync_write(MASTER_FD, take(towire_channel_announce(peer, - cannounce, - cupdate))); - tal_free(cannounce); + tal_free(tmpctx); } static void handle_peer_announcement_signatures(struct peer *peer, const u8 *msg) @@ -2215,7 +2215,6 @@ static void req_in(struct peer *peer, const u8 *msg) case WIRE_CHANNEL_INIT: case WIRE_CHANNEL_OFFER_HTLC_REPLY: case WIRE_CHANNEL_PING_REPLY: - case WIRE_CHANNEL_ANNOUNCE: case WIRE_CHANNEL_SENDING_COMMITSIG: case WIRE_CHANNEL_GOT_COMMITSIG: case WIRE_CHANNEL_GOT_REVOKE: diff --git a/channeld/channel_wire.csv b/channeld/channel_wire.csv index bf49cdf0f618..5aff8d4e0e27 100644 --- a/channeld/channel_wire.csv +++ b/channeld/channel_wire.csv @@ -109,13 +109,6 @@ channel_ping,,len,u16 channel_ping_reply,1111 channel_ping_reply,,totlen,u16 -# Channeld tells the master to announce the channel (with first update) -channel_announce,1012 -channel_announce,,announce_len,u16 -channel_announce,,announce,announce_len*u8 -channel_announce,,update_len,u16 -channel_announce,,update,update_len*u8 - # When we receive funding_locked. channel_got_funding_locked,1019 channel_got_funding_locked,,next_per_commit_point,struct pubkey diff --git a/gossipd/gossip.c b/gossipd/gossip.c index 709105b2be2e..172a0f9cc11c 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossip.c @@ -1284,15 +1284,6 @@ static struct io_plan *resolve_channel_req(struct io_conn *conn, return daemon_conn_read_next(conn, &daemon->master); } -static void handle_forwarded_msg(struct io_conn *conn, struct daemon *daemon, const u8 *msg) -{ - u8 *payload; - if (!fromwire_gossip_forwarded_msg(msg, msg, NULL, &payload)) - master_badmsg(WIRE_GOSSIP_FORWARDED_MSG, msg); - - handle_gossip_msg(daemon, payload); -} - static struct io_plan *handshake_out_success(struct io_conn *conn, const struct pubkey *id, const struct wireaddr *addr, @@ -1536,10 +1527,6 @@ static struct io_plan *recv_req(struct io_conn *conn, struct daemon_conn *master case WIRE_GOSSIP_RESOLVE_CHANNEL_REQUEST: return resolve_channel_req(conn, daemon, daemon->master.msg_in); - case WIRE_GOSSIP_FORWARDED_MSG: - handle_forwarded_msg(conn, daemon, daemon->master.msg_in); - return daemon_conn_read_next(conn, &daemon->master); - case WIRE_GOSSIPCTL_HAND_BACK_PEER: return hand_back_peer(conn, daemon, master->msg_in); diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index 2f4651663d9a..e5149503bcef 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -121,12 +121,6 @@ gossip_resolve_channel_reply,3109 gossip_resolve_channel_reply,,num_keys,u16 gossip_resolve_channel_reply,,keys,num_keys*struct pubkey -# The main daemon forward some gossip message to gossipd, allows injecting -# arbitrary gossip messages. -gossip_forwarded_msg,3010 -gossip_forwarded_msg,,msglen,u16 -gossip_forwarded_msg,,msg,msglen*u8 - # The main daemon asks for peers gossip_getpeers_request,3011 diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 2e9a66d62538..9b4eeee03da5 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -66,7 +66,6 @@ static unsigned gossip_msg(struct subd *gossip, const u8 *msg, const int *fds) case WIRE_GOSSIP_GETPEERS_REQUEST: case WIRE_GOSSIP_PING: case WIRE_GOSSIP_RESOLVE_CHANNEL_REQUEST: - case WIRE_GOSSIP_FORWARDED_MSG: case WIRE_GOSSIPCTL_REACH_PEER: case WIRE_GOSSIPCTL_HAND_BACK_PEER: case WIRE_GOSSIPCTL_RELEASE_PEER: diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index c32dcd634c9e..992c5aa91e46 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1599,81 +1599,10 @@ static void opening_got_hsm_funding_sig(struct funding_channel *fc, tal_free(fc); } -/* Create a node_announcement with the given signature. It may be NULL - * in the case we need to create a provisional announcement for the - * HSM to sign. This is typically called twice: once with the dummy - * signature to get it signed and a second time to build the full - * packet with the signature. The timestamp is handed in since that is - * the only thing that may change between the dummy creation and the - * call with a signature.*/ -static u8 *create_node_announcement(const tal_t *ctx, struct lightningd *ld, - secp256k1_ecdsa_signature *sig, - u32 timestamp) -{ - u8 *features = NULL; - u8 *addresses = tal_arr(ctx, u8, 0); - u8 *announcement; - size_t i; - if (!sig) { - sig = tal(ctx, secp256k1_ecdsa_signature); - memset(sig, 0, sizeof(*sig)); - } - for (i = 0; i < tal_count(ld->wireaddrs); i++) - towire_wireaddr(&addresses, ld->wireaddrs+i); - - announcement = - towire_node_announcement(ctx, sig, features, timestamp, - &ld->id, ld->rgb, (u8 *)ld->alias, - addresses); - return announcement; -} - /* We were informed by channeld that it announced the channel and sent * an update, so we can now start sending a node_announcement. The * first step is to build the provisional announcement and ask the HSM * to sign it. */ -static void peer_channel_announce(struct peer *peer, const u8 *msg) -{ - struct lightningd *ld = peer->ld; - tal_t *tmpctx = tal_tmpctx(peer); - secp256k1_ecdsa_signature sig; - u8 *cannounce, *cupdate; - u8 *announcement, *wrappedmsg; - u32 timestamp = time_now().ts.tv_sec; - - if (!fromwire_channel_announce(msg, msg, NULL, &cannounce, &cupdate)) { - peer_internal_error(peer, "bad fromwire_channel_announced %s", - tal_hex(peer, msg)); - return; - } - - msg = towire_hsm_node_announcement_sig_req( - tmpctx, create_node_announcement(tmpctx, ld, NULL, timestamp)); - - if (!wire_sync_write(ld->hsm_fd, take(msg))) - fatal("Could not write to HSM: %s", strerror(errno)); - - msg = hsm_sync_read(tmpctx, ld); - if (!fromwire_hsm_node_announcement_sig_reply(msg, NULL, &sig)) - fatal("HSM returned an invalid node_announcement sig"); - - /* We got the signature for out provisional node_announcement back - * from the HSM, create the real announcement and forward it to - * gossipd so it can take care of forwarding it. */ - announcement = create_node_announcement(tmpctx, ld, &sig, timestamp); - - /* We have to send channel_announce before channel_update and - * node_announcement */ - wrappedmsg = towire_gossip_forwarded_msg(tmpctx, cannounce); - subd_send_msg(ld->gossip, take(wrappedmsg)); - - wrappedmsg = towire_gossip_forwarded_msg(tmpctx, cupdate); - subd_send_msg(ld->gossip, take(wrappedmsg)); - - wrappedmsg = towire_gossip_forwarded_msg(tmpctx, announcement); - subd_send_msg(ld->gossip, take(wrappedmsg)); - tal_free(tmpctx); -} static void peer_got_funding_locked(struct peer *peer, const u8 *msg) { @@ -2021,9 +1950,6 @@ static unsigned channel_msg(struct subd *sd, const u8 *msg, const int *fds) case WIRE_CHANNEL_GOT_REVOKE: peer_got_revoke(sd->peer, msg); break; - case WIRE_CHANNEL_ANNOUNCE: - peer_channel_announce(sd->peer, msg); - break; case WIRE_CHANNEL_GOT_FUNDING_LOCKED: peer_got_funding_locked(sd->peer, msg); break; @@ -2418,7 +2344,7 @@ static unsigned int opening_negotiation_failed(struct subd *openingd, /* We need the peer fd and gossip fd. */ if (tal_count(fds) == 0) return 2; - + if (!fromwire_opening_negotiation_failed(msg, msg, NULL, &cs, &gossip_index, &err)) { peer_internal_error(peer, From 234b41235a9a850c419c86b2fb42a87fca3cc566 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 13 Dec 2017 13:44:09 +0100 Subject: [PATCH 25/25] pytest: Minor cleanup Now using assertRaisesRegex instead of try-except and added restart to nodes. Signed-off-by: Christian Decker --- tests/test_lightningd.py | 44 +++++++++++++--------------------------- tests/utils.py | 18 ++++++++++++++-- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index c28a5c46028a..21e06785f543 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -2124,52 +2124,36 @@ def test_funding_fail(self): l2 = self.node_factory.get_node(options=['--locktime-blocks={}'.format(max_locktime + 1)]) l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + funds = 1000000 + addr = l1.rpc.newaddr()['address'] - txid = l1.bitcoin.rpc.sendtoaddress(addr, 0.01) + txid = l1.bitcoin.rpc.sendtoaddress(addr, funds / 10**8) bitcoind.generate_block(1) # Wait for it to arrive. wait_for(lambda: len(l1.rpc.listfunds()['outputs']) > 0) # Fail because l1 dislikes l2's huge locktime. - try: - l1.rpc.fundchannel(l2.info['id'], 100000) - except ValueError as verr: - str(verr).index('to_self_delay {} larger than {}' - .format(max_locktime+1, max_locktime)) - except Exception as err: - self.fail("Unexpected exception {}".format(err)) - else: - self.fail("huge locktime ignored?") - - # We don't have enough left to cover fees if we try to spend it all. - try: - l1.rpc.fundchannel(l2.info['id'], 1000000) - except ValueError as verr: - str(verr).index('Cannot afford funding transaction') - except Exception as err: - self.fail("Unexpected exception {}".format(err)) - else: - self.fail("We somehow covered fees?") - - # Should still be connected. + self.assertRaisesRegex(ValueError, r'to_self_delay \d+ larger than \d+', + l1.rpc.fundchannel, l2.info['id'], int(funds/10)) assert l1.rpc.getpeers()['peers'][0]['connected'] assert l2.rpc.getpeers()['peers'][0]['connected'] # Restart l2 without ridiculous locktime. - l2.daemon.proc.terminate() - l2.daemon.cmd_line.remove('--locktime-blocks={}'.format(max_locktime + 1)) + l2.restart() + l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) - # Wait for l1 to notice - wait_for(lambda: len(l1.rpc.getpeers()['peers']) == 0) + # We don't have enough left to cover fees if we try to spend it all. + self.assertRaisesRegex(ValueError, r'Cannot afford funding transaction', + l1.rpc.fundchannel, l2.info['id'], funds) - # Now restart l2, reconnect. - l2.daemon.start() - l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + # Should still be connected. + assert l1.rpc.getpeers()['peers'][0]['connected'] + assert l2.rpc.getpeers()['peers'][0]['connected'] # This works. - l1.rpc.fundchannel(l2.info['id'], int(0.01 * 10**8 / 2)) + l1.rpc.fundchannel(l2.info['id'], int(funds/10)) def test_addfunds_from_block(self): """Send funds to the daemon without telling it explicitly diff --git a/tests/utils.py b/tests/utils.py index b15191455f33..fda6a78916bf 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -79,8 +79,8 @@ def stop(self, timeout=10): self.proc.wait() self.thread.join() - if failed: - raise(ValueError("Process '{}' did not cleanly shutdown".format(self.proc.pid))) + if self.proc.returncode: + raise ValueError("Process '{}' did not cleanly shutdown: return code {}".format(self.proc.pid, rc)) return self.proc.returncode @@ -364,3 +364,17 @@ def stop(self, timeout=10): raise ValueError("Node did not exit cleanly, rc={}".format(rc)) else: return rc + + def restart(self, timeout=10, clean=True): + """Stop and restart the lightning node. + + Keyword arguments: + timeout: number of seconds to wait for a shutdown + clean: whether to issue a `stop` RPC command before killing + """ + if clean: + self.stop(timeout) + else: + self.daemon.stop() + + self.daemon.start()