channeld: restore ping command, but only for channeld.

It's probably not worth fixing for the other daemons.

Changelog-Changed: JSON-RPC: `ping` now only works if we have a channel with the peer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/channeld.c

@@ -49,6 +49,15 @@
 #define MASTER_FD STDIN_FILENO
 #define HSM_FD 6
 
+enum pong_expect_type {
+	/* We weren't expecting a ping reply */
+	PONG_UNEXPECTED = 0,
+	/* We were expecting a ping reply due to ping command */
+	PONG_EXPECTED_COMMAND = 1,
+	/* We were expecting a ping reply due to ping timer */
+	PONG_EXPECTED_PROBING = 2,
+};
+
 struct peer {
 	struct per_peer_state *pps;
 	bool funding_locked[NUM_SIDES];
@@ -104,7 +113,7 @@ struct peer {
 	struct oneshot *ping_timer;
 
 	/* Are we expecting a pong? */
-	bool expecting_pong;
+	enum pong_expect_type expecting_pong;
 
 	/* The feerate we want. */
 	u32 desired_feerate;
@@ -1100,13 +1109,13 @@ static void set_ping_timer(struct peer *peer)
 static void send_ping(struct peer *peer)
 {
 	/* Already have a ping in flight? */
-	if (peer->expecting_pong) {
+	if (peer->expecting_pong != PONG_UNEXPECTED) {
 		status_debug("Last ping unreturned: hanging up");
 		exit(0);
 	}
 
 	sync_crypto_write_no_delay(peer->pps, take(make_ping(NULL, 1, 0)));
-	peer->expecting_pong = true;
+	peer->expecting_pong = PONG_EXPECTED_PROBING;
 	set_ping_timer(peer);
 }
 
@@ -2138,6 +2147,29 @@ static void handle_unexpected_reestablish(struct peer *peer, const u8 *msg)
 				       &channel_id));
 }
 
+static void handle_ping_reply(struct peer *peer, const u8 *msg)
+{
+	u8 *ignored;
+	size_t i;
+
+	/* We print this out because we asked for pong, so can't spam us... */
+	if (!fromwire_pong(msg, msg, &ignored))
+		status_unusual("Got malformed ping reply %s",
+			       tal_hex(tmpctx, msg));
+
+	/* We print this because dev versions of c-lightning embed
+	 * version here: see check_ping_make_pong! */
+	for (i = 0; i < tal_count(ignored); i++) {
+		if (ignored[i] < ' ' || ignored[i] == 127)
+			break;
+	}
+	status_debug("Got pong %zu bytes (%.*s...)",
+		     tal_count(ignored), (int)i, (char *)ignored);
+	wire_sync_write(MASTER_FD,
+			take(towire_channeld_ping_reply(NULL, true,
+							tal_bytelen(msg))));
+}
+
 static void peer_in(struct peer *peer, const u8 *msg)
 {
 	enum peer_wire type = fromwire_peektype(msg);
@@ -2231,12 +2263,18 @@ static void peer_in(struct peer *peer, const u8 *msg)
 	case WIRE_ACK_RBF:
 		break;
 	case WIRE_PONG:
-		if (peer->expecting_pong) {
-			peer->expecting_pong = false;
+		switch (peer->expecting_pong) {
+		case PONG_EXPECTED_COMMAND:
+			handle_ping_reply(peer, msg);
+			/* fall thru */
+		case PONG_EXPECTED_PROBING:
+			peer->expecting_pong = PONG_UNEXPECTED;
 			return;
-		}
+		case PONG_UNEXPECTED:
 			status_debug("Unexpected pong?");
 			return;
+		}
+		abort();
 
 	case WIRE_CHANNEL_REESTABLISH:
 		handle_unexpected_reestablish(peer, msg);
@@ -3509,6 +3547,57 @@ static void handle_send_error(struct peer *peer, const u8 *msg)
 			take(towire_channeld_send_error_reply(NULL)));
 }
 
+static void handle_send_ping(struct peer *peer, const u8 *msg)
+{
+	u8 *ping;
+	u16 len, num_pong_bytes;
+
+	if (!fromwire_channeld_ping(msg, &num_pong_bytes, &len))
+		master_badmsg(WIRE_CHANNELD_PING, msg);
+
+	/* We're not supposed to send another ping until previous replied */
+	if (peer->expecting_pong != PONG_UNEXPECTED) {
+		wire_sync_write(MASTER_FD,
+				take(towire_channeld_ping_reply(NULL, false, 0)));
+		return;
+	}
+
+	/* It should never ask for an oversize ping. */
+	ping = make_ping(NULL, num_pong_bytes, len);
+	if (tal_count(ping) > 65535)
+		status_failed(STATUS_FAIL_MASTER_IO, "Oversize ping");
+
+	sync_crypto_write_no_delay(peer->pps, take(ping));
+
+	/* Since we're doing this manually, kill and restart timer. */
+	status_debug("sending ping expecting %sresponse",
+		     num_pong_bytes >= 65532 ? "no " : "");
+
+	/* BOLT #1:
+	 *
+	 * A node receiving a `ping` message:
+	 *...
+	 *  - if `num_pong_bytes` is less than 65532:
+	 *    - MUST respond by sending a `pong` message, with `byteslen` equal
+	 *      to `num_pong_bytes`.
+	 *  - otherwise (`num_pong_bytes` is **not** less than 65532):
+	 *    - MUST ignore the `ping`.
+	 */
+	if (num_pong_bytes >= 65532) {
+		wire_sync_write(MASTER_FD,
+				take(towire_channeld_ping_reply(NULL,
+								true, 0)));
+		return;
+	}
+
+	/* We'll respond to lightningd once the pong comes in */
+	peer->expecting_pong = PONG_EXPECTED_COMMAND;
+
+	/* Restart our timed pings now. */
+	tal_free(peer->ping_timer);
+	set_ping_timer(peer);
+}
+
 #if DEVELOPER
 static void handle_dev_reenable_commit(struct peer *peer)
 {
@@ -3607,6 +3696,9 @@ static void req_in(struct peer *peer, const u8 *msg)
 	case WIRE_CHANNELD_SEND_ERROR:
 		handle_send_error(peer, msg);
 		return;
+	case WIRE_CHANNELD_PING:
+		handle_send_ping(peer, msg);
+		return;
 #if DEVELOPER
 	case WIRE_CHANNELD_DEV_REENABLE_COMMIT:
 		handle_dev_reenable_commit(peer);
@@ -3642,6 +3734,7 @@ static void req_in(struct peer *peer, const u8 *msg)
 	case WIRE_CHANNELD_SEND_ERROR_REPLY:
 	case WIRE_CHANNELD_DEV_QUIESCE_REPLY:
 	case WIRE_CHANNELD_UPGRADED:
+	case WIRE_CHANNELD_PING_REPLY:
 		break;
 	}
 
@@ -3879,7 +3972,7 @@ int main(int argc, char *argv[])
 	status_setup_sync(MASTER_FD);
 
 	peer = tal(NULL, struct peer);
-	peer->expecting_pong = false;
+	peer->expecting_pong = PONG_UNEXPECTED;
 	timers_init(&peer->timers, time_mono());
 	peer->commit_timer = NULL;
 	set_ping_timer(peer);

channeld/channeld_wire.csv

@@ -238,3 +238,14 @@ msgdata,channeld_upgraded,new_type,channel_type,
 # Tell peer about our latest and greatest blockheight.
 msgtype,channeld_blockheight,1012
 msgdata,channeld_blockheight,blockheight,u32,
+
+# Ping/pong test.  Waits for a reply if it expects one.
+msgtype,channeld_ping,1030
+msgdata,channeld_ping,num_pong_bytes,u16,
+msgdata,channeld_ping,len,u16,
+
+msgtype,channeld_ping_reply,1130
+# False if we there was already a ping in progress.
+msgdata,channeld_ping_reply,sent,bool,
+# 0 == no pong expected, otherwise length of pong.
+msgdata,channeld_ping_reply,totlen,u16,

common/ping.c

@@ -66,25 +66,3 @@ u8 *make_ping(const tal_t *ctx, u16 num_pong_bytes, u16 padlen)
 	tal_free(ignored);
 	return ping;
 }
-
-const char *got_pong(const u8 *pong, size_t *num_pings_outstanding)
-{
-	u8 *ignored;
-	int i;
-
-	if (!fromwire_pong(pong, pong, &ignored))
-		return "Bad pong";
-
-	if (*num_pings_outstanding == 0)
-		return "Unexpected pong";
-
-	for (i = 0; i < tal_count(ignored); i++) {
-		if (ignored[i] < ' ' || ignored[i] == 127)
-			break;
-	}
-	status_debug("Got pong %zu bytes (%.*s...)",
-		     tal_count(ignored), i, (char *)ignored);
-
-	(*num_pings_outstanding)--;
-	return NULL;
-}

common/ping.h

@@ -10,7 +10,4 @@ bool check_ping_make_pong(const tal_t *ctx, const u8 *ping, u8 **pong);
 /* Make a ping packet requesting num_pong_bytes */
 u8 *make_ping(const tal_t *ctx, u16 num_pong_bytes, u16 padlen);
 
-/* Returns error string if something wrong. */
-const char *got_pong(const u8 *pong, size_t *num_pings_outstanding);
-
 #endif /* LIGHTNING_COMMON_PING_H */

doc/lightning-ping.7.md

@@ -9,11 +9,15 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-The **ping** command checks if the node with *id* is ready to talk. It accepts the following parameters:
+The **ping** command checks if the node with *id* is ready to talk. 
+It currently only works for peers we have a channel with.
+
+It accepts the following parameters:
 
 - *id*: A string that represents the node id;
 - *len*: A integer that represents the length of the ping (default 128);
 - *pongbytes*: An integer that represents the length of the reply (default 128).
+  A value of 65532 to 65535 means "don't reply".
 
 EXAMPLE JSON REQUEST
 ------------
@@ -39,7 +43,7 @@ On success, an object is returned, containing:
 
 On failure, one of the following error codes may be returned:
 
-- -32602: Error in given parameters or unknown peer.
+- -32602: Error in given parameters or we're already waiting for a ping response from peer.
 
 EXAMPLE JSON RESPONSE
 -----

lightningd/channel_control.c

@@ -19,6 +19,7 @@
 #include <lightningd/hsm_control.h>
 #include <lightningd/notification.h>
 #include <lightningd/peer_control.h>
+#include <lightningd/ping.h>
 #include <wire/common_wiregen.h>
 
 static void update_feerates(struct lightningd *ld, struct channel *channel)
@@ -491,6 +492,9 @@ static unsigned channel_msg(struct subd *sd, const u8 *msg, const int *fds)
 	case WIRE_CHANNELD_SEND_ERROR_REPLY:
 		handle_error_channel(sd->channel, msg);
 		break;
+	case WIRE_CHANNELD_PING_REPLY:
+		ping_reply(sd, msg);
+		break;
 #if EXPERIMENTAL_FEATURES
 	case WIRE_CHANNELD_UPGRADED:
 		handle_channel_upgrade(sd->channel, msg);
@@ -520,6 +524,7 @@ static unsigned channel_msg(struct subd *sd, const u8 *msg, const int *fds)
 	case WIRE_CHANNELD_DEV_MEMLEAK_REPLY:
 	case WIRE_CHANNELD_SEND_ERROR:
 	case WIRE_CHANNELD_DEV_QUIESCE_REPLY:
+	case WIRE_CHANNELD_PING:
 		break;
 	}
 

lightningd/gossip_control.c

@@ -14,7 +14,6 @@
 #include <lightningd/hsm_control.h>
 #include <lightningd/jsonrpc.h>
 #include <lightningd/onion_message.h>
-#include <lightningd/ping.h>
 
 static void got_txout(struct bitcoind *bitcoind,
 		      const struct bitcoin_tx_output *output,

lightningd/ping.c

@@ -1,9 +1,11 @@
+#include <channeld/channeld_wiregen.h>
 #include <common/json_command.h>
 #include <common/json_tok.h>
 #include <common/param.h>
-#include <gossipd/gossipd_wiregen.h>
+#include <lightningd/channel.h>
 #include <lightningd/jsonrpc.h>
 #include <lightningd/lightningd.h>
+#include <lightningd/peer_control.h>
 #include <lightningd/ping.h>
 #include <lightningd/subd.h>
 
@@ -45,33 +47,37 @@ static struct ping_command *new_ping_command(const tal_t *ctx,
 	return pc;
 }
 
-void ping_reply(struct subd *subd, const u8 *msg)
+void ping_reply(struct subd *channeld, const u8 *msg)
 {
-	(void)find_ping_cmd;
-#if 0 /* Disabled until channeld sends us ping info */
 	u16 totlen;
-	bool ok, sent = true;
-	struct node_id id;
+	bool sent;
 	struct ping_command *pc;
+	struct channel *c = channeld->channel;
 
-	log_debug(subd->ld->log, "Got ping reply!");
-	ok = fromwire_gossipd_ping_reply(msg, &id, &sent, &totlen);
+	log_debug(channeld->log, "Got ping reply!");
+	pc = find_ping_cmd(channeld->ld, &c->peer->id);
+	if (!pc) {
+		log_broken(channeld->log, "Unexpected ping reply?");
+		return;
+	}
 
-	pc = find_ping_cmd(subd->ld, &id);
-	assert(pc);
-
-	if (!ok)
+	if (!fromwire_channeld_ping_reply(msg, &sent, &totlen)) {
+		log_broken(channeld->log, "Malformed ping reply %s",
+			   tal_hex(tmpctx, msg));
 		was_pending(command_fail(pc->cmd, LIGHTNINGD,
 					 "Bad reply message"));
-	else if (!sent)
-		was_pending(command_fail(pc->cmd, LIGHTNINGD, "Unknown peer"));
+		return;
+	}
+
+	if (!sent)
+		was_pending(command_fail(pc->cmd, LIGHTNINGD,
+					 "Ping already pending"));
 	else {
 		struct json_stream *response = json_stream_success(pc->cmd);
 
 		json_add_num(response, "totlen", totlen);
 		was_pending(command_success(pc->cmd, response));
 	}
-#endif
 }
 
 static struct command_result *json_ping(struct command *cmd,
@@ -81,6 +87,9 @@ static struct command_result *json_ping(struct command *cmd,
 {
 	unsigned int *len, *pongbytes;
 	struct node_id *id;
+	struct peer *peer;
+	struct channel *channel;
+	u8 *msg;
 
 	if (!param(cmd, buffer, params,
 		   p_req("id", param_node_id, &id),
@@ -114,14 +123,20 @@ static struct command_result *json_ping(struct command *cmd,
 				    "pongbytes %u > 65535", *pongbytes);
 	}
 
+	peer = peer_by_id(cmd->ld, id);
+	if (!peer)
+		return command_fail(cmd, LIGHTNINGD, "Peer not connected");
+
+	channel = peer_active_channel(peer);
+	if (!channel || !channel->owner || channel->state != CHANNELD_NORMAL)
+		return command_fail(cmd, LIGHTNINGD, "Peer bad state");
+
 	/* parent is cmd, so when we complete cmd, we free this. */
 	new_ping_command(cmd, cmd->ld, id, cmd);
 
-#if 0 /* FIXME: make channeld take this message */
-	/* gossipd handles all pinging, even if it's in another daemon. */
-	msg = towire_gossipd_ping(NULL, id, *pongbytes, *len);
-	subd_send_msg(cmd->ld->gossip, take(msg));
-#endif
+	msg = towire_channeld_ping(NULL, *pongbytes, *len);
+	subd_send_msg(channel->owner, take(msg));
+
 	return command_still_pending(cmd);
 }
 

tests/test_misc.py

@@ -253,9 +253,8 @@ def test_lightningd_still_loading(node_factory, bitcoind, executor):
     l1.pay(l2, 1000)
 
 
-@pytest.mark.skip(reason="FIXME: channeld needs to handle pings")
 def test_ping(node_factory):
-    l1, l2 = node_factory.line_graph(2, fundchannel=False)
+    l1, l2 = node_factory.line_graph(2)
 
     def ping_tests(l1, l2):
         # 0-byte pong gives just type + length field.
@@ -284,14 +283,6 @@ def test_ping(node_factory):
         with pytest.raises(RpcError, match=r'oversize ping'):
             l1.rpc.ping(l2.info['id'], 65530, 1)
 
-    # Test gossip pinging.
-    ping_tests(l1, l2)
-    if DEVELOPER:
-        l1.daemon.wait_for_log(r'Got pong 1000 bytes \({}\.\.\.\)'
-                               .format(l2.info['version']), timeout=1)
-
-    l1.fundchannel(l2, 10**5)
-
     # channeld pinging
     ping_tests(l1, l2)
     if DEVELOPER: