lightning-terminal icon indicating copy to clipboard operation
lightning-terminal copied to clipboard

Add support for custodial off-chain accounts

Open guggero opened this issue 3 years ago • 6 comments

Early preview of the LiT custodial off-chain accounts feature.

~Depends on #308.~ Depends on #432.

EDIT: All TODOs have been resolved :tada:

guggero avatar May 12 '22 13:05 guggero

This still has a bunch of TODOs and things that can be improved, that's why it's still in draft. But would be nice if you could take a first look at the general architecture, @ellemouton and @Roasbeef.

guggero avatar May 20 '22 10:05 guggero

yep! currently doing so :)

ellemouton avatar May 20 '22 10:05 ellemouton

Taking this out of WIP state since this is now a bit better tested and also received its own set of integration tests!

The following TODOs are still in place:

  • add unit tests
  • take in-flight payments into account when calculating remaining account balance
  • add support for creating a custom "account" LNC session by adding custom caveats to the AddSession RPC.

guggero avatar Jun 21 '22 11:06 guggero

@guggero, remember to re-request review from reviewers when ready

lightninglabs-deploy avatar Aug 05 '22 09:08 lightninglabs-deploy

!lightninglabs-deploy mute

guggero avatar Aug 05 '22 09:08 guggero

Yayy, I was finally able to resolve all TODOs :rocket: This PR is now fully ready for final reviews.

guggero avatar Oct 17 '22 15:10 guggero

Addressed all comments, should be ready for a final look.

guggero avatar Nov 17 '22 15:11 guggero

Thanks a lot for the review and for testing the happy path :rocket: Happy to hear you find this useful.

Does the rpc interceptor get re-registered and the TrackPayment subscriptions re-established along with it?

Yes, on every startup we re-initialize the payment tracking for each payment and invoice update stream.

So this should be pretty robust. Though in general "lnd instance that comes online & offline unpredictably" is never recommended in a production environment. Or are you thinking about a mobile setup? Curious about your use case!

Going to address your nits/comments inline.

guggero avatar Nov 28 '22 09:11 guggero

Yes, on every startup we re-initialize the payment tracking for each payment and invoice update stream.

So this should be pretty robust. Though in general "lnd instance that comes online & offline unpredictably" is never recommended in a production environment. Or are you thinking about a mobile setup? Curious about your use case!

Gotcha, I think one thing I was overlooking was that (if I'm not mistaken) litd will stop when it sees that lnd has stopped. So this Start function will be called every time either litd or lnd restart. My concern was only that lnd goes down, all subscriptions are lost, and then when it comes back up we don't re-establish the streams.

I wasn't thinking about a mobile setup or anything like that, just about what might happen if there are unexpected outages in an otherwise stable environment.

sangaman avatar Nov 28 '22 13:11 sangaman

litd will stop when it sees that lnd has stopped

Ah yes, that is important to know in this context. litd is indeed set up to stop (and be re-started by systemd, docker, k8s, whatever) when the connection to lnd is lost.

guggero avatar Nov 28 '22 14:11 guggero

My last handful of thoughts about this PR & feature:

  1. It might be nice to have an RPC call that increments an account's balance. That way there's an easy way to, for example, add 1000 satoshis to an account. Or possibly decrement balance (by passing a negative value), but then there'd need to be the checks for sufficient balance on the account. Right now, you can update an account's balance via RPC, but that's only good if you're trying to set the balance to a specific amount, not add or subtract from the existing balance. Trying to do that by querying the balance and then updating the account balance would be subject to race conditions (maybe the balance updates in between the two calls). I figure this can be done in a follow-up PR, though.

  2. It'd be nice if self-payments from one account to another were better supported. Right now, it seems like it does work if I use the --allow_self_payment flag, which then sends a payment out one channel and back in on the same channel. I think it'd be cool to be able to make an account-to-account payment without actually needing to make a real lightning payment (which may incur a fee, delay, and/or chance of failure). But I can imagine that would be a complicated change since it would likely break some internal assumptions in lnd about payments, namely that they actually involve the external lightning network. Just something to think about for future work.

sangaman avatar Nov 28 '22 18:11 sangaman

My last handful of thoughts about this PR & feature:

Thanks a lot for your feedback! I think both of these features make sense for follow-up PRs (given the added complexity for item 2 and the age of this PR, this has been in the works since late 2018 in some form or another). Feel free to add feature request issues to the repo so they don't drop off the radar.

guggero avatar Nov 29 '22 11:11 guggero