sync: sync all payment updates #1095

JssDWt · 2024-10-04T13:55:23Z

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 #1094

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

roeierez · 2024-10-05T07:29:52Z

@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 · 2024-10-05T07:56:59Z

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

filter out the round 2 payment based on the created_at timestamp, if another payment had already succeeded after the current.
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 · 2024-10-05T09:01:14Z

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

filter out the round 2 payment based on the created_at timestamp, if another payment had already succeeded after the current.

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?

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?
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 · 2024-10-05T09:16:23Z

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.

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 · 2024-10-05T10:43:52Z

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 · 2024-10-05T11:52:35Z

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

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.

roeierez

Looks good. It would be great if the indices query would be available also for list_pays freeing us from handling it in low level but unfortunately it is not the case.

roeierez · 2024-10-06T08:32:13Z

libs/sdk-core/src/greenlight/node_api.rs

+            let mut key = hex::encode(&send_pay.payment_hash);
+            key.push('|');
+            key.push_str(&send_pay.groupid.to_string());
+            if !hash_groups.contains(&key) {


Do you think it makes sense to query from the db directly those that we want? (based on the payment hash and group id).

roeierez · 2024-10-06T08:54:47Z

libs/sdk-core/src/greenlight/node_api.rs

+        if let Some(last) = created_invoices.invoices.last() {
+            new_sync_state.list_invoices_index.created = last.created_index()
+        }
+        let updated_invoices = node_client


Currently we are adding extra call for invoices and payments (created, updated). Did you check perhaps querying by updated is enough? I mean what is the value of updated for a newly create invoice or payment? IF it is the same as the created timestamp than we should be good.

Newly created pays/invoices are not returned when querying the updated index. It's really only for updates to previously created pays/invoices.

roeierez · 2024-10-06T08:56:05Z

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

hydra-yse · 2024-10-10T08:19:29Z

libs/sdk-core/src/greenlight/node_api.rs

+    let amount_msat = match send_pay_amount_msat {
+        Some(amount_msat) => amount_msat,
+        None => {
+            agg.amount = None;
+            return;
+        }
+    };
+
+    if let Some(amount) = agg.amount {
+        agg.amount = Some(amount + amount_msat);
+    }


This should fix breez/c-breez#909:

if let Some(send_pay_amount_msat) = send_pay_amount_msat { agg.amount = Some(agg.amount.unwrap_or(0) + send_pay_amount_msat); }

JssDWt requested review from dangeross, roeierez and ok300 October 4, 2024 13:55

JssDWt force-pushed the jssdwt-sync-all-updates branch from 3c5588a to 0b1d841 Compare October 5, 2024 14:38

roeierez requested changes Oct 6, 2024

View reviewed changes

roeierez approved these changes Oct 7, 2024

View reviewed changes

JssDWt merged commit 0b1d841 into main Oct 7, 2024
9 checks passed

JssDWt temporarily deployed to github-pages October 7, 2024 10:59 — with GitHub Actions Inactive

hydra-yse reviewed Oct 10, 2024

View reviewed changes

JssDWt deleted the jssdwt-sync-all-updates branch October 14, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: sync all payment updates #1095

sync: sync all payment updates #1095

JssDWt commented Oct 4, 2024 •

edited

Loading

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024 •

edited

Loading

roeierez left a comment

roeierez Oct 6, 2024

roeierez Oct 6, 2024

JssDWt Oct 6, 2024

roeierez commented Oct 6, 2024

hydra-yse Oct 10, 2024 •

edited

Loading

sync: sync all payment updates #1095

sync: sync all payment updates #1095

Conversation

JssDWt commented Oct 4, 2024 • edited Loading

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024

roeierez commented Oct 5, 2024

JssDWt commented Oct 5, 2024 • edited Loading

roeierez left a comment

Choose a reason for hiding this comment

roeierez Oct 6, 2024

Choose a reason for hiding this comment

roeierez Oct 6, 2024

Choose a reason for hiding this comment

JssDWt Oct 6, 2024

Choose a reason for hiding this comment

roeierez commented Oct 6, 2024

hydra-yse Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

JssDWt commented Oct 4, 2024 •

edited

Loading

JssDWt commented Oct 5, 2024 •

edited

Loading

hydra-yse Oct 10, 2024 •

edited

Loading