Chain notifier implementations to handle reorgs #410

jimpo · 2017-11-14T01:40:00Z

This refactors the transaction confirmation watching logic into a shared module and implements correct handling of chain reorganizations.

Depends on #360.

samvrlewis · 2017-11-14T11:56:52Z

chainntnfs/btcdnotify/btcd.go

Maybe a dumb question, but isn't a reorg possible at a block that's not the latest? Should this call DisconnectTip at each block height between the current currentHeight and update.blockHeight (inclusive)?

btcd and neutrino reorgs both fire a disconnectblock notification for each block disconnected in a reorg. Makes it more convenient to work with than bitcoind's notifications, which only notify you of the latest block in such a situation.

samvrlewis · 2017-11-14T12:07:29Z

chainntnfs/btcdnotify/btcd.go

Admittedly I'm not totally familiar with how btcd callbacks fire, but if a reorg happens, I imagine that you'd probably get callbacks for new blocks and disconnected blocks around the same time. If the new block callback is processed first, won't this cause the block to get lost as the txConfNotifier doesn't know that a reorg has occurred, so will drop the update?

btcd fires disconnectblock notifications in reverse order, and then fires connectblock for each new connected block in order. The same goes for neutrino, which intentionally replicates the behavior.

halseth

Looks really good in my opinion!

halseth · 2017-11-16T00:24:01Z

chainntnfs/interface_test.go

notification

halseth · 2017-11-16T00:40:58Z

chainntnfs/interface_test.go

this can be written

select { case <-confirmed: case <-time.After(1 * time.Second): }

halseth · 2017-11-16T00:44:33Z

chainntnfs/interface_test.go

maybe check that height is at miner2's height?

What do you mean? Which height?

halseth · 2017-11-16T00:44:48Z

chainntnfs/interface_test.go

same as above

halseth · 2017-11-17T19:20:14Z

chainntnfs/txconfnotifier.go

halseth · 2017-11-17T19:20:39Z

chainntnfs/txconfnotifier.go

maybe add comment that this means the tx is unconfirmed

halseth · 2017-11-17T19:28:08Z

chainntnfs/txconfnotifier.go

delete instead?

halseth · 2017-11-17T19:31:10Z

chainntnfs/txconfnotifier.go

This if else statement needs some more comments, not obvious what's going on imho :P

halseth · 2017-11-17T19:31:29Z

chainntnfs/txconfnotifier.go

an explanation would be nice

Roasbeef · 2017-12-04T02:09:12Z

chainntnfs/btcdnotify/btcd.go

We can eliminate the nesting here by inserting a continue after the first case completes.

Roasbeef · 2017-12-04T02:14:43Z

chainntnfs/interface_test.go

Roasbeef · 2017-12-04T02:16:55Z

chainntnfs/neutrinonotify/neutrino.go

Similar comment here about avoiding the nesting by using a continue after the first clause.

Yes, technically true, but I don't think it makes sense in this context. The first block handles the case of a block connection, the second handles a block disconnection. The logic is symmetrical (like it would make sense if the if was !update.connect and the blocks were flipped), so I'd rather have them at the same level of indentation.

Removing the nesting will reduce the number of lines in the diff. It's not a block as it's mostly a style thing, but in the codebase we generally try to limit the degree of nesting in a given function.

Roasbeef · 2017-12-04T02:22:39Z

chainntnfs/neutrinonotify/neutrino.go

The comments (and also the code itself) can be re-flowed now to take advantage of the increased column limit due to the elimination of the nesting.

Roasbeef · 2017-12-04T02:23:15Z

chainntnfs/neutrinonotify/neutrino.go

A similar nesting optimization can be made here: we'd instead check for !ok above and continue if so.

Roasbeef · 2017-12-04T02:33:49Z

chainntnfs/txconfnotifier.go

This should also close a quit channel localized to the TxConfNotifier. All sends above would select on this quit channel to ensure things can be shutdown gracefully.

Roasbeef · 2017-12-04T02:58:05Z

chainntnfs/btcdnotify/btcd.go

Can you add a comment here to indicate that this means the transaction wasn't yet found in the mempool? Thanks!

Roasbeef · 2017-12-04T02:59:42Z

chainntnfs/btcdnotify/btcd.go

Seems like there's no need for an additional type in this case? As is, it'll just replicate the existing struct with a diff name.

That is right, but I kept it for symmetry with neutrinonotify. I don't believe there's any overhead to having this wrapper struct.

Roasbeef · 2017-12-04T03:01:00Z

chainntnfs/interface_test.go

First portion of sentence seems to be cut off?

Roasbeef · 2017-12-04T03:01:53Z

chainntnfs/txconfnotifier.go

Roasbeef

Nearly there, have one last round of comments.

I'll update one of my testnet nodes to start running this patch to see if any issues crop up over the next few days.

Roasbeef · 2017-12-08T23:28:50Z

chainntnfs/btcdnotify/btcd.go

In this case, it seems like we shouldn't accept this block as it may cause us to miss notifications.

Added a continue after this line. The TxConfNotifier would have returned an error anyway.

Roasbeef · 2017-12-08T23:29:38Z

chainntnfs/btcdnotify/btcd.go

Ping on comment about eliminating unnecessary nesting.

Done. I personally think the logic is less clear now, but it's your call.

Roasbeef · 2017-12-08T23:32:26Z

chainntnfs/neutrinonotify/neutrino.go

Removing the nesting will reduce the number of lines in the diff. It's not a block as it's mostly a style thing, but in the codebase we generally try to limit the degree of nesting in a given function.

Roasbeef · 2017-12-08T23:34:39Z

chainntnfs/txconfnotifier.go

Master was recently updated to always log whenever a confirmation is dispatched. Ideally this patch replicates the existing set of logs exactly to mirror the prior operation.

Roasbeef · 2017-12-08T23:37:29Z

chainntnfs/txconfnotifier.go

This can be simplified a great bit by using a second level of map which is based on the client ID.

Roasbeef · 2017-12-08T23:39:44Z

chainntnfs/txconfnotifier_test.go

Comments on these new added tests are virtually non existent.

Tests are failing for both btcd and neutrino notifiers.

This does not implement full handling of block disconnections, but ensures that all chain updates are processed in order.

All implementations of the ChainNotifier interface support registering notifications on transaction confirmations. This struct is intended to be used internally by ChainNotifier implementations to handle much of this logic.

Also fix overflow issue with reorg handling.

Roasbeef

LGTM 👾

samvrlewis reviewed Nov 14, 2017

View reviewed changes

jimpo force-pushed the notifier-reorg branch from 01477de to 63dd4ff Compare November 14, 2017 17:38

Roasbeef mentioned this pull request Nov 16, 2017

chainntnfs: Use new ConcurrentQueue in neutrino notifier. #360

Merged

jimpo force-pushed the notifier-reorg branch from 63dd4ff to dee1efe Compare November 16, 2017 23:30

halseth reviewed Nov 17, 2017

View reviewed changes

jimpo force-pushed the notifier-reorg branch 2 times, most recently from 038cb70 to cc5a880 Compare November 27, 2017 20:30

jimpo force-pushed the notifier-reorg branch from cc5a880 to 2eadbbb Compare November 29, 2017 01:47

Roasbeef requested changes Dec 4, 2017

View reviewed changes

jimpo force-pushed the notifier-reorg branch 2 times, most recently from e1af5da to a7a6e8a Compare December 6, 2017 22:54

Roasbeef requested changes Dec 8, 2017

View reviewed changes

Jim Posen added 11 commits December 10, 2017 11:27

chainntnfs: Test that chain notifiers handle chain reorgs correctly.

b6e6d1a

Tests are failing for both btcd and neutrino notifiers.

chainntnfs/btcd: Handle block disconnects with chainUpdate.

63acefc

This does not implement full handling of block disconnections, but ensures that all chain updates are processed in order.

chainntnfs/neutrino: Handle block connects and disconnects in order.

7cddf08

chainntnfs: Unit tests for TxConfNotifier.

da17404

Also fix overflow issue with reorg handling.

chainntnfs/btcd: Refactor BtcdNotifier to use TxConfNotifier.

6b7c06a

chainntnfs/neutrino: Refactor NeutrinoNotifier to use TxConfNotifier.

15c2925

chainntnfs: Test that neutrino rescan plays nice with txConfNotifier.

7efd7f5

chainntnfs: Send negative confirmation notifications.

984741d

chainntnfs: Implement quit signal in TxConfNotifier.

fa8c019

chainntnfs: Fix stylistic issues.

fa458c7

jimpo force-pushed the notifier-reorg branch 2 times, most recently from a4c5985 to fa458c7 Compare December 10, 2017 19:32

Roasbeef approved these changes Dec 15, 2017

View reviewed changes

Roasbeef merged commit bc7c834 into lightningnetwork:master Dec 15, 2017

jimpo deleted the notifier-reorg branch December 15, 2017 03:23

Chain notifier implementations to handle reorgs #410

Chain notifier implementations to handle reorgs #410

Uh oh!

Conversation

jimpo commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!