Persist pending payment amount #623

dleutenegger · 2023-11-16T17:15:41Z

Resolves #599

Builds upon #621

dangeross

Looking good, just need to change how the attempted_amount_msat is persisted

dangeross · 2023-11-16T21:22:06Z

libs/sdk-core/src/breez_services.rs

+            id: invoice.payment_hash.clone(),
+            payment_type: PaymentType::Sent,
+            payment_time: SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs() as i64,
+            amount_msat,


This can be set to 0 and instead call persister.insert_payment_external_info() to store the attempted_amount_msat in the payment_external_info sync table.

Suggested change

amount_msat,

amount_msat: 0,

This can be set to 0 and instead call persister.insert_payment_external_info() to store the attempted_amount_msat in the payment_external_info sync table.

@dangeross why do we need to sync the payment_external_info with pending payment?
I think of pending payments as transient by nature.

Maybe I misunderstood the PR, but it looks like the intention is to store the pending amount in attempted_amount_msat. @dleutenegger ?

@dangeross either this or the migration was added by mistake because I don't see any update to the sync table.
Let's wait for @dleutenegger answer.

@roeierez yes the intention is to persist the attempted_amount_msat. But as @dangeross pointed out here, the actual call to persist just got missed in the persist_new_bolt11_payment_attempt-method

Addressed in 49e5938

dangeross · 2023-11-16T21:22:51Z

libs/sdk-core/src/breez_services.rs

+                },
+            },
+        }])?;
+        Ok(())


Suggested change

Ok(())

self.persister.insert_payment_external_info(

&invoice.payment_hash,

None,

None,

None,

None,

amount_msat,

)?;

Ok(())

Addressed in 49e5938

dangeross · 2023-11-17T08:05:05Z

libs/sdk-core/src/persist/migrations.rs

@@ -531,5 +531,6 @@ pub(crate) fn current_sync_migrations() -> Vec<&'static str> {
        END;
        ",
        "ALTER TABLE payments_external_info ADD COLUMN failed_amount_msat INTEGER;",
+        "ALTER TABLE payments_external_info RENAME COLUMN failed_amount_msat TO attempted_amount_msat;",


Here @roeierez

Yes I saw that but I assumed it is a mistake since I don't see any inserts/updates to this table as part of this PR.

roeierez · 2023-11-17T11:51:45Z

libs/sdk-core/src/breez_services.rs

+            },
+        }])?;
+
+        self.persister.insert_payment_external_info(


Why do we need to add the amount to the sync db? Is there a reason to sync pending payments?

Because the attempted_amount_msat is also used for failed open invoices to display the actual amount that was attempted to be paid.
And I assume, although not a 100% sure, that other signers would be able to sync the pending payment from Greenlight as well and thus need the data to display the accurate amount.

But we already handled failed opened invoices. Why persist for every invoice? For success payments the amount is retrieved correctly.
I see disadvantage for this way of sync where the synced data become larger without any real advantage.

I see your point, but if we don't sync the attempted amount we couldn't display the actual intended amount for pending payments on all remote signers, as amount_msat is only the amount delivered to destination (we could display it on the sending signer if we would just store it locally), which seems problematic to me as this would result it inconsistencies.
The other option I see would be to clean up the synced entry after payment success.

I would like to get your input on this

I see your point, but if we don't sync the attempted amount we couldn't display the actual intended amount for pending payments on all remote signers, as amount_msat is only the amount delivered to destination (we could display it on the sending signer if we would just store it locally), which seems problematic to me as this would result it inconsistencies. The other option I see would be to clean up the synced entry after payment success.

I would like to get your input on this

The signer is synced with the channel state from greenlight not using our own sync db mechanism to it is irrelevant to the signer sync.
The sdk itself using the sync db to synchronize but it doesn't do that in realtime and we don't have a way to notify remote sdk instances to sync so I don't see how adding it to the sync db contributes here.
In addition immediately after the send_payment is called remote sdk that will sync (actually call list_pays behind the scene) will receive the pending payment as pending so that will should your concern as well.

Do I understand this correctly; Other signers might get the pending payment (list_pays) before they sync the remote DB?

Do I understand this correctly; Other signers might get the pending payment (list_pays) before they sync the remote DB?

In short Yes, exactly.
Let me elaborate a bit on the way we sync data across sdk instances which should make it even more clear:
there are two ways we synchronize:

Sync remote data that exists on greenlight (payments, invoices, channels, etc...) this data comes from greenlight and is fetched by simple calling greenlight gRpc API. All of it is encapsulated inside the sync method.

Sync remote data that doesn't exist in greenlight and only the sdk "knows" about it (lnurl metadata, swaps, etc...) for this kind of data we use the sync.db and whenever such data changes locally the sdk ensures it is uploaded to the greenlight storage (using internal backup API) so other sdk instances can pull it.

The pending payments are related to the first kind of sync (greenlight data) so in theory we don't need to do anything about it as if one sdk instance pays, other instances will receive it as soon as they call sync.
Your requirement was a local requirement as far as I understand, to be able to see the pending payment immediately after calling send_payment. For that purpose only adding locally is needed and no need to add it to the sync.db as well.
Hope that makes sense.

We came up with the following solution proposal:
Adding a new table similar to the payments_external_info-table but stored locally, which stores the attempted_msat amount. Which will be cleaned up once the payment failed/succeeded.

Storing it within Payments doesn't seem to work from our perspective as pending payments get deleted and replaced once a sync occurs.

WDYT @roeierez

roeierez · 2023-11-21T13:56:36Z

@dleutenegger could you please rebase it on top of master, currently there are some conflicts.

roeierez

LGTM

roeierez · 2023-11-21T15:30:04Z

Will wait for @dangeross to finish his review before merging.

dangeross

Looks good!

dangeross requested changes Nov 16, 2023

View reviewed changes

dangeross reviewed Nov 17, 2023

View reviewed changes

dleutenegger force-pushed the fix/pending-payment-amount branch 3 times, most recently from c7942fd to 638ff60 Compare November 17, 2023 10:58

dleutenegger requested a review from dangeross November 17, 2023 10:58

dleutenegger force-pushed the fix/pending-payment-amount branch from 638ff60 to b700555 Compare November 17, 2023 11:49

roeierez requested changes Nov 17, 2023

View reviewed changes

dleutenegger requested a review from roeierez November 17, 2023 16:18

dleutenegger force-pushed the fix/pending-payment-amount branch 2 times, most recently from 8b6562b to 2b5e43c Compare November 21, 2023 10:07

Persist pending payment amount

9367191

dleutenegger force-pushed the fix/pending-payment-amount branch from 2b5e43c to 9367191 Compare November 21, 2023 15:01

roeierez approved these changes Nov 21, 2023

View reviewed changes

dangeross approved these changes Nov 21, 2023

View reviewed changes

roeierez merged commit 5fc10f3 into breez:main Nov 22, 2023
7 checks passed

dleutenegger deleted the fix/pending-payment-amount branch November 22, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist pending payment amount #623

Persist pending payment amount #623

dleutenegger commented Nov 16, 2023

dangeross left a comment

dangeross Nov 16, 2023

roeierez Nov 17, 2023 •

edited

Loading

dangeross Nov 17, 2023

roeierez Nov 17, 2023

dleutenegger Nov 17, 2023 •

edited

Loading

dleutenegger Nov 17, 2023

dangeross Nov 16, 2023

dleutenegger Nov 17, 2023

dangeross Nov 17, 2023

roeierez Nov 17, 2023

roeierez Nov 17, 2023

dleutenegger Nov 17, 2023 •

edited

Loading

roeierez Nov 17, 2023

dleutenegger Nov 20, 2023

roeierez Nov 20, 2023

dleutenegger Nov 20, 2023

roeierez Nov 20, 2023

dleutenegger Nov 20, 2023

roeierez commented Nov 21, 2023

roeierez left a comment

roeierez commented Nov 21, 2023

dangeross left a comment

Persist pending payment amount #623

Persist pending payment amount #623

Conversation

dleutenegger commented Nov 16, 2023

dangeross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeierez Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleutenegger Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleutenegger Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeierez commented Nov 21, 2023

roeierez left a comment

Choose a reason for hiding this comment

roeierez commented Nov 21, 2023

dangeross left a comment

Choose a reason for hiding this comment

roeierez Nov 17, 2023 •

edited

Loading

dleutenegger Nov 17, 2023 •

edited

Loading

dleutenegger Nov 17, 2023 •

edited

Loading