From 66ed456f8a4588cc5b7888b796bfb5b7d21525b1 Mon Sep 17 00:00:00 2001 From: Lagrang3 Date: Fri, 7 Feb 2025 20:24:20 +0100 Subject: [PATCH] askrene: change getroutes amount and delay values In the previous implementation getroutes would provide for every i-th hop the value of the amount and delay which correspond to the (i-1)-th HTLC. This commit changes that, instead the i-th hop contains the value of the amount and delay of the i-th HTLC. Changelog-Changed: askrene: getroutes outputs contains for each hop the correct amount and delay values. Signed-off-by: Lagrang3 --- contrib/msggen/msggen/schema.json | 2 +- doc/schemas/lightning-getroutes.json | 2 +- plugins/askrene/askrene.c | 4 +- plugins/xpay/xpay.c | 18 +--- tests/test_askrene.py | 154 +++++++++++++++++++++------ 5 files changed, 133 insertions(+), 47 deletions(-) diff --git a/contrib/msggen/msggen/schema.json b/contrib/msggen/msggen/schema.json index 7174ef5cb855..3bc91bf0ba69 100644 --- a/contrib/msggen/msggen/schema.json +++ b/contrib/msggen/msggen/schema.json @@ -16190,7 +16190,7 @@ "delay": { "type": "u32", "description": [ - "The total CLTV expected by the node at the start of this hop." + "The total CLTV on this hop's HTLC." ] } } diff --git a/doc/schemas/lightning-getroutes.json b/doc/schemas/lightning-getroutes.json index ee3f913ab4c6..d5a98ecb9055 100644 --- a/doc/schemas/lightning-getroutes.json +++ b/doc/schemas/lightning-getroutes.json @@ -155,7 +155,7 @@ "delay": { "type": "u32", "description": [ - "The total CLTV expected by the node at the start of this hop." + "The total CLTV on this hop's HTLC." ] } } diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c index d8c82883fde9..d6e5c7729933 100644 --- a/plugins/askrene/askrene.c +++ b/plugins/askrene/askrene.c @@ -526,6 +526,8 @@ static const char *get_routes(const tal_t *ctx, struct gossmap_node *far_end; const struct half_chan *h = flow_edge(flows[i], j); + rh->amount = msat; + rh->delay = delay; if (!amount_msat_add_fee(&msat, h->base_fee, h->proportional_fee)) plugin_err(rq->plugin, "Adding fee to amount"); delay += h->delay; @@ -534,8 +536,6 @@ static const char *get_routes(const tal_t *ctx, rh->direction = flows[i]->dirs[j]; far_end = gossmap_nth_node(rq->gossmap, flows[i]->path[j], !flows[i]->dirs[j]); gossmap_node_get_id(rq->gossmap, far_end, &rh->node_id); - rh->amount = msat; - rh->delay = delay; } (*amounts)[i] = flows[i]->delivers; rq_log(tmpctx, rq, LOG_INFORM, "Flow %zu/%zu: %s", diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 20d42e9c53f6..1234690297f9 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -127,12 +127,8 @@ struct hop { struct pubkey next_node; /* Via this channel */ struct short_channel_id_dir scidd; - /* This is amount the node needs (including fees) */ - struct amount_msat amount_in; /* ... to send this amount */ struct amount_msat amount_out; - /* This is the delay, including delay across node */ - u32 cltv_value_in; /* This is the delay, out from node. */ u32 cltv_value_out; }; @@ -347,14 +343,14 @@ static struct amount_msat initial_sent(const struct attempt *attempt) { if (tal_count(attempt->hops) == 0) return attempt->delivers; - return attempt->hops[0].amount_in; + return attempt->hops[0].amount_out; } static u32 initial_cltv_delta(const struct attempt *attempt) { if (tal_count(attempt->hops) == 0) return attempt->payment->final_cltv; - return attempt->hops[0].cltv_value_in; + return attempt->hops[0].cltv_value_out; } /* The current attempt is the first to succeed: we assume all the ones @@ -776,7 +772,7 @@ static struct amount_msat total_fees_being_sent(const struct payment *payment) if (tal_count(attempt->hops) == 0) continue; if (!amount_msat_sub(&fee, - attempt->hops[0].amount_in, + attempt->hops[0].amount_out, attempt->delivers)) abort(); if (!amount_msat_accumulate(&sum, fee)) @@ -1053,16 +1049,12 @@ static struct command_result *getroutes_done(struct command *aux_cmd, ",delay:%}", JSON_SCAN(json_to_short_channel_id_dir, &hop->scidd), - JSON_SCAN(json_to_msat, &hop->amount_in), + JSON_SCAN(json_to_msat, &hop->amount_out), JSON_SCAN(json_to_pubkey, &hop->next_node), - JSON_SCAN(json_to_u32, &hop->cltv_value_in)); + JSON_SCAN(json_to_u32, &hop->cltv_value_out)); if (err) plugin_err(aux_cmd->plugin, "Malformed routes: %s", err); - if (j > 0) { - hops[j-1].amount_out = hop->amount_in; - hops[j-1].cltv_value_out = hop->cltv_value_in; - } } hops[j-1].amount_out = delivers; hops[j-1].cltv_value_out = payment->final_cltv; diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 44e029194a33..6d41aba9fbc3 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -11,6 +11,7 @@ import subprocess import time import tempfile +import shutil def direction(src, dst): @@ -476,8 +477,8 @@ def test_getroutes(node_factory): 'amount_msat': 1000, 'path': [{'short_channel_id_dir': '0x1x0/1', 'next_node_id': nodemap[1], - 'amount_msat': 1010, - 'delay': 99 + 6}]}]} + 'amount_msat': 1000, + 'delay': 99}]}]} # Two hop, still easy. assert l1.rpc.getroutes(source=nodemap[0], destination=nodemap[3], @@ -490,12 +491,12 @@ def test_getroutes(node_factory): 'amount_msat': 100000, 'path': [{'short_channel_id_dir': '0x1x0/1', 'next_node_id': nodemap[1], - 'amount_msat': 103020, - 'delay': 99 + 6 + 6}, + 'amount_msat': 102000, + 'delay': 99 + 6}, {'short_channel_id_dir': '1x3x2/1', 'next_node_id': nodemap[3], - 'amount_msat': 102000, - 'delay': 99 + 6} + 'amount_msat': 100000, + 'delay': 99} ]}]} # Too expensive @@ -534,8 +535,8 @@ def test_getroutes(node_factory): 'amount_msat': 1000000, 'path': [{'short_channel_id_dir': '0x2x3/1', 'next_node_id': nodemap[2], - 'amount_msat': 1000001, - 'delay': 99 + 6}]}]} + 'amount_msat': 1000000, + 'delay': 99}]}]} # For 10000 sats, we will split. check_getroute_paths(l1, @@ -544,12 +545,12 @@ def test_getroutes(node_factory): 10000000, [[{'short_channel_id_dir': '0x2x1/1', 'next_node_id': nodemap[2], - 'amount_msat': 4500004, - 'delay': 99 + 6}], + 'amount_msat': 4500000, + 'delay': 99}], [{'short_channel_id_dir': '0x2x3/1', 'next_node_id': nodemap[2], - 'amount_msat': 5500005, - 'delay': 99 + 6}]]) + 'amount_msat': 5500000, + 'delay': 99}]]) def test_getroutes_fee_fallback(node_factory): @@ -617,8 +618,8 @@ def test_getroutes_auto_sourcefree(node_factory): 'amount_msat': 1000, 'path': [{'short_channel_id_dir': '0x1x0/1', 'next_node_id': nodemap[1], - 'amount_msat': 1010, - 'delay': 105}]}]} + 'amount_msat': 1000, + 'delay': 99}]}]} # Start easy assert l1.rpc.getroutes(source=nodemap[0], @@ -650,8 +651,8 @@ def test_getroutes_auto_sourcefree(node_factory): 'delay': 99 + 6}, {'short_channel_id_dir': '1x3x2/1', 'next_node_id': nodemap[3], - 'amount_msat': 102000, - 'delay': 99 + 6} + 'amount_msat': 100000, + 'delay': 99} ]}]} # Too expensive @@ -698,8 +699,8 @@ def test_getroutes_maxdelay(node_factory): 'amount_msat': 1000, 'path': [{'short_channel_id_dir': '0x1x0/1', 'next_node_id': nodemap[1], - 'amount_msat': 1010, - 'delay': 179}]}]} + 'amount_msat': 1000, + 'delay': 99}]}]} # But use the channel with lower delay when needed assert l1.rpc.getroutes(source=nodemap[0], @@ -714,8 +715,8 @@ def test_getroutes_maxdelay(node_factory): 'amount_msat': 1000, 'path': [{'short_channel_id_dir': '0x1x1/1', 'next_node_id': nodemap[1], - 'amount_msat': 1010, - 'delay': 139}]}]} + 'amount_msat': 1000, + 'delay': 99}]}]} # Excessive maxdelay parameter with pytest.raises(RpcError, match="maximum delay allowed is 2016"): @@ -758,9 +759,9 @@ def test_getroutes_auto_localchans(node_factory): 100000, maxfee_msat=100000, layers=['auto.localchans'], - paths=[[{'short_channel_id_dir': scid21dir, 'amount_msat': 102012, 'delay': 99 + 6 + 6 + 6}, - {'short_channel_id_dir': '0x1x0/0', 'amount_msat': 102010, 'delay': 99 + 6 + 6}, - {'short_channel_id_dir': '1x2x1/1', 'amount_msat': 101000, 'delay': 99 + 6}]]) + paths=[[{'short_channel_id_dir': scid21dir, 'amount_msat': 102010, 'delay': 99 + 6 + 6}, + {'short_channel_id_dir': '0x1x0/0', 'amount_msat': 101000, 'delay': 99 + 6}, + {'short_channel_id_dir': '1x2x1/1', 'amount_msat': 100000, 'delay': 99}]]) # This should get self-discount correct check_getroute_paths(l2, @@ -770,8 +771,8 @@ def test_getroutes_auto_localchans(node_factory): maxfee_msat=100000, layers=['auto.localchans', 'auto.sourcefree'], paths=[[{'short_channel_id_dir': scid21dir, 'amount_msat': 102010, 'delay': 99 + 6 + 6}, - {'short_channel_id_dir': '0x1x0/0', 'amount_msat': 102010, 'delay': 99 + 6 + 6}, - {'short_channel_id_dir': '1x2x1/1', 'amount_msat': 101000, 'delay': 99 + 6}]]) + {'short_channel_id_dir': '0x1x0/0', 'amount_msat': 101000, 'delay': 99 + 6}, + {'short_channel_id_dir': '1x2x1/1', 'amount_msat': 100000, 'delay': 99}]]) def test_fees_dont_exceed_constraints(node_factory): @@ -847,7 +848,7 @@ def test_sourcefree_on_mods(node_factory, bitcoind): final_cltv=99)['routes'] # Expect no fee. check_route_as_expected(routes, [[{'short_channel_id_dir': '0x3x3/1', - 'amount_msat': 1003000, 'delay': 117}]]) + 'amount_msat': 1000000, 'delay': 99}]]) def test_live_spendable(node_factory, bitcoind): @@ -1013,8 +1014,8 @@ def test_max_htlc(node_factory, bitcoind): final_cltv=10) check_route_as_expected(routes['routes'], - [[{'short_channel_id_dir': '0x1x0/1', 'amount_msat': 1_000_001, 'delay': 10 + 6}], - [{'short_channel_id_dir': '0x1x1/1', 'amount_msat': 19_000_019, 'delay': 10 + 6}]]) + [[{'short_channel_id_dir': '0x1x0/1', 'amount_msat': 1_000_000, 'delay': 10}], + [{'short_channel_id_dir': '0x1x1/1', 'amount_msat': 19_000_000, 'delay': 10}]]) # If we can't use channel 2, we fail. l1.rpc.askrene_create_layer('removechan2') @@ -1362,8 +1363,8 @@ def test_askrene_fake_channeld(node_factory, bitcoind): # delay and amount_msat for sendpay are amounts at *end* of hop, not start! with_end = r['path'] + [{'amount_msat': r['amount_msat'], 'delay': r['final_cltv']}] for n, h in enumerate(paths[i]): - h['delay'] = with_end[n + 1]['delay'] - h['amount_msat'] = with_end[n + 1]['amount_msat'] + h['delay'] = with_end[n]['delay'] + h['amount_msat'] = with_end[n]['amount_msat'] l1.rpc.sendpay(paths[i], hash_hex, amount_msat=AMOUNT, @@ -1398,3 +1399,96 @@ def test_askrene_fake_channeld(node_factory, bitcoind): 'constrained') else: raise err + + +def test_forwarding_fees(node_factory): + """Tests if getroutes gets the fees right.""" + + def fees(amt, propfee, basefee): + return basefee + (amt * propfee) // 1000000 + + l1 = node_factory.get_node(start=False) + + basefee = 7 + propfee = 10000 + msat = 123000 + capacity = 1000000 + # 0 has to use two paths (1 and 2) to reach 3. But we tell it 0->1 has limited capacity. + gsfile, nodemap = generate_gossip_store( + [ + GenChannel( + 0, + 1, + capacity_sats=capacity, + forward=GenChannel.Half(propfee=propfee, basefee=basefee), + ), + GenChannel( + 1, + 2, + capacity_sats=capacity, + forward=GenChannel.Half(propfee=propfee, basefee=basefee), + ), + GenChannel( + 2, + 3, + capacity_sats=capacity, + forward=GenChannel.Half(propfee=propfee, basefee=basefee), + ), + ] + ) + + # Set up l1 with this as the gossip_store + shutil.copy( + gsfile.name, os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "gossip_store") + ) + l1.start() + + routes = l1.rpc.getroutes( + source=nodemap[0], + destination=nodemap[3], + amount_msat=msat, + layers=[], + maxfee_msat=msat, + final_cltv=99, + )["routes"] + assert len(routes) == 1 + assert len(routes[0]["path"]) == 3 + assert routes[0]["path"][-1]["amount_msat"] >= msat + msat = routes[0]["path"][-1]["amount_msat"] + msat = msat + fees(msat, propfee, basefee) + assert routes[0]["path"][-2]["amount_msat"] >= msat + msat = routes[0]["path"][-2]["amount_msat"] + msat = msat + fees(msat, propfee, basefee) + assert routes[0]["path"][-3]["amount_msat"] >= msat + + +def test_forwarding_fees2(node_factory): + """Make sure we use correct delay and fees for the direction we're going.""" + + def fees(amt, propfee, basefee): + return basefee + (amt * propfee) // 1000000 + + l1, l2, l3 = node_factory.line_graph( + 3, + wait_for_announce=True, + opts=[ + {}, + {"fee-base": 2000, "fee-per-satoshi": 20, "cltv-delta": 20}, + {"fee-base": 3000, "fee-per-satoshi": 30, "cltv-delta": 30}, + ], + ) + msat = 123000 + routes = l1.rpc.getroutes( + source=l1.info["id"], + destination=l3.info["id"], + layers=["auto.localchans"], + maxfee_msat=msat, + amount_msat=msat, + final_cltv=99, + )["routes"] + assert len(routes) == 1 + assert len(routes[0]["path"]) == 2 + assert routes[0]["path"][-1]["amount_msat"] >= msat + msat = routes[0]["path"][-1]["amount_msat"] + msat = msat + fees(msat, 20, 2000) + assert routes[0]["path"][-2]["amount_msat"] >= msat