Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pay: Remember and update channel_hints across payments #7494

Merged
merged 24 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
51ba252
pay: Add a function to update `channel_hint`s based on their age
cdecker Jul 25, 2024
194ce79
route: Add the total capacity to route_hops
cdecker Jul 25, 2024
977b70f
pay: Use the total_mast amount as the upper limit for channel_hints
cdecker Jul 25, 2024
a5c20aa
pay: Make the `channel_hint`s global
cdecker Jul 29, 2024
e5c719d
make: Weaken over aggressive check-amount-access test
cdecker Jul 29, 2024
2552e5c
plugin: Split out the `struct channel_hint` handling
cdecker Aug 2, 2024
729fe8b
pay: Rename overall_capacity to just capacity
cdecker Aug 11, 2024
d918f4c
route: Simplify direction
cdecker Aug 16, 2024
ae5329a
pay: Subscribe to the `channel_hint_update` notifications
cdecker Aug 9, 2024
337b452
route: Use safe `amount_sat_to_msat` conversion
cdecker Aug 16, 2024
fa9fe09
route: Change the type of the funding capacity to `amount_sat`
cdecker Aug 16, 2024
54e02fe
pytest: Test that we remember learnt channel hints across payments
cdecker Jul 31, 2024
10b3fbb
pay: Use the global `channel_hint_set` and remember across payments
cdecker Aug 22, 2024
257ec32
pay: Add a hysteresis for channel_hint relaxation
cdecker Aug 23, 2024
bb7eca8
pay: Add `channel_hint_set_count` primitive
cdecker Aug 23, 2024
3e47249
pay: Log when and why we exclude a channel from the route
cdecker Aug 23, 2024
c0b6847
pay: Inject `channel_hint`s we receive via plugin notifications
cdecker Aug 23, 2024
5cd37b5
pay: Remove use of temporary local `channel_hint`
cdecker Aug 28, 2024
9bd1b6f
pytest: Fix up the `test_mutual_connect_race`
cdecker Aug 29, 2024
785778d
test: Fix up the `test_pay_routeboost` test
cdecker Sep 2, 2024
1180eeb
pytest: Fix up the `test_sendpay_grouping` test
cdecker Sep 4, 2024
fddadaf
pay: Simplify the `channel_hint` update logic
cdecker Sep 5, 2024
47743bb
route: Re-add the assertion that we're one side of a channel
cdecker Oct 2, 2024
d0b2f3c
pay: Switch to msat for total_capacity
cdecker Oct 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ check-discouraged-functions:
# since it risks overflow.
check-amount-access:
@! (git grep -nE "(->|\.)(milli)?satoshis" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*" | grep -v '/* Raw:')
@! git grep -nE "\\(struct amount_(m)?sat\\)" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*"
@! git grep -nE "\\(struct amount_(m)?sat\\)" -- "*.c" "*.h" ":(exclude)common/amount.*" ":(exclude)*/test/*" | grep -vE "sizeof.struct amount_(m)?sat."

# For those without working cppcheck
check-source-no-cppcheck: check-makefile check-source-bolt check-whitespace check-spelling check-python check-includes check-shellcheck check-setup_locale check-tmpctx check-discouraged-functions check-amount-access
Expand Down
8 changes: 8 additions & 0 deletions common/json_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,14 @@ void json_add_amount_msat(struct json_stream *result,
json_add_u64(result, msatfieldname, msat.millisatoshis); /* Raw: low-level helper */
}

void json_add_amount_sat(struct json_stream *result,
const char *satfieldname,
struct amount_sat sat)
{
assert(strends(satfieldname, "_sat") || streq(satfieldname, "sat"));
json_add_u64(result, satfieldname, sat.satoshis); /* Raw: low-level helper */
Comment on lines +629 to +634
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree. There's a reason this function was removed. In general, we want to expose values in msat. In this case, the fact that the capacity has to be in whole sats is kind of a detail.

}

void json_add_amount_sat_msat(struct json_stream *result,
const char *msatfieldname,
struct amount_sat sat)
Expand Down
7 changes: 6 additions & 1 deletion common/json_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
#define LIGHTNING_COMMON_JSON_STREAM_H
#include "config.h"

#include <bitcoin/short_channel_id.h>
#define JSMN_STRICT 1
# include <external/jsmn/jsmn.h>

#include <bitcoin/short_channel_id.h>
#include <ccan/short_types/short_types.h>
#include <ccan/tal/tal.h>
#include <ccan/time/time.h>
Expand Down Expand Up @@ -334,6 +334,11 @@ void json_add_amount_msat(struct json_stream *result,
struct amount_msat msat)
NO_NULL_ARGS;

void json_add_amount_sat(struct json_stream *result,
const char *satfieldname,
struct amount_sat sat)
NO_NULL_ARGS;

/* Adds an 'msat' field */
void json_add_amount_sat_msat(struct json_stream *result,
const char *msatfieldname,
Expand Down
12 changes: 6 additions & 6 deletions common/route.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ static bool dijkstra_to_hops(struct route_hop **hops,
const struct gossmap_node *next;
size_t num_hops = tal_count(*hops);
const struct half_chan *h;
struct amount_msat total_msat;

if (dist == 0)
return true;
Expand All @@ -92,12 +93,9 @@ static bool dijkstra_to_hops(struct route_hop **hops,

/* OK, populate other fields. */
c = dijkstra_best_chan(dij, curidx);
if (c->half[0].nodeidx == curidx) {
(*hops)[num_hops].direction = 0;
} else {
assert(c->half[1].nodeidx == curidx);
(*hops)[num_hops].direction = 1;
}

assert(c->half[0].nodeidx == curidx || c->half[1].nodeidx == curidx);
(*hops)[num_hops].direction = c->half[0].nodeidx == curidx ? 0 : 1;
cdecker marked this conversation as resolved.
Show resolved Hide resolved
(*hops)[num_hops].scid = gossmap_chan_scid(gossmap, c);

/* Find other end of channel. */
Expand All @@ -107,6 +105,8 @@ static bool dijkstra_to_hops(struct route_hop **hops,
if (!dijkstra_to_hops(hops, gossmap, dij, next, amount, cltv))
return false;

total_msat = gossmap_chan_get_capacity(gossmap, c);
(*hops)[num_hops].capacity = total_msat;
(*hops)[num_hops].amount = *amount;
(*hops)[num_hops].delay = *cltv;

Expand Down
2 changes: 2 additions & 0 deletions common/route.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ struct gossmap_node;
* @direction: 0 (dest node_id < src node_id), 1 (dest node_id > src).
* @node_id: the node_id of the destination of this hop.
* @amount: amount to send through this hop.
* @capacity: The amount the channel was funded with.
* @delay: total cltv delay at this hop.
*/
struct route_hop {
struct short_channel_id scid;
int direction;
struct node_id node_id;
struct amount_msat amount;
struct amount_msat capacity;
u32 delay;
};

Expand Down
1 change: 1 addition & 0 deletions common/test/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ common/test/run-route common/test/run-route-specific common/test/run-route-inflo
common/node_id.o \
common/pseudorand.o \
common/route.o \
gossipd/gossip_store_wiregen.o \
wire/fromwire.o \
wire/peer_wiregen.o \
wire/towire.o
Expand Down
8 changes: 8 additions & 0 deletions common/test/run-route-specific.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <common/setup.h>
#include <common/utils.h>
#include <bitcoin/chainparams.h>
#include <gossipd/gossip_store_wiregen.h>
#include <stdio.h>
#include <wire/peer_wiregen.h>
#include <unistd.h>
Expand Down Expand Up @@ -139,6 +140,13 @@ static void add_connection(int store_fd,
ids[0], ids[1],
&dummy_key, &dummy_key);
write_to_store(store_fd, msg);
tal_free(msg);

/* Also needs a hint as to the funding size. */
struct amount_sat capacity = AMOUNT_SAT(100000000);
msg = towire_gossip_store_channel_amount(tmpctx, capacity);
write_to_store(store_fd, msg);
tal_free(msg);

update_connection(store_fd, from, to, shortid, min, max,
base_fee, proportional_fee,
Expand Down
10 changes: 8 additions & 2 deletions common/test/run-route.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <common/setup.h>
#include <common/utils.h>
#include <bitcoin/chainparams.h>
#include <gossipd/gossip_store_wiregen.h>
#include <stdio.h>
#include <wire/peer_wiregen.h>
#include <unistd.h>
Expand Down Expand Up @@ -129,8 +130,13 @@ static void add_connection(int store_fd,
&dummy_key, &dummy_key);
write_to_store(store_fd, msg);

update_connection(store_fd, from, to, base_fee, proportional_fee,
delay, false);
/* Also needs a hint as to the funding size. */
struct amount_sat capacity = AMOUNT_SAT(100000000);
msg = towire_gossip_store_channel_amount(tmpctx, capacity);
write_to_store(store_fd, msg);
tal_free(msg);
update_connection(store_fd, from, to, base_fee, proportional_fee, delay,
false);
}

static bool channel_is_between(const struct gossmap *gossmap,
Expand Down
10 changes: 8 additions & 2 deletions plugins/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,14 @@ PLUGIN_LIB_SRC := plugins/libplugin.c
PLUGIN_LIB_HEADER := plugins/libplugin.h
PLUGIN_LIB_OBJS := $(PLUGIN_LIB_SRC:.c=.o)

PLUGIN_PAY_LIB_SRC := plugins/libplugin-pay.c
PLUGIN_PAY_LIB_HEADER := plugins/libplugin-pay.h
PLUGIN_PAY_LIB_SRC := \
plugins/channel_hint.c \
plugins/libplugin-pay.c

PLUGIN_PAY_LIB_HEADER := \
plugins/channel_hint.h \
plugins/libplugin-pay.h

PLUGIN_PAY_LIB_OBJS := $(PLUGIN_PAY_LIB_SRC:.c=.o)

PLUGIN_OFFERS_SRC := plugins/offers.c plugins/offers_offer.c plugins/offers_invreq_hook.c plugins/offers_inv_hook.c plugins/establish_onion_path.c plugins/fetchinvoice.c
Expand Down
199 changes: 199 additions & 0 deletions plugins/channel_hint.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
#include "config.h"
#include <plugins/channel_hint.h>

void channel_hint_to_json(const char *name, const struct channel_hint *hint,
struct json_stream *dest)
{
json_object_start(dest, name);
json_add_u32(dest, "timestamp", hint->timestamp);
json_add_short_channel_id_dir(dest, "scid", hint->scid);
json_add_amount_msat(dest, "estimated_capacity_msat",
hint->estimated_capacity);
json_add_amount_msat(dest, "total_capacity_msat", hint->capacity);
json_add_bool(dest, "enabled", hint->enabled);
json_object_end(dest);
}

/* How long until even a channel whose estimate is down at 0msat will
* be considered fully refilled. The refill rate is the inverse of
* this times the channel size. The refilling is a linear
* approximation, with a small hysteresis applied in order to prevent
* a single payment relaxing its own constraints thus causing it to
* prematurely retry an already attempted channel.
*/
#define PAY_REFILL_TIME 7200

/* Add an artificial delay before accepting updates. This ensures we
* don't actually end up relaxing a tight constraint inbetween the
* attempt that added it and the next retry. If we were to relax right
* away, then we could end up retrying the exact same path we just
* failed at. If the `time_between_attempts * refill > 1msat`, we'd
* end up not actually constraining at all, because we set the
* estimate to `attempt - 1msat`. This also results in the updates
* being limited to once every minute, and causes a stairway
* pattern. The hysteresis has to be >60s otherwise a single payment
* can already end up retrying a previously excluded channel.
*/
#define PAY_REFILL_HYSTERESIS 60
/**
* Update the `channel_hint` in place, return whether it should be kept.
*
* This computes the refill-rate based on the overall capacity, and
* the time elapsed since the last update and relaxes the upper bound
* on the capacity, and resets the enabled flag if appropriate. If the
* hint is no longer useful, i.e., it does not provide any additional
* information on top of the structural information we've learned from
* the gossip, then we return `false` to signal that the
* `channel_hint` may be removed.
*/
bool channel_hint_update(const struct timeabs now, struct channel_hint *hint)
{
/* Precision is not required here, so integer division is good
* enough. But keep the order such that we do not round down
* too much. We do so by first multiplying, before
* dividing. The formula is `current = last + delta_t *
* overall / refill_rate`.
*/
struct amount_msat refill;
struct amount_msat capacity = hint->capacity;

if (now.ts.tv_sec < hint->timestamp + PAY_REFILL_HYSTERESIS)
return true;

u64 seconds = now.ts.tv_sec - hint->timestamp;
if (!amount_msat_mul(&refill, capacity, seconds))
abort();

refill = amount_msat_div(refill, PAY_REFILL_TIME);
if (!amount_msat_add(&hint->estimated_capacity,
hint->estimated_capacity, refill))
abort();

/* Clamp the value to the `overall_capacity` */
if (amount_msat_greater(hint->estimated_capacity, capacity))
hint->estimated_capacity = capacity;

/* TODO This is rather coarse. We could map the disabled flag
to having 0msat capacity, and then relax from there. But it'd
likely be too slow of a relaxation.*/
if (seconds > 60)
hint->enabled = true;

/* Since we update in-place we should make sure that we can
* just call update again and the result is stable, if no time
* has passed. */
hint->timestamp = now.ts.tv_sec;

/* We report this hint as useless, if the hint does not
* restrict the channel, i.e., if it is enabled and the
* estimate is the same as the overall capacity. */
return !hint->enabled ||
amount_msat_greater(capacity, hint->estimated_capacity);
}

struct channel_hint *channel_hint_set_find(struct channel_hint_set *self,
const struct short_channel_id_dir *scidd)
{
for (size_t i=0; i<tal_count(self->hints); i++) {
struct channel_hint *hint = &self->hints[i];
if (short_channel_id_dir_eq(&hint->scid, scidd))
return hint;
}
return NULL;
}

/* See header */
struct channel_hint *
channel_hint_set_add(struct channel_hint_set *self, u32 timestamp,
const struct short_channel_id_dir *scidd, bool enabled,
const struct amount_msat *estimated_capacity,
const struct amount_msat capacity, u16 *htlc_budget)
{
struct channel_hint *copy, *old, *newhint;

/* If the channel is marked as enabled it must have an estimate. */
assert(!enabled || estimated_capacity != NULL);

/* If there was no hint, add the new one, if there was one,
* pick the one with the newer timestamp. */
old = channel_hint_set_find(self, scidd);
copy = tal_dup(tmpctx, struct channel_hint, old);
if (old == NULL) {
newhint = tal(tmpctx, struct channel_hint);
newhint->enabled = enabled;
newhint->scid = *scidd;
newhint->capacity = capacity;
if (estimated_capacity != NULL)
newhint->estimated_capacity = *estimated_capacity;
newhint->local = NULL;
newhint->timestamp = timestamp;
tal_arr_expand(&self->hints, *newhint);
return &self->hints[tal_count(self->hints) - 1];
} else if (old->timestamp <= timestamp) {
/* `local` is kept, since we do not pass in those
* annotations here. */
old->enabled = enabled;
old->timestamp = timestamp;
if (estimated_capacity != NULL)
old->estimated_capacity = *estimated_capacity;

/* We always pick the larger of the capacities we are
* being told. This is because in some cases, such as
* routehints, we're not actually being told the total
* capacity, just lower values. */
if (amount_msat_greater(capacity, old->capacity))
old->capacity = capacity;

return copy;
} else {
return NULL;
}
}

/**
* Load a channel_hint from its JSON representation.
*
* @return The initialized `channel_hint` or `NULL` if we encountered a parsing
* error.
*/
struct channel_hint *channel_hint_from_json(const tal_t *ctx,
const char *buffer,
const jsmntok_t *toks)
{
const char *ret;
const jsmntok_t *payload = json_get_member(buffer, toks, "payload"),
*jhint =
json_get_member(buffer, payload, "channel_hint");
struct channel_hint *hint = tal(ctx, struct channel_hint);

ret = json_scan(ctx, buffer, jhint,
"{timestamp:%,scid:%,estimated_capacity_msat:%,total_capacity_msat:%,enabled:%}",
JSON_SCAN(json_to_u32, &hint->timestamp),
JSON_SCAN(json_to_short_channel_id_dir, &hint->scid),
JSON_SCAN(json_to_msat, &hint->estimated_capacity),
JSON_SCAN(json_to_msat, &hint->capacity),
JSON_SCAN(json_to_bool, &hint->enabled));

if (ret != NULL)
hint = tal_free(hint);
return hint;
}

struct channel_hint_set *channel_hint_set_new(const tal_t *ctx)
{
struct channel_hint_set *set = tal(ctx, struct channel_hint_set);
set->hints = tal_arr(set, struct channel_hint, 0);
return set;
}

void channel_hint_set_update(struct channel_hint_set *set,
const struct timeabs now)
{
for (size_t i = 0; i < tal_count(set->hints); i++)
channel_hint_update(time_now(), &set->hints[i]);
}

size_t channel_hint_set_count(const struct channel_hint_set *set)
{
return tal_count(set->hints);
}
Loading
Loading