From ff0a63a0d7149100bff9c00ba5cf120e8daead7a Mon Sep 17 00:00:00 2001 From: niftynei Date: Thu, 2 Feb 2023 16:04:42 -0600 Subject: [PATCH] valgrind-fix: patch valgrind error on log statement in pay plugin The htlc_budget only exists iff the hint is a 'local' one; we were failing to write to the htlc_budget field for non-local cases. To avoid this, we make `local` into a struct that contains the fields that pertain to local-only payments (in this case, `htlc_budget`). Valgrind error file: valgrind-errors.1813487 ==1813487== Conditional jump or move depends on uninitialised value(s) ==1813487== at 0x4A9C958: __vfprintf_internal (vfprintf-internal.c:1687) ==1813487== by 0x4AB0F99: __vsnprintf_internal (vsnprintf.c:114) ==1813487== by 0x1D2EF9: do_vfmt (str.c:66) ==1813487== by 0x1D3006: tal_vfmt_ (str.c:92) ==1813487== by 0x11A60A: paymod_log (libplugin-pay.c:167) ==1813487== by 0x11B749: payment_chanhints_apply_route (libplugin-pay.c:534) ==1813487== by 0x11EB36: payment_compute_onion_payloads (libplugin-pay.c:1707) ==1813487== by 0x12000F: payment_continue (libplugin-pay.c:2135) ==1813487== by 0x1245B9: adaptive_splitter_cb (libplugin-pay.c:3800) ==1813487== by 0x11FFB6: payment_continue (libplugin-pay.c:2123) ==1813487== by 0x1206BC: retry_step_cb (libplugin-pay.c:2301) ==1813487== by 0x11FFB6: payment_continue (libplugin-pay.c:2123) ==1813487== { Memcheck:Cond fun:__vfprintf_internal fun:__vsnprintf_internal fun:do_vfmt fun:tal_vfmt_ fun:paymod_log fun:payment_chanhints_apply_route fun:payment_compute_onion_payloads fun:payment_continue fun:adaptive_splitter_cb fun:payment_continue fun:retry_step_cb [sesh] 0:[tmux]*Z Suggested-By: @nothingmuch --- plugins/libplugin-pay.c | 45 +++++++++++++++++++++++------------------ plugins/libplugin-pay.h | 15 ++++++++------ 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 1eb7edb02..314721662 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -351,8 +351,9 @@ static void channel_hints_update(struct payment *p, hint->estimated_capacity = *estimated_capacity; modified = true; } - if (htlc_budget != NULL && *htlc_budget < hint->htlc_budget) { - hint->htlc_budget = *htlc_budget; + if (htlc_budget != NULL) { + assert(hint->local); + hint->local->htlc_budget = *htlc_budget; modified = true; } @@ -376,17 +377,15 @@ static void channel_hints_update(struct payment *p, newhint.enabled = enabled; newhint.scid.scid = scid; newhint.scid.dir = direction; - newhint.local = local; + if (local) { + newhint.local = tal(root->channel_hints, struct local_hint); + assert(htlc_budget); + newhint.local->htlc_budget = *htlc_budget; + } else + newhint.local = NULL; if (estimated_capacity != NULL) newhint.estimated_capacity = *estimated_capacity; - /* This happens if we get a temporary channel failure: we don't know - * htlc capacity here, so assume it's not a problem. */ - if (htlc_budget != NULL) - newhint.htlc_budget = *htlc_budget; - else - newhint.htlc_budget = 20; - tal_arr_expand(&root->channel_hints, newhint); paymod_log( @@ -515,7 +514,8 @@ static bool payment_chanhints_apply_route(struct payment *p, bool remove) /* For local channels we check that we don't overwhelm * them with too many HTLCs. */ - apply = (!curhint->local) || curhint->htlc_budget > 0; + apply = (!curhint->local) || + (curhint->local->htlc_budget > 0); /* For all channels we check that they have a * sufficiently large estimated capacity to have some @@ -538,12 +538,15 @@ static bool payment_chanhints_apply_route(struct payment *p, bool remove) paymod_log( p, LOG_DBG, "Capacity: estimated_capacity=%s, hop_amount=%s. " - "HTLC Budget: htlc_budget=%d, local=%d", + "local=%s%s", type_to_string(tmpctx, struct amount_msat, &curhint->estimated_capacity), type_to_string(tmpctx, struct amount_msat, &curhop->amount), - curhint->htlc_budget, curhint->local); + curhint->local ? "Y" : "N", + curhint->local ? + tal_fmt(tmpctx, " HTLC Budget: htlc_budget=%d", + curhint->local->htlc_budget) : ""); return false; } } @@ -558,10 +561,12 @@ apply_changes: /* Update the number of htlcs for any local * channel in the route */ - if (curhint->local && remove) - curhint->htlc_budget++; - else if (curhint->local) - curhint->htlc_budget--; + if (curhint->local) { + if (remove) + curhint->local->htlc_budget++; + else + curhint->local->htlc_budget--; + } if (remove && !amount_msat_add( &curhint->estimated_capacity, @@ -602,7 +607,7 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p) hint->estimated_capacity)) tal_arr_expand(&res, hint->scid); - else if (hint->local && hint->htlc_budget == 0) + else if (hint->local && hint->local->htlc_budget == 0) /* If we cannot add any HTLCs to the channel we * shouldn't look for a route through that channel */ tal_arr_expand(&res, hint->scid); @@ -679,7 +684,7 @@ static bool payment_route_check(const struct gossmap *gossmap, * estimate to the smallest failed attempt. */ return false; - if (hint->local && hint->htlc_budget == 0) + if (hint->local && hint->local->htlc_budget == 0) /* If we cannot add any HTLCs to the channel we * shouldn't look for a route through that channel */ return false; @@ -3471,7 +3476,7 @@ static u32 payment_max_htlcs(const struct payment *p) for (size_t i = 0; i < tal_count(p->channel_hints); i++) { h = &p->channel_hints[i]; if (h->local && h->enabled) - res += h->htlc_budget; + res += h->local->htlc_budget; } root = p; while (root->parent) diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index e30142074..4087df2d9 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -53,6 +53,13 @@ struct payment_result { int *erring_direction; }; +struct local_hint { + /* How many more htlcs can we send over this channel? Only set if this + * is a local channel, because those are the channels we have exact + * numbers on, and they are the bottleneck onto the network. */ + u16 htlc_budget; +}; + /* Information about channels we inferred from a) looking at our channels, and * b) from failures encountered during attempts to perform a payment. These * are attached to the root payment, since that information is @@ -73,13 +80,9 @@ struct channel_hint { /* Is the channel enabled? */ bool enabled; - /* True if we are one endpoint of this channel */ - bool local; + /* Non-null if we are one endpoint of this channel */ + struct local_hint *local; - /* How many more htlcs can we send over this channel? Only set if this - * is a local channel, because those are the channels we have exact - * numbers on, and they are the bottleneck onto the network. */ - u16 htlc_budget; }; /* Each payment goes through a number of steps that are always processed in