Skip to content

Renepay patch htlc_max=0 cases #7159

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.

Sign up for GitHub

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

Merged
rustyrussell merged 3 commits into from
Apr 2, 2024
Merged

Renepay patch htlc_max=0 cases #7159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e99256
add more fields to gossmods_listpeerchannels
Lagrang3 Mar 18, 2024
d94d15d
renepay: limit arc capacities by htlc_max
Lagrang3 Mar 27, 2024
e5c705f
remove is_local variable from listpeerchannels cb
Lagrang3 Apr 2, 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
31 changes: 15 additions & 16 deletions common/gossmods_listpeerchannels.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ void gossmod_add_localchan(struct gossmap_localmods *mods,
const struct node_id *self,
const struct node_id *peer,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat htlcmin,
struct amount_msat htlcmax,
struct amount_msat spendable,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand All @@ -19,6 +20,11 @@ void gossmod_add_localchan(struct gossmap_localmods *mods,
const jsmntok_t *chantok UNUSED,
void *cbarg UNUSED)
{
struct amount_msat min = htlcmin, max = htlcmax;

if (amount_msat_less(spendable, max))
max = spendable;

/* FIXME: features? */
gossmap_local_addchan(mods, self, peer, scidd->scid, NULL);

Expand All @@ -40,8 +46,9 @@ gossmods_from_listpeerchannels_(const tal_t *ctx,
const struct node_id *self,
const struct node_id *peer,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat htlcmin,
struct amount_msat htlcmax,
struct amount_msat sr_able,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand Down Expand Up @@ -130,10 +137,6 @@ gossmods_from_listpeerchannels_(const tal_t *ctx,
&& !streq(state, "CHANNELD_AWAITING_SPLICE"))
enabled = false;

/* Cut htlc max to spendable. */
if (amount_msat_less(spendable, htlc_max[LOCAL]))
htlc_max[LOCAL] = spendable;

/* We route better if we know we won't charge
* ourselves fees (though if fees are a signal on what
* channel we prefer to use, this ignores that
Expand All @@ -146,22 +149,18 @@ gossmods_from_listpeerchannels_(const tal_t *ctx,

/* We add both directions */
cb(mods, self, &dst, &scidd, htlc_min[LOCAL], htlc_max[LOCAL],
fee_base[LOCAL], fee_proportional[LOCAL], cltv_delta[LOCAL],
enabled, buf, channel, cbarg);
spendable, fee_base[LOCAL], fee_proportional[LOCAL],
cltv_delta[LOCAL], enabled, buf, channel, cbarg);

/* If we didn't have a remote update, it's not usable yet */
if (fee_proportional[REMOTE] == -1U)
continue;

scidd.dir = !scidd.dir;

/* Cut htlc max to receivable. */
if (amount_msat_less(receivable, htlc_max[REMOTE]))
htlc_max[REMOTE] = receivable;

cb(mods, self, &dst, &scidd, htlc_min[REMOTE], htlc_max[REMOTE],
fee_base[REMOTE], fee_proportional[REMOTE], cltv_delta[REMOTE],
enabled, buf, channel, cbarg);
receivable, fee_base[REMOTE], fee_proportional[REMOTE],
cltv_delta[REMOTE], enabled, buf, channel, cbarg);
}

return mods;
Expand Down
11 changes: 7 additions & 4 deletions common/gossmods_listpeerchannels.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ struct gossmap_localmods *gossmods_from_listpeerchannels_(const tal_t *ctx,
const struct node_id *self_,
const struct node_id *peer,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat htlcmin,
struct amount_msat htlcmax,
struct amount_msat spendable,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand All @@ -51,6 +52,7 @@ struct gossmap_localmods *gossmods_from_listpeerchannels_(const tal_t *ctx,
struct amount_msat, \
struct amount_msat, \
struct amount_msat, \
struct amount_msat, \
u32, \
u32, \
bool, \
Expand All @@ -63,8 +65,9 @@ void gossmod_add_localchan(struct gossmap_localmods *mods,
const struct node_id *self,
const struct node_id *peer,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat htlcmin,
struct amount_msat htlcmax,
struct amount_msat spendable,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand Down
9 changes: 8 additions & 1 deletion plugins/renepay/mcf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,11 +483,18 @@ static bool linearize_channel(const struct pay_parameters *params,
a = MAX(a,0);
b = MAX(a+1,b);

/* An extra bound on capacity, here we use it to reduce the flow such
* that it does not exceed htlcmax. */
s64 cap_on_capacity =
channel_htlc_max(c, dir).millisatoshis/1000; /* Raw: linearize_channel */

capacity[0]=a;
cost[0]=0;
for(size_t i=1;i<CHANNEL_PARTS;++i)
{
capacity[i] = params->cap_fraction[i]*(b-a);
capacity[i] = MIN(params->cap_fraction[i]*(b-a), cap_on_capacity);
cap_on_capacity -= capacity[i];
Comment thread
Lagrang3 marked this conversation as resolved.
assert(cap_on_capacity>=0);

cost[i] = params->cost_fraction[i]
*params->amount.millisatoshis /* Raw: linearize_channel */
Expand Down
29 changes: 24 additions & 5 deletions plugins/renepay/pay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,9 @@ static void gossmod_cb(struct gossmap_localmods *mods,
const struct node_id *self,
const struct node_id *peer,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat htlcmin,
struct amount_msat htlcmax,
struct amount_msat spendable,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand All @@ -468,9 +469,27 @@ static void gossmod_cb(struct gossmap_localmods *mods,
const jsmntok_t *chantok,
struct payment *payment)
{
/* Add to gossmap like normal */
gossmod_add_localchan(mods, self, peer, scidd, min, max,
fee_base, fee_proportional, cltv_delta, enabled, buf, chantok, NULL);
struct amount_msat min, max;

if (scidd->dir == node_id_idx(self, peer)) {
/* our side of the channel can send up to what's spendable */
min = AMOUNT_MSAT(0);
max = spendable;
} else {
/* the remote side can send up to no more than spendable */
min = htlcmin;
max = amount_msat_min(spendable, htlcmax);
}
Comment thread
rustyrussell marked this conversation as resolved.

/* FIXME: features? */
gossmap_local_addchan(mods, self, peer, scidd->scid, NULL);

gossmap_local_updatechan(mods, scidd->scid, min, max,
fee_base.millisatoshis, /* Raw: gossmap */
fee_proportional,
cltv_delta,
enabled,
scidd->dir);

/* Also update uncertainty map */
uncertainty_network_update_from_listpeerchannels(payment, scidd, max, enabled,
Expand Down
10 changes: 6 additions & 4 deletions plugins/test/run-route-calc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ void gossmod_add_localchan(struct gossmap_localmods *mods UNNEEDED,
const struct node_id *self UNNEEDED,
const struct node_id *peer UNNEEDED,
const struct short_channel_id_dir *scidd UNNEEDED,
struct amount_msat min UNNEEDED,
struct amount_msat max UNNEEDED,
struct amount_msat htlcmin UNNEEDED,
struct amount_msat htlcmax UNNEEDED,
struct amount_msat spendable UNNEEDED,
struct amount_msat fee_base UNNEEDED,
u32 fee_proportional UNNEEDED,
u32 cltv_delta UNNEEDED,
Expand All @@ -62,8 +63,9 @@ struct gossmap_localmods *gossmods_from_listpeerchannels_(const tal_t *ctx UNNEE
const struct node_id *self_ UNNEEDED,
const struct node_id *peer UNNEEDED,
const struct short_channel_id_dir *scidd UNNEEDED,
struct amount_msat min UNNEEDED,
struct amount_msat max UNNEEDED,
struct amount_msat htlcmin UNNEEDED,
struct amount_msat htlcmax UNNEEDED,
struct amount_msat spendable UNNEEDED,
struct amount_msat fee_base UNNEEDED,
u32 fee_proportional UNNEEDED,
u32 cltv_delta UNNEEDED,
Expand Down
10 changes: 6 additions & 4 deletions plugins/test/run-route-overlong.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ void gossmod_add_localchan(struct gossmap_localmods *mods UNNEEDED,
const struct node_id *self UNNEEDED,
const struct node_id *peer UNNEEDED,
const struct short_channel_id_dir *scidd UNNEEDED,
struct amount_msat min UNNEEDED,
struct amount_msat max UNNEEDED,
struct amount_msat htlcmin UNNEEDED,
struct amount_msat htlcmax UNNEEDED,
struct amount_msat spendable UNNEEDED,
struct amount_msat fee_base UNNEEDED,
u32 fee_proportional UNNEEDED,
u32 cltv_delta UNNEEDED,
Expand All @@ -59,8 +60,9 @@ struct gossmap_localmods *gossmods_from_listpeerchannels_(const tal_t *ctx UNNEE
const struct node_id *self_ UNNEEDED,
const struct node_id *peer UNNEEDED,
const struct short_channel_id_dir *scidd UNNEEDED,
struct amount_msat min UNNEEDED,
struct amount_msat max UNNEEDED,
struct amount_msat htlcmin UNNEEDED,
struct amount_msat htlcmax UNNEEDED,
struct amount_msat spendable UNNEEDED,
struct amount_msat fee_base UNNEEDED,
u32 fee_proportional UNNEEDED,
u32 cltv_delta UNNEEDED,
Expand Down
3 changes: 2 additions & 1 deletion plugins/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ static void gossmod_add_unknown_localchan(struct gossmap_localmods *mods,
const struct short_channel_id_dir *scidd,
struct amount_msat min,
struct amount_msat max,
struct amount_msat spendable,
struct amount_msat fee_base,
u32 fee_proportional,
u32 cltv_delta,
Expand All @@ -375,7 +376,7 @@ static void gossmod_add_unknown_localchan(struct gossmap_localmods *mods,
if (gossmap_find_chan(gossmap, &scidd->scid))
return;

gossmod_add_localchan(mods, self, peer, scidd, min, max,
gossmod_add_localchan(mods, self, peer, scidd, min, max, spendable,
fee_base, fee_proportional, cltv_delta, enabled,
buf, chantok, gossmap);
}
Expand Down
61 changes: 59 additions & 2 deletions tests/test_renepay.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,12 +489,12 @@ def test_htlc_max(node_factory):
]
)

inv = l6.rpc.invoice("1800000sat", "inv", "description")
inv = l6.rpc.invoice("800000sat", "inv", "description")

l1.rpc.call("renepay", {"invstring": inv["bolt11"]})
l1.wait_for_htlcs()
invoice = only_one(l6.rpc.listinvoices("inv")["invoices"])
assert invoice["amount_received_msat"] >= Millisatoshi("1800000sat")
assert invoice["amount_received_msat"] >= Millisatoshi("800000sat")


def test_previous_sendpays(node_factory, bitcoind):
Expand Down Expand Up @@ -624,3 +624,60 @@ def test_fees(node_factory):
source.rpc.call("renepay", {"invstring": invstr})
invoice = only_one(dest.rpc.listinvoices("inv2")["invoices"])
assert invoice["amount_received_msat"] == Millisatoshi("150000sat")


def test_local_htlcmax0(node_factory):
"""Testing a simple pay route when local channels have htlcmax=0."""
l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True)
l1.rpc.setchannel(l2.info["id"], htlcmax=0)
inv = l3.rpc.invoice(123000, "test_renepay", "description")["bolt11"]
details = l1.rpc.call("renepay", {"invstring": inv})
assert details["status"] == "complete"
assert details["amount_msat"] == Millisatoshi(123000)
assert details["destination"] == l3.info["id"]


def test_htlcmax0(node_factory):
"""
Topology:
1----2----4
| |
3----5----6
Tests the plugin when some routes have htlc_max=0.
"""
opts = [
{"disable-mpp": None, "fee-base": 0, "fee-per-satoshi": 0},
{"disable-mpp": None, "fee-base": 0, "fee-per-satoshi": 0},
{"disable-mpp": None, "fee-base": 0, "fee-per-satoshi": 0},
{
"disable-mpp": None,
"fee-base": 0,
"fee-per-satoshi": 0,
"htlc-maximum-msat": 0,
},
{
"disable-mpp": None,
"fee-base": 0,
"fee-per-satoshi": 0,
"htlc-maximum-msat": 800000000,
},
{"disable-mpp": None, "fee-base": 0, "fee-per-satoshi": 0},
]
l1, l2, l3, l4, l5, l6 = node_factory.get_nodes(6, opts=opts)
start_channels(
[
(l1, l2, 10000000),
(l2, l4, 1000000),
(l4, l6, 1000000),
(l1, l3, 10000000),
(l3, l5, 1000000),
(l5, l6, 1000000),
]
)

inv = l6.rpc.invoice("600000sat", "inv", "description")

l1.rpc.call("renepay", {"invstring": inv["bolt11"]})
l1.wait_for_htlcs()
invoice = only_one(l6.rpc.listinvoices("inv")["invoices"])
assert invoice["amount_received_msat"] >= Millisatoshi("600000sat")
Comment on lines +680 to +683

@vincenzopalazzo vincenzopalazzo Mar 29, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not relevant to this PR, but I think that maybe would be good for this kind of testing to know what is the path that the payment is taking.

Maybe we can add this as a pay result?

@Lagrang3 Lagrang3 Apr 1, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Neither pay does this, we get this kind of information from logs alone.

@vincenzopalazzo vincenzopalazzo Apr 1, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Neither pay does this, we get this kind of information from logs alone.

My idea was also to implement it inside vanilla pay

@rustyrussell rustyrussell Apr 2, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice if renepay gave the successful paths in the response, though that's pretty verbose!

@Lagrang3 Lagrang3 Apr 2, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What if the response from waitsendpay (as well as the payment success/failure notification) had a field with the route?
In renepay there is an internal database of these routes pushed to sendpay so that the plugin knows when a specific (groupid, partid) fail or succeeds it can recover which route was that and act accordingly, for example by updating the uncertainty/knowledge. If the route was already provided through the result of waitsendpay it would be easier and less fragile on renepay's side.