Fix `sendpay` aggregation and ordering in `listpays` by cdecker · Pull Request #4567 · ElementsProject/lightning

cdecker · 2021-05-31T15:33:59Z

This is still work in progress, but I wanted to share the current
state. The main goal is to address #4481 and #4482, i.e., fix the
ordering and address flip-flopping states on supposedly finished
payments. It takes a bit of a detour to get there. The idea is to add
a grouping key groupid to sendpay instances, such that all
sendpay instances that originate from the same pay invocation
share the same groupid. This way listpays can group them into the
correct group when aggregating sendpays for each pay
invocation. However, while implementing this simple approach I noticed
we actually delete prior attempts automagically when retrying them,
which still results in flip-flopping, and is strange if you ask me: if
I called pay 2 times, I should see it 2 times in listpays.

So we now no longer delete prior attempts, but it also means we need
to add groupid to any RPC call that may refer to a specific
sendpay call: waitsendpay, sendonion, sendpay, etc. In
addition we now need to annotate HTLCs we have in the DB with which
(payment_hash, partid, groupid) triple they refer to in order to
correctly route results back to the caller.

I'm still working on some failing tests, but it should just be a
matter of time now :-)

Fixes #4481
Fixes #4482
Fixes #4780

rustyrussell · 2021-06-03T05:08:27Z

lightningd/htlc_end.c

 			      const struct pubkey *blinding,
 			      bool am_origin,
 			      u64 partid,
+			      u64 groupid,


This is kinda true, except that we do not support more than one group in flight at a time, so this commit, while not wrong and a nice sanity check, should be unnecessary.

(I wish we could comment on commits not just code!)

lightningd/pay.c

Also add `groupid` to the payment fields so we can retrieve them too.

When doing things like `waitsendpay` without specifying the `groupid` we likely want to use the latest `groupid` we created, since that's the one in flight. This adds a function to quickly retrieve that.

So far we've always been deferring the deletion, retry and early abort logic to `sendonion` and `sendpay` which do not have the context to decide if a call is legitimate or not (they were mostly based on heuristics). By calling `listsendpays` for the invoice's `payment_hash` we can identify what our `groupid` should be, but more importantly we can also abort if another payment is pending or a prior attempt has already succeeded.

This was the main cause of the pay states flip-flopping, since we reset the status on each attempt any final status is not really final. Let's keep them around, and provide a stable history.

One of the fundamental constraints of the payment groups idea is that there may only ever be one group in flight at any point in time, so if we find a group that is in flight, any new `sendpay` or `sendonion` must match its `groupid`.

This re-establishes the prior behavior where a `sendpay` or `sendonion` that'd match a prior payment would cause the prior payment to be deleted. While we no longer delete prior attempts we now avoid a primary key collision by incrementing once. This helps us not having to touch all existing tests, and likely avoids breaking other users too.

We're about to suspend duplicate calls to `pay` and this will help us notify them if the original payment completes.

We want to avoid returning duplicate results when cross-completing, so mark them as completed when we return a result.

Fixes ElementsProject#4482 Fixes ElementsProject#4481 Changelog-Added: pay: Payment attempts are now grouped by the pay command that initiated them Changelog-Fixed: pay: `listpays` returns payments orderd by their creation date Changelog-Fixed: pay: `listpays` no longer groups attempts from multiple attempts to pay an invoice

SQL doesn't really allow `a OR 1` as a clause since `1` is not a boolean expression. Moving it into `a OR 1=1` however is valid again.

rustyrussell · 2021-10-12T05:38:42Z

plugins/pay.c

+		/* New group, reset what we collected. */
+		if (last_group != groupid) {
+			completed = false;
+			pending = false;
+			last_group = groupid;
+		}


This assumes that listsendpays is listed in increasing groupid order? That assumption does not seem documented?

This is a listsendpays call limited to the payment_hash of the invoice. There cannot be multiple groups in flight concurrently. So ordering by id which we do internally in lightningd/pay.c will result in the desired ordering of groupid. I'll add a comment to that effect 👍

rustyrussell · 2021-10-12T05:40:11Z

lightningd/pay.c

-	res = wait_payment(cmd->ld, cmd, rhash, *partid);
+	if (groupid == NULL) {
+		groupid = tal(cmd, u64);
+		*groupid = wallet_payment_get_groupid(cmd->ld->wallet, rhash);


Would prefer this function to be called "wallet_payment_get_last_groupid" to be clearer? (or max_groupid)?

rustyrussell · 2021-10-12T05:45:39Z

plugins/pay.c

 			last_group = groupid;
+
+			parts = 1;
+			json_scan(tmpctx, buf, t,


Hmm, is there no group_id 0? Since you initialize last_group to 0, it seems you won't run this for the first group. Since msat is not initialized elsewhere, this means you will use it uninitialized below?

The pay plugin Will indeed skip over groupid 0, but pure sendpay calls outside of pay may produce that group too

Could either make it a pointer to make that explicit, but the msats field is only used if we detect that there was a prior success or failure and replicate the result for idempotency, so not having a prior attempt means that we'll not use msats at al

rustyrussell · 2021-10-13T03:11:04Z

Ack 71752df

cdecker changed the title ~~Fix sendpay aggregation and ordering in listpays~~ Fix sendpay aggregation and ordering in listpays May 31, 2021

cdecker marked this pull request as draft May 31, 2021 20:09

rustyrussell reviewed Jun 3, 2021

View reviewed changes

cdecker force-pushed the sendpay-groups branch 2 times, most recently from 99bbf73 to 80aa906 Compare September 27, 2021 14:52

cdecker force-pushed the sendpay-groups branch from 80aa906 to cb684e9 Compare September 28, 2021 15:01

cdecker added this to the v0.10.2 milestone Sep 29, 2021

cdecker added 3 commits October 4, 2021 13:26

pytest: Reproduce ElementsProject#4482

db5181f

db: Add groupid to the payments table

9dc4726

jsonrpc: Add groupid to sendpay and sendonion

af371b6

Also add `groupid` to the payment fields so we can retrieve them too.

cdecker force-pushed the sendpay-groups branch from cb684e9 to eaebbb9 Compare October 6, 2021 13:50

cdecker marked this pull request as ready for review October 6, 2021 13:50

cdecker force-pushed the sendpay-groups branch from eaebbb9 to c2871a9 Compare October 6, 2021 13:51

cdecker added 3 commits October 6, 2021 17:37

pyln: Add groupid to sendpay and sendonion

9f887f3

pay: Add groupid to the payment struct

8163ae3

wallet: Add function to retrieve the latest groupid for a payment

e73564d

When doing things like `waitsendpay` without specifying the `groupid` we likely want to use the latest `groupid` we created, since that's the one in flight. This adds a function to quickly retrieve that.

cdecker force-pushed the sendpay-groups branch 2 times, most recently from ef656e6 to 83d3551 Compare October 6, 2021 15:48

cdecker added 12 commits October 8, 2021 15:04

db: Add groupid to HTLCs

779826a

jsonrpc: Add missing partid in listsendpays schema

2529daa

doc: Add missing amount_sent_msat to listpays

fc62b7f

jsonrpc: Add groupid to waitsendpay

38ec80d

pay: Make pay idempotent

95641e3

pay: Do not delete old sendpay attempts if we retry

dd78d61

This was the main cause of the pay states flip-flopping, since we reset the status on each attempt any final status is not really final. Let's keep them around, and provide a stable history.

paycore: Prevent multiple concurrent payment groups

6e1341e

One of the fundamental constraints of the payment groups idea is that there may only ever be one group in flight at any point in time, so if we find a group that is in flight, any new `sendpay` or `sendonion` must match its `groupid`.

pytest: Fix up test_partial_payment to use a single groupid

d555a99

pay: Fail a sendpay or sendonion that'd produce a DB collision

75a4f5b

pytest: Add groupid to test_partial_payment_{timeout,restart}

fc94852

cdecker added 7 commits October 8, 2021 15:04

pytest: Fix test_onchain_timeout to use groupid

fea3d08

libplugin: Add callbacks for successful and failed payments

f726fc6

We're about to suspend duplicate calls to `pay` and this will help us notify them if the original payment completes.

pytest: Adjust test_sendpay to the new semantics

11d18c1

pay: Mark completed payments as such by nullifying cmd

b8280be

We want to avoid returning duplicate results when cross-completing, so mark them as completed when we return a result.

pay: Stash and forward results for duplicate pay calls

0da9623

db: Fix a syntax error with the optional parameters

71752df

SQL doesn't really allow `a OR 1` as a clause since `1` is not a boolean expression. Moving it into `a OR 1=1` however is valid again.

cdecker force-pushed the sendpay-groups branch from a42d89e to 71752df Compare October 8, 2021 13:04

cdecker mentioned this pull request Oct 10, 2021

listpays vs paystatus #4780

Closed

rustyrussell reviewed Oct 12, 2021

View reviewed changes

rustyrussell merged commit cd7d87f into ElementsProject:master Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `sendpay` aggregation and ordering in `listpays`#4567

Fix `sendpay` aggregation and ordering in `listpays`#4567
rustyrussell merged 25 commits intoElementsProject:masterfrom
cdecker:sendpay-groups

cdecker commented May 31, 2021 •

edited

Loading

Uh oh!

rustyrussell Jun 3, 2021

Uh oh!

rustyrussell Jun 3, 2021

Uh oh!

Uh oh!

rustyrussell Oct 12, 2021

Uh oh!

cdecker Oct 12, 2021

Uh oh!

rustyrussell Oct 12, 2021

Uh oh!

rustyrussell Oct 12, 2021

Uh oh!

cdecker Oct 12, 2021

Uh oh!

cdecker Oct 12, 2021

Uh oh!

rustyrussell commented Oct 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cdecker commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Oct 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cdecker commented May 31, 2021 •

edited

Loading