From 2d07d1f578cc8992d6dd02c08410475a0ac6d233 Mon Sep 17 00:00:00 2001
From: Christian Decker <decker.christian@gmail.com>
Date: Sun, 19 Jul 2020 21:34:50 +0200
Subject: [PATCH] paymod: Rewrite the shadow-route constraint enforcement
We now check against both constraints on the modifier and the payment before
applying either. This "fixes" the assert that was causing the crash in #3851,
but we are still looking for the source of the inconsistency where the
modifier constraints, initialized to 1/4th of the payment, suddenly get more
permissive than the payment itself.
---
plugins/libplugin-pay.c | 80 +++++++++++++++++++++++++++++------------
1 file changed, 57 insertions(+), 23 deletions(-)
diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c
index 55769bda5..602d56c98 100644
--- a/plugins/libplugin-pay.c
+++ b/plugins/libplugin-pay.c
@@ -1782,7 +1782,7 @@ static struct command_result *shadow_route_listchannels(struct command *cmd,
u64 sample = 0;
struct amount_msat best_fee;
const jsmntok_t *sattok, *delaytok, *basefeetok, *propfeetok, *desttok,
- *channelstok, *chan;
+ *channelstok, *chan, *scidtok;
/* Check the invariants on the constraints between payment and modifier. */
assert(d->constraints.cltv_budget <= p->constraints.cltv_budget / 4);
@@ -1800,18 +1800,21 @@ static struct command_result *shadow_route_listchannels(struct command *cmd,
delaytok = json_get_member(buf, chan, "delay");
basefeetok = json_get_member(buf, chan, "base_fee_millisatoshi");
propfeetok = json_get_member(buf, chan, "fee_per_millionth");
+ scidtok = json_get_member(buf, chan, "short_channel_id");
desttok = json_get_member(buf, chan, "destination");
if (sattok == NULL || delaytok == NULL ||
delaytok->type != JSMN_PRIMITIVE || basefeetok == NULL ||
basefeetok->type != JSMN_PRIMITIVE || propfeetok == NULL ||
- propfeetok->type != JSMN_PRIMITIVE || desttok == NULL)
+ propfeetok->type != JSMN_PRIMITIVE || desttok == NULL ||
+ scidtok == NULL)
continue;
json_to_u16(buf, delaytok, &curr.cltv_expiry_delta);
json_to_number(buf, basefeetok, &curr.fee_base_msat);
json_to_number(buf, propfeetok,
&curr.fee_proportional_millionths);
+ json_to_short_channel_id(buf, scidtok, &curr.short_channel_id);
json_to_sat(buf, sattok, &capacity);
json_to_node_id(buf, desttok, &curr.pubkey);
@@ -1841,33 +1844,64 @@ static struct command_result *shadow_route_listchannels(struct command *cmd,
}
if (best != NULL) {
- bool ok;
- /* Ok, we found an extension, let's add it. */
- d->destination = best->pubkey;
-
- /* Apply deltas to the constraints in the shadow route so we
- * don't overshoot our 1/4th target. */
- if (!payment_constraints_update(&d->constraints, best_fee,
- best->cltv_expiry_delta)) {
+ /* Check that we could apply the shadow route extension. Check
+ * against both the shadow route budget as well as the
+ * original payment's budget. */
+ if (best->cltv_expiry_delta > d->constraints.cltv_budget ||
+ best->cltv_expiry_delta > p->constraints.cltv_budget) {
best = NULL;
goto next;
}
- /* Now do the same to the payment constraints so other
- * modifiers don't do it either. */
- ok = payment_constraints_update(&p->constraints, best_fee,
- best->cltv_expiry_delta);
-
- /* And now the thing that caused all of this: adjust the call
- * to getroute. */
- if (d->fuzz_amount) {
- /* Only fuzz the amount to route to the destination if
- * we didn't opt-out earlier. */
- ok &= amount_msat_add(&p->getroute->amount,
- p->getroute->amount, best_fee);
+ /* Check the fee budget only if we didn't opt out, since
+ * testing against a virtual budget is not useful if we do not
+ * actually use it (it could give false positives and fail
+ * attempts that might have gone through, */
+ if (d->fuzz_amount &&
+ (amount_msat_greater(best_fee, d->constraints.fee_budget) ||
+ (amount_msat_greater(best_fee,
+ p->constraints.fee_budget)))) {
+ best = NULL;
+ goto next;
}
+
+ /* Now we can be sure that adding the shadow route will succeed */
+ plugin_log(
+ p->plugin, LOG_DBG,
+ "Adding shadow_route hop over channel %s: adding %s "
+ "in fees and %d CLTV delta",
+ type_to_string(tmpctx, struct short_channel_id,
+ &best->short_channel_id),
+ type_to_string(tmpctx, struct amount_msat, &best_fee),
+ best->cltv_expiry_delta);
+
+ d->destination = best->pubkey;
+ d->constraints.cltv_budget -= best->cltv_expiry_delta;
p->getroute->cltv += best->cltv_expiry_delta;
- assert(ok);
+
+ if (!d->fuzz_amount)
+ goto next;
+
+ /* Only try to apply the fee budget changes if we want to fuzz
+ * the amount. Virtual fees that we then don't deliver to the
+ * destination could otherwise cause the route to be too
+ * expensive, while really being ok. If any of these fail then
+ * the above checks are insufficient. */
+ if (!amount_msat_sub(&d->constraints.fee_budget,
+ d->constraints.fee_budget, best_fee) ||
+ !amount_msat_sub(&p->constraints.fee_budget,
+ p->constraints.fee_budget, best_fee))
+ plugin_err(p->plugin,
+ "Could not update fee constraints "
+ "for shadow route extension. "
+ "payment fee budget %s, modifier "
+ "fee budget %s, shadow fee to add %s",
+ type_to_string(tmpctx, struct amount_msat,
+ &p->constraints.fee_budget),
+ type_to_string(tmpctx, struct amount_msat,
+ &d->constraints.fee_budget),
+ type_to_string(tmpctx, struct amount_msat,
+ &best_fee));
}
next: