lightningnetwork
Let gRPC SubscribeTransactions replay transactions from the request
Fixes #5339
This PR allows SubscribeTransactions to use of its GetTransactionsRequest argument to replay transactions before notifying of new transactions.
Changes
Some general changes that this PR makes:
- Add the possibility of canceling
btcwallet.ListTransactionDetailwhile loading transactions from the database. - Expose the
quitchannel on theSubscriptionClientvia aDonefunction and use it to cancel loading replay transactions if the client is closed before.
The SubscribeTransactions method of the RPCServer has logic added to load replay transactions. During this process it captures notifications to allow replaying of transactions before new notifications.
Implementation details
The RPCServer is made responsible for replay of transactions. This way the replay transactions are loaded in the background while making this process cancel-able if the SubscriptionClient is closed early.
If however there is feedback and insights into why it would make more sense to implement the logic of replaying transactions directly to the WalletController interface/lnwallet.LightningWallet, I am open to take a look at that and make those changes.
But my initial thoughts on this is that for e.g. SubscribeInvoices it is possible to lock the whole InvoiceRegistry and block invoices from being notified and created/paid (making sure that any relevant invoices are completely blocked until replay is done and the client is available to listen for new invoices). This is not exactly true for the btcwallet dependency, which will notify of relevant transactions even if it cannot write to the db and is blocked there. If the behaviour should be to notify of replay transactions before any new nofitications, then those new notifications must be captured while loading the replay transactions. This can also be done in the lnwallet.LightningWallet, but since this doesn't give a benefit as far as I can see and also makes canceling the process more complicated, I chose to let the RPCServer handle this.
The process of loading replay transactions must happen after starting the SubscriptionClient as otherwise there is a small but potential gap where new notifications could get lost. This however opens up a (tiny?) window where e.g. a block could be notified and directly afterwards loaded as replay transactions. Logic is implemented to filter out duplicates from those captured notifications while loading replay transactions.
Is this PR still active? If the conflicts can be resolved I'd be happy to review & test locally.
@sangaman Thanks for showing interest! I have rebased this PR to make it work under the current master.
I haven't looked into the code but I tried testing this manually. I confirmed that if I do GetTransactions with a certain start height, I get back a list of transactions with several entries as expected. If I do SubscribeTransactions with the same value for start height, I don't get notifications right away as I was expecting. Am I testing it wrong?
I'm testing with:
$ lnd --version
lnd version 0.14.1-beta commit=kvdb/v1.3.0-132-g0197159b5
@sangaman Ah, right! It didn't consider setting the end_height to the unconfirmed case (-1) as default if no explicit value is set. This is fixed with the last force-push. (Not sure if this edge case was missed with the re-base as I thought I had considered it when I initially worked on it).
@bjarnemagnussen, remember to re-request review from reviewers when ready
@bjarnemagnussen, remember to re-request review from reviewers when ready
@bjarnemagnussen, remember to re-request review from reviewers when ready