From 6e86fa92206eb6e935a8dcba37b9e7849d2cc816 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 25 Sep 2022 22:44:12 +0930 Subject: [PATCH] lightningd: figure out optimal channel *before* forward_htlc hook. Otherwise what the hook sees is actually a lie, and if it sets it we might override it. The side effect is that we add an explicit "forward_to" field, and allow hooks to override it. This lets a *hook* control channel choice explicitly. Changelod-Added: Plugins: `htlc_accepted_hook` return can specify what channel to forward htlc to. Signed-off-by: Rusty Russell --- doc/PLUGINS.md | 5 +- lightningd/channel.h | 1 - lightningd/peer_htlcs.c | 154 +++++++++++++++++++-------- tests/plugins/htlc_accepted-fwdto.py | 31 ++++++ tests/test_opening.py | 12 ++- tests/test_plugin.py | 18 ++++ wallet/test/run-wallet.c | 4 + 7 files changed, 178 insertions(+), 47 deletions(-) create mode 100755 tests/plugins/htlc_accepted-fwdto.py diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index 6152bed95..dcefa4edc 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -1432,7 +1432,8 @@ The payload of the hook call has the following format: "cltv_expiry": 500028, "cltv_expiry_relative": 10, "payment_hash": "0000000000000000000000000000000000000000000000000000000000000000" - } + }, + "forward_to": "0000000000000000000000000000000000000000000000000000000000000000" } ``` @@ -1469,6 +1470,7 @@ For detailed information about each field please refer to [BOLT 04 of the specif blockheight. - `payment_hash` is the hash whose `payment_preimage` will unlock the funds and allow us to claim the HTLC. + - `forward_to`: if set, the channel_id we intend to forward this to (will not be present if the short_channel_id was invalid or we were the final destination). The hook response must have one of the following formats: @@ -1489,6 +1491,7 @@ the response. Note that this is always a TLV-style payload, so unlike hex digits long). This will be re-parsed; it's useful for removing onion fields which a plugin doesn't want lightningd to consider. +It can also specify `forward_to` in the response, replacing the destination. This usually only makes sense if it wants to choose an alternate channel to the same next peer, but is useful if the `payload` is also replaced. ```json { diff --git a/lightningd/channel.h b/lightningd/channel.h index 9f8ec75e5..9a31157c6 100644 --- a/lightningd/channel.h +++ b/lightningd/channel.h @@ -9,7 +9,6 @@ #include #include -struct channel_id; struct uncommitted_channel; struct wally_psbt; diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index e1764bb27..043510208 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -648,11 +648,51 @@ const u8 *send_htlc_out(const tal_t *ctx, return NULL; } +/* What's the best channel to this peer? + * If @hint is set, channel must match that one. */ +static struct channel *best_channel(struct lightningd *ld, + const struct peer *next_peer, + struct amount_msat amt_to_forward, + struct channel *hint) +{ + struct amount_msat best_spendable = AMOUNT_MSAT(0); + struct channel *channel, *best = hint; + + /* Seek channel with largest spendable! */ + list_for_each(&next_peer->channels, channel, list) { + struct amount_msat spendable; + if (!channel_can_add_htlc(channel)) + continue; + spendable = channel_amount_spendable(channel); + if (!amount_msat_greater(spendable, best_spendable)) + continue; + + /* Don't override if fees differ... */ + if (hint) { + if (hint->feerate_base != channel->feerate_base + || hint->feerate_ppm != channel->feerate_ppm) + continue; + } + + /* Or if this would be below min for channel! */ + if (amount_msat_less(amt_to_forward, + channel->channel_info.their_config.htlc_minimum)) + continue; + + best = channel; + best_spendable = spendable; + } + return best; +} + +/* forward_to is where we're actually sending it (or NULL), and + * forward_scid is where they asked to send it (or NULL). */ static void forward_htlc(struct htlc_in *hin, u32 cltv_expiry, struct amount_msat amt_to_forward, u32 outgoing_cltv_value, - const struct short_channel_id *scid, + const struct short_channel_id *forward_scid, + const struct channel_id *forward_to, const u8 next_onion[TOTAL_PACKET_SIZE(ROUTING_INFO_SIZE)], const struct pubkey *next_blinding) { @@ -660,52 +700,21 @@ static void forward_htlc(struct htlc_in *hin, struct lightningd *ld = hin->key.channel->peer->ld; struct channel *next; struct htlc_out *hout = NULL; - struct short_channel_id *altscid; - /* This is a shortcut for specifying next peer; doesn't mean - * the actual channel! */ - next = any_channel_by_scid(ld, scid, false); - if (next) { - struct peer *peer = next->peer; - struct channel *channel; - struct amount_msat best_spendable = channel_amount_spendable(next); - - /* Seek channel with largest spendable! */ - list_for_each(&peer->channels, channel, list) { - struct amount_msat spendable; - if (!channel_can_add_htlc(channel)) - continue; - spendable = channel_amount_spendable(channel); - if (!amount_msat_greater(spendable, best_spendable)) - continue; - - /* Don't override if fees differ... */ - if (channel->feerate_base != next->feerate_base - || channel->feerate_ppm != next->feerate_ppm) - continue; - /* Or if this would be below min for channel! */ - if (amount_msat_less(amt_to_forward, - channel->channel_info.their_config.htlc_minimum)) - continue; - - altscid = channel->scid != NULL ? channel->scid - : channel->alias[LOCAL]; - - /* OK, it's better! */ - log_debug(next->log, "Chose a better channel: %s", - type_to_string(tmpctx, - struct short_channel_id, - altscid)); - next = channel; - } - } + if (forward_to) { + next = channel_by_cid(ld, forward_to); + /* Update this to where we're actually trying to send. */ + if (next) + forward_scid = channel_scid_or_local_alias(next); + }else + next = NULL; /* Unknown peer, or peer not ready. */ if (!next || !channel_active(next)) { local_fail_in_htlc(hin, take(towire_unknown_next_peer(NULL))); wallet_forwarded_payment_add(hin->key.channel->peer->ld->wallet, hin, get_onion_style(hin), - scid, NULL, + forward_scid, NULL, FORWARD_LOCAL_FAILED, WIRE_UNKNOWN_NEXT_PEER); return; @@ -803,7 +812,7 @@ static void forward_htlc(struct htlc_in *hin, fail: local_fail_in_htlc(hin, failmsg); wallet_forwarded_payment_add(ld->wallet, - hin, get_onion_style(hin), scid, hout, + hin, get_onion_style(hin), forward_scid, hout, FORWARD_LOCAL_FAILED, fromwire_peektype(failmsg)); } @@ -819,6 +828,8 @@ struct htlc_accepted_hook_payload { struct channel *channel; struct lightningd *ld; struct pubkey *next_blinding; + /* NULL if we couldn't find it */ + struct channel_id *fwd_channel_id; u8 *next_onion; u64 failtlvtype; size_t failtlvpos; @@ -907,7 +918,7 @@ static bool htlc_accepted_hook_deserialize(struct htlc_accepted_hook_payload *re struct htlc_in *hin = request->hin; struct lightningd *ld = request->ld; struct preimage payment_preimage; - const jsmntok_t *resulttok, *paykeytok, *payloadtok; + const jsmntok_t *resulttok, *paykeytok, *payloadtok, *fwdtok; u8 *payload, *failonion; if (!toks || !buffer) @@ -939,10 +950,22 @@ static bool htlc_accepted_hook_deserialize(struct htlc_accepted_hook_payload *re ld->accept_extra_tlv_types, &request->failtlvtype, &request->failtlvpos); - } else payload = NULL; + fwdtok = json_get_member(buffer, toks, "forward_to"); + if (fwdtok) { + tal_free(request->fwd_channel_id); + request->fwd_channel_id = tal(request, struct channel_id); + if (!json_to_channel_id(buffer, fwdtok, + request->fwd_channel_id)) { + fatal("Bad forward_to for htlc_accepted" + " hook: %.*s", + fwdtok->end - fwdtok->start, + buffer + fwdtok->start); + } + } + if (json_tok_streq(buffer, resulttok, "continue")) { return true; } @@ -1074,6 +1097,9 @@ static void htlc_accepted_hook_serialize(struct htlc_accepted_hook_payload *p, json_add_secret(s, "shared_secret", hin->shared_secret); json_object_end(s); + if (p->fwd_channel_id) + json_add_channel_id(s, "forward_to", p->fwd_channel_id); + json_object_start(s, "htlc"); json_add_short_channel_id( s, "short_channel_id", @@ -1117,6 +1143,7 @@ htlc_accepted_hook_final(struct htlc_accepted_hook_payload *request STEALS) request->payload->amt_to_forward, request->payload->outgoing_cltv, request->payload->forward_channel, + request->fwd_channel_id, serialize_onionpacket(tmpctx, rs->next), request->next_blinding); } else @@ -1166,6 +1193,37 @@ REGISTER_PLUGIN_HOOK(htlc_accepted, struct htlc_accepted_hook_payload *); +/* Figures out how to fwd, allocating return off hp */ +static struct channel_id *calc_forwarding_channel(struct lightningd *ld, + struct htlc_accepted_hook_payload *hp, + const struct route_step *rs) +{ + const struct onion_payload *p = hp->payload; + struct channel *c, *best; + + if (rs->nextcase != ONION_FORWARD) + return NULL; + + if (!p || !p->forward_channel) + return NULL; + + c = any_channel_by_scid(ld, p->forward_channel, false); + if (!c) + return NULL; + + best = best_channel(ld, c->peer, p->amt_to_forward, c); + if (best != c) { + log_debug(hp->channel->log, + "Chose a better channel than %s: %s", + type_to_string(tmpctx, struct short_channel_id, + p->forward_channel), + type_to_string(tmpctx, struct short_channel_id, + channel_scid_or_local_alias(best))); + } + + return tal_dup(hp, struct channel_id, &best->cid); +} + /** * Everyone is committed to this htlc of theirs * @@ -1300,6 +1358,16 @@ static bool peer_accepted_htlc(const tal_t *ctx, #endif hook_payload->next_blinding = NULL; + /* The scid is merely used to indicate the next peer, it is not + * a requirement (nor, ideally, observable anyway). We can change + * to a more-preferred one now, that way the hook sees the value + * we're actually going to (try to) use */ + + /* We don't store actual channel as it could vanish while + * we're in hook */ + hook_payload->fwd_channel_id + = calc_forwarding_channel(ld, hook_payload, rs); + plugin_hook_call_htlc_accepted(ld, NULL, hook_payload); /* Falling through here is ok, after all the HTLC locked */ diff --git a/tests/plugins/htlc_accepted-fwdto.py b/tests/plugins/htlc_accepted-fwdto.py new file mode 100755 index 000000000..5e53d78d1 --- /dev/null +++ b/tests/plugins/htlc_accepted-fwdto.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python3 +"""A plugin that tells us to forward HTLCs to a specific channel. + +""" +from pyln.client import Plugin + + +plugin = Plugin() + + +@plugin.hook("htlc_accepted") +def on_htlc_accepted(htlc, onion, plugin, **kwargs): + if plugin.fwdto is None: + return {"result": "continue"} + + return {"result": "continue", "forward_to": plugin.fwdto} + + +@plugin.method("setfwdto") +def setfailonion(plugin, fwdto): + """Sets the channel_id to forward to when receiving an incoming HTLC. + """ + plugin.fwdto = fwdto + + +@plugin.init() +def on_init(**kwargs): + plugin.fwdto = None + + +plugin.run() diff --git a/tests/test_opening.py b/tests/test_opening.py index c182fefcf..574f82c7f 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -1722,14 +1722,22 @@ def test_zeroconf_multichan_forward(node_factory): # Now create a channel that is twice as large as the real channel, # and don't announce it. l2.fundwallet(10**7) - l2.rpc.fundchannel(l3.info['id'], 2 * 10**6, mindepth=0) + zeroconf_cid = l2.rpc.fundchannel(l3.info['id'], 2 * 10**6, mindepth=0)['channel_id'] l2.daemon.wait_for_log(r'peer_in WIRE_CHANNEL_READY') l3.daemon.wait_for_log(r'peer_in WIRE_CHANNEL_READY') inv = l3.rpc.invoice(amount_msat=10000, label='lbl1', description='desc')['bolt11'] l1.rpc.pay(inv) - assert l2.daemon.is_in_log(r'Chose a better channel: .*') + + for c in only_one(l2.rpc.listpeers(l3.info['id'])['peers'])['channels']: + if c['channel_id'] == zeroconf_cid: + zeroconf_scid = c['alias']['local'] + else: + normal_scid = c['short_channel_id'] + + assert l2.daemon.is_in_log(r'Chose a better channel than {}: {}' + .format(normal_scid, zeroconf_scid)) def test_zeroreserve(node_factory, bitcoind): diff --git a/tests/test_plugin.py b/tests/test_plugin.py index a4a488608..e46314419 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2381,6 +2381,24 @@ def test_htlc_accepted_hook_failonion(node_factory): l1.rpc.pay(inv) +@pytest.mark.developer("Gossip without developer is slow.") +def test_htlc_accepted_hook_fwdto(node_factory): + plugin = os.path.join(os.path.dirname(__file__), 'plugins/htlc_accepted-fwdto.py') + l1, l2, l3 = node_factory.line_graph(3, opts=[{}, {'plugin': plugin}, {}], wait_for_announce=True) + + # Add some balance + l1.rpc.pay(l2.rpc.invoice(10**9 // 2, 'balance', '')['bolt11']) + wait_for(lambda: only_one(only_one(l1.rpc.listpeers()['peers'])['channels'])['htlcs'] == []) + + # make it forward back down same channel. + l2.rpc.setfwdto(only_one(only_one(l1.rpc.listpeers()['peers'])['channels'])['channel_id']) + inv = l3.rpc.invoice(42, 'fwdto', '')['bolt11'] + with pytest.raises(RpcError, match="WIRE_INVALID_ONION_HMAC"): + l1.rpc.pay(inv) + + assert l2.rpc.listforwards()['forwards'][0]['out_channel'] == only_one(only_one(l1.rpc.listpeers()['peers'])['channels'])['short_channel_id'] + + def test_dynamic_args(node_factory): plugin_path = os.path.join(os.getcwd(), 'contrib/plugins/helloworld.py') diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 7157358ac..fcd85fd32 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -426,6 +426,10 @@ char *json_strdup(const tal_t *ctx UNNEEDED, const char *buffer UNNEEDED, const /* Generated stub for json_stream_success */ struct json_stream *json_stream_success(struct command *cmd UNNEEDED) { fprintf(stderr, "json_stream_success called!\n"); abort(); } +/* Generated stub for json_to_channel_id */ +bool json_to_channel_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, + struct channel_id *cid UNNEEDED) +{ fprintf(stderr, "json_to_channel_id called!\n"); abort(); } /* Generated stub for json_to_node_id */ bool json_to_node_id(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, struct node_id *id UNNEEDED)