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

feat: track payment on cln nodes #75

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

okjodom
Copy link
Collaborator

@okjodom okjodom commented Aug 23, 2023

Polls lightning-listpays until payment completes or fails

@okjodom okjodom force-pushed the cln-track-payment branch from 256e64b to 0c9b464 Compare August 24, 2023 14:24
@okjodom okjodom changed the title feat: tack payment on cln nodes feat: track payment on cln nodes Aug 24, 2023
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

🏄

}
}

time::sleep(Duration::from_millis(100)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also select on sleep + shutdown here? That lets us exit a little faster if we shut down while tracking the payment.

If we do this we can probably bump it up to polling once a second so that we don't hammer CLN too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

select kind of fun

Copy link
Member

Choose a reason for hiding this comment

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

Aren't we sleeping twice now?

Feels like this sleep should be removed and the duration of the sleep in select should be reduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this sleep should be removed and the duration of the sleep in select should be reduced.

Yeah, that's what I was thinking. If we have both in the select we'll exit when we need, otherwise wait so this line can be removed!

Copy link
Collaborator Author

@okjodom okjodom Aug 28, 2023

Choose a reason for hiding this comment

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

fixed! I was inattentive

sim-lib/src/cln.rs Outdated Show resolved Hide resolved
@okjodom okjodom force-pushed the cln-track-payment branch from 0c9b464 to 4b9dd98 Compare August 25, 2023 17:06
}
}

time::sleep(Duration::from_millis(100)).await;
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we sleeping twice now?

Feels like this sleep should be removed and the duration of the sleep in select should be reduced.

sim-lib/src/cln.rs Outdated Show resolved Hide resolved
tokio::select! {
biased;
_ = shutdown.clone() => {
break Err(LightningError::TrackPaymentError("Shutdown before tracking results".to_string()));
Copy link
Member

Choose a reason for hiding this comment

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

Same here, replace by return

Copy link
Member

Choose a reason for hiding this comment

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

I also think it is worth considering that, if a shutdown signal is raised, we may want to wait until the next tick before stopping the simulation.

This would certainly need a followup, but I think it is worth we discuss it (and we open an issue for it if it makes sense)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wait until the next tick before stopping the simulation

would that be a tick in the current track payment task, i.e 1 second later? how would we "harmonize waiting" for all the other timers and timeouts?

Copy link
Member

Choose a reason for hiding this comment

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

What I had in mind is, when a shutdown is triggered:

  • First, you stop the producers, so no new payments are passed down the pipeline
  • Then you wait for whatever we feel is reasonable for inflight payments to complete

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

concept ack. I created a tracking issue here: #82

let payment_outcome = match payment_status {
ListpaysPaysStatus::Pending => continue,
ListpaysPaysStatus::Complete => PaymentOutcome::Success,
// Task: https://github.com/bitcoin-dev-project/sim-ln/issues/26#issuecomment-1691780018
Copy link
Member

Choose a reason for hiding this comment

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

I'll look into this.

sim-lib/src/cln.rs Outdated Show resolved Hide resolved
@okjodom okjodom force-pushed the cln-track-payment branch from 4b9dd98 to 516a52d Compare August 28, 2023 08:19
@okjodom okjodom force-pushed the cln-track-payment branch from 516a52d to c27b8f6 Compare August 28, 2023 15:53
@carlaKC
Copy link
Contributor

carlaKC commented Aug 31, 2023

Looks good! Going to find some time to test this in the wild this week, should be good to go after that 🥇

@sr-gi
Copy link
Member

sr-gi commented Aug 31, 2023

LGTM too. There is not much we can do regarding tracking without polling atm, and there is an open issue at CLN for the error reporting in list_pays on payment failure

@carlaKC
Copy link
Contributor

carlaKC commented Sep 5, 2023

Did ya'll run CLN with anything other than --grpc-port to get the grpc plugin firing? Trying to test this now and struggling to connect :(

Update: fml, forgot about https in config, what a joyful use of 3 hours.

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

tACK! Sorry this took me so long to get to.

Tested LND <-> CLN keysend payments, everything as expected + tracked in results.

We do however need to take another look at error handling for sending CLN payments, when I tried to test out failures (just closed the only channel) I get an error that shuts down the simulation:

ERROR [sim_lib] Error while sending payment 0366dc9e42d6cb2a89d3ba9a347a0995c88e62949618db6f62bd86fd1da0364868 
-> 02c0c6b8be1a367c2fce4efdd9ea075b1e2b15d2566c6eab845513a88e313ee0ab. 

Terminating consumer. Send payment error status: 
Unknown, message: "Error calling method KeySend: RpcError { 
code: Some(210), message: \"Ran out of routes to try after 1 attempt: see `paystatus`\" }", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Mon, 11 Sep 2023 15:48:33 GMT", "content-length": "0"} 
}

Created #96 to track!

@carlaKC carlaKC merged commit c29abd9 into bitcoin-dev-project:main Sep 11, 2023
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.

3 participants