Reported-by: ZmnSCPxj
Signed-off-by: Christian Decker <@cdecker>
Changelog-Fixed: pay: Correct a case where we put the sub-payment value instead of the *total* value in the `total_msat` field of a multi-part payment.
Only advance through routehints if no route was found at all, or if the
estimated capacity at the routehint is lower than the amount that we
have to send through the routehint.
Changelog-Fixed: pay: Be less aggressive with forgetting routehints.
The worst effect is that unpublished nodes are harder to pay, but
even published ones make us do unnecessary work, since we are
losing routehints from the published ones that could help us
actually route better to them.
listpays: make doc-all missed
Changelog-Added: JSON-RPC: `listpays` can be used to query payments using the `payment_hash`
Changelog-Added: JSON-RPC: `listpays` now includes the `payment_hash`
Changelog-Fixed: pay: Fixed a bug where routehints would be ignored if the payment exceeded 10,000 satoshi. This is particularly bad if the payee is only reachable via routehints in an invoice.
the way we use PSBTs to sign things requires that we have the
scriptpubkey available on the utxo so we can populate the witness-utxo
field with it.
this causes problems if we don't already have the scriptpubkey cached in
the database, as in *some* cases we require a round trip to the HSM to
populate them
to get over this hump, we backfill any and all missing scriptpubkey
information for the utxo's that we hold in our wallet.
this will allow us to clean up the NULL handling of missing
scriptpubkeys.
we're about to add a migration that requires access to the bip32_key
in order to calculate missing scriptpubkeys.
prior to this patch, we don't have access to the bip32 key in the db
migration, as it's set on the wallet but after the db migrations are
run.
here we patch it through so that every migration can access it
```
# Excludes channel, then ignores routehint which includes that, then
# it excludes other channel.
> assert len(status) == 2
E assert 1 == 2
E -1
E +2
```
The invoice we use at the end has a routehint: 50% of the time it's
to l2 (which fails), 50% to l5 (which succeeds).
Change it to create invoice before channel with l5 so it does the
retry like we expect here.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We sum up the amounts just like we do with `amount_sent`, however we may have
parts that don't have that annotation, so make it optional.
Suggested-by: Rusty Russell <@rustyrussell>
While not directly necessary, it still feeds the `listpays` result, and so we
should pass it along if we can, so we don't have to rely solely on the
`amount_sent` field, which includes the fees.
Reported-by: Rusty Russell <@rustyrussell>
Since we started using `sendonion` in the `pay` plugin we no longer
automatically have the `amount` annotation on (partial) payments. This
replicates the issue so we can fix it.
Reported-by: Rusty Russell <@rustyrussell>
There were no channel updates in my log; because sendonion doesn't know the
actual node_ids or channel_ids, we can't tell gossipd what node/channel it was
so it can no longer remove them on PERM errors.
However, we can tell it the error message so it can apply the update.
Fixes: #3877
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And document the partid arg.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `sendonion` has a new optional `bolt11` argument for when it's used to pay an invoice.
The shortcut in the retry_mod that we can skip retrying if getroute fails or
we have no result is only valid if the parameters don't change. As we iterate
through the routehints the parameters change, and so we must signal to the
retry_mod that it can retry even in those cases.
The child payments will sometimes depend on the step of the parent, and making
sure that the parent state is correct before we create the children is
therefore important.
This uses @cdecker's idea of excluding the routehinted channel from the route,
and also consumes the route hints as it goes so that it makes progress.
I don't know if this is correct, but it reliably passes tests/test_pay.py::test_tlv_or_legacy
now.
We store an offset of the current routehint in the modifier data. It gets
incremented on retry, and it gets reset to 0 on split. This is because once we
split we have a different amount and a previously unusable routehint becomes
usable again.
We have two places we need to do that now: in the root payment after we
checked if the destination is reachable, and in any other payment directly in
the initialization-step callback.
It was spread over the step callback, but we only need to initialize the
routehints array there, child-payments can just inherit most of the information.
This does two things: it checks if the destination of the payment is at all
reachable without routehints, and if it is it adds a direct attempt as option
to the routehints in the form of a NULL routehint. It also simplifies the
selection of the routehint since the direct case is no longer special, instead
we just return a NULL routehint as if it were a normal routehint.
The adaptive MPP test was showing an issue with always using a routehint, even
when it wasn't necessary: we would insist on routhing to the entrypoint of the
routehint, even through the actual destination. If a channel on that loop
would result being over capacity we'd slam below 0, and then increase again by
unapplying the route. The solution really is not to insist on routing through
a routehint, so we implement random skipping of routehints, and we rotate them
if we have multiples.
As the hints get new fields added it is easy to forget to amend one of the
places we create them, since we already have an update method let's use that
to handle all additions to the array of known channel hints.
We were removing the current hint from the list and not inheriting the current
routehint, so we'd be forgetting a hint at each retry. Now we keep the array
unchanged in the root, and simply skip the ones that are not usable given the
current information we have about the channels (in the form of channel_hints).
Fixes#3861
This may be related to the issue #3862, however the water was muddied by it
being the wrong error to return, and the node should not expect this courtesy
feature to be present at all...
There is little point in trying to split if the resulting HTLCs exceed the
maximum number of HTLCs we can add to our channels. So abort if a split would
result in more HTLCs than our channels can support.
The presplit modifier could end up exceeding the maximum number of HTLCs we
can add to a channel right out the gate, so we switch to a dynamic presplit if
that is the case. The presplit will now at most use 1/3rd of the available
HTLCs on the channels if the normal split would exceed the number of availabe
HTLCs. And we also abort early if we don't have a sufficient HTLCs available.
It turns out that by aggressively splitting payments we may end up exhausting
the number of HTLCs we can add to a channel quickly. By tracking the number of
HTLCs we can still add, and excluding the channels to which we cannot add any
more we increase the route diversity, and avoid quickly exhausting the HTLC
budget.
In the next commit we'll also implement an early abort if we've exhausted all
channels, so we don't end up splitting indefinitely and we can also optimize
the initial split to not run afoul of that limit.
Changelog-Fixed: plugin: `bcli` no longer logs a harmless warning about being unable to connect to the JSON-RPC interface.
Changelog-Added: plugin: Plugins can opt out of having an RPC connection automatically initialized on startup.
This PR includes the fix discussed on PR #3855. This fix was tested with the use case described inside the issue and worked.
Fixes: #3855
Changelog-None
ZmnSCPxj jxPCSnmZ
Christian Decker
Vincenzo Palazzo
Vincent
niftynei
Rusty Russell