From 1bf3eebbf61577cd6c9c1bcdeeb3333f1ca58889 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 20 Oct 2020 14:28:06 +1030 Subject: [PATCH] dijkstra: fix heap ordering. We were always ordering heap by distance, not score (which are different if we are routing by cheapest, not shortest!). This simplifies our callbacks, too. Signed-off-by: Rusty Russell --- common/dijkstra.c | 42 +++++++++++++++++----------------------- common/dijkstra.h | 19 +++++------------- common/route.c | 47 +++++++++++++++------------------------------ common/route.h | 18 ++++++----------- devtools/route.c | 2 +- devtools/topology.c | 6 +++--- 6 files changed, 49 insertions(+), 85 deletions(-) diff --git a/common/dijkstra.c b/common/dijkstra.c index 824b82e97..ac2fb309a 100644 --- a/common/dijkstra.c +++ b/common/dijkstra.c @@ -22,6 +22,9 @@ struct dijkstra { /* NULL means it's been visited already. */ const struct gossmap_node **heapptr; + /* How we decide "best" */ + u64 score; + /* We could re-evaluate to determine this, but keeps it simple */ struct gossmap_chan *best_chan; }; @@ -67,9 +70,9 @@ static int less_comparer(const void *const ctx, const void *const b) { return get_dijkstra(global_dijkstra, global_map, - *(struct gossmap_node **)a)->distance + *(struct gossmap_node **)a)->score > get_dijkstra(global_dijkstra, global_map, - *(struct gossmap_node **)b)->distance; + *(struct gossmap_node **)b)->score; } static void item_mover(void *const dst, const void *const src) @@ -101,6 +104,7 @@ static const struct gossmap_node **mkheap(const tal_t *ctx, d->distance = 0; d->total_delay = 0; d->cost = sent; + d->score = 0; i--; } else { heap[i] = n; @@ -108,6 +112,7 @@ static const struct gossmap_node **mkheap(const tal_t *ctx, d->distance = UINT_MAX; d->cost = AMOUNT_MSAT(-1ULL); d->total_delay = 0; + d->score = -1ULL; } } assert(i == tal_count(heap)); @@ -142,13 +147,9 @@ dijkstra_(const tal_t *ctx, int dir, struct amount_msat amount, void *arg), - bool (*path_better)(u32 old_distance, - u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *arg), + u64 (*path_score)(u32 distance, + struct amount_msat cost, + struct amount_msat risk), void *arg) { struct dijkstra *dij; @@ -229,7 +230,8 @@ dijkstra_(const tal_t *ctx, int which_half; struct gossmap_chan *c; struct dijkstra *d; - struct amount_msat cost, new_risk, old_risk; + struct amount_msat cost, risk; + u64 score; c = gossmap_nth_chan(map, cur, i, &which_half); neighbor = gossmap_nth_node(map, c, !which_half); @@ -251,20 +253,11 @@ dijkstra_(const tal_t *ctx, continue; /* cltv_delay can't overflow: only 20 bits per hop. */ - new_risk = risk_price(cost, riskfactor, - cur_d->total_delay - + c->half[!which_half].delay); - - old_risk = risk_price(d->cost, riskfactor, - d->total_delay); - - if (!path_better(d->distance, - cur_d->distance + 1, - d->cost, - cost, - old_risk, - new_risk, - arg)) + risk = risk_price(cost, riskfactor, + cur_d->total_delay + + c->half[!which_half].delay); + score = path_score(cur_d->distance + 1, cost, risk); + if (score >= d->score) continue; d->distance = cur_d->distance + 1; @@ -272,6 +265,7 @@ dijkstra_(const tal_t *ctx, + c->half[!which_half].delay; d->cost = cost; d->best_chan = c; + d->score = score; gheap_restore_heap_after_item_increase(&gheap_ctx, heap, heapsize, d->heapptr - heap); diff --git a/common/dijkstra.h b/common/dijkstra.h index b8f2c2ff8..d20e08fc7 100644 --- a/common/dijkstra.h +++ b/common/dijkstra.h @@ -21,28 +21,19 @@ dijkstra_(const tal_t *ctx, int dir, struct amount_msat amount, void *arg), - bool (*path_better)(u32 old_distance, - u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *arg), + u64 (*path_score)(u32 distance, + struct amount_msat cost, + struct amount_msat risk), void *arg); #define dijkstra(ctx, map, start, amount, riskfactor, channel_ok, \ - path_better, arg) \ + path_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), \ - typesafe_cb_preargs(bool, void *, (path_better), (arg), \ - u32, u32, \ - struct amount_msat, \ - struct amount_msat, \ - struct amount_msat, \ - struct amount_msat), \ + (path_score), \ (arg)) /* Returns UINT_MAX if unreachable. */ diff --git a/common/route.c b/common/route.c index a3ae3ac01..e734a3cbe 100644 --- a/common/route.c +++ b/common/route.c @@ -37,43 +37,28 @@ bool route_can_carry(const struct gossmap *map, return route_can_carry_even_disabled(map, c, dir, amount, arg); } -bool route_path_shorter(u32 old_distance, u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *unused) +/* Prioritize distance over costs */ +u64 route_score_shorter(u32 distance, + struct amount_msat cost, + struct amount_msat risk) { - if (new_distance > old_distance) - return false; - if (new_distance < old_distance) - return true; + u64 costs = cost.millisatoshis + risk.millisatoshis; /* Raw: score */ + if (costs > 0xFFFFFFFF) + costs = 0xFFFFFFFF; - /* Tiebreak by cost */ - if (!amount_msat_add(&old_cost, old_cost, old_risk) - || !amount_msat_add(&new_cost, new_cost, new_risk)) - return false; - return amount_msat_less(new_cost, old_cost); + return costs + ((u64)distance << 32); } -bool route_path_cheaper(u32 old_distance, u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *unused) +/* Prioritize costs over distance */ +u64 route_score_cheaper(u32 distance, + struct amount_msat cost, + struct amount_msat risk) { - if (!amount_msat_add(&old_cost, old_cost, old_risk) - || !amount_msat_add(&new_cost, new_cost, new_risk)) - return false; - - if (amount_msat_greater(new_cost, old_cost)) - return false; - if (amount_msat_less(new_cost, old_cost)) - return true; + u64 costs = cost.millisatoshis + risk.millisatoshis; /* Raw: score */ + if (costs > 0xFFFFFFFF) + costs = 0xFFFFFFFF; - /* Tiebreak by distance */ - return new_distance < old_distance; + return (costs << 32) + distance; } struct route **route_from_dijkstra(const struct gossmap *map, diff --git a/common/route.h b/common/route.h index 91961844f..b864c23d1 100644 --- a/common/route.h +++ b/common/route.h @@ -27,20 +27,14 @@ bool route_can_carry_even_disabled(const struct gossmap *map, void *unused); /* Shortest path, with lower amount tiebreak */ -bool route_path_shorter(u32 old_distance, u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *unused); +u64 route_score_shorter(u32 distance, + struct amount_msat cost, + struct amount_msat risk); /* Cheapest path, with shorter path tiebreak */ -bool route_path_cheaper(u32 old_distance, u32 new_distance, - struct amount_msat old_cost, - struct amount_msat new_cost, - struct amount_msat old_risk, - struct amount_msat new_risk, - void *unused); +u64 route_score_cheaper(u32 distance, + struct amount_msat cost, + struct amount_msat risk); /* Extract route tal_arr from completed dijkstra */ struct route **route_from_dijkstra(const struct gossmap *map, diff --git a/devtools/route.c b/devtools/route.c index 864513af2..91d54f6af 100644 --- a/devtools/route.c +++ b/devtools/route.c @@ -52,7 +52,7 @@ static struct route **least_cost(struct gossmap *map, tstart = time_mono(); dij = dijkstra(tmpctx, map, dst, sent, riskfactor, route_can_carry, - route_path_cheaper, NULL); + route_score_cheaper, NULL); tstop = time_mono(); printf("# Time to find route: %"PRIu64" usec\n", diff --git a/devtools/topology.c b/devtools/topology.c index b311b1812..e2eb2d9ff 100644 --- a/devtools/topology.c +++ b/devtools/topology.c @@ -55,7 +55,7 @@ static size_t count_possible_sources(const struct gossmap *map, size_t distance_budget, num; dij = dijkstra(tmpctx, map, n, AMOUNT_MSAT(0), 0, - channel_usable_to_excl, route_path_shorter, exclude); + channel_usable_to_excl, route_score_shorter, exclude); if (is_last_node) distance_budget = ROUTING_MAX_HOPS - 1; @@ -132,7 +132,7 @@ static size_t count_possible_destinations(const struct gossmap *map, size_t distance_budget, num; dij = dijkstra(tmpctx, map, start, AMOUNT_MSAT(0), 0, - channel_usable_from_excl, route_path_shorter, exclude); + channel_usable_from_excl, route_score_shorter, exclude); if (is_first_node) distance_budget = ROUTING_MAX_HOPS - 1; @@ -191,7 +191,7 @@ static bool measure_least_cost(struct gossmap *map, tstart = time_mono(); dij = dijkstra(tmpctx, map, dst, sent, riskfactor, channel_usable, - route_path_cheaper, NULL); + route_score_cheaper, NULL); tstop = time_mono(); printf("# Time to find path: %"PRIu64" usec\n",