Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Retry InvoiceRequest Flow #3010

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented Apr 22, 2024

resolves #2836

Description:

  • Add functionality to handle retrying the sending of invoice_request messages on new reply_paths that are still awaiting invoices.

Changes:

  1. Introduced invoice_request as an optional field in the PendingOutboundPayments::AwaitingInvoice variant to accommodate instances without invoice requests.
  2. Refactored logic from pay_for_offer to create invoice request messages into a separate function for reuse with retry message flow.
  3. Implemented the retry_tick_occurred function in ChannelManager to handle generating invoice request messages for AwaitingInvoice payments and enqueueing them.
  4. Added retry_tick_occurred to ln_background_processor with a timer duration of 5 seconds for timely retries without overwhelming the system with too many onion messages.

@shaavan
Copy link
Contributor Author

shaavan commented Apr 26, 2024

Updated from pr3010.01 to pr3010.02 (diff):

Changes:

  1. Introduce Readable impl for InvoiceRequest
  2. Restructure commits.

@shaavan
Copy link
Contributor Author

shaavan commented Apr 27, 2024

Updated from pr3010.02 to pr3010.03 (diff):

Updates:

  1. Rebase on main, to resolve merge conflicts.
  2. Fix ci

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2024

Codecov Report

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

Project coverage is 89.14%. Comparing base (bd3cc00) to head (9cc86bc).

❗ Current head 9cc86bc differs from pull request most recent head dce32e7. Consider uploading reports for the commit dce32e7 to get more accurate results

Files Patch % Lines
lightning/src/offers/invoice_request.rs 50.00% 0 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs 92.85% 1 Missing ⚠️
lightning/src/ln/offers_tests.rs 95.00% 1 Missing ⚠️
lightning/src/ln/outbound_payment.rs 95.83% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3010      +/-   ##
==========================================
- Coverage   89.17%   89.14%   -0.04%     
==========================================
  Files         118      118              
  Lines       97986    97783     -203     
  Branches    97986    97783     -203     
==========================================
- Hits        87376    87165     -211     
+ Misses       8419     8371      -48     
- Partials     2191     2247      +56     

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

@shaavan
Copy link
Contributor Author

shaavan commented Apr 29, 2024

Updated from pr3010.03 to pr3010.04 (diff):

Changes:

  1. Introduce retry_tick_occurred test.

@shaavan shaavan changed the title Introduce Retry Invoice Flow Introduce Retry InvoiceRequest Flow Apr 29, 2024
@shaavan
Copy link
Contributor Author

shaavan commented May 3, 2024

Updated from pr3010.04 to pr3010.05 (diff):

Addressed @TheBlueMatt comments

Updates:

  1. Removed the secondary timer in ln_background_processor.
  2. Moved the retry_tick_occurred to a private function, and renamed it to retry_invoice_request_messages.
  3. Decrease the FRESHNESS_TIMER duration, while introducing a call counter within timer_tick_occurred. This is done so that retry_invoice_request_messages can be called faster than the rest of the code without introducing a secondary timer.
  4. Update the test appropriately.

@shaavan
Copy link
Contributor Author

shaavan commented May 3, 2024

Updated from pr3010.05 to pr3010.06 (diff):

Update:

  1. Rebase on main.
  2. To keep the code consistent with the main, now the create_invoice_reqeust_message, and retry_invoice_request_messages functions return Result<(), Bolt12SemanticError>

@shaavan
Copy link
Contributor Author

shaavan commented May 3, 2024

Updated from pr3010.06 to pr3010.07 (diff):

Update:

  1. Replaced using unsafe with using internal Mutex variable for tracking call counts.

This will be used to recreate InvoiceRequest messages for retries for
the payments that are still awaiting receiving an invoice.
We need to retry InvoiceRequest messages in a smaller time duration
than timer_tick_occurred. This function provides the base for doing
the retry, by getting the InvoiceRequest for PendingOutboundPayments
and using a new reply_path to create and enqueue InvoiceRequest messages.
1. And introduce static CALL_COUNTER, to trigger the `timer_tick_occurred`
   code only every 10th call.
2. Decrease Freshness timer duration.
3. This is done so that `retry_invoice_request_messages` can be called faster
   than the rest of the code without introducing a secondary timer.
@shaavan
Copy link
Contributor Author

shaavan commented May 13, 2024

Updated from pr3010.07 to pr3010.08 (diff):

Changes:

  1. Rebase on main, to solve merge conflicts.

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented May 13, 2024

I don't buy that we need a new counter. There's a lot of things that hang on the timer rate, but this is a great opportunity to redefine the constants in seconds and multiply out a rate. Looking at all the things we do in the timer tick, as far as I can tell the only one that doesn't have a counter and a limit constant already is timer_check_closing_negotiation_process, and it should be easy to add there.

@shaavan
Copy link
Contributor Author

shaavan commented May 14, 2024

Hi Matt!
Thanks a lot for the suggestion!

I think this will definitely be a more maintainable way to handle this change than introducing a counter!
Worked out your suggestion in a separate PR since they were quite different from this one: #3065
Let me know if the approach seems sound!

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry InvoiceRequest messages
3 participants