Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

valentinewallace · 2024-09-12T19:28:57Z

This fixes the following bug:
- An outbound payment is AwaitingInvoice
- We receive an invoice and lock the HTLCs into the relevant ChannelMonitors
- The monitors are successfully persisted, but the ChannelManager fails to
  persist, so the outbound payment remains AwaitingInvoice
- We restart, causing the channel to close due to a stale ChannelManager
- We receive a duplicate invoice, and attempt to pay it again due to the
  payment still being AwaitingInvoice in the stale ChannelManager

After the fix for this, we will notice that the payment is already locked into
the monitor on startup and transition the incorrectly-AwaitingInvoice payment
to Retryable, which prevents double-paying on duplicate invoice receipt.

Caught by @TheBlueMatt: #3140 (comment).

lightning/src/ln/outbound_payment.rs

lightning/src/ln/offers_tests.rs

codecov · 2024-09-12T21:44:59Z

Codecov Report

Attention: Patch coverage is 95.09804% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (6662c5c) to head (fbb3ab2).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	78.94%	2 Missing and 2 partials ⚠️
lightning/src/ln/offers_tests.rs	98.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3313      +/-   ##
==========================================
- Coverage   89.65%   89.62%   -0.03%     
==========================================
  Files         126      126              
  Lines      102546   102622      +76     
  Branches   102546   102622      +76     
==========================================
+ Hits        91935    91975      +40     
- Misses       7890     7924      +34     
- Partials     2721     2723       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

LGTM

lightning/src/ln/outbound_payment.rs

TheBlueMatt

Thanks, yea, I think this looks right, though it'd be nice to include a test that sent the payment MPP.

valentinewallace · 2024-09-16T17:26:47Z

Thanks, yea, I think this looks right, though it'd be nice to include a test that sent the payment MPP.

Adapted the test to use MPP, as a fixup in case you meant a separate test.

Move the code that ensures that HTLCs locked into ChannelMonitors are synchronized with the ChannelManager's OutboundPayments store to the outbound_payments module. This is useful both because ChannelManager::read is very long/confusing method, so it's nice to encapsulate some of its functionality, and because we need to fix an existing bug in this logic where we may risk double-paying an offer due to outbound_payments being stale on startup. See the next commit for this bugfix.

valentinewallace · 2024-09-16T17:40:00Z

Rebased due to silent conflict.

TheBlueMatt · 2024-09-16T20:04:00Z

LGTM, feel free to squash IMO.

valentinewallace · 2024-09-16T20:16:45Z

Squashed.

jkczyz

LGTM modulo any CI failures. Can't tell from the UI.

This fixes the following bug: - An outbound payment is AwaitingInvoice - We receive an invoice and lock the HTLCs into the relevant ChannelMonitors - The monitors are successfully persisted, but the ChannelManager fails to persist, so the outbound payment remains AwaitingInvoice - We restart, causing the channels to close due to a stale ChannelManager - We receive a duplicate invoice, and attempt to pay it again due to the payment still being AwaitingInvoice in the stale ChannelManager After the fix for this, we will notice that the payment is already locked into the monitor on startup and transition the incorrectly-AwaitingInvoice payment to Retryable, which prevents double-paying on duplicate invoice receipt.

valentinewallace · 2024-09-17T13:56:40Z

Kicked CI 🤞

TheBlueMatt

LGTM but the test needs to make sure we actually get PaymentSent events, which is the other half of not double-paying, but can happen in a followup.

TheBlueMatt · 2024-09-17T14:28:39Z

lightning/src/ln/offers_tests.rs

+ // attempt would have resulted in a PaymentFailed event here, since the only channels between
+ // Alice and Bob is closed. Since no 2nd attempt should be made, check that no events are
+ // generated in response to the duplicate invoice.
+ assert!(nodes[0].node.get_and_clear_pending_events().is_empty());


We should follow through, claiming the HTLCs on-chain and making sure we get a PaymentSent event.

jkczyz self-requested a review September 12, 2024 19:41

jkczyz reviewed Sep 12, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/ln/offers_tests.rs Outdated Show resolved Hide resolved

jkczyz reviewed Sep 13, 2024

View reviewed changes

valentinewallace commented Sep 13, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Sep 16, 2024

View reviewed changes

valentinewallace force-pushed the 2024-09-fix-offer-double-pay branch from 19dc035 to cd5709f Compare September 16, 2024 17:00

valentinewallace added 2 commits September 16, 2024 13:34

Test utils: use full import path in reload_node macro.

8c356d8

valentinewallace force-pushed the 2024-09-fix-offer-double-pay branch from cd5709f to 0c2c339 Compare September 16, 2024 17:39

valentinewallace force-pushed the 2024-09-fix-offer-double-pay branch from 2f1542e to ffe936b Compare September 16, 2024 20:16

jkczyz approved these changes Sep 16, 2024

View reviewed changes

valentinewallace force-pushed the 2024-09-fix-offer-double-pay branch from ffe936b to fbb3ab2 Compare September 17, 2024 13:53

TheBlueMatt approved these changes Sep 17, 2024

View reviewed changes

TheBlueMatt merged commit 866cedf into lightningdevkit:main Sep 17, 2024
17 of 19 checks passed

TheBlueMatt mentioned this pull request Sep 17, 2024

#3313 test should be expanded #3320

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

valentinewallace commented Sep 12, 2024

codecov bot commented Sep 12, 2024 •

edited

Loading

jkczyz left a comment

TheBlueMatt left a comment

valentinewallace commented Sep 16, 2024

valentinewallace commented Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

valentinewallace commented Sep 16, 2024

jkczyz left a comment

valentinewallace commented Sep 17, 2024

TheBlueMatt left a comment •

edited

Loading

TheBlueMatt Sep 17, 2024

Don't pay a duplicate BOLT 12 invoice if ChannelManager is stale #3313

Don't pay a duplicate BOLT 12 invoice if ChannelManager is stale #3313

Conversation

valentinewallace commented Sep 12, 2024

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Sep 16, 2024

valentinewallace commented Sep 16, 2024

TheBlueMatt commented Sep 16, 2024

valentinewallace commented Sep 16, 2024

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace commented Sep 17, 2024

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Sep 17, 2024

Choose a reason for hiding this comment

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

codecov bot commented Sep 12, 2024 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading