Askrene and xpay improve by rustyrussell · Pull Request #9150 · ElementsProject/lightning

rustyrussell · 2026-05-20T08:47:32Z

Depends on #9138

This prepwork for repeatpay improves askrene and xpay:

Cleans up JSON ids for plugins, now they're always strings.
Add "success" knowledge to askrene, and have xpay use it.
Make askrene's explain-failure smarter when we have insufficient funds.
Simplify recurrence handling (repeatpay will ensure consistency).
Make sure currency-based offers give invoices with 10-minute timeouts, for currency fluctuations.

cdecker

ACK 9b3f051

Lagrang3 · 2026-05-25T09:55:56Z

Overall I like the improvements introduced in this PR.
Askrene's layers as they are right now, convey unprocessed information in the form of
constraints and impressions with their associated timestamp. There is not space for interpretation here.

In this way we delegate the interpretation to the moment when we call getroutes:
"given these channel intels we recorded in time what do you think is the most likely current liquidity range".
I wanted to add a smooth time relaxation of the channel constraints and it can be very much
compatible with this abstraction if we implement it this way.

Lagrang3 · 2026-06-18T08:37:34Z

rebased,
fixed memory leak,
added unit tests for str_to_u64,

Lagrang3 · 2026-06-22T12:40:55Z

rebased
fixed xpay tests after changes in the "explain failure" messages

Lagrang3 · 2026-06-25T08:55:49Z

+							       &scidd, &timestamp, &msat)) {
+				/* We don't convert, just omit these */
+				if (!convert_impression)
+					copy_data(&out, data_in, olddata - data_in);


Is this correct? Why is olddata >= data_in?
I thought wire's cursors moved to greater values after reading.

Lagrang3 · 2026-06-25T12:43:52Z

On one commit comment:

commit 199e0572040e13e86c580897f694e63faf347766 (HEAD)
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Wed May 20 18:00:49 2026 +0930

    askrene: correctly order constraints.
    
    Pure "constraints" don't care about order (they simply clamp max and
    min), but "impressions" are relative, so they do.  Change the
    hashtable to keep them timestamp sorted.
    
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Ordering is awesome! Besides the fact that impressions being relative do care about order, it can also be argued that "pure constraints" should also because older constraints are less reliable than the more recent.

Lagrang3 · 2026-06-25T13:30:42Z

added a small optimization: now that "channel intels" are sorted by timestamp we can remove old entries with a single realloc,
rebased,

Lagrang3 · 2026-06-26T05:40:09Z

added a new case to "explain failure", for capacity that is known and enabled.

Lagrang3 · 2026-07-01T10:49:18Z

replaced

tal_free(intelarr[i].impression);
tal_free(intelarr[i].constraint);

with

tal_steal(tmpctx, intelarr[i].impression);
tal_steal(tmpctx, intelarr[i].constraint);

rebased

Lagrang3 · 2026-07-02T11:23:03Z

fixed some failing tests due to change in the error messages,
rebased

We used to handle it being a literal, but this was removed in 73fc9b0 (v25.05) so we don't need to handle that at all. Not using the raw JSON means we handle weird methodnames by replacement: otherwise we would not match the responses. Only an issue for commando, where the command would time out rather than report "Unknown method". Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ngs. In particular, "struct jsonrpc_request"'s id is always a string. cmd->id isn't, though. We can also remove the now-unused json_get_id. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We have three uses already, about to add a fourth. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

The prior implementation could read past the end of the buffer (we actually pad our JSON so this isn't harmful, but still). Fix up json_to_s64 and json_to_double too, but since they're not used as often, just copy the string there. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

Currently used for offers, we will use it for repeatpay too. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Normal constraints are clamps on min/max caused by failed payments: min for the channels that succeeded, max for the channel which failed. Impressions are the results of successful payments, which alter both min and max (negatively in the forward direction, positively in the reverse). impression: n 1. An effect, feeling, or image retained as a consequence of experience. 2. A vague notion, remembrance, or belief. 3. A mark produced on a surface by pressure. Unlike constraints, this is the result of our own effect on the network: they're related but different enough to get their own API and terminology. The name conveys both we made an impression on the channel, and that the results are a bit vague (due to other changes since then, which we won't know about). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: JSON-RPC: `askrene` layers now contain "impressions" representing the effects of successful payments we made through channels.

…n downgrading to v26.06. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld_fakenet sudbdaemon purposedly crafted for this test needs updating to account for funds moving after every success payment. I've tried to fix that but in doing so I also triggered an xpay bug not related to this PR. Therefore, for the moment we skip this test. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

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

Pure "constraints" don't care about order (they simply clamp max and min), but "impressions" are relative, so they do. Change the hashtable to keep them timestamp sorted. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

from a tal array. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

On deletion of individual channel intel entries we need to free the pointer inside the structure. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

Use a single realloc to remove old entries. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

It diagnoses if the *total capacity* of the source/dest are insufficient, but not if the *known capacity* is. So we get: The shortest path is 103x1x0->105x1x0, but 103x1x0/1 layer auto.localchans says max is 77704899msat Whereas it would be better to do: We know from auto.localchans that source has maximum capacity xxx msat (in 1 channels) Similarly for the destination, we get: The shortest path is 103x1x0->105x1x0, but 103x1x0/1 layer auto.localchans says max is 77704899msat

Add PAY_INSUFFICIENT_FUNDS and PAY_ROUTE_NOT_FOUND, and give nice detailed errors for those. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Changed: JSON-RPC: `getroutes` can now return PAY_INSUFFICIENT_FUNDS (215) and PAY_DESTINATION_INSUFFICIENT_CAPACITY (220) error codes.

Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

Consider the case in which payment fails due to not enough "known enabled" liquidity. Notice that we cover the "known" and "enabled" cases already. But: known_enabled <= enabled and known_enabled <= known Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

…rrencies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ersion. This mirrors the previous commit, where we did it for recurring offers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: Offers: we set a 10 minute expiry when we create invoices for offers in other currencies.

We don't actually need to enforce this check here: we can make that the users' responsibility. This simplifies our work quite a lot, since createinvoicerequest won't have to do a lookup any more. This can be done by the repeatpay plugin itself. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is an undocumented interface, so we can just change it. Rename "recurrence_label" to the more general "label", now we don't require it to find previous payments. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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

Lagrang3 · 2026-07-06T10:46:17Z

temporarily skipped test test_xpay_fake_channeld, the fix for this test triggers an older bug in xpay (see xpay: fakenet test: move funds when payment succeeds #9282),
rebased

Lagrang3 · 2026-07-06T11:04:31Z

Perfect is the enemy of good. I don't want to delay anymore this PR, it is ready for review.

I had to skip test_xpay_fake_channeld which is probably failing for more than one reason.
As far as I know one reason is the fact that the simulated network has fixed liquidity which is not compatible
with the impressions view of the network we gain in this PR. To fix this test I have upgraded the
channeld_fakenet daemon and then discovered that this triggers an older bug in xpay (see #9282).

Also the way impressions move the min/max bars in the channels are too strong in my opinion.
These will be revised and tested in a further PR.

Fix test_explain_source_dest_failure by increasing the amounts by a factor of ten, which reduces the relative significance of on-chain fees on the commitment transaction. This used to fail on liquid-regtest due to insufficient liquidity on l2->l4 to prevent channel starvation. At line ``` l1.rpc.xpay(l4.rpc.invoice('30000sat', 'test_explain_simple_failures2', 'test_explain_simple_failures2')['bolt11']) ``` we would get ``` lightningd-2 2026-07-06T11:29:09.973Z DEBUG 02287bfac8b99b35477ebe9334eede1e32b189e24644eb701c079614712331cec0-channeld-chan#3: Adding HTLC would leave us only 19454000msat: we need 25281sat for another HTLC if fees increase from 7500perkw to 13906perkw lightningd-2 2026-07-06T11:29:09.973Z DEBUG 02287bfac8b99b35477ebe9334eede1e32b189e24644eb701c079614712331cec0-channeld-chan#3: Adding HTLC 0 amount=30000000msat cltv=130 gave CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED ``` Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

nGoline

Overall this is solid, but it's really five independent changes (JSON-id cleanup, askrene impressions, explain-failure error codes, recurrence simplification, currency-offer expiry) bundled as one. Splitting the JSON-id and currency-expiry commits into their own PRs would make them landable independently while the impressions work continues to bake.

The thing that worries me the most is that the downgrade tool adds two new copy_data(&out, data_in, olddata - data_in) instances with inverted args (crashes on downgrade with persisted impressions/node-bias), see inline. Please also confirm the recurrence_label -> label rename doesn't need deprecation. The rest are nits/questions inline, non-blocking.

CI is green and the impressions memory-ownership issues from earlier rounds all look resolved.

nGoline · 2026-07-08T19:27:04Z

+	       val == 10000000000000000000ULL);
+
+	/* 999...9 (20 times) overflows */
+	assert(!str_to_u64("", 20, &val));


Are we missing the "99999999999999999999" string here? The input is an empty string, so it returns false because '\0' < '0', not because of overflow. You should either fix the input to twenty 9s or fix the comment.
The function itself is correct. The overflow guard val > (UINT64_MAX - digit) / 10 is right.

nGoline · 2026-07-08T19:32:10Z

+	if (buflen < ISO4217_NAMELEN)
+		return tal_fmt(ctx, "Not a number, and too short for currency");
+
+ 	*isocode = find_iso4217(buf + buflen - ISO4217_NAMELEN, ISO4217_NAMELEN);


nit: Indentation here is a space followed by a tab (+ \t*isocode = ...). Should be a plain tab to match surrounding lines.

nGoline · 2026-07-08T19:36:42Z

-		return false;
-
-	return true;
+	return str_to_u64(buffer + tok->start, tok->end - tok->start, num);


Routing json_to_u64 through str_to_u64 tightens parsing: the old strtoull(buf, 0) accepted hex (0x…), octal, a leading +/-, and surrounding whitespace. str_to_u64 accepts strict decimal only. This is more JSON-correct, but note json_to_s64/json_to_double still use permissive strtoll/strtod, so u64 is now asymmetric with the others. Fine if intended, just flagging that any field previously relying on hex-through-json_to_u64 changes meaning.

nGoline · 2026-07-08T19:39:02Z

 			 p_req("bolt12", param_b12_invreq, &invreq),
 			 p_req("savetodb", param_bool, &save),
-			 p_opt("recurrence_label", param_label, &label),
+			 p_opt("label", param_label, &label),


createinvoicerequest's recurrence_label parameter is renamed to label here (and the internal callers in offers_offer.c/fetchinvoice.c now send label). That's a user-visible parameter rename without going through the deprecation infrastructure or the doc/developers-guide/deprecated-features.md table. Is createinvoicerequest considered internal enough to skip deprecation? If so, worth a note in the commit, otherwise it needs the deprecation path.

nGoline · 2026-07-08T19:41:35Z

+		num_removed += count_old;
+		if(count_old){
+			/* Remove from table before realloc! */
+			channel_intel_hash_del(layer->channel_intels, intelarr);


layer_trim_constraints mutates the hashtable (channel_intel_hash_del -> tal_arr_remove_range -> channel_intel_hash_add) while iterating it with channel_intel_hash_next. This is safe only because the operation is net non-growing (no resize) and re-processing a re-added same-scid entry is a no-op. A one-line comment noting the "no resize during iteration" assumption would make the safety explicit.

nGoline · 2026-07-08T19:50:24Z

 							   &scidd, &bool_ptr,
 							   &msat_ptr, &msat_ptr, &msat_ptr,
 							   &u32_ptr, &u16_ptr))
 				copy_data(&out, data_in, olddata - data_in);


Here.

Suggested change

copy_data(&out, olddata, data_in - olddata);

nGoline · 2026-07-08T19:50:37Z

 			if (fromwire_dstore_channel_constraint(tmpctx, &data_in, &len,
 							       &scidd, &timestamp,
 							       &msat_ptr, &msat_ptr))
 				copy_data(&out, data_in, olddata - data_in);


Here.

Suggested change

copy_data(&out, olddata, data_in - olddata);

nGoline · 2026-07-08T19:50:52Z

 			if (fromwire_dstore_channel_bias(tmpctx, &data_in, &len,
 							 &scidd, &bias,
 							 &string))
 				copy_data(&out, data_in, olddata - data_in);


Here.

Suggested change

copy_data(&out, olddata, data_in - olddata);

nGoline · 2026-07-08T19:51:05Z

 			continue;
 		case DSTORE_DISABLED_NODE:
 			if (fromwire_dstore_disabled_node(&data_in, &len, &n))
 				copy_data(&out, data_in, olddata - data_in);


Here.

Suggested change

copy_data(&out, olddata, data_in - olddata);

nGoline · 2026-07-08T19:51:24Z

+				if (convert_bias)
+					towire_dstore_channel_bias(&out, &scidd, bias, string);
+				else
+					copy_data(&out, data_in, olddata - data_in);


And here.

Suggested change

copy_data(&out, data_in, olddata - data_in);

copy_data(&out, olddata, data_in - olddata);

rustyrussell added this to the v26.09 milestone May 20, 2026

rustyrussell requested a review from cdecker as a code owner May 20, 2026 08:47

rustyrussell mentioned this pull request May 20, 2026

Repeatpay: a plugin to handle recurring payments. #9151

Open

cdecker previously approved these changes May 23, 2026

View reviewed changes

Lagrang3 reviewed May 25, 2026

View reviewed changes

Comment thread common/utils.c

Comment thread plugins/askrene/layer.c

Comment thread plugins/askrene/layer.c Outdated

Comment thread plugins/askrene/layer.c

madelinevibes assigned Lagrang3 Jun 17, 2026

madelinevibes added the Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. label Jun 17, 2026

Lagrang3 force-pushed the askrene-and-xpay-improve branch from 9b3f051 to 7bddd3b Compare June 18, 2026 08:34

Lagrang3 force-pushed the askrene-and-xpay-improve branch 2 times, most recently from bec4e38 to e65c114 Compare June 22, 2026 12:39

Lagrang3 reviewed Jun 25, 2026

View reviewed changes

Lagrang3 force-pushed the askrene-and-xpay-improve branch 2 times, most recently from f3836ff to 386e3a6 Compare June 25, 2026 13:29

Lagrang3 force-pushed the askrene-and-xpay-improve branch from 386e3a6 to f9df6f5 Compare June 26, 2026 05:38

Lagrang3 force-pushed the askrene-and-xpay-improve branch 2 times, most recently from 4a0e9e5 to 9083938 Compare July 1, 2026 10:47

Lagrang3 reviewed Jul 2, 2026

View reviewed changes

Comment thread plugins/offers_invreq_hook.c

Lagrang3 force-pushed the askrene-and-xpay-improve branch from 9083938 to fe5fb13 Compare July 2, 2026 11:22

Lagrang3 reviewed Jul 3, 2026

View reviewed changes

Comment thread tests/test_xpay.py

nGoline requested a review from cdecker July 3, 2026 12:40

Lagrang3 force-pushed the askrene-and-xpay-improve branch 2 times, most recently from 84b7e43 to 202f985 Compare July 6, 2026 10:32

rustyrussell and others added 23 commits July 6, 2026 11:38

lightningd: internal cleanups since all non-command JSON IDs are stri…

013e247

…ngs. In particular, "struct jsonrpc_request"'s id is always a string. cmd->id isn't, though. We can also remove the now-unused json_get_id. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common: hoist hash_str helper into its own header.

9851fad

We have three uses already, about to add a fourth. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Add unit tests for str_to_u64

afd39db

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

common: hoist amount+currency parsing into common code.

eb0970a

Currently used for offers, we will use it for repeatpay too. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

downgrade: we need to remove "impressions" from askrene datastore whe…

795861d

…n downgrading to v26.06. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

xpay: actually tell askrene when a payment succeeded.

f41d189

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

askrene: correctly order constraints.

a20a565

Pure "constraints" don't care about order (they simply clamp max and min), but "impressions" are relative, so they do. Change the hashtable to keep them timestamp sorted. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common: add helper to remove a range of elements

51404ff

from a tal array. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

askrene: don't leak channel intel entries

787dc81

On deletion of individual channel intel entries we need to free the pointer inside the structure. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

xpay: trim constraints: small optimization

c4df280

Use a single realloc to remove old entries. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

xpay: tests should expect the new error messages

a92a670

Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

offers: limit invoices to 10 minutes for recurring offers in other cu…

9fa147e

…rrencies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

fetchinvoice: handle weird labels in recurrence_label parameter.

cd2d6cc

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

Lagrang3 force-pushed the askrene-and-xpay-improve branch from 202f985 to cd2d6cc Compare July 6, 2026 10:44

Lagrang3 requested a review from nGoline July 8, 2026 07:34

rustyrussell dismissed cdecker’s stale review via 6b230d4 July 8, 2026 09:49

nGoline requested changes Jul 8, 2026

View reviewed changes

	copy_data(&out, data_in, olddata - data_in);
	copy_data(&out, olddata, data_in - olddata);

Uh oh!

Conversation

rustyrussell commented May 20, 2026

Uh oh!

cdecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lagrang3 commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lagrang3 commented Jun 18, 2026

Uh oh!

Lagrang3 commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lagrang3 commented Jun 25, 2026

Uh oh!

Lagrang3 commented Jun 25, 2026

Uh oh!

Lagrang3 commented Jun 26, 2026

Uh oh!

Lagrang3 commented Jul 1, 2026

Uh oh!

Uh oh!

Lagrang3 commented Jul 2, 2026

Uh oh!

Uh oh!

Lagrang3 commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lagrang3 commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nGoline left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Lagrang3 commented May 25, 2026 •

edited

Loading

Lagrang3 commented Jul 6, 2026 •

edited

Loading

Lagrang3 commented Jul 6, 2026 •

edited

Loading