Changelog-Fixed: Plugins: pay now correctly estimates channel capacity
after payment failure, fixing some retry cases.
The `estimated_capacity` was properly substracted from the channel
hint, but adding the amount back did not work. The amount was added back
and then immediately substracted again. This caused the
`estimated_capacity` to ever decrease. This commit makes sure re-adding
the amount to the `estimated_capacity` works as expected.
is_local, ie. "is this side of the channel ours?" is not needed since we
can determine that predicate by evaluating
`scidd->dir == node_id_idx(self, peer)`
In the Min. Cost Flow solver we put a constraint on arcs capacities,
such that the total flow that can be allocated through a channel does
not exceed htlc_max. This solves two previous problem of the htlc_max
constraint at the MCF level.
Add spendable/receivable and is_local fields to the callback function
used in gossmods_from_listpeerchannels. This allows to do more fine
grained use of the listpeerchannels call.
The first use case is renepay, for which we need to ignore the htlc_max
of the local channels only.
Changelog-Removed: Plugins: no longer allow missing `id` field in commando requests (deprecated v23.02, EOL v24.02)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We tried to put *everything* into the "all" output, which didn't work
if there were other outputs!
Fixes: https://github.com/ElementsProject/lightning/issues/6664
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: JSON-RPC: `multifundchannel` with `all` as an amount works as expected.
It's a u64, we should pass by copy. This is a big sweeping change,
but mainly mechanical (change one, compile, fix breakage, repeat).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This has the benefit of being shorter, as well as more reliable (you
will get a link error if we can't print it, not a runtime one!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have various functions to convert to a string, rename them all so we can
count on fmt_X being the formatter for struct X, and make them all return
`char *`.
Sometimes they existed but were private, sometimes they had a
different name. Most take a pointer, but simple types pass by copy:
short_channel_id, amount_msat and amount_sat.
The following public functions changed:
1. psbt_to_b64 -> fmt_wally_psbt.
2. pubkey_to_hexstr -> fmt_pubkey.
3. short_channel_id_to_str -> fmt_short_channel_id (scid by copy now!)
4. fmt_signature -> fmt_secp256k1_ecdsa_signature
5. fmt_amount_sat/fmt_amount_msat pass copy not pointer, return non-const char *.
6. node_id_to_hexstr -> fmt_node_id
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
a.k.a. "Pay with a friend!".
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `pay` has a new parameter `partial_msat` to only pay part of an invoice (someone else presumably will pay the rest at the same time!)
Suggested-by: Calle
In some weird situations it may happen that some channel along the route
could have htlcmax=htlcmin, so that the supremum of htlcmin and the
infimum of htlcmax are the same number. In that case there is only one
allowed amount that can go through that route.
Without this patch renepay would not handle correctly this cornercase.
Set up a simple line of channel pairs, where one should be preferred
over the other for our various reasons. Make sure this works, both
using a low-level call to Dijkstra and at a higher level as the pay
plugin does.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed that run-route-infloop chose some worse-looking paths after
routing was fixed, eg the second node:
Before:
Destination node, success, probability, hops, fees, cltv, scid...
02b3aa1e4ed31be83cca4bd367b2c01e39502cb25e282a9b4520ad376a1ba0a01a,1,0.991856,2,1004,40,2572260x39x0/1,2131897x45x0/0
After:
Destination node, success, probability, hops, fees, cltv, scid...
02b3aa1e4ed31be83cca4bd367b2c01e39502cb25e282a9b4520ad376a1ba0a01a,1,0.954540,3,1046,46,2570715x21x0/1,2346882x26x14/1,2131897x45x0/0
This is because although the final costs don't reflect it, routing was taking
into account local channels, and 2572260x39x0/1 has a base fee of 2970.
There's an easy fix: when we the pay plugin creates localmods for our
gossip graph, add all local channels with delay and fees equal to 0.
We do the same thing in our unit test. This improves things across
the board:
Linear success probability (when found): min-max(mean +/- stddev)
Before: 0.487040-0.999543(0.952548+/-0.075)
After: 0.486985-0.999750(0.975978+/-0.053)
Hops:
Before: 1-5(2.98374+/-0.77)
After: 1-5(2.09593+/-0.63)
Fees:
Before: 0-50848(922.457+/-2.7e+03)
After: 0-50041(861.621+/-2.7e+03)
Delay (blocks):
Before: 0-196(65.8081+/-60)
After: 0-190(60.3285+/-60)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Plugins: `pay` route algorithm doesn't bias against our own "expensive" channels any more.
The "path_score" callback was supposed to evaluate the *entire path*,
but that was counter-intuitive and opened the door to a cost function
bug which caused this path cost to be less than the closer path.
In particular, the capacity bias code didn't understand this at all.
1. Rename the function to `channel_score` and remove the "distance"
parameter (always "1" since you're supposed to be evaluating a
single hop).
2. Rename "cost" to the more specific "fee": "score" is our
actual cost function result (we avoid the word "cost" as it
may get confused with satoshi amounts).
3. For capacity biassing, we do want to know the amount, but
explicitly hand that as a separate parameter "total".
4. Fix a minor bug where total handed to scoring function previously
included channel fee (this is wrong: fee is paid before sending into
channel).
5. Remove the now-unused total_delay member from the dijkstra
struct.
Here are the results of our test now (routing 4194303 msat, which
didn't crash the old code, so we could compare). In both cases
we could find routes to 615 nodes:
Linear success probability (when found): min-max(mean +/- stddev)
Before: 0.484764-0.999750(0.9781+/-0.049)
After: 0.487040-0.999543(0.952548+/-0.075)
Hops:
Before: 1-5(2.13821+/-0.66)
After: 1-5(2.98374+/-0.77)
Fees:
Before: 0-50041(2173.75+/-5.3e+03)
After: 0-50848(922.457+/-2.7e+03)
Delay (blocks):
Before: 0-294(83.1642+/-68)
After: 0-196(65.8081+/-60)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: https://github.com/ElementsProject/lightning/issues/7092
Changelog-Fixed: Plugins: `pay` would occasionally crash on routing.
Changelog-Fixed: Plugins: `pay` route algorithm fixed and refined to balance fees and capacity far better.
It gave 0. A lot.
Firstly, rmsat was often very small, because delays are often small. Much
smaller than the actual fee.
We really just want to offset the bias and multiply it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We attempted to introduce a "capacity bias" so we would penalize
channels where we were using a large amount of their total capacity.
Unfortunately, this was both naive, and applied wrongly:
-log((capmsat + 1 - amtmsat) / (capmsat + 1));
As an integer gives 0 up to about 65% capacity. We then applied this
by multiplying:
(cmsat * rmsat * bias) / (cmsat + rmsat + bias + 1);
Giving a total score of 0 in these cases! If we're using the
arithmetic mean we really need to add 1 to bias. We might as well
use a double the whole way through, for a slightly more fine-grained
approach.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
So far we would call `preapproveinvoice` once for each payment split,
i.e., at least once per HTLC, and potentially more often. There is no
point in doing so repeatedly, and especially in remote signer setup
this is actually counterproductive due to the additional roundtrips.
Changelog-Changed pay: Improved performance by removing repeated `preapproveinvoice` calls
Whenever there is a payment failure that requires gossip update, for
example changing the fee rates of remote channels, we call addgossip.
For renepay to consider this changes in the coming payment attempts, it
must update gossmap.
- previous pending sendpays must add up so that the plugin tries to pay
the rest of the amount,
- avoid groupid, partid collisions,
- add shadow fees if the option is set and the payment amount - total
delivering = 0
- add a test,
- also fix a buggy shadow routing test
In the old version requesting the config-value of an option
was a little bit tricky.
Let's say we want to have a plugin which uses a default
port of 1234.
```rust
let value = plugin.option("plugin-port");
match value {
Some(Value::Integer(_)) => {},
Some(Value::String(_)) => {}, // Can never happen
Some(Value::Boolean(_)) => {}, // Can never happen
None => {}, // Can never happen
}
```
Many users of the `cln_plugin` crate are overly cautious
and handle all these error scenario's. Which is completely unneeded.
Core Lightning will complain if you put a `String` where an `Integer` is
expected and will never send the value to the plug-in.
This change makes the API much more ergonomical and actually motivates
some of the changes in previous commits.
```
const MY_OPTION : ConfigOption<i64> = ConfigOption::new_i64_with_default(
"plugin-port',
1235,
"Description");
let value : Result<i64> = plugin.option(MY_OPTION);
```
The result will provide a proper error-message.
It is also safe to `unwrap` the result because it will
only be triggered if the user neglected to provide the
option to the `Builder`.
This is the first part of two commits that attempts to simplify
the API that is used to retrieve configuration values.
Previously, we encouraged the following pattern to build a plugin.
```rust
let configured_plugin =
Builder::new(
tokio::io::stdin(),
tokio::io::stdout())
.option(ConfigOption::new_i64_with_default("some-option", 0, "Description"))
.configure();
let value = configured_plugion.option("some-option");
match value {
Some(Integer(i)) => {}, // Config provided
None => {}, // No config set
_ => {}, // This should never happened
}
```
This commit helps to move to the following pattern
```rust
const SOME_OPTION : ConfigOption<i64> = ConfigOption::new_i64_with_default(
"some-option",
"description");
async fn main() -> Result<()> {
let plugin = Builder::new(tokio::io::stdin(), tokio::io::stdoout())
.option(SOME_OPTION)
.configure()
.await?;
let value : i64 = plugin.option(SOME_OPTION)?;
}
```
Breaking changes here.
This improves the semantics of `ConfigOption` and `options::Value`
drastically.
We've been using `options::Value` for 2 purposes
1. To specify the type and default value of an option
2. Coummunicate how a user configured an option
We fall here in the pit-fall of being poor at both purposes.
I've edited the code to ensure
- `options::Value` -> To read or specify the value of an option
- `option::ValueType` -> To specify the type of an option
**Configure an option**
Let's create an string-typed option create an option named `"required-string-opt"` the
developer has to use the following code.
```rust
let option = ConfigOption::new("required-string", Value::OptString, "description");
```
The semantics of `OptString` might falsely suggest that it is optional to specify the config.
However, we use `OptString` to say we are not providing a default-value.
After this commit we can use instead
```rust
let option = ConfigOption::new_str_no_default("required-string", "description");
```
For reading a configured value the `option::Value` is somewhat
cumbersome. The old version of had 6 different types of value
```rust
let value = plugin.option("required-string");
match value {
String(s) => {}, // User has configured string value or default
Integer(i) => {}, // User has configured int value or default
Boolean(b) => {}, // User has configured bool value or default
OptString => {}, // User has not configured value and no default
OptInteger = {}, // User has not configured value and no default
OptBOolean => {}, // User has not configured value and no default
}
```
This has been changed to
```rust
let value = plugin.option("required-string");
match value {
Some(String(s)) => {},
Some(Integer(i)) => {},
Some(Boolean(b)) => {},
None => {},
}
```
Christian points out that this makes the type harder, and it's just awkward.
Changelog-EXPERIMENTAL: JSON-RPC: Deprecated `offer` parameter `recurrence_base` with `@` prefix: use `recurrence_start_any_period`.
Changelog-EXPERIMENTAL: JSON-RPC: Added `offer` parameter `recurrence_start_any_period`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
gossip_store_del - takes a gossmap-style offset-of-msg not offset-of-hdr.
gossip_store_flag: set an arbitrary flag on a gossip_store hdr.
gossip_store_get_timestamp/gossip_store_set_timestamp: access gossip_store hdr.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The only way you'll see private channel_updates is if you put them
there yourself with localmods.
I also renamed the confusing gossmap_chan_capacity to gossmap_chan_has_capacity.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
update_count is just the count of the records for a tx. To calculate onchain fees
for an account we must sum all credits vs debits. We don't need to GROUP BY update_count
nor ORDER BY update_count since it is just a running index of updates to this tx.
Remove grouping by update_count which resulted in a crash due to bad arithmetic
caused by fee calculation returned rows not being consolidated.
Remove xfail.
