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.
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
The code had incorrect assertions, partially because it didn't clearly
distinguish errors from the final node (which, barring blockheight issues,
mean a complete failre) and intermediate nodes.
In particular, we can't trust the *values*, so we need to distinguish
these by the *sender*.
If a route is of length 2 (A, B):
- erring_index == 0 means us complaining about channel A.
- erring_index == 1 means A.node complaining about channel B.
- erring_index == 2 means the final destination node B.node.
This is particularly of note because Travis does NOT run test_pay_routeboost!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were using the current constraints, including any shadow route and other
modifications, when computing the remainder that the second child should
use. Instead we should use the `start_constraints` on the parent payment,
which is a copy of `constraints` created in `payment_start` exactly for this
purpose.
Also added an assert for the invariant on the multiplier.
When using mpp we need to always have partids>0, since we bumped the partid
for the root, but not the next_id we'd end up with partid=1 being
duplicated. Not a big problem since we never ended up sending the root to
lightningd, instead skipping it, but it was confusing me while trying to trace
sub-payment's ancestry.
We skip most payment steps and all sub-payments, so consolidate the skip
conditions in one if-statement. We also not use `payment_set_step` to skip any
modifiers after us after the step change.
We now check against both constraints on the modifier and the payment before
applying either. This "fixes" the assert that was causing the crash in #3851,
but we are still looking for the source of the inconsistency where the
modifier constraints, initialized to 1/4th of the payment, suddenly get more
permissive than the payment itself.
This was highlighted in #3851, so I added an assertion. After the rewrite in
the next commit we would simply skip if any of the constraints were not
maintained, but this serves as the canary in the coalmine, so we don't paper over.
While we were unsetting the `payment->cmd` in case of a success to signal that
we should not return to the JSON-RPC command twice, we were not doing that in
the case of failures. This was causing multiple responses to a single incoming
command, and `lightningd` was correctly killing the plugin. This issue was
introduced through early returns (anything setting `payment->abort=true`) and
was caused in Rusty's case through an MPP timeout.
Fixes#3847
Reported-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
We were rather pedanticly failing the plugin if we were unable to parse the
`waitsendpay` result, but had coded all the modifiers in such a way that they
can handle a `NULL` result (verified in the code and manually by randomly
failing the parsing). So we now just log the result we failed to parse and
merrily go our way.
Worst case is that we end up retrying the same route multiple times, since we
can't blacklist any nodes / channels without understanding the error, but that
is still in the scope of what we must handle anyway.
This modifier splits a payment that has been attempted a number of times (by a
modifier earlier in the mod chain) and has failed consistently. It splits the
amount roughly in half, with a but if random fuzz, and then starts a new round
of attempts for the two smaller amounts.
With the `presplit`-modifier we actually skip execution of the root altogether
which results in the root not having a result at all. Instead we should use
the result returned by `payment_collect_result`.
With MPP we require that the sum of parts is equal to the `total_msat` amount
declared in the onion. Since that can't be changed once the first part arrives
we need a way to disable amount fuzzing for MPP.
Technically an API break, but nobody relies on these I hope!
Note that the feerates warning was buried inside the style object:
it should be top-level.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were applying the fee exemption to all payments individually, which is ok
until we switch to MPP, where amounts change. Also the log entry was referring
to the total amount, and not the fee of the payment.
This was causing the state flapping test to fail, since we were yielding
control of the io_loop, waiting for the blockheight to be reached, and not
setting the status beforehand. An interim `paystatus` call would then find a
failed leaf and deduce the entire payment failed. Setting it back to the
previous state keeps the overall payment pending while we wait.
As suggested during the paymod-03 review it is better to activate the new code
right away, and give users an escape hatch to use the legacy code instead. The
way I implemented it allows using either `legacypay` or `pay` and then set
`legacy` to switch to the other implementation.
Changelog-Added: JSON-RPC: The `pay` command now uses the new payment flow, the new `legacypay` command can be used to issue payment with the legacy code if required.
Suggested-by: Rusty Russell <@rustyrussell>
Suggested-by: ZmnSCPxj <@zmnscpxj>
It turns out that the `failcodename` doesn't get populated if the `failcode`
isn't a known error from the enum (duh...) so don't fail parsing if it's
missing.
We want to differentiate a wrong block-height from other failure reasons, such
as an unknown `payment_hash`, so we skip the `waitblockheight` if we're
already at the correct height.
Christian Decker
Vincent
Rusty Russell