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

chainntnfs: handle spend notifications within TxConfNotifier #2004

Merged
merged 23 commits into from Oct 31, 2018

Conversation

Projects
None yet
4 participants
@wpaulino
Collaborator

wpaulino commented Oct 4, 2018

In this PR, we overhaul the TxConfNotifier struct to also track spend notifications. This introduces a number of benefits, such as: removing duplicate spend notification logic within the different ChainNotifier implementations, spend reorg detection, and many other minor optimizations. This will serve as a stepping stone for re-enabling the height hint cache, as it previously had issues under reorgs. Details for the most important commits can be found inline, explaining what each one does and why.

Depends on #1787.

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 4 times, most recently from 4012931 to 38e0203 Oct 4, 2018

@wpaulino wpaulino added this to the 0.5.1 milestone Oct 5, 2018

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 3 times, most recently from 40c57fb to f4c2e0c Oct 5, 2018

@halseth

Did an initial pass! It is a bit massive, but it looks pretty good so far 👍

Show resolved Hide resolved chainntnfs/interface_test.go
Show resolved Hide resolved chainntnfs/height_hint_cache.go
// transactions. Each height hint represents the earliest height at
// which the transactions could have been confirmed within the chain.
hintCache ConfirmHintCache
confirmHintCache ConfirmHintCache

This comment has been minimized.

@halseth

halseth Oct 6, 2018

Collaborator

Good job on the step-by-step commits here, makes it easy to review! 🎉

Show resolved Hide resolved chainntnfs/interface.go Outdated
Show resolved Hide resolved chainntnfs/interface.go
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved lnd_test.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 3 times, most recently from a879b38 to 7e0b74b Oct 10, 2018

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Oct 12, 2018

Addressed remaining comments and extended the tests a bit more to correctly test the behavior of the SpendHintCache like we do with the ConfirmHintCache.

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 3 times, most recently from 366d7d9 to 368e623 Oct 13, 2018

Show resolved Hide resolved chainntnfs/interface.go
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/tx_notifier.go Outdated
Show resolved Hide resolved chainntnfs/bitcoindnotify/bitcoind.go Outdated
Show resolved Hide resolved chainntnfs/btcdnotify/btcd.go
Show resolved Hide resolved chainntnfs/btcdnotify/btcd.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 3 times, most recently from cd826ae to 02381fe Oct 19, 2018

@halseth

Minor comments, else looks good! 💯 💯

Show resolved Hide resolved chainntnfs/neutrinonotify/neutrino.go
Show resolved Hide resolved chainntnfs/interface.go
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go Outdated
Show resolved Hide resolved chainntnfs/txnotifier_test.go
Show resolved Hide resolved chainntnfs/txnotifier_test.go
Show resolved Hide resolved chainntnfs/bitcoindnotify/bitcoind.go
Show resolved Hide resolved chainntnfs/interface_test.go Outdated
Show resolved Hide resolved chainntnfs/interface_test.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch from 02381fe to 82dd815 Oct 24, 2018

@cfromknecht

first sweep done, looking really solid! great to see so much duplicated code removed. have some small comments, mostly regarding differences between how confs are handled in #1787 and how spends are handled here

Show resolved Hide resolved chainntnfs/neutrinonotify/neutrino.go
Show resolved Hide resolved chainntnfs/interface.go
func NewTxNotifier(startHeight uint32, reorgSafetyLimit uint32,
confirmHintCache ConfirmHintCache) *TxNotifier {
confirmHintCache ConfirmHintCache,

This comment has been minimized.

@cfromknecht

cfromknecht Oct 25, 2018

Collaborator

still don't see the use in having these separate when the can just share a HintCache, but not a blocker

This comment has been minimized.

@wpaulino

wpaulino Oct 25, 2018

Collaborator

Agreed - we already have the unified HeightHintCache struct that handles both of these cases. Perhaps this should be done within #2044 or even another PR?

This comment has been minimized.

@halseth

halseth Oct 29, 2018

Collaborator

Best to just do in this PR imo, to avoid ever introducing the spendHintCache.

This comment has been minimized.

@cfromknecht

cfromknecht Oct 29, 2018

Collaborator

just remembering that it is already done in #1767, though it was never merged

Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go
Show resolved Hide resolved chainntnfs/txnotifier.go

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch 3 times, most recently from b238e5c to 19c1aa6 Oct 25, 2018

wpaulino added some commits Oct 5, 2018

chainntnfs/bitcoindnotify: handle spend notification registration w/ …
…TxNotifier

In this commit, we modify the logic within RegisterSpendNtfn for the
BitcoindNotifier to account for the recent changes made to the
TxNotifier. Since it is now able to handle spend notification
registration and dispatch, we can bypass all the current logic within
the BitcoindNotifier and interact directly with the TxNotifier instead.

The most notable changes include the following:

  1. We'll only attempt a historical rescan if the TxNotifier tells us
  so.

  2. We'll dispatch the historical rescan within the main goroutine to
  prevent WaitGroup panics, due to the asynchronous nature of the
  notifier.
chainntnfs/txnotifier: correctly update confirm/spend hints on chain …
…updates

In this commit, we address an issue w.r.t. updating the confirm hints
for transactions and spend hints for outpoints on chain updates.
Previously, upon a block being disconnected, we'd attempt to commit a
new height hint for all outstanding confirmation notifications. This is
not correct because we'll end up modifying the height hint for things
that have confirmed at a previous height than the one being
disconnected. This would cause issues on restart when attempting a
historical dispatch, as we would start scanning at a height above which
the transaction actually confirmed in.

This has been addressed by only updating the hints for outstanding
notifications that are still unconfirmed/unspent and for notifications
that were confirmed/spent within the block being connected/disconnected.
chainntnfs/interface_test: stop UnsafeStart notifiers within test
In this commit, we modify the set of tests that start the different
backend notifiers with UnsafeStart to stop them within the tests
themselves. This prevents us from running into a panic when attempting
to run the package-level tests with a filter (using test.run).
chainntnfs/txnotifier: allow registration of spend notifications
In this commit, we introduce the registration logic for spend
notifications to the TxNotifier. Most of this logic was taken from the
different existing ChainNotifier implementations, however, it features
some useful additions in order to make the ChainNotifier a bit more robust.

Some of these additions include the following:

  1. RegisterSpend will now return a HistoricalSpendDispatch struct,
  which includes the details for successfully determining if an outpoint
  was spent in the past. A HistoricalSpendDispatch will only be returned
  upon the first registration of an outpoint. This is done as,
  previously, if multiple clients registered for the same outpoint, then
  multiple historical rescans would also be dispatched, incurring a toll
  on the backend itself.

  2. UpdateSpendDetails will now be used to determine when a historical
  rescan has completed, no matter if a spending transaction was found or
  not. This is needed in order to responsibly update the spend hints for
  outpoints at tip, otherwise we'd attempt to update them even though we
  haven't yet determined if they have been spent or not. This will
  dispatch notifications to all currently registered clients for the
  same outpoint. In the event that another client registers later on,
  then the spending details are cached in memory in order to prevent
  further historical rescans.
chainntnfs/btcdnotify: remove old spend notification handling logic
In this commit, we remove the old spend notification logic within the
BtcdNotifier as it's been phased out by the TxNotifier.
chainntnfs: extend SpendEvent with reorg channel
In this commit, we add a new channel within the SpendEvent struct that
will be sent upon whenever the spending transaction of the registered
outpoint gets reorged out of the chain. This will pave the road for
successfully handling a funding transaction getting reorged out of the
chain among other things.
chainntnfs/btcdnotify: handle spend notification registration w/ TxNo…
…tifier

In this commit, we modify the logic within RegisterSpendNtfn for the
BtcdNotifier to account for the recent changes made to the TxNotifier.
Since it is now able to handle spend notification registration and
dispatch, we can bypass all the current logic within the
BtcdNotifier and interact directly with the TxNotifier instead.

The most notable change is that now we'll only attempt a historical
rescan if the TxNotifier tells us so.
chainntnfs/neutrinonotify: handle spend notification registration w/ …
…TxNotifier

In this commit, we modify the logic within RegisterSpendNtfn for the
NeutrinoNotifier to account for the recent changes made to the
TxNotifier. Since it is now able to handle spend notification
registration and dispatch, we can bypass all the current logic within
the NeutrinoNotifier and interact directly with the TxNotifier instead.

The most notable change is that now we'll only attempt a historical
rescan if the TxNotifier tells us so.
chainntnfs/neutrinonotify: remove old spend notification handling logic
In this commit, we remove the old spend notification logic within the
NeutrinoNotifier as it's been phased out by the TxNotifier.
chainntnfs/txnotifier: watch for spends at tip
In this commit, we add support to allow the TxNotifier to properly
determine whether a new block extending the chain contains a transaction
that spends a registered outpoint. In the event that it does, spend
notifications will be dispatched to all active registered clients for
such outpoint.
chainntnfs/txnotifier: detect reorgs for spending transactions of reg…
…istered outpoints

In this commit, we introduce support to the TxNotifier to detect
spending transactions of registered outpoints being reorged out of the
chain. In the event that a reorg does occur, we'll consume the Spend
notification if it hasn't been consumed yet, and dispatch a Reorg
notification instead.
chainntnfs/height_hint_cache: prevent db transactions with no updates
In this commit, we modify our height hint cache to no longer start a
database transaction if no outpoints/txids are provided to update the
height hints for.
chainntnfs/txnotifier: add fields/structs to track spend notifications
In this commit, we introduce the required fields for the TxNotifier to
properly carry its duties in notifying its registered clients about the
spend of an outpoint. These are not yet used, but will be throughout the
some of the following commits.
chainntnfs/txnotifier: commit height hint after rescan is complete
In this commit, we'll now commit the current height of the TxNotifier as
the height hint for an outpoint/transaction after a rescan has been
completed and has determined that the outpoint is unspent/transaction is
unconfirmed. We do this to prevent another potentially long rescan if
the daemon is restarted before a new block comes in (which is when the
hints will be updated again).
chainntnfs/neutrinonotify: make filter update synchronous
In this commit, we modify the notifier to handle filter updates
synchronously. We do this to prevent race conditions between new block
notifications and filter updates. Otherwise, it's possible for a new
block to come in that should match our filter, but doesn't due to the
filter being updated after.

We also modify their order so that the filter is updated first. We do
this so we can immediately start watching for the event at tip while the
rescan is ongoing.

@wpaulino wpaulino dismissed stale reviews from Roasbeef and halseth via e6b1a27 Oct 31, 2018

@wpaulino wpaulino force-pushed the wpaulino:spend-tx-notifier branch from 74147a0 to e6b1a27 Oct 31, 2018

@cfromknecht

LGTM 🔥optimizations incoming!

@Roasbeef Roasbeef merged commit 9f0b008 into lightningnetwork:master Oct 31, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 55.873%
Details

@wpaulino wpaulino deleted the wpaulino:spend-tx-notifier branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment