Reservations leak v25.05

plugins/askrene/refine.c

@@ -22,8 +22,15 @@ static void get_scidd(const struct gossmap *gossmap,
 
 static void destroy_reservations(struct reserve_hop *rhops, struct askrene *askrene)
 {
-	for (size_t i = 0; i < tal_count(rhops); i++)
-		reserve_remove(askrene->reserved, &rhops[i]);
+	for (size_t i = 0; i < tal_count(rhops); i++){
+		if (!reserve_remove(askrene->reserved, &rhops[i])) {
+			plugin_log(
+			    askrene->plugin, LOG_BROKEN,
+			    "reserve_remove failed: %s on %s",
+			    fmt_amount_msat(tmpctx, rhops[i].amount),
+			    fmt_short_channel_id_dir(tmpctx, &rhops[i].scidd));
+		}
+	}
 }
 
 static struct reserve_hop *new_reservations(const tal_t *ctx,
@@ -592,7 +599,7 @@ refine_with_fees_and_limits(const tal_t *ctx,
 	/* Total flowset probability is now easily calculated given reservations
 	 * contains the total amounts through each channel (once we remove them) */
 	destroy_reservations(reservations, get_askrene(rq->plugin));
-	tal_add_destructor2(reservations, destroy_reservations, get_askrene(rq->plugin));
+	tal_del_destructor2(reservations, destroy_reservations, get_askrene(rq->plugin));
 
 	*flowset_probability = 1.0;
 	for (size_t i = 0; i < tal_count(reservations); i++) {

tests/test_askrene.py

@@ -1473,3 +1473,63 @@ def test_simple_dummy_channel(node_factory):
         final_cltv=5,
         layers=["mylayer"],
     )
+
+
+def test_reservations_leak(node_factory, executor):
+    l1, l2, l3, l4, l5, l6 = node_factory.get_nodes(
+        6,
+        opts=[
+            {"fee-base": 0, "fee-per-satoshi": 0},
+            {"fee-base": 0, "fee-per-satoshi": 0},
+            {
+                "fee-base": 0,
+                "fee-per-satoshi": 0,
+                "plugin": os.path.join(os.getcwd(), "tests/plugins/hold_htlcs.py"),
+            },
+            {"fee-base": 0, "fee-per-satoshi": 0},
+            {"fee-base": 0, "fee-per-satoshi": 0},
+            {"fee-base": 1000, "fee-per-satoshi": 0},
+        ],
+    )
+
+    # There must be a common non-local channel in both payment paths.
+    # With a local channel we cannot trigger the reservation leak because we
+    # reserve slightly different amounts locally due to HTLC onchain costs.
+    node_factory.join_nodes([l1, l2, l4, l6, l3], wait_for_announce=True)
+    node_factory.join_nodes([l1, l2, l4, l5], wait_for_announce=True)
+
+    # Use offers instead of bolt11 because we are going to pay through a blinded
+    # path and trigger a fake channel collision between both payments.
+    offer1 = l3.rpc.offer("any")["bolt12"]
+    offer2 = l5.rpc.offer("any")["bolt12"]
+
+    inv1 = l1.rpc.fetchinvoice(offer1, "100sat")["invoice"]
+    inv2 = l1.rpc.fetchinvoice(offer2, "101sat")["invoice"]
+
+    # Initiate the first payment that has a delay.
+    fut = executor.submit(l1.rpc.xpay, (inv1))
+
+    # Wait for the first payment to reserve the path.
+    l1.daemon.wait_for_log(r"json_askrene_reserve called")
+
+    # A second payment starts.
+    l1.rpc.xpay(inv2)
+    l1.daemon.wait_for_log(r"json_askrene_unreserve called")
+
+    l3.daemon.wait_for_log(r"Holding onto an incoming htlc for 10 seconds")
+
+    # There is a payment pending therefore we expect reservations.
+    reservations = l1.rpc.askrene_listreservations()
+    assert reservations != {"reservations": []}
+
+    l3.daemon.wait_for_log(r"htlc_accepted hook called")
+    fut.result()
+    l1.daemon.wait_for_log(r"json_askrene_unreserve called")
+
+    # The first payment has finished we expect no reservations.
+    reservations = l1.rpc.askrene_listreservations()
+    assert reservations == {"reservations": []}
+
+    # We shouldn't fail askrene-unreserve. If it does it means something went
+    # wrong.
+    assert l1.daemon.is_in_log("askrene-unreserve failed") is None