Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

paymod: Feature parity, finally (part 03) #3773

Merged
merged 25 commits into from Jul 7, 2020
Merged

paymod: Feature parity, finally (part 03) #3773

merged 25 commits into from Jul 7, 2020

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jun 16, 2020

This PR merges into the branch of the second part paymod-02 and not
master. This should reduce the number of commits that are to be reviewed,
but I'm still unclear if merging the first part will re-address this PR to
point to master. It's an experiment, so let's see if this works for stacking
multiple PRs 馃槈

This is the final part of the large refactor into a new payment flow (see
#3753 and #3760 for the previous parts). It implements most of the missing
functionality as payment_modifiers and reaches feature parity with the old
pay command.

The PR is not as clean as it could be, in particular some things get moved
around a bit in commits to consolidate their location, but rebasing to get
them into their final position right away would be more work than its worth.

  • I moved some of the deserialization functions of RPC results into
    common/rpcclient.[ch] in order to cut down on future need to manually
    deserialize those results. It parses a bit more than any single use might
    need, but it frees us from having to juggle JSON dict keys and the like in
    the future.

New Features

  • I added a new direct_pay_mod which overrides the call to getroute
    altogether if we know we have a direct channel that can be attempted for
    the payment, thus disabling any form of fuzzing or shadow-routing in cases
    we definitely don't need it. On failure we will still fall back to the
    normal behavior.

  • We now track channel_hints by applying routes when we start an attempt,
    and un-applying them if the attempt fails. While we do not share these
    hints across payments due to concerns of acting on stale information, these
    updated hints will become very important once we have an MPP modifier which
    will result in multiple concurrent attempts needing up-to-date estimates on
    the state of a channel. These hints are still only best effort estimates,
    and we will update them as we learn more information on capacity and other
    constraints.

  • We move from a "compute-constraints-on-the-fly" system to a precomputed
    constraints model, in which we precompute the fee and cltv budget at the
    beginning and can then split that budget for retries and parallel
    attempts. This reduces the number of places in which the fees and CTLV
    deltas get computed considerably.

Activation

For testing purposes the new implementation is guarded behind the COMPAT
flag, we are discussing however whether we just activate the new
implementation by default, and provide an escape hatch in the form of a CLI
flag to use the old implementation. We can likely gather some feedback once
merged into master since the plugin repository has nightly CI runs
configured to run against COMPAT=0 on master.

Next steps

And now for the reason we started out on this long and tortuous journey in the
first place. We can now implement keysend sending support and MPP sending
support with relative ease, by implementing them as modifiers 馃槈 I will
queue up those as separate pull requests.

Changelog-None

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Jun 18, 2020

we are discussing however whether we just activate the new
implementation by default, and provide an escape hatch in the form of a CLI
flag to use the old implementation.

I say go for it, and keep the old pay as the command pay_old, which we warn as deprecated.

@cdecker
Copy link
Member Author

cdecker commented Jun 22, 2020

Rebased on top of paymod-02: range-diff

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Excellent series!

I believe we should do as @ZmnSCPxj suggested, and move the old one to oldpay, and simply call this pay.

We also need to remove the over-pay change: it reveals an actual issue with our implementation, which should not overpay when doing MPP of more than 1 part, IMHO (i.e. adjust CLTV only on shadow routes)

plugins/pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Show resolved Hide resolved
lightningd/htlc_set.c Outdated Show resolved Hide resolved
common/rpcclient.c Outdated Show resolved Hide resolved
@cdecker
Copy link
Member Author

cdecker commented Jul 2, 2020

we are discussing however whether we just activate the new
implementation by default, and provide an escape hatch in the form of a CLI
flag to use the old implementation.

I say go for it, and keep the old pay as the command pay_old, which we warn as deprecated.

I wonder if we should even provide a pay_old at all, or whether we just add a legacy flag to the pay command that then switches over to the legacy code, directly after parsing the options. Could get us into trouble if we add params, but for now it works.

I have also been gathering some results by probing the network with the old and the new payment flow, and comparing the outcomes. This is mainly to bolster my own confidence in the new code, and get a comparison between the behavior and stability of the two. So after doing 50k payments with the new code I feel much more comfortable doing a hard switch.

I also found a rare border case that would crash the pay plugin, when a node replies with an error code that we don't understand, which then results in the failcodename not being filled in by waitsendpay. Fixed that as well.

We also need to remove the over-pay change: it reveals an actual issue with our implementation, which should not overpay when doing MPP of more than 1 part, IMHO (i.e. adjust CLTV only on shadow routes)

Like mentioned in the above comment, I'll move the fix in the form of an opt-out flag for amount-fuzzing to getroute and the shadow_route modifier. CLTV fuzzing in the shadow_route is still desirable imho, and we should keep the amount-fuzzing for non-MPP payments. I plan to also add a test for MPP that we are really sending the exact amount, and don't overpay. More on that in the MPP PR which I'll hopefully publish by the end of the week.

@cdecker cdecker force-pushed the paymod-03 branch 2 times, most recently from 8715eb1 to ff3798b Compare Jul 3, 2020
@cdecker cdecker requested a review from rustyrussell Jul 3, 2020
@cdecker cdecker force-pushed the paymod-03 branch 2 times, most recently from a91dd6f to 5849969 Compare Jul 3, 2020
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Ack f0602e8

Minor style changes and a few questions only...

plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
lightningd/htlc_set.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Show resolved Hide resolved
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
plugins/libplugin-pay.c Show resolved Hide resolved
@cdecker cdecker force-pushed the paymod-03 branch 4 times, most recently from f97fd2c to 8ebcef8 Compare Jul 6, 2020
cdecker added 25 commits Jul 7, 2020
We need to set some variables as well, and the transition to FAILED is a bit
special in that quite some modifiers will skip into FAILED.
This proved to be rather difficult given the tight coupling of the old structs
and the output of the command.
This is a slight change in interface in that the string no longer spells out
that a specific node was excluded.
This is necessary for the routehint modifier.
Keeping the arguments in their non-serialized form allows us to amend them as
we apply modifiers and learn new information.
So far we got away with not caring about these but since we're implementing
modifiers that impact these limits, we better keep track of them.
These are primarily the fee and cltv constraints that we need to keep up to
date in order to give modifiers a correct view of what is and what isn't
allowed.
Since we change the interface for `pay` and `paystatus` we need a way to
determine which one to expect. This just parses the COMPAT_Vxyz flags and
allows us to check if, based on the configuration, we are in compat mode for
that set of changes or not.
Add/remove the HTLC amount from the channel hints so concurrent getroute calls
have the correct exclusions. This can sometimes underflow, if we're unlucky
and call getroute too closely, but that's not a big issue, it can only result
in a failed MPP attempt too many, nothing fatal, and it'll get corrected based
on the result returned by the failing node.
This makes it easier to stash a human readable failure message in an attempt.
This is necessary otherwise we would not be calling modifiers for the newly
set state which can lead to unexpected results.
We handle these in a number of different ways, and regularly get the parsing
and logic for optional fields wrong, so let's consolidate them here.
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.
These get reflected in the `listsendpays` command, and are quite useful.
We've been adding modifiers and arguments out of order, and we need the
arguments order to match up if we want `paymod` to be a drop-in replacement.
This commit collects the changes required to the tests caused by the changes
to the `pay` and `paystatus` commands. They are also rather good hints as to
what these changes entail.
I wrongly used a pointer to the array that'd move on append, so an extra
dereference was required here.
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.
@cdecker
Copy link
Member Author

cdecker commented Jul 7, 2020

Rebased on top of master

@rustyrussell
Copy link
Contributor

rustyrussell commented Jul 7, 2020

Ack b606f46

@cdecker cdecker merged commit af4955c into master Jul 7, 2020
4 of 5 checks passed
@rustyrussell rustyrussell deleted the paymod-03 branch Aug 15, 2022
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.

None yet

4 participants