Faster feedback failure receiving invoice (updated)

[mrfelton]

Updated version of #122 that has been rebased ontop of latest codebase.

Additionally, it adds:

• validation of the invoice timeout startup param on startup
• support for passing invoice response timeout to get-invoice rpc
• Refactors to use Default instead of custom new_default method, as per pr comment

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (6459123) to head (856eb68).

Files	Patch %	Lines
src/main.rs	0.00%	9 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #148   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines          97     105    +8     
======================================
- Misses         97     105    +8     

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

[orbitalturtle]

Thanks for this! Tested it and it seems to be working well. A few nits below and I think it would be good to squash the three commits into one since the second and third commit are reverting changes that were made in the first commit.

As I'm playing around with LNDK, I'm also starting to feel like 30 seconds is maybe still too long for the timeout. Maybe 15 seconds instead. Any thoughts there?

[mrfelton]

Addressed all comments in squashed commit 25e9378

As I'm playing around with LNDK, I'm also starting to feel like 30 seconds is maybe still too long for the timeout. Maybe 15 seconds instead. Any thoughts there?

30 seconds is definitely too long in the context of making a payment in the real world. But I don't know what a reasonable expectation is in the context of onion messaging realities. But I've reduced it to 15 seconds in this PR.

I also notice that we have a separate hardcoded 20 second timeout in our connect peer calls, so in reality attempting to fetch an invoice could take 20+15 seconds before a timeout happens.

[orbitalturtle]

Just one more nit and then good to go

[orbitalturtle]

@mrfelton Just needs a cargo fmt!

I also notice that we have a separate hardcoded 20 second timeout in our connect peer calls, so in reality attempting to fetch an invoice could take 20+15 seconds before a timeout happens.

Actually if we hit the connect_peer timeout, the payment should just fail and not reach the 15s timeout. But it definitely might be worth adding another timeout config option if people need it eventually.

[mrfelton]

re-pushed with format

[mrfelton]

Actually if we hit the connect_peer timeout, the payment should just fail and not reach the 15s timeout. But it definitely might be worth adding another timeout config option if people need it eventually.

I would think it more rare that connecting to a peer would take longer than a second or two. It's the onion message exchange that seems to take time. Possibly what we need more than configurable timeouts on the peer connect would be fallbacks to connecting to other peers if we can't connect directly to the first one we attempt.

[orbitalturtle]

Possibly what we need more than configurable timeouts on the peer connect would be fallbacks to connecting to other peers if we can't connect directly to the first one we attempt.

Yeah good point! If an offer includes more than one blinded path option, we should loop through the other options in the case of a connection failure. This is relevant to a PR I'm working on now so I'll try to include it.