multi: config timeout receiving invoice

[a-mpch]

This pr solves #120

We could separate this PR in 3 parts regarding the timeout when waiting for a invoice from the offer creator:

1. Server configuration: adds a config parameter that we can use to change the default timeout of the server. If is not present default value is 30s.
2. Timeout parameter grpc: add a new parameter in grpc pay_offer to setup a custom timeout for the specific payment.
3. Timeout in CLI: adds a optional flag on cli, so we could use the parameter from grpc.

How can reproduce?

Leaving lndk on, disconnect a connect a peer in lnd (my case eclair). Usually it takes a while to setup the network, so it will return the error

Examples

With flag response invoice amount

lndk-cli -m <MACAROON> pay-offer lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrc2zfjx7mnpw35k7m3qw3hjqetrd3skjuskyypxyevamydhychq9cglpsg2tx6tkc60lpcmtxckgnmzrn832hwrkws 10 --response-invoice-timeout 5

Error paying for offer: Status { code: Internal, message: "Internal error: Did not receive invoice in 5 seconds.", metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "-", "content-length": "0"} }, source: None }

Without flag

lndk-cli -m <MACAROON> pay-offer lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrc2zfjx7mnpw35k7m3qw3hjqetrd3skjuskyypxyevamydhychq9cglpsg2tx6tkc60lpcmtxckgnmzrn832hwrkws 10

Error paying for offer: Status { code: Internal, message: "Internal error: Did not receive invoice in 30 seconds.", metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "-", "content-length": "0"} }, source: None }

Open to any comment 🙏🏼

[a-mpch]

rebased 🙏🏼

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (d1c668c) to head (2d9c520).

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

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #122   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines          96      96           
======================================
  Misses         96      96           

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

[orbitalturtle]

Thanks for doing this @mauricepoirrier! 🚀 Just a few small changes:

• Let's move the change from 100 to 30 seconds to a new commit with a commit message that notes that we're changing to 30 seconds because 100 seconds was too high and made testing more difficult.
• Make both commit messages wrap around at 80 characters

[orbitalturtle]

Hmm I think I'd rather remove this new_default function and in impl Default (and wherever else new_default is called) we can just call new(None) instead?

[mrfelton]

I addressed this in e989fbb

[orbitalturtle]

Ah is this to make sure a user doesn't set a timeout of zero? I think it'd be good to do validation of this first -- so before OfferHandler::new is called in main.rs, check that the timeout isn't less than 1. If it is less than one log an error! an exit so the user can then provide a valid timeout

[mrfelton]

I addressed this in 6f18e38

[a-mpch]

I have bandwidth now! I'll do it over the weekend.

[mrfelton]

NOTE: When #131 is merged, this should be extended to cover the new GetInvoice endpoint.

[mrfelton]

I pushed a rebased and updated version of this PR in #148. Have not done much in the way of testing yet tho.

[a-mpch]

@mrfelton thanks for the heads-up. A lot going on. This week I'll re take these.

Let me know what's the next plan on the other PR so I would close this one.

[kannapoix]

It seems lndk still doesn't fail immediately on error. Even when it encounters an error, such as a connection failure with a peer, the CLI continues to wait until it times out.
I understand this PR is only responsible for custom timeout options, but I think it's worth mentioning because the original issue states, An immediate error or a retry mechanism could be more effective.
(I'm not suggesting you should implement this in this PR)

The --response-invoice-timeout option appears to be working well 😀

[a-mpch]

@kannapoix that's a great point! I might check how to do that in a separate PR, probably would mean few more changes.

[orbitalturtle]

Closing this since @mrfelton finished it off with #148. Thank you very much for getting this started @mauricepoirrier ❤️

@kannapoix Yes totally agree, fixing this would be extremely helpful. If either of you would be interested in exploring a fix it'd be very welcome. My understanding is that in most parts of the process, LNDK will fail the payment upon error. But there is one common case where it doesn't: if we've passed the invoice request to the LDK onion messenger and it fails saying it can't find a path -- LNDK should just fail, but instead it goes through the whole timeout. Is this the case you're talking about? This might be a tad bit deceptively tricky to fix but I can outline some thoughts in an issue if it'd be helpful.

[kannapoix]

@orbitalturtle

But there is one common case where it doesn't: if we've passed the invoice request to the LDK onion messenger and it fails saying it can't find a path -- LNDK should just fail, but instead it goes through the whole timeout. Is this the case you're talking about?

I forgot the details, but probably it was the case your are mentioning.
@mauricepoirrier is trying to tackle this, so I will take something different.