routing: allow runtime updates to mission control config

[carlaKC]

This PR adds the ability to get and set mission control's config via rpc, allowing for experimentation with these values without a restart.

Since we already have locking in mission control, we can just acquire the lock and update the cfg.

[bhandras]

Pretty straightforward changes and nice refactoring along the way.
My only real comment is that since we can change the mission control config during route calculation (since locking is per point) we may create an unwanted path as a result. It's very unlikely as a single user but may be likely as service provider. I'd consider fixing that too, although it's probably not a huge deal.

[bhandras]

I'm a bit unsure about using integer percents for the probability. Maybe better to use floats since it results in simpler code and also less confusing? Also for weight?

[halseth]

Just from a high level look, I'm wondering if we can do this without introducing race conditions?

Even though we get/set the config behind a mutex, MissionControl itself still reads the config without acquiring the lock first.

[carlaKC]

MissionControl itself still reads the config without acquiring the lock first.

I thought I'd checked all of these, but looks like ReportPaymentSuccess and ReportPaymentFailure don't acquire the lock for adding results to the store. Although they do acquire it later to update MC, so wondering whether than can be moved up. Otherwise the other calls to MC all use the lock so it shouldn't be too much of an issue?

[halseth]

@carlaKC I was thinking about places like this: https://github.com/lightningnetwork/lnd/blob/d7ecde2972bfb949c6df717d53ebfbb3283de90b/routing/probability_estimator.go#L82

One alternative to swapping out the mission control config, is to create a new MissionControl with the new config and swap that out instead.

[halseth]

Looks pretty good :)

[halseth]

use double instead?

[joostjager]

Perhaps it is worth documenting in the proto that the runtime settings will only be in effect until the next restart.

An alternative would be to hot-reload the config file, either automatically or via a cli command (similar to systemctl daemon-reload). That would at least keep the config in one place. It also opens up the possibility to hot-reload other config settings without adding rpcs for all of them (which aren't really useful other than for experimentation).

[halseth]

I think just adding to the doc that it won't be persisted is good enough for now?

[carlaKC]

Perhaps it is worth documenting in the proto that the runtime settings will only be in effect until the next restart.

Indeed, will add some comments on the rpc making a note of it.

An alternative would be to hot-reload the config file, either automatically or via a cli command (similar to systemctl daemon-reload). That would at least keep the config in one place. It also opens up the possibility to hot-reload other config settings without adding rpcs for all of them (which aren't really useful other than for experimentation).

I think this would be a nice feature to have, but likely a larger project since not everybody runs lnd with a config file. Although it's kicking the can down the road a bit, I'd like to go ahead with adding this rpc because mission control is a pretty underused feature IMO, and surfacing it will help people experiment with it. If we find ourselves exposing other config rpcs, then it's time to talk hot reload.

[carlaKC]

My only real comment is that since we can change the mission control config during route calculation

I don't think that there's a clean way to do this, without restructuring the way payment sessions use mission control quite significantly, which I'm not sure is worth the effort.

It's very unlikely as a single user but may be likely as service provider

If we do run into problems here, I think there could be an option to provide specific MC params for an individual payment, so that you can specify exactly what you want for that route. WDYT?

[bhandras]

LGTM 🧮

[bhandras]

nit: maxpayments ?

[alexbosworth]

    The amount of time mission control will take to restore a penalized node
    or channel back to 50% success probability, expressed as a unix timestamp
    in seconds. Setting this value to a higher value will penalize failures for
    longer, making mission control less likely to route through nodes and
    channels that we have previously recorded failures for.
    */
    uint64 half_life_seconds = 1;

If the value is just the number of seconds, why is it described as a unix timestamp?

[carlaKC]

If the value is just the number of seconds, why is it described as a unix timestamp?

Shouldn't be, fix in #5078.