Skip to content

Commit

Permalink
channel_hint: minor cleanups.
Browse files Browse the repository at this point in the history
1. short_channel_id_dir should be passed by pointer, unlike short_channel_id.
2. "scid" is not a good name for a field which is not (just) an scid
   (The spec used sciddir_or_pubkey in drafts, so use sciddir).
3. ... nor is it a good member name, so fix that (we use scidd usually).
4. json_add_channel_hint is a more expected name than channel_hint_to_json.
5. Use amount_sat_to_msat instead of warning suppression.
6. Rename "total_amount" in "struct route_hop" to "capacity".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  • Loading branch information
rustyrussell committed Aug 9, 2024
1 parent 564555c commit acc063d
Show file tree
Hide file tree
Showing 12 changed files with 59 additions and 55 deletions.
1 change: 1 addition & 0 deletions common/json_parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <bitcoin/privkey.h>
#include <bitcoin/psbt.h>
#include <bitcoin/pubkey.h>
#include <bitcoin/short_channel_id.h>
#include <bitcoin/tx.h>
#include <ccan/json_escape/json_escape.h>
#include <ccan/mem/mem.h>
Expand Down
2 changes: 1 addition & 1 deletion common/json_parse.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#ifndef LIGHTNING_COMMON_JSON_PARSE_H
#define LIGHTNING_COMMON_JSON_PARSE_H
#include "config.h"
#include <bitcoin/short_channel_id.h>
#include <ccan/crypto/sha256/sha256.h>
#include <common/coin_mvt.h>
#include <common/errcode.h>
Expand All @@ -18,6 +17,7 @@ struct secret;
struct pubkey;
struct node_id;
struct short_channel_id;
struct short_channel_id_dir;
struct amount_sat;
struct amount_msat;
struct bitcoin_txid;
Expand Down
10 changes: 5 additions & 5 deletions common/json_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -485,13 +485,13 @@ void json_add_short_channel_id(struct json_stream *response,
}

void json_add_short_channel_id_dir(struct json_stream *response,
const char *fieldname,
struct short_channel_id_dir scidd)
const char *fieldname,
const struct short_channel_id_dir *scidd)
{
json_add_str_fmt(response, fieldname, "%dx%dx%d/%d",
short_channel_id_blocknum(scidd.scid),
short_channel_id_txnum(scidd.scid),
short_channel_id_outnum(scidd.scid), scidd.dir);
short_channel_id_blocknum(scidd->scid),
short_channel_id_txnum(scidd->scid),
short_channel_id_outnum(scidd->scid), scidd->dir);
}

static void json_add_address_fields(struct json_stream *response,
Expand Down
3 changes: 1 addition & 2 deletions common/json_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ struct channel_id;
struct channel_type;
struct bitcoin_txid;
struct bitcoin_outpoint;
struct short_channel_id;
struct sha256;
struct preimage;
struct bitcoin_tx;
Expand Down Expand Up @@ -317,7 +316,7 @@ void json_add_short_channel_id(struct json_stream *response,
/* '"fieldname" : "1234:5:6/1"' */
void json_add_short_channel_id_dir(struct json_stream *response,
const char *fieldname,
struct short_channel_id_dir idd);
const struct short_channel_id_dir *scidd);

/* JSON serialize a network address for a node */
void json_add_address(struct json_stream *response, const char *fieldname,
Expand Down
3 changes: 2 additions & 1 deletion common/route.c
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ static bool dijkstra_to_hops(struct route_hop **hops,
return false;

gossmap_chan_get_capacity(gossmap, c, &total);
(*hops)[num_hops].total_amount.millisatoshis = total.satoshis * 1000; /* Raw: simpler. */
if (!amount_sat_to_msat(&(*hops)[num_hops].capacity, total))
abort();
(*hops)[num_hops].amount = *amount;
(*hops)[num_hops].delay = *cltv;

Expand Down
4 changes: 2 additions & 2 deletions common/route.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +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.
* @total_amount: The amount the channel was funded with.
* @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 total_amount;
struct amount_msat capacity;
u32 delay;
};

Expand Down
23 changes: 12 additions & 11 deletions plugins/channel_hint.c
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
#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)
void json_add_channel_hint(struct json_stream *js,
const char *fieldname,
const struct channel_hint *hint)
{
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",
json_object_start(js, fieldname);
json_add_u32(js, "timestamp", hint->timestamp);
json_add_short_channel_id_dir(js, "sciddir", &hint->scidd);
json_add_amount_msat(js, "estimated_capacity_msat",
hint->estimated_capacity);
json_add_amount_msat(dest, "overall_capacity_msat",
json_add_amount_msat(js, "overall_capacity_msat",
hint->overall_capacity);
json_add_bool(dest, "enabled", hint->enabled);
json_object_end(dest);
json_add_bool(js, "enabled", hint->enabled);
json_object_end(js);
}

#define PAY_REFILL_TIME 7200
Expand Down Expand Up @@ -83,9 +84,9 @@ struct channel_hint *channel_hint_from_json(const tal_t *ctx,
const char *ret;
struct channel_hint *hint = tal(ctx, struct channel_hint);
ret = json_scan(ctx, buffer, toks,
"{timestamp:%,scid:%,estimated_capacity_msat:%,overall_capacity_msat:%,enabled:%}",
"{timestamp:%,sciddir:%,estimated_capacity_msat:%,overall_capacity_msat:%,enabled:%}",
JSON_SCAN(json_to_u32, &hint->timestamp),
JSON_SCAN(json_to_short_channel_id_dir, &hint->scid),
JSON_SCAN(json_to_short_channel_id_dir, &hint->scidd),
JSON_SCAN(json_to_msat, &hint->estimated_capacity),
JSON_SCAN(json_to_bool, &hint->enabled));
Expand Down
8 changes: 5 additions & 3 deletions plugins/channel_hint.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ struct channel_hint {
* local alias. The `pay` algorithm doesn't really care which
* one it is, but we'll prefer the scid as that's likely more
* readable than the alias. */
struct short_channel_id_dir scid;
struct short_channel_id_dir scidd;

/* Upper bound on remove channels inferred from payment failures. */
struct amount_msat estimated_capacity;
Expand All @@ -54,8 +54,10 @@ struct channel_hint_set {
};

bool channel_hint_update(const struct timeabs now,
struct channel_hint *hint);
struct channel_hint *hint);

void channel_hint_to_json(const char *name, const struct channel_hint *hint, struct json_stream *dest);
void json_add_channel_hint(struct json_stream *js,
const char *fieldname,
const struct channel_hint *hint);

#endif /* LIGHTNING_PLUGINS_CHANNEL_HINT_H */
54 changes: 27 additions & 27 deletions plugins/libplugin-pay.c
Original file line number Diff line number Diff line change
Expand Up @@ -401,12 +401,12 @@ static void channel_hint_notify(struct plugin *plugin,
plugin_notification_start(plugin, "channel_hint_update");

/* The timestamp used to decay the observation over time. */
channel_hint_to_json("channel_hint", hint, js);
json_add_channel_hint(js, "channel_hint", hint);
plugin_notification_end(plugin, js);
}

static void channel_hints_update(struct payment *p,
const struct short_channel_id scid,
struct short_channel_id scid,
int direction, bool enabled, bool local,
const struct amount_msat *estimated_capacity,
const struct amount_msat overall_capacity,
Expand All @@ -423,8 +423,8 @@ static void channel_hints_update(struct payment *p,
/* Try and look for an existing hint: */
for (size_t i=0; i<tal_count(root->channel_hints); i++) {
struct channel_hint *hint = &root->channel_hints[i];
if (short_channel_id_eq(hint->scid.scid, scid) &&
hint->scid.dir == direction) {
if (short_channel_id_eq(hint->scidd.scid, scid) &&
hint->scidd.dir == direction) {
bool modified = false;
/* Prefer to disable a channel. */
if (!enabled && hint->enabled) {
Expand Down Expand Up @@ -452,7 +452,7 @@ static void channel_hints_update(struct payment *p,
"enabled %s, "
"estimated capacity %s",
fmt_short_channel_id_dir(tmpctx,
&hint->scid),
&hint->scidd),
hint->enabled ? "true" : "false",
fmt_amount_msat(tmpctx,
hint->estimated_capacity));
Expand All @@ -465,8 +465,8 @@ static void channel_hints_update(struct payment *p,
/* No hint found, create one. */
newhint.enabled = enabled;
newhint.timestamp = timestamp;
newhint.scid.scid = scid;
newhint.scid.dir = direction;
newhint.scidd.scid = scid;
newhint.scidd.dir = direction;
newhint.overall_capacity = overall_capacity;
if (local) {
newhint.local = tal(root->channel_hints, struct local_hint);
Expand All @@ -484,7 +484,7 @@ static void channel_hints_update(struct payment *p,
paymod_log(
p, LOG_DBG,
"Added a channel hint for %s: enabled %s, estimated capacity %s",
fmt_short_channel_id_dir(tmpctx, &newhint.scid),
fmt_short_channel_id_dir(tmpctx, &newhint.scidd),
newhint.enabled ? "true" : "false",
fmt_amount_msat(tmpctx, newhint.estimated_capacity));
channel_hint_notify(p->plugin, &newhint);
Expand All @@ -505,7 +505,7 @@ static void payment_exclude_most_expensive(struct payment *p)
}
}
channel_hints_update(p, e->scid, e->direction, false, false, NULL,
e->total_amount, NULL);
e->capacity, NULL);
}

static void payment_exclude_longest_delay(struct payment *p)
Expand All @@ -521,7 +521,7 @@ static void payment_exclude_longest_delay(struct payment *p)
}
}
channel_hints_update(p, e->scid, e->direction, false, false, NULL,
e->total_amount, NULL);
e->capacity, NULL);
}

static struct amount_msat payment_route_fee(struct payment *p)
Expand Down Expand Up @@ -565,8 +565,8 @@ static struct channel_hint *payment_chanhints_get(struct payment *p,
struct channel_hint *curhint;
for (size_t j = 0; j < tal_count(root->channel_hints); j++) {
curhint = &root->channel_hints[j];
if (short_channel_id_eq(curhint->scid.scid, h->scid) &&
curhint->scid.dir == h->direction) {
if (short_channel_id_eq(curhint->scidd.scid, h->scid) &&
curhint->scidd.dir == h->direction) {
return curhint;
}
}
Expand Down Expand Up @@ -619,7 +619,7 @@ static bool payment_chanhints_apply_route(struct payment *p)
"for %s. Could be a concurrent "
"`getroute` call.",
fmt_short_channel_id_dir(tmpctx,
&curhint->scid));
&curhint->scidd));
paymod_log(
p, LOG_DBG,
"Capacity: estimated_capacity=%s, hop_amount=%s. "
Expand Down Expand Up @@ -701,16 +701,16 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p)
hint = &root->channel_hints[i];

if (!hint->enabled)
tal_arr_expand(&res, hint->scid);
tal_arr_expand(&res, hint->scidd);

else if (amount_msat_greater(p->our_amount,
hint->estimated_capacity))
tal_arr_expand(&res, hint->scid);
tal_arr_expand(&res, hint->scidd);

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);
tal_arr_expand(&res, hint->scidd);
}
return res;
}
Expand All @@ -728,8 +728,8 @@ static const struct channel_hint *find_hint(const struct channel_hint *hints,
int dir)
{
for (size_t i = 0; i < tal_count(hints); i++) {
if (short_channel_id_eq(scid, hints[i].scid.scid)
&& dir == hints[i].scid.dir)
if (short_channel_id_eq(scid, hints[i].scidd.scid)
&& dir == hints[i].scidd.dir)
return &hints[i];
}
return NULL;
Expand Down Expand Up @@ -1536,7 +1536,7 @@ handle_intermediate_failure(struct command *cmd,
case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING:
/* All of these result in the channel being marked as disabled. */
channel_hints_update(root, errchan->scid, errchan->direction,
false, false, NULL, errchan->total_amount,
false, false, NULL, errchan->capacity,
NULL);
break;

Expand All @@ -1555,7 +1555,7 @@ handle_intermediate_failure(struct command *cmd,
* remember the amount we tried as an estimate. */
channel_hints_update(root, errchan->scid, errchan->direction,
true, false, &estimated,
errchan->total_amount, NULL);
errchan->capacity, NULL);
goto error;
}

Expand Down Expand Up @@ -2876,7 +2876,7 @@ static bool routehint_excluded(struct payment *p,
* channel, which is greater than the destination.
*/
for (size_t j = 0; j < tal_count(hints); j++) {
if (!short_channel_id_eq(hints[j].scid.scid, r->short_channel_id))
if (!short_channel_id_eq(hints[j].scidd.scid, r->short_channel_id))
continue;
/* We exclude on equality because we set the estimate
* to the smallest failed attempt. */
Expand Down Expand Up @@ -3173,7 +3173,7 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p)
hop.amount = dest_amount;
hop.delay = route_cltv(d->final_cltv, routehint + i + 1,
tal_count(routehint) - i - 1);
hop.total_amount = estimate;
hop.capacity = estimate;

/* Should we get a failure inside the routehint we'll
* need the direction so we can exclude it. Luckily
Expand Down Expand Up @@ -3525,7 +3525,7 @@ static void direct_pay_override(struct payment *p) {
/* If we have a channel we need to make sure that it still has
* sufficient capacity. Look it up in the channel_hints. */
for (size_t i=0; i<tal_count(root->channel_hints); i++) {
struct short_channel_id_dir *cur = &root->channel_hints[i].scid;
struct short_channel_id_dir *cur = &root->channel_hints[i].scidd;
if (short_channel_id_eq(cur->scid, d->chan->scid) &&
cur->dir == d->chan->dir) {
hint = &root->channel_hints[i];
Expand All @@ -3539,14 +3539,14 @@ static void direct_pay_override(struct payment *p) {
p->route = tal_arr(p, struct route_hop, 1);
p->route[0].amount = p->our_amount;
p->route[0].delay = p->getroute->cltv;
p->route[0].scid = hint->scid.scid;
p->route[0].direction = hint->scid.dir;
p->route[0].scid = hint->scidd.scid;
p->route[0].direction = hint->scidd.dir;
p->route[0].node_id = *p->route_destination;
p->route[0].total_amount = hint->overall_capacity;
p->route[0].capacity = hint->overall_capacity;
paymod_log(p, LOG_DBG,
"Found a direct channel (%s) with sufficient "
"capacity, skipping route computation.",
fmt_short_channel_id_dir(tmpctx, &hint->scid));
fmt_short_channel_id_dir(tmpctx, &hint->scidd));

payment_set_step(p, PAYMENT_STEP_GOT_ROUTE);
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/test/run-route-calc.c
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ void json_add_short_channel_id(struct json_stream *response UNNEEDED,
/* Generated stub for json_add_short_channel_id_dir */
void json_add_short_channel_id_dir(struct json_stream *response UNNEEDED,
const char *fieldname UNNEEDED,
struct short_channel_id_dir idd UNNEEDED)
const struct short_channel_id_dir *scidd UNNEEDED)
{ fprintf(stderr, "json_add_short_channel_id_dir called!\n"); abort(); }
/* Generated stub for json_add_string */
void json_add_string(struct json_stream *js UNNEEDED,
Expand Down
2 changes: 1 addition & 1 deletion plugins/test/run-route-overlong.c
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ void json_add_short_channel_id(struct json_stream *response UNNEEDED,
/* Generated stub for json_add_short_channel_id_dir */
void json_add_short_channel_id_dir(struct json_stream *response UNNEEDED,
const char *fieldname UNNEEDED,
struct short_channel_id_dir idd UNNEEDED)
const struct short_channel_id_dir *scidd UNNEEDED)
{ fprintf(stderr, "json_add_short_channel_id_dir called!\n"); abort(); }
/* Generated stub for json_add_string */
void json_add_string(struct json_stream *js UNNEEDED,
Expand Down
2 changes: 1 addition & 1 deletion wallet/test/run-wallet.c
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,7 @@ void topology_add_sync_waiter_(const tal_t *ctx UNNEEDED,
u8 *towire_announcement_signatures(const tal_t *ctx UNNEEDED, const struct channel_id *channel_id UNNEEDED, struct short_channel_id short_channel_id UNNEEDED, const secp256k1_ecdsa_signature *node_signature UNNEEDED, const secp256k1_ecdsa_signature *bitcoin_signature UNNEEDED)
{ fprintf(stderr, "towire_announcement_signatures called!\n"); abort(); }
/* Generated stub for towire_channel_reestablish */
u8 *towire_channel_reestablish(const tal_t *ctx UNNEEDED, const struct channel_id *channel_id UNNEEDED, u64 next_commitment_number UNNEEDED, u64 next_revocation_number UNNEEDED, const struct secret *your_last_per_commitment_secret UNNEEDED, const struct pubkey *my_current_per_commitment_point UNNEEDED, const struct tlv_channel_reestablish_tlvs *tlvs UNNEEDED)
u8 *towire_channel_reestablish(const tal_t *ctx UNNEEDED, const struct channel_id *channel_id UNNEEDED, u64 next_commitment_number UNNEEDED, u64 next_revocation_number UNNEEDED, const struct secret *your_last_per_commitment_secret UNNEEDED, const struct pubkey *my_current_per_commitment_point UNNEEDED, const struct tlv_channel_reestablish_tlvs *channel_reestablish UNNEEDED)
{ fprintf(stderr, "towire_channel_reestablish called!\n"); abort(); }
/* Generated stub for towire_channeld_dev_memleak */
u8 *towire_channeld_dev_memleak(const tal_t *ctx UNNEEDED)
Expand Down

0 comments on commit acc063d

Please sign in to comment.