Rune handling cleanups, new DEBUG_LIGHTNINGD option

ccan/README

@@ -1,3 +1,3 @@
 CCAN imported from http://ccodearchive.net.
 
-CCAN version: init-2577-g1ae4c432
+CCAN version: init-2578-g29e55f74

ccan/ccan/rune/rune.c

@@ -345,13 +345,17 @@ static const char *rune_alt_single(const tal_t *ctx,
 				 memmem(fieldval_str, fieldval_strlen,
 					alt->value, strlen(alt->value)));
 	case RUNE_COND_INT_LESS:
+		if (!fieldval_str && !fieldval_int)
+			return tal_fmt(ctx, "%s not present", alt->fieldname);
 		err = integer_compare_valid(ctx, fieldval_int,
 					    alt, &runeval_int);
 		if (err)
 			return err;
 		return cond_test(ctx, alt, "is greater or equal to",
 				 *fieldval_int < runeval_int);
 	case RUNE_COND_INT_GREATER:
+		if (!fieldval_str && !fieldval_int)
+			return tal_fmt(ctx, "%s not present", alt->fieldname);
 		err = integer_compare_valid(ctx, fieldval_int,
 					    alt, &runeval_int);
 		if (err)

common/daemon.c

@@ -196,7 +196,7 @@ bool daemon_developer_mode(char *argv[])
 	bool developer = false, debug = false;
 
 	for (int i = 1; argv[i]; i++) {
-		if (streq(argv[i], "--debugger"))
+		if (streq(argv[i], "--dev-debug-self"))
 			debug = true;
 		else if (streq(argv[i], "--developer"))
 			developer = true;

common/daemon.h

@@ -22,7 +22,7 @@ const char *backtrace_symname(const tal_t *ctx, const void *addr);
 void daemon_shutdown(void);
 
 /* If --developer is set, set up extra developer checks, kick in a
- * debugger if they set --debugger, and return true.   */
+ * debugger if they set --dev-debug-self, and return true.   */
 bool daemon_developer_mode(char *argv[]);
 
 #endif /* LIGHTNING_COMMON_DAEMON_H */

contrib/msggen/msggen/schema.json

@@ -4847,7 +4847,7 @@
                 "    * per: how often the rune can be used, with suffix \"sec\" (default), \"min\", \"hour\", \"day\" or \"msec\", \"usec\" or \"nsec\". e.g. \"per=5sec\".",
                 "    * rate: the rate limit, per minute, e.g. \"rate=60\" is equivalent to \"per=1sec\".",
                 "    * pnum: the number of parameters. e.g. \"pnum<2\".",
-                "    * pnameX: the parameter named X (with any punctuation like `_` removed). e.g. \"pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\".",
+                "    * pnameX: the parameter named X e.g. \"pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\".  NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed.",
                 "    * parrN: the N'th parameter. e.g. \"parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\"."
               ],
               "items": {

contrib/pyln-testing/pyln/testing/utils.py

@@ -789,6 +789,8 @@ def __init__(self, node_id, lightning_dir, bitcoind, executor, valgrind, may_fai
             self.daemon.opts["dev-no-version-checks"] = None
         if os.getenv("DEBUG_SUBD"):
             self.daemon.opts["dev-debugger"] = os.getenv("DEBUG_SUBD")
+        if os.getenv("DEBUG_LIGHTNINGD"):
+            self.daemon.opts["dev-debug-self"] = None
         if valgrind:
             self.daemon.env["LIGHTNINGD_DEV_NO_BACKTRACE"] = "1"
             self.daemon.opts["dev-no-plugin-checksum"] = None

doc/contribute-to-core-lightning/testing.md

@@ -41,7 +41,7 @@ There are four kinds of tests:
 
   `PYTHONPATH=contrib/pyln-client:contrib/pyln-testing:contrib/pyln-proto:contrib/pyln-grpc-proto py.test -v tests/`
 
-  You can also append `-k TESTNAME` to run a single test.  Environment variables `DEBUG_SUBD=<subdaemon>` and `TIMEOUT=<seconds>` can be useful for debugging subdaemons on individual tests.
+  You can also append `-k TESTNAME` to run a single test.  Environment variables `DEBUG_SUBD=<subdaemon>` and `TIMEOUT=<seconds>` can be useful for debugging subdaemons on individual tests, and `DEBUG_LIGHTNINGD` for attaching a debugger to each `lightningd` instance created.
 
 - **pylightning tests** - will check contrib pylightning for codestyle and run the tests in `contrib/pylightning/tests` afterwards:
 

doc/schemas/lightning-createrune.json

@@ -35,7 +35,7 @@
               "    * per: how often the rune can be used, with suffix \"sec\" (default), \"min\", \"hour\", \"day\" or \"msec\", \"usec\" or \"nsec\". e.g. \"per=5sec\".",
               "    * rate: the rate limit, per minute, e.g. \"rate=60\" is equivalent to \"per=1sec\".",
               "    * pnum: the number of parameters. e.g. \"pnum<2\".",
-              "    * pnameX: the parameter named X (with any punctuation like `_` removed). e.g. \"pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\".",
+              "    * pnameX: the parameter named X e.g. \"pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\".  NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed.",
               "    * parrN: the N'th parameter. e.g. \"parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T\"."
             ],
             "items": {

lightningd/lightningd.c

@@ -1068,6 +1068,12 @@ int main(int argc, char *argv[])
 
 	trace_span_start("lightningd/startup", argv);
 
+	/*~ What happens in strange locales should stay there. */
+	setup_locale();
+
+	/*~ This handles --dev-debug-self really early, which we otherwise ignore */
+	daemon_developer_mode(argv);
+
 	/*~ We fork out new processes very very often; every channel gets its
 	 * own process, for example, and we have `hsmd` and `gossipd` and
 	 * the plugins as well.
@@ -1106,9 +1112,6 @@ int main(int argc, char *argv[])
 	 */
 	closefrom_limit(4096);
 
-	/*~ What happens in strange locales should stay there. */
-	setup_locale();
-
 	/*~ This sets up SIGCHLD to make sigchld_rfd readable. */
 	sigchld_rfd = setup_sig_handlers();
 

lightningd/options.c

@@ -784,6 +784,11 @@ static char *opt_force_featureset(const char *optarg,
 	return NULL;
 }
 
+static char *opt_ignore(void *unused)
+{
+	return NULL;
+}
+
 static void dev_register_opts(struct lightningd *ld)
 {
 	/* We might want to debug plugins, which are started before normal
@@ -915,6 +920,11 @@ static void dev_register_opts(struct lightningd *ld)
 		     opt_set_bool,
 		     &ld->dev_allow_shutdown_destination_change,
 		     "Allow destination override on close, even if risky");
+	/* This is handled directly in daemon_developer_mode(), so we ignore it here */
+	clnopt_noarg("--dev-debug-self", OPT_DEV,
+		     opt_ignore,
+		     NULL,
+		     "Fire up a terminal window with a debugger in it on initialization");
 }
 
 static const struct config testnet_config = {

lightningd/plugin.c

@@ -1985,7 +1985,7 @@ const char *plugin_send_getmanifest(struct plugin *p, const char *cmd_id)
 	cmd = tal_arr(tmpctx, char *, 1);
 	cmd[0] = p->cmd;
 	if (debugging(p))
-		tal_arr_expand(&cmd, "--debugger");
+		tal_arr_expand(&cmd, "--dev-debug-self");
 	if (p->plugins->ld->developer)
 		tal_arr_expand(&cmd, "--developer");
 	tal_arr_expand(&cmd, NULL);

lightningd/runes.c

@@ -758,12 +758,17 @@ static const char *check_condition(const tal_t *ctx,
 
 		if (cinfo->params->type == JSMN_OBJECT) {
 			json_for_each_obj(i, t, cinfo->params) {
-				char *pmemname = tal_fmt(tmpctx,
+				char *pmemname = tal_fmt(ctx,
 							 "pname%.*s",
 							 t->end - t->start,
 							 cinfo->buf + t->start);
 				size_t off = strlen("pname");
-				/* Remove punctuation! */
+
+				/* First, add version with underscores intact. */
+				strmap_add(&cinfo->cached_params,
+					   tal_strdup(ctx, pmemname), t+1);
+
+				/* Now with punctuation removed: */
 				for (size_t n = off; pmemname[n]; n++) {
 					if (cispunct(pmemname[n]))
 						continue;
@@ -849,6 +854,14 @@ static struct command_result *json_checkrune(struct command *cmd,
 	/* Just in case they manage to make us speak non-JSON, escape! */
 	if (err) {
 		err = json_escape(tmpctx, err)->s;
+
+		/* Turn runespeak into english! */
+		if (strstarts(err, "pname"))
+			err = tal_strcat(tmpctx, "parameter ", err + strlen("pname"));
+		else if (strstarts(err, "parr"))
+			err = tal_strcat(tmpctx, "parameter #", err + strlen("parr"));
+		else if (strstarts(err, "pnum"))
+			err = tal_strcat(tmpctx, "number of parameters", err + strlen("pnum"));
 		return command_fail(cmd, RUNE_NOT_PERMITTED, "Not permitted: %s", err);
 	}
 

lightningd/subd.c

@@ -258,7 +258,7 @@ static int subd(const char *path, const char *name,
 		if (io_logging)
 			args[num_args++] = "--log-io";
 		if (debugging)
-			args[num_args++] = "--debugger";
+			args[num_args++] = "--dev-debug-self";
 		if (developer)
 			args[num_args++] = "--developer";
 		execv(args[0], args);

lightningd/test/run-find_my_abspath.c

@@ -27,6 +27,9 @@ int connectd_init(struct lightningd *ld UNNEEDED)
 /* Generated stub for connectd_start_shutdown */
 void connectd_start_shutdown(struct subd *connectd UNNEEDED)
 { fprintf(stderr, "connectd_start_shutdown called!\n"); abort(); }
+/* Generated stub for daemon_developer_mode */
+bool daemon_developer_mode(char *argv[])
+{ fprintf(stderr, "daemon_developer_mode 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(); }

tests/test_plugin.py

@@ -2939,35 +2939,35 @@ def test_commando_rune_pay_amount(node_factory):
     inv2 = l2.rpc.invoice(amount_msat='any', label='inv2', description='description2')['bolt11']
 
     # Rune requires amount_msat!
-    with pytest.raises(RpcError, match='Invalid rune: Not permitted: pnameamountmsat is not an integer field'):
+    with pytest.raises(RpcError, match='Invalid rune: Not permitted: parameter amountmsat not present'):
         l2.rpc.commando(peer_id=l1.info['id'],
                         rune=rune,
                         method='pay',
                         params={'bolt11': inv1})
 
     # As a named parameter!
-    with pytest.raises(RpcError, match='Invalid rune: Not permitted: pnameamountmsat is not an integer field'):
+    with pytest.raises(RpcError, match='Invalid rune: Not permitted: parameter amountmsat not present'):
         l2.rpc.commando(peer_id=l1.info['id'],
                         rune=rune,
                         method='pay',
                         params=[inv1])
 
     # Can't get around it this way!
-    with pytest.raises(RpcError, match='Invalid rune: Not permitted: pnameamountmsat is not an integer field'):
+    with pytest.raises(RpcError, match='Invalid rune: Not permitted: parameter amountmsat not present'):
         l2.rpc.commando(peer_id=l1.info['id'],
                         rune=rune,
                         method='pay',
                         params=[inv2, 12000])
 
     # Nor this way, using a string!
-    with pytest.raises(RpcError, match='Invalid rune: Not permitted: pnameamountmsat is not an integer field'):
+    with pytest.raises(RpcError, match='Invalid rune: Not permitted: parameter amountmsat is not an integer field'):
         l2.rpc.commando(peer_id=l1.info['id'],
                         rune=rune,
                         method='pay',
                         params={'bolt11': inv2, 'amount_msat': '10000sat'})
 
     # Too much!
-    with pytest.raises(RpcError, match='Invalid rune: Not permitted: pnameamountmsat is greater or equal to 10000'):
+    with pytest.raises(RpcError, match='Invalid rune: Not permitted: parameter amountmsat is greater or equal to 10000'):
         l2.rpc.commando(peer_id=l1.info['id'],
                         rune=rune,
                         method='pay',

tests/test_runes.py

@@ -454,45 +454,45 @@ def test_rune_pay_amount(node_factory):
     # This doesn't really work, since amount_msat is illegal if invoice
     # includes an amount, and runes aren't smart enough to decode bolt11!
     rune = l1.rpc.createrune(restrictions=[['method=pay'],
-                                           ['pnameamountmsat<10000']])['rune']
+                                           ['pnameamount_msat<10000']])['rune']
 
     inv1 = l2.rpc.invoice(amount_msat=12300, label='inv1', description='description1')['bolt11']
     inv2 = l2.rpc.invoice(amount_msat='any', label='inv2', description='description2')['bolt11']
 
     # Rune requires amount_msat < 10,000!
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
+    with pytest.raises(RpcError, match='Not permitted: parameter amount_msat not present') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
                          rune=rune,
                          method='pay',
                          params={'bolt11': inv1})
     assert exc_info.value.error['code'] == 0x5de
 
     # As a named parameter!
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
+    with pytest.raises(RpcError, match='Not permitted: parameter amount_msat not present') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
                          rune=rune,
                          method='pay',
                          params=[inv1])
     assert exc_info.value.error['code'] == 0x5de
 
     # Can't get around it this way!
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
+    with pytest.raises(RpcError, match='Not permitted: parameter amount_msat not present') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
                          rune=rune,
                          method='pay',
                          params=[inv2, 12000])
     assert exc_info.value.error['code'] == 0x5de
 
     # Nor this way, using a string!
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
+    with pytest.raises(RpcError, match='Not permitted: parameter amount_msat is not an integer') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
                          rune=rune,
                          method='pay',
                          params={'bolt11': inv2, 'amount_msat': '10000sat'})
     assert exc_info.value.error['code'] == 0x5de
 
     # Too much!
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
+    with pytest.raises(RpcError, match='Not permitted: parameter amount_msat is greater or equal to 10000') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
                          rune=rune,
                          method='pay',
@@ -671,3 +671,49 @@ def test_id_migration(node_factory):
 
     # Our migration should have removed this row now
     assert l1.db_query("SELECT * FROM vars WHERE name = 'runes_uniqueid';") == []
+
+
+def test_rune_error_messages(node_factory):
+    l1 = node_factory.get_node()
+
+    rune1 = l1.rpc.createrune(restrictions=[['method=pay'],
+                                            ['pnum=1']])['rune']
+    with pytest.raises(RpcError, match='Not permitted: number of parameters is not equal to 1'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune1,
+                         method='pay',
+                         params=['xxx', 12000])
+    with pytest.raises(RpcError, match='Not permitted: number of parameters is not equal to 1'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune1,
+                         method='pay',
+                         params={'bolt11': 'xxx', 'amount_msat': 17})
+
+    rune2 = l1.rpc.createrune(restrictions=[['method=pay'],
+                                            ['parr1=17']])['rune']
+
+    with pytest.raises(RpcError, match='Not permitted: parameter #1 is not equal to 17'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune2,
+                         method='pay',
+                         params=['xxx', 12000])
+
+    with pytest.raises(RpcError, match='Not permitted: parameter #1 not present'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune2,
+                         method='pay',
+                         params={'bolt11': 'xxx', 'amount_msat': 12000})
+
+    rune3 = l1.rpc.createrune(restrictions=[['method=pay'],
+                                            ['pnamebolt11=xxx']])['rune']
+
+    with pytest.raises(RpcError, match='Not permitted: parameter bolt11 is not equal to xxx'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune3,
+                         method='pay',
+                         params={'bolt11': 'yyy', 'amount_msat': 12000})
+    with pytest.raises(RpcError, match='Not permitted: parameter bolt11 not present'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune3,
+                         method='pay',
+                         params=['xxx', 12000])