Skip to content

Askrene algorithm switcher #8163

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.

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

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Mar 14, 2025

This PR only does a bit of refactoring to askrene.

I would like to add the possibility to switch algorithms in askrene.
This would not be available from the user interface, it is just for developers.
It would allow us to test different algorithms for experimenting performance and reliability
without risking breaking the existing code.
This PR sets the stage for switching between multiple MCF algorithms.

This PR is built on top of #7932.

@Lagrang3 Lagrang3 marked this pull request as ready for review March 14, 2025 10:34
@Lagrang3 Lagrang3 force-pushed the askrene-algorithm-switcher branch 2 times, most recently from 089eed9 to b570166 Compare March 14, 2025 10:48
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Not a bad idea! I'm just not sure about some of the refactorings.

Comment on lines -339 to -368
/* Returns an error message, or sets *routes */
static const char *get_routes(const tal_t *ctx,
struct command *cmd,
const struct node_id *source,
const struct node_id *dest,
struct amount_msat amount,
struct amount_msat maxfee,
u32 finalcltv,
u32 maxdelay,
const char **layers,
struct gossmap_localmods *localmods,
const struct layer *local_layer,
bool single_path,
struct route ***routes,
struct amount_msat **amounts,
const struct additional_cost_htable *additional_costs,
double *probability)
Copy link
Contributor

Choose a reason for hiding this comment

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

Handing arguments inside a struct (rather than using them explicitly) is an anti-pattern. We need to use this for callbacks, which is why we do it, but we try to constrain the damage to one location.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I don't understand your request here.
I didn't change the arguments passed to do_getroutes.
That function changed from this:

static struct command_result *do_getroutes(struct command *cmd,
					   struct gossmap_localmods *localmods,
					   const struct getroutes_info *info)
{
	...
	// unpack getroutes_info and calls get_routes
	err = get_routes(cmd, cmd,
			 info->source, info->dest,
			 *info->amount, *info->maxfee, *info->finalcltv,
			 *info->maxdelay, info->layers, localmods, info->local_layer,
			 have_layer(info->layers, "auto.no_mpp_support"),
			 &routes, &amounts, info->additional_costs, &probability);

	// write results in a json stream and returns
	...	
	return command_finished(cmd, response);
}

static struct command_result *json_getroutes(struct command *cmd,
					     const char *buffer,
					     const jsmntok_t *params)
{
	// packages all input data into getroutes_info
	struct getroutes_info *info = tal(cmd, struct getroutes_info);
	...
	return do_getroutes(cmd, gossmap_localmods_new(cmd), info);
}

to this

static struct command_result *do_getroutes(struct command *cmd,
					   struct gossmap_localmods *localmods,
					   const struct getroutes_info *info)
{
        // do some stuff that previously was done in get_routes
	...
	// unpack getroutes_info and calls default_routes
       err = default_routes(rq, rq, srcnode, dstnode, *info->amount,
			     /* only one path? = */
			     have_layer(info->layers, "auto.no_mpp_support"),
			     *info->maxfee, *info->finalcltv, *info->maxdelay,
			     &flows, &probability);


	// write results in a json stream and returns
	...	
	return command_finished(cmd, response);
}

static struct command_result *json_getroutes(struct command *cmd,
					     const char *buffer,
					     const jsmntok_t *params)
{
	// packages all input data into getroutes_info
	struct getroutes_info *info = tal(cmd, struct getroutes_info);
	...
	return do_getroutes(cmd, gossmap_localmods_new(cmd), info);
}

Some of the code in the definition of get_routes was moved to mcf.c under the name
default_routes and some was moved to do_getroutes.

@Lagrang3 Lagrang3 force-pushed the askrene-algorithm-switcher branch 5 times, most recently from c194763 to 28dc007 Compare March 24, 2025 08:43
@Lagrang3 Lagrang3 requested a review from rustyrussell March 25, 2025 11:03
@Lagrang3 Lagrang3 force-pushed the askrene-algorithm-switcher branch 2 times, most recently from bdd97fc to 62a2200 Compare April 30, 2025 14:35
@Lagrang3 Lagrang3 force-pushed the askrene-algorithm-switcher branch from 62a2200 to 27fc1c0 Compare May 19, 2025 05:21
@Lagrang3 Lagrang3 added this to the v25.08 milestone May 19, 2025
@Lagrang3 Lagrang3 mentioned this pull request May 19, 2025
3 tasks
@Lagrang3 Lagrang3 force-pushed the askrene-algorithm-switcher branch from 27fc1c0 to 98b4eeb Compare June 3, 2025 09:38
@rustyrussell
Copy link
Contributor

OK, I broke this up into smaller commits for the refactoring, because I got very confused trying to read all that (as you can see, above)

Only minor changes:

  1. apply_layers takes just the things it needs, not the entire getroutes_info.
  2. convert_routes_to_flows returns the routes, and puts the amounts parameter at the end.
  3. penultimate commit changes struct getroutes_info not to use pointers (that was only a side-effect of the param() weird calling convention, but it's not intuitive to have compulsory members be pointers).
  4. Fixed an existing (minor) bug where the maxdelay parameter check was after the command_check_only test.

@rustyrussell rustyrussell force-pushed the askrene-algorithm-switcher branch from 98b4eeb to e4e79f0 Compare July 8, 2025 04:26
@rustyrussell rustyrussell force-pushed the askrene-algorithm-switcher branch from e4e79f0 to b8880af Compare July 10, 2025 06:19
@rustyrussell
Copy link
Contributor

Fixed a commit missing a commit message, otherwise unchanged.

@rustyrussell rustyrussell enabled auto-merge (rebase) July 10, 2025 06:20
Lagrang3 and others added 7 commits July 10, 2025 23:38
Move the feature tuning algorithm to mcf.c, ie. the loops for searching
a good mu and delay_feefactor to satisfy the problem constraints.

We are looking to set the stage for an execution logic that allows for
multiple choices of routing algorithms, mainly for experimenting without
breaking the default working code.

Changelog-None

Signed-off-by: Lagrang3 <[email protected]>
Simple refactoring.

Stolen entirely from Eduardo.
Signed-off-by: Rusty Russell <[email protected]>
It was originally a wrapper for JSON param() results, but now it's a more
generic struct.  So make it clear that the fields are not optional.  This
means a manual assignment in the initial population of this struct, but
all the users are now far clearer.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell force-pushed the askrene-algorithm-switcher branch from b8880af to cf6c33d Compare July 11, 2025 00:44
@rustyrussell
Copy link
Contributor

Trivial rebase onto master now #7932 merged

@rustyrussell rustyrussell merged commit 34cf333 into ElementsProject:master Jul 11, 2025
76 of 78 checks passed
@Lagrang3 Lagrang3 deleted the askrene-algorithm-switcher branch July 11, 2025 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants