Repository navigation
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
cee3056 to
6bdeb5b
Compare
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Just some minor comments. Otherwise, looks good.
| match &info.kind { | ||
| PaymentKind::Bolt11 { .. } => { | ||
| let update = PaymentDetailsUpdate { | ||
| amount_msat: Some(Some(amount_msat)), |
There was a problem hiding this comment.
This means records with status failed that were claimable now transition from holding gross amount in v0.7 to holding the net amount in this commit. So we would have some discrepancy in the failed status records depending on whether they were written by 0.7, or by this commit.
The challenge is that not all v0.7 records hold the gross amount. If the record went pending -> failed, it holds the gross amount, but if the record went pending -> succeeded -> failed (for example when rejecting a duplicate payment, see commit a176d51), it holds the net amount that was applied at payment_claimed on the succeeded record.
There was a problem hiding this comment.
Yes. It was a mistake to update the field from the invoice amount to the actually claimed amount (i.e., shifting semantics). But I don't think there's anything we can do about it rather than make sure going forward we're consistently reporting net amounts everywhere? Anything else we could fix in your opinion?
| (OldStatus::Pending, Some(100_000), None, Some(100_000)), | ||
| (OldStatus::Pending, None, Some(1_000), None), | ||
| (OldStatus::Succeeded, Some(99_000), Some(1_000), Some(99_000)), | ||
| (OldStatus::Failed, Some(100_000), Some(1_000), Some(100_000)), |
There was a problem hiding this comment.
following from above, records written by this commit would have expected = 99_000.
There was a problem hiding this comment.
Right, newly received payments would hold 99_000 here. See above, not sure what else we can do to correct the historic mistake.
| ); | ||
| for _ in 0..3 { | ||
| let encoded = payment.encode(); | ||
| let raw = <OldDetails as ldk_node_070::lightning::util::ser::Readable>::read( |
There was a problem hiding this comment.
To clarify does this mean we want to allow downgrades from 0.8 to 0.7 ?
There was a problem hiding this comment.
We def. aim to eventually allow downgrades, but for 0.8 that ship has sailed as we have too many non-forwards compatible changes. But this line also only checks backwards compatibility.
There was a problem hiding this comment.
Spoke offline, this part ofc indeed tests the whole roundtrip, i.e., also downgrades.
Pending BOLT11 payments included withheld LSP fees, unlike completed payments and BOLT12 receipts. This made the reported amount change when a payment was claimed and required callers to account for its state. Report the received amount when a payment becomes claimable and retain the historical pending-payment encoding for v0.7 upgrades. Translate fees at the serialization boundary so restarts do not deduct them twice, and refresh migrated invoice amounts when another payment event arrives. Co-Authored-By: HAL 9000
Legacy pending BOLT11 payments can record fees above their invoice amount, preventing the node from reading its payment store. An unrepresentable amount plus fee also makes persistence panic. Clamp the amount conversions at the serialization boundary so these records remain readable and writable. Keep ordinary amounts exact and verify that clamped records remain stable across restarts. Co-Authored-By: HAL 9000
Manual claims asked applications to repeat an amount already reported in PaymentClaimable. This made the validation ambiguous without expressing the application's own expectations for the payment. Claim using only the payment ID and preimage. Require applications to validate the event amount before every approval, including payments for invoices created before an upgrade. Fixes lightningdevkit#1091 Co-Authored-By: HAL 9000
6bdeb5b to
8d9f793
Compare
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
Agreed not much we can do, perhaps this ambiguity is worth noting somewhere ?
4 participants
Here we fix two minor issues that were introduced in #948:
a) We now consistently (BOLT11 and BOLT12) report net amounts (not including
counterparty_skimmed_fee) inPaymentDetails::amount_msat. For pending BOLT11 payments that requires some migration step on read/write to maintain backwards compatibility (pulling out a commit that we had already included in #999, but makes sense to land it independently).b) We simplify the manual BOLT11 claiming logic, addressing #1091.