Add support for custodial off-chain accounts #363

guggero · 2022-05-12T13:13:34Z

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

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

EDIT: All TODOs have been resolved 🎉

guggero · 2022-05-20T10:21:17Z

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.

ellemouton · 2022-05-20T10:43:34Z

yep! currently doing so :)

ellemouton

still need to do a more in depth pass. But so far this looks amazing 😱 🔥 leaving a few comments so long

accounts/interceptor.go

rpcmiddleware/interface.go

guggero · 2022-06-21T11:31:40Z

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.

rpcmiddleware/interface.go

Roasbeef

Amazing PR!

Rather large, so I did an initial pass through, focusing on internalizing the porposed architecture. To main things popped into my head:

Why not return the macaroon from the account creation/list calls so the caller can store that along side the account ID?
What if we instead stored the latest account details in the macaroon itself? This would make the service more stateless as the balances would be encoded in the macaroon itself. The persistence gets a bit simpler, but then we need to sort of burden the user with storing that new updated macaroon. Also this wouldn't really work for the invoices unfortunately, since we'd want to keep the user's macaroon somewhat compact.

rpcmiddleware/interface.go

Roasbeef · 2022-07-01T00:36:42Z

rpcmiddleware/interface.go

+// request and inspects and potentially modifies the response.
+func NewResponseRewriter(requestSample proto.Message,
+	responseSample proto.Message,
+	typedResponseHandler interface{}) *DefaultChecker {


Perhaps we can dip into a bit of generics usage here? So something like:

type responseHandler[T any] func(T) error

Still getting through the diff, so I don't know if it'll use useful, but just a hunch.

I tried that but failed. Maybe I'll try again after I've completed everything else.

terminal.go

accounts/checkers.go

accounts/store.go

Roasbeef · 2022-07-01T00:57:16Z

accounts/store.go

+// chargeAccount subtracts the given amount from an account's balance.
+func (s *Store) chargeAccount(id AccountID, paymentHash lntypes.Hash,
+	amount lnwire.MilliSatoshi) error {
+


I need to get further in the diff, but in order to prevent race conditions, we may need to actually be holding an internal mutex here. Though I think that things are all serial based on my mental model so far.

Yeah, a lock was definitely missing. Fixed now.

litrpc/lit-accounts.proto

Roasbeef · 2022-07-01T01:00:14Z

litrpc/lit-accounts.proto

+}
+
+message Account {
+    /*


Should we also return the raw macaroon here? Otherwise how will it be obtained?

One architecture I have in mind is:

Service maintains an external DB of the macaroon for each user's accoutn

Each time a user wants to do something, the services get the macaroon then issues the response as normal

So the service just needs to handle the mapping from user to account macaroon.

One other alternative architecture would be the discard model: the account service actually encodes the latest balance in the macaroon itself. This minimizes the state, as its the clients jobs to maintain this macaroon, and they can't modify it w/ breaking the HAMC check.

Added the baking of an account macaroon to the RPC server and updated the docs.

Roasbeef · 2022-07-01T01:03:02Z

doc/accounts.md

+This created a new account (ID ``) with an initial balance of 50k satoshis and
+no expiration.
+
+### Bake a new macaroon for the user


Related to my comment above: what if we did this for the user?

Yeah, great idea. Did that.

ellemouton

tACK 🔥 Leaving some initial comments :)

rpcmiddleware/interface.go

terminal.go

accounts/store.go

accounts/service.go

accounts/interceptor.go

rpcmiddleware/manager.go

accounts/checkers.go

accounts/store.go

lightninglabs-deploy · 2022-08-05T09:15:17Z

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

guggero · 2022-08-05T09:17:02Z

!lightninglabs-deploy mute

guggero · 2022-10-17T15:13:34Z

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

sangaman · 2022-11-28T13:42:57Z

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.

guggero · 2022-11-28T14:49:01Z

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.

sangaman

Thanks for taking the time to respond to my feedback, this is great stuff and will be very useful I think.

sangaman · 2022-11-28T18:07:27Z

My last handful of thoughts about this PR & feature:

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

guggero · 2022-11-29T11:14:55Z

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.

Roasbeef

LGTM 🌜

sangaman · 2022-12-01T23:46:26Z

litrpc/lit-accounts.proto

+    /*
+    The list of invoices created by the account. An invoice created by an
+    account will credit the account balance if it is settled.
+    */
+    repeated AccountInvoice invoices = 6;
+
+    /*
+    The list of payments made by the account. A payment made by an account will
+    debit the account balance if it is settled.
+    */
+    repeated AccountPayment payments = 7;


One last thought when I was looking through this PR, and also probably something that could be addressed in a follow-up PR, but it looks like these include all payments and invoices ever made by the accounts. Therefore when calling ListAccounts, you're fetching all payment and invoice history for all accounts across all time. That seems like it could potentially be a very heavy query.

It might make sense to have a ListAccounts call that just gives you the accounts and balances that exist, and then separate ListInvoices and ListPayments calls that work for a single account specified in the request? Something like that, just so that there's a way to list the accounts without needing to fetch all invoice & payment history.

I can put this in an issue as well, just wanted to raise it here first.

ellemouton

🔥

With this commit we add a set of utility functions that allow us to store and retrieve an account in/from a context.

This commit adds the checkers component that is responsible for making sure all incoming RPC requests and outgoing responses are tracked and their effects on an account are updated accordingly.

This commit introduces the account interceptor service which makes sure that each request that contains an account macaroon is intercepted and passed through the checkers component to track each RPC call's effects on the account.

guggero force-pushed the accounts branch from d7f6ed3 to 6838abf Compare May 17, 2022 12:11

guggero mentioned this pull request May 17, 2022

LSAT for subscription payments? lightninglabs/L402#14

Closed

ellemouton reviewed May 26, 2022

View reviewed changes

accounts/interceptor.go Outdated Show resolved Hide resolved

rpcmiddleware/interface.go Outdated Show resolved Hide resolved

ellemouton mentioned this pull request Jun 17, 2022

multi: add ability to modify requests #380

Closed

guggero force-pushed the accounts branch 2 times, most recently from 88804ed to e4e45cd Compare June 21, 2022 11:30

guggero marked this pull request as ready for review June 21, 2022 11:30

ellemouton reviewed Jun 23, 2022

View reviewed changes

rpcmiddleware/interface.go Outdated Show resolved Hide resolved

ellemouton mentioned this pull request Jun 28, 2022

multi: use the ability to intercept rpc requests and errors #386

Merged

Roasbeef reviewed Jul 1, 2022

View reviewed changes

ellemouton reviewed Jul 29, 2022

View reviewed changes

guggero force-pushed the accounts branch from a0d9858 to 52e91ef Compare August 8, 2022 14:34

guggero force-pushed the accounts branch from 52e91ef to a770a44 Compare August 16, 2022 14:27

This was referenced Sep 14, 2022

multi: rpcmiddleware package #416

Closed

multi: rpcmiddleware package #419

Merged

guggero force-pushed the accounts branch 2 times, most recently from 64ffcac to 4fd1e29 Compare September 22, 2022 14:36

guggero force-pushed the accounts branch 2 times, most recently from 3f18152 to c2a6484 Compare October 13, 2022 20:08

guggero force-pushed the accounts branch from c2a6484 to a5e71c1 Compare October 14, 2022 17:20

guggero mentioned this pull request Oct 15, 2022

litd: maintenance, code cleanup, middleware fixes #432

Merged

guggero force-pushed the accounts branch 3 times, most recently from 10d1a60 to 0f29d10 Compare October 17, 2022 15:12

guggero dismissed stale reviews from sangaman and ellemouton via c01eb05 November 28, 2022 09:57

guggero force-pushed the accounts branch from b6e8f33 to c01eb05 Compare November 28, 2022 09:57

guggero force-pushed the accounts branch from c01eb05 to 672681e Compare November 28, 2022 14:44

sangaman approved these changes Nov 28, 2022

View reviewed changes

guggero force-pushed the accounts branch from 672681e to 0582aaf Compare November 29, 2022 11:11

Roasbeef approved these changes Dec 1, 2022

View reviewed changes

sangaman reviewed Dec 1, 2022

View reviewed changes

ellemouton approved these changes Dec 2, 2022

View reviewed changes

guggero added 13 commits December 2, 2022 10:06

mod: run go mod tidy

babe3c4

log+accounts: add accounts package

ff40b20

accounts: add account store

43ae6e0

accounts: add context utils

9f0b861

With this commit we add a set of utility functions that allow us to store and retrieve an account in/from a context.

rpcmiddleware+accounts: add checkers

4fd0ed5

This commit adds the checkers component that is responsible for making sure all incoming RPC requests and outgoing responses are tracked and their effects on an account are updated accordingly.

accounts: add account interceptor service

e4e51fb

This commit introduces the account interceptor service which makes sure that each request that contains an account macaroon is intercepted and passed through the checkers component to track each RPC call's effects on the account.

litrpc: add accounts service

6af85cf

session+accounts: add RPC server

cd74109

terminal: use account RPC server

8b58c10

cmd/litcli: add account commands

c7fb004

itest: add account system integration tests

51618de

doc: document accounts feature

3f959ab

multi: add account based LNC sessions

fc4f4a4

guggero force-pushed the accounts branch from 0582aaf to fc4f4a4 Compare December 2, 2022 09:10

guggero merged commit a7ca140 into master Dec 2, 2022

guggero deleted the accounts branch December 2, 2022 09:31

AndySchroder mentioned this pull request Feb 19, 2023

we need a python module lightninglabs/lightning-node-connect#71

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custodial off-chain accounts #363

Add support for custodial off-chain accounts #363

guggero commented May 12, 2022 •

edited

guggero commented May 20, 2022

ellemouton commented May 20, 2022

ellemouton left a comment

guggero commented Jun 21, 2022

Roasbeef left a comment

Roasbeef Jul 1, 2022

guggero Oct 7, 2022

Roasbeef Jul 1, 2022

guggero Oct 17, 2022

Roasbeef Jul 1, 2022

guggero Oct 17, 2022

Roasbeef Jul 1, 2022

guggero Oct 17, 2022

ellemouton left a comment

lightninglabs-deploy commented Aug 5, 2022

guggero commented Aug 5, 2022

guggero commented Oct 17, 2022

sangaman commented Nov 28, 2022

guggero commented Nov 28, 2022

sangaman left a comment

sangaman commented Nov 28, 2022

guggero commented Nov 29, 2022

Roasbeef left a comment

sangaman Dec 1, 2022

ellemouton left a comment

Add support for custodial off-chain accounts #363

Add support for custodial off-chain accounts #363

Conversation

guggero commented May 12, 2022 • edited

guggero commented May 20, 2022

ellemouton commented May 20, 2022

ellemouton left a comment

Choose a reason for hiding this comment

guggero commented Jun 21, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Aug 5, 2022

guggero commented Aug 5, 2022

guggero commented Oct 17, 2022

sangaman commented Nov 28, 2022

guggero commented Nov 28, 2022

sangaman left a comment

Choose a reason for hiding this comment

sangaman commented Nov 28, 2022

guggero commented Nov 29, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

guggero commented May 12, 2022 •

edited