lndk-org / lndk

MIT License
84 stars 22 forks source link

multi: config timeout receiving invoice #122

Closed a-mpch closed 3 months ago

a-mpch commented 5 months ago

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 commented 5 months ago

rebased πŸ™πŸΌ

codecov[bot] commented 5 months ago

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 :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #122 +/- ## ====================================== Coverage 0.00% 0.00% ====================================== Files 1 1 Lines 96 96 ====================================== Misses 96 96 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

a-mpch commented 4 months ago

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

mrfelton commented 4 months ago

NOTE: When https://github.com/lndk-org/lndk/pull/131 is merged, this should be extended to cover the new GetInvoice endpoint.

mrfelton commented 3 months ago

I pushed a rebased and updated version of this PR in https://github.com/lndk-org/lndk/pull/148. Have not done much in the way of testing yet tho.

a-mpch commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

orbitalturtle commented 3 months ago

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

@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 commented 3 months ago

@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.