runes: Reimplement rate in terms of per

Changelog-Changed: JSON-RPC: `checkrune` `rate` restriction is slightly stricter (exact division of time like `per`)

doc/lightning-createrune.7.md

@@ -35,8 +35,8 @@ being run:
 * time: the current UNIX time, e.g. "time<1656759180".
 * id: the node\_id of the peer, e.g. "id=024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605".
 * method: the command being run, e.g. "method=withdraw".
-* rate: the rate limit, per minute, e.g. "rate=60".
 * 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".
 * parrN: the N'th parameter. e.g. "parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T".

lightningd/runes.c

@@ -19,27 +19,7 @@
 #include <lightningd/runes.h>
 #include <wallet/wallet.h>
 
-struct usage {
-	/* If you really issue more than 2^32 runes, they'll share ratelimit buckets */
-	u32 id;
-	u32 counter;
-};
-
-static u64 usage_id(const struct usage *u)
-{
-	return u->id;
-}
-
-static size_t id_hash(u64 id)
-{
-	return siphash24(siphash_seed(), &id, sizeof(id));
-}
-
-static bool usage_eq_id(const struct usage *u, u64 id)
-{
-	return u->id == id;
-}
-HTABLE_DEFINE_TYPE(struct usage, usage_id, id_hash, usage_eq_id, usage_table);
+static const u64 sec_per_nsec = 1000000000;
 
 struct cond_info {
 	const struct runes *runes;
@@ -49,7 +29,6 @@ struct cond_info {
 	struct timeabs now;
 	const jsmntok_t *params;
 	STRMAP(const jsmntok_t *) cached_params;
-	struct usage *usage;
 };
 
 /* This is lightningd->runes */
@@ -58,7 +37,6 @@ struct runes {
 	struct rune *master;
 	u64 next_unique_id;
 	struct rune_blacklist *blacklist;
-	struct usage_table *usage_table;
 };
 
 const char *rune_is_ours(struct lightningd *ld, const struct rune *rune)
@@ -66,23 +44,6 @@ const char *rune_is_ours(struct lightningd *ld, const struct rune *rune)
 	return rune_is_derived(ld->runes->master, rune);
 }
 
-static void memleak_help_usage_table(struct htable *memtable,
-				     struct usage_table *usage_table)
-{
-	memleak_scan_htable(memtable, &usage_table->raw);
-}
-
-/* Every minute we forget entries. */
-static void flush_usage_table(struct runes *runes)
-{
-	tal_free(runes->usage_table);
-	runes->usage_table = tal(runes, struct usage_table);
-	usage_table_init(runes->usage_table);
-	memleak_add_helper(runes->usage_table, memleak_help_usage_table);
-
-	notleak(new_reltimer(runes->ld->timers, runes, time_from_sec(60), flush_usage_table, runes));
-}
-
 /* Convert unique_id string to u64.  We only expect this to fail when we're
  * dealing with runes from elsewhere (i.e. param_rune) */
 static bool unique_id_num(const struct rune *rune, u64 *num)
@@ -114,16 +75,36 @@ u64 rune_unique_id(const struct rune *rune)
 	return num;
 }
 
-static const char *last_time_check(const tal_t *ctx,
+static const char *last_time_check(const struct rune *rune,
+				    struct cond_info *cinfo,
+					u64 n_sec)
+{
+	u64 diff;
+	struct timeabs last_used;
+
+	if (!wallet_get_rune(tmpctx, cinfo->runes->ld->wallet, atol(rune->unique_id), &last_used)) {
+		/* FIXME: If we do not know the rune, per does not work */
+		return NULL;
+	}
+	if (time_before(cinfo->now, last_used)) {
+		last_used = cinfo->now;
+	}
+
+	diff = time_to_nsec(time_between(cinfo->now, last_used));
+	if (diff < n_sec) {
+		return "too soon";
+	}
+	return NULL;
+}
+
+static const char *per_time_check(const tal_t *ctx,
 				    const struct runes *runes,
 				    const struct rune *rune,
 				    const struct rune_altern *alt,
 				    struct cond_info *cinfo)
 {
-	u64 r, multiplier, diff;
-	struct timeabs last_used;
+	u64 r, multiplier;
 	char *endp;
-	const u64 sec_per_nsec = 1000000000;
 
 	if (alt->condition != '=')
 		return "per operator must be =";
@@ -151,19 +132,7 @@ static const char *last_time_check(const tal_t *ctx,
 	if (mul_overflows_u64(r, multiplier)) {
 		return "per overflow";
 	}
-	if (!wallet_get_rune(tmpctx, cinfo->runes->ld->wallet, atol(rune->unique_id), &last_used)) {
-		/* FIXME: If we do not know the rune, per does not work */
-		return NULL;
-	}
-	if (time_before(cinfo->now, last_used)) {
-		last_used = cinfo->now;
-	}
-	diff = time_to_nsec(time_between(cinfo->now, last_used));
-
-	if (diff < (r * multiplier)) {
-		return "too soon";
-	}
-	return NULL;
+	return last_time_check(rune, cinfo, r * multiplier);
 }
 
 static const char *rate_limit_check(const tal_t *ctx,
@@ -174,8 +143,6 @@ static const char *rate_limit_check(const tal_t *ctx,
 {
 	unsigned long r;
 	char *endp;
-	u64 unique_id = rune_unique_id(rune);
-
 	if (alt->condition != '=')
 		return "rate operator must be =";
 
@@ -183,22 +150,7 @@ static const char *rate_limit_check(const tal_t *ctx,
 	if (endp == alt->value || *endp || r == 0 || r >= UINT32_MAX)
 		return "malformed rate";
 
-	/* We cache this: we only add usage counter if whole rune succeeds! */
-	if (!cinfo->usage) {
-		cinfo->usage = usage_table_get(runes->usage_table, unique_id);
-		if (!cinfo->usage) {
-			cinfo->usage = tal(runes->usage_table, struct usage);
-			cinfo->usage->id = unique_id;
-			cinfo->usage->counter = 0;
-			usage_table_add(runes->usage_table, cinfo->usage);
-		}
-	}
-
-	/* >= becuase if we allow this, counter will increment */
-	if (cinfo->usage->counter >= r)
-		return tal_fmt(ctx, "Rate of %lu per minute exceeded", r);
-
-	return NULL;
+	return last_time_check(rune, cinfo, 60 * sec_per_nsec / r);
 }
 
 /* We need to initialize master runes secret early, so db can use rune_is_ours */
@@ -227,10 +179,6 @@ void runes_finish_init(struct runes *runes)
 
 	runes->next_unique_id = db_get_intvar(ld->wallet->db, "runes_uniqueid", 0);
 	runes->blacklist = wallet_get_runes_blacklist(runes, ld->wallet);
-
-	/* Initialize usage table and start flush timer. */
-	runes->usage_table = NULL;
-	flush_usage_table(runes);
 }
 
 struct rune_and_string {
@@ -795,7 +743,7 @@ static const char *check_condition(const tal_t *ctx,
 	} else if (streq(alt->fieldname, "rate")) {
 		return rate_limit_check(ctx, cinfo->runes, rune, alt, cinfo);
 	} else if (streq(alt->fieldname, "per")) {
-		return last_time_check(ctx, cinfo->runes, rune, alt, cinfo);
+		return per_time_check(ctx, cinfo->runes, rune, alt, cinfo);
 	}
 
 	/* Rest are params looksup: generate this once! */
@@ -850,6 +798,7 @@ static const char *check_condition(const tal_t *ctx,
 static void update_rune_usage_time(struct runes *runes,
 						 struct rune *rune, struct timeabs now)
 {
+	/* FIXME: we could batch DB access if this is too slow */
 	wallet_rune_update_last_used(runes->ld->wallet, rune, now);
 }
 
@@ -882,8 +831,6 @@ static struct command_result *json_checkrune(struct command *cmd,
 	cinfo.method = method;
 	cinfo.params = methodparams;
 	cinfo.now = time_now();
-	/* We will populate it in rate_limit_check if required. */
-	cinfo.usage = NULL;
 	strmap_init(&cinfo.cached_params);
 
 	err = rune_is_ours(cmd->ld, ras->rune);
@@ -900,9 +847,6 @@ static struct command_result *json_checkrune(struct command *cmd,
 		return command_fail(cmd, RUNE_NOT_PERMITTED, "Not permitted: %s", err);
 	}
 
-	/* If it succeeded, *now* we increment any associated usage counter. */
-	if (cinfo.usage)
-		cinfo.usage->counter++;
 	update_rune_usage_time(cmd->ld->runes, ras->rune, cinfo.now);
 
 	js = json_stream_success(cmd);

tests/test_runes.py

@@ -124,10 +124,7 @@ def test_createrune(node_factory):
                  (rune2, "getinfo", {}),
                  (rune3, "getinfo", {}),
                  (rune7, "listpeers", []),
-                 (rune7, "getinfo", {}),
-                 (rune9, "getinfo", {}),
-                 (rune8, "getinfo", {}),
-                 (rune8, "getinfo", {}))
+                 (rune7, "getinfo", {}))
 
     failures = ((rune2, "withdraw", {}),
                 (rune2, "plugin", {'subcommand': 'list'}),
@@ -136,7 +133,10 @@ def test_createrune(node_factory):
                 (rune5, "listpeers", {'id': l1.info['id'], 'level': 'io'}),
                 (rune6, "listpeers", [l1.info['id'], 'io']),
                 (rune7, "listpeers", [l1.info['id']]),
-                (rune7, "listpeers", {'id': l1.info['id']}))
+                (rune7, "listpeers", {'id': l1.info['id']}),
+                # These are derived from rune7, so they have been recently used
+                (rune8, "getinfo", {}),
+                (rune9, "getinfo", {}))
 
     for rune, cmd, params in successes:
         l1.rpc.checkrune(nodeid=l1.info['id'],
@@ -156,17 +156,11 @@ def test_createrune(node_factory):
                              params=params)
         assert exc_info.value.error['code'] == 0x5de
 
-    # Now, this can flake if we cross a minute boundary!  So wait until
-    # It succeeds again.
-    while True:
-        try:
-            l1.rpc.checkrune(nodeid=l1.info['id'],
-                             rune=rune8['rune'],
-                             method='getinfo')
-            break
-        except RpcError as e:
-            assert e.error['code'] == 0x5de
-        time.sleep(1)
+    # Rune 7 succeeded, so we need to wait for 20 seconds
+    time.sleep(21)
+    l1.rpc.checkrune(nodeid=l1.info['id'],
+                     rune=rune8['rune'],
+                     method='getinfo')['valid'] is True
 
     # This fails immediately, since we've done one.
     with pytest.raises(RpcError, match='Not permitted:') as exc_info:
@@ -176,22 +170,6 @@ def test_createrune(node_factory):
                          params={})
     assert exc_info.value.error['code'] == 0x5de
 
-    # Two more succeed for rune8.
-    for _ in range(2):
-        l1.rpc.checkrune(nodeid=l1.info['id'],
-                         rune=rune8['rune'],
-                         method='getinfo',
-                         params={})
-    assert exc_info.value.error['code'] == 0x5de
-
-    # Now we've had 3 in one minute, this will fail.
-    with pytest.raises(RpcError, match='Not permitted:') as exc_info:
-        l1.rpc.checkrune(nodeid=l1.info['id'],
-                         rune=rune8['rune'],
-                         method='getinfo',
-                         params={})
-    assert exc_info.value.error['code'] == 0x5de
-
     # rune5 can only be used by l2:
     with pytest.raises(RpcError, match='Not permitted:') as exc_info:
         l1.rpc.checkrune(nodeid=l1.info['id'],
@@ -200,16 +178,22 @@ def test_createrune(node_factory):
                          params={})
     assert exc_info.value.error['code'] == 0x5de
 
-    # Now wait for ratelimit expiry, ratelimits should reset.
-    time.sleep(61)
+    # Rune8 has rate 3 per minute (20 seconds) and rune9 has rate 1 per minute (60 seconds)
+    time.sleep(21)
+    with pytest.raises(RpcError, match='Not permitted: too soon'):
+        l1.rpc.checkrune(nodeid=l1.info['id'],
+                         rune=rune9['rune'],
+                         method="getinfo")
 
-    for rune, cmd, params in ((rune9, "getinfo", {}),
-                              (rune8, "getinfo", {}),
-                              (rune8, "getinfo", {})):
-        assert l1.rpc.checkrune(nodeid=l1.info['id'],
-                                rune=rune['rune'],
-                                method=cmd,
-                                params=params)['valid'] is True
+    assert l1.rpc.checkrune(nodeid=l1.info['id'],
+                            rune=rune8['rune'],
+                            method="getinfo")['valid'] is True
+
+    # Rune8 uses the same unique_id as rune9, so we need to wait for full 1 minute
+    time.sleep(61)
+    assert l1.rpc.checkrune(nodeid=l1.info['id'],
+                            rune=rune9['rune'],
+                            method="getinfo")['valid'] is True
 
 
 def do_test_rune_per_restriction(l1, rune_to_test, per_sec):