From a21f025a13646d3dbd78732472ddec52e6850887 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 Aug 2018 13:34:23 +0930 Subject: [PATCH 01/39] TAGS: add python files. This is part of our new "Python programmers are people too" policy. Signed-off-by: Rusty Russell --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2cb42046558a..69546abb94a4 100644 --- a/Makefile +++ b/Makefile @@ -307,7 +307,7 @@ ncc: external/libwally-core/src/libwallycore.la # Ignore test/ directories. TAGS: FORCE - $(RM) TAGS; find * -name test -type d -prune -o -name '*.[ch]' -print | xargs etags --append + $(RM) TAGS; find * -name test -type d -prune -o -name '*.[ch]' -print -o -name '*.py' -print | xargs etags --append FORCE:: ccan/ccan/cdump/tools/cdump-enumstr: ccan/ccan/cdump/tools/cdump-enumstr.o $(CDUMP_OBJS) $(CCAN_OBJS) From 6091e5158ba2a588c5cc295a00983ec2c03ee89d Mon Sep 17 00:00:00 2001 From: Stephanie Stroka Date: Tue, 28 Aug 2018 10:42:55 +0200 Subject: [PATCH 02/39] cleanup: derive_basepoints is no longer needed in channel.c --- channeld/channel.c | 1 - 1 file changed, 1 deletion(-) diff --git a/channeld/channel.c b/channeld/channel.c index d3101d9197a3..7441c06ccccd 100644 --- a/channeld/channel.c +++ b/channeld/channel.c @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include From 317a830e9405eb6d53cea336fc7626efe21beb20 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 30 Aug 2018 09:34:28 +0930 Subject: [PATCH 03/39] devtools: dump-gossipstore. Not very useful by itself, but when combined with decodemsg it can tell us quite a bit. Signed-off-by: Rusty Russell --- devtools/.gitignore | 1 + devtools/Makefile | 9 ++-- devtools/dump-gossipstore.c | 82 +++++++++++++++++++++++++++++++++++++ gossipd/gossip_store.c | 11 +++-- gossipd/gossip_store.h | 2 + 5 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 devtools/dump-gossipstore.c diff --git a/devtools/.gitignore b/devtools/.gitignore index f37d42aab9a1..eb53afe43646 100644 --- a/devtools/.gitignore +++ b/devtools/.gitignore @@ -1,3 +1,4 @@ bolt11-cli decodemsg onion +dump-gossipstore diff --git a/devtools/Makefile b/devtools/Makefile index 1b8625fcea48..c2af6a2a7a38 100644 --- a/devtools/Makefile +++ b/devtools/Makefile @@ -1,6 +1,6 @@ DEVTOOLS_SRC := devtools/gen_print_wire.c devtools/gen_print_onion_wire.c devtools/print_wire.c DEVTOOLS_OBJS := $(DEVTOOLS_SRC:.c=.o) -DEVTOOLS_TOOL_SRC := devtools/bolt11-cli.c devtools/decodemsg.c devtools/onion.c +DEVTOOLS_TOOL_SRC := devtools/bolt11-cli.c devtools/decodemsg.c devtools/onion.c devtools/dump-gossipstore.c DEVTOOLS_TOOL_OBJS := $(DEVTOOLS_TOOL_SRC:.c=.o) DEVTOOLS_COMMON_OBJS := \ @@ -15,7 +15,7 @@ DEVTOOLS_COMMON_OBJS := \ common/version.o \ common/wireaddr.o -devtools-all: devtools/bolt11-cli devtools/decodemsg devtools/onion +devtools-all: devtools/bolt11-cli devtools/decodemsg devtools/onion devtools/dump-gossipstore devtools/gen_print_wire.h: $(WIRE_GEN) wire/gen_peer_wire_csv $(WIRE_GEN) --bolt --printwire --header $@ wire_type < wire/gen_peer_wire_csv > $@ @@ -33,6 +33,9 @@ devtools/bolt11-cli: $(DEVTOOLS_OBJS) $(DEVTOOLS_COMMON_OBJS) $(JSMN_OBJS) $(CCA devtools/decodemsg: $(DEVTOOLS_OBJS) $(DEVTOOLS_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS) $(BITCOIN_OBJS) wire/fromwire.o wire/towire.o devtools/decodemsg.o +devtools/dump-gossipstore: $(DEVTOOLS_OBJS) $(DEVTOOLS_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS) $(BITCOIN_OBJS) wire/fromwire.o wire/towire.o devtools/dump-gossipstore.o gossipd/gen_gossip_store.o + +devtools/dump-gossipstore.o: gossipd/gen_gossip_store.h devtools/onion.c: ccan/config.h devtools/onion: $(DEVTOOLS_OBJS) $(DEVTOOLS_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS) $(BITCOIN_OBJS) wire/fromwire.o wire/towire.o devtools/onion.o common/sphinx.o @@ -42,7 +45,7 @@ devtools/gen_print_wire.o: devtools/gen_print_wire.h wire/gen_peer_wire.h devtoo devtools/gen_print_onion_wire.o: devtools/gen_print_onion_wire.h devtools/print_wire.h # Make sure these depend on everything. -ALL_PROGRAMS += devtools/bolt11-cli devtools/decodemsg devtools/onion +ALL_PROGRAMS += devtools/bolt11-cli devtools/decodemsg devtools/onion devtools/dump-gossipstore ALL_OBJS += $(DEVTOOLS_OBJS) $(DEVTOOLS_TOOL_OBJS) check-source: $(DEVTOOLS_SRC:%=check-src-include-order/%) $(DEVTOOLS_TOOLS_SRC:%=check-src-include-order/%) diff --git a/devtools/dump-gossipstore.c b/devtools/dump-gossipstore.c new file mode 100644 index 000000000000..7604ce17fce3 --- /dev/null +++ b/devtools/dump-gossipstore.c @@ -0,0 +1,82 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +int main(int argc, char *argv[]) +{ + int fd; + u8 version; + beint32_t belen, becsum; + + setup_locale(); + + if (argc > 2) + errx(1, "Need the filename of a gossip store, or stdin"); + + if (argc == 2) { + fd = open(argv[1], O_RDONLY); + if (fd < 0) + err(1, "Opening %s", argv[1]); + } else + fd = STDIN_FILENO; + + if (read(fd, &version, sizeof(version)) != sizeof(version)) + errx(1, "Empty file"); + + if (version != GOSSIP_STORE_VERSION) + warnx("UNSUPPORTED GOSSIP VERSION %u (expected %u)", + version, GOSSIP_STORE_VERSION); + + printf("GOSSIP VERSION %u\n", version); + + while (read(fd, &belen, sizeof(belen)) == sizeof(belen) && + read(fd, &becsum, sizeof(becsum)) == sizeof(becsum)) { + u64 satoshis; + struct short_channel_id scid; + u8 *gossip_msg; + u32 msglen = be32_to_cpu(belen); + u8 *msg = tal_arr(NULL, u8, msglen); + + if (read(fd, msg, msglen) != msglen) + errx(1, "Truncated file?"); + + if (be32_to_cpu(becsum) != crc32c(0, msg, msglen)) + warnx("Checksum verification failed"); + + if (fromwire_gossip_store_channel_announcement(msg, msg, + &gossip_msg, + &satoshis)) { + printf("channel_announce for %"PRIu64" satoshis: %s\n", + satoshis, tal_hex(msg, gossip_msg)); + } else if (fromwire_gossip_store_channel_update(msg, msg, + &gossip_msg)) { + printf("channel_update: %s\n", + tal_hex(msg, gossip_msg)); + } else if (fromwire_gossip_store_node_announcement(msg, msg, + &gossip_msg)) { + printf("node_announcement: %s\n", + tal_hex(msg, gossip_msg)); + } else if (fromwire_gossip_store_channel_delete(msg, &scid)) { + printf("channel_delete: %s\n", + type_to_string(msg, struct short_channel_id, + &scid)); + } else if (fromwire_gossip_store_local_add_channel( + msg, msg, &gossip_msg)) { + printf("local_add_channel: %s\n", + tal_hex(msg, gossip_msg)); + } else { + warnx("Unknown message %u", fromwire_peektype(msg)); + } + tal_free(msg); + } + return 0; +} diff --git a/gossipd/gossip_store.c b/gossipd/gossip_store.c index 8060b7d772cf..2e070e69ffba 100644 --- a/gossipd/gossip_store.c +++ b/gossipd/gossip_store.c @@ -16,7 +16,6 @@ #define GOSSIP_STORE_FILENAME "gossip_store" #define GOSSIP_STORE_TEMP_FILENAME "gossip_store.tmp" -static u8 gossip_store_version = 0x02; struct gossip_store { int fd; @@ -62,17 +61,17 @@ struct gossip_store *gossip_store_new(const tal_t *ctx, if (read(gs->fd, &gs->version, sizeof(gs->version)) == sizeof(gs->version)) { /* Version match? All good */ - if (gs->version == gossip_store_version) + if (gs->version == GOSSIP_STORE_VERSION) return gs; status_unusual("Gossip store version %u not %u: removing", - gs->version, gossip_store_version); + gs->version, GOSSIP_STORE_VERSION); if (ftruncate(gs->fd, 0) != 0) status_failed(STATUS_FAIL_INTERNAL_ERROR, "Truncating store: %s", strerror(errno)); } /* Empty file, write version byte */ - gs->version = gossip_store_version; + gs->version = GOSSIP_STORE_VERSION; if (write(gs->fd, &gs->version, sizeof(gs->version)) != sizeof(gs->version)) status_failed(STATUS_FAIL_INTERNAL_ERROR, @@ -177,8 +176,8 @@ static void gossip_store_compact(struct gossip_store *gs) goto disable; } - if (write(fd, &gossip_store_version, sizeof(gossip_store_version)) - != sizeof(gossip_store_version)) { + if (write(fd, &gs->version, sizeof(gs->version)) + != sizeof(gs->version)) { status_broken("Writing version to store: %s", strerror(errno)); goto unlink_disable; } diff --git a/gossipd/gossip_store.h b/gossipd/gossip_store.h index e4f7863524bb..6a78d2ca9d7c 100644 --- a/gossipd/gossip_store.h +++ b/gossipd/gossip_store.h @@ -11,6 +11,8 @@ /** * gossip_store -- On-disk storage related information */ +#define GOSSIP_STORE_VERSION 2 + struct gossip_store; struct routing_state; From 9c28f997d3d652d32c34e8cba45e7e6748e65fe2 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Tue, 21 Aug 2018 08:58:55 -0500 Subject: [PATCH 04/39] param: json.c style improvements Suggested-by: @rustyrussell Signed-off-by: Mark Beckwith --- lightningd/json.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lightningd/json.c b/lightningd/json.c index c01cccda816e..921eee08cd51 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -1,6 +1,7 @@ #include "json.h" #include #include +#include #include #include #include @@ -98,12 +99,12 @@ bool json_tok_bool(struct command *cmd, const char *name, *b = tal(cmd, bool); if (tok->type == JSMN_PRIMITIVE) { if (tok->end - tok->start == strlen("true") - && !memcmp(buffer + tok->start, "true", strlen("true"))) { + && memeqstr(buffer + tok->start, strlen("true"), "true")) { **b = true; return true; } if (tok->end - tok->start == strlen("false") - && !memcmp(buffer + tok->start, "false", strlen("false"))) { + && memeqstr(buffer + tok->start, strlen("false"), "false")) { **b = false; return true; } @@ -213,8 +214,7 @@ bool json_tok_short_channel_id(struct command *cmd, const char *name, struct short_channel_id **scid) { *scid = tal(cmd, struct short_channel_id); - if (short_channel_id_from_str(buffer + tok->start, - tok->end - tok->start, *scid)) + if (json_to_short_channel_id(buffer, tok, *scid)) return true; command_fail(cmd, JSONRPC2_INVALID_PARAMS, From a79e64c0a07e24bf5b8fbb3f962329b9a5ccccd1 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Tue, 21 Aug 2018 09:08:35 -0500 Subject: [PATCH 05/39] param: consistent callback format The `json_tok_X` functions now consistently check the success case first and call `command_fail` at the end. Signed-off-by: Mark Beckwith --- lightningd/json.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/lightningd/json.c b/lightningd/json.c index 921eee08cd51..40f398a03263 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -120,13 +120,13 @@ bool json_tok_double(struct command *cmd, const char *name, double **num) { *num = tal(cmd, double); - if (!json_to_double(buffer, tok, *num)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a double, not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; - } - return true; + if (json_to_double(buffer, tok, *num)) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a double, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; } bool json_tok_number(struct command *cmd, const char *name, @@ -134,13 +134,13 @@ bool json_tok_number(struct command *cmd, const char *name, unsigned int **num) { *num = tal(cmd, unsigned int); - if (!json_to_number(buffer, tok, *num)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be an integer, not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; - } - return true; + if (json_to_number(buffer, tok, *num)) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be an integer, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; } bool json_tok_sha256(struct command *cmd, const char *name, @@ -164,13 +164,13 @@ bool json_tok_u64(struct command *cmd, const char *name, uint64_t **num) { *num = tal(cmd, uint64_t); - if (!json_to_u64(buffer, tok, *num)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be an unsigned 64 bit integer, not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; - } - return true; + if (json_to_u64(buffer, tok, *num)) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be an unsigned 64 bit integer, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; } bool json_to_pubkey(const char *buffer, const jsmntok_t *tok, From eb1a5b16c7e1b50a5dc120d3c2d9c4fb2c991a7e Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Tue, 21 Aug 2018 09:12:50 -0500 Subject: [PATCH 06/39] param: return type consistency We were returning a pointer but expecting a bool. Signed-off-by: Mark Beckwith --- lightningd/param.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lightningd/param.c b/lightningd/param.c index bb11317e98bc..a20c00938a7a 100644 --- a/lightningd/param.c +++ b/lightningd/param.c @@ -45,7 +45,7 @@ static bool make_callback(struct command *cmd, return def->cbx(cmd, def->name, buffer, tok, def->arg); } -static struct param *post_check(struct command *cmd, struct param *params) +static bool post_check(struct command *cmd, struct param *params) { struct param *first = params; struct param *last = first + tal_count(params); @@ -56,11 +56,11 @@ static struct param *post_check(struct command *cmd, struct param *params) command_fail(cmd, JSONRPC2_INVALID_PARAMS, "missing required parameter: '%s'", first->name); - return NULL; + return false; } first++; } - return params; + return true; } static bool parse_by_position(struct command *cmd, From 4cef0d062c62d87b168340762ffb9844cd5d4875 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Thu, 23 Aug 2018 11:08:32 -0500 Subject: [PATCH 07/39] param: use param for json_pay routes Note: Unlike before, this will now accept positional parameters. Note: In case of error we no longer report the hop number. Is this acceptable? We still report the name of the bad param and its value. One option is to log the hop number if param() returns false. This would require a change to command_fail so it doesn't delete the cmd, so we can still access cmd->ld->log. Signed-off-by: Mark Beckwith --- lightningd/pay.c | 59 ++++++++++++------------------------------------ 1 file changed, 15 insertions(+), 44 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index b1fe2f5fdfd4..304b24e3aa72 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -975,54 +975,25 @@ static void json_sendpay(struct command *cmd, route = tal_arr(cmd, struct route_hop, n_hops); for (t = routetok + 1; t < end; t = json_next(t)) { - /* FIXME: Use param() to handle parsing each route? -- @wythe */ - const jsmntok_t *amttok, *idtok, *delaytok, *chantok; - - if (t->type != JSMN_OBJECT) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu '%.*s' is not an object", - n_hops, - t->end - t->start, - buffer + t->start); - return; - } - amttok = json_get_member(buffer, t, "msatoshi"); - idtok = json_get_member(buffer, t, "id"); - delaytok = json_get_member(buffer, t, "delay"); - chantok = json_get_member(buffer, t, "channel"); - if (!amttok || !idtok || !delaytok || !chantok) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu needs msatoshi/id/channel/delay", - n_hops); + u64 *amount; + struct pubkey *id; + struct short_channel_id *channel; + unsigned *delay; + + if (!param(cmd, buffer, t, + p_req("msatoshi", json_tok_u64, &amount), + p_req("id", json_tok_pubkey, &id), + p_req("delay", json_tok_number, &delay), + p_req("channel", json_tok_short_channel_id, &channel), + NULL)) return; - } tal_resize(&route, n_hops + 1); - /* What that hop will forward */ - if (!json_to_u64(buffer, amttok, &route[n_hops].amount)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu invalid msatoshi", - n_hops); - return; - } - - if (!json_to_short_channel_id(buffer, chantok, - &route[n_hops].channel_id)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu invalid channel_id", n_hops); - return; - } - if (!json_to_pubkey(buffer, idtok, &route[n_hops].nodeid)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu invalid id", n_hops); - return; - } - if (!json_to_number(buffer, delaytok, &route[n_hops].delay)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Route %zu invalid delay", n_hops); - return; - } + route[n_hops].amount = *amount; + route[n_hops].nodeid = *id; + route[n_hops].delay = *delay; + route[n_hops].channel_id = *channel; n_hops++; } From c32f7910cc552f374f2a81169be1159af9cf52f7 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Thu, 23 Aug 2018 16:04:38 -0500 Subject: [PATCH 08/39] param: upgraded json_tok_label Added utility function json_tok_is_num so I would avoid using goto. Signed-off-by: Mark Beckwith --- common/json.c | 11 ++++++ common/json.h | 3 ++ lightningd/invoice.c | 87 +++++++++++--------------------------------- 3 files changed, 35 insertions(+), 66 deletions(-) diff --git a/common/json.c b/common/json.c index 23aab5b8b017..d1abe52f022e 100644 --- a/common/json.c +++ b/common/json.c @@ -119,6 +119,17 @@ bool json_tok_bitcoin_amount(const char *buffer, const jsmntok_t *tok, return true; } +bool json_tok_is_num(const char *buffer, const jsmntok_t *tok) +{ + if (tok->type != JSMN_PRIMITIVE) + return false; + + for (int i = tok->start; i < tok->end; i++) + if (!cisdigit(buffer[i])) + return false; + return true; +} + bool json_tok_is_null(const char *buffer, const jsmntok_t *tok) { if (tok->type != JSMN_PRIMITIVE) diff --git a/common/json.h b/common/json.h index 02ef93058e47..c5e5b048e554 100644 --- a/common/json.h +++ b/common/json.h @@ -39,6 +39,9 @@ bool json_to_double(const char *buffer, const jsmntok_t *tok, double *num); bool json_tok_bitcoin_amount(const char *buffer, const jsmntok_t *tok, uint64_t *satoshi); +/* Is this a number? [0..9]+ */ +bool json_tok_is_num(const char *buffer, const jsmntok_t *tok); + /* Is this the null primitive? */ bool json_tok_is_null(const char *buffer, const jsmntok_t *tok); diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 88994474c022..c8ffcb78a38b 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -102,27 +102,24 @@ static bool hsm_sign_b11(const u5 *u5bytes, return true; } -/* We allow a string, or a literal number, for labels */ -static struct json_escaped *json_tok_label(const tal_t *ctx, - const char *buffer, - const jsmntok_t *tok) +static bool json_tok_label(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + struct json_escaped **label) { - struct json_escaped *label; - label = json_tok_escaped_string(ctx, buffer, tok); - if (label) - return label; + if ((*label = json_tok_escaped_string(cmd, buffer, tok))) + return true; /* Allow literal numbers */ - if (tok->type != JSMN_PRIMITIVE) - return NULL; - - for (int i = tok->start; i < tok->end; i++) - if (!cisdigit(buffer[i])) - return NULL; - - return json_escaped_string_(ctx, buffer + tok->start, - tok->end - tok->start); + if (json_tok_is_num(buffer, tok) && + ((*label = json_escaped_string_(cmd, buffer + tok->start, + tok->end - tok->start)))) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a string or number, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; } static bool parse_fallback(struct command *cmd, @@ -154,10 +151,10 @@ static void json_invoice(struct command *cmd, { struct invoice invoice; const struct invoice_details *details; - const jsmntok_t *msatoshi, *label, *desctok, *fallbacks; + const jsmntok_t *msatoshi, *desctok, *fallbacks; const jsmntok_t *preimagetok; u64 *msatoshi_val; - const struct json_escaped *label_val, *desc; + struct json_escaped *label_val, *desc; const char *desc_val; struct json_result *response = new_json_result(cmd); struct wallet *wallet = cmd->ld->wallet; @@ -169,7 +166,7 @@ static void json_invoice(struct command *cmd, if (!param(cmd, buffer, params, p_req("msatoshi", json_tok_tok, &msatoshi), - p_req("label", json_tok_tok, &label), + p_req("label", json_tok_label, &label_val), p_req("description", json_tok_tok, &desctok), p_opt_def("expiry", json_tok_u64, &expiry, 3600), p_opt("fallbacks", json_tok_tok, &fallbacks), @@ -192,14 +189,6 @@ static void json_invoice(struct command *cmd, return; } } - /* label */ - label_val = json_tok_label(cmd, buffer, label); - if (!label_val) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "label '%.*s' not a string or number", - label->end - label->start, buffer + label->start); - return; - } if (wallet_invoice_find_by_label(wallet, &invoice, label_val)) { command_fail(cmd, INVOICE_LABEL_ALREADY_EXISTS, "Duplicate label '%s'", label_val->s); @@ -366,28 +355,13 @@ static void json_add_invoices(struct json_result *response, static void json_listinvoices(struct command *cmd, const char *buffer, const jsmntok_t *params) { - const jsmntok_t *labeltok; struct json_escaped *label; struct json_result *response = new_json_result(cmd); struct wallet *wallet = cmd->ld->wallet; - if (!param(cmd, buffer, params, - p_opt("label", json_tok_tok, &labeltok), + p_opt("label", json_tok_label, &label), NULL)) return; - - if (labeltok) { - label = json_tok_label(cmd, buffer, labeltok); - if (!label) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "label '%.*s' is not a string or number", - labeltok->end - labeltok->start, - buffer + labeltok->start); - return; - } - } else - label = NULL; - json_object_start(response, NULL); json_array_start(response, "invoices"); json_add_invoices(response, wallet, label); @@ -408,26 +382,18 @@ static void json_delinvoice(struct command *cmd, { struct invoice i; const struct invoice_details *details; - const jsmntok_t *labeltok, *statustok; + const jsmntok_t *statustok; struct json_result *response = new_json_result(cmd); const char *status, *actual_status; struct json_escaped *label; struct wallet *wallet = cmd->ld->wallet; if (!param(cmd, buffer, params, - p_req("label", json_tok_tok, &labeltok), + p_req("label", json_tok_label, &label), p_req("status", json_tok_tok, &statustok), NULL)) return; - label = json_tok_label(cmd, buffer, labeltok); - if (!label) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "label '%.*s' is not a string or number", - labeltok->end - labeltok->start, - buffer + labeltok->start); - return; - } if (!wallet_invoice_find_by_label(wallet, &i, label)) { command_fail(cmd, LIGHTNINGD, "Unknown invoice"); return; @@ -564,24 +530,13 @@ static void json_waitinvoice(struct command *cmd, struct invoice i; const struct invoice_details *details; struct wallet *wallet = cmd->ld->wallet; - const jsmntok_t *labeltok; struct json_escaped *label; if (!param(cmd, buffer, params, - p_req("label", json_tok_tok, &labeltok), + p_req("label", json_tok_label, &label), NULL)) return; - /* Search for invoice */ - label = json_tok_label(cmd, buffer, labeltok); - if (!label) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "label '%.*s' is not a string or number", - labeltok->end - labeltok->start, - buffer + labeltok->start); - return; - } - if (!wallet_invoice_find_by_label(wallet, &i, label)) { command_fail(cmd, LIGHTNINGD, "Label not found"); return; From c553bba7a8a517f484ba4ecca301bd1f849c8c83 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Mon, 27 Aug 2018 09:19:09 -0500 Subject: [PATCH 09/39] param: getroute fuzz now uses json_tok_percent Signed-off-by: Mark Beckwith --- lightningd/gossip_control.c | 8 +------- lightningd/json.c | 15 +++++++++++++++ lightningd/json.h | 5 +++++ lightningd/payalgo.c | 16 ---------------- 4 files changed, 21 insertions(+), 23 deletions(-) diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 2b9495f0b3ec..559b6c534cba 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -293,16 +293,10 @@ static void json_getroute(struct command *cmd, const char *buffer, const jsmntok p_opt_def("cltv", json_tok_number, &cltv, 9), p_opt_def("fromid", json_tok_pubkey, &source, ld->id), p_opt("seed", json_tok_tok, &seedtok), - p_opt_def("fuzzpercent", json_tok_double, &fuzz, 75.0), + p_opt_def("fuzzpercent", json_tok_percent, &fuzz, 75.0), NULL)) return; - if (!(0.0 <= *fuzz && *fuzz <= 100.0)) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "fuzz must be in range 0.0 <= %f <= 100.0", - *fuzz); - return; - } /* Convert from percentage */ *fuzz = *fuzz / 100.0; diff --git a/lightningd/json.c b/lightningd/json.c index 40f398a03263..70ca770bba77 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -159,6 +159,21 @@ bool json_tok_sha256(struct command *cmd, const char *name, return false; } +bool json_tok_percent(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t *tok, + double **num) +{ + *num = tal(cmd, double); + if (json_to_double(buffer, tok, *num)) + if (**num >= 0.0 && **num >= 100.0) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a double in range [0.0, 100.0], not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; +} + bool json_tok_u64(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, uint64_t **num) diff --git a/lightningd/json.h b/lightningd/json.h index 069042ddf9cb..25f85a21e0af 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -63,6 +63,11 @@ bool json_tok_sha256(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, struct sha256 **hash); +/* Extract double in range [0.0, 100.0] */ +bool json_tok_percent(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t *tok, + double **num); + /* Extract a pubkey from this */ bool json_to_pubkey(const char *buffer, const jsmntok_t *tok, struct pubkey *pubkey); diff --git a/lightningd/payalgo.c b/lightningd/payalgo.c index 04f698262ab6..21eecad73c34 100644 --- a/lightningd/payalgo.c +++ b/lightningd/payalgo.c @@ -596,22 +596,6 @@ static void json_pay_stop_retrying(struct pay *pay) json_pay_failure(pay, sr); } -/* Extract double in range [0.0, 100.0] */ -static bool json_tok_percent(struct command *cmd, const char *name, - const char *buffer, const jsmntok_t *tok, - double **num) -{ - *num = tal(cmd, double); - if (json_to_double(buffer, tok, *num)) - if (**num >= 0.0 && **num >= 100.0) - return true; - - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a double in range [0.0, 100.0], not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; -} - static void json_pay(struct command *cmd, const char *buffer, const jsmntok_t *params) { From 4f81cd38520c612f435f0dcfdb4994d851dda1c8 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Mon, 27 Aug 2018 15:58:50 -0500 Subject: [PATCH 10/39] param: added json_tok_msat This could have been a local static but its used by the run-param test, so putting it in json.c made things easier. Signed-off-by: Mark Beckwith --- lightningd/invoice.c | 19 ++----------------- lightningd/json.c | 21 +++++++++++++++++++++ lightningd/json.h | 5 +++++ lightningd/test/run-param.c | 23 ----------------------- 4 files changed, 28 insertions(+), 40 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index c8ffcb78a38b..0f10421f1fc2 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -151,7 +151,7 @@ static void json_invoice(struct command *cmd, { struct invoice invoice; const struct invoice_details *details; - const jsmntok_t *msatoshi, *desctok, *fallbacks; + const jsmntok_t *desctok, *fallbacks; const jsmntok_t *preimagetok; u64 *msatoshi_val; struct json_escaped *label_val, *desc; @@ -165,7 +165,7 @@ static void json_invoice(struct command *cmd, bool result; if (!param(cmd, buffer, params, - p_req("msatoshi", json_tok_tok, &msatoshi), + p_req("msatoshi", json_tok_msat, &msatoshi_val), p_req("label", json_tok_label, &label_val), p_req("description", json_tok_tok, &desctok), p_opt_def("expiry", json_tok_u64, &expiry, 3600), @@ -174,21 +174,6 @@ static void json_invoice(struct command *cmd, NULL)) return; - /* Get arguments. */ - /* msatoshi */ - if (json_tok_streq(buffer, msatoshi, "any")) - msatoshi_val = NULL; - else { - msatoshi_val = tal(cmd, u64); - if (!json_to_u64(buffer, msatoshi, msatoshi_val) - || *msatoshi_val == 0) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%.*s' is not a valid positive number", - msatoshi->end - msatoshi->start, - buffer + msatoshi->start); - return; - } - } if (wallet_invoice_find_by_label(wallet, &invoice, label_val)) { command_fail(cmd, INVOICE_LABEL_ALREADY_EXISTS, "Duplicate label '%s'", label_val->s); diff --git a/lightningd/json.c b/lightningd/json.c index 70ca770bba77..2717f6355f21 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -159,6 +159,27 @@ bool json_tok_sha256(struct command *cmd, const char *name, return false; } +bool json_tok_msat(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t * tok, + u64 **msatoshi_val) +{ + if (json_tok_streq(buffer, tok, "any")) { + *msatoshi_val = NULL; + return true; + } + *msatoshi_val = tal(cmd, u64); + + if (json_to_u64(buffer, tok, *msatoshi_val) && *msatoshi_val != 0) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a positive number or 'any', not '%.*s'", + name, + tok->end - tok->start, + buffer + tok->start); + return false; +} + bool json_tok_percent(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, double **num) diff --git a/lightningd/json.h b/lightningd/json.h index 25f85a21e0af..9358110eb279 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -63,6 +63,11 @@ bool json_tok_sha256(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, struct sha256 **hash); +/* Extract positive integer, or NULL if tok is 'any'. */ +bool json_tok_msat(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t * tok, + u64 **msatoshi_val); + /* Extract double in range [0.0, 100.0] */ bool json_tok_percent(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, diff --git a/lightningd/test/run-param.c b/lightningd/test/run-param.c index 79419eb3c48a..66f489c035a4 100644 --- a/lightningd/test/run-param.c +++ b/lightningd/test/run-param.c @@ -413,29 +413,6 @@ static void sendpay_nulltok(void) assert(msatoshi == NULL); } -static bool json_tok_msat(struct command *cmd, - const char *name, - const char *buffer, - const jsmntok_t * tok, - u64 **msatoshi_val) -{ - if (json_tok_streq(buffer, tok, "any")) { - *msatoshi_val = NULL; - return true; - } - *msatoshi_val = tal(cmd, u64); - - if (json_to_u64(buffer, tok, *msatoshi_val) && *msatoshi_val != 0) - return true; - - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a positive number or 'any', not '%.*s'", - name, - tok->end - tok->start, - buffer + tok->start); - return false; -} - /* * New version of json_tok_label conforming to advanced style. This can eventually * replace the existing json_tok_label. From 8590dbedfb8c5769e2957a3c47f99f2d3399189f Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Wed, 29 Aug 2018 13:43:28 -0500 Subject: [PATCH 11/39] param: make json_tok_label non-static Needed to do this so I could remove the implementation in the run-param test. Signed-off-by: Mark Beckwith --- lightningd/invoice.c | 20 ------------------- lightningd/json.c | 19 +++++++++++++++++++ lightningd/json.h | 6 ++++++ lightningd/test/run-param.c | 38 +++---------------------------------- 4 files changed, 28 insertions(+), 55 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 0f10421f1fc2..57c62b9b127a 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -102,26 +102,6 @@ static bool hsm_sign_b11(const u5 *u5bytes, return true; } -static bool json_tok_label(struct command *cmd, const char *name, - const char * buffer, const jsmntok_t *tok, - struct json_escaped **label) -{ - - if ((*label = json_tok_escaped_string(cmd, buffer, tok))) - return true; - - /* Allow literal numbers */ - if (json_tok_is_num(buffer, tok) && - ((*label = json_escaped_string_(cmd, buffer + tok->start, - tok->end - tok->start)))) - return true; - - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a string or number, not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; -} - static bool parse_fallback(struct command *cmd, const char *buffer, const jsmntok_t *fallback, const u8 **fallback_script) diff --git a/lightningd/json.c b/lightningd/json.c index 2717f6355f21..f621440f2bad 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -129,6 +129,25 @@ bool json_tok_double(struct command *cmd, const char *name, return false; } +bool json_tok_label(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + struct json_escaped **label) +{ + if ((*label = json_tok_escaped_string(cmd, buffer, tok))) + return true; + + /* Allow literal numbers */ + if (json_tok_is_num(buffer, tok) && + ((*label = json_escaped_string_(cmd, buffer + tok->start, + tok->end - tok->start)))) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a string or number, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; +} + bool json_tok_number(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, unsigned int **num) diff --git a/lightningd/json.h b/lightningd/json.h index 9358110eb279..f90da4213162 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -16,6 +16,7 @@ struct bitcoin_txid; struct channel_id; struct command; +struct json_escaped; struct json_result; struct pubkey; struct route_hop; @@ -53,6 +54,11 @@ bool json_tok_double(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, double **num); +/* Extract a label. It is either an escaped string or a number. */ +bool json_tok_label(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + struct json_escaped **label); + /* Extract number from this (may be a string, or a number literal) */ bool json_tok_number(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, diff --git a/lightningd/test/run-param.c b/lightningd/test/run-param.c index 66f489c035a4..64fb4b94335b 100644 --- a/lightningd/test/run-param.c +++ b/lightningd/test/run-param.c @@ -413,38 +413,6 @@ static void sendpay_nulltok(void) assert(msatoshi == NULL); } -/* - * New version of json_tok_label conforming to advanced style. This can eventually - * replace the existing json_tok_label. - */ -static bool json_tok_label_x(struct command *cmd, - const char *name, - const char *buffer, - const jsmntok_t *tok, - struct json_escaped **label) -{ - if ((*label = json_tok_escaped_string(cmd, buffer, tok))) - return true; - - /* Allow literal numbers */ - if (tok->type != JSMN_PRIMITIVE) - goto fail; - - for (int i = tok->start; i < tok->end; i++) - if (!cisdigit(buffer[i])) - goto fail; - - if ((*label = json_escaped_string_(cmd, buffer + tok->start, - tok->end - tok->start))) - return true; - -fail: - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%s' should be a string or number, not '%.*s'", - name, tok->end - tok->start, buffer + tok->start); - return false; -} - static void advanced(void) { { @@ -456,7 +424,7 @@ static void advanced(void) const jsmntok_t *tok; assert(param(cmd, j->buffer, j->toks, - p_req("description", json_tok_label_x, &label), + p_req("description", json_tok_label, &label), p_req("msat", json_tok_msat, &msat), p_req("tok", json_tok_tok, &tok), p_opt("msat_opt1", json_tok_msat, &msat_opt1), @@ -474,8 +442,8 @@ static void advanced(void) struct json *j = json_parse(cmd, "[ 3 'foo' ]"); struct json_escaped *label, *foo; assert(param(cmd, j->buffer, j->toks, - p_req("label", json_tok_label_x, &label), - p_opt("foo", json_tok_label_x, &foo), + p_req("label", json_tok_label, &label), + p_opt("foo", json_tok_label, &foo), NULL)); assert(!strcmp(label->s, "3")); assert(!strcmp(foo->s, "foo")); From aa6005713488cbb2a8ecea76f2bcb35333e24c97 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Wed, 29 Aug 2018 15:44:04 -0500 Subject: [PATCH 12/39] param: upgraded json_tok_escaped_string Signed-off-by: Mark Beckwith --- common/json_escaped.c | 6 +++--- common/json_escaped.h | 6 +++--- lightningd/invoice.c | 23 +++-------------------- lightningd/json.c | 19 ++++++++++++++++++- lightningd/json.h | 5 +++++ lightningd/pay.c | 27 ++------------------------- 6 files changed, 34 insertions(+), 52 deletions(-) diff --git a/common/json_escaped.c b/common/json_escaped.c index 0780bccc7ba1..d60654b9277d 100644 --- a/common/json_escaped.c +++ b/common/json_escaped.c @@ -13,9 +13,9 @@ struct json_escaped *json_escaped_string_(const tal_t *ctx, return esc; } -struct json_escaped *json_tok_escaped_string(const tal_t *ctx, - const char *buffer, - const jsmntok_t *tok) +struct json_escaped *json_to_escaped_string(const tal_t *ctx, + const char *buffer, + const jsmntok_t *tok) { if (tok->type != JSMN_STRING) return NULL; diff --git a/common/json_escaped.h b/common/json_escaped.h index 75eecdb18407..0f742a86f39f 100644 --- a/common/json_escaped.h +++ b/common/json_escaped.h @@ -17,9 +17,9 @@ struct json_escaped *json_partial_escape(const tal_t *ctx, const char *str TAKES); /* Extract a JSON-escaped string. */ -struct json_escaped *json_tok_escaped_string(const tal_t *ctx, - const char *buffer, - const jsmntok_t *tok); +struct json_escaped *json_to_escaped_string(const tal_t *ctx, + const char *buffer, + const jsmntok_t *tok); /* Are two escaped json strings identical? */ bool json_escaped_eq(const struct json_escaped *a, diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 57c62b9b127a..9163f860dd01 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -131,10 +131,10 @@ static void json_invoice(struct command *cmd, { struct invoice invoice; const struct invoice_details *details; - const jsmntok_t *desctok, *fallbacks; + const jsmntok_t *fallbacks; const jsmntok_t *preimagetok; u64 *msatoshi_val; - struct json_escaped *label_val, *desc; + struct json_escaped *label_val; const char *desc_val; struct json_result *response = new_json_result(cmd); struct wallet *wallet = cmd->ld->wallet; @@ -147,7 +147,7 @@ static void json_invoice(struct command *cmd, if (!param(cmd, buffer, params, p_req("msatoshi", json_tok_msat, &msatoshi_val), p_req("label", json_tok_label, &label_val), - p_req("description", json_tok_tok, &desctok), + p_req("description", json_tok_escaped_string, &desc_val), p_opt_def("expiry", json_tok_u64, &expiry, 3600), p_opt("fallbacks", json_tok_tok, &fallbacks), p_opt("preimage", json_tok_tok, &preimagetok), @@ -166,23 +166,6 @@ static void json_invoice(struct command *cmd, return; } - desc = json_tok_escaped_string(cmd, buffer, desctok); - if (!desc) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "description '%.*s' not a string", - desctok->end - desctok->start, - buffer + desctok->start); - return; - } - desc_val = json_escaped_unescape(cmd, desc); - if (!desc_val) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "description '%s' is invalid" - " (note: we don't allow \\u)", - desc->s); - return; - } - /* description */ if (strlen(desc_val) >= BOLT11_FIELD_BYTE_LIMIT) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Descriptions greater than %d bytes " diff --git a/lightningd/json.c b/lightningd/json.c index f621440f2bad..75f49dcdf0b0 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -129,11 +129,28 @@ bool json_tok_double(struct command *cmd, const char *name, return false; } +bool json_tok_escaped_string(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str) +{ + struct json_escaped *esc = json_to_escaped_string(cmd, buffer, tok); + if (esc) + if ((*str = json_escaped_unescape(cmd, esc))) + return true; + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be a string, not '%.*s'" + " (note, we don't allow \\u)", + name, + tok->end - tok->start, buffer + tok->start); + return false; +} + bool json_tok_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, struct json_escaped **label) { - if ((*label = json_tok_escaped_string(cmd, buffer, tok))) + if ((*label = json_to_escaped_string(cmd, buffer, tok))) return true; /* Allow literal numbers */ diff --git a/lightningd/json.h b/lightningd/json.h index f90da4213162..531eef896c6f 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -54,6 +54,11 @@ bool json_tok_double(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, double **num); +/* Extract an escaped string (and unescape it) */ +bool json_tok_escaped_string(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str); + /* Extract a label. It is either an escaped string or a number. */ bool json_tok_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, diff --git a/lightningd/pay.c b/lightningd/pay.c index 304b24e3aa72..64ac0cc6346a 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -945,19 +945,18 @@ static void json_sendpay_on_resolve(const struct sendpay_result* r, static void json_sendpay(struct command *cmd, const char *buffer, const jsmntok_t *params) { - const jsmntok_t *routetok, *desctok; + const jsmntok_t *routetok; const jsmntok_t *t, *end; size_t n_hops; struct sha256 *rhash; struct route_hop *route; u64 *msatoshi; - const struct json_escaped *desc; const char *description; if (!param(cmd, buffer, params, p_req("route", json_tok_tok, &routetok), p_req("payment_hash", json_tok_sha256, &rhash), - p_opt("description", json_tok_tok, &desctok), + p_opt("description", json_tok_escaped_string, &description), p_opt("msatoshi", json_tok_u64, &msatoshi), NULL)) return; @@ -1017,28 +1016,6 @@ static void json_sendpay(struct command *cmd, } } - if (desctok) { - desc = json_tok_escaped_string(cmd, buffer, desctok); - if (!desc) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "description '%.*s' not a string", - desctok->end - desctok->start, - buffer + desctok->start); - return; - } - description = json_escaped_unescape(cmd, desc); - if (description == NULL) { - command_fail( - cmd, JSONRPC2_INVALID_PARAMS, - "description '%.*s' not a valid escaped string", - desctok->end - desctok->start, - buffer + desctok->start); - return; - } - } else { - description = NULL; - } - if (send_payment(cmd, cmd->ld, rhash, route, msatoshi ? *msatoshi : route[n_hops-1].amount, description, From 0b26a17a0f7420e2867537d56776ca9b008131df Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Wed, 29 Aug 2018 16:58:07 -0500 Subject: [PATCH 13/39] param: added json_tok_array Signed-off-by: Mark Beckwith --- lightningd/gossip_control.c | 10 +--------- lightningd/invoice.c | 7 +------ lightningd/json.c | 13 +++++++++++++ lightningd/json.h | 5 +++++ lightningd/pay.c | 10 +--------- 5 files changed, 21 insertions(+), 24 deletions(-) diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 559b6c534cba..1a9a01eccc8e 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -431,18 +431,10 @@ static void json_dev_query_scids(struct command *cmd, if (!param(cmd, buffer, params, p_req("id", json_tok_pubkey, &id), - p_req("scids", json_tok_tok, &scidstok), + p_req("scids", json_tok_array, &scidstok), NULL)) return; - if (scidstok->type != JSMN_ARRAY) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%.*s' is not an array", - scidstok->end - scidstok->start, - buffer + scidstok->start); - return; - } - scids = tal_arr(cmd, struct short_channel_id, scidstok->size); end = json_next(scidstok); for (i = 0, t = scidstok + 1; t < end; t = json_next(t), i++) { diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 9163f860dd01..375169aaa773 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -149,7 +149,7 @@ static void json_invoice(struct command *cmd, p_req("label", json_tok_label, &label_val), p_req("description", json_tok_escaped_string, &desc_val), p_opt_def("expiry", json_tok_u64, &expiry, 3600), - p_opt("fallbacks", json_tok_tok, &fallbacks), + p_opt("fallbacks", json_tok_array, &fallbacks), p_opt("preimage", json_tok_tok, &preimagetok), NULL)) return; @@ -180,11 +180,6 @@ static void json_invoice(struct command *cmd, const jsmntok_t *i, *end = json_next(fallbacks); size_t n = 0; - if (fallbacks->type != JSMN_ARRAY) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "fallback must be an array"); - return; - } fallback_scripts = tal_arr(cmd, const u8 *, n); for (i = fallbacks + 1; i < end; i = json_next(i)) { tal_resize(&fallback_scripts, n+1); diff --git a/lightningd/json.c b/lightningd/json.c index 75f49dcdf0b0..e821d34e5473 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -92,6 +92,19 @@ void json_add_txid(struct json_result *result, const char *fieldname, json_add_string(result, fieldname, hex); } +bool json_tok_array(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t *tok, + const jsmntok_t **arr) +{ + if (tok->type == JSMN_ARRAY) + return (*arr = tok); + + command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "'%s' should be an array, not '%.*s'", + name, tok->end - tok->start, buffer + tok->start); + return false; +} + bool json_tok_bool(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, bool **b) diff --git a/lightningd/json.h b/lightningd/json.h index 531eef896c6f..232271243961 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -44,6 +44,11 @@ void json_add_pubkey(struct json_result *response, void json_add_txid(struct json_result *result, const char *fieldname, const struct bitcoin_txid *txid); +/* Extract json array token */ +bool json_tok_array(struct command *cmd, const char *name, + const char *buffer, const jsmntok_t *tok, + const jsmntok_t **arr); + /* Extract boolean this (must be a true or false) */ bool json_tok_bool(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, diff --git a/lightningd/pay.c b/lightningd/pay.c index 64ac0cc6346a..040e5a68bf46 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -954,21 +954,13 @@ static void json_sendpay(struct command *cmd, const char *description; if (!param(cmd, buffer, params, - p_req("route", json_tok_tok, &routetok), + p_req("route", json_tok_array, &routetok), p_req("payment_hash", json_tok_sha256, &rhash), p_opt("description", json_tok_escaped_string, &description), p_opt("msatoshi", json_tok_u64, &msatoshi), NULL)) return; - if (routetok->type != JSMN_ARRAY) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%.*s' is not an array", - routetok->end - routetok->start, - buffer + routetok->start); - return; - } - end = json_next(routetok); n_hops = 0; route = tal_arr(cmd, struct route_hop, n_hops); From 47510a8e7478f8ccee4c7fd02fe57a9c00cc64fd Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Wed, 29 Aug 2018 17:53:00 -0500 Subject: [PATCH 14/39] param: added json_tok_string Signed-off-by: Mark Beckwith --- lightningd/connect_control.c | 12 +++--------- lightningd/invoice.c | 22 +++++----------------- lightningd/json.c | 9 +++++++++ lightningd/json.h | 5 +++++ lightningd/pay.c | 13 ++++++------- lightningd/payalgo.c | 16 ++++------------ 6 files changed, 32 insertions(+), 45 deletions(-) diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index 2beb875ea216..31b5a05a8200 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -74,7 +74,6 @@ static void connect_cmd_succeed(struct command *cmd, const struct pubkey *id) static void json_connect(struct command *cmd, const char *buffer, const jsmntok_t *params) { - const jsmntok_t *hosttok; u32 *port; jsmntok_t *idtok; struct pubkey id; @@ -89,7 +88,7 @@ static void json_connect(struct command *cmd, if (!param(cmd, buffer, params, p_req("id", json_tok_tok, (const jsmntok_t **) &idtok), - p_opt("host", json_tok_tok, &hosttok), + p_opt("host", json_tok_string, &name), p_opt("port", json_tok_number, &port), NULL)) return; @@ -113,7 +112,7 @@ static void json_connect(struct command *cmd, return; } - if (hosttok && ataddr) { + if (name && ataddr) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Can't specify host as both xxx@yyy " "and separate argument"); @@ -121,13 +120,8 @@ static void json_connect(struct command *cmd, } /* Get parseable host if provided somehow */ - if (hosttok) - name = tal_strndup(cmd, buffer + hosttok->start, - hosttok->end - hosttok->start); - else if (ataddr) + if (!name && ataddr) name = ataddr; - else - name = NULL; /* Port without host name? */ if (port && !name) { diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 375169aaa773..c5e229347d6a 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -325,7 +325,6 @@ static void json_delinvoice(struct command *cmd, { struct invoice i; const struct invoice_details *details; - const jsmntok_t *statustok; struct json_result *response = new_json_result(cmd); const char *status, *actual_status; struct json_escaped *label; @@ -333,7 +332,7 @@ static void json_delinvoice(struct command *cmd, if (!param(cmd, buffer, params, p_req("label", json_tok_label, &label), - p_req("status", json_tok_tok, &statustok), + p_req("status", json_tok_string, &status), NULL)) return; @@ -344,8 +343,6 @@ static void json_delinvoice(struct command *cmd, details = wallet_invoice_details(cmd, cmd->ld->wallet, i); - status = tal_strndup(cmd, buffer + statustok->start, - statustok->end - statustok->start); /* This is time-sensitive, so only call once; otherwise error msg * might not make sense if it changed! */ actual_status = invoice_status_str(details); @@ -543,26 +540,17 @@ static void json_add_fallback(struct json_result *response, static void json_decodepay(struct command *cmd, const char *buffer, const jsmntok_t *params) { - const jsmntok_t *bolt11tok, *desctok; struct bolt11 *b11; struct json_result *response; - char *str, *desc, *fail; + const char *str, *desc; + char *fail; if (!param(cmd, buffer, params, - p_req("bolt11", json_tok_tok, &bolt11tok), - p_opt("description", json_tok_tok, &desctok), + p_req("bolt11", json_tok_string, &str), + p_opt("description", json_tok_string, &desc), NULL)) return; - str = tal_strndup(cmd, buffer + bolt11tok->start, - bolt11tok->end - bolt11tok->start); - - if (desctok) - desc = tal_strndup(cmd, buffer + desctok->start, - desctok->end - desctok->start); - else - desc = NULL; - b11 = bolt11_decode(cmd, str, desc, &fail); if (!b11) { diff --git a/lightningd/json.c b/lightningd/json.c index e821d34e5473..b05ead48c453 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -159,6 +159,15 @@ bool json_tok_escaped_string(struct command *cmd, const char *name, return false; } +bool json_tok_string(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str) +{ + *str = tal_strndup(cmd, buffer + tok->start, + tok->end - tok->start); + return true; +} + bool json_tok_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, struct json_escaped **label) diff --git a/lightningd/json.h b/lightningd/json.h index 232271243961..02902f7989a9 100644 --- a/lightningd/json.h +++ b/lightningd/json.h @@ -64,6 +64,11 @@ bool json_tok_escaped_string(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, const char **str); +/* Extract a string */ +bool json_tok_string(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str); + /* Extract a label. It is either an escaped string or a number. */ bool json_tok_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, diff --git a/lightningd/pay.c b/lightningd/pay.c index 040e5a68bf46..5c1cf0d60ff0 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1061,28 +1061,27 @@ static void json_listpayments(struct command *cmd, const char *buffer, { const struct wallet_payment **payments; struct json_result *response = new_json_result(cmd); - const jsmntok_t *bolt11tok, *rhashtok; + const jsmntok_t *rhashtok; struct sha256 *rhash = NULL; + const char *b11str; if (!param(cmd, buffer, params, - p_opt("bolt11", json_tok_tok, &bolt11tok), + p_opt("bolt11", json_tok_string, &b11str), p_opt("payment_hash", json_tok_tok, &rhashtok), NULL)) return; - if (rhashtok && bolt11tok) { + if (rhashtok && b11str) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Can only specify one of" " {bolt11} or {payment_hash}"); return; } - if (bolt11tok) { + if (b11str) { struct bolt11 *b11; - char *b11str, *fail; + char *fail; - b11str = tal_strndup(cmd, buffer + bolt11tok->start, - bolt11tok->end - bolt11tok->start); b11 = bolt11_decode(cmd, b11str, NULL, &fail); if (!b11) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, diff --git a/lightningd/payalgo.c b/lightningd/payalgo.c index 21eecad73c34..f5d412e7a60f 100644 --- a/lightningd/payalgo.c +++ b/lightningd/payalgo.c @@ -599,21 +599,21 @@ static void json_pay_stop_retrying(struct pay *pay) static void json_pay(struct command *cmd, const char *buffer, const jsmntok_t *params) { - const jsmntok_t *bolt11tok, *desctok; double *riskfactor; double *maxfeepercent; u64 *msatoshi; struct pay *pay = tal(cmd, struct pay); struct bolt11 *b11; - char *fail, *b11str, *desc; + const char *b11str, *desc; + char *fail; unsigned int *retryfor; unsigned int *maxdelay; unsigned int *exemptfee; if (!param(cmd, buffer, params, - p_req("bolt11", json_tok_tok, &bolt11tok), + p_req("bolt11", json_tok_string, &b11str), p_opt("msatoshi", json_tok_u64, &msatoshi), - p_opt("description", json_tok_tok, &desctok), + p_opt("description", json_tok_string, &desc), p_opt_def("riskfactor", json_tok_double, &riskfactor, 1.0), p_opt_def("maxfeepercent", json_tok_percent, &maxfeepercent, 0.5), p_opt_def("retry_for", json_tok_number, &retryfor, 60), @@ -623,14 +623,6 @@ static void json_pay(struct command *cmd, NULL)) return; - b11str = tal_strndup(cmd, buffer + bolt11tok->start, - bolt11tok->end - bolt11tok->start); - if (desctok) - desc = tal_strndup(cmd, buffer + desctok->start, - desctok->end - desctok->start); - else - desc = NULL; - b11 = bolt11_decode(pay, b11str, desc, &fail); if (!b11) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, From 30e6471fc1d733c789a5445c16e7a429dbdcba67 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Thu, 30 Aug 2018 08:48:36 -0500 Subject: [PATCH 15/39] param: listpayments now uses json_tok_sha256 Signed-off-by: Mark Beckwith --- lightningd/json.c | 2 +- lightningd/pay.c | 18 +++--------------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/lightningd/json.c b/lightningd/json.c index b05ead48c453..f042eb4662de 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -1,7 +1,7 @@ #include "json.h" #include -#include #include +#include #include #include #include diff --git a/lightningd/pay.c b/lightningd/pay.c index 5c1cf0d60ff0..84d052ad7824 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1061,17 +1061,16 @@ static void json_listpayments(struct command *cmd, const char *buffer, { const struct wallet_payment **payments; struct json_result *response = new_json_result(cmd); - const jsmntok_t *rhashtok; - struct sha256 *rhash = NULL; + struct sha256 *rhash; const char *b11str; if (!param(cmd, buffer, params, p_opt("bolt11", json_tok_string, &b11str), - p_opt("payment_hash", json_tok_tok, &rhashtok), + p_opt("payment_hash", json_tok_sha256, &rhash), NULL)) return; - if (rhashtok && b11str) { + if (rhash && b11str) { command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Can only specify one of" " {bolt11} or {payment_hash}"); @@ -1089,17 +1088,6 @@ static void json_listpayments(struct command *cmd, const char *buffer, return; } rhash = &b11->payment_hash; - } else if (rhashtok) { - rhash = tal(cmd, struct sha256); - if (!hex_decode(buffer + rhashtok->start, - rhashtok->end - rhashtok->start, - rhash, sizeof(*rhash))) { - command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "'%.*s' is not a valid sha256 hash", - rhashtok->end - rhashtok->start, - buffer + rhashtok->start); - return; - } } payments = wallet_payment_list(cmd, cmd->ld->wallet, rhash); From 4ad16b67f15642dca293dd0e544f50e6a76d0747 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Thu, 30 Aug 2018 14:11:27 -0500 Subject: [PATCH 16/39] param: updated comments in the spirit of #1899 Signed-off-by: Mark Beckwith --- lightningd/param.h | 47 ++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/lightningd/param.h b/lightningd/param.h index 1aac100ffb43..039dde46cf24 100644 --- a/lightningd/param.h +++ b/lightningd/param.h @@ -2,26 +2,33 @@ #define LIGHTNING_LIGHTNINGD_PARAM_H #include "config.h" -/* - Typesafe callback system for unmarshalling and validating json parameters. - - Typical usage: - unsigned *cltv; - u64 *msatoshi; - const jsmntok_t *note; - u64 *expiry; - - if (!param(cmd, buffer, params, - p_req("cltv", json_tok_number, &cltv), - p_opt("msatoshi", json_tok_u64, &msatoshi), - p_opt_tok("note", ¬e), - p_opt_def("expiry", json_tok_u64, &expiry, 3600), - NULL)) - return; - - See json_invoice() for a good example. The common callbacks can be found in - lightningd/json.c. Use them as an example for writing your own custom - callbacks. +/*~ Greetings adventurer! + * + * Do you want to automatically validate json input and unmarshall it into + * local variables, all using typesafe callbacks? And on error, + * call command_fail with a proper error message? Then you've come to the + * right place! + * + * Here is a simple example of using the system: + * + * unsigned *cltv; + * u64 *msatoshi; + * const jsmntok_t *note; + * u64 *expiry; + * + * if (!param(cmd, buffer, params, + * p_req("cltv", json_tok_number, &cltv), + * p_opt("msatoshi", json_tok_u64, &msatoshi), + * p_opt("note", json_tok_tok, ¬e), + * p_opt_def("expiry", json_tok_u64, &expiry, 3600), + * NULL)) + * return; + * + * If param() returns true then you're good to go. + * + * All the command handlers throughout the code use this system. + * json_invoice() is a great example. The common callbacks can be found in + * lightningd/json.c. Use them directly or feel free to write your own. */ /* From 721f77f5283297437a752cc212b699ffb1fe1f4c Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Fri, 31 Aug 2018 10:22:31 -0500 Subject: [PATCH 17/39] param: feedback fixes Signed-off-by: Mark Beckwith --- lightningd/json.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/lightningd/json.c b/lightningd/json.c index f042eb4662de..d99ab9ec10a6 100644 --- a/lightningd/json.c +++ b/lightningd/json.c @@ -111,13 +111,11 @@ bool json_tok_bool(struct command *cmd, const char *name, { *b = tal(cmd, bool); if (tok->type == JSMN_PRIMITIVE) { - if (tok->end - tok->start == strlen("true") - && memeqstr(buffer + tok->start, strlen("true"), "true")) { + if (memeqstr(buffer + tok->start, tok->end - tok->start, "true")) { **b = true; return true; } - if (tok->end - tok->start == strlen("false") - && memeqstr(buffer + tok->start, strlen("false"), "false")) { + if (memeqstr(buffer + tok->start, tok->end - tok->start, "false")) { **b = false; return true; } @@ -147,10 +145,11 @@ bool json_tok_escaped_string(struct command *cmd, const char *name, const char **str) { struct json_escaped *esc = json_to_escaped_string(cmd, buffer, tok); - if (esc) - if ((*str = json_escaped_unescape(cmd, esc))) + if (esc) { + *str = json_escaped_unescape(cmd, esc); + if (*str) return true; - + } command_fail(cmd, JSONRPC2_INVALID_PARAMS, "'%s' should be a string, not '%.*s'" " (note, we don't allow \\u)", @@ -172,14 +171,10 @@ bool json_tok_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, struct json_escaped **label) { - if ((*label = json_to_escaped_string(cmd, buffer, tok))) - return true; - - /* Allow literal numbers */ - if (json_tok_is_num(buffer, tok) && - ((*label = json_escaped_string_(cmd, buffer + tok->start, - tok->end - tok->start)))) - return true; + /* We accept both strings and number literals here. */ + *label = json_escaped_string_(cmd, buffer + tok->start, tok->end - tok->start); + if (*label && (tok->type == JSMN_STRING || json_tok_is_num(buffer, tok))) + return true; command_fail(cmd, JSONRPC2_INVALID_PARAMS, "'%s' should be a string or number, not '%.*s'", @@ -244,7 +239,7 @@ bool json_tok_percent(struct command *cmd, const char *name, { *num = tal(cmd, double); if (json_to_double(buffer, tok, *num)) - if (**num >= 0.0 && **num >= 100.0) + if (**num >= 0.0 && **num <= 100.0) return true; command_fail(cmd, JSONRPC2_INVALID_PARAMS, From ca40cfa0cebd3a6d27eebfdc457eee1fee037f16 Mon Sep 17 00:00:00 2001 From: Mark Beckwith Date: Fri, 31 Aug 2018 18:01:29 -0500 Subject: [PATCH 18/39] param: started adding callback unit tests Well its about time. Signed-off-by: Mark Beckwith --- lightningd/test/run-param.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lightningd/test/run-param.c b/lightningd/test/run-param.c index 64fb4b94335b..6b6576ec52f0 100644 --- a/lightningd/test/run-param.c +++ b/lightningd/test/run-param.c @@ -476,6 +476,36 @@ static void advanced_fail(void) } } +#define test_cb(cb, T, json_, value, pass) \ +{ \ + struct json *j = json_parse(cmd, json_); \ + T *v; \ + bool ret = cb(cmd, "name", j->buffer, j->toks + 1, &v); \ + assert(ret == pass); \ + if (ret) { \ + assert(v); \ + assert(*v == value); \ + } \ +} + +static void json_tok_tests(void) +{ + test_cb(json_tok_bool, bool, "[ true ]", true, true); + test_cb(json_tok_bool, bool, "[ false ]", false, true); + test_cb(json_tok_bool, bool, "[ tru ]", false, false); + test_cb(json_tok_bool, bool, "[ 1 ]", false, false); + + test_cb(json_tok_percent, double, "[ -0.01 ]", 0, false); + test_cb(json_tok_percent, double, "[ 0.00 ]", 0, true); + test_cb(json_tok_percent, double, "[ 1 ]", 1, true); + test_cb(json_tok_percent, double, "[ 1.1 ]", 1.1, true); + test_cb(json_tok_percent, double, "[ 1.01 ]", 1.01, true); + test_cb(json_tok_percent, double, "[ 99.99 ]", 99.99, true); + test_cb(json_tok_percent, double, "[ 100.0 ]", 100, true); + test_cb(json_tok_percent, double, "[ 100.001 ]", 0, false); + test_cb(json_tok_percent, double, "[ 1000 ]", 0, false); + test_cb(json_tok_percent, double, "[ 'wow' ]", 0, false); +} int main(void) { @@ -497,6 +527,7 @@ int main(void) sendpay_nulltok(); advanced(); advanced_fail(); + json_tok_tests(); tal_free(tmpctx); printf("run-params ok\n"); } From 1e9152366371809ebacb6027e8913faa951d77a4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:11:27 +0930 Subject: [PATCH 19/39] lightningd: remove unnecessary globals. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 8 +++----- lightningd/lightningd.h | 2 ++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index abac82e0a825..9a182c30de1d 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -39,10 +39,6 @@ #include #include -char *bitcoin_datadir; - -int pid_fd; - static struct lightningd *new_lightningd(const tal_t *ctx) { struct lightningd *ld = tal(ctx, struct lightningd); @@ -277,6 +273,7 @@ static void daemonize_but_keep_dir(struct lightningd *ld) static void pidfile_create(const struct lightningd *ld) { char *pid; + int pid_fd; /* Create PID file */ pid_fd = open(ld->pidfile, O_WRONLY|O_CREAT, 0640); @@ -291,6 +288,8 @@ static void pidfile_create(const struct lightningd *ld) /* Get current PID and write to PID fie */ pid = tal_fmt(tmpctx, "%d\n", getpid()); write_all(pid_fd, pid, strlen(pid)); + + /* Leave file open: we close it implicitly when we exit */ } /* Yuck, we need globals here. */ @@ -472,7 +471,6 @@ int main(int argc, char *argv[]) tal_free(ld->rpc_listener); db_commit_transaction(ld->wallet->db); - close(pid_fd); remove(ld->pidfile); /* FIXME: pay can have children off tmpctx which unlink from diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 9b57c035e2af..4ae2d5883bc3 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -76,6 +76,8 @@ struct lightningd { /* Are we told to run in the background. */ bool daemon; + int pid_fd; + /* Our config dir, and rpc file */ char *config_dir; From c33c971478841fb3c6c93a6247be18443f57dc14 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:12:27 +0930 Subject: [PATCH 20/39] lightningd: rename 'daemons' to 'subdaemons'. We're a daemon. They're subdaemons. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 23 ++++++++++++----------- lightningd/lightningd.h | 2 +- lightningd/options.c | 8 ++++---- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 9a182c30de1d..cbfaeaebf5a3 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -87,7 +87,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) return ld; } -static const char *daemons[] = { +static const char *subdaemons[] = { "lightning_channeld", "lightning_closingd", "lightning_connectd", @@ -98,12 +98,12 @@ static const char *daemons[] = { }; /* Check we can run them, and check their versions */ -void test_daemons(const struct lightningd *ld) +void test_subdaemons(const struct lightningd *ld) { size_t i; - for (i = 0; i < ARRAY_SIZE(daemons); i++) { + for (i = 0; i < ARRAY_SIZE(subdaemons); i++) { int outfd; - const char *dpath = path_join(tmpctx, ld->daemon_dir, daemons[i]); + const char *dpath = path_join(tmpctx, ld->daemon_dir, subdaemons[i]); const char *verstring; pid_t pid = pipecmd(&outfd, NULL, &outfd, dpath, "--version", NULL); @@ -116,17 +116,18 @@ void test_daemons(const struct lightningd *ld) err(1, "Could not get output from %s", dpath); if (!strstarts(verstring, version()) || verstring[strlen(version())] != '\n') - errx(1, "%s: bad version '%s'", daemons[i], verstring); + errx(1, "%s: bad version '%s'", + subdaemons[i], verstring); } } -/* Check if all daemons exist in specified directory. */ -static bool has_all_daemons(const char* daemon_dir) +/* Check if all subdaemons exist in specified directory. */ +static bool has_all_subdaemons(const char* daemon_dir) { size_t i; bool missing_daemon = false; - for (i = 0; i < ARRAY_SIZE(daemons); ++i) { - if (!path_is_file(path_join(tmpctx, daemon_dir, daemons[i]))) { + for (i = 0; i < ARRAY_SIZE(subdaemons); ++i) { + if (!path_is_file(path_join(tmpctx, daemon_dir, subdaemons[i]))) { missing_daemon = true; break; } @@ -189,7 +190,7 @@ static const char *find_my_pkglibexec_path(const tal_t *ctx, static const char *find_daemon_dir(const tal_t *ctx, const char *argv0) { const char *my_path = find_my_path(ctx, argv0); - if (has_all_daemons(my_path)) + if (has_all_subdaemons(my_path)) return my_path; return find_my_pkglibexec_path(ctx, take(my_path)); } @@ -333,7 +334,7 @@ int main(int argc, char *argv[]) signal(SIGPIPE, SIG_IGN); /* Make sure we can reach other daemons, and versions match. */ - test_daemons(ld); + test_subdaemons(ld); /* Initialize wallet, now that we are in the correct directory */ ld->wallet = wallet_new(ld, ld->log, &ld->timers); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 4ae2d5883bc3..cdd90b188562 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -207,7 +207,7 @@ struct lightningd { const struct chainparams *get_chainparams(const struct lightningd *ld); /* Check we can run subdaemons, and check their versions */ -void test_daemons(const struct lightningd *ld); +void test_subdaemons(const struct lightningd *ld); /* Notify lightningd about new blocks. */ void notify_new_block(struct lightningd *ld, u32 block_height); diff --git a/lightningd/options.c b/lightningd/options.c index e2b20511f3a1..aa23ea218745 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -678,9 +678,9 @@ static void opt_parse_from_config(struct lightningd *ld) tal_free(contents); } -static char *test_daemons_and_exit(struct lightningd *ld) +static char *test_subdaemons_and_exit(struct lightningd *ld) { - test_daemons(ld); + test_subdaemons(ld); exit(0); return NULL; } @@ -705,7 +705,7 @@ void register_opts(struct lightningd *ld) opt_register_early_noarg("--help|-h", opt_lightningd_usage, ld, "Print this message."); opt_register_early_noarg("--test-daemons-only", - test_daemons_and_exit, + test_subdaemons_and_exit, ld, opt_hidden); opt_register_arg("--bitcoin-datadir", opt_set_talstr, NULL, @@ -885,7 +885,7 @@ static void add_config(struct lightningd *ld, || opt->cb == (void *)opt_set_testnet || opt->cb == (void *)opt_set_mainnet || opt->cb == (void *)opt_lightningd_usage - || opt->cb == (void *)test_daemons_and_exit) { + || opt->cb == (void *)test_subdaemons_and_exit) { /* These are not important */ } else if (opt->cb == (void *)opt_set_bool) { const bool *b = opt->u.carg; From c3ec5fc267969445eebf86560cf182c6774e1ca4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:13:27 +0930 Subject: [PATCH 21/39] lightningd: remove gratuitous SIG_IGN: daemon_setup() does it already. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index cbfaeaebf5a3..4050161b7b9c 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -330,9 +330,6 @@ int main(int argc, char *argv[]) /* Handle options and config; move to .lightningd */ handle_opts(ld, argc, argv); - /* Ignore SIGPIPE: we look at our write return values*/ - signal(SIGPIPE, SIG_IGN); - /* Make sure we can reach other daemons, and versions match. */ test_subdaemons(ld); From 2af94f1817da1f25ba976ba27f6f95e68e68f6b9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:14:27 +0930 Subject: [PATCH 22/39] chaintopology: remove redundant wallet pointer. We already have access via the ld object, and we initialized this one twice anyway. Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 30 ++++++++++++++++-------------- lightningd/chaintopology.h | 4 +--- lightningd/lightningd.c | 1 - 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index de7112150ddd..fa921aa16671 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -76,7 +76,8 @@ static void filter_block_txs(struct chain_topology *topo, struct block *b) txo = txowatch_hash_get(&topo->txowatches, &out); if (txo) { - wallet_transaction_add(topo->wallet, tx, b->height, i); + wallet_transaction_add(topo->ld->wallet, + tx, b->height, i); txowatch_fire(txo, tx, j, b); } } @@ -92,7 +93,8 @@ static void filter_block_txs(struct chain_topology *topo, struct block *b) bitcoin_txid(tx, &txid); if (watching_txid(topo, &txid) || we_broadcast(topo, &txid) || satoshi_owned != 0) { - wallet_transaction_add(topo->wallet, tx, b->height, i); + wallet_transaction_add(topo->ld->wallet, + tx, b->height, i); } } b->full_txs = tal_free(b->full_txs); @@ -101,7 +103,7 @@ static void filter_block_txs(struct chain_topology *topo, struct block *b) size_t get_tx_depth(const struct chain_topology *topo, const struct bitcoin_txid *txid) { - u32 blockheight = wallet_transaction_height(topo->wallet, txid); + u32 blockheight = wallet_transaction_height(topo->ld->wallet, txid); if (blockheight == 0) return 0; @@ -161,7 +163,7 @@ static void rebroadcast_txs(struct chain_topology *topo, struct command *cmd) /* Put any txs we want to broadcast in ->txs. */ txs->txs = tal_arr(txs, const char *, 0); list_for_each(&topo->outgoing_txs, otx, list) { - if (wallet_transaction_height(topo->wallet, &otx->txid)) + if (wallet_transaction_height(topo->ld->wallet, &otx->txid)) continue; tal_resize(&txs->txs, num_txs+1); @@ -229,7 +231,7 @@ void broadcast_tx(struct chain_topology *topo, log_add(topo->log, " (tx %s)", type_to_string(tmpctx, struct bitcoin_txid, &otx->txid)); - wallet_transaction_add(topo->wallet, tx, 0, 0); + wallet_transaction_add(topo->ld->wallet, tx, 0, 0); bitcoind_sendrawtx(topo->bitcoind, otx->hextx, broadcast_done, otx); } @@ -566,7 +568,7 @@ static void topo_update_spends(struct chain_topology *topo, struct block *b) const struct bitcoin_tx *tx = b->full_txs[i]; for (size_t j = 0; j < tal_count(tx->input); j++) { const struct bitcoin_tx_input *input = &tx->input[j]; - scid = wallet_outpoint_spend(topo->wallet, tmpctx, + scid = wallet_outpoint_spend(topo->ld->wallet, tmpctx, b->height, &input->txid, input->index); if (scid) { @@ -584,7 +586,7 @@ static void topo_add_utxos(struct chain_topology *topo, struct block *b) for (size_t j = 0; j < tal_count(tx->output); j++) { const struct bitcoin_tx_output *output = &tx->output[j]; if (is_p2wsh(output->script, NULL)) { - wallet_utxoset_add(topo->wallet, tx, j, + wallet_utxoset_add(topo->ld->wallet, tx, j, b->height, i, output->script, output->amount); } @@ -599,7 +601,7 @@ static void add_tip(struct chain_topology *topo, struct block *b) b->prev = topo->tip; topo->tip->next = b; topo->tip = b; - wallet_block_add(topo->wallet, b); + wallet_block_add(topo->ld->wallet, b); topo_add_utxos(topo, b); topo_update_spends(topo, b); @@ -648,16 +650,16 @@ static void remove_tip(struct chain_topology *topo) b->height, type_to_string(tmpctx, struct bitcoin_blkid, &b->blkid)); - txs = wallet_transactions_by_height(b, topo->wallet, b->height); + txs = wallet_transactions_by_height(b, topo->ld->wallet, b->height); n = tal_count(txs); /* Notify that txs are kicked out. */ for (i = 0; i < n; i++) txwatch_fire(topo, &txs[i], 0); - wallet_block_remove(topo->wallet, b); + wallet_block_remove(topo->ld->wallet, b); /* This may have unconfirmed txs: reconfirm as we add blocks. */ - watch_for_utxo_reconfirmation(topo, topo->wallet); + watch_for_utxo_reconfirmation(topo, topo->ld->wallet); block_map_del(&topo->block_map, b); tal_free(b); } @@ -744,9 +746,9 @@ static void get_init_blockhash(struct bitcoind *bitcoind, u32 blockcount, /* Rollback to the given blockheight, so we start track * correctly again */ - wallet_blocks_rollback(topo->wallet, topo->max_blockheight); + wallet_blocks_rollback(topo->ld->wallet, topo->max_blockheight); /* This may have unconfirmed txs: reconfirm as we add blocks. */ - watch_for_utxo_reconfirmation(topo, topo->wallet); + watch_for_utxo_reconfirmation(topo, topo->ld->wallet); /* Get up to speed with topology. */ bitcoind_getblockhash(bitcoind, topo->max_blockheight, @@ -843,6 +845,7 @@ struct chain_topology *new_topology(struct lightningd *ld, struct log *log) { struct chain_topology *topo = tal(ld, struct chain_topology); + topo->ld = ld; block_map_init(&topo->block_map); list_head_init(&topo->outgoing_txs); txwatch_hash_init(&topo->txwatches); @@ -850,7 +853,6 @@ struct chain_topology *new_topology(struct lightningd *ld, struct log *log) topo->log = log; memset(topo->feerate, 0, sizeof(topo->feerate)); topo->bitcoind = new_bitcoind(topo, ld, log); - topo->wallet = ld->wallet; topo->poll_seconds = 30; topo->feerate_uninitialized = true; topo->root = NULL; diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index c2bf44e3dd55..175dfcdc5395 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -83,6 +83,7 @@ static inline bool block_eq(const struct block *b, const struct bitcoin_blkid *k HTABLE_DEFINE_TYPE(struct block, keyof_block_map, hash_sha, block_eq, block_map); struct chain_topology { + struct lightningd *ld; struct block *root; struct block *prev_tip, *tip; struct block_map block_map; @@ -90,9 +91,6 @@ struct chain_topology { bool feerate_uninitialized; u32 feehistory[NUM_FEERATES][FEE_HISTORY_NUM]; - /* Where to store blockchain info. */ - struct wallet *wallet; - /* Where to log things. */ struct log *log; diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 4050161b7b9c..ff5c1564db0e 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -336,7 +336,6 @@ int main(int argc, char *argv[]) /* Initialize wallet, now that we are in the correct directory */ ld->wallet = wallet_new(ld, ld->log, &ld->timers); ld->owned_txfilter = txfilter_new(ld); - ld->topology->wallet = ld->wallet; /* We do extra checks in io_loop. */ io_poll_debug = io_poll_override(io_poll_lightningd); From 3e53a63cf23abc96fce1d5eaf52dc6d95bc37751 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:15:27 +0930 Subject: [PATCH 23/39] wallet: do wallet_invoice init during preparation. We have a transaction anyway, and it's simpler. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 5 ---- lightningd/test/run-find_my_path.c | 3 -- wallet/invoices.c | 46 ++++++++++++------------------ wallet/invoices.h | 8 ------ wallet/test/run-wallet.c | 3 -- wallet/wallet.c | 8 +----- wallet/wallet.h | 14 --------- 7 files changed, 20 insertions(+), 67 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index ff5c1564db0e..9430b1c86d54 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -361,11 +361,6 @@ int main(int argc, char *argv[]) /* Initialize the transaction filter with our pubkeys. */ init_txfilter(ld->wallet, ld->owned_txfilter); - /* Check invoices loaded from the database */ - if (!wallet_invoice_load(ld->wallet)) { - fatal("Could not load invoices from the database"); - } - /* Set up invoice autoclean. */ wallet_invoice_autoclean(ld->wallet, ld->ini_autocleaninvoice_cycle, diff --git a/lightningd/test/run-find_my_path.c b/lightningd/test/run-find_my_path.c index b16ba138a884..82fa522f3dab 100644 --- a/lightningd/test/run-find_my_path.c +++ b/lightningd/test/run-find_my_path.c @@ -145,9 +145,6 @@ void wallet_invoice_autoclean(struct wallet * wallet UNNEEDED, u64 cycle_seconds UNNEEDED, u64 expired_by UNNEEDED) { fprintf(stderr, "wallet_invoice_autoclean called!\n"); abort(); } -/* Generated stub for wallet_invoice_load */ -bool wallet_invoice_load(struct wallet *wallet UNNEEDED) -{ fprintf(stderr, "wallet_invoice_load called!\n"); abort(); } /* Generated stub for wallet_network_check */ bool wallet_network_check(struct wallet *w UNNEEDED, const struct chainparams *chainparams UNNEEDED) diff --git a/wallet/invoices.c b/wallet/invoices.c index 17cf67f8d486..ba0f71ab49ba 100644 --- a/wallet/invoices.c +++ b/wallet/invoices.c @@ -128,6 +128,23 @@ static struct invoice_details *wallet_stmt2invoice_details(const tal_t *ctx, return dtl; } +/* Update expirations. */ +static void update_db_expirations(struct invoices *invoices, u64 now) +{ + sqlite3_stmt *stmt; + stmt = db_prepare(invoices->db, + "UPDATE invoices" + " SET state = ?" + " WHERE state = ?" + " AND expiry_time <= ?;"); + sqlite3_bind_int(stmt, 1, EXPIRED); + sqlite3_bind_int(stmt, 2, UNPAID); + sqlite3_bind_int64(stmt, 3, now); + db_exec_prepared(invoices->db, stmt); +} + +static void install_expiration_timer(struct invoices *invoices); + struct invoices *invoices_new(const tal_t *ctx, struct db *db, struct log *log, @@ -144,30 +161,16 @@ struct invoices *invoices_new(const tal_t *ctx, invs->expiration_timer = NULL; invs->autoclean_timer = NULL; + update_db_expirations(invs, time_now().ts.tv_sec); + install_expiration_timer(invs); return invs; } -/* Update expirations. */ -static void update_db_expirations(struct invoices *invoices, u64 now) -{ - sqlite3_stmt *stmt; - stmt = db_prepare(invoices->db, - "UPDATE invoices" - " SET state = ?" - " WHERE state = ?" - " AND expiry_time <= ?;"); - sqlite3_bind_int(stmt, 1, EXPIRED); - sqlite3_bind_int(stmt, 2, UNPAID); - sqlite3_bind_int64(stmt, 3, now); - db_exec_prepared(invoices->db, stmt); -} - struct invoice_id_node { struct list_node list; u64 id; }; -static void install_expiration_timer(struct invoices *invoices); static void trigger_expiration(struct invoices *invoices) { struct list_head idlist; @@ -254,17 +257,6 @@ static void install_expiration_timer(struct invoices *invoices) invoices); } -bool invoices_load(struct invoices *invoices) -{ - u64 now = time_now().ts.tv_sec; - - update_db_expirations(invoices, now); - - install_expiration_timer(invoices); - - return true; -} - bool invoices_create(struct invoices *invoices, struct invoice *pinvoice, u64 *msatoshi TAKES, diff --git a/wallet/invoices.h b/wallet/invoices.h index 00f8fa874027..de825b8ab529 100644 --- a/wallet/invoices.h +++ b/wallet/invoices.h @@ -29,14 +29,6 @@ struct invoices *invoices_new(const tal_t *ctx, struct log *log, struct timers *timers); -/** - * invoices_load - Second-stage constructor for invoice handler. - * Must be called before the other functions are called - * - * @invoices - the invoice handler. - */ -bool invoices_load(struct invoices *invoices); - /** * invoices_create - Create a new invoice. * diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 392a1ab32261..41dcee370c60 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -128,9 +128,6 @@ const struct invoice_details *invoices_iterator_deref( const tal_t *ctx UNNEEDED, struct invoices *invoices UNNEEDED, const struct invoice_iterator *it UNNEEDED) { fprintf(stderr, "invoices_iterator_deref called!\n"); abort(); } -/* Generated stub for invoices_load */ -bool invoices_load(struct invoices *invoices UNNEEDED) -{ fprintf(stderr, "invoices_load called!\n"); abort(); } /* Generated stub for invoices_new */ struct invoices *invoices_new(const tal_t *ctx UNNEEDED, struct db *db UNNEEDED, diff --git a/wallet/wallet.c b/wallet/wallet.c index abef40eca6dc..0f6b350f8c9c 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -52,10 +52,10 @@ struct wallet *wallet_new(struct lightningd *ld, wallet->db = db_setup(wallet, log); wallet->log = log; wallet->bip32_base = NULL; - wallet->invoices = invoices_new(wallet, wallet->db, log, timers); list_head_init(&wallet->unstored_payments); db_begin_transaction(wallet->db); + wallet->invoices = invoices_new(wallet, wallet->db, log, timers); outpointfilters_init(wallet); db_commit_transaction(wallet->db); return wallet; @@ -1422,12 +1422,6 @@ bool wallet_htlcs_reconnect(struct wallet *wallet, return true; } -/* Almost all wallet_invoice_* functions delegate to the - * appropriate invoices_* function. */ -bool wallet_invoice_load(struct wallet *wallet) -{ - return invoices_load(wallet->invoices); -} bool wallet_invoice_create(struct wallet *wallet, struct invoice *pinvoice, u64 *msatoshi TAKES, diff --git a/wallet/wallet.h b/wallet/wallet.h index 5c1f56b099e6..d93795be244a 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -565,20 +565,6 @@ struct invoice { #define INVOICE_MAX_LABEL_LEN 128 -/** - * wallet_invoice_load - Load the invoices from the database - * - * @wallet - the wallet whose invoices are to be loaded. - * - * All other wallet_invoice_* functions cannot be called - * until this function is called. - * As a database operation it must be called within - * db_begin_transaction .. db_commit_transaction - * (all other invoice functions also have this requirement). - * Returns true if loaded successfully. - */ -bool wallet_invoice_load(struct wallet *wallet); - /** * wallet_invoice_create - Create a new invoice. * From 168bec09742908535fd314762672a1e200f4eb0b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:38:53 +0930 Subject: [PATCH 24/39] lightningd: move channel/peer/htlc load into own function. Also, wallet has no business wiring up HTLCs; move that code to peer_htlcs.c. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 22 +------- lightningd/peer_control.c | 27 ++++++++++ lightningd/peer_control.h | 3 ++ lightningd/peer_htlcs.c | 42 +++++++++++++++ lightningd/peer_htlcs.h | 4 ++ lightningd/test/run-find_my_path.c | 17 ++---- wallet/test/run-wallet.c | 83 ++++++++++++++++++++++++++++-- wallet/wallet.c | 39 -------------- wallet/wallet.h | 12 +---- 9 files changed, 162 insertions(+), 87 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 9430b1c86d54..514d9f4d55f9 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -366,26 +366,8 @@ int main(int argc, char *argv[]) ld->ini_autocleaninvoice_cycle, ld->ini_autocleaninvoice_expiredby); - /* Load peers from database */ - if (!wallet_channels_load_active(ld, ld->wallet)) - fatal("Could not load channels from the database"); - - /* TODO(cdecker) Move this into common location for initialization */ - struct peer *peer; - list_for_each(&ld->peers, peer, list) { - struct channel *channel; - - list_for_each(&peer->channels, channel, list) { - if (!wallet_htlcs_load_for_channel(ld->wallet, - channel, - &ld->htlcs_in, - &ld->htlcs_out)) { - fatal("could not load htlcs for channel"); - } - } - } - if (!wallet_htlcs_reconnect(ld->wallet, &ld->htlcs_in, &ld->htlcs_out)) - fatal("could not reconnect htlcs loaded from wallet, wallet may be inconsistent."); + /* Pull peers, channels and HTLCs from db. */ + load_channels_from_wallet(ld); /* Get the blockheight we are currently at, UINT32_MAX is used to signal * an unitialized wallet and that we should start off of bitcoind's diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 7c7c1902e928..cba4f5130ee5 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -945,6 +945,33 @@ void activate_peers(struct lightningd *ld) activate_peer(p); } +/* Pull peers, channels and HTLCs from db, and wire them up. */ +void load_channels_from_wallet(struct lightningd *ld) +{ + struct peer *peer; + + /* Load peers from database */ + if (!wallet_channels_load_active(ld, ld->wallet)) + fatal("Could not load channels from the database"); + + /* This is a poor-man's db join :( */ + list_for_each(&ld->peers, peer, list) { + struct channel *channel; + + list_for_each(&peer->channels, channel, list) { + if (!wallet_htlcs_load_for_channel(ld->wallet, + channel, + &ld->htlcs_in, + &ld->htlcs_out)) { + fatal("could not load htlcs for channel"); + } + } + } + + /* Now connect HTLC pointers together */ + htlcs_reconnect(ld, &ld->htlcs_in, &ld->htlcs_out); +} + static void json_disconnect(struct command *cmd, const char *buffer, const jsmntok_t *params) { diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 860f7bb33752..56ea372ad60c 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -89,4 +89,7 @@ void activate_peers(struct lightningd *ld); void drop_to_chain(struct lightningd *ld, struct channel *channel, bool cooperative); void channel_watch_funding(struct lightningd *ld, struct channel *channel); + +/* Pull peers, channels and HTLCs from db, and wire them up. */ +void load_channels_from_wallet(struct lightningd *ld); #endif /* LIGHTNING_LIGHTNINGD_PEER_CONTROL_H */ diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 8f3e43a28ba3..db7652feb27d 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -1671,6 +1671,48 @@ void htlcs_notify_new_block(struct lightningd *ld, u32 height) } while (removed); } +/** + * htlcs_reconnect -- Link outgoing HTLCs to their origins after initial db load + * + * For each outgoing HTLC find the incoming HTLC that triggered it. If + * we are the origin of the transfer then we cannot resolve the + * incoming HTLC in which case we just leave it `NULL`. + */ +void htlcs_reconnect(struct lightningd *ld, + struct htlc_in_map *htlcs_in, + struct htlc_out_map *htlcs_out) +{ + struct htlc_in_map_iter ini; + struct htlc_out_map_iter outi; + struct htlc_in *hin; + struct htlc_out *hout; + + for (hout = htlc_out_map_first(htlcs_out, &outi); hout; + hout = htlc_out_map_next(htlcs_out, &outi)) { + + if (hout->origin_htlc_id == 0) { + continue; + } + + for (hin = htlc_in_map_first(htlcs_in, &ini); hin; + hin = htlc_in_map_next(htlcs_in, &ini)) { + if (hout->origin_htlc_id == hin->dbid) { + log_debug(ld->log, + "Found corresponding htlc_in %" PRIu64 + " for htlc_out %" PRIu64, + hin->dbid, hout->dbid); + hout->in = hin; + break; + } + } + + if (!hout->in) + fatal("Unable to find corresponding htlc_in %"PRIu64" for htlc_out %"PRIu64, + hout->origin_htlc_id, hout->dbid); + } +} + + #if DEVELOPER static void json_dev_ignore_htlcs(struct command *cmd, const char *buffer, const jsmntok_t *params) diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index f1083315ee76..a36c01b965b1 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -60,4 +60,8 @@ void onchain_fulfilled_htlc(struct channel *channel, void htlcs_notify_new_block(struct lightningd *ld, u32 height); +void htlcs_reconnect(struct lightningd *ld, + struct htlc_in_map *htlcs_in, + struct htlc_out_map *htlcs_out); + #endif /* LIGHTNING_LIGHTNINGD_PEER_HTLCS_H */ diff --git a/lightningd/test/run-find_my_path.c b/lightningd/test/run-find_my_path.c index 82fa522f3dab..672223ef014b 100644 --- a/lightningd/test/run-find_my_path.c +++ b/lightningd/test/run-find_my_path.c @@ -71,6 +71,9 @@ void htlcs_notify_new_block(struct lightningd *ld UNNEEDED, u32 height UNNEEDED) /* Generated stub for json_escape */ struct json_escaped *json_escape(const tal_t *ctx UNNEEDED, const char *str TAKES UNNEEDED) { fprintf(stderr, "json_escape called!\n"); abort(); } +/* Generated stub for load_channels_from_wallet */ +void load_channels_from_wallet(struct lightningd *ld UNNEEDED) +{ fprintf(stderr, "load_channels_from_wallet called!\n"); abort(); } /* Generated stub for log_ */ void log_(struct log *log UNNEEDED, enum log_level level UNNEEDED, const char *fmt UNNEEDED, ...) @@ -126,20 +129,6 @@ const char *version(void) /* Generated stub for wallet_blocks_heights */ void wallet_blocks_heights(struct wallet *w UNNEEDED, u32 def UNNEEDED, u32 *min UNNEEDED, u32 *max UNNEEDED) { fprintf(stderr, "wallet_blocks_heights called!\n"); abort(); } -/* Generated stub for wallet_channels_load_active */ -bool wallet_channels_load_active(const tal_t *ctx UNNEEDED, struct wallet *w UNNEEDED) -{ fprintf(stderr, "wallet_channels_load_active called!\n"); abort(); } -/* Generated stub for wallet_htlcs_load_for_channel */ -bool wallet_htlcs_load_for_channel(struct wallet *wallet UNNEEDED, - struct channel *chan UNNEEDED, - struct htlc_in_map *htlcs_in UNNEEDED, - struct htlc_out_map *htlcs_out UNNEEDED) -{ fprintf(stderr, "wallet_htlcs_load_for_channel called!\n"); abort(); } -/* Generated stub for wallet_htlcs_reconnect */ -bool wallet_htlcs_reconnect(struct wallet *wallet UNNEEDED, - struct htlc_in_map *htlcs_in UNNEEDED, - struct htlc_out_map *htlcs_out UNNEEDED) -{ fprintf(stderr, "wallet_htlcs_reconnect called!\n"); abort(); } /* Generated stub for wallet_invoice_autoclean */ void wallet_invoice_autoclean(struct wallet * wallet UNNEEDED, u64 cycle_seconds UNNEEDED, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 41dcee370c60..dee09cd697c0 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -12,6 +12,7 @@ static void db_log_(struct log *log UNUSED, enum log_level level UNUSED, const c #include "wallet/wallet.c" #include "lightningd/htlc_end.c" #include "lightningd/peer_control.c" +#include "lightningd/peer_htlcs.c" #include "lightningd/channel.c" #include "wallet/db.c" @@ -69,12 +70,30 @@ void delay_then_reconnect(struct channel *channel UNNEEDED, u32 seconds_delay UN /* Generated stub for fatal */ void fatal(const char *fmt UNNEEDED, ...) { fprintf(stderr, "fatal called!\n"); abort(); } +/* Generated stub for fromwire_channel_got_commitsig */ +bool fromwire_channel_got_commitsig(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *commitnum UNNEEDED, u32 *feerate UNNEEDED, secp256k1_ecdsa_signature *signature UNNEEDED, secp256k1_ecdsa_signature **htlc_signature UNNEEDED, struct added_htlc **added UNNEEDED, struct secret **shared_secret UNNEEDED, struct fulfilled_htlc **fulfilled UNNEEDED, struct failed_htlc ***failed UNNEEDED, struct changed_htlc **changed UNNEEDED, struct bitcoin_tx **tx UNNEEDED) +{ fprintf(stderr, "fromwire_channel_got_commitsig called!\n"); abort(); } +/* Generated stub for fromwire_channel_got_revoke */ +bool fromwire_channel_got_revoke(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *revokenum UNNEEDED, struct secret *per_commitment_secret UNNEEDED, struct pubkey *next_per_commit_point UNNEEDED, u32 *feerate UNNEEDED, struct changed_htlc **changed UNNEEDED) +{ fprintf(stderr, "fromwire_channel_got_revoke called!\n"); abort(); } +/* Generated stub for fromwire_channel_offer_htlc_reply */ +bool fromwire_channel_offer_htlc_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *id UNNEEDED, u16 *failure_code UNNEEDED, u8 **failurestr UNNEEDED) +{ fprintf(stderr, "fromwire_channel_offer_htlc_reply called!\n"); abort(); } +/* Generated stub for fromwire_channel_sending_commitsig */ +bool fromwire_channel_sending_commitsig(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *commitnum UNNEEDED, u32 *feerate UNNEEDED, struct changed_htlc **changed UNNEEDED, secp256k1_ecdsa_signature *commit_sig UNNEEDED, secp256k1_ecdsa_signature **htlc_sigs UNNEEDED) +{ fprintf(stderr, "fromwire_channel_sending_commitsig called!\n"); abort(); } /* Generated stub for fromwire_connect_peer_connected */ bool fromwire_connect_peer_connected(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct pubkey *id UNNEEDED, struct wireaddr_internal *addr UNNEEDED, struct crypto_state *crypto_state UNNEEDED, u8 **gfeatures UNNEEDED, u8 **lfeatures UNNEEDED) { fprintf(stderr, "fromwire_connect_peer_connected called!\n"); abort(); } +/* Generated stub for fromwire_gossip_resolve_channel_reply */ +bool fromwire_gossip_resolve_channel_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct pubkey **keys UNNEEDED) +{ fprintf(stderr, "fromwire_gossip_resolve_channel_reply called!\n"); abort(); } /* Generated stub for fromwire_hsm_sign_commitment_tx_reply */ bool fromwire_hsm_sign_commitment_tx_reply(const void *p UNNEEDED, secp256k1_ecdsa_signature *sig UNNEEDED) { fprintf(stderr, "fromwire_hsm_sign_commitment_tx_reply called!\n"); abort(); } +/* Generated stub for get_block_height */ +u32 get_block_height(const struct chain_topology *topo UNNEEDED) +{ fprintf(stderr, "get_block_height called!\n"); abort(); } /* Generated stub for invoices_autoclean_set */ void invoices_autoclean_set(struct invoices *invoices UNNEEDED, u64 cycle_seconds UNNEEDED, @@ -275,6 +294,9 @@ enum watch_result onchaind_funding_spent(struct channel *channel UNNEEDED, const struct bitcoin_tx *tx UNNEEDED, u32 blockheight UNNEEDED) { fprintf(stderr, "onchaind_funding_spent called!\n"); abort(); } +/* Generated stub for onion_type_name */ +const char *onion_type_name(int e UNNEEDED) +{ fprintf(stderr, "onion_type_name called!\n"); abort(); } /* Generated stub for opening_peer_no_active_channels */ void opening_peer_no_active_channels(struct peer *peer UNNEEDED) { fprintf(stderr, "opening_peer_no_active_channels called!\n"); abort(); } @@ -297,6 +319,24 @@ void outpointfilter_remove(struct outpointfilter *of UNNEEDED, bool param(struct command *cmd UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t params[] UNNEEDED, ...) { fprintf(stderr, "param called!\n"); abort(); } +/* Generated stub for parse_onionpacket */ +struct onionpacket *parse_onionpacket( + const tal_t *ctx UNNEEDED, + const void *src UNNEEDED, + const size_t srclen + ) +{ fprintf(stderr, "parse_onionpacket called!\n"); abort(); } +/* Generated stub for payment_failed */ +void payment_failed(struct lightningd *ld UNNEEDED, const struct htlc_out *hout UNNEEDED, + const char *localfail UNNEEDED) +{ fprintf(stderr, "payment_failed called!\n"); abort(); } +/* Generated stub for payment_store */ +void payment_store(struct lightningd *ld UNNEEDED, const struct sha256 *payment_hash UNNEEDED) +{ fprintf(stderr, "payment_store called!\n"); abort(); } +/* Generated stub for payment_succeeded */ +void payment_succeeded(struct lightningd *ld UNNEEDED, struct htlc_out *hout UNNEEDED, + const struct preimage *rval UNNEEDED) +{ fprintf(stderr, "payment_succeeded called!\n"); abort(); } /* Generated stub for peer_start_channeld */ void peer_start_channeld(struct channel *channel UNNEEDED, const struct crypto_state *cs UNNEEDED, @@ -317,6 +357,20 @@ void peer_start_openingd(struct peer *peer UNNEEDED, int peer_fd UNNEEDED, int gossip_fd UNNEEDED, const u8 *msg UNNEEDED) { fprintf(stderr, "peer_start_openingd called!\n"); abort(); } +/* Generated stub for process_onionpacket */ +struct route_step *process_onionpacket( + const tal_t * ctx UNNEEDED, + const struct onionpacket *packet UNNEEDED, + const u8 *shared_secret UNNEEDED, + const u8 *assocdata UNNEEDED, + const size_t assocdatalen + ) +{ fprintf(stderr, "process_onionpacket called!\n"); abort(); } +/* Generated stub for serialize_onionpacket */ +u8 *serialize_onionpacket( + const tal_t *ctx UNNEEDED, + const struct onionpacket *packet UNNEEDED) +{ fprintf(stderr, "serialize_onionpacket called!\n"); abort(); } /* Generated stub for subd_release_channel */ void subd_release_channel(struct subd *owner UNNEEDED, void *channel UNNEEDED) { fprintf(stderr, "subd_release_channel called!\n"); abort(); } @@ -334,6 +388,24 @@ void subd_send_msg(struct subd *sd UNNEEDED, const u8 *msg_out UNNEEDED) /* Generated stub for towire_channel_dev_reenable_commit */ u8 *towire_channel_dev_reenable_commit(const tal_t *ctx UNNEEDED) { fprintf(stderr, "towire_channel_dev_reenable_commit called!\n"); abort(); } +/* Generated stub for towire_channel_fail_htlc */ +u8 *towire_channel_fail_htlc(const tal_t *ctx UNNEEDED, const struct failed_htlc *failed_htlc UNNEEDED) +{ fprintf(stderr, "towire_channel_fail_htlc called!\n"); abort(); } +/* Generated stub for towire_channel_fulfill_htlc */ +u8 *towire_channel_fulfill_htlc(const tal_t *ctx UNNEEDED, const struct fulfilled_htlc *fulfilled_htlc UNNEEDED) +{ fprintf(stderr, "towire_channel_fulfill_htlc called!\n"); abort(); } +/* Generated stub for towire_channel_got_commitsig_reply */ +u8 *towire_channel_got_commitsig_reply(const tal_t *ctx UNNEEDED) +{ fprintf(stderr, "towire_channel_got_commitsig_reply called!\n"); abort(); } +/* Generated stub for towire_channel_got_revoke_reply */ +u8 *towire_channel_got_revoke_reply(const tal_t *ctx UNNEEDED) +{ fprintf(stderr, "towire_channel_got_revoke_reply called!\n"); abort(); } +/* Generated stub for towire_channel_offer_htlc */ +u8 *towire_channel_offer_htlc(const tal_t *ctx UNNEEDED, u64 amount_msat UNNEEDED, u32 cltv_expiry UNNEEDED, const struct sha256 *payment_hash UNNEEDED, const u8 onion_routing_packet[1366]) +{ fprintf(stderr, "towire_channel_offer_htlc called!\n"); abort(); } +/* Generated stub for towire_channel_sending_commitsig_reply */ +u8 *towire_channel_sending_commitsig_reply(const tal_t *ctx UNNEEDED) +{ fprintf(stderr, "towire_channel_sending_commitsig_reply called!\n"); abort(); } /* Generated stub for towire_channel_send_shutdown */ u8 *towire_channel_send_shutdown(const tal_t *ctx UNNEEDED) { fprintf(stderr, "towire_channel_send_shutdown called!\n"); abort(); } @@ -348,9 +420,15 @@ u8 *towire_errorfmt(const tal_t *ctx UNNEEDED, const struct channel_id *channel UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "towire_errorfmt called!\n"); abort(); } +/* Generated stub for towire_gossip_resolve_channel_request */ +u8 *towire_gossip_resolve_channel_request(const tal_t *ctx UNNEEDED, const struct short_channel_id *channel_id UNNEEDED) +{ fprintf(stderr, "towire_gossip_resolve_channel_request called!\n"); abort(); } /* Generated stub for towire_hsm_sign_commitment_tx */ u8 *towire_hsm_sign_commitment_tx(const tal_t *ctx UNNEEDED, const struct pubkey *peer_id UNNEEDED, u64 channel_dbid UNNEEDED, const struct bitcoin_tx *tx UNNEEDED, const struct pubkey *remote_funding_key UNNEEDED, u64 funding_amount UNNEEDED) { fprintf(stderr, "towire_hsm_sign_commitment_tx called!\n"); abort(); } +/* Generated stub for towire_onchain_known_preimage */ +u8 *towire_onchain_known_preimage(const tal_t *ctx UNNEEDED, const struct preimage *preimage UNNEEDED) +{ fprintf(stderr, "towire_onchain_known_preimage called!\n"); abort(); } /* Generated stub for watch_txid */ struct txwatch *watch_txid(const tal_t *ctx UNNEEDED, struct chain_topology *topo UNNEEDED, @@ -903,10 +981,9 @@ static bool test_htlc_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(wallet_htlcs_load_for_channel(w, chan, htlcs_in, htlcs_out), "Failed loading HTLCs"); - - CHECK_MSG(wallet_htlcs_reconnect(w, htlcs_in, htlcs_out), - "Unable to reconnect htlcs."); db_commit_transaction(w->db); + + htlcs_reconnect(w->ld, htlcs_in, htlcs_out); CHECK(!wallet_err); hin = htlc_in_map_get(htlcs_in, &in.key); diff --git a/wallet/wallet.c b/wallet/wallet.c index 0f6b350f8c9c..500a745fd4c8 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1383,45 +1383,6 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, return ok; } -bool wallet_htlcs_reconnect(struct wallet *wallet, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out) -{ - struct htlc_in_map_iter ini; - struct htlc_out_map_iter outi; - struct htlc_in *hin; - struct htlc_out *hout; - - for (hout = htlc_out_map_first(htlcs_out, &outi); hout; - hout = htlc_out_map_next(htlcs_out, &outi)) { - - if (hout->origin_htlc_id == 0) { - continue; - } - - for (hin = htlc_in_map_first(htlcs_in, &ini); hin; - hin = htlc_in_map_next(htlcs_in, &ini)) { - if (hout->origin_htlc_id == hin->dbid) { - log_debug(wallet->log, - "Found corresponding htlc_in %" PRIu64 - " for htlc_out %" PRIu64, - hin->dbid, hout->dbid); - hout->in = hin; - break; - } - } - - if (!hout->in) { - log_broken( - wallet->log, - "Unable to find corresponding htlc_in %"PRIu64" for htlc_out %"PRIu64, - hout->origin_htlc_id, hout->dbid); - } - - } - return true; -} - bool wallet_invoice_create(struct wallet *wallet, struct invoice *pinvoice, u64 *msatoshi TAKES, diff --git a/wallet/wallet.h b/wallet/wallet.h index d93795be244a..28df8f21720b 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -480,7 +480,7 @@ void wallet_htlc_update(struct wallet *wallet, const u64 htlc_dbid, * may not have been loaded yet. In the latter case the pay_command * does not exist anymore since we restarted. * - * Use `wallet_htlcs_reconnect` to wire htlc_out instances to the + * Use `htlcs_reconnect` to wire htlc_out instances to the * corresponding htlc_in after loading all channels. */ bool wallet_htlcs_load_for_channel(struct wallet *wallet, @@ -488,16 +488,6 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, struct htlc_in_map *htlcs_in, struct htlc_out_map *htlcs_out); -/** - * wallet_htlcs_reconnect -- Link outgoing HTLCs to their origins - * - * For each outgoing HTLC find the incoming HTLC that triggered it. If - * we are the origin of the transfer then we cannot resolve the - * incoming HTLC in which case we just leave it `NULL`. - */ -bool wallet_htlcs_reconnect(struct wallet *wallet, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out); /* /!\ This is a DB ENUM, please do not change the numbering of any * already defined elements (adding is ok) /!\ */ From 8bc845d7b6953643ecd5471a358870ca342687a4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 10:38:56 +0930 Subject: [PATCH 25/39] lightningd: inline overview documentation (part 1 of 8). Signed-off-by: Rusty Russell --- doc/HACKING.md | 37 ++--- lightningd/lightningd.c | 319 +++++++++++++++++++++++++++++++++++----- 2 files changed, 298 insertions(+), 58 deletions(-) diff --git a/doc/HACKING.md b/doc/HACKING.md index 35f544a62151..fa35e52001b5 100644 --- a/doc/HACKING.md +++ b/doc/HACKING.md @@ -14,24 +14,10 @@ Getting Started It's in C, to encourage alternate implementations. Patches are welcome! You should read our [Style Guide](STYLE.md). -To read the code, you'll probably need to understand `ccan/tal`: it's a -hierarchical memory allocator, where each allocation has a parent, and -thus lifetimes are grouped. eg. a `struct bitcoin_tx` has a pointer -to an array of `struct bitcoin_tx_input`; they are allocated off the -`struct bitcoind_tx`, so freeing the `struct bitcoind_tx` frees them all. -Tal also supports destructors, which are usually used to remove things -from lists, etc. - -Some routines use take(): take() marks a pointer as to be consumed -(e.g. freed automatically before return) by a called function. -It can safely accept NULL pointers. -Functions whose prototype in headers has the macro TAKES can have the -specific argument as a take() call. -Use this sparingly, as it can be very confusing. - -The more complex daemons use async io (ccan/io): you register callbacks -and they happen once I/O is available, then you return what to do next. -This does not use threads, so the code flow is generally fairly simple. +To read the code, you should start from +[lightningd.c](../lightningd/lightningd.c) and hop your way through +the '~' comments at the head of each daemon in the suggested +order. The Components -------------- @@ -61,8 +47,13 @@ Here's a list of parts, with notes: - mockup.sh / update-mocks.sh: tools to generate mock functions for unit tests. +* tests/ - blackbox tests (mainly) + - unit tests are in tests/ subdirectories in each other directory. + +* doc/ - you are here + * devtools/ - tools for developers - - Currently just bolt11-cli for decoding bolt11 + - Generally for decoding our formats. * contrib/ - python support and other stuff which doesn't belong :) @@ -80,10 +71,12 @@ Here's a list of parts, with notes: * hsmd/ - daemon which looks after the cryptographic secret, and performs commitment signing. -* gossipd/ - daemon to chat to peers which don't have any channels, - and maintains routing information and broadcasts gossip. +* gossipd/ - daemon to maintain routing information and broadcast gossip. + +* connectd/ - daemon to connect to other peers, and receive incoming. -* openingd/ - daemon to open a channel for a single peer. +* openingd/ - daemon to open a channel for a single peer, and chat to + a peer which doesn't have any channels/ * channeld/ - daemon to operate a single peer once channel is operating normally. diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 514d9f4d55f9..f9c7c7110837 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -1,9 +1,39 @@ +/*~ Welcome, wonderful reader! + * + * This is the core of c-lightning: the main file of the master daemon + * `lightningd`. It's mainly cluttered with the miscellany of setup, + * and a few startup sanity checks. + * + * The role of this daemon is to start the subdaemons, shuffle peers + * between them, handle the JSON RPC requests, bitcoind, the database + * and centralize logging. In theory, it doesn't trust the other + * daemons, though we expect hsmd to be responsive. + * + * Comments beginning with a ~ (like this one!) are part of our shared + * adventure through the source, so they're more meta than normal code + * comments, and mean to be read in a certain order. + */ + +/*~ Notice how includes are in ASCII order: this is actually enforced by + * the build system under 'make check-source'. It avoids merge conflicts + * and keeps things consistent. */ #include "gossip_control.h" #include "hsm_control.h" #include "lightningd.h" #include "peer_control.h" #include "subd.h" + +/*~ This is Ian Lance Taylor's libbacktrace. It turns out that it's + * horrifically difficult to obtain a decent backtrace in C; the standard + * backtrace function is useless in most programs. */ #include + +/*~ These headers are from CCAN: http://ccodearchive.net. + * + * It's another one of Rusty's projects, and we copy and paste it + * automatically into the source tree here, so you should never edit + * it. There's a Makefile target update-ccan to update it (and add modules + * if CCAN_NEW is specified). */ #include #include #include @@ -18,11 +48,14 @@ #include #include #include + +/*~ This is common code: routines shared by one or more programs. */ #include #include #include #include #include + #include #include #include @@ -39,39 +72,86 @@ #include #include +/*~ The core lightning object: it's passed everywhere, and is basically a + * global variable. This new_xxx pattern is something we'll see often: + * it allocates and initializes a new structure, using *tal*, the heirarchitcal + * allocator. */ static struct lightningd *new_lightningd(const tal_t *ctx) { + /*~ tal: each allocation is a child of an existing object (or NULL, + * the top-level object). When an object is freed, all the objects + * 'tallocated' off it are also freed. In this case, freeing 'ctx' + * will free 'ld'. + * + * It's incredibly useful for grouping object lifetimes, as we'll see. + */ struct lightningd *ld = tal(ctx, struct lightningd); + /*~ Note that we generally EXPLICITLY #if-wrap DEVELOPER code. This + * is a nod to keeping it minimal and explicit: we need this code for + * testing, but its existence means we're not actually testing the + * same exact code users will be running. */ #if DEVELOPER ld->dev_debug_subdaemon = NULL; ld->dev_disconnect_fd = -1; ld->dev_subdaemon_fail = false; ld->dev_allow_localhost = false; + /*~ Behaving differently depending on environment variables is a hack, + * *but* hacks are allowed for dev-mode stuff. In this case, there's + * a significant overhead to the memory leak detection stuff, and + * we can't use it under valgrind, so the test harness uses this var + * to disable it in that case. */ if (getenv("LIGHTNINGD_DEV_MEMLEAK")) memleak_init(); #endif + /*~ These are CCAN lists: an embedded double-linked list. It's not + * really typesafe, but relies on convention to access the contents. + * It's inspired by the closely-related Linux kernel list.h. */ list_head_init(&ld->peers); + + /*~ These are hash tables of incoming and outgoing HTLCs (contracts) */ htlc_in_map_init(&ld->htlcs_in); htlc_out_map_init(&ld->htlcs_out); + + /*~ We have a log-book infrastructure: we define a 20MB log book and + * point our log objects into it. */ ld->log_book = new_log_book(20*1024*1024, LOG_INFORM); + /*~ Note the tal context arg (by convention, the first argument to any + * allocation function): ld->log will be implicitly freed when ld + * is. */ ld->log = new_log(ld, ld->log_book, "lightningd(%u):", (int)getpid()); ld->logfile = NULL; + + /*~ We explicitly set these to NULL: if they're still NULL after option + * parsing, we know they're to be set to the defaults. */ ld->alias = NULL; ld->rgb = NULL; list_head_init(&ld->connects); list_head_init(&ld->waitsendpay_commands); list_head_init(&ld->sendpay_commands); list_head_init(&ld->close_commands); + + /*~ Tal also explicitly supports arrays: it stores the number of + * elements, which can be accessed with tal_count() (or tal_bytelen() + * for raw bytecount). It's common for simple arrays to use + * tal_resize(), which is a typesafe realloc function, but as all + * talocations need a parent, we start with an empty array rather than + * NULL. */ ld->proposed_wireaddr = tal_arr(ld, struct wireaddr_internal, 0); ld->proposed_listen_announce = tal_arr(ld, enum addr_listen_announce, 0); ld->portnum = DEFAULT_PORT; ld->listen = true; ld->autolisten = true; ld->reconnect = true; + + /*~ This is from ccan/timer: a scalable timer system which has a + * fascinating implementation you should read if you have a spare + * few hours */ timers_init(&ld->timers, time_mono()); + + /*~ This is detailed in chaintopology.c */ ld->topology = new_topology(ld, ld->log); ld->daemon = false; ld->config_filename = NULL; @@ -87,6 +167,8 @@ static struct lightningd *new_lightningd(const tal_t *ctx) return ld; } +/*~ We list our daemons here so on startup we can test they're the + * correct versions and that they exist. */ static const char *subdaemons[] = { "lightning_channeld", "lightning_closingd", @@ -97,29 +179,43 @@ static const char *subdaemons[] = { "lightning_openingd" }; -/* Check we can run them, and check their versions */ +/*~ Check we can run them, and check their versions */ void test_subdaemons(const struct lightningd *ld) { size_t i; + + /*~ CCAN's ARRAY_SIZE() should always be used on defined arrays: it will + * fail to build if the argument is actually a pointer, not an array! */ for (i = 0; i < ARRAY_SIZE(subdaemons); i++) { int outfd; + /*~ CCAN's path module uses tal, so wants a context to allocate + * from. We have a magic context 'tmpctx' which is freed in + * the event loop for transient allocations like this. */ const char *dpath = path_join(tmpctx, ld->daemon_dir, subdaemons[i]); const char *verstring; + /*~ CCAN's pipecmd module is like popen for grownups. */ pid_t pid = pipecmd(&outfd, NULL, &outfd, dpath, "--version", NULL); + /*~ Our logging system: spam goes in at log_debug level */ log_debug(ld->log, "testing %s", dpath); if (pid == -1) err(1, "Could not run %s", dpath); + /*~ CCAN's grab_file module contains a routine to read into a + * tallocated buffer until EOF */ verstring = grab_fd(tmpctx, outfd); + /*~ Like many CCAN modules, it set errno on failure, which + * err (ccan/err, but usually just the BSD ) prints */ if (!verstring) err(1, "Could not get output from %s", dpath); + /*~ strstarts is from CCAN/str. */ if (!strstarts(verstring, version()) || verstring[strlen(version())] != '\n') errx(1, "%s: bad version '%s'", subdaemons[i], verstring); } } + /* Check if all subdaemons exist in specified directory. */ static bool has_all_subdaemons(const char* daemon_dir) { @@ -136,10 +232,16 @@ static bool has_all_subdaemons(const char* daemon_dir) return !missing_daemon; } +/* This routine tries to determine what path the lightningd binary is in. + * It's not actually that simple! */ static const char *find_my_path(const tal_t *ctx, const char *argv0) { char *me; + /* A command containing / is run relative to the current directory, + * not searched through the path. The shell sets argv0 to the command + * run, though something else could set it to a arbitrary value and + * this logic would be wrong. */ if (strchr(argv0, PATH_SEP)) { const char *path; /* Absolute paths are easy. */ @@ -159,6 +261,7 @@ static const char *find_my_path(const tal_t *ctx, const char *argv0) const char *pathenv = getenv("PATH"); size_t i; + /* This replicates the standard shell path search algorithm */ if (!pathenv) errx(1, "Cannot find myself: no $PATH set"); @@ -177,47 +280,85 @@ static const char *find_my_path(const tal_t *ctx, const char *argv0) errx(1, "Cannot find %s in $PATH", argv0); } + /*~ The caller just wants the directory we're in. + * + * Note the magic "take()" macro here: it annotates a pointer as "to + * be taken", and the recipient is expected to take ownership of the + * pointer. + * + * Many CCAN and our own routines support this, but if you hand a take() + * to a non-take routine unfortunately you don't get a compile error. + */ return path_dirname(ctx, take(me)); } + +/*~ This returns the PKGLIBEXEC path which is where binaries get installed. + * Note the 'TAKES' annotation which is merely documentation that it will + * take ownership of 'my_path' if the caller hands take() there. + */ static const char *find_my_pkglibexec_path(const tal_t *ctx, const char *my_path TAKES) { const char *pkglibexecdir; pkglibexecdir = path_join(ctx, my_path, BINTOPKGLIBEXECDIR); + + /*~ Sometimes take() can be more efficient, since the routine can + * manipulate the string in place. This is the case here. */ return path_simplify(ctx, take(pkglibexecdir)); } + /* Determine the correct daemon dir. */ static const char *find_daemon_dir(const tal_t *ctx, const char *argv0) { const char *my_path = find_my_path(ctx, argv0); + /* If we're running in-tree, all the subdaemons are with lightningd. */ if (has_all_subdaemons(my_path)) return my_path; + + /* Otherwise we assume they're in the installed dir. */ return find_my_pkglibexec_path(ctx, take(my_path)); } +/*~ We like to free everything on exit, so valgrind doesn't complain. In some + * ways it would be neater not to do this, but it turns out some transient + * objects still need cleaning. */ static void shutdown_subdaemons(struct lightningd *ld) { struct peer *p; + /*~ Because tal objects can be free indirectly, by freeing their parents + * it turns out to be vital to be able to add *destructors* to objects. + * As a result, freeing them may cause callbacks; in this case, some + * objects freed here can cause database writes, which must be inside + * a transaction */ db_begin_transaction(ld->wallet->db); + /* Let everyone shutdown cleanly. */ close(ld->hsm_fd); + /*~ The three "global" daemons, which we shutdown explicitly. */ subd_shutdown(ld->connectd, 10); subd_shutdown(ld->gossip, 10); subd_shutdown(ld->hsm, 10); + /* Now we free all the HTLCs */ free_htlcs(ld, NULL); + /*~ For every peer, we free every channel. Note that the peer has a + * destructor (by convention, called destroy_peer) which removes it + * from the list. Thus we use list_top() not list_pop() here. */ while ((p = list_top(&ld->peers, struct peer, list)) != NULL) { struct channel *c; + /*~ A peer can have multiple channels; we only allow one to be + * open at any time, but we remember old ones for 100 blocks, + * after all the outputs we care about are spent. */ while ((c = list_top(&p->channels, struct channel, list)) != NULL) { /* Removes itself from list as we free it */ tal_free(c); } - /* Freeing uncommitted channel will free peer. */ + /* A peer may have a channel in the process of opening. */ if (p->uncommitted_channel) { struct uncommitted_channel *uc = p->uncommitted_channel; @@ -229,20 +370,37 @@ static void shutdown_subdaemons(struct lightningd *ld) /* Removes itself from list as we free it */ tal_free(p); } + + /*~ Commit the transaction. Note that the db is actually + * single-threaded, so commits never fail and we don't need + * spin-and-retry logic everywhere. */ db_commit_transaction(ld->wallet->db); } +/*~ Chainparams are the parameters for eg. testnet vs mainnet. This wrapper + * saves lots of struggles with our 80-column guideline! */ const struct chainparams *get_chainparams(const struct lightningd *ld) { + /* "The lightningd is connected to the chain topology." + * "The chain topology is connected to the bitcoind API." + * "The bitcoind API is connected chain parameters." + * -- Worst childhood song ever. */ return ld->topology->bitcoind->chainparams; } +/*~ Our wallet logic needs to know what outputs we might be interested in: we + * keep the maximum-ever-used key index in the db, and add them all to the + * filter here. */ static void init_txfilter(struct wallet *w, struct txfilter *filter) { + /*~ This is defined in libwally, so we didn't have to reimplement */ struct ext_key ext; + /*~ Note the use of ccan/short_types u64 rather than uint64_t. + * Thank me later. */ u64 bip32_max_index; bip32_max_index = db_get_intvar(w->db, "bip32_max_index", 0); + /*~ One of the C99 things I unequivocally approve: for-loop scope. */ for (u64 i = 0; i <= bip32_max_index; i++) { if (bip32_key_from_parent(w->bip32_base, i, BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) { abort(); @@ -251,26 +409,40 @@ static void init_txfilter(struct wallet *w, struct txfilter *filter) } } +/*~ The normal advice for daemons is to move into the root directory, so you + * don't prevent unmounting whatever filesystem you happen to start in. + * + * But we define every path relative to our (~/.lightning) data dir, so we + * make sure we stay there. + */ static void daemonize_but_keep_dir(struct lightningd *ld) { /* daemonize moves us into /, but we want to be here */ const char *cwd = path_cwd(NULL); + /*~ SQLite3 does NOT like being open across fork(), a.k.a. daemonize() */ db_close_for_fork(ld->wallet->db); if (!cwd) fatal("Could not get current directory: %s", strerror(errno)); if (!daemonize()) fatal("Could not become a daemon: %s", strerror(errno)); - /* Move back: important, since lightning dir may be relative! */ + /*~ Move back: important, since lightning dir may be relative! */ if (chdir(cwd) != 0) fatal("Could not return to directory %s: %s", cwd, strerror(errno)); db_reopen_after_fork(ld->wallet->db); + + /*~ Why not allocate cwd off tmpctx? Probably because this code predates + * tmpctx. So we free manually here. */ tal_free(cwd); } +/*~ It's pretty standard behaviour (especially for daemons) to create and + * file-lock a pidfile. This not only prevents accidentally running multiple + * daemons on the same database at once, but lets nosy sysadmins see what pid + * the currently-running daemon is supposed to be. */ static void pidfile_create(const struct lightningd *ld) { char *pid; @@ -281,19 +453,30 @@ static void pidfile_create(const struct lightningd *ld) if (pid_fd < 0) err(1, "Failed to open PID file"); - /* Lock PID file */ + /* Lock PID file: this will stay locked until we exit. */ if (lockf(pid_fd, F_TLOCK, 0) < 0) /* Problem locking file */ err(1, "lightningd already running? Error locking PID file"); - /* Get current PID and write to PID fie */ + /*~ Note that tal_fmt() is what asprintf() dreams of being. */ pid = tal_fmt(tmpctx, "%d\n", getpid()); + /*~ CCAN's write_all writes to a file descriptor, looping if necessary + * (which, on a file unlike a socket, is never, for historical UNIX + * reasons). It also isn't declared with GCC's warn_unused_result + * which write() is when FORTIFY_SOURCE is defined, so we're allowed + * to ignore the result without jumping through hoops. */ write_all(pid_fd, pid, strlen(pid)); /* Leave file open: we close it implicitly when we exit */ } -/* Yuck, we need globals here. */ +/*~ Yuck, we need a global here. + * + * ccan/io allows overriding the poll() function for special effects: for + * lightningd, we make sure we haven't left a db transaction open. All + * daemons which use ccan/io add sanity checks in this loop, so we chain + * that after our own override. + */ static int (*io_poll_debug)(struct pollfd *, nfds_t, int); static int io_poll_lightningd(struct pollfd *fds, nfds_t nfds, int timeout) { @@ -302,8 +485,12 @@ static int io_poll_lightningd(struct pollfd *fds, nfds_t nfds, int timeout) return io_poll_debug(fds, nfds, timeout); } -void notify_new_block(struct lightningd *ld, - u32 block_height) +/*~ Ever had one of those functions which doesn't quite fit anywhere? Me too. + * Implementing a generic notifier framework is overkill in a static codebase + * like this, and it's always better to have compile-time calls than runtime, + * as it makes the code more explicit. But pasting in direct calls is also an + * abstraction violation, so we use this middleman function. */ +void notify_new_block(struct lightningd *ld, u32 block_height) { /* Inform our subcomponents individually. */ htlcs_notify_new_block(ld, block_height); @@ -316,8 +503,18 @@ int main(int argc, char *argv[]) u32 min_blockheight, max_blockheight; int connectd_gossipd_fd; + /*~ What happens in strange locales should stay there. */ setup_locale(); + /*~ Every daemon calls this in some form: the hooks are for dumping + * backtraces when we crash (if supported on this platform). */ daemon_setup(argv[0], log_backtrace_print, log_backtrace_exit); + + /*~ There's always a battle between what a constructor like this + * should do, and what should be added later by the caller. In + * general, because we use valgrind heavily for testing, we prefer not + * to intialize unused fields which we expect the caller to set: + * valgrind will warn us if we make decisions based on uninitialized + * variables. */ ld = new_lightningd(NULL); /* Figure out where our daemons are first. */ @@ -325,56 +522,75 @@ int main(int argc, char *argv[]) if (!ld->daemon_dir) errx(1, "Could not find daemons"); + /*~ The ccan/opt code requires registration then parsing; we + * mimic this API here, even though they're on separate lines.*/ register_opts(ld); - /* Handle options and config; move to .lightningd */ + /*~ Handle options and config; move to .lightningd (--lightning-dir) */ handle_opts(ld, argc, argv); - /* Make sure we can reach other daemons, and versions match. */ + /*~ Make sure we can reach the subdaemons, and versions match. */ test_subdaemons(ld); - /* Initialize wallet, now that we are in the correct directory */ + /*~ Our "wallet" code really wraps the db, which is more than a simple + * bitcoin wallet (though it's that too). */ ld->wallet = wallet_new(ld, ld->log, &ld->timers); + + /*~ We keep a filter of scriptpubkeys we're interested in. */ ld->owned_txfilter = txfilter_new(ld); - /* We do extra checks in io_loop. */ + /*~ This is the ccan/io central poll override from above. */ io_poll_debug = io_poll_override(io_poll_lightningd); - /* Set up HSM. */ + /*~ Set up HSM: it knows our node secret key, so tells us who we are. */ hsm_init(ld); - /* Now we know our ID, we can set our color/alias if not already. */ + /*~ Our default color and alias are derived from our node id, so we + * can only set those now (if not set by config options). */ setup_color_and_alias(ld); - /* Set up connect daemon. */ + /*~ Set up connect daemon: this manages receiving and making + * TCP connections. It needs to talk to the gossip daemon + * which knows (via node_announcement messages) the public + * addresses of nodes, so connectd_init hands it one end of a + * socket pair, and gives us the other */ connectd_gossipd_fd = connectd_init(ld); - /* Set up gossip daemon. */ + /*~ The gossip daemon looks after the routing gossip; + * channel_announcement, channel_update, node_announcement and gossip + * queries. */ gossip_init(ld, connectd_gossipd_fd); - /* Everything is within a transaction. */ + /*~ We do every database operation within a transaction; usually this + * is covered by the infrastructure (eg. opening a transaction before + * handling a message or expiring a timer), but for startup we do this + * explicitly. */ db_begin_transaction(ld->wallet->db); + /*~ Our default names, eg. for the database file, are not dependent on + * the network. Instead, the db knows what chain it belongs to, and we + * simple barf here if it's wrong. */ if (!wallet_network_check(ld->wallet, get_chainparams(ld))) errx(1, "Wallet network check failed."); - /* Initialize the transaction filter with our pubkeys. */ + /*~ Initialize the transaction filter with our pubkeys. */ init_txfilter(ld->wallet, ld->owned_txfilter); - /* Set up invoice autoclean. */ + /*~ Set up invoice autoclean. */ wallet_invoice_autoclean(ld->wallet, ld->ini_autocleaninvoice_cycle, ld->ini_autocleaninvoice_expiredby); - /* Pull peers, channels and HTLCs from db. */ + /*~ Pull peers, channels and HTLCs from db. */ load_channels_from_wallet(ld); - /* Get the blockheight we are currently at, UINT32_MAX is used to signal + /*~ Get the blockheight we are currently at, UINT32_MAX is used to signal * an unitialized wallet and that we should start off of bitcoind's * current height */ - wallet_blocks_heights(ld->wallet, UINT32_MAX, &min_blockheight, &max_blockheight); + wallet_blocks_heights(ld->wallet, UINT32_MAX, + &min_blockheight, &max_blockheight); - /* If we were asked to rescan from an absolute height (--rescan < 0) + /*~ If we were asked to rescan from an absolute height (--rescan < 0) * then just go there. Otherwise compute the diff to our current height, * lowerbounded by 0. */ if (ld->config.rescan < 0) @@ -384,51 +600,80 @@ int main(int argc, char *argv[]) else if (max_blockheight != UINT32_MAX) max_blockheight -= ld->config.rescan; + /*~ That's all of the wallet db operations for now. */ db_commit_transaction(ld->wallet->db); - /* Initialize block topology (does its own transaction) */ - setup_topology(ld->topology, &ld->timers, min_blockheight, max_blockheight); + /*~ Initialize block topology. This does its own io_loop to + * talk to bitcoind, so does its own db transactions. */ + setup_topology(ld->topology, &ld->timers, + min_blockheight, max_blockheight); - /* Create RPC socket (if any) */ + /*~ Create RPC socket (if any): now we can talk to clients. */ setup_jsonrpc(ld, ld->rpc_filename); - /* Now we're about to start, become daemon if desired. */ + /*~ We defer --daemon until we've completed most initialization: that + * way we'll exit with an error rather than silently exiting 0, then + * realizing we can't start and forcing the confused user to read the + * logs. */ if (ld->daemon) daemonize_but_keep_dir(ld); - /* Create PID file */ + /*~ Now create the PID file: this has to be after daemonize, since that + * changes our pid! */ pidfile_create(ld); - /* Activate connect daemon. Needs to be after the initialization of - * chaintopology, otherwise we may be asking for uninitialized data. */ + /*~ Activate connect daemon. Needs to be after the initialization of + * chaintopology, otherwise peers may connect and ask for + * uninitialized data. */ connectd_activate(ld); - /* Replay transactions for all running onchainds */ + /*~ "onchaind" is a dumb daemon which tries to get our funds back: it + * doesn't handle reorganizations, but it's idempotent, so we can + * simply just restart it if the chain moves. Similarly, we replay it + * chain events from the database on restart, beginning with the + * "funding transaction spent" event which creates it. */ onchaind_replay_channels(ld); - /* Mark ourselves live. */ + /*~ Mark ourselves live. + * + * Note the use of type_to_string() here: it's a typesafe formatter, + * often handed 'tmpctx' like here to allocate a throwaway string for + * formatting. json_escape() avoids printing weird characters in our + * log. And tal_hex() is a helper from utils which returns a hex string; + * it's assumed that the argument was allocated with tal or tal_arr + * so it can use tal_bytelen() to get the length. */ log_info(ld->log, "Server started with public key %s, alias %s (color #%s) and lightningd %s", type_to_string(tmpctx, struct pubkey, &ld->id), json_escape(tmpctx, (const char *)ld->alias)->s, tal_hex(tmpctx, ld->rgb), version()); - /* Start the peers. */ + /*~ This is where we ask connectd to reconnect to any peers who have + * live channels with us, and makes sure we're watching the funding + * tx. */ activate_peers(ld); - /* Now kick off topology update, now peers have watches. */ + /*~ Now that all the notifications for transactions are in place, we + * can start the poll loop which queries bitcoind for new blocks. */ begin_topology(ld->topology); - /* Activate crash log now we're not reporting startup failures. */ + /*~ Setting this (global) activates the crash log: we don't usually need + * a backtrace if we fail during startup. */ crashlog = ld->log; + /*~ The root of every backtrace (almost). */ for (;;) { + /* ~io_loop returns if there's an expired timer, *or* someone + * calls io_break, or if there are no more IO connections + * (which never happens in our code). */ struct timer *expired; void *v = io_loop(&ld->timers, &expired); - /* We use io_break(dstate) to shut down. */ + /*~ We use io_break(ld) to shut down. */ if (v == ld) break; + /*~ Notice that timers are called here in the event loop like + * anything else, so there are no weird concurrency issues. */ if (expired) { db_begin_transaction(ld->wallet->db); timer_expired(ld, expired); @@ -457,5 +702,7 @@ int main(int argc, char *argv[]) memleak_cleanup(); #endif daemon_shutdown(); + + /*~ Farewell. Next stop: hsmd/hsm.c. */ return 0; } From 0d46a3d6b07be472f04463b61f62d674170c3f1a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 13:10:00 +0930 Subject: [PATCH 26/39] Put the 'd' back in the daemons. @renepickhardt: why is it actually lightningd.c with a d but hsm.c without d ? And delete unused gossipd/gossip.h. Signed-off-by: Rusty Russell --- channeld/Makefile | 2 +- channeld/{channel.c => channeld.c} | 0 closingd/Makefile | 2 +- closingd/{closing.c => closingd.c} | 0 connectd/Makefile | 2 +- connectd/{connect.c => connectd.c} | 2 +- connectd/{connect.h => connectd.h} | 6 +++--- connectd/tor.c | 2 +- gossipd/Makefile | 3 +-- gossipd/gossip.h | 10 ---------- gossipd/{gossip.c => gossipd.c} | 1 - hsmd/Makefile | 2 +- hsmd/{hsm.c => hsmd.c} | 0 onchaind/Makefile | 2 +- onchaind/{onchain.c => onchaind.c} | 0 onchaind/test/run-grind_feerate.c | 2 +- openingd/Makefile | 2 +- openingd/{opening.c => openingd.c} | 0 18 files changed, 13 insertions(+), 25 deletions(-) rename channeld/{channel.c => channeld.c} (100%) rename closingd/{closing.c => closingd.c} (100%) rename connectd/{connect.c => connectd.c} (99%) rename connectd/{connect.h => connectd.h} (52%) delete mode 100644 gossipd/gossip.h rename gossipd/{gossip.c => gossipd.c} (99%) rename hsmd/{hsm.c => hsmd.c} (100%) rename onchaind/{onchain.c => onchaind.c} (100%) rename openingd/{opening.c => openingd.c} (100%) diff --git a/channeld/Makefile b/channeld/Makefile index 9c281d005339..1c15d2ef8040 100644 --- a/channeld/Makefile +++ b/channeld/Makefile @@ -20,7 +20,7 @@ LIGHTNINGD_CHANNEL_HEADERS_NOGEN := \ LIGHTNINGD_CHANNEL_HEADERS := $(LIGHTNINGD_CHANNEL_HEADERS_GEN) $(LIGHTNINGD_CHANNEL_HEADERS_NOGEN) -LIGHTNINGD_CHANNEL_SRC := channeld/channel.c \ +LIGHTNINGD_CHANNEL_SRC := channeld/channeld.c \ channeld/commit_tx.c \ channeld/full_channel.c \ channeld/gen_channel_wire.c diff --git a/channeld/channel.c b/channeld/channeld.c similarity index 100% rename from channeld/channel.c rename to channeld/channeld.c diff --git a/closingd/Makefile b/closingd/Makefile index 64cc6a16ad20..0eedf7af8ade 100644 --- a/closingd/Makefile +++ b/closingd/Makefile @@ -16,7 +16,7 @@ LIGHTNINGD_CLOSING_HEADERS_NOGEN := LIGHTNINGD_CLOSING_HEADERS := $(LIGHTNINGD_CLOSING_HEADERS_GEN) $(LIGHTNINGD_CLOSING_HEADERS_NOGEN) -LIGHTNINGD_CLOSING_SRC := closingd/closing.c \ +LIGHTNINGD_CLOSING_SRC := closingd/closingd.c \ $(LIGHTNINGD_CLOSING_HEADERS:.h=.c) LIGHTNINGD_CLOSING_OBJS := $(LIGHTNINGD_CLOSING_SRC:.c=.o) diff --git a/closingd/closing.c b/closingd/closingd.c similarity index 100% rename from closingd/closing.c rename to closingd/closingd.c diff --git a/connectd/Makefile b/connectd/Makefile index c12719d3db5c..f32dc7bce597 100644 --- a/connectd/Makefile +++ b/connectd/Makefile @@ -14,7 +14,7 @@ LIGHTNINGD_CONNECT_CONTROL_OBJS := $(LIGHTNINGD_CONNECT_CONTROL_SRC:.c=.o) # connectd needs these: LIGHTNINGD_CONNECT_HEADERS := connectd/gen_connect_wire.h \ connectd/gen_connect_gossip_wire.h \ - connectd/connect.h \ + connectd/connectd.h \ connectd/handshake.h \ connectd/netaddress.h \ connectd/tor_autoservice.h \ diff --git a/connectd/connect.c b/connectd/connectd.c similarity index 99% rename from connectd/connect.c rename to connectd/connectd.c index b78e2ecf2249..192dae73a17a 100644 --- a/connectd/connect.c +++ b/connectd/connectd.c @@ -30,7 +30,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/connectd/connect.h b/connectd/connectd.h similarity index 52% rename from connectd/connect.h rename to connectd/connectd.h index 6664c89be252..2fd6feae3fd9 100644 --- a/connectd/connect.h +++ b/connectd/connectd.h @@ -1,5 +1,5 @@ -#ifndef LIGHTNING_CONNECTD_CONNECT_H -#define LIGHTNING_CONNECTD_CONNECT_H +#ifndef LIGHTNING_CONNECTD_CONNECTD_H +#define LIGHTNING_CONNECTD_CONNECTD_H #include "config.h" struct io_conn; @@ -7,4 +7,4 @@ struct reaching; struct io_plan *connection_out(struct io_conn *conn, struct reaching *reach); -#endif /* LIGHTNING_CONNECTD_CONNECT_H */ +#endif /* LIGHTNING_CONNECTD_CONNECTD_H */ diff --git a/connectd/tor.c b/connectd/tor.c index 88611298abef..a2a175765bdd 100644 --- a/connectd/tor.c +++ b/connectd/tor.c @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/gossipd/Makefile b/gossipd/Makefile index 57f5c38be3d0..264fe33aee93 100644 --- a/gossipd/Makefile +++ b/gossipd/Makefile @@ -13,12 +13,11 @@ LIGHTNINGD_GOSSIP_CONTROL_OBJS := $(LIGHTNINGD_GOSSIP_CONTROL_SRC:.c=.o) # gossipd needs these: LIGHTNINGD_GOSSIP_HEADERS := gossipd/gen_gossip_wire.h \ - gossipd/gossip.h \ gossipd/gen_gossip_store.h \ gossipd/gossip_store.h \ gossipd/routing.h \ gossipd/broadcast.h -LIGHTNINGD_GOSSIP_SRC := $(LIGHTNINGD_GOSSIP_HEADERS:.h=.c) +LIGHTNINGD_GOSSIP_SRC := $(LIGHTNINGD_GOSSIP_HEADERS:.h=.c) gossipd/gossipd.c LIGHTNINGD_GOSSIP_OBJS := $(LIGHTNINGD_GOSSIP_SRC:.c=.o) # Make sure these depend on everything. diff --git a/gossipd/gossip.h b/gossipd/gossip.h deleted file mode 100644 index 70c8377bf2b9..000000000000 --- a/gossipd/gossip.h +++ /dev/null @@ -1,10 +0,0 @@ -#ifndef LIGHTNING_GOSSIPD_GOSSIP_H -#define LIGHTNING_GOSSIPD_GOSSIP_H -#include "config.h" - -struct io_conn; -struct reaching; - -struct io_plan *connection_out(struct io_conn *conn, struct reaching *reach); - -#endif /* LIGHTNING_GOSSIPD_GOSSIP_H */ diff --git a/gossipd/gossip.c b/gossipd/gossipd.c similarity index 99% rename from gossipd/gossip.c rename to gossipd/gossipd.c index 993f4acb0b6f..e2dcfcad0a1b 100644 --- a/gossipd/gossip.c +++ b/gossipd/gossipd.c @@ -35,7 +35,6 @@ #include #include #include -#include #include #include #include diff --git a/hsmd/Makefile b/hsmd/Makefile index 6eb03aa770f0..5147f21f3dc4 100644 --- a/hsmd/Makefile +++ b/hsmd/Makefile @@ -14,7 +14,7 @@ LIGHTNINGD_HSM_CLIENT_OBJS := $(LIGHTNINGD_HSM_CLIENT_SRC:.c=.o) # lightningd/hsm needs these: LIGHTNINGD_HSM_HEADERS := hsmd/gen_hsm_client_wire.h -LIGHTNINGD_HSM_SRC := hsmd/hsm.c \ +LIGHTNINGD_HSM_SRC := hsmd/hsmd.c \ $(LIGHTNINGD_HSM_HEADERS:.h=.c) LIGHTNINGD_HSM_OBJS := $(LIGHTNINGD_HSM_SRC:.c=.o) diff --git a/hsmd/hsm.c b/hsmd/hsmd.c similarity index 100% rename from hsmd/hsm.c rename to hsmd/hsmd.c diff --git a/onchaind/Makefile b/onchaind/Makefile index 7a663e479614..1ea8b7bbcaf5 100644 --- a/onchaind/Makefile +++ b/onchaind/Makefile @@ -22,7 +22,7 @@ LIGHTNINGD_ONCHAIN_HEADERS_NOGEN := \ LIGHTNINGD_ONCHAIN_HEADERS := $(LIGHTNINGD_ONCHAIN_HEADERS_GEN) $(LIGHTNINGD_ONCHAIN_HEADERS_NOGEN) -LIGHTNINGD_ONCHAIN_SRC := onchaind/onchain.c \ +LIGHTNINGD_ONCHAIN_SRC := onchaind/onchaind.c \ onchaind/gen_onchain_wire.c \ onchaind/onchain_wire.c diff --git a/onchaind/onchain.c b/onchaind/onchaind.c similarity index 100% rename from onchaind/onchain.c rename to onchaind/onchaind.c diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 49072ffd8fdc..988d9801e16b 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -7,7 +7,7 @@ #define main unused_main int main(int argc, char *argv[]); -#include "../onchain.c" +#include "../onchaind.c" #undef main /* AUTOGENERATED MOCKS START */ diff --git a/openingd/Makefile b/openingd/Makefile index c834cea41012..92fb90a0ac67 100644 --- a/openingd/Makefile +++ b/openingd/Makefile @@ -16,7 +16,7 @@ LIGHTNINGD_OPENING_HEADERS_NOGEN := LIGHTNINGD_OPENING_HEADERS := $(LIGHTNINGD_OPENING_HEADERS_GEN) $(LIGHTNINGD_OPENING_HEADERS_NOGEN) -LIGHTNINGD_OPENING_SRC := openingd/opening.c \ +LIGHTNINGD_OPENING_SRC := openingd/openingd.c \ $(LIGHTNINGD_OPENING_HEADERS:.h=.c) LIGHTNINGD_OPENING_OBJS := $(LIGHTNINGD_OPENING_SRC:.c=.o) diff --git a/openingd/opening.c b/openingd/openingd.c similarity index 100% rename from openingd/opening.c rename to openingd/openingd.c From 76f116daf1650070ab6f3ff32ffbb9b68590b7ac Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 13:10:00 +0930 Subject: [PATCH 27/39] lightningd: minor cleanups Code changes: 1. Expose daemon_poll() so lightningd can call it directly, which avoids us having store a global and document it. 2. Remove the (undocumented, unused, forgotten) --rpc-file="" option to disable JSON RPC. 3. Move the ickiness of finding the executable path into subd.c, so it doesn't distract from lightningd.c overview. Signed-off-by: Rusty Russell --- common/daemon.c | 3 +- common/daemon.h | 4 + lightningd/jsonrpc.c | 3 - lightningd/lightningd.c | 82 +++++-------------- lightningd/subd.c | 50 +++++++++++ lightningd/subd.h | 3 + ...n-find_my_path.c => run-find_my_abspath.c} | 51 +++++++----- 7 files changed, 109 insertions(+), 87 deletions(-) rename lightningd/test/{run-find_my_path.c => run-find_my_abspath.c} (80%) diff --git a/common/daemon.c b/common/daemon.c index c181545ab156..f55e84510682 100644 --- a/common/daemon.c +++ b/common/daemon.c @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -66,7 +65,7 @@ static void crashlog_activate(void) } #endif -static int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout) +int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout) { const char *t; diff --git a/common/daemon.h b/common/daemon.h index c3ae0991528d..b211e37772db 100644 --- a/common/daemon.h +++ b/common/daemon.h @@ -1,12 +1,16 @@ #ifndef LIGHTNING_COMMON_DAEMON_H #define LIGHTNING_COMMON_DAEMON_H #include "config.h" +#include /* Common setup for all daemons */ void daemon_setup(const char *argv0, void (*backtrace_print)(const char *fmt, ...), void (*backtrace_exit)(void)); +/* Exposed for lightningd's use. */ +int daemon_poll(struct pollfd *fds, nfds_t nfds, int timeout); + /* Shutdown for a valgrind-clean exit (frees everything) */ void daemon_shutdown(void); diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 152c2a518421..558e581bd13b 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -600,9 +600,6 @@ void setup_jsonrpc(struct lightningd *ld, const char *rpc_filename) struct sockaddr_un addr; int fd, old_umask; - if (streq(rpc_filename, "")) - return; - if (streq(rpc_filename, "/dev/tty")) { fd = open(rpc_filename, O_RDWR); if (fd == -1) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index f9c7c7110837..bd120187902e 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -232,62 +232,24 @@ static bool has_all_subdaemons(const char* daemon_dir) return !missing_daemon; } -/* This routine tries to determine what path the lightningd binary is in. - * It's not actually that simple! */ -static const char *find_my_path(const tal_t *ctx, const char *argv0) +/* Returns the directory this executable is running from */ +static const char *find_my_directory(const tal_t *ctx, const char *argv0) { - char *me; - - /* A command containing / is run relative to the current directory, - * not searched through the path. The shell sets argv0 to the command - * run, though something else could set it to a arbitrary value and - * this logic would be wrong. */ - if (strchr(argv0, PATH_SEP)) { - const char *path; - /* Absolute paths are easy. */ - if (strstarts(argv0, PATH_SEP_STR)) - path = argv0; - /* It contains a '/', it's relative to current dir. */ - else - path = path_join(tmpctx, path_cwd(tmpctx), argv0); - - me = path_canon(ctx, path); - if (!me || access(me, X_OK) != 0) - errx(1, "I cannot find myself at %s based on my name %s", - path, argv0); - } else { - /* No /, search path */ - char **pathdirs; - const char *pathenv = getenv("PATH"); - size_t i; - - /* This replicates the standard shell path search algorithm */ - if (!pathenv) - errx(1, "Cannot find myself: no $PATH set"); - - pathdirs = tal_strsplit(tmpctx, pathenv, ":", STR_NO_EMPTY); - me = NULL; - for (i = 0; pathdirs[i]; i++) { - /* This returns NULL if it doesn't exist. */ - me = path_canon(ctx, - path_join(tmpctx, pathdirs[i], argv0)); - if (me && access(me, X_OK) == 0) - break; - /* Nope, try again. */ - me = tal_free(me); - } - if (!me) - errx(1, "Cannot find %s in $PATH", argv0); - } + /* find_my_abspath simply exits on failure, so never returns NULL. */ + const char *me = find_my_abspath(NULL, argv0); /*~ The caller just wants the directory we're in. * - * Note the magic "take()" macro here: it annotates a pointer as "to + * Note the magic `take()` macro here: it annotates a pointer as "to * be taken", and the recipient is expected to take ownership of the - * pointer. + * pointer. This improves efficiency because the recipient might + * choose to use or even keep it rather than make a copy (or it + * might just free it). * - * Many CCAN and our own routines support this, but if you hand a take() - * to a non-take routine unfortunately you don't get a compile error. + * Many CCAN and our own routines support this, but if you hand a + * `take()` to a routine which *doesn't* expect it, unfortunately you + * don't get a compile error (we have runtime detection for this + * case, however). */ return path_dirname(ctx, take(me)); } @@ -310,7 +272,7 @@ static const char *find_my_pkglibexec_path(const tal_t *ctx, /* Determine the correct daemon dir. */ static const char *find_daemon_dir(const tal_t *ctx, const char *argv0) { - const char *my_path = find_my_path(ctx, argv0); + const char *my_path = find_my_directory(ctx, argv0); /* If we're running in-tree, all the subdaemons are with lightningd. */ if (has_all_subdaemons(my_path)) return my_path; @@ -470,19 +432,17 @@ static void pidfile_create(const struct lightningd *ld) /* Leave file open: we close it implicitly when we exit */ } -/*~ Yuck, we need a global here. - * - * ccan/io allows overriding the poll() function for special effects: for - * lightningd, we make sure we haven't left a db transaction open. All - * daemons which use ccan/io add sanity checks in this loop, so we chain - * that after our own override. - */ -static int (*io_poll_debug)(struct pollfd *, nfds_t, int); +/*~ ccan/io allows overriding the poll() function that is the very core + * of the event loop it runs for us. We override it so that we can do + * extra sanity checks, and it's also a good point to free the tmpctx. */ static int io_poll_lightningd(struct pollfd *fds, nfds_t nfds, int timeout) { + /*~ In particular, we should *not* have left a database transaction + * open! */ db_assert_no_outstanding_statements(); - return io_poll_debug(fds, nfds, timeout); + /* The other checks and freeing tmpctx are common to all daemons. */ + return daemon_poll(fds, nfds, timeout); } /*~ Ever had one of those functions which doesn't quite fit anywhere? Me too. @@ -540,7 +500,7 @@ int main(int argc, char *argv[]) ld->owned_txfilter = txfilter_new(ld); /*~ This is the ccan/io central poll override from above. */ - io_poll_debug = io_poll_override(io_poll_lightningd); + io_poll_override(io_poll_lightningd); /*~ Set up HSM: it knows our node secret key, so tells us who we are. */ hsm_init(ld); diff --git a/lightningd/subd.c b/lightningd/subd.c index 4f7f791f6a62..589f1a412461 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -822,3 +822,53 @@ bool dev_disconnect_permanent(struct lightningd *ld) return false; } #endif /* DEVELOPER */ + +/* Ugly helper to get full pathname of the current binary. */ +const char *find_my_abspath(const tal_t *ctx, const char *argv0) +{ + char *me; + + /* A command containing / is run relative to the current directory, + * not searched through the path. The shell sets argv0 to the command + * run, though something else could set it to a arbitrary value and + * this logic would be wrong. */ + if (strchr(argv0, PATH_SEP)) { + const char *path; + /* Absolute paths are easy. */ + if (strstarts(argv0, PATH_SEP_STR)) + path = argv0; + /* It contains a '/', it's relative to current dir. */ + else + path = path_join(tmpctx, path_cwd(tmpctx), argv0); + + me = path_canon(ctx, path); + if (!me || access(me, X_OK) != 0) + errx(1, "I cannot find myself at %s based on my name %s", + path, argv0); + } else { + /* No /, search path */ + char **pathdirs; + const char *pathenv = getenv("PATH"); + size_t i; + + /* This replicates the standard shell path search algorithm */ + if (!pathenv) + errx(1, "Cannot find myself: no $PATH set"); + + pathdirs = tal_strsplit(tmpctx, pathenv, ":", STR_NO_EMPTY); + me = NULL; + for (i = 0; pathdirs[i]; i++) { + /* This returns NULL if it doesn't exist. */ + me = path_canon(ctx, + path_join(tmpctx, pathdirs[i], argv0)); + if (me && access(me, X_OK) == 0) + break; + /* Nope, try again. */ + me = tal_free(me); + } + if (!me) + errx(1, "Cannot find %s in $PATH", argv0); + } + + return me; +} diff --git a/lightningd/subd.h b/lightningd/subd.h index 4dacd59a3e11..72e929eab20a 100644 --- a/lightningd/subd.h +++ b/lightningd/subd.h @@ -202,6 +202,9 @@ void subd_release_channel(struct subd *owner, void *channel); */ void subd_shutdown(struct subd *subd, unsigned int seconds); +/* Ugly helper to get full pathname of the current binary. */ +const char *find_my_abspath(const tal_t *ctx, const char *argv0); + #if DEVELOPER char *opt_subd_debug(const char *optarg, struct lightningd *ld); char *opt_subd_dev_disconnect(const char *optarg, struct lightningd *ld); diff --git a/lightningd/test/run-find_my_path.c b/lightningd/test/run-find_my_abspath.c similarity index 80% rename from lightningd/test/run-find_my_path.c rename to lightningd/test/run-find_my_abspath.c index 672223ef014b..c13f9fd21bc5 100644 --- a/lightningd/test/run-find_my_path.c +++ b/lightningd/test/run-find_my_abspath.c @@ -3,6 +3,7 @@ int unused_main(int argc, char *argv[]); #include "../../common/base32.c" #include "../../common/wireaddr.c" #include "../lightningd.c" +#include "../subd.c" /* AUTOGENERATED MOCKS START */ /* Generated stub for activate_peers */ @@ -21,6 +22,9 @@ void connectd_activate(struct lightningd *ld UNNEEDED) /* Generated stub for connectd_init */ int connectd_init(struct lightningd *ld UNNEEDED) { fprintf(stderr, "connectd_init called!\n"); abort(); } +/* Generated stub for daemon_poll */ +int daemon_poll(struct pollfd *fds UNNEEDED, nfds_t nfds UNNEEDED, int timeout UNNEEDED) +{ fprintf(stderr, "daemon_poll called!\n"); abort(); } /* Generated stub for daemon_setup */ void daemon_setup(const char *argv0 UNNEEDED, void (*backtrace_print)(const char *fmt UNNEEDED, ...) UNNEEDED, @@ -53,6 +57,18 @@ void fatal(const char *fmt UNNEEDED, ...) /* Generated stub for free_htlcs */ void free_htlcs(struct lightningd *ld UNNEEDED, const struct channel *channel UNNEEDED) { fprintf(stderr, "free_htlcs called!\n"); abort(); } +/* Generated stub for fromwire_status_fail */ +bool fromwire_status_fail(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, enum status_failreason *failreason UNNEEDED, wirestring **desc UNNEEDED) +{ fprintf(stderr, "fromwire_status_fail called!\n"); abort(); } +/* Generated stub for fromwire_status_peer_billboard */ +bool fromwire_status_peer_billboard(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, bool *perm UNNEEDED, wirestring **happenings UNNEEDED) +{ fprintf(stderr, "fromwire_status_peer_billboard called!\n"); abort(); } +/* Generated stub for fromwire_status_peer_error */ +bool fromwire_status_peer_error(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct channel_id *channel UNNEEDED, wirestring **desc UNNEEDED, struct crypto_state *crypto_state UNNEEDED, u8 **error_for_them UNNEEDED) +{ fprintf(stderr, "fromwire_status_peer_error called!\n"); abort(); } +/* Generated stub for get_log_book */ +struct log_book *get_log_book(const struct log *log UNNEEDED) +{ fprintf(stderr, "get_log_book called!\n"); abort(); } /* Generated stub for gossip_init */ void gossip_init(struct lightningd *ld UNNEEDED, int connectd_fd UNNEEDED) { fprintf(stderr, "gossip_init called!\n"); abort(); } @@ -84,6 +100,12 @@ void log_backtrace_exit(void) /* Generated stub for log_backtrace_print */ void log_backtrace_print(const char *fmt UNNEEDED, ...) { fprintf(stderr, "log_backtrace_print called!\n"); abort(); } +/* Generated stub for log_prefix */ +const char *log_prefix(const struct log *log UNNEEDED) +{ fprintf(stderr, "log_prefix called!\n"); abort(); } +/* Generated stub for log_status_msg */ +bool log_status_msg(struct log *log UNNEEDED, const u8 *msg UNNEEDED) +{ fprintf(stderr, "log_status_msg called!\n"); abort(); } /* Generated stub for new_log */ struct log *new_log(const tal_t *ctx UNNEEDED, struct log_book *record UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "new_log called!\n"); abort(); } @@ -110,9 +132,6 @@ void setup_jsonrpc(struct lightningd *ld UNNEEDED, const char *rpc_filename UNNE void setup_topology(struct chain_topology *topology UNNEEDED, struct timers *timers UNNEEDED, u32 min_blockheight UNNEEDED, u32 max_blockheight UNNEEDED) { fprintf(stderr, "setup_topology called!\n"); abort(); } -/* Generated stub for subd_shutdown */ -void subd_shutdown(struct subd *subd UNNEEDED, unsigned int seconds UNNEEDED) -{ fprintf(stderr, "subd_shutdown called!\n"); abort(); } /* Generated stub for timer_expired */ void timer_expired(tal_t *ctx UNNEEDED, struct timer *timer UNNEEDED) { fprintf(stderr, "timer_expired called!\n"); abort(); } @@ -144,16 +163,6 @@ struct wallet *wallet_new(struct lightningd *ld UNNEEDED, { fprintf(stderr, "wallet_new called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ -/* We only need these in developer mode */ -#if DEVELOPER -/* Generated stub for opt_subd_debug */ -char *opt_subd_debug(const char *optarg UNNEEDED, struct lightningd *ld UNNEEDED) -{ fprintf(stderr, "opt_subd_debug called!\n"); abort(); } -/* Generated stub for opt_subd_dev_disconnect */ -char *opt_subd_dev_disconnect(const char *optarg UNNEEDED, struct lightningd *ld UNNEEDED) -{ fprintf(stderr, "opt_subd_dev_disconnect called!\n"); abort(); } -#endif - struct log *crashlog; #undef main @@ -166,26 +175,26 @@ int main(int argc UNUSED, char *argv[] UNUSED) const char *answer; setup_tmpctx(); - answer = path_canon(tmpctx, "lightningd/test"); + answer = path_canon(tmpctx, "lightningd/test/run-find_my_abspath"); /* Various different ways we could find ourselves. */ argv0 = path_join(tmpctx, - path_cwd(tmpctx), "lightningd/test/run-find_my_path"); + path_cwd(tmpctx), "lightningd/test/run-find_my_abspath"); unsetenv("PATH"); /* Absolute path. */ - assert(streq(find_my_path(tmpctx, argv0), answer)); + assert(streq(find_my_abspath(tmpctx, argv0), answer)); /* Relative to cwd. */ - argv0 = "lightningd/test/run-find_my_path"; - assert(streq(find_my_path(tmpctx, argv0), answer)); + argv0 = "lightningd/test/run-find_my_abspath"; + assert(streq(find_my_abspath(tmpctx, argv0), answer)); /* Using $PATH */ setenv("PATH", path_join(tmpctx, path_cwd(tmpctx), "lightningd/test"), 1); - argv0 = "run-find_my_path"; + argv0 = "run-find_my_abspath"; - assert(streq(find_my_path(tmpctx, argv0), answer)); + assert(streq(find_my_abspath(tmpctx, argv0), answer)); /* Even with dummy things in path. */ char **pathelems = tal_arr(tmpctx, char *, 4); @@ -195,7 +204,7 @@ int main(int argc UNUSED, char *argv[] UNUSED) pathelems[3] = NULL; setenv("PATH", tal_strjoin(tmpctx, pathelems, ":", STR_NO_TRAIL), 1); - assert(streq(find_my_path(tmpctx, argv0), answer)); + assert(streq(find_my_abspath(tmpctx, argv0), answer)); assert(!taken_any()); take_cleanup(); From dfc2a6b873d8408db460c4943f8633e0fc0bfa19 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 13:10:01 +0930 Subject: [PATCH 28/39] More documentation changes. Documentation changes: 1. Lots of extra detail suggested by @renepickhardt. 2. typo fixes from @practicalswift. 3. A section on 'const' usage. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 205 ++++++++++++++++++++++++++++++---------- 1 file changed, 153 insertions(+), 52 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index bd120187902e..d31a255e9d90 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -7,7 +7,8 @@ * The role of this daemon is to start the subdaemons, shuffle peers * between them, handle the JSON RPC requests, bitcoind, the database * and centralize logging. In theory, it doesn't trust the other - * daemons, though we expect hsmd to be responsive. + * daemons, though we expect `hsmd` (which holds secret keys) to be + * responsive. * * Comments beginning with a ~ (like this one!) are part of our shared * adventure through the source, so they're more meta than normal code @@ -15,7 +16,7 @@ */ /*~ Notice how includes are in ASCII order: this is actually enforced by - * the build system under 'make check-source'. It avoids merge conflicts + * the build system under `make check-source`. It avoids merge conflicts * and keeps things consistent. */ #include "gossip_control.h" #include "hsm_control.h" @@ -33,7 +34,11 @@ * It's another one of Rusty's projects, and we copy and paste it * automatically into the source tree here, so you should never edit * it. There's a Makefile target update-ccan to update it (and add modules - * if CCAN_NEW is specified). */ + * if CCAN_NEW is specified). + * + * The most used of these are `ccan/tal` and `ccan/take`, which we'll describe + * in detail below. + */ #include #include #include @@ -49,7 +54,8 @@ #include #include -/*~ This is common code: routines shared by one or more programs. */ +/*~ This is common code: routines shared by one or more executables + * (separate daemons, or the lightning-cli program). */ #include #include #include @@ -74,19 +80,30 @@ /*~ The core lightning object: it's passed everywhere, and is basically a * global variable. This new_xxx pattern is something we'll see often: - * it allocates and initializes a new structure, using *tal*, the heirarchitcal + * it allocates and initializes a new structure, using *tal*, the hierarchical * allocator. */ static struct lightningd *new_lightningd(const tal_t *ctx) { /*~ tal: each allocation is a child of an existing object (or NULL, * the top-level object). When an object is freed, all the objects - * 'tallocated' off it are also freed. In this case, freeing 'ctx' - * will free 'ld'. + * `tallocated` off it are also freed. We use it in place of malloc + * and free. * * It's incredibly useful for grouping object lifetimes, as we'll see. + * For example, a `struct bitcoin_tx` has a pointer to an array of + * `struct bitcoin_tx_input`; they are allocated off the `struct + * bitcoind_tx`, so freeing the `struct bitcoind_tx` frees them all. + * + * In this case, freeing `ctx` will free `ld`: */ struct lightningd *ld = tal(ctx, struct lightningd); + /*~ Style note: `ctx` is declared `const`, yet we can `tallocate` from + * it. Adding/removing children is not considered to change an + * object; nor, in fact, is freeing it with tal_free(). This allows + * us to use const more liberally: the style rule here is that you + * should use 'const' on pointers if you can. */ + /*~ Note that we generally EXPLICITLY #if-wrap DEVELOPER code. This * is a nod to keeping it minimal and explicit: we need this code for * testing, but its existence means we're not actually testing the @@ -99,8 +116,9 @@ static struct lightningd *new_lightningd(const tal_t *ctx) /*~ Behaving differently depending on environment variables is a hack, * *but* hacks are allowed for dev-mode stuff. In this case, there's - * a significant overhead to the memory leak detection stuff, and - * we can't use it under valgrind, so the test harness uses this var + * a significant overhead to the memory leak detection stuff, and we + * can't use it under valgrind (an awesome runtime memory usage + * detector for C and C++ programs), so the test harness uses this var * to disable it in that case. */ if (getenv("LIGHTNINGD_DEV_MEMLEAK")) memleak_init(); @@ -108,15 +126,39 @@ static struct lightningd *new_lightningd(const tal_t *ctx) /*~ These are CCAN lists: an embedded double-linked list. It's not * really typesafe, but relies on convention to access the contents. - * It's inspired by the closely-related Linux kernel list.h. */ + * It's inspired by the closely-related Linux kernel list.h. + * + * You declare them as a `struct list_head` (or use the LIST_HEAD() + * macro which doesn't work on dynamically-allocated objects like `ld` + * here). The item which will go into the list must declared a + * `struct list_node` for each list it can be in. + * + * The most common operations are list_head_init(), list_add(), + * list_del() and list_for_each(). + * + * This method of manually declaring the list hooks avoids dynamic + * allocations to put things into a list. */ list_head_init(&ld->peers); - /*~ These are hash tables of incoming and outgoing HTLCs (contracts) */ + /*~ These are hash tables of incoming and outgoing HTLCs (contracts), + * defined as `struct htlc_in` and `struct htlc_out`in htlc_end.h. + * The hash tables are declared ther using the very ugly + * HTABLE_DEFINE_TYPE macro. The key is the channel the HTLC is in + * and the 64-bit htlc-id which is unique for that channel and + * direction. That htlc-id is used in the inter-peer wire protocol, + * so it is the logical key. + * + * There aren't usually many HTLCs, so we could have just used a linked + * list attached to the channel structure itself, or even left them in + * the database rather than making an in-memory version. Obviously + * I was in a premature optimization mood when I wrote this: */ htlc_in_map_init(&ld->htlcs_in); htlc_out_map_init(&ld->htlcs_out); - /*~ We have a log-book infrastructure: we define a 20MB log book and - * point our log objects into it. */ + /*~ We have a two-level log-book infrastructure: we define a 20MB log + * book to hold all the entries (and trims as necessary), and multiple + * log objects which each can write into it, each with a unique + * prefix. */ ld->log_book = new_log_book(20*1024*1024, LOG_INFORM); /*~ Note the tal context arg (by convention, the first argument to any * allocation function): ld->log will be implicitly freed when ld @@ -136,9 +178,8 @@ static struct lightningd *new_lightningd(const tal_t *ctx) /*~ Tal also explicitly supports arrays: it stores the number of * elements, which can be accessed with tal_count() (or tal_bytelen() * for raw bytecount). It's common for simple arrays to use - * tal_resize(), which is a typesafe realloc function, but as all - * talocations need a parent, we start with an empty array rather than - * NULL. */ + * tal_resize() to expand, which does not work on NULL. So we start + * with an zero-length array. */ ld->proposed_wireaddr = tal_arr(ld, struct wireaddr_internal, 0); ld->proposed_listen_announce = tal_arr(ld, enum addr_listen_announce, 0); ld->portnum = DEFAULT_PORT; @@ -146,9 +187,11 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->autolisten = true; ld->reconnect = true; - /*~ This is from ccan/timer: a scalable timer system which has a - * fascinating implementation you should read if you have a spare - * few hours */ + /*~ This is from ccan/timer: it is efficient for the case where timers + * are deleted before expiry (as is common with timeouts) using an + * ingenious bucket system which more precisely sorts timers as they + * approach expiry. It's a fascinating implementation you should read + * if you have a spare few hours. */ timers_init(&ld->timers, time_mono()); /*~ This is detailed in chaintopology.c */ @@ -184,23 +227,50 @@ void test_subdaemons(const struct lightningd *ld) { size_t i; - /*~ CCAN's ARRAY_SIZE() should always be used on defined arrays: it will - * fail to build if the argument is actually a pointer, not an array! */ + /*~ CCAN's ARRAY_SIZE() should always be used on defined arrays like + * the subdaemons array above. You can calculate the number of + * elements it has using `sizeof(subdaemons)/sizeof(subdaemons[0])` + * but if `subdaemons` were refactored into a pointer (eg. to make + * it a dynamic array) that would erroneously evaluate to `1`. + * + * ARRAY_SIZE will cause a compiler error if the argument is actually + * a pointer, not an array. */ for (i = 0; i < ARRAY_SIZE(subdaemons); i++) { int outfd; - /*~ CCAN's path module uses tal, so wants a context to allocate - * from. We have a magic context 'tmpctx' which is freed in - * the event loop for transient allocations like this. */ + /*~ CCAN's path module uses tal, so wants a context to + * allocate from. We have a magic convenience context + * `tmpctx` for temporary allocations like this. + * + * Because all our daemons at their core are of form `while + * (!stopped) handle_events();` (an event loop pattern), we + * can free `tmpctx` in that top-level loop after each event + * is handled. + */ const char *dpath = path_join(tmpctx, ld->daemon_dir, subdaemons[i]); const char *verstring; - /*~ CCAN's pipecmd module is like popen for grownups. */ + /*~ CCAN's pipecmd module is like popen for grownups: it + * takes pointers to fill in stdout, stdin and stderr file + * descriptors if desired, and the remainder of arguments + * are the command and its argument. */ pid_t pid = pipecmd(&outfd, NULL, &outfd, dpath, "--version", NULL); - /*~ Our logging system: spam goes in at log_debug level */ + /*~ Our logging system: spam goes in at log_debug level, but + * logging is mainly added by developer necessity and removed + * by developer/user complaints . The only strong convention + * is that log_broken() is used for "should never happen". + * + * Note, however, that logging takes care to preserve the + * global `errno` which is set above. */ log_debug(ld->log, "testing %s", dpath); + + /*~ ccan/err is a wrapper around BSD's err.h, which defines + * the convenience functions err() (error with message + * followed by a string based on errno) and errx() (same, + * but no errno string). */ if (pid == -1) err(1, "Could not run %s", dpath); + /*~ CCAN's grab_file module contains a routine to read into a * tallocated buffer until EOF */ verstring = grab_fd(tmpctx, outfd); @@ -217,7 +287,7 @@ void test_subdaemons(const struct lightningd *ld) } /* Check if all subdaemons exist in specified directory. */ -static bool has_all_subdaemons(const char* daemon_dir) +static bool has_all_subdaemons(const char *daemon_dir) { size_t i; bool missing_daemon = false; @@ -255,13 +325,24 @@ static const char *find_my_directory(const tal_t *ctx, const char *argv0) } /*~ This returns the PKGLIBEXEC path which is where binaries get installed. - * Note the 'TAKES' annotation which is merely documentation that it will - * take ownership of 'my_path' if the caller hands take() there. + * Note the `TAKES` annotation which indicates that the `my_path` parameter + * can be take(); in which case, this function will handle freeing it. + * + * TAKES is only a convention unfortunately, and ignored by the compiler. */ static const char *find_my_pkglibexec_path(const tal_t *ctx, const char *my_path TAKES) { const char *pkglibexecdir; + + /*~`path_join` is declared in ccan/path/path.h as: + * + * char *path_join(const tal_t *ctx, + * const char *base TAKES, const char *a TAKES); + * + * So, as we promised with 'TAKES' in our own declaration, if the + * caller has called `take()` the `my_path` parameter, path_join() + * will free it. */ pkglibexecdir = path_join(ctx, my_path, BINTOPKGLIBEXECDIR); /*~ Sometimes take() can be more efficient, since the routine can @@ -288,16 +369,20 @@ static void shutdown_subdaemons(struct lightningd *ld) { struct peer *p; - /*~ Because tal objects can be free indirectly, by freeing their parents - * it turns out to be vital to be able to add *destructors* to objects. - * As a result, freeing them may cause callbacks; in this case, some - * objects freed here can cause database writes, which must be inside - * a transaction */ + /*~ tal supports *destructors* using `tal_add_destructor()`; the most + * common use is for an object to delete itself from a linked list + * when it's freed. + * + * As a result, freeing an object (which frees any tal objects + * allocated off it, and any allocated off them, etc) may cause + * callbacks; in this case, some objects freed here can cause database + * writes, which must be inside a transaction. */ db_begin_transaction(ld->wallet->db); /* Let everyone shutdown cleanly. */ close(ld->hsm_fd); - /*~ The three "global" daemons, which we shutdown explicitly. */ + /*~ The three "global" daemons, which we shutdown explicitly: we + * give them 10 seconds to exit gracefully before killing them. */ subd_shutdown(ld->connectd, 10); subd_shutdown(ld->gossip, 10); subd_shutdown(ld->hsm, 10); @@ -305,9 +390,9 @@ static void shutdown_subdaemons(struct lightningd *ld) /* Now we free all the HTLCs */ free_htlcs(ld, NULL); - /*~ For every peer, we free every channel. Note that the peer has a - * destructor (by convention, called destroy_peer) which removes it - * from the list. Thus we use list_top() not list_pop() here. */ + /*~ For every peer, we free every channel. On allocation the peer was + * given a destructor (`destroy_peer`) which removes itself from the + * list. Thus we use list_top() not list_pop() here. */ while ((p = list_top(&ld->peers, struct peer, list)) != NULL) { struct channel *c; @@ -343,14 +428,15 @@ static void shutdown_subdaemons(struct lightningd *ld) * saves lots of struggles with our 80-column guideline! */ const struct chainparams *get_chainparams(const struct lightningd *ld) { - /* "The lightningd is connected to the chain topology." - * "The chain topology is connected to the bitcoind API." + /* "The lightningd is connected to the blockchain." + * "The blockchain is connected to the bitcoind API." * "The bitcoind API is connected chain parameters." * -- Worst childhood song ever. */ return ld->topology->bitcoind->chainparams; } -/*~ Our wallet logic needs to know what outputs we might be interested in: we +/*~ Our wallet logic needs to know what outputs we might be interested in. We + * use BIP32 (a.k.a. "HD wallet") to generate keys from a single seed, so we * keep the maximum-ever-used key index in the db, and add them all to the * filter here. */ static void init_txfilter(struct wallet *w, struct txfilter *filter) @@ -415,7 +501,7 @@ static void pidfile_create(const struct lightningd *ld) if (pid_fd < 0) err(1, "Failed to open PID file"); - /* Lock PID file: this will stay locked until we exit. */ + /* Lock PID file, so future lockf will fail. */ if (lockf(pid_fd, F_TLOCK, 0) < 0) /* Problem locking file */ err(1, "lightningd already running? Error locking PID file"); @@ -429,7 +515,8 @@ static void pidfile_create(const struct lightningd *ld) * to ignore the result without jumping through hoops. */ write_all(pid_fd, pid, strlen(pid)); - /* Leave file open: we close it implicitly when we exit */ + /*~ As closing the file will remove the lock, we need to keep it open; + * the OS will close it implicitly when we exit for any reason. */ } /*~ ccan/io allows overriding the poll() function that is the very core @@ -472,7 +559,7 @@ int main(int argc, char *argv[]) /*~ There's always a battle between what a constructor like this * should do, and what should be added later by the caller. In * general, because we use valgrind heavily for testing, we prefer not - * to intialize unused fields which we expect the caller to set: + * to initialize unused fields which we expect the caller to set: * valgrind will warn us if we make decisions based on uninitialized * variables. */ ld = new_lightningd(NULL); @@ -493,7 +580,8 @@ int main(int argc, char *argv[]) test_subdaemons(ld); /*~ Our "wallet" code really wraps the db, which is more than a simple - * bitcoin wallet (though it's that too). */ + * bitcoin wallet (though it's that too). It also stores channel + * states, invoices, payments, blocks and bitcoin transactions. */ ld->wallet = wallet_new(ld, ld->log, &ld->timers); /*~ We keep a filter of scriptpubkeys we're interested in. */ @@ -502,7 +590,13 @@ int main(int argc, char *argv[]) /*~ This is the ccan/io central poll override from above. */ io_poll_override(io_poll_lightningd); - /*~ Set up HSM: it knows our node secret key, so tells us who we are. */ + /*~ Set up the HSM daemon, which knows our node secret key, so tells + * us who we are. + * + * HSM stands for Hardware Security Module, which is the industry + * standard of key storage; ours is in software for now, so the name + * doesn't really make sense, but we can't call it the Badly-named + * Daemon Software Module. */ hsm_init(ld); /*~ Our default color and alias are derived from our node id, so we @@ -545,7 +639,7 @@ int main(int argc, char *argv[]) load_channels_from_wallet(ld); /*~ Get the blockheight we are currently at, UINT32_MAX is used to signal - * an unitialized wallet and that we should start off of bitcoind's + * an uninitialized wallet and that we should start off of bitcoind's * current height */ wallet_blocks_heights(ld->wallet, UINT32_MAX, &min_blockheight, &max_blockheight); @@ -568,7 +662,8 @@ int main(int argc, char *argv[]) setup_topology(ld->topology, &ld->timers, min_blockheight, max_blockheight); - /*~ Create RPC socket (if any): now we can talk to clients. */ + /*~ Create RPC socket: now lightning-cli can send us JSON RPC commands + * over a UNIX domain socket specified by `ld->rpc_filename`. */ setup_jsonrpc(ld, ld->rpc_filename); /*~ We defer --daemon until we've completed most initialization: that @@ -620,10 +715,16 @@ int main(int argc, char *argv[]) * a backtrace if we fail during startup. */ crashlog = ld->log; - /*~ The root of every backtrace (almost). */ + /*~ The root of every backtrace (almost). This is our main event + * loop. */ for (;;) { - /* ~io_loop returns if there's an expired timer, *or* someone - * calls io_break, or if there are no more IO connections + /* ~ccan/io's io_loop() continuously calls + * io_poll_lightningd() for all file descriptors registered + * with it, then calls their callbacks or closes them if they + * fail, as appropriate. + * + * It will only exit if there's an expired timer, *or* someone + * calls io_break, or if there are no more file descriptors * (which never happens in our code). */ struct timer *expired; void *v = io_loop(&ld->timers, &expired); @@ -663,6 +764,6 @@ int main(int argc, char *argv[]) #endif daemon_shutdown(); - /*~ Farewell. Next stop: hsmd/hsm.c. */ + /*~ Farewell. Next stop: hsmd/hsmd.c. */ return 0; } From f2e085778cc1eb1eb6f9e980ca5cfbd03807a4ad Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 13:10:02 +0930 Subject: [PATCH 29/39] lightningd: more comment fixes. Suggested-by: @practicalswift and @cdecker. Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index d31a255e9d90..6d09f1e97e4f 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -12,7 +12,7 @@ * * Comments beginning with a ~ (like this one!) are part of our shared * adventure through the source, so they're more meta than normal code - * comments, and mean to be read in a certain order. + * comments, and meant to be read in a certain order. */ /*~ Notice how includes are in ASCII order: this is actually enforced by @@ -87,7 +87,9 @@ static struct lightningd *new_lightningd(const tal_t *ctx) /*~ tal: each allocation is a child of an existing object (or NULL, * the top-level object). When an object is freed, all the objects * `tallocated` off it are also freed. We use it in place of malloc - * and free. + * and free. For the technically inclined: tal allocations usually + * build a tree, and tal_freeing any node in the tree will result in + * the entire subtree rooted at that node to be freed. * * It's incredibly useful for grouping object lifetimes, as we'll see. * For example, a `struct bitcoin_tx` has a pointer to an array of @@ -142,7 +144,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) /*~ These are hash tables of incoming and outgoing HTLCs (contracts), * defined as `struct htlc_in` and `struct htlc_out`in htlc_end.h. - * The hash tables are declared ther using the very ugly + * The hash tables are declared there using the very ugly * HTABLE_DEFINE_TYPE macro. The key is the channel the HTLC is in * and the 64-bit htlc-id which is unique for that channel and * direction. That htlc-id is used in the inter-peer wire protocol, @@ -258,7 +260,7 @@ void test_subdaemons(const struct lightningd *ld) /*~ Our logging system: spam goes in at log_debug level, but * logging is mainly added by developer necessity and removed * by developer/user complaints . The only strong convention - * is that log_broken() is used for "should never happen". + * is that log_broken() is used for "should never happen". * * Note, however, that logging takes care to preserve the * global `errno` which is set above. */ From ae61f645abe62f5bd40e32bc1653780202e2f2a4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Sep 2018 12:26:59 +0930 Subject: [PATCH 30/39] chaintopology: don't "fix" unknown feerate against known one. This was found because it means we have a non-zero feerate without filling in the history of that feerate: ==15895== Conditional jump or move depends on uninitialised value(s) ==15895== at 0x408699: feerate_max (chaintopology.c:828) ==15895== by 0x41BE49: peer_start_openingd (opening_control.c:733) ==15895== by 0x425FE9: peer_connected (peer_control.c:515) ==15895== by 0x40CB8F: connectd_msg (connect_control.c:304) ==15895== by 0x42DB4E: sd_msg_read (subd.c:475) ==15895== by 0x42D499: read_fds (subd.c:302) ==15895== by 0x46EB18: next_plan (io.c:59) ==15895== by 0x46F5E9: do_plan (io.c:387) ==15895== by 0x46F627: io_ready (io.c:397) ==15895== by 0x471187: io_loop (poll.c:310) ==15895== by 0x41683D: main (lightningd.c:732) Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index fa921aa16671..290142c562f6 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -386,11 +386,15 @@ static void update_feerates(struct bitcoind *bitcoind, maybe_completed_init(topo); } - /* Make sure fee rates are in order. */ + /* Make sure (known) fee rates are in order. */ for (size_t i = 0; i < NUM_FEERATES; i++) { + if (!topo->feerate[i]) + continue; for (size_t j = 0; j < i; j++) { + if (!topo->feerate[j]) + continue; if (topo->feerate[j] < topo->feerate[i]) { - log_debug(topo->log, + log_unusual(topo->log, "Feerate estimate for %s (%u) above %s (%u)", feerate_name(i), topo->feerate[i], feerate_name(j), topo->feerate[j]); From 7b9341e7627950852f246595ef23c94cd9c03cd4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 12:23:32 +0930 Subject: [PATCH 31/39] subdaemon: better GDB support. It was annoying me, so I made it much nicer to use. Signed-off-by: Rusty Russell --- common/subdaemon.c | 15 ++++++--------- doc/HACKING.md | 18 +++++++++--------- lightningd/options.c | 2 +- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/common/subdaemon.c b/common/subdaemon.c index 3a602105c2e6..61abfd6f80d5 100644 --- a/common/subdaemon.c +++ b/common/subdaemon.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -24,11 +25,6 @@ static void status_backtrace_exit(void) status_failed(STATUS_FAIL_INTERNAL_ERROR, "FATAL SIGNAL"); } -#if DEVELOPER -extern volatile bool debugger_connected; -volatile bool debugger_connected; -#endif - void subdaemon_setup(int argc, char *argv[]) { if (argc == 2 && streq(argv[1], "--version")) { @@ -45,10 +41,11 @@ void subdaemon_setup(int argc, char *argv[]) /* From debugger, set debugger_spin to 0. */ for (int i = 1; i < argc; i++) { if (streq(argv[i], "--debugger")) { - fprintf(stderr, "gdb -ex 'attach %u' -ex 'p debugger_connected=1' %s\n", - getpid(), argv[0]); - while (!debugger_connected) - usleep(10000); + char *cmd = tal_fmt(NULL, "${DEBUG_TERM:-gnome-terminal --} gdb -ex 'attach %u' %s &", getpid(), argv[0]); + fprintf(stderr, "Running %s\n", cmd); + system(cmd); + /* Continue in the debugger. */ + kill(getpid(), SIGSTOP); } if (strstarts(argv[i], "--dev-disconnect=")) { dev_disconnect_init(atoi(argv[i] diff --git a/doc/HACKING.md b/doc/HACKING.md index fa35e52001b5..d0e2c1eeac13 100644 --- a/doc/HACKING.md +++ b/doc/HACKING.md @@ -92,15 +92,15 @@ Debugging You can build c-lightning with DEVELOPER=1 to use dev commands listed in ``cli/lightning-cli help``. ``./configure --enable-developer`` will do that. You can log console messages with log_info() in lightningd and status_trace() in other subdaemons. You can debug crashing subdaemons with the argument -`--dev-debugger=lightning_channeld`, where `channeld` is the subdaemon name. -It will print out (to stderr) a command such as: - - gdb -ex 'attach 22398' -ex 'p debugger_connected=1' \ - lightningd/lightning_hsmd - -Run this command to start debugging. -You may need to type `return` one more time to exit the infinite while -loop, otherwise you can type `continue` to begin. +`--dev-debugger=channeld`, where `channeld` is the subdaemon name. It +will run `gnome-terminal` by default with a gdb attached to the +subdaemon when it starts. You can change the terminal used by setting +the `DEBUG_TERM` environment variable, such as `DEBUG_TERM="xterm -e"` +or `DEBUG_TERM="konsole -e"`. + +It will also print out (to stderr) the gdb command for manual connection. The +subdaemon will be stopped (it sends itself a SIGSTOP); you'll need to +`continue` in gdb. Database -------- diff --git a/lightningd/options.c b/lightningd/options.c index aa23ea218745..bed2029f3e99 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -406,7 +406,7 @@ static void dev_register_opts(struct lightningd *ld) opt_register_noarg("--dev-fail-on-subdaemon-fail", opt_set_bool, &ld->dev_subdaemon_fail, opt_hidden); opt_register_arg("--dev-debugger=", opt_subd_debug, NULL, - ld, "Wait for gdb attach at start of "); + ld, "Invoke gdb at start of "); opt_register_arg("--dev-broadcast-interval=", opt_set_uintval, opt_show_uintval, &ld->config.broadcast_interval, "Time between gossip broadcasts in milliseconds"); From 312209ad60a722b91145785e6f92802a942e769b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 12:24:32 +0930 Subject: [PATCH 32/39] pytest: support simple subdaemon debugging. Signed-off-by: Rusty Russell --- doc/HACKING.md | 6 ++++-- tests/utils.py | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/doc/HACKING.md b/doc/HACKING.md index d0e2c1eeac13..bdfb8488f975 100644 --- a/doc/HACKING.md +++ b/doc/HACKING.md @@ -174,7 +174,7 @@ A modern desktop can build and run through all the tests in a couple of minutes make -j12 check PYTEST_PAR=24 DEVELOPER=1 VALGRIND=0 -Adust `-j` and `PYTEST_PAR` accordingly for your hardware. +Adjust `-j` and `PYTEST_PAR` accordingly for your hardware. There are three kinds of tests: @@ -200,7 +200,9 @@ There are three kinds of tests: `PYTHONPATH=contrib/pylightning py.test -v tests/`. - You can also append `-k TESTNAME` to run a single test. + You can also append `-k TESTNAME` to run a single test. Environment variables + `DEBUG_SUBD=` and `TIMEOUT=` can be useful for debugging + subdaemons on individual tests. Our Travis CI instance (see `.travis.yml`) runs all these for each pull request. diff --git a/tests/utils.py b/tests/utils.py index 6b1faa1011c6..cc2fe9373798 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -703,6 +703,8 @@ def get_node(self, disconnect=None, options=None, may_fail=False, may_reconnect= if DEVELOPER: daemon.opts["dev-fail-on-subdaemon-fail"] = None daemon.env["LIGHTNINGD_DEV_MEMLEAK"] = "1" + if os.getenv("DEBUG_SUBD"): + daemon.opts["dev-debugger"] = os.getenv("DEBUG_SUBD") if VALGRIND: daemon.env["LIGHTNINGD_DEV_NO_BACKTRACE"] = "1" if not may_reconnect: From e2f426903d653e310e483a610dd1d0015e536c9c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 14:52:03 +0930 Subject: [PATCH 33/39] gossipd: handle premature node_announcements in the store. These happen after we compact the store; every log I've seen of a restart on a real node has a message about truncating the store, because node_announcements predate channel_announcements. I extracted one such case from testnet, and reduced it to test here. Signed-off-by: Rusty Russell --- gossipd/gossip_store.c | 31 +++++++++++++++++++++++++------ gossipd/routing.c | 16 ++++++++++++---- gossipd/routing.h | 6 +++++- tests/test_gossip.py | 24 ++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/gossipd/gossip_store.c b/gossipd/gossip_store.c index 2e070e69ffba..be25ecd65d31 100644 --- a/gossipd/gossip_store.c +++ b/gossipd/gossip_store.c @@ -251,9 +251,12 @@ void gossip_store_load(struct routing_state *rstate, struct gossip_store *gs) /* We set/check version byte on creation */ off_t known_good = 1; const char *bad; - size_t stats[] = {0, 0, 0, 0}; + size_t stats[] = {0, 0, 0, 0, 0}; int fd = gs->fd; gs->fd = -1; + bool unknown_node; + size_t num_delayed_na = 0; + u8 **delayed_na = tal_arr(tmpctx, u8 *, num_delayed_na); if (lseek(fd, known_good, SEEK_SET) < 0) { status_unusual("gossip_store: lseek failure"); @@ -294,11 +297,18 @@ void gossip_store_load(struct routing_state *rstate, struct gossip_store *gs) stats[1]++; } else if (fromwire_gossip_store_node_announcement(msg, msg, &gossip_msg)) { - if (!routing_add_node_announcement(rstate, gossip_msg)) { - bad = "Bad node_announcement"; - goto truncate; - } - stats[2]++; + if (!routing_add_node_announcement(rstate, gossip_msg, + &unknown_node)) { + if (!unknown_node) { + bad = "Bad node_announcement"; + goto truncate; + } + /* Defer until later. */ + tal_resize(&delayed_na, num_delayed_na+1); + delayed_na[num_delayed_na++] + = tal_steal(delayed_na, gossip_msg); + } else + stats[2]++; } else if (fromwire_gossip_store_channel_delete(msg, &scid)) { struct chan *c = get_channel(rstate, &scid); if (!c) { @@ -318,6 +328,15 @@ void gossip_store_load(struct routing_state *rstate, struct gossip_store *gs) gs->count++; tal_free(msg); } + + for (size_t i = 0; i < tal_count(delayed_na); i++) { + if (routing_add_node_announcement(rstate, delayed_na[i], NULL)) { + stats[2]++; + stats[4]++; + } + } + status_trace("Successfully processed %zu/%zu unknown node_announcement", + stats[4], tal_count(delayed_na)); goto out; truncate: diff --git a/gossipd/routing.c b/gossipd/routing.c index c6ef01cf20d9..619dd921ab72 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -1276,7 +1276,9 @@ static struct wireaddr *read_addresses(const tal_t *ctx, const u8 *ser) return wireaddrs; } -bool routing_add_node_announcement(struct routing_state *rstate, const u8 *msg TAKES) +bool routing_add_node_announcement(struct routing_state *rstate, + const u8 *msg TAKES, + bool *unknown_node) { struct node *node; secp256k1_ecdsa_signature signature; @@ -1290,15 +1292,21 @@ bool routing_add_node_announcement(struct routing_state *rstate, const u8 *msg T if (!fromwire_node_announcement(tmpctx, msg, &signature, &features, ×tamp, &node_id, rgb_color, alias, - &addresses)) + &addresses)) { + if (unknown_node) + *unknown_node = false; return false; + } node = get_node(rstate, &node_id); /* May happen if we accepted the node_announcement due to a local * channel, for which we didn't have the announcement yet. */ - if (node == NULL) + if (node == NULL) { + if (unknown_node) + *unknown_node = true; return false; + } wireaddrs = read_addresses(tmpctx, addresses); tal_free(node->addresses); @@ -1451,7 +1459,7 @@ u8 *handle_node_announcement(struct routing_state *rstate, const u8 *node_ann) status_trace("Received node_announcement for node %s", type_to_string(tmpctx, struct pubkey, &node_id)); - applied = routing_add_node_announcement(rstate, serialized); + applied = routing_add_node_announcement(rstate, serialized, NULL); assert(applied); return NULL; } diff --git a/gossipd/routing.h b/gossipd/routing.h index 999b23571a26..1631498bb644 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -300,9 +300,13 @@ bool routing_add_channel_update(struct routing_state *rstate, * Directly add the node being announced to the network view, without verifying * it. This must be from a trusted source, e.g., gossip_store. For untrusted * sources (peers) please use @see{handle_node_announcement}. + * + * Populates *unknown_node if it isn't NULL and this returns false to indicate + * if failure was due to an unknown node_id. */ bool routing_add_node_announcement(struct routing_state *rstate, - const u8 *msg TAKES); + const u8 *msg TAKES, + bool *unknown_node); /** diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 67b4f84a1c01..ce53d1ac3abe 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -815,3 +815,27 @@ def test_gossip_addresses(node_factory, bitcoind): {'type': 'torv2', 'address': '3fyb44wdhnd2ghhl.onion', 'port': 1234}, {'type': 'torv3', 'address': 'vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd.onion', 'port': 9735} ] + + +def test_gossip_store_load(node_factory): + """Make sure we can read canned gossip store""" + l1 = node_factory.get_node(start=False) + with open(os.path.join(l1.daemon.lightning_dir, 'gossip_store'), 'wb') as f: + f.write(bytearray.fromhex("02" # GOSSIP_VERSION + "00000099" # len + "12abbbba" # csum + "1002" # WIRE_GOSSIP_STORE_NODE_ANNOUNCEMENT + "00950101cf5d870bc7ecabcb7cd16898ef66891e5f0c6c5851bd85b670f03d325bc44d7544d367cd852e18ec03f7f4ff369b06860a3b12b07b29f36fb318ca11348bf8ec00005aab817c03f113414ebdc6c1fb0f33c99cd5a1d09dd79e7fdf2468cf1fe1af6674361695d23974b250757a7a6c6549544300000000000000000000000000000000000000000000000007010566933e2607" + "000001bc" # len + "521ef598" # csum + "1000" # WIRE_GOSSIP_STORE_CHANNEL_ANNOUNCEMENT + "01b00100bb8d7b6998cca3c2b3ce12a6bd73a8872c808bb48de2a30c5ad9cdf835905d1e27505755087e675fb517bbac6beb227629b694ea68f49d357458327138978ebfd7adfde1c69d0d2f497154256f6d5567a5cf2317c589e0046c0cc2b3e986cf9b6d3b44742bd57bce32d72cd1180a7f657795976130b20508b239976d3d4cdc4d0d6e6fbb9ab6471f664a662972e406f519eab8bce87a8c0365646df5acbc04c91540b4c7c518cec680a4a6af14dae1aca0fd5525220f7f0e96fcd2adef3c803ac9427fe71034b55a50536638820ef21903d09ccddd38396675b598587fa886ca711415c813fc6d69f46552b9a0a539c18f265debd0e2e286980a118ba349c216000043497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea33090000000013a63c0000b50001021bf3de4e84e3d52f9a3e36fbdcd2c4e8dbf203b9ce4fc07c2f03be6c21d0c67503f113414ebdc6c1fb0f33c99cd5a1d09dd79e7fdf2468cf1fe1af6674361695d203801fd8ab98032f11cc9e4916dd940417082727077609d5c7f8cc6e9a3ad25dd102517164b97ab46cee3826160841a36c46a2b7b9c74da37bdc070ed41ba172033a0000000001000000" + "00000086" # len + "88c703c8" # csum + "1001" # WIRE_GOSSIP_STORE_CHANNEL_UPDATE + "008201021ea7c2eadf8a29eb8690511a519b5656e29aa0a853771c4e38e65c5abf43d907295a915e69e451f4c7a0c3dc13dd943cfbe3ae88c0b96667cd7d58955dbfedcf43497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea33090000000013a63c0000b500015b8d9b440000009000000000000003e8000003e800000001")) + + l1.start() + # May preceed the Started msg waited for in 'start'. + wait_for(lambda: l1.daemon.is_in_log('gossip_store: Read 1/1/1/0 cannounce/cupdate/nannounce/cdelete from store in 744 bytes')) + assert not l1.daemon.is_in_log('gossip_store.*truncating') From 97c7ba2f801cab13224bd05bc4e3c50e866b9e10 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 14:52:47 +0930 Subject: [PATCH 34/39] gossipd: fix reordering of node_announcements in presence of a unannounced channel. If we receive a channel_announce but not a channel_update, we store the announce but don't put it in the broadcast map. When we delete a channel, we check if the node_announcement broadcast now preceeds all channel_announcements, and if so, we move it to the end of the map. However, with a channel_announcement at index '0', this test fails. This is at least one potential cause of the node map getting out of order. Signed-off-by: Rusty Russell --- gossipd/routing.c | 2 +- gossipd/routing.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/gossipd/routing.c b/gossipd/routing.c index 619dd921ab72..a0ab36a8054a 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -199,7 +199,7 @@ static bool remove_channel_from_array(struct chan ***chans, const struct chan *c static bool node_announce_predates_channels(const struct node *node) { for (size_t i = 0; i < tal_count(node->chans); i++) { - if (!is_chan_public(node->chans[i])) + if (!is_chan_announced(node->chans[i])) continue; if (node->chans[i]->channel_announcement_index diff --git a/gossipd/routing.h b/gossipd/routing.h index 1631498bb644..f8552611ea95 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -61,11 +61,19 @@ struct chan { u64 satoshis; }; +/* A local channel can exist which isn't announcable. */ static inline bool is_chan_public(const struct chan *chan) { return chan->channel_announce != NULL; } +/* A channel is only announced once we have a channel_update to send + * with it. */ +static inline bool is_chan_announced(const struct chan *chan) +{ + return chan->channel_announcement_index != 0; +} + static inline bool is_halfchan_defined(const struct half_chan *hc) { return hc->channel_update != NULL; From db12a1452f9a87da2cefa7d5e62543d1e955d6a7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 14:48:55 +0930 Subject: [PATCH 35/39] pytest: reproduce problem with restarting and retransmitting multiple outgoing htlcs Signed-off-by: Rusty Russell --- tests/test_connection.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 7dbcb1da8af4..c76798e0f0fc 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1295,3 +1295,31 @@ def test_dataloss_protection(node_factory, bitcoind): # l2 should have it in wallet. assert (closetxid, "confirmed") in set([(o['txid'], o['status']) for o in l2.rpc.listfunds()['outputs']]) + + +@pytest.mark.xfail(strict=True) +@unittest.skipIf(not DEVELOPER, "needs dev_disconnect") +def test_restart_multi_htlc_rexmit(node_factory, bitcoind, executor): + # l1 disables commit timer once we send first htlc, dies on commit + disconnects = ['=WIRE_UPDATE_ADD_HTLC-nocommit', + '-WIRE_COMMITMENT_SIGNED'] + l1, l2 = node_factory.line_graph(2, opts=[{'disconnect': disconnects, + 'may_reconnect': True}, + {'may_reconnect': True}]) + + executor.submit(l1.pay, l2, 20000) + executor.submit(l1.pay, l2, 30000) + + l1.daemon.wait_for_logs(['peer_out WIRE_UPDATE_ADD_HTLC'] * 2) + l1.rpc.dev_reenable_commit(l2.info['id']) + l1.daemon.wait_for_log('dev_disconnect: -WIRE_COMMITMENT_SIGNED') + + # This will make it reconnect + l1.stop() + # Clear the disconnect so we can proceed normally + del l1.daemon.opts['dev-disconnect'] + l1.start() + + # Payments will fail due to restart, but we can see results in listpayments. + print(l1.rpc.listpayments()) + wait_for(lambda: [p['status'] for p in l1.rpc.listpayments()['payments']] == ['complete', 'complete']) From cefb6925b251d9f0e9420fd8250032652ae137d1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 4 Sep 2018 16:32:35 +0930 Subject: [PATCH 36/39] db: save and restore last_sent_commit correctly. It's an array: we were only saving the single element; if there was more than one changed HTLC we'd get a bad signature! The report in #1907 is probably caused by the other side re-requesting something we considered already finalized; to avoid this particular error, we should set the field to NULL if there's no last_sent_commit. I'm increasingly of the opinion we want to just save all the update packets to the db and blast them out, instead of doing this second-guessing dance. Fixes: #1907 Signed-off-by: Rusty Russell --- Makefile | 2 +- tests/test_connection.py | 1 - wallet/db.c | 2 ++ wallet/test/Makefile | 1 + wallet/test/run-wallet.c | 14 ++++++----- wallet/wallet.c | 51 ++++++++++++++++++++++++++++------------ 6 files changed, 48 insertions(+), 23 deletions(-) diff --git a/Makefile b/Makefile index 69546abb94a4..71ae936a0552 100644 --- a/Makefile +++ b/Makefile @@ -35,7 +35,7 @@ endif ifeq ($(COMPAT),1) # We support compatibility with pre-0.6. -COMPAT_CFLAGS=-DCOMPAT_V052=1 +COMPAT_CFLAGS=-DCOMPAT_V052=1 -DCOMPAT_V060=1 endif PYTEST_OPTS := -v -x diff --git a/tests/test_connection.py b/tests/test_connection.py index c76798e0f0fc..5b555866ae9a 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1297,7 +1297,6 @@ def test_dataloss_protection(node_factory, bitcoind): assert (closetxid, "confirmed") in set([(o['txid'], o['status']) for o in l2.rpc.listfunds()['outputs']]) -@pytest.mark.xfail(strict=True) @unittest.skipIf(not DEVELOPER, "needs dev_disconnect") def test_restart_multi_htlc_rexmit(node_factory, bitcoind, executor): # l1 disables commit timer once we send first htlc, dies on commit diff --git a/wallet/db.c b/wallet/db.c index 0e50c8500d4d..f7dd6b39b705 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -337,6 +337,8 @@ char *dbmigrations[] = { "ALTER TABLE payments ADD description TEXT;", /* future_per_commitment_point if other side proves we're out of date -- */ "ALTER TABLE channels ADD future_per_commitment_point BLOB;", + /* last_sent_commit array fix */ + "ALTER TABLE channels ADD last_sent_commit BLOB;", NULL, }; diff --git a/wallet/test/Makefile b/wallet/test/Makefile index 019fd944d4e6..e33af34824bd 100644 --- a/wallet/test/Makefile +++ b/wallet/test/Makefile @@ -6,6 +6,7 @@ WALLET_TEST_COMMON_OBJS := \ common/base32.o \ common/derive_basepoints.o \ common/htlc_state.o \ + common/htlc_wire.o \ common/type_to_string.o \ common/memleak.o \ common/key_derive.o \ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index dee09cd697c0..8d460e28d3f4 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -747,9 +747,10 @@ static bool channelseq(struct channel *c1, struct channel *c2) CHECK(c1->our_config.id != 0 && c1->our_config.id == c2->our_config.id); CHECK((lc1 != NULL) == (lc2 != NULL)); - if(lc1) { - CHECK(lc1->newstate == lc2->newstate); - CHECK(lc1->id == lc2->id); + CHECK(tal_count(lc1) == tal_count(lc2)); + for (size_t i = 0; i < tal_count(lc1); i++) { + CHECK(lc1[i].newstate == lc2[i].newstate); + CHECK(lc1[i].id == lc2[i].id); } CHECK((c1->last_tx != NULL) == (c2->last_tx != NULL)); @@ -795,7 +796,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) struct channel_info *ci = &c1.channel_info; struct bitcoin_txid *hash = tal(w, struct bitcoin_txid); struct pubkey pk; - struct changed_htlc last_commit; + struct changed_htlc *last_commit; secp256k1_ecdsa_signature *sig = tal(w, secp256k1_ecdsa_signature); u8 *scriptpubkey = tal_arr(ctx, u8, 100); @@ -804,7 +805,8 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) memset(ci, 3, sizeof(*ci)); mempat(hash, sizeof(*hash)); mempat(sig, sizeof(*sig)); - mempat(&last_commit, sizeof(last_commit)); + last_commit = tal_arr(w, struct changed_htlc, 2); + mempat(last_commit, tal_bytelen(last_commit)); pubkey_from_der(tal_hexdata(w, "02a1633cafcc01ebfb6d78e39f687a1f0995c62fc95f51ead10a02ee0be551b5dc", 66), 33, &pk); ci->feerate_per_kw[LOCAL] = ci->feerate_per_kw[REMOTE] = 31337; mempat(scriptpubkey, tal_count(scriptpubkey)); @@ -864,7 +866,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK(c1.their_shachain.id == 1); /* Variant 3: update with last_commit_sent */ - c1.last_sent_commit = &last_commit; + c1.last_sent_commit = last_commit; wallet_channel_save(w, &c1); CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(c2 = wallet_channel_load(w, c1.dbid), tal_fmt(w, "Load from DB")); diff --git a/wallet/wallet.c b/wallet/wallet.c index 500a745fd4c8..23b94020f03a 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -609,13 +609,26 @@ static struct channel *wallet_stmt2channel(const tal_t *ctx, struct wallet *w, s remote_shutdown_scriptpubkey = sqlite3_column_arr(tmpctx, stmt, 28, u8); /* Do we have a last_sent_commit, if yes, populate */ - if (sqlite3_column_type(stmt, 30) != SQLITE_NULL) { + if (sqlite3_column_type(stmt, 41) != SQLITE_NULL) { + const u8 *cursor = sqlite3_column_blob(stmt, 41); + size_t len = sqlite3_column_bytes(stmt, 41); + size_t n = 0; + last_sent_commit = tal_arr(tmpctx, struct changed_htlc, n); + while (len) { + tal_resize(&last_sent_commit, n+1); + fromwire_changed_htlc(&cursor, &len, + &last_sent_commit[n++]); + } + } else + last_sent_commit = NULL; + +#ifdef COMPAT_V060 + if (!last_sent_commit && sqlite3_column_type(stmt, 30) != SQLITE_NULL) { last_sent_commit = tal(tmpctx, struct changed_htlc); last_sent_commit->newstate = sqlite3_column_int64(stmt, 30); last_sent_commit->id = sqlite3_column_int64(stmt, 31); - } else { - last_sent_commit = NULL; } +#endif if (sqlite3_column_type(stmt, 40) != SQLITE_NULL) { future_per_commitment_point = tal(tmpctx, struct pubkey); @@ -715,7 +728,8 @@ static const char *channel_fields = /*30*/ "last_sent_commit_state, last_sent_commit_id, " /*32*/ "last_tx, last_sig, last_was_revoke, first_blocknum, " /*36*/ "min_possible_feerate, max_possible_feerate, " - /*38*/ "msatoshi_to_us_min, msatoshi_to_us_max, future_per_commitment_point "; + /*38*/ "msatoshi_to_us_min, msatoshi_to_us_max, future_per_commitment_point, " + /*41*/ "last_sent_commit"; bool wallet_channels_load_active(const tal_t *ctx, struct wallet *w) { @@ -893,6 +907,7 @@ u64 wallet_get_channel_dbid(struct wallet *wallet) void wallet_channel_save(struct wallet *w, struct channel *chan) { sqlite3_stmt *stmt; + u8 *last_sent_commit; assert(chan->first_blocknum); wallet_channel_config_save(w, &chan->our_config); @@ -996,17 +1011,23 @@ void wallet_channel_save(struct wallet *w, struct channel *chan) db_exec_prepared(w->db, stmt); /* If we have a last_sent_commit, store it */ - if (chan->last_sent_commit) { - stmt = db_prepare(w->db, - "UPDATE channels SET" - " last_sent_commit_state=?," - " last_sent_commit_id=?" - " WHERE id=?"); - sqlite3_bind_int(stmt, 1, chan->last_sent_commit->newstate); - sqlite3_bind_int64(stmt, 2, chan->last_sent_commit->id); - sqlite3_bind_int64(stmt, 3, chan->dbid); - db_exec_prepared(w->db, stmt); - } + last_sent_commit = tal_arr(tmpctx, u8, 0); + for (size_t i = 0; i < tal_count(chan->last_sent_commit); i++) + towire_changed_htlc(&last_sent_commit, + &chan->last_sent_commit[i]); + + stmt = db_prepare(w->db, + "UPDATE channels SET" + " last_sent_commit=?" + " WHERE id=?"); + if (tal_count(last_sent_commit)) + sqlite3_bind_blob(stmt, 1, + last_sent_commit, tal_count(last_sent_commit), + SQLITE_TRANSIENT); + else + sqlite3_bind_null(stmt, 1); + sqlite3_bind_int64(stmt, 2, chan->dbid); + db_exec_prepared(w->db, stmt); } void wallet_channel_insert(struct wallet *w, struct channel *chan) From b52fb14726cb22a882041a5d26ef3a6000f17a2a Mon Sep 17 00:00:00 2001 From: lucash-dev Date: Tue, 4 Sep 2018 19:29:06 -0700 Subject: [PATCH 37/39] pytest: Fix configure to find pytest when installed using pip3. Installing pytest through pip3 (at least sometimes) doesn't create a script. This means calling `which` won't work. Changed configure so that it can also test if the module is present by calling python/python3. Change the error message for when pytest can't be found, so that it's clear to the user `configure` must be ran again after installing pytest. --- Makefile | 2 +- configure | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 71ae936a0552..4ad9274a2e7e 100644 --- a/Makefile +++ b/Makefile @@ -216,7 +216,7 @@ check: pytest: $(ALL_PROGRAMS) ifndef PYTEST - @echo "py.test is required to run the integration tests, please install using 'pip3 install -r tests/requirements.txt'" + @echo "py.test is required to run the integration tests, please install using 'pip3 install -r tests/requirements.txt', and rerun 'configure'." exit 1 else # Explicitly hand DEVELOPER and VALGRIND so you can override on make cmd line. diff --git a/configure b/configure index 3e6739fcc251..eb43b59bf827 100755 --- a/configure +++ b/configure @@ -71,6 +71,17 @@ find_pytest() return fi done + + PYTHON_BINS="python python3" + for p in $PYTHON_BINS; do + if [ "$(which $p)" != "" ] ; then + $p --version 2>&1 | grep -q "Python 3." || continue + if $p -c "import pytest" ; then + echo "$p -m pytest" + return + fi + fi + done } PYTEST=${PYTEST:-`find_pytest`} From 6a4468edca25e5bcb9c5b801e50fb19f85116e13 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 7 Sep 2018 16:47:47 +0200 Subject: [PATCH 38/39] doc: Spelling corrections --- doc/lightning-fundchannel.7.txt | 2 +- doc/lightning-listfunds.7.txt | 4 ++-- doc/lightning-pay.7.txt | 2 +- doc/lightning-waitinvoice.7.txt | 2 +- doc/lightningd-config.5.txt | 6 +++--- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/lightning-fundchannel.7.txt b/doc/lightning-fundchannel.7.txt index 2ed2e649b31a..1d1595571cb4 100644 --- a/doc/lightning-fundchannel.7.txt +++ b/doc/lightning-fundchannel.7.txt @@ -12,7 +12,7 @@ SYNOPSIS DESCRIPTION ----------- -The *fundchannel* RPC command opens a payment channel with a peer by commiting +The *fundchannel* RPC command opens a payment channel with a peer by committing a funding transaction to the blockchain as defined in BOLT #2. *fundchannel* by itself does not attempt to open a connection. A connection must first be established using *connect*. diff --git a/doc/lightning-listfunds.7.txt b/doc/lightning-listfunds.7.txt index ddd55aa8006a..fa9477b8baa3 100644 --- a/doc/lightning-listfunds.7.txt +++ b/doc/lightning-listfunds.7.txt @@ -43,10 +43,10 @@ Each entry in 'channels' will include: number and output index of the channel funding transaction). - 'channel_sat' - available satoshis on our node's end of the channel -(values rounded to the nearest satoshi as internal storage is in milisatoshi). +(values rounded to the nearest satoshi as internal storage is in millisatoshi). - 'channel_total_sat' - total channel value in satoshi -(values rounded to the nearest satoshi as internal storage is in milisatoshi). +(values rounded to the nearest satoshi as internal storage is in millisatoshi). - 'funding_txid' - funding transaction id. diff --git a/doc/lightning-pay.7.txt b/doc/lightning-pay.7.txt index ae56b2a44eda..c2dde8c10742 100644 --- a/doc/lightning-pay.7.txt +++ b/doc/lightning-pay.7.txt @@ -25,7 +25,7 @@ paid. The `exemptfee` option can be used for tiny payments which would be dominated by the fee leveraged by forwarding nodes. Setting `exemptfee` allows the `maxfeepercent` check to be skipped on fees that are smaller than `exemptfee` -(default: 5000 millsatoshi). +(default: 5000 millisatoshi). The *pay* RPC command will randomize routes slightly, as long as the route achieves the targeted 'maxfeepercent' and 'maxdelay'. diff --git a/doc/lightning-waitinvoice.7.txt b/doc/lightning-waitinvoice.7.txt index 44215669a5aa..f3238701b927 100644 --- a/doc/lightning-waitinvoice.7.txt +++ b/doc/lightning-waitinvoice.7.txt @@ -23,7 +23,7 @@ The 'status' field will be 'paid'. If the invoice is deleted while unpaid, or the invoice does not exist, this command will return with an error with code -1. -If the invoice expires before being pad, or is already expired, this +If the invoice expires before being paid, or is already expired, this command will return with an error with code -2, with the data being the invoice data as per *listinvoice*. diff --git a/doc/lightningd-config.5.txt b/doc/lightningd-config.5.txt index 30a9a76e393b..00f9709f6358 100644 --- a/doc/lightningd-config.5.txt +++ b/doc/lightningd-config.5.txt @@ -18,7 +18,7 @@ exists, when it starts up. The location of this file defaults to *.lightning* in the home directory, but can be overridden by the '--lightning-dir' option on the lightningd(8) command line. -Configuration file options are processsed first, then command line +Configuration file options are processed first, then command line options: later options override earlier ones except 'addr' options which accumulate. @@ -235,7 +235,7 @@ or precisely control where to bind and what to announce with the *bind-addr*='[IPADDRESS[:PORT]]|SOCKETPATH':: Set an IP address or UNIX domain socket to listen to, but do not - announce. A UNIX domain socket is distingished from an IP address + announce. A UNIX domain socket is distinguished from an IP address by beginning with a '/'. An empty 'IPADDRESS' is a special value meaning bind to IPv4 and/or @@ -294,7 +294,7 @@ or precisely control where to bind and what to announce with the BUGS ---- You should report bugs on our github issues page, and maybe submit a -fix to gain our eternal gratutide! +fix to gain our eternal gratitude! AUTHOR ------ From 634f19a7b230edc686be56ab950b80784e56252c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 7 Sep 2018 16:49:37 +0200 Subject: [PATCH 39/39] doc: Regenerate man-pages after spelling corrections --- doc/lightning-fundchannel.7 | 6 +++--- doc/lightning-listfunds.7 | 8 ++++---- doc/lightning-pay.7 | 6 +++--- doc/lightning-waitinvoice.7 | 6 +++--- doc/lightningd-config.5 | 10 +++++----- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/doc/lightning-fundchannel.7 b/doc/lightning-fundchannel.7 index 3c85d3345777..e99ef51e048e 100644 --- a/doc/lightning-fundchannel.7 +++ b/doc/lightning-fundchannel.7 @@ -2,12 +2,12 @@ .\" Title: lightning-fundchannel .\" Author: [FIXME: author] [see http://docbook.sf.net/el/author] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 08/29/2018 +.\" Date: 09/07/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-FUNDCHANN" "7" "08/29/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-FUNDCHANN" "7" "09/07/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -34,7 +34,7 @@ lightning-fundchannel \- Command for establishing a lightning channel\&. \fBfundchannel\fR \fIid\fR \fIsatoshi\fR [\fIfeerate\fR] .SH "DESCRIPTION" .sp -The \fBfundchannel\fR RPC command opens a payment channel with a peer by commiting a funding transaction to the blockchain as defined in BOLT #2\&. \fBfundchannel\fR by itself does not attempt to open a connection\&. A connection must first be established using \fBconnect\fR\&. Once the transaction is confirmed, normal channel operations may begin\&. Readiness is indicated by \fBlistpeers\fR reporting a \fIstate\fR of CHANNELD_NORMAL for the channel\&. +The \fBfundchannel\fR RPC command opens a payment channel with a peer by committing a funding transaction to the blockchain as defined in BOLT #2\&. \fBfundchannel\fR by itself does not attempt to open a connection\&. A connection must first be established using \fBconnect\fR\&. Once the transaction is confirmed, normal channel operations may begin\&. Readiness is indicated by \fBlistpeers\fR reporting a \fIstate\fR of CHANNELD_NORMAL for the channel\&. .sp \fIid\fR is the peer id obtained from \fBconnect\fR\&. .sp diff --git a/doc/lightning-listfunds.7 b/doc/lightning-listfunds.7 index 3c43140f473b..a3f014a4bbeb 100644 --- a/doc/lightning-listfunds.7 +++ b/doc/lightning-listfunds.7 @@ -2,12 +2,12 @@ .\" Title: lightning-listfunds .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 04/26/2018 +.\" Date: 09/07/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-LISTFUNDS" "7" "04/26/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-LISTFUNDS" "7" "09/07/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -131,7 +131,7 @@ Each entry in \fIchannels\fR will include: .IP \(bu 2.3 .\} \fIchannel_sat\fR -\- available satoshis on our node\(cqs end of the channel (values rounded to the nearest satoshi as internal storage is in milisatoshi)\&. +\- available satoshis on our node\(cqs end of the channel (values rounded to the nearest satoshi as internal storage is in millisatoshi)\&. .RE .sp .RS 4 @@ -143,7 +143,7 @@ Each entry in \fIchannels\fR will include: .IP \(bu 2.3 .\} \fIchannel_total_sat\fR -\- total channel value in satoshi (values rounded to the nearest satoshi as internal storage is in milisatoshi)\&. +\- total channel value in satoshi (values rounded to the nearest satoshi as internal storage is in millisatoshi)\&. .RE .sp .RS 4 diff --git a/doc/lightning-pay.7 b/doc/lightning-pay.7 index b603040c8493..fd1ef7ad8ea2 100644 --- a/doc/lightning-pay.7 +++ b/doc/lightning-pay.7 @@ -2,12 +2,12 @@ .\" Title: lightning-pay .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 07/28/2018 +.\" Date: 09/07/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-PAY" "7" "07/28/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-PAY" "7" "09/07/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -34,7 +34,7 @@ lightning-pay \- Command for sending a payment to a BOLT11 invoice \fBpay\fR \fIbolt11\fR [\fImsatoshi\fR] [\fIdescription\fR] [\fIriskfactor\fR] [\fImaxfeepercent\fR] [\fIretry_for\fR] [\fImaxdelay\fR] .SH "DESCRIPTION" .sp -The \fBpay\fR RPC command attempts to find a route to the given destination, and send the funds it asks for\&. If the \fIbolt11\fR does not contain an amount, \fImsatoshi\fR is required, otherwise if it is specified it must be \fInull\fR\&. If \fIbolt11\fR contains a description hash (\fIh\fR field) \fIdescription\fR is required, otherwise it is unused\&. The \fIriskfactor\fR is described in detail in lightning\-getroute(7), and defaults to 1\&.0\&. The \fImaxfeepercent\fR limits the money paid in fees, and defaults to 0\&.5\&. The maxfeepercent\*(Aq is a percentage of the amount that is to be paid\&. The `exemptfee option can be used for tiny payments which would be dominated by the fee leveraged by forwarding nodes\&. Setting exemptfee allows the maxfeepercent check to be skipped on fees that are smaller than exemptfee (default: 5000 millsatoshi)\&. +The \fBpay\fR RPC command attempts to find a route to the given destination, and send the funds it asks for\&. If the \fIbolt11\fR does not contain an amount, \fImsatoshi\fR is required, otherwise if it is specified it must be \fInull\fR\&. If \fIbolt11\fR contains a description hash (\fIh\fR field) \fIdescription\fR is required, otherwise it is unused\&. The \fIriskfactor\fR is described in detail in lightning\-getroute(7), and defaults to 1\&.0\&. The \fImaxfeepercent\fR limits the money paid in fees, and defaults to 0\&.5\&. The maxfeepercent\*(Aq is a percentage of the amount that is to be paid\&. The `exemptfee option can be used for tiny payments which would be dominated by the fee leveraged by forwarding nodes\&. Setting exemptfee allows the maxfeepercent check to be skipped on fees that are smaller than exemptfee (default: 5000 millisatoshi)\&. .sp The \fBpay\fR RPC command will randomize routes slightly, as long as the route achieves the targeted \fImaxfeepercent\fR and \fImaxdelay\fR\&. .sp diff --git a/doc/lightning-waitinvoice.7 b/doc/lightning-waitinvoice.7 index d492982c3e9a..a26824805068 100644 --- a/doc/lightning-waitinvoice.7 +++ b/doc/lightning-waitinvoice.7 @@ -2,12 +2,12 @@ .\" Title: lightning-waitinvoice .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 04/26/2018 +.\" Date: 09/07/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-WAITINVOI" "7" "04/26/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-WAITINVOI" "7" "09/07/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -41,7 +41,7 @@ On success, an invoice description will be returned as per lightning\-listinvoic .sp If the invoice is deleted while unpaid, or the invoice does not exist, this command will return with an error with code \-1\&. .sp -If the invoice expires before being pad, or is already expired, this command will return with an error with code \-2, with the data being the invoice data as per \fBlistinvoice\fR\&. +If the invoice expires before being paid, or is already expired, this command will return with an error with code \-2, with the data being the invoice data as per \fBlistinvoice\fR\&. .SH "AUTHOR" .sp Christian Decker is mainly responsible\&. diff --git a/doc/lightningd-config.5 b/doc/lightningd-config.5 index 3b2ded246e9e..38f3f6d3c579 100644 --- a/doc/lightningd-config.5 +++ b/doc/lightningd-config.5 @@ -2,12 +2,12 @@ .\" Title: lightningd-config .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 08/15/2018 +.\" Date: 09/07/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNINGD\-CONFIG" "5" "08/15/2018" "\ \&" "\ \&" +.TH "LIGHTNINGD\-CONFIG" "5" "09/07/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -36,7 +36,7 @@ lightningd-config \- Lightning daemon configuration file .sp lightningd(8) reads a configuration file called \fIconfig\fR, if it exists, when it starts up\&. The location of this file defaults to \fB\&.lightning\fR in the home directory, but can be overridden by the \fI\-\-lightning\-dir\fR option on the lightningd(8) command line\&. .sp -Configuration file options are processsed first, then command line options: later options override earlier ones except \fIaddr\fR options which accumulate\&. +Configuration file options are processed first, then command line options: later options override earlier ones except \fIaddr\fR options which accumulate\&. .sp All these options are mirrored as commandline arguments to lightningd(8), so \fI\-\-foo\fR becomes simply \fIfoo\fR in the configuration file, and \fI\-\-foo=bar\fR becomes \fIfoo=bar\fR in the configuration file\&. .sp @@ -316,7 +316,7 @@ or \*(AqTORIPADDRESS\*(Aq\&. .PP \fBbind\-addr\fR=\fI[IPADDRESS[:PORT]]|SOCKETPATH\fR .RS 4 -Set an IP address or UNIX domain socket to listen to, but do not announce\&. A UNIX domain socket is distingished from an IP address by beginning with a +Set an IP address or UNIX domain socket to listen to, but do not announce\&. A UNIX domain socket is distinguished from an IP address by beginning with a \fI/\fR\&. .sp .if n \{\ @@ -436,7 +436,7 @@ to authenticate to the Tor control port\&. .RE .SH "BUGS" .sp -You should report bugs on our github issues page, and maybe submit a fix to gain our eternal gratutide! +You should report bugs on our github issues page, and maybe submit a fix to gain our eternal gratitude! .SH "AUTHOR" .sp Rusty Russell wrote this man page, and much of the configuration language, but many others did the hard work of actually implementing these options\&.