paymod: Slow but steady progress (part 02) #3760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdecker
force-pushed
the
paymod-02
branch
8 times, most recently
from
35d6238
to
a2208bf
Compare
niftynei
reviewed
Jun 12, 2020
cdecker
force-pushed
the
paymod-02
branch
2 times, most recently
from
26306c7
to
00bbed1
Compare
cdecker
force-pushed
the
paymod-01
branch
2 times, most recently
from
34efe7e
to
dd4e95f
Compare
rustyrussell
approved these changes
Jun 19, 2020
There was a problem hiding this comment.
Mainly minor comments, though JSON return one needs addressing.
Ack 00bbed1
cdecker
force-pushed
the
paymod-01
branch
2 times, most recently
from
e581e3c
to
ae6d53e
Compare
|
Rebased on top of paymod-01: |
|
Ack a7540de |
|
Needs annotations in a couple of places for direct amount access. |
Since we end up consolidating some of the return values for `pay` and `paystatus` and change the public interface we need to add the compatibility flag and guard the switchover behind it.
We can have quite detailed information about our local channels, so call `listpeers` before the `getroute` call on the root payment, to seed that information in the channel_hints.
We'll need it in the next commit to exclude channels and their directions.
An important life lesson: if there is no path to happiness, then trying harder will still not get you there.
There is a race between `getroute` learning that our peer accepts TLVs and us initiating the payment. Waiting for announcements ensures we always use TLVs, matching our expectation in the test / plugin.
This allows us to drive the payment from outside, and still get the aggregation that we want.
This was racy since we didn't know whether the peer supports TLV payloads yet so we defaulted to legacy, which doesn't support secrets.
We frequently query by the bolt11 string, so keeping it around saves us from having to parse the query or serialize the parsed one.
Without this the flapping behavior tested in `test_pay_retry` would reappear.
We're lucky that we can distinguish the severity of the failure based on the failcode, so we bubble up the one with the maximum failcode, and let callers inspect details if they need more information.
It was there only for demonstration purposes, and is no longer useful.
|
Rebased on top of |
We could end up with multiple channel hints, which is a bit wasteful. We now look for existing ones before adding a new one, and if one exists we use the more restrictive parameters. Suggested-by: Lisa Neigut <@niftynei>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR merges into the branch of the first part
paymod-01and notmaster. 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 stackingmultiple PRs 😉
We start by switching over to
We then do a lotjson_paymodif we are not running inCOMPAT-mode, as mentioned in the first part of this series.of the grunt work required to get
payandpaystatusreturn values toinclude the information from our new payment flow. It's not perfect and I
decided not to go for 100% result compatibility, opting instead of guarding
the change behind the
COMPAT_V090flag, allowing for users to test beforemaking the switch.
No earth shattering changes here, just incremental work, but a couple of that might be interesting to look at:
local_channel_hintsmodifier: fill in thechannel_hintsfromlistpeersso we don't try to use a channel we should know won't workanyway.
information reported to the caller of
payandpaystatus. It's also usedto determine whether the payment is still in progress, and triages the
errors to bubble up the most important.
Edit: I had to disable the
COMPAT-flag switchover, since Travis is testing withCOMPAT=0, causing a number of tests to fail. We'll re-arm it once we have fixed all failures and fixed up the tests to match.Changelog-None