From b75ada701759773ce68ff3bc60094e11cc66b300 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 3 Jan 2023 14:53:28 +1030
Subject: [PATCH] commando: track incoming and outgoing JSON IDs.  Get upset if
 they don't match!

They currently don't, so we get some BROKEN messages.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 plugins/commando.c   | 25 +++++++++++++++++++++++--
 tests/test_plugin.py | 10 +++++++---
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/plugins/commando.c b/plugins/commando.c
index 8baaf999f..0e4a8b3ba 100644
--- a/plugins/commando.c
+++ b/plugins/commando.c
@@ -4,6 +4,7 @@
 #include <ccan/htable/htable_type.h>
 #include <ccan/json_escape/json_escape.h>
 #include <ccan/json_out/json_out.h>
+#include <ccan/mem/mem.h>
 #include <ccan/rune/rune.h>
 #include <ccan/tal/str/str.h>
 #include <ccan/time/time.h>
@@ -36,6 +37,9 @@ struct commando {
 
 	/* This is set to NULL if they seem to be spamming us! */
 	u8 *contents;
+
+	/* Literal JSON token containing JSON id (including "") */
+	const char *json_id;
 };
 
 static struct plugin *plugin;
@@ -403,10 +407,15 @@ static void try_command(struct node_id *peer,
 			return;
 		}
 		cmdid_prefix = NULL;
+		incoming->json_id = NULL;
 	} else {
 		cmdid_prefix = tal_fmt(tmpctx, "%.*s/",
 				       id->end - id->start,
 				       buf + id->start);
+		/* Includes quotes, if any! */
+		incoming->json_id = tal_strndup(incoming,
+						json_tok_full(buf, id),
+						json_tok_full_len(id));
 	}
 
 	failmsg = check_rune(tmpctx, incoming, peer, buf, method, params, rune);
@@ -510,7 +519,7 @@ static struct command_result *handle_reply(struct node_id *peer,
 {
 	struct commando *ocmd;
 	struct json_stream *res;
-	const jsmntok_t *toks, *result, *err;
+	const jsmntok_t *toks, *result, *err, *id;
 	const char *replystr;
 	size_t i;
 	const jsmntok_t *t;
@@ -542,6 +551,17 @@ static struct command_result *handle_reply(struct node_id *peer,
 				    "Reply was unparsable: '%.*s'",
 				    (int)tal_bytelen(ocmd->contents), replystr);
 
+	id = json_get_member(replystr, toks, "id");
+
+	/* Old commando didn't reply with id, but newer should get it right! */
+	if (id && !memeq(json_tok_full(replystr, id), json_tok_full_len(id),
+			 ocmd->json_id, strlen(ocmd->json_id))) {
+		plugin_log(plugin, LOG_BROKEN, "Commando reply with wrong id:"
+			   " I sent %s, they replied with %.*s!",
+			   ocmd->json_id,
+			   json_tok_full_len(id), json_tok_full(replystr, id));
+	}
+
 	err = json_get_member(replystr, toks, "error");
 	if (err) {
 		const jsmntok_t *code = json_get_member(replystr, err, "code");
@@ -682,6 +702,7 @@ static struct command_result *json_commando(struct command *cmd,
 	ocmd->cmd = cmd;
 	ocmd->peer = *peer;
 	ocmd->contents = tal_arr(ocmd, u8, 0);
+	ocmd->json_id = tal_strdup(ocmd, cmd->id);
 	do {
 		ocmd->id = pseudorand_u64();
 	} while (find_commando(outgoing_commands, NULL, &ocmd->id));
@@ -691,7 +712,7 @@ static struct command_result *json_commando(struct command *cmd,
 	/* We pass through their JSON id untouched. */
 	json = tal_fmt(tmpctx,
 		       "{\"method\":\"%s\",\"id\":%s,\"params\":%s", method,
-		       cmd->id, cparams ? cparams : "{}");
+		       ocmd->json_id, cparams ? cparams : "{}");
 	if (rune)
 		tal_append_fmt(&json, ",\"rune\":\"%s\"", rune);
 	tal_append_fmt(&json, "}");
diff --git a/tests/test_plugin.py b/tests/test_plugin.py
index c2d06580c..822744329 100644
--- a/tests/test_plugin.py
+++ b/tests/test_plugin.py
@@ -2609,7 +2609,9 @@ def test_plugin_shutdown(node_factory):
 
 def test_commando(node_factory, executor):
     l1, l2 = node_factory.line_graph(2, fundchannel=False,
-                                     opts={'log-level': 'io'})
+                                     opts={'log-level': 'io',
+                                           # FIXME: Currently, our JSON ids in replies are wrong, hence BROKEN!
+                                           'allow_broken_log': True})
 
     # Nothing works until we've issued a rune.
     fut = executor.submit(l2.rpc.call, method='commando',
@@ -2704,7 +2706,8 @@ def test_commando(node_factory, executor):
 
 
 def test_commando_rune(node_factory):
-    l1, l2 = node_factory.get_nodes(2)
+    # FIXME: Currently, our JSON ids in replies are wrong, hence BROKEN!
+    l1, l2 = node_factory.get_nodes(2, opts={'allow_broken_log': True})
 
     # Force l1's commando secret
     l1.rpc.datastore(key=['commando', 'secret'], hex='1241faef85297127c2ac9bde95421b2c51e5218498ae4901dc670c974af4284b')
@@ -2939,7 +2942,8 @@ def test_commando_rune(node_factory):
 
 def test_commando_stress(node_factory, executor):
     """Stress test to slam commando with many large queries"""
-    nodes = node_factory.get_nodes(5)
+    # FIXME: Currently, our JSON ids in replies are wrong, hence BROKEN!
+    nodes = node_factory.get_nodes(5, opts={'allow_broken_log': True})
 
     rune = nodes[0].rpc.commando_rune()['rune']
     for n in nodes[1:]: