davide/palladum-lightning
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

common: fix dijkstra scoring.

Browse Source 
The "path_score" callback was supposed to evaluate the *entire path*,
but that was counter-intuitive and opened the door to a cost function
bug which caused this path cost to be less than the closer path.

In particular, the capacity bias code didn't understand this at all.

1. Rename the function to `channel_score` and remove the "distance"
   parameter (always "1" since you're supposed to be evaluating a
   single hop).
2. Rename "cost" to the more specific "fee": "score" is our
   actual cost function result (we avoid the word "cost" as it
   may get confused with satoshi amounts).
3. For capacity biassing, we do want to know the amount, but
   explicitly hand that as a separate parameter "total".
4. Fix a minor bug where total handed to scoring function previously
   included channel fee (this is wrong: fee is paid before sending into
   channel).
5. Remove the now-unused total_delay member from the dijkstra
   struct.

Here are the results of our test now (routing 4194303 msat, which
didn't crash the old code, so we could compare).  In both cases
we could find routes to 615 nodes:

Linear success probability (when found): min-max(mean +/- stddev)
	Before: 0.484764-0.999750(0.9781+/-0.049)
	After:  0.487040-0.999543(0.952548+/-0.075)

Hops:
	Before: 1-5(2.13821+/-0.66)
	After:  1-5(2.98374+/-0.77)

Fees:
	Before: 0-50041(2173.75+/-5.3e+03)
	After:  0-50848(922.457+/-2.7e+03)

Delay (blocks):
	Before: 0-294(83.1642+/-68)
	After:  0-196(65.8081+/-60)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: https://github.com/ElementsProject/lightning/issues/7092
Changelog-Fixed: Plugins: `pay` would occasionally crash on routing.
Changelog-Fixed: Plugins: `pay` route algorithm fixed and refined to balance fees and capacity far better.
This commit is contained in:
Rusty Russell
2024-03-07 13:26:36 +10:30
committed by Christian Decker
parent d5c576bd69
commit ffb324f283
6 changed files with 95 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

57
common/dijkstra.c
View File

@@ -4,21 +4,21 @@
#include <ccan/cast/cast.h>
#include <common/dijkstra.h>
#include <common/gossmap.h>
#include <common/overflows.h>
#include <gheap.h>
/* Each node has this side-info. */
struct dijkstra {
/* Number of hops: destination == 0, unreachable == UINTMAX */
u32 distance;
/* Total CLTV delay */
u32 total_delay;
/* Total cost from here to destination */
struct amount_msat cost;
/* Total amount from here to destination */
struct amount_msat amount;
/* I want to use an index here, except that gheap moves things onto
* a temporary on the stack and that makes things complex. */
/* NULL means it's been visited already. */
const struct gossmap_node **heapptr;
/* How we decide "best", lower is better */
/* How we decide "best", lower is better (this is the cost function output) */
u64 score;
/* We could re-evaluate to determine this, but keeps it simple */
@@ -86,16 +86,14 @@ static const struct gossmap_node **mkheap(const tal_t *ctx,
heap[0] = start;
d->heapptr = &heap[0];
d->distance = 0;
d->total_delay = 0;
d->cost = sent;
d->amount = sent;
d->score = 0;
i--;
} else {
heap[i] = n;
d->heapptr = &heap[i];
d->distance = UINT_MAX;
d->cost = AMOUNT_MSAT(-1ULL);
d->total_delay = 0;
d->amount = AMOUNT_MSAT(-1ULL);
d->score = -1ULL;
}
}
@@ -131,11 +129,11 @@ dijkstra_(const tal_t *ctx,
int dir,
struct amount_msat amount,
void *arg),
u64 (*path_score)(u32 distance,
struct amount_msat cost,
struct amount_msat risk,
int dir,
const struct gossmap_chan *c),
u64 (*channel_score)(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total,
int dir,
const struct gossmap_chan *c),
void *arg)
{
struct dijkstra *dij;
@@ -216,7 +214,7 @@ dijkstra_(const tal_t *ctx,
int which_half;
struct gossmap_chan *c;
struct dijkstra *d;
struct amount_msat cost, risk;
struct amount_msat fee, risk;
u64 score;
c = gossmap_nth_chan(map, cur, i, &which_half);
@@ -228,28 +226,33 @@ dijkstra_(const tal_t *ctx,
continue;
/* We're going from neighbor to c, hence !which_half */
if (!channel_ok(map, c, !which_half, cur_d->cost, arg))
if (!channel_ok(map, c, !which_half, cur_d->amount, arg))
continue;
cost = cur_d->cost;
if (!amount_msat_add_fee(&cost,
c->half[!which_half].base_fee,
c->half[!which_half].proportional_fee))
if (!amount_msat_fee(&fee, cur_d->amount,
c->half[!which_half].base_fee,
c->half[!which_half].proportional_fee)) {
/* Shouldn't happen! */
continue;
}
/* cltv_delay can't overflow: only 20 bits per hop. */
risk = risk_price(cost, riskfactor,
cur_d->total_delay
+ c->half[!which_half].delay);
score = path_score(cur_d->distance + 1, cost, risk, !which_half, c);
risk = risk_price(cur_d->amount, riskfactor, c->half[!which_half].delay);
score = channel_score(fee, risk, cur_d->amount, !which_half, c);
/* That score is on top of current score */
if (add_overflows_u64(score, cur_d->score))
continue;
score += cur_d->score;
if (score >= d->score)
continue;
/* Shouldn't happen! */
if (!amount_msat_add(&d->amount, cur_d->amount, fee))
continue;
d->distance = cur_d->distance + 1;
d->total_delay = cur_d->total_delay
+ c->half[!which_half].delay;
d->cost = cost;
d->best_chan = c;
d->score = score;
gheap_restore_heap_after_item_increase(&gheap_ctx,

15
common/dijkstra.h
View File

@@ -19,21 +19,22 @@ dijkstra_(const tal_t *ctx,
int dir,
struct amount_msat amount,
void *arg),
u64 (*path_score)(u32 distance,
struct amount_msat cost,
struct amount_msat risk,
int dir,
const struct gossmap_chan *c),
u64 (*channel_score)(struct amount_msat fee,
struct amount_msat risk,
/* We want to know this to compare with channel capacity */
struct amount_msat total,
int dir,
const struct gossmap_chan *c),
void *arg);
#define dijkstra(ctx, map, start, amount, riskfactor, channel_ok, \
path_score, arg) \
channel_score, arg) \
dijkstra_((ctx), (map), (start), (amount), (riskfactor), \
typesafe_cb_preargs(bool, void *, (channel_ok), (arg), \
const struct gossmap *, \
const struct gossmap_chan *, \
int, struct amount_msat), \
(path_score), \
(channel_score), \
(arg))
/* Returns UINT_MAX if unreachable. */

16
common/route.c
View File

@@ -33,33 +33,33 @@ bool route_can_carry(const struct gossmap *map,
}
/* Squeeze total costs into a u32 */
static u32 costs_to_score(struct amount_msat cost,
static u32 costs_to_score(struct amount_msat fee,
struct amount_msat risk)
{
u64 costs = cost.millisatoshis + risk.millisatoshis; /* Raw: score */
u64 costs = fee.millisatoshis + risk.millisatoshis; /* Raw: score */
if (costs > 0xFFFFFFFF)
costs = 0xFFFFFFFF;
return costs;
}
/* Prioritize distance over costs */
u64 route_score_shorter(u32 distance,
struct amount_msat cost,
u64 route_score_shorter(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total UNUSED,
int dir UNUSED,
const struct gossmap_chan *c UNUSED)
{
return costs_to_score(cost, risk) + ((u64)distance << 32);
return costs_to_score(fee, risk) + ((u64)1 << 32);
}
/* Prioritize costs over distance */
u64 route_score_cheaper(u32 distance,
struct amount_msat cost,
u64 route_score_cheaper(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total UNUSED,
int dir UNUSED,
const struct gossmap_chan *c UNUSED)
{
return ((u64)costs_to_score(cost, risk) << 32) + distance;
return ((u64)costs_to_score(fee, risk) << 32) + 1;
}
/* Recursive version: return false if we can't get there.

8
common/route.h
View File

@@ -43,16 +43,16 @@ bool route_can_carry_even_disabled(const struct gossmap *map,
void *unused);
/* Shortest path, with lower amount tiebreak */
u64 route_score_shorter(u32 distance,
struct amount_msat cost,
u64 route_score_shorter(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total,
int dir UNUSED,
const struct gossmap_chan *c UNUSED);
/* Cheapest path, with shorter path tiebreak */
u64 route_score_cheaper(u32 distance,
struct amount_msat cost,
u64 route_score_cheaper(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total,
int dir UNUSED,
const struct gossmap_chan *c UNUSED);

51
common/test/run-route-infloop.c
View File

@@ -47,10 +47,10 @@ void towire_tlv(u8 **pptr UNNEEDED,
/* Node id 03942f5fe67645fdce4584e7f159c1f0a396b05fbc15f0fb7d6e83c553037b1c73 */
static struct gossmap *gossmap;
static u64 capacity_bias(const struct gossmap *map,
const struct gossmap_chan *c,
int dir,
struct amount_msat amount)
static double capacity_bias(const struct gossmap *map,
const struct gossmap_chan *c,
int dir,
struct amount_msat amount)
{
struct amount_sat capacity;
u64 amtmsat = amount.millisatoshis; /* Raw: lengthy math */
@@ -64,22 +64,41 @@ static u64 capacity_bias(const struct gossmap *map,
return -log((capmsat + 1 - amtmsat) / (capmsat + 1));
}
static u64 route_score(u32 distance,
struct amount_msat cost,
/* Prioritize costs over distance, but bias to larger channels. */
static u64 route_score(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total,
int dir,
const struct gossmap_chan *c)
{
u64 cmsat = cost.millisatoshis; /* Raw: lengthy math */
u64 rmsat = risk.millisatoshis; /* Raw: lengthy math */
u64 bias = capacity_bias(gossmap, c, dir, cost);
double score;
struct amount_msat msat;
/* Smoothed harmonic mean to avoid division by 0 */
u64 costs = (cmsat * rmsat * bias) / (cmsat + rmsat + bias + 1);
/* These two are comparable, so simply sum them. */
if (!amount_msat_add(&msat, fee, risk))
msat = AMOUNT_MSAT(-1ULL);
if (costs > 0xFFFFFFFF)
costs = 0xFFFFFFFF;
return costs;
/* Slight tiebreaker bias: 1 msat per distance */
if (!amount_msat_add(&msat, msat, AMOUNT_MSAT(1)))
msat = AMOUNT_MSAT(-1ULL);
/* Percent penalty at different channel capacities:
* 1%: 1%
* 10%: 11%
* 25%: 29%
* 50%: 69%
* 75%: 138%
* 90%: 230%
* 95%: 300%
* 99%: 461%
*/
score = (capacity_bias(gossmap, c, dir, total) + 1)
* msat.millisatoshis; /* Raw: Weird math */
if (score > 0xFFFFFFFF)
return 0xFFFFFFFF;
/* Cast unnecessary, but be explicit! */
return (u64)score;
}
int main(int argc, char *argv[])
@@ -92,9 +111,9 @@ int main(int argc, char *argv[])
common_setup(argv[0]);
/* 8388607 crashes, use half that so we don't break CI! */
/* This used to crash! */
if (!argv[1])
amt = 8388607 / 2;
amt = 8388607;
else
amt = atol(argv[1]);
gossmap = gossmap_load(tmpctx, "tests/data/routing_gossip_store", NULL);

18
plugins/libplugin-pay.c
View File

@@ -765,21 +765,21 @@ static double capacity_bias(const struct gossmap *map,
}
/* Prioritize costs over distance, but bias to larger channels. */
static u64 route_score(u32 distance,
struct amount_msat cost,
static u64 route_score(struct amount_msat fee,
struct amount_msat risk,
struct amount_msat total,
int dir,
const struct gossmap_chan *c)
{
double costs;
double score;
struct amount_msat msat;
/* These two are comparable, so simply sum them. */
if (!amount_msat_add(&msat, cost, risk))
if (!amount_msat_add(&msat, fee, risk))
msat = AMOUNT_MSAT(-1ULL);
/* Slight tiebreaker bias: 1 msat per distance */
if (!amount_msat_add(&msat, msat, amount_msat(distance)))
if (!amount_msat_add(&msat, msat, AMOUNT_MSAT(1)))
msat = AMOUNT_MSAT(-1ULL);
/* Percent penalty at different channel capacities:
@@ -792,11 +792,13 @@ static u64 route_score(u32 distance,
* 95%: 300%
* 99%: 461%
*/
costs = (capacity_bias(global_gossmap, c, dir, cost) + 1)
score = (capacity_bias(global_gossmap, c, dir, total) + 1)
* msat.millisatoshis; /* Raw: Weird math */
if (costs > 0xFFFFFFFF)
if (score > 0xFFFFFFFF)
return 0xFFFFFFFF;
return (u64)costs;
/* Cast unnecessary, but be explicit! */
return (u64)score;
}
static struct route_hop *route(const tal_t *ctx,