chainntnfs: Isolate conf notifications during historical scans #1787

cfromknecht · 2018-08-25T03:41:32Z

In this PR, we modify the TxConfNotifier to isolate confirmation notifications while a historical dispatch is being processed. The purpose of this is to segment which notifications should have their height hints updated in the cache. During a historical rescan, we don't yet know if the txn is in the chain, so we shouldn't continue to update its height hint until after finishing the rescan. Once the rescan finishes, the notification is grouped along with all other notifications at tip so that their height hints can progress as blocks are added.

This PR also introduces:

a reorg safety delta to height hints, currently 144 blocks
- with the recent LRU cache added to neutrino, this shouldn't result too much unnecessary network I/O
- the height hint is still clamped to whatever the client initial prescribes, so initial registration doesn't take a hit either
moves querying of confirmation height hints into the TxConfNotifier
reorders the persistence/spend-notification dispatch to improve external consistency

In a follow PR, we will do the same for spend notifications.

This PR depends on #1951, which contains the first four commits

halseth · 2018-09-20T09:04:09Z

Is this needed for initial height hint cache enabling?

cfromknecht · 2018-09-20T09:10:15Z

@halseth yes, and similar work needs to be done for spends

halseth

Did initial review pass.

chainntnfs/txconfnotifier.go

halseth · 2018-09-26T11:18:29Z

chainntnfs/txconfnotifier.go

+	confSet.ntfns[ntfn.ConfID] = ntfn
+
+	switch {
+	case ntfn.HeightHint > tcn.currentHeight:


Also comments on what is happening here.

chainntnfs/txconfnotifier.go

chainntnfs/txconfnotifier_test.go

halseth · 2018-10-01T12:19:08Z

chainntnfs/txconfnotifier_test.go

@@ -139,6 +143,11 @@ func TestTxConfFutureDispatch(t *testing.T) {
 		t.Fatalf("unable to register ntfn: %v", err)
 	}

+	err = tcn.UpdateConfDetails(*ntfn2.TxID, 0, nil)


why is this added/not here from before?

halseth · 2018-10-01T12:19:59Z

chainntnfs/txconfnotifier.go

@@ -124,35 +135,35 @@ func NewTxConfNotifier(startHeight uint32, reorgSafetyLimit uint32,
 // the confirmation details must be provided with the UpdateConfDetails method,
 // otherwise we will wait for the transaction to confirm even though it already
 // has.
-func (tcn *TxConfNotifier) Register(ntfn *ConfNtfn) error {
+func (tcn *TxConfNotifier) Register(ntfn *ConfNtfn) (bool, uint32, error) {


return values should be explained.

halseth · 2018-10-04T10:18:21Z

Needs a rebase! (I think?)

halseth

These changes are starting to look good IMO now :)

It will need a rebase, and some commits can be squashed together, as now it is a bit hard to review going in Github commit order (maybe an --ignore-date is needed?).

halseth · 2018-10-04T15:06:55Z

chainntnfs/txconfnotifier.go

 	}

-	ntfn, ok := ntfns[clientID]
+	ntfn, ok := confSet.ntfns[clientID]


I feel like this check could be removed in favor of the loop below now?

I don't think so, otherwise we will dereference a nil confSet

ahh nvm, misread. this is done in a later commit

chainntnfs/txconfnotifier.go

chainntnfs/bitcoindnotify/bitcoind.go

chainntnfs/txconfnotifier.go

lnd_test.go

…nature

to UpdateConfDetails

This commit ensures that a confSet's details are assigned in the confNotifications index after discovering the transaction at tip. The recent changes allow a later notification to be dispatched on registration if an earlier one has already discovered the confirmation details. Before this change, it was observed that a later registration would attempt an immediate delivery, but fail to do so because the confset's details were nil. This commit remedies that dispatch path, allowing the integration tests to pass again.

…irmed at tip In this commit, we mark the rescan status for a transaction as complete if we happen to detect it has confirmed within a new block that extends the chain. We do this as otherwise, it's possible for us to not immediately dispatch the notification upon a subsequent registration due to the rescan state machine.

…n block inclusion In this commit, we'll attempt to consume a reorg notification for a transaction that was previously reorged out of the chain upon block inclusion to ensure that it is not lingering due to a client not handling it the first time.

In this commit, we address a small bug where it's possible to deliver a confirmation notification with stale confirmation details upon registration. This can happen if a transaction has confirmed but was reorged out of the chain later on, and a subsequent notification is registered.

Removes details field from conf notifications, in favor of using the details on the confSet. We also bound the requested conf depth to the reorg saftey limit, as the behavior of state tracking within the notifier is undefined otherwise.

cfromknecht · 2018-10-27T01:33:08Z

Rebased

Roasbeef

LGTM 💡

cfromknecht force-pushed the isolate-scanning-ntfns branch 2 times, most recently from 02f825f to 63213cd Compare August 25, 2018 03:47

cfromknecht added this to the 0.5 milestone Aug 25, 2018

Roasbeef modified the milestones: 0.5, 0.5.1 Aug 26, 2018

cfromknecht force-pushed the isolate-scanning-ntfns branch from 63213cd to b40ab48 Compare August 27, 2018 04:42

Roasbeef added notifications P2 should be fixed if one has time bug fix labels Sep 9, 2018

cfromknecht force-pushed the isolate-scanning-ntfns branch from b40ab48 to 925a658 Compare September 13, 2018 23:00

cfromknecht force-pushed the isolate-scanning-ntfns branch from 925a658 to 0ddcce8 Compare September 21, 2018 00:59

cfromknecht mentioned this pull request Sep 21, 2018

chainntnfs: Reorder commit hints and notify #1951

Merged

cfromknecht force-pushed the isolate-scanning-ntfns branch from 0ddcce8 to 08fbd20 Compare September 21, 2018 20:25

halseth reviewed Sep 26, 2018

View reviewed changes

cfromknecht force-pushed the isolate-scanning-ntfns branch 5 times, most recently from e3b585b to 141c5e2 Compare October 1, 2018 10:53

halseth reviewed Oct 1, 2018

View reviewed changes

cfromknecht force-pushed the isolate-scanning-ntfns branch 4 times, most recently from d2f2551 to 5d6d206 Compare October 2, 2018 08:10

wpaulino mentioned this pull request Oct 4, 2018

chainntnfs: handle spend notifications within TxConfNotifier #2004

Merged

halseth suggested changes Oct 4, 2018

View reviewed changes

cfromknecht force-pushed the isolate-scanning-ntfns branch from 5d6d206 to 75a7cd9 Compare October 5, 2018 04:53

cfromknecht and others added 25 commits October 26, 2018 18:32

chainntnfs/txnotifier_test: update nil spend details to restore tests

012d17e

chainntnfs/txconfnotifier: add rescanStates

2f0b559

chainntnfs/txconfnotifier: isolate scanning ntfns

1babec9

chainntnfs/bitcoind+btcd+neutrino: pass nil conf details

a1756b0

chainntnfs/txconfnotifier: query conf hint in Register

8b8007b

chainntnfs/bitcoind+btcd+neutrino: let tcn query for height hint

cf7700e

chainntnfs/txconfnotifier_test: update to use multi-value Register

e804b30

chainntnfs/txconfnotifier: add HistoricalConfDispatch struct

37c864d

chainntnfs/txconfnotifier: add PkScript to ConfNtfn

74122e0

chainntnfs/txconfnotifier: return HistoricalConfDispatch from Register

f94de23

chainntnfs/txconfnotifier: return HistoricalConfDispatch from Register

217b1fc

chainntnfs/txconfnotifier: split out ntfn dispatch into helper

9ae6d43

chainntnfs/txconfnotifier: remove clientID from UpdateConfDetails sig…

32e7368

…nature

chainntnfs/txconfnotifier_test: remove clientID argument...

11c231d

to UpdateConfDetails

chainntnfs/bitcoind: use HistoricalConfDispatch in ntfn registry

a4c9f62

chainntnfs/btcd: use HistoricalConfDispatch in ntfn registry

6cd0f86

chainntnfs/neutrino: use HistoricalConfDispatch in ntfn registry

df9bb56

chainntnfs/txconnotifier: add debug logs, log errs/warnings, godocs

eee5311

chainntnfs/tx_notifier: extract conf reorg dispatch into method

589dc96

chainntnfs/neutrinonotify/neutrino: fix debug logs

ba28ec3

cfromknecht dismissed halseth’s stale review via ba28ec3 October 27, 2018 01:32

cfromknecht force-pushed the isolate-scanning-ntfns branch from ce395f4 to ba28ec3 Compare October 27, 2018 01:32

Roasbeef approved these changes Oct 27, 2018

View reviewed changes

Roasbeef merged commit eaba39d into lightningnetwork:master Oct 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chainntnfs: Isolate conf notifications during historical scans #1787

chainntnfs: Isolate conf notifications during historical scans #1787

cfromknecht commented Aug 25, 2018 •

edited

Loading

halseth commented Sep 20, 2018

cfromknecht commented Sep 20, 2018

halseth left a comment

halseth Sep 26, 2018

halseth Oct 1, 2018

halseth Oct 1, 2018

halseth commented Oct 4, 2018

halseth left a comment

halseth Oct 4, 2018

cfromknecht Oct 5, 2018

cfromknecht Oct 5, 2018

cfromknecht commented Oct 27, 2018

Roasbeef left a comment

chainntnfs: Isolate conf notifications during historical scans #1787

chainntnfs: Isolate conf notifications during historical scans #1787

Conversation

cfromknecht commented Aug 25, 2018 • edited Loading

halseth commented Sep 20, 2018

cfromknecht commented Sep 20, 2018

halseth left a comment

Choose a reason for hiding this comment

halseth Sep 26, 2018

Choose a reason for hiding this comment

halseth Oct 1, 2018

Choose a reason for hiding this comment

halseth Oct 1, 2018

Choose a reason for hiding this comment

halseth commented Oct 4, 2018

halseth left a comment

Choose a reason for hiding this comment

halseth Oct 4, 2018

Choose a reason for hiding this comment

cfromknecht Oct 5, 2018

Choose a reason for hiding this comment

cfromknecht Oct 5, 2018

Choose a reason for hiding this comment

cfromknecht commented Oct 27, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

cfromknecht commented Aug 25, 2018 •

edited

Loading