lnrpc: holdinvoice design [wip] #2263

joostjager · 2018-12-03T11:07:15Z

Decisions:

Report and accept all values in sat and msat.
CancelInvoice and SettleInvoice use the standard grpc error value for their result.
Keep invoice state and values history.
Send out the exact same updates during catch up as the first time (deterministic)
Rename add_index to invoice_number for less impl. oriented naming

To decide:

When exactly to accept htlcs in ACCEPTED and SETTLED?
Is the add event in the update stream as well?
Do we want an update stream for a single invoice? Can we make addinvoice blocking and idempotent to achieve this?
Do we want an accept_date and how to deal with state transitions OPEN -> ACCEPT -> OPEN -> ACCEPT?
Bucket structure:
- Duplicate dynamic fields in the invoice or only reference them using the update_index pointer?
- Store invoice event in invoice sub bucket?
- Keep settle_index bucket?

lnrpc/invoicerpc/invoice.proto

joostjager · 2018-12-03T11:20:26Z

lnrpc/invoicerpc/invoice.proto

+        ACCEPTED = 1;
+
+        /**
+        In the settled state, the invoice isn't payable anymore. We have


Non-payable settled invoices is a change from the current implementation. Not sure what the use case is for paying to a settled invoice. It may be negative, because business owners can feel obliged to issue refunds? Because it is insecure (preimage already revealed), we may not want to encourage this either? I also try to see the parallel with traditional web checkouts. I don't think it is possible to pay an order twice.

I think the choice should be left to the receiver if they want to allow receiving multiple times but agree the default should be that they cannot

Related issue: #2208

Shouldn't we then also accept (not yet settle) payments with a value less than the invoice amt?

@cfromknecht What is your idea about these state transitions and when to accept/settle htlcs? I we extend the 'greedy' approach further, we should really lock-in every htlc that we get, even thought the final cltv and/or value may be less than what is specified in the invoice.

I would say that we should only transition to accept if we have one (or more) HTLCs at that time that we could plausibly settle and meet the invoice requirements.

For single payments: if the cltv or value is less than the invoice requirements, i'd say just reject and not accept.

For multi-path payments:

if the ctlv is incorrect, fail it

otherwise,

if the cltv of any other previous htlcs has expired, fail those

if the total value hasn't been received, wait

otherwise, if the total value has been received, accept

(just a rough sketch)

Okay, that is for the OPEN state. What shall we do in the ACCEPT and SETTLED states? Do we go into catch all mode?

lnrpc/invoicerpc/invoice.proto

cfromknecht · 2018-12-04T01:49:39Z

lnrpc/invoicerpc/invoice.proto

    weren't connected to the streaming RPC.
    */
-    uint64 add_index = 1;


why not keep this, and call it last_invoice_number? that way clients can be sure they're seeing all created invoices

With update_index they can do exactly do same, because the creation is an event as well?

I was thinking they would be separate for backwards compatibility. We can have a migration that copies over the existing settle_index entries into the update index, and client indices would be compatible.

If adds are also in the update index, they must have an index that precedes the settle index which breaks old values.

May this isn't so bad, and people just reindex?

So the migration would be to create update records as follows?

update index: settle index invoice_number: add index state: settled settle_date: settle_date amt_paid: amt_paid

I think it breaks an assumption anyway in that from the migration onwards not every value of update_index will be a settle event anymore.

What I don't like about two indices is that you need to call SubscribeInvoices twice to get a complete picture of the current state.

I think I am in favor of just leaving settle and add index what they are - we need to do that anyway because we are only adding a subserver - and add an updateindex to the db that includes the created event as well. For the migration, we can first insert all the existing adds in the updateindex and then all existing settles.

cfromknecht · 2018-12-04T02:34:34Z

lnrpc/invoicerpc/invoice.proto

@@ -244,28 +352,34 @@ message ListInvoiceResponse {
 message InvoiceSubscription {
    /**
    If specified (non-zero), then we'll first start by sending out
-    notifications for all added indexes with an add_index greater than this
-    value. This allows callers to catch up on any events they missed while they
+    all invoices that have an last_update_index greater than this value. This allows


Another option here is to replay the entire event stream since the last_update_index, instead of skipping to the aggregate of multiple updates to the same invoice. Of the two, I have a slight preference for replaying the event-stream since:

client handling is simpler, updates always contiguous, no initial-startup handling for jumping ahead in stream

doesn't require a scan of the entire db for each new client (would need to decode+check O(n) invoices, even if client only missed a few updates)

OTOH, the log-based approach takes up slightly more storage by keeping all of the events and not-aggregating the final values

doesn't require a scan of the entire db for each new client (would need to decode+check O(n) invoices, even if client only missed a few updates)

this isn't necessary, because of the update_index bucket, which is sorted? it looks up the update_index specified by the subscribeinvoices client and then uses a cursor to go through all the changed invoices until the end of the bucket is reached.

Ahh okay, I was thinking the invoices themselves would be mutated, and we'd just search them all for ones that contain last_update_index >= last_client_update_index. Is your thinking we'd store a complete invoice for each update_index?

From your comments above, would we garbage collect a prior update_index for an invoice that gets updated?

Okay, I think i've got a picture of what you're describing. The update_index only stores the invoice_number, the invoice itself gets mutated directly and we can use the update_index to scan for any invoices that have been recently modified.

Indeed I like this and agree it's perfectly feasible. From an API perspsective it seems the only real tradeoff between the two is whether we expose intermediate state transitions to the client.

I wonder how much more complex the client state machine would have to be to handle these gaps. Say the invoice goes through states A -> B -> C -> D. If all intermediate transitions are preserved, the client only needs to handle A -> B, B -> C, and C -> D since they'll be replayed reliably.

If we skip intermediate states, the client also has to be ready to handle A -> C, A -> D, and B -> D, which definitely increases number of transitions that need to implemented and tested.

There is also information lost via aggregation, e.g. the client can't distinguish between the OPEN -> CANCEL and OPEN -> ACCEPTED -> CANCELED if they only see the first and last states.

I don't know how many applications will require those intermediate states, but that info sounds useful at the same time :)

If we choose to have different semantics via rpc vs internal subscriptions, I don't think this poses too much overhead. The mechanism you proposed would work the same, just ignoring the additional fields in the value (since the invoice has already been updated to its final state).

It could be a simple boolean option internally (possibly exposed via rpc as well) that asks for intermediate states on restart

I was just considering the trade-off between implementation effort and functionality. I agree that deterministic behaviour is better. What is an RSM btw?

You are right about the abstraction level. These are two independent things.

And yes, the diff in definition of update_index isn't much. It is a new bucket anyway.

Okay, so we go for replayable history.

Just wondering if we want to keep the dynamic fields in the invoice as well. The alternative is only store static fields in the invoice and use the invoice field update_index to retrieve the latest dynamic values.

In any case, I think if we store intermediate values, we should also stream them over rpc.

I added a commit for the InvoiceEvent message.

Catching up on this conversation, I think this is similar to what I described above, in that the API can be purely event based so the user doesn't need to filter out fields which each update, instead we give them a discrete event to handle.

Filter out, as in ignore all static invoice fields because they are already known?

joostjager · 2018-12-05T11:03:05Z

lnrpc/invoicerpc/invoice.proto

+    notifying the client of invoice updates. The caller can optionally specify
+    a last_update_index to catch up with missed updates.
+    */
+    rpc SubscribeInvoices (InvoiceSubscription) returns (stream Invoice) {}


Instead of this global subscribe function (is it scalable?), we could also go for a SubscribeInvoice with a specific invoice number. Isn't it that applications (except for some kind of global node monitoring dashboard), know their own invoices?

Then we also wouldn't need all those indices. Just start the stream by sending back the current invoice state and keep sending updates as they happen. It will be a never ending stream in case we decide to keep accepting htlcs even though the invoice is settled. But when the client isn't interested anymore, it can just close the stream.

This SubscribeInvoice can even be combined with AddInvoice. Create an idempotent AddInvoice.

Agreed! It would certainly be useful to also be able to filter for a specific invoice (or maybe even a list?)

How do you feel about an idempotent, streaming AddInvoice? It can still be used like the current AddInvoice. Just close the stream after the first (creation) update is received.

Calling AddInvoice again, will stream again all events from the beginning.

How could this be done efficiently? Retrieving all updates for an invoice without scanning through the full update_index bucket? Maybe we need a sub-bucket in the invoice containing all update_indices? Then it feels maybe safer to also store the dynamic values in that bucket. All invoice data close to each other. A different index is then needed to do the global SubscribeInvoices. This can again be just update_index -> invoice_number.

Another question that I have with the existing global SubscribeInvoices is when to store those last indices? Updating a persistent store after every event doesn't sound good. But using a timer is also not so clean.

SubscribeInvoices is nice for a global monitoring dashboard, but I suspect that most other applications know their own invoices and are much better served with a per invoice subscription.

joostjager · 2018-12-11T10:06:01Z

Do we also need to think about the on-chain invoice settle path? (contract resolver settling invoice)

Roasbeef · 2018-12-12T01:40:37Z

Do we also need to think about the on-chain invoice settle path? (contract resolver settling invoice)

Seems we wouldn't need to special case this at all? The resolver would just call the same method as the link does when it goes to settle an incoming invoice afaict.

lnrpc/invoicerpc/invoice.proto

Roasbeef · 2018-12-12T01:43:56Z

lnrpc/invoicerpc/invoice.proto

+    flag to false. If none of the parameters are specified, then the last 100
+    invoices will be returned.
+    */
+    rpc ListInvoices (ListInvoiceRequest) returns (ListInvoiceResponse) {}


In our current case, do we have any need for stuff like reversal?

For my use, I don't need to list invoices at all. Keep track of the hashes myself.

This is in the proto to have a complete proposal for a new invoice interface. Once that stabilizes it should be able to replace what we currently have in the main rpc. I am leaving the call in to prevent us coming up with a system that doesn't take it into account.

Roasbeef · 2018-12-12T01:44:42Z

lnrpc/invoicerpc/invoice.proto

+    duplicated invoices are rejected, therefore all invoices *must* have a
+    unique payment preimage.
+    */
+    rpc AddInvoice (AddInvoiceRequest) returns (AddInvoiceResponse) {}


Perhaps we should introduce a distinct call for hodl invoices to make the difference more clear on the RPC interface?

Separate call vs a field in the request, not much difference to me. The hold field will have a default of false, so unaware users won't be bothered.

lnrpc/invoicerpc/invoice.proto

Roasbeef · 2018-12-12T01:53:17Z

lnrpc/invoicerpc/invoice.proto

+    InvoiceState state = 22;
+}
+
+message InvoiceEvent {


Perhaps we should use a oneof here to create a sort of sum-type? So for example, an InvoiceEvent is either a:

AddEvent

AcceptEvent

SettleEvent

CancelEvent

This way the client doesn't need to know which fields will be set and which one won't be when we send an update. Instead, they just need to be aware of each event type, and we can add additional ones in the future.

I think we want that state type anyway to provide full info on list/lookup invoice(s). Also oneof isn't a prototype of beauty in go.

Roasbeef · 2018-12-12T01:55:15Z

lnrpc/invoicerpc/invoice.proto

+}
+
+
+enum InvoiceState {


See the comment above about the possibility of adding an event type using a oneof. This might cause us to meld this state into the events themselves. It may even be the case that when we list or query for an invoice, then we show all the individual events in the response, and a final set of fields that summarizes the final state.

But that final set probably contains a state field again?

Roasbeef · 2018-12-12T01:59:50Z

lnrpc/invoicerpc/invoice.proto

+    A list of invoices from the time slice of the time series specified in the
+    request.
+    */
+    repeated Invoice invoices = 1;


See comment re including the entire event stream for each invoice and the final state. Can also be helpful to know when an invoice was settled, and how long it took for it to be settled, etc. This would mean we can remove things like settled_date from the invoice as it's in the main event stream.

We need to discuss whether we always want to return the full history. For now I did an intermediate step. Updated the proto to return the last version of the dynamic data in an invoice.

Roasbeef · 2018-12-12T02:03:32Z

lnrpc/invoicerpc/invoice.proto

@@ -244,28 +352,34 @@ message ListInvoiceResponse {
 message InvoiceSubscription {
    /**
    If specified (non-zero), then we'll first start by sending out
-    notifications for all added indexes with an add_index greater than this
-    value. This allows callers to catch up on any events they missed while they
+    all invoices that have an last_update_index greater than this value. This allows


Catching up on this conversation, I think this is similar to what I described above, in that the API can be purely event based so the user doesn't need to filter out fields which each update, instead we give them a discrete event to handle.

Roasbeef · 2019-03-15T20:27:07Z

Superseded by #2022.

joostjager commented Dec 3, 2018

View reviewed changes

lnrpc/invoicerpc/invoice.proto Show resolved Hide resolved

joostjager force-pushed the holdinvoice-design branch from 1960c2e to 9b51ed6 Compare December 3, 2018 11:12

joostjager commented Dec 3, 2018

View reviewed changes

lnrpc/invoicerpc/invoice.proto Outdated Show resolved Hide resolved

joostjager commented Dec 3, 2018

View reviewed changes

cfromknecht reviewed Dec 4, 2018

View reviewed changes

joostjager mentioned this pull request Dec 5, 2018

htlcswitch: hodl invoice #2022

Merged

joostjager commented Dec 5, 2018

View reviewed changes

joostjager force-pushed the holdinvoice-design branch from 9b51ed6 to 61256ce Compare December 6, 2018 08:18

Roasbeef reviewed Dec 12, 2018

View reviewed changes

lnrpc: copy original invoice rpcs into sub server

9e3589c

joostjager force-pushed the holdinvoice-design branch 2 times, most recently from e9c6034 to 03f589b Compare December 12, 2018 12:50

joostjager added 2 commits December 12, 2018 13:55

lnrpc: remove unused invoice fields

7d424d3

lnrpc: hold invoice proto

4d478aa

joostjager force-pushed the holdinvoice-design branch from 03f589b to 4d478aa Compare December 12, 2018 12:55

joostjager mentioned this pull request Dec 21, 2018

channeldb+lnrpc: remove receipt invoice field #2360

Merged

Roasbeef closed this Mar 15, 2019

		}


		enum InvoiceState {

lnrpc: holdinvoice design [wip] #2263

lnrpc: holdinvoice design [wip] #2263

Conversation

joostjager commented Dec 3, 2018 • edited Loading

joostjager Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

alexbosworth Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht Dec 6, 2018 • edited Loading

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

Choose a reason for hiding this comment

cfromknecht Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

cfromknecht Dec 6, 2018 • edited Loading

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

joostjager Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Dec 11, 2018

Roasbeef commented Dec 12, 2018

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

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

Roasbeef commented Mar 15, 2019

joostjager commented Dec 3, 2018 •

edited

Loading

joostjager Dec 3, 2018 •

edited

Loading

alexbosworth Dec 3, 2018 •

edited

Loading

cfromknecht Dec 6, 2018 •

edited

Loading

cfromknecht Dec 5, 2018 •

edited

Loading

cfromknecht Dec 6, 2018 •

edited

Loading

joostjager Dec 6, 2018 •

edited

Loading