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

rpc: try to catch EOF error from lnd sendpayment #1217

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Nov 26, 2024

A user observed that when using Python, they get an EOF even if the payment succeeds. My guess is that lnd has hung up, then we try to recv and get an EOF error. With this PR, we try to catch that EOF error and just return nil.

One other idea: do we need to call CloseSend at the very end or in a defer?

Fixes: #1216

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12038637695

Details

  • 0 of 6 (0.0%) changed or added relevant lines in 1 file are covered.
  • 32 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.001%) to 40.846%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
tapchannel/aux_leaf_signer.go 3 36.31%
asset/mock.go 3 92.2%
tapgarden/caretaker.go 4 68.5%
rpcserver.go 6 0.0%
asset/asset.go 6 80.96%
universe/interface.go 10 52.81%
Totals Coverage Status
Change from base Build 12038519798: -0.001%
Covered Lines: 25714
Relevant Lines: 62954

💛 - Coveralls

@ZZiigguurraatt
Copy link

Not sure if I'm doing something wrong, but I'm getting this error.

unknown flag `taproot-assets.experimental.rfq.priceoracleaddress'
could not load config: error parsing flags: unknown flag `taproot-assets.experimental.rfq.priceoracleaddress'

@ZZiigguurraatt
Copy link

Has taproot-assets.experimental.rfq.priceoracleaddress been depreciated for something else?

@guggero
Copy link
Member

guggero commented Nov 27, 2024

Are you running tapd directly? Then you need to remove the taproot-assets prefix as that's only valid in litd.

@ZZiigguurraatt
Copy link

I'm running litd. I didn't think you could use sendpayment with tapd directly.

@guggero
Copy link
Member

guggero commented Nov 27, 2024

you could use sendpayment with tapd directly

Yeah, you can't. Just trying to find out what's wrong here... Is it possible you have an old litd somewhere in your $PATH that's being picked up?
The flag still exists on the update-to-lnd-18-4 branch (but it probably doesn't on master):

litd --help | grep oracle
      --taproot-assets.experimental.rfq.priceoracleaddress=                    Price oracle gRPC server address (rfqrpc://<hostname>:<port>). To use the
                                                                               use_mock_price_oracle_service_promise_to_not_use_on_mainnet

@ZZiigguurraatt
Copy link

you could use sendpayment with tapd directly

Yeah, you can't. Just trying to find out what's wrong here... Is it possible you have an old litd somewhere in your $PATH that's being picked up? The flag still exists on the update-to-lnd-18-4 branch (but it probably doesn't on master):

litd --help | grep oracle
      --taproot-assets.experimental.rfq.priceoracleaddress=                    Price oracle gRPC server address (rfqrpc://<hostname>:<port>). To use the
                                                                               use_mock_price_oracle_service_promise_to_not_use_on_mainnet

I was running inside docker, so I don't think that was the issue.

@Roasbeef
Copy link
Member Author

Updated the PR body with more context.

@ZZiigguurraatt
Copy link

Sorry, I accidentally was not using dev.Dockerfile, so it was just pulling from master instead of using my locally checked out branch.

I can confirm this fix works!

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! LGTM 🎉

@@ -7193,7 +7193,12 @@ func (r *rpcServer) SendPayment(req *tchrpc.SendPaymentRequest,

update, err := updateStream.Recv()
if err != nil {
return err
// Stream is closed; no more updates.
if err == io.EOF {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Use errors.Is() instead? Though it shouldn't really be wrapped at this point, so doesn't matter too much.

@Roasbeef Roasbeef merged commit 651ab88 into main Dec 4, 2024
17 of 18 checks passed
@guggero guggero deleted the sendpayment-eof branch December 4, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]: SendPayment fails with EOF error after successful payment
4 participants