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

channeldb+lnrpc: re-work invoicing API to implement robust streaming notification delivery #1125

Merged
merged 26 commits into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@Roasbeef
Member

Roasbeef commented Apr 25, 2018

In this PR, we modify the streaming invoice subscription API slightly in order to allow callers to have assurance that they haven't missed any new payments. The SubscribeInvoice API now has two new values: add_index, and settle_index. To match these new values, the Invoice message has also gained a similar set of fields. These two indexes effectively act as an event time series: each time a new invoice is added the add_index will be incremented, and each time a new invoice is settled the settle_index will be incremented. With this new feature, clients can now specify one or both of these new optional fields with the last index they know of. If specified, then we'll query the database to find all events greater than this index, and then deliver these backlog notifications before sending out any new notifications.

Care has been taken to ensure that the new API is backwards compatible with the expectations of the old API. Namely, if the fields aren't specified (are zero), then no backlog notifications will be delivered. As a result, the index on-disk actually starts at 1.

A database migration has been created in order to upgrade old databases to the new invoice schema that has these two new indexes which need to be updated each time a new invoice has been added, or an exiting one settled.

Finally, a new field has been added to the on-disk Invoice struct: AmtPaid. This new field allows the link to commit exactly what value was accepted for the final invoice. This is important as invoices may have not have any value attached to them at all ("donation" invoices), or it may be the case that the invoice was overpaid. In either case, the final value accepted for an invoice will now be stored on disk, and queryable over the RPC interface.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 25, 2018

Will add an additional integration test to ensure the backlog notification delivery works properly on the client-side.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Apr 25, 2018

Considering just converting to a full event time-series/log...

@cfromknecht

Nice, this is a cool feature 😎Design pattern looks like it'd be similar to upcoming changes to the block epoch registration.

Did an initial pass, overall design looks complete. There may be issues with reordering of add/settle indexes, but idt they will be difficult to fix.

if err != nil {
return err
}
settleEvents, err := i.cdb.InvoicesAddedSince(client.settleIndex)

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

InvoicesSettledSince?

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Fixed.

return err
}
case settledInvoice := <-invoiceClient.SettledInvoices:

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

why do we need to differentiate here? logic looks identical, and just needs to be detected by client i think

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Simply logical separation. You're suggesting we should instead just return modified invoices, directly rather than partitioning into settled/added?

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

just saying that there is no difference in how we handle them in the rpcserver, so they could all be sent over the same stream

return i.cdb.AddInvoice(invoice)
// We'll launch a new goroutine to notify all of our active listeners
// that a new invoice was just added.
go i.notifyClients(invoice, false)

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

notify after write w/ no error?

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

the invoices AddIndex wouldn't be set until after the db call

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Nice catch, fixed! Will also add a set of integration tests before the next round of review.

// dispatch notifications to all registered clients.
case event := <-i.invoiceEvents:
for _, client := range i.notificationClients {
client.ntfnQueue.ChanIn() <- &invoiceEvent{

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

select on quit as well?

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Fixed.

invoiceCursor.Seek(startIndex[:])
seqNo, invoiceKey := invoiceCursor.Next()
for ; seqNo != nil && bytes.Compare(seqNo, startIndex[:]) > 0; seqNo, invoiceKey = invoiceCursor.Next() {

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

nit: 80 chars

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Any attempts to wrap are promptly shutdown by gofmt :/

@@ -0,0 +1,3241 @@
=== RUN TestInvoiceAddTimeSeries

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

accidental add? :)

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

lol yeh should be gone in the latest version

eventChan <- invoice
}()
select {
case i.invoiceEvents <- event:

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

This may cause invoices to be delivered out of order, which means the clients may have holes in their history

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

What will?

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

execution of this method w/in a goroutine can permit reordering of the settled invoices. the settle ids will be assigned in-order, but we need to ensure they're also queued in that order

ltndLog.Errorf("unable to deliver backlog invoice "+
"notifications: %v", err)
}

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

maybe need a check to ensure the next add and settle indexes are aligned with upcoming ntfns

This comment has been minimized.

@cfromknecht

cfromknecht Jun 14, 2018

Collaborator

I think this can be done by keeping track of the "last delivered" add/settle, then passing these into deliverBacklogEvents. If it delivers up to and including those values, the next things in the pipeline should be gtg

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

What do you mean aligned with the upcoming notifications? To ensure that there isn't a gap between what the client new last, and the next upcoming notification?

The aspiration here was that since we deliver these backlogs within the main event loop, the client should then get all the notifications since that point as well.

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

right now i believe it'd be possible to deliver duplicate notifications since the database state can be modified outside of the main loop.

to prevent this, we should clamp the delivery of historical adds/settles using the last delivered heights of each

}
i.ntfnQueue.Stop()
close(i.cancelChan)

This comment has been minimized.

@cfromknecht

cfromknecht Jun 15, 2018

Collaborator

multiple calls to Cancel would cause this to panic

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Fixed.

@Roasbeef Roasbeef force-pushed the Roasbeef:streaming-invoice-improvements branch 2 times, most recently from 2bd010c to ac327b3 Jun 27, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 30, 2018

Fixed some consistency issues, addressed the comments, fixed a bug, and also added a set of new itests. PTAL!

@Roasbeef Roasbeef closed this Jun 30, 2018

@Roasbeef Roasbeef reopened this Jun 30, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:streaming-invoice-improvements branch from ac327b3 to f85e04b Jun 30, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 30, 2018

Still need to test the migration itself though.

Roasbeef added some commits Apr 25, 2018

lnrpc: extend invoice subscriptions to allow callers to receive backl…
…og notifications

In this commit, we extend the current SubscribeInvoice streaming RPC
call. We add two new values to the InvoiceSubscription message:
add_index and settle_index. These fields have also been added to the
current Invoice message. Each time a new invoice is added, the add index
will be incremented. Each time a new invoice is settled the settle index
will be incremented. The new field on the InvoiceSubscription message
allow callers to specify the last add index and the last settle index
they know of. With this new addition, callers will now be able to
reliably receive notifications for new received payments.

Care has been taken to ensure that these changes are backwards
compatible. If callers don't specify either of the new fields, then they
won't receive any notification backlog at all.

Fixes #862.
htlcswitch: modify the InvoiceDatabase interface to allow specifying …
…final payment amt

In this commit, we modify the InvoiceDatabase slightly to allow the link
to record what the final payment about for an invoice was. It may be the
case that the invoice actually had no specified value, or that the payer
paid more than necessary. As a result, it's important that our on-disk
records properly reflect this.

To fix this issue, the SettleInvoice method now also accepts the final
amount paid.

Fixes #856.
channeldb: add new AmtPaid field to the Invoice struct
In this commit, in order to allow the caller to specify the amount that
was ultimately accepted for an invoice, the SettleInvoice method has
gained a new parameter: amtPaid. SettleInvoice will now populate the
final amount paid in the database upon db commit.
channeldb: add new add+settle index to invoice database
In this commit, we add two new indexes to the invoice database: the add
index, and the settle index. These to indexes essentially form a time
series index on top of the existing primary index bucket. Each time an
invoice is added, we'll advance the addIndex seqno, and then create a
mapping from seqNo -> invoiceNum. Each time an invoice is settled, we'll
do the same, but within the settle index.

This change is required in order to allow callers to effectively seek
into the current invoice database in order to obtain notifications for
any invoices they may have missed out on while they were disconnected.
This will allow us to implement robust streaming invoice notifications
within lnd to ensure that clients never miss an event.
channeldb: add two methods to allow seeking into the invoice time series
In this commit, we add two new methods: InvoicesAddedSince and
InvoicesSettledSince. These methods will be used by higher level
sub-systems that implement notifications to deliver any notifications
backlog based on the last add index, and last settle index that the
client knows of.

It's important to note that care has been taken to ensure that this new
API can be used in a backwards compatible manner. If a client specifies
and index of 0 for either of the methods, then no backlog will be sent.
This is due to the fact that current users of the API don't expect any
backlog notifications to be sent. Additionally, the index actually
starts at 1, instead of 0.
invoiceregistry: re-work logic to support delivering notification bac…
…klog

In this commit, we re-work the existing invoiceRegistry struct to
support delivering backlog notifications to subscription clients if
needed. Rather than using 1 goroutine per-client per-event, each client
now gains a concurrent notification queue. This queue will then be used
to ensure in-order delivery of all notifications from the
invoiceEventNotifier.

The SubscribeNotifications method now takes two params: addIndex, and
settleIndex. These should be the values of the last add index and settle
index the caller knows of. If specified (not zero), then we'll look up
all the notifications that the caller has missed, and then deliver those
before sending out any new notifications. In order to do this without
losing ordering of events, we've added a new central goroutine which
will ensure that all events are properly serialized.
invoiceregistry: serialize all invoice modifications, eliminate extra…
… db call for settle

In this commit, we now ensure that all modifications to the invoice DB
are properly serialized. This ensures that our time series within the
database will be properly coherent. Additionally, within SettleInvoice,
we remove an extra DB call by taking advantage of the new SettleInvoice
method which will return the invoice being settled as well.
invoiceregistry: ensure we never send duplicate add/settle notifications
In this commit, we add additional logic to the primary notification
dispatch loop to ensure that we'll never send the same add/settle event
to the same client twice.

Consider the case where as we're sending a client its notification
backlog, a new invoice is settled (index=9). In this case, the database
will be reflected immediately, and the event will also be queued for
notifying later. We'll send out this latest event during the backlog
clear, but then will send it again once we return back to the main loop.

To ensure that this never happens, we'll now record the last index that
we’ve sent to a client to ensure that we no longer are able to send
duplicate notification events.

Roasbeef added some commits Jun 30, 2018

@Roasbeef Roasbeef force-pushed the Roasbeef:streaming-invoice-improvements branch from 4921cad to ceeeb6a Jul 6, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 6, 2018

Pushed out a new version with a bug in the migration logic fixed, as well as an update for lncli to display the new add_index field.

// We'll launch a new goroutine to notify all of our active listeners
// that a new invoice was just added.
i.notifyClients(invoice, false)

This comment has been minimized.

@cfromknecht

cfromknecht Jul 6, 2018

Collaborator

comment needs updating

This comment has been minimized.

@Roasbeef

Roasbeef Jul 6, 2018

Member

Updated.

err := i.deliverBacklogEvents(newClient)
if err != nil {
ltndLog.Errorf("unable to deliver backlog invoice "+
"notifications: %v", err)

This comment has been minimized.

@cfromknecht

cfromknecht Jul 6, 2018

Collaborator

we might wanna be returning this error to the registerer, so that they are aware the registration failed

This comment has been minimized.

@Roasbeef

Roasbeef Jul 6, 2018

Member

There currently isn't really a clean way of doing so as the deliverBacklogEvents method sends to the client directly, and the client has already returned the subscription after the initial send so the caller can consume the backlog notifications. FWIW, it should only return an error if the db is messed up, or if lnd is shutting down. In the latter case, the socket will break so they'll know that something is up.

This comment has been minimized.

@cfromknecht

cfromknecht Jul 10, 2018

Collaborator

I think we'd just need to pass in an errChan from SubscribeInvoices, and then report any errors or return nil if the backlogs were delivered successfully? Basically block at end SubscribeInvoices until the registration is complete?

This comment has been minimized.

@cfromknecht

cfromknecht Jul 10, 2018

Collaborator

Ahh i see wym about caller needing to pull backlogged updates, yep ignore this then

// If we've already sent this settle event to
// the client, then we can skip this.
case event.isSettle &&
client.settleIndex == invoice.SettleIndex:

This comment has been minimized.

@cfromknecht

cfromknecht Jul 6, 2018

Collaborator

This check (and the one below) could be made stricter by using <=

This comment has been minimized.

@Roasbeef

Roasbeef Jul 6, 2018

Member

Well if it's <, then would seem that the client has actually missed a set of events?

This comment has been minimized.

@cfromknecht

cfromknecht Jul 10, 2018

Collaborator

If it's <, then that means the stream has reverted to a prior update, no?

This comment has been minimized.

@cfromknecht

cfromknecht Jul 10, 2018

Collaborator

Just seems like if we're filtering duplicate sends at index i, that we should filter for all indexes <= i

This comment has been minimized.

@Roasbeef

Roasbeef Jul 11, 2018

Member

If we add a < here, then we'll just skip relevant notifications all together. Consider the case where the client's latest index is 9, and a notification for 10 arrives. 9 <= 10 so we'll skip, instead of actually delivering.

This comment has been minimized.

@cfromknecht

cfromknecht Jul 11, 2018

Collaborator

Gotcha, you're right. I had my left and right sides mixed up. >= is what I meant lol

This comment has been minimized.

@Roasbeef

Roasbeef Jul 11, 2018

Member

Fixed!

case !event.isSettle &&
client.addIndex == invoice.AddIndex:
continue
}

This comment has been minimized.

@cfromknecht

cfromknecht Jul 6, 2018

Collaborator

do you think it'd be worthwhile to log in the even that client.addIndex + 1 != invoice.AddIndex, and similarly for settles? I don't think it's possible atm, but could quickly alert us in the event we need to look into it

This comment has been minimized.

@Roasbeef

Roasbeef Jul 6, 2018

Member

Done!

Roasbeef added some commits Jun 30, 2018

channeldb: skip sub-buckets during invoice time series migration
In this commit, we fix an existing bu gin the invoice time series
migration code. Before this commit, the migration would fail as we would
try to migrate an empty invoice. We now detect this case and skip all
empty invoices.

We also add a bit more logging on both the info and trace logging level.

@Roasbeef Roasbeef force-pushed the Roasbeef:streaming-invoice-improvements branch from ceeeb6a to db4a09d Jul 6, 2018

@Roasbeef Roasbeef merged commit 9320760 into lightningnetwork:master Jul 11, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.3%) to 54.459%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment