sync: sync all payment updates

[JssDWt]

Rather than syncing payments based on the payment time, sync based on the created and updated indices in core lightning. This ensures that all updates are retrieved, also when a payment is updated after the fact. To do this, listsendpays is called, rather than listpays. listsendpays contains a filter based on index. Starting off with the last synced index, fetch all missing newly created payments and updates. It's possible that this particular sync doesn't fetch all payment parts associated to a payment. Therefore, the sendpays are cached into a local database. When a new sync round is done, all known existing parts are fetched from the local database as well, to ensure the resulting sdk payments are 'complete', i.e. they're not missing any parts. This should fix the issue where for closed channels, payments would be marked forever pending.

Should fix https://github.com/breez/breez-sdk-greenlight/issues/1094

Inspiration from https://github.com/ElementsProject/lightning/blob/136244835215ae7fcb48ffcaf3c5bb80c3173348/plugins/pay.c#L415-L533

[roeierez]

@JssDWt this looks very smart and the main benefit I see is that we can be truly incremental by fetching only the changed data. One thing that is not clear to me is why the stuck pending payments happens with the current code. After all we do fetch all outbound payments and as long as the payment is not completed we keep syncing it.

[JssDWt]

One thing that is not clear to me is why the stuck pending payments happens with the current code. After all we do fetch all outbound payments and as long as the payment is not completed we keep syncing it.

Take the situation where a payment was pending on round 1, and failed on round 2. The current filter could 1) filter out the round 2 payment based on the created_at timestamp, if another payment had already succeeded after the current. 2) filter out the round 2 payment because it doesn't have a completed_at (this is only present for complete payments)

Note that this current PR also syncs all failed payments. Maybe it's worth adding a commit to delete failed payments from the payments database?

[roeierez]

One thing that is not clear to me is why the stuck pending payments happens with the current code. After all we do fetch all outbound payments and as long as the payment is not completed we keep syncing it.

Take the situation where a payment was pending on round 1, and failed on round 2. The current filter could

1. filter out the round 2 payment based on the created_at timestamp, if another payment had already succeeded after the current.
2. filter out the round 2 payment because it doesn't have a completed_at (this is only present for complete payments)

Note that this current PR also syncs all failed payments. Maybe it's worth adding a commit to delete failed payments from the payments database?

1. For the first case isn't that what we want? If eventually the payment succeeded for that hash why do we care about old attempts?
2. For the second case as far as I see in the code we don't filter these payments that don't have completed_at value so the payment should be included in the sync.

[JssDWt]

1. For the first case isn't that what we want? If eventually the payment succeeded for that hash why do we care about old attempts?

I don't mean the same payment. If any other payment succeeds after the pending one is synced, then the since_timestamp will be after the one of the pending payment, so it will be skipped.

2. For the second case as far as I see in the code we don't filter these payments that don't have completed_at value so the payment should be included in the sync.

You're right. Only the since_timestamp matters there, which could again be after the completed_at value in case of a success.

[roeierez]

I don't mean the same payment. If any other payment succeeds after the pending one is synced, then the since_timestamp will be after the one of the pending payment, so it will be skipped.

I see. Still there is this question how do we skip that payment if we never filter out payments that were completed after the last_sync_time. I mean even if meantime there is a different payment that has been succeed and changed the last_sync_time and only after that the "round 2" payment completed successfully then we shouldn't have skipped it because we don't skip those that their completed_at is after the last_sync_time: https://github.com/breez/breez-sdk-greenlight/blob/main/libs/sdk-core/src/greenlight/node_api.rs#L1796 In short, perhaps I am missing something here but I don't see how the since_timestamp can be after the completed_at unless the payment already has been synced as completed.

Sorry for being Nitty on this, I think the incremental benefit is very valuable by itself just curious if this indeed solves the pending annoying issue.

[JssDWt]

I mean even if meantime there is a different payment that has been succeed and changed the last_sync_time and only after that the "round 2" payment completed successfully then we shouldn't have skipped it because we don't skip those that their completed_at is after the last_sync_time: https://github.com/breez/breez-sdk-greenlight/blob/main/libs/sdk-core/src/greenlight/node_api.rs#L1796

Right, so basically it should only be filtered out if the payment has completed successfully, and has a completed_at time that is before the highest payment_time (which is either created_at or) time of the latest sync. That seems unlikely indeed.

I agree it should work for the most part, as long as the timestamps are accurate, and all created on the same machine.

There's another suspect: https://github.com/breez/breez-sdk/blob/f6c7c8729c51c01ab7a62787bc94c340e590f43a/libs/sdk-core/src/greenlight/node_api.rs#L1851

Used here: https://github.com/breez/breez-sdk/blob/3fb6672e7970ad4b3e1495b18305f655ddd920fe/libs/sdk-core/src/breez_services.rs#L1640

[roeierez]

@JssDWt do you think it worth that @andrei-21 will check this PR on his issue to get feedback?