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==
{
   <insert_a_suppression_name_here>
   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
This commit is contained in:
niftynei 2023-02-02 16:04:42 -06:00 committed by Alex Myers
parent 6347ee7308
commit ff0a63a0d7
2 changed files with 34 additions and 26 deletions

View file

@ -351,8 +351,9 @@ static void channel_hints_update(struct payment *p,
hint->estimated_capacity = *estimated_capacity; hint->estimated_capacity = *estimated_capacity;
modified = true; modified = true;
} }
if (htlc_budget != NULL && *htlc_budget < hint->htlc_budget) { if (htlc_budget != NULL) {
hint->htlc_budget = *htlc_budget; assert(hint->local);
hint->local->htlc_budget = *htlc_budget;
modified = true; modified = true;
} }
@ -376,17 +377,15 @@ static void channel_hints_update(struct payment *p,
newhint.enabled = enabled; newhint.enabled = enabled;
newhint.scid.scid = scid; newhint.scid.scid = scid;
newhint.scid.dir = direction; 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) if (estimated_capacity != NULL)
newhint.estimated_capacity = *estimated_capacity; 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); tal_arr_expand(&root->channel_hints, newhint);
paymod_log( 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 /* For local channels we check that we don't overwhelm
* them with too many HTLCs. */ * 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 /* For all channels we check that they have a
* sufficiently large estimated capacity to have some * sufficiently large estimated capacity to have some
@ -538,12 +538,15 @@ static bool payment_chanhints_apply_route(struct payment *p, bool remove)
paymod_log( paymod_log(
p, LOG_DBG, p, LOG_DBG,
"Capacity: estimated_capacity=%s, hop_amount=%s. " "Capacity: estimated_capacity=%s, hop_amount=%s. "
"HTLC Budget: htlc_budget=%d, local=%d", "local=%s%s",
type_to_string(tmpctx, struct amount_msat, type_to_string(tmpctx, struct amount_msat,
&curhint->estimated_capacity), &curhint->estimated_capacity),
type_to_string(tmpctx, struct amount_msat, type_to_string(tmpctx, struct amount_msat,
&curhop->amount), &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; return false;
} }
} }
@ -558,10 +561,12 @@ apply_changes:
/* Update the number of htlcs for any local /* Update the number of htlcs for any local
* channel in the route */ * channel in the route */
if (curhint->local && remove) if (curhint->local) {
curhint->htlc_budget++; if (remove)
else if (curhint->local) curhint->local->htlc_budget++;
curhint->htlc_budget--; else
curhint->local->htlc_budget--;
}
if (remove && !amount_msat_add( if (remove && !amount_msat_add(
&curhint->estimated_capacity, &curhint->estimated_capacity,
@ -602,7 +607,7 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p)
hint->estimated_capacity)) hint->estimated_capacity))
tal_arr_expand(&res, hint->scid); 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 /* If we cannot add any HTLCs to the channel we
* shouldn't look for a route through that channel */ * shouldn't look for a route through that channel */
tal_arr_expand(&res, hint->scid); 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. */ * estimate to the smallest failed attempt. */
return false; 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 /* If we cannot add any HTLCs to the channel we
* shouldn't look for a route through that channel */ * shouldn't look for a route through that channel */
return false; 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++) { for (size_t i = 0; i < tal_count(p->channel_hints); i++) {
h = &p->channel_hints[i]; h = &p->channel_hints[i];
if (h->local && h->enabled) if (h->local && h->enabled)
res += h->htlc_budget; res += h->local->htlc_budget;
} }
root = p; root = p;
while (root->parent) while (root->parent)

View file

@ -53,6 +53,13 @@ struct payment_result {
int *erring_direction; 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 /* Information about channels we inferred from a) looking at our channels, and
* b) from failures encountered during attempts to perform a payment. These * b) from failures encountered during attempts to perform a payment. These
* are attached to the root payment, since that information is * are attached to the root payment, since that information is
@ -73,13 +80,9 @@ struct channel_hint {
/* Is the channel enabled? */ /* Is the channel enabled? */
bool enabled; bool enabled;
/* True if we are one endpoint of this channel */ /* Non-null if we are one endpoint of this channel */
bool local; 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 /* Each payment goes through a number of steps that are always processed in