Implement IRC peer discovery

[cdecker]

Implements a simple IRC client on top of io_loop and uses it to announce existing channels periodically and collect announced channels to get a local view of the network's topology.

[rustyrussell]

Christian Decker notifications@github.com writes:

Implements a simple IRC client on top of io_loop and uses it to announce existing channels periodically and collect announced channels to get a local view of the network's topology.
You can view, comment on, or merge this pull request online at:

Heh, it was easier to create a big diff then do this via email.
Old-school :)

I'm nit-picking deliberately here, so feel free to ignore any of this
advice...

diff --git a/bitcoin/signature.c b/bitcoin/signature.c
index 001ec2b..e3561a9 100644
--- a/bitcoin/signature.c
+++ b/bitcoin/signature.c
@@ -251,6 +251,12 @@ size_t signature_to_der(secp256k1_context *secpctx,
return len;
}

+void signature_from_der(secp256k1_context *secpctx,

•       u8 *der, size_t len, struct signature *sig)
  

  +{
• secp256k1_ecdsa_signature_parse_der(secpctx, &sig->sig, der, len);
  +}

Needs to return bool, and der should be const.

+struct txlocator *locate_tx(struct lightningd_state *dstate,

•         const struct sha256_double *txid)
  

  +{
• struct block *block = block_for_tx(dstate, txid);
• if (block == NULL) {

• return NULL;
  

• }

Adding a new struct is one way, but do want block height rather than
block id. Funding transactions should be beyond reorgs anyway, so it's
a convenient shorthand.

• struct txlocator *loc = talz(dstate, struct txlocator);
• loc->blkid = talz(dstate, struct sha256_double);
• memcpy(loc->blkid, &block->blkid, sizeof(struct sha256_double));
  +
• size_t i, n = tal_count(block->txids);
• for (i = 0; i < n; i++) {

• if (structeq(&block->txids[i], txid)){
  

•     loc->index = i;
  

•     break;
  

• }
  

• }
• return loc;
  +}
  +
  void setup_topology(struct lightningd_state *dstate)
  {
  dstate->topology = tal(dstate, struct topology);
  diff --git a/daemon/chaintopology.h b/daemon/chaintopology.h
  index 815c8a1..d99cc2b 100644
  --- a/daemon/chaintopology.h
  +++ b/daemon/chaintopology.h
  @@ -10,6 +10,16 @@ struct peer;
  struct sha256_double;
  struct txwatch;

+/* Information relevant to locating a TX in a blockchain. */
+struct txlocator {
+

• /* The id of the block that includes this transaction */

• struct sha256_double *blkid;
  +

• /* Position of the transaction in the transactions list */

• u32 index;
  +};
  +
  /* This is the number of blocks which would have to be mined to invalidate

  • the tx. */
    size_t get_tx_depth(struct lightningd_state *dstate,
    @@ -33,4 +43,6 @@ void broadcast_tx(struct peer *peer, const struct bitcoin_tx *tx);

  void setup_topology(struct lightningd_state *dstate);

+struct txlocator locate_tx(struct lightningd_state *dstate, const struct sha256_double *txid);
+
#endif / LIGHTNING_DAEMON_CRYPTOPKT_H */
diff --git a/daemon/lightningd.c b/daemon/lightningd.c
index 4f00d3b..6d7e722 100644
--- a/daemon/lightningd.c
+++ b/daemon/lightningd.c
@@ -145,6 +145,9 @@ static void config_register_opts(struct lightningd_state *dstate)
dstate,
"Add route of form srcid/dstid/base/var/delay/minblocks"
"(base in millisatoshi, var in millionths of satoshi per satoshi)");

• opt_register_noarg("--disable-irc", opt_set_bool,

•        &dstate->config.disable_irc,
  

•        "Disable IRC peer discovery for routing");
  

  }

Option that's negative is fine, but perhaps name it
'dstate->config.use_irc' opt_set_invbool?

@@ -210,6 +213,9 @@ static void default_config(struct config config)
config->fee_base = 546000;
/ Take 0.001% */
config->fee_per_satoshi = 10;
+

• /* Don't discover new peers */
• config->disable_irc = false;
  }

Yeah, this comment seems confused, so I think this justifies not
using negative options :)

+static void announce_channels(struct ircstate *state)
+{

• tal_t *ctx = tal(NULL, tal_t);

Even for tmp vars, I would alloc off something (state in this case).
Makes it a bit more trackable if it leaks.

• struct peer *p;
  +
• list_for_each(&state->dstate->peers, p, list) {
  +

• if (!state_is_normal(p->state))
  

•     continue;
  

• struct txlocator *loc = locate_tx(state->dstate, &p->anchor.txid);
  

• // Did we get a valid locator? If not, do not announce.
  

• if (loc == NULL)
  

•     continue;
  

Perhaps the following should be inside irc.c? Or a new irc_announce.c?
That will make surgery less complex when we replace it?

So have routing control timing, etc (have this take struct dstate), and
have the irc-specific stuff in irc_announce_channel(dstate->irc,
&p->anchor.txid, loc->blkid)?

• struct signature sig;
  

•     u8 der[72];
  

• char hexder[2*sizeof(der) + 1];
  

• memset(der, 0, sizeof(der));
  

• privkey_sign(state->dstate, msg->msg, strlen(msg->msg), &sig);
  

• int numbytes = signature_to_der(state->dstate->secpctx, der, &sig);
  

• hex_encode(der, numbytes, hexder, sizeof(hexder));
  

• msg->msg = tal_fmt(msg, "%s %s", hexder, msg->msg);
  

I'd tend to use tal_hexstr() for hexder here...

• irc_send_msg(state, msg);
  

• tal_free(loc);
  

Since we have a temp context, perhaps make locate_tx take that as first
arg (const void *ctx), then you don't need to free anything until the end:

• }
• tal_free(ctx);
  +
• // Schedule next iteration
• new_reltimer(state->dstate, state, time_from_sec(60), announce_channels, state);

+// Reconnect to IRC server upon disconnection.
+// TODO(cdecker) This should ideally have some backoff to avoid hammering
+// the server.

I use FIXME for my greps, might as well keep them all together.

+static void handle_irc_disconnect(struct ircstate *state)
+{

• new_reltimer(state->dstate, state, state->reconnect_timeout, irc_connect, state);
  +}
  +
  +static void handle_irc_privmsg(struct ircstate *istate, const struct privmsg *msg)
  +{
• // Strip the initial ':" and split it.
• char **splits = tal_strsplit(msg, msg->msg + 1, " ", STR_NO_EMPTY);

Again, I think irc_announce.c should have the irc-specific code in it.

• // Ignore what cannot be channel announcements
• if (tal_count(splits) != 11 || streq(splits[1], "CHAN"))

• return;
  
• // Attempt to parse the signature
• int siglen = strlen(splits[0])/2;

A comment on the expected format would help me read this :)

Also, there's hex_str_size() and hex_data_size() which make this a bit
clearer.

• u8 der[72];
• if(!hex_decode(splits[0], strlen(splits[0]), der, siglen))

• return;
  

• struct signature sig;
• struct sha256_double hash;
• char *content = strchr(msg->msg, ' ') + 1;
• signature_from_der(istate->dstate->secpctx, der, siglen, &sig);
• sha256_double(&hash, content, strlen(content));
• splits++;
  +
• struct pubkey *pk1 = talz(msg, struct pubkey);
• struct pubkey *pk2 = talz(msg, struct pubkey);
• struct sha256_double *blkid = talz(msg, struct sha256_double);
• struct sha256_double *txid = talz(msg, struct sha256_double);
• int index;
  +
• bool ok = true;
• ok &= pubkey_from_hexstr(istate->dstate->secpctx, splits[1], strlen(splits[1]), pk1);
• ok &= pubkey_from_hexstr(istate->dstate->secpctx, splits[2], strlen(splits[2]), pk2);
• ok &= bitcoin_txid_from_hex(splits[3], strlen(splits[3]), txid);
• ok &= bitcoin_blkid_from_hex(splits[4], strlen(splits[4]), blkid);
• index = atoi(splits[5]);
• if (!ok || index < 0) {

• // Something went wrong...
  

• return;
  

log_debug would help here...

• }
• if(!check_signed_hash(istate->dstate->secpctx, &hash, &sig, pk1)){

• log_debug(istate->log,
  

•       "Ignoring announcement from %s, signature check failed.",
  

•       splits[1]);
  

• return;
  

• }
  +
• // TODO(cdecker) Check in topology that the tx is in the block and that
• // the endpoints match.
  +
• add_connection(istate->dstate, pk1, pk2, atoi(splits[6]),

•        atoi(splits[7]), atoi(splits[8]), 6);
  

  +}
  +
  +void setup_irc_connection(struct lightningd_state *dstate)
  +{
• // Register callback
• irc_privmsg_cb = *handle_irc_privmsg;
• irc_disconnect_cb = *handle_irc_disconnect;
  +
• struct ircstate *state = talz(NULL, struct ircstate);
• state->dstate = dstate;

I would point the other way, and have a 'struct ircstate *irc' in struct
lightningd_state.

Also, dstate should be the owner (talz(dstate, struct ircstate)).

• state->server = "irc.freenode.net";

• state->reconnect_timeout = time_from_sec(15);

• state->log = new_log(state, state->dstate->log_record, "%s:irc",

•          log_prefix(state->dstate->base_log));
  

• // Truncate nick at 13 bytes, would be imposed by freenode anyway

• state->nick = tal_fmt(

• state,
  

• "N%.12s",
  

• pubkey_to_hexstr(state, dstate->secpctx, &dstate->id) + 1);
  

• irc_connect(state);

• announce_channels(state);
  +}
  diff --git a/daemon/routing.h b/daemon/routing.h
  index 8aac6ac..6297d2b 100644
  --- a/daemon/routing.h
  +++ b/daemon/routing.h
  @@ -2,6 +2,7 @@
  #define LIGHTNING_DAEMON_ROUTING_H
  #include "config.h"
  #include "bitcoin/pubkey.h"
  +#include "irc.h"

  #define ROUTING_MAX_HOPS 20

@@ -63,4 +64,7 @@ struct node_map *empty_node_map(struct lightningd_state *dstate);

char *opt_add_route(const char *arg, struct lightningd_state *dstate);

+// Main entrypoint for the lightning daemon
+void setup_irc_connection(struct lightningd_state dstate);
+
#endif / LIGHTNING_DAEMON_ROUTING_H _/
diff --git a/irc.c b/irc.c
new file mode 100644
index 0000000..95e193a
--- /dev/null
+++ b/irc.c
@@ -0,0 +1,185 @@
+#include "irc.h"
+#include "daemon/dns.h"
+#include "daemon/log.h"
+
+void (_irc_privmsg_cb)(struct ircstate _, const struct privmsg *) = NULL;
+void (_irc_disconnect_cb)(struct ircstate *) = NULL;
+
+static struct io_plan *irc_connected(struct io_conn *conn, struct lightningd_state *dstate, struct ircstate *state);
+static void irc_disconnected(struct io_conn *conn, struct ircstate *state);
+
+void irc_send_msg(struct ircstate *state, struct privmsg *m)
+{

• irc_send(state, "PRIVMSG", "%s :%s", m->channel, m->msg);
  +}
  +
  +// Send a raw irccommand to the IRC server.
  +void irc_send(struct ircstate *state, const char *command, const char *fmt, ...)
  +{
• va_list ap;
• struct irccommand *c = tal(state, struct irccommand);
  +
• c->prefix = NULL;
  +
• if (!state->connected)

• return;
  
• // Avoid buffering if we are not connected.
• if (state->conn == NULL)

• return;
  
• va_start(ap, fmt);
• c->command = tal_strdup(c, command);
• c->params = tal_vfmt(c, fmt, ap);

You need a 'va_end(ap);' here. (Not on Linux, but portably).

• list_add_tail(&state->writequeue, &c->list);
• io_wake(state);
  +}

Maybe make this return bool, to show if it queued something?

+// Write buffered irccommands to the IRC connection. Commands can be
+// buffered using irc_send.
+static struct io_plan *irc_write_loop(struct io_conn *conn, struct ircstate *state)
+{

• if (state->writebuffer != NULL)

• state->writebuffer = tal_free(state->writebuffer);
  

tal_free(NULL) is perfectly OK, so you can skip the if().

• // Attempt to get a new element from the writequeue
• struct irccommand *m = list_pop(&state->writequeue, struct irccommand, list);
  +
• if (m == NULL)

• return io_out_wait(conn, state, irc_write_loop, state);
  
• bool hasprefix = m->prefix == NULL;
  +
• // Serialize message
• state->writebuffer = tal_fmt(

• state, "%s%s%s %s\r\n",
  

• hasprefix ? "" : m->prefix,
  

• hasprefix ? "" : " ",
  

• m->command,
  

• m->params);
  
• tal_free(m);
  +
• log_debug(state->log, "Sending: "%s"", state->writebuffer);
  +
• return io_write(

•        conn,
  

•        state->writebuffer, strlen(state->writebuffer),
  

•        irc_write_loop, state
  

•        );
  

  +}
  +
  +// Called by the read loop to handle individual lines. This splits the
  +// line into a struct irccommand and passes it on to the specific
  +// handlers for the irccommand type. It silently drops any irccommand that
  +// has an unhandled type.
  +static void handle_irc_command(struct ircstate *state, const char *line)
  +{
• log_debug(state->log, "Received: "%s"", line);
  +
• // Parse the message
• struct irccommand *m = talz(state, struct irccommand);
• char** splits = tal_strsplit(m, line, " ", STR_NO_EMPTY);
  +
• int numsplits = tal_count(splits) - 1;
  +
• if (numsplits > 2 && strstarts(splits[0], ":")) {

• m->prefix = splits[0];
  

• splits++;
  

• }
• m->command = splits[0];
• m->params = tal_strjoin(m, splits + 1, " ", STR_NO_TRAIL);
  +
• if (streq(m->command, "PING")) {

• irc_send(state, "PONG", "%s", tal_steal(state, m->params));
  

You don't want to tal_steal() here, since that will leak. irc_send()
does a copy anyway.

• } else if (streq(m->command, "PRIVMSG")) {

• // TODO parse privmsg and pass to callback (if set)
  

• struct privmsg *pm = talz(m, struct privmsg);
  

• pm->sender = m->prefix;
  

• pm->channel = splits[1];
  

• pm->msg = tal_strjoin(m, &splits[2], " ", STR_NO_TRAIL);
  

OK, minor gripe. I use &arr[n] if I'm passing just that element, and
arr + n if I'm passing the entire array from that point. Of course,
there's no distinction in C, but that's how I think about it.

• irc_privmsg_cb(state, pm);
  

• }
• tal_free(m);
  +}
  +
  +// Read incoming data and split it along the newline boundaries. Takes
  +// care of buffering incomplete lines and passes the lines to the
  +// handle_irc_command handler.
  +static struct io_plan *irc_read_loop(struct io_conn *conn, struct ircstate *state)
  +{
  +
• size_t len = state->readlen + state->buffered;
• char *start = state->buffer, *end;
  +
• while ((end = memchr(start, '\n', len)) != NULL) {

• // Strip "\r\n" from lines.
  

• const char *line = tal_strndup(state, start, end - 1 - start);
  

• handle_irc_command(state, line);
  

• tal_free(line);
  

• len -= (end + 1 - start);
  

• start = end + 1;
  

• }
  +
• /* Move any partial data back down. */
• memmove(state->buffer, start, len);
• state->buffered = len;
  +
• return io_read_partial(conn, state->buffer + state->buffered,

•            sizeof(state->buffer) - state->buffered,
  

•            &state->readlen, irc_read_loop, state);
  

  +}
  +
  +static void irc_failed(struct lightningd_state *dstate, struct ircstate *state)
  +{
• irc_disconnected(state->conn, state);
• state->connected = false;
  +}
  +
  +static void irc_disconnected(struct io_conn *conn, struct ircstate *state)
  +{
• log_debug(state->log, "Lost connection to IRC server");
• state->connected = false;
• state->conn = NULL;
• state->readlen = 0;
• state->buffered = 0;
• memset(state->buffer, 0, sizeof(state->buffer));
  +
• // Clear any pending commands, they're no longer useful
• while (!list_empty(&state->writequeue))

• tal_free(list_pop(&state->writequeue, struct irccommand, list));
  
• // Same goes for partially written commands
• if (state->writebuffer != NULL)

• state->writebuffer = tal_free(state->writebuffer);
  

Again with the unnecessary NULL check...

• if (irc_disconnect_cb != NULL)

• irc_disconnect_cb(state);
  

  +}
  +
  +void irc_connect(struct ircstate *state)
  +{
• state->connected = false;
• list_head_init(&state->writequeue);
  +
• log_debug(state->log, "Connecting to IRC server %s", state->server);
• dns_resolve_and_connect(state->dstate, state->server, "6667", irc_connected, irc_failed, state);
  +}
  +
  +static struct io_plan *irc_connected(struct io_conn *conn, struct lightningd_state *dstate, struct ircstate *state)
  +{
• io_set_finish(conn, irc_disconnected, state);
• state->conn = conn;
• state->connected = true;
• irc_send(state, "USER", "%s 0 * :A lightning node", state->nick);
• irc_send(state, "NICK", "%s", state->nick);
• irc_send(state, "JOIN", "#lightning-nodes");
  +
• return io_duplex(conn,

•      io_read_partial(conn,
  

•              state->buffer, sizeof(state->buffer),
  

•              &state->readlen, irc_read_loop, state),
  

•      irc_write_loop(conn, state));
  

  +}
  diff --git a/irc.h b/irc.h
  new file mode 100644
  index 0000000..f7bcc24
  --- /dev/null
  +++ b/irc.h
  @@ -0,0 +1,67 @@
  +#ifndef LIGHTNING_IRC_H
  +#define LIGHTNING_IRC_H
  +
  +#include <stdio.h>
  +#include <sys/socket.h>
  +#include <netdb.h>
  +#include <ccan/io/io.h>
  +#include <ccan/short_types/short_types.h>
  +#include <ccan/str/str.h>
  +#include <ccan/tal/str/str.h>
  +#include <ccan/time/time.h>
  +#include <ccan/timer/timer.h>
  +
  +#include "daemon/lightningd.h"
  +
  +struct irccommand {
• struct list_node list;
• const char *prefix;
• const char *command;
• const char *params;
  +};
  +
  +struct privmsg {
• const char *channel;
• const char *sender;
• const char *msg;
  +};
  +
  +struct ircstate {
• // Meta information
• const char *nick;
• const char *server;
  +
• // Connection and reading
• struct io_conn *conn;
• char buffer[512];
• size_t readlen;
• size_t buffered;
  +
• // Write queue related
• struct list_head writequeue;
• char *writebuffer;
  +
• /* Pointer to external state, making it available to callbacks */
• struct lightningd_state *dstate;
  +
• struct log *log;
  +
• /* Are we currently connected? */
• bool connected;
  +
• /* Time to wait after getting disconnected before reconnecting. */
• struct timerel reconnect_timeout;
  +};
  +
  +// Callback to register for incoming messages
  +extern void (_irc_privmsg_cb)(struct ircstate *, const struct privmsg *);
  +extern void (_irc_disconnect_cb)(struct ircstate );
  +
  +// Send messages to IRC
  +void irc_send(struct ircstate *state, const char *command, const char *fmt, ...) PRINTF_FMT(3,4);
  +void irc_send_msg(struct ircstate *state, struct privmsg *m);
  +
  +// Register IRC connection with io
  +void irc_connect(struct ircstate *state);
  +
  +#endif / LIGHTNING_IRC_H */

We should eventually make this a CCAN module, but that's a fair bit more
work.

diff --git a/test/test_irc.c b/test/test_irc.c
new file mode 100644
index 0000000..4a92469
--- /dev/null
+++ b/test/test_irc.c
@@ -0,0 +1,65 @@
+#include "irc.h"
+
+static void schedule_timer(struct timers *timers)
+{

• struct timer *t = talz(timers, struct timer);
  +
• timer_init(t);
• timer_add(timers, t, timeabs_add(time_now(), time_from_sec(10)));
  +}
  +
  +static void send_message(struct timers *timers, struct ircstate *state)
  +{
• struct privmsg msg;
  +
• msg.channel = "#lightning-nodes";
• msg.msg = "Still alive and kicking";
• irc_send_msg(state, &msg);
• schedule_timer(timers);
  +}
  +
  +static void reconnect(struct ircstate *state)
  +{
• printf("Disconnected, reconnecting\n");
• irc_connect(state);
  +}
  +
  +static void handle_privmsg(struct ircstate *state, const struct privmsg *msg)
  +{
• printf("%s %s > %s\n", msg->channel, msg->sender, msg->msg);
  +}
  +
  +// Tiny test program to test IRC connectivity
  +int main(void)
  +{
• struct addrinfo *addrinfo, hints;
  +
• memset(&hints, 0, sizeof(hints));
• hints.ai_family = AF_UNSPEC;
• hints.ai_socktype = SOCK_STREAM;
• hints.ai_flags = AI_PASSIVE;
• hints.ai_protocol = 0;
  +
• getaddrinfo("irc.freenode.net", "6667", &hints, &addrinfo);
  +
• struct ircstate *state = talz(NULL, struct ircstate);
• state->addr = addrinfo;
• state->nick = "l2-node-123";
  +
• irc_privmsg_cb = *handle_privmsg;
• irc_disconnect_cb = *reconnect;
  +
• irc_connect(state);
  +
• struct timers *timers = talz(NULL, struct timers);
• timers_init(timers, time_now());
• schedule_timer(timers);
  +
• for (;; ) {

• struct timer *expired;
  

• io_loop(timers, &expired);
  

• if (expired)
  

•     send_message(timers, state);
  

• }
• return 0;
  +}

Thanks!
Rusty.

[cdecker]

Thanks for the detailed feedback Rusty, I should have addressed most of your comments with the last two pushes. The IRC specific things in routing.c have been moved to irc_announce.{h,c} so that routing.c now is almost untouched.

One thing I did not yet address is the ircstate -> lightningd_state inversion, since that'd make the IRC module dependent on lightningd, which I'm trying to avoid. I'll have a look at how io_loop handles callbacks with custom argument types and try to adapt it so that we can register the handlers to accept lightningd_state as arguments. That way we can definitely keep the IRC module separate and eventually port it to CCAN :-)