askrene fixes and API updates #7707

rustyrussell · 2024-10-01T02:04:56Z

This contains a pile of API tweaks and updates, from @Lagrang3 and myself as we actually use and test the API. Hopefully this makes it more useful!

plugins/askrene/askrene.c

Lagrang3 · 2024-10-01T10:10:47Z

plugins/askrene/reserve.c

+					      "short_channel_id_dir",
+					      r->scidd);
+		json_add_u64(js, "age_in_seconds",
+			     timemono_between(time_mono(), r->timestamp).ts.tv_sec);


Small optimization: make a single call to time_mono() before the loop. Use that value as the present time
and use that to compute the age.

tests/test_askrene.py

plugins/askrene/layer.c

common/gossmap.h

plugins/askrene/explain_failure.c

plugins/askrene/layer.c

We allow adding them, but crash when we remove the localmods. Yet this could theoretically happen if a channel we modified was removed from the gossmap, anyway. Reported-by: Lagrang3 <lagrang3@protonmail.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Based-on-the-patch-by: Lagrang3 <lagrang3@protonmail.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Changelog-EXPERIMENTAL: askrene: add askrene-disable-channel RPC Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

It's generally much more convenient, and it's already present in other APIs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It's generally better to be explicit with these things: currently typos would be ignored. But it's also much easier to clean up entire layers as we use them for temporary (per-payment) effects. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is important for errors and feedback. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

I got confused, as we had a struct containing two arrays. Simply expose the reserve_hop struct and use arrays directly. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Suggested-by: Lagrang3 <lagrang3@protonmail.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

And actually write tests! Suggested-by: Lagrang3 <lagrang3@protonmail.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Lagrang3 points out that if we hit a maximum, we should take into account the reserve. This is true, but it's hard for the caller to do, so change the API to be slightly higher level. Tell "inform" what happened, and it adjust the constraints appropriately. This makes the least assumptions possible (a reserve does *not* mean that the capacity was actually used at that time). We also add a mode to say "this succeeded": for now this does nothing, but it could reduce both min/max capacities, and add capacity in the other direction. This is useful for future payments, but not as useful for the current one. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is a bit more efficient, but moreover the JSONRPC API is more logical this way. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Doesn't matter now, but will with the next change where we want to pass a pointer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…t* auto.sourcefree. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We also set htlc_max to 0 when disabling, so the tests worked, but this is correct. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is actually what we want in several places: to only override one or two fields in a channel_update. We add a gossmap_local_setchan() with a similar API to the old gossmap_local_updatechan(), for the case where we want to set every field. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It was weird not to have a capacity associated with localmods channels, and fixing it has some very nice side effects. Now the gossmap_chan_get_capacity() call never fails (we prevented reading of channels from gossmap in the partially-written case already), so we make it return the capacity. We do this in msat, because that's what all the callers want. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…channel. This allows for explicit partial updates to channels (e.g. just change fees, or just disable) without haveing to set the other fields. This generalizes askrene-disable-channel, which is removed. We also take the chance to use the proper BOLT 7 terms in the API: - htlc_minimum_msat - htlc_maximum_msat - cltv_expiry_delta - fee_base_msat - fee_proportional_millionths Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Lagrang3 points out it's less useful (when we time them out), and probably a premature optimization anyway. Suggested-by: Lagrang3 <lagrang3@protonmail.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This turns out to be critical for users: also stops them from bothering us when their node is offline or has insufficient capacity! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

The code is a bit too complex for gcc to track it: ``` In file included from ccan/ccan/tal/str/str.h:7, from plugins/askrene/askrene.c:11: plugins/askrene/askrene.c: In function ‘do_getroutes’: ccan/ccan/tal/tal.h:324:23: error: ‘routes’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 324 | #define tal_count(p) (tal_bytelen(p) / sizeof(*p)) | ^~~~~~~~~~~ plugins/askrene/askrene.c:476:24: note: ‘routes’ was declared here 476 | struct route **routes; | ^~~~~~ plugins/askrene/askrene.c:475:29: error: ‘amounts’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 475 | struct amount_msat *amounts; | ^~~~~~~ plugins/askrene/askrene.c:488:69: error: ‘probability’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 488 | json_add_u64(response, "probability_ppm", (u64)(probability * 1000000)); | ~~~~~~~~~~~~~^~~~~~~~~~ cc plugins/askrene/dijkstra.c cc1: all warnings being treated as errors ``` On my local machine, it also warns in param_dev_channel, so I fixed that too. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

1. describe_disabled should point out if node itself is disabled. 2. Hoist constraint check for neater if branching. 3. Use amount_msat_max/min for greater clarity. 4. Simply disable channels, don't zero htlc_min/max when node disabled. I also fixed the diagnostic of htlc_max correctly, which removes a FIXME. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2024-10-03T23:49:14Z

Added fixes for your comments, fixed FIXME in tests (to give a nicer explanation, which we now do!) and trivial rebase on master. Will merge once CI happy!

rustyrussell requested a review from Lagrang3 October 1, 2024 02:04

rustyrussell requested a review from cdecker as a code owner October 1, 2024 02:04