Historical block epoch ntfns #1439

valentinewallace · 2018-06-24T19:39:58Z

Modifies RegisterBlockEpochNtfn to accept an optional parameter bestBlock, a tuple of (bestHeight, bestHash).
Adds a bestBlock field to ChainNotifiers to keep track of the best block they've seen.

This fixes two uncovered situations:

Notification clients being behind on blocks when registering for block notifications. Now, a client can pass in their bestBlock when registering, and the ChainNotifier will send out notifications for the missed blocks.
A chain backend occasionally skips or misses a series of blocks, causing the ChainNotifier to not send out notifications for them. Now, this event is detected and notifications are dispatched for the missing blocks.

Fixes 1188

wpaulino

Nice work! The changes mostly read well to me, although the commit structure could be improved a bit. There are multiple commits that end up modifying the same parts of some files, so those could be squashed together. Also, most of these comments should apply to all backend notifiers, not just bitcoind. Will give it a second pass once the feedback has been addressed!

wpaulino · 2018-06-26T23:23:29Z

chainntnfs/interface.go

@@ -57,7 +57,7 @@ type ChainNotifier interface {
 	// new block connected to the tip of the main chain. The returned
 	// BlockEpochEvent struct contains a channel which will be sent upon
 	// for each new block discovered.
-	RegisterBlockEpochNtfn() (*BlockEpochEvent, error)
+	RegisterBlockEpochNtfn(*BlockEpoch) (*BlockEpochEvent, error)


We should update the godoc to reflect the new changes.

wpaulino · 2018-06-26T23:41:09Z

chainntnfs/bitcoindnotify/bitcoind.go

+		return nil
+	}
+
+	_, currHeight, err := b.chainConn.GetBestBlock()


Should this maybe use the best block contained within the BitcoindNotifier struct itself? It might be the case that GetBestBlock returns a new block that the notifier isn't aware of yet, so the registered client would end up receiving two notifications for the same block.

Good point, i'll update that

wpaulino · 2018-06-27T01:30:48Z

chainntnfs/bitcoindnotify/bitcoind.go

-						"current height=%d, new height=%d",
-						bestHeight, item.Height)
-					continue
+					// Send historical block notifications to clients


We should log that we missed some blocks and we're attempting to recover them.

wpaulino · 2018-06-27T01:34:04Z

chainntnfs/bitcoindnotify/bitcoind.go

+		b.notifyBlockEpochs(height, hash)
+
+		txns := btcutil.NewBlock(rawBlock).Transactions()
+		err = b.txConfNotifier.ConnectTip(hash,


Rather than calling notifyBlockEpochs and ConnectTip, this should call handleBlockConnected instead to also include the handling of spend notifications.

wpaulino · 2018-06-27T01:34:36Z

chainntnfs/bitcoindnotify/bitcoind.go

+	}
+
+	// We want to start catching up at the block after the client's best
+	// known block, to avoid a redundant notification


Comment should be updated as this function does not depend on the clients missing blocks, but rather the ChainNotifier itself missing them.

wpaulino · 2018-06-27T02:11:09Z

chainntnfs/bitcoindnotify/bitcoind.go

+	// If a reorg causes the hash to be incorrect, start from the bestBlock's
+	// height.
+	// Doesn't handle the case where other past blocks are incorrect
+	if bestBlock.Hash == nil || *hashAtBestHeight != *bestBlock.Hash {


This should rewind until the fork point.

wpaulino · 2018-06-27T02:11:34Z

chainntnfs/bitcoindnotify/bitcoind.go

+					b.bestBlockMtx.RLock()
+					bestBlock := b.bestBlock
+					b.bestBlockMtx.RUnlock()
+					b.catchUpOnMissedBlocks(bestBlock)


Handle and log error.

wpaulino · 2018-06-27T02:12:53Z

chainntnfs/bitcoindnotify/bitcoind.go

 				}
 				bestHeight = item.Height

+				b.bestBlockMtx.Lock()
+				b.bestBlock = &chainntnfs.BlockEpoch{


We can just update the best block itself by modifying its members.

wpaulino · 2018-06-27T02:13:25Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -307,6 +325,13 @@ out:
 				}
 				bestHeight = item.Height - 1

+				b.bestBlockMtx.Lock()
+				b.bestBlock = &chainntnfs.BlockEpoch{


Same comment here w.r.t. modifying the best block.

wpaulino · 2018-06-27T02:14:04Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -71,6 +71,9 @@ type BitcoindNotifier struct {

 	blockEpochClients map[uint64]*blockEpochRegistration

+	bestBlockMtx sync.RWMutex
+	bestBlock    *chainntnfs.BlockEpoch


I believe there's no need for this to be a pointer.

cfromknecht

Nice work @valentinewallace! Overall design looks solid, my comments are mostly regarding consistency/safety, order of operations, and behavior under failure conditions. Excited to get this in so we can tighten up all of our subsystems :)

cfromknecht · 2018-07-06T02:32:37Z

chainntnfs/bitcoindnotify/bitcoind.go

+	chainntnfs.Log.Infof("New block: height=%v, sha=%v",
+		height, hash)
+
+	b.notifyBlockEpochs(height, &hash)


We should wait to deliver notifications until after calling ConnectTip. Otherwise we may notify clients before our own internal state has finished processing the connected block.

cfromknecht · 2018-07-06T02:33:17Z

chainntnfs/bitcoindnotify/bitcoind.go

+				blockHash, err := b.chainConn.GetBlockHash(int64(bestHeight))
+				if err != nil {
+					chainntnfs.Log.Warnf("unable to get hash from block "+
+						"with height=%d: %v", bestHeight, err)


Is this safe to unwind if the block hash could not be found?

cfromknecht · 2018-07-06T02:35:24Z

discovery/gossiper.go

+	bestBlock := &chainntnfs.BlockEpoch{Hash: hash, Height: int32(height)}
+	if err != nil {
+		bestBlock = nil
+		log.Warnf("unable to retrieve best block from router: %v", err)


If we encounter an error here, we should prob return the error to indicate a startup failure.

cfromknecht · 2018-07-06T03:34:57Z

chainntnfs/bitcoindnotify/bitcoind.go

+// It returns the height of the nearest common ancestor between the two hashes,
+// or an error
+func (b *BitcoindNotifier) getCommonBlockAncestorHeight(
+	reorgHash chainhash.Hash, chainHash chainhash.Hash) (int32, error) {


go will let you drop the type for consecutive arguments of the same type :) we can write these args as:

func (b *BitcoindNotifier) getCommonBlockAncestorHeight( reorgHash, chainHash chainhash.Hash) (int32, error)

cfromknecht · 2018-07-06T08:47:44Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -263,6 +271,10 @@ out:
 			case *blockEpochRegistration:
 				chainntnfs.Log.Infof("New block epoch subscription")
 				b.blockEpochClients[msg.epochID] = msg
+				err := b.catchUpClientOnBlocks(msg.bestBlock)
+				if err != nil {
+					chainntnfs.Log.Error(err)


might want to return this error to the caller, so that they are aware registration failed

cfromknecht · 2018-07-06T09:10:55Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -71,6 +71,9 @@ type BitcoindNotifier struct {

 	blockEpochClients map[uint64]*blockEpochRegistration

+	bestBlockMtx sync.RWMutex


Is this mutex necessary? Looks like all modifications are inside one goroutine

cfromknecht · 2018-07-06T09:14:16Z

chainntnfs/neutrinonotify/neutrino.go

+			txns:    txns,
+			connect: true,
+		}
+		fmt.Printf("catching up on missed blocks w/ height %d", height)


log instead of print?

cfromknecht · 2018-07-06T09:26:20Z

chainntnfs/bitcoindnotify/bitcoind.go

+	// If a reorg causes the hash to be incorrect, determine the height of the
+	// first common parent block between the client's view of the chain and the
+	// main chain and dispatch notifications from there
+	if bestBlock.Hash != nil && *hashAtBestHeight != *bestBlock.Hash {


block hash being nil seems more like a failure, is this possible?

cfromknecht · 2018-07-06T09:26:46Z

chainntnfs/bitcoindnotify/bitcoind.go

+			return 0, fmt.Errorf("unable to get header for hash=%v: %v", chainHash, err)
+		}
+		reorgHash = reorgHeader.PrevBlock
+		chainHash = chainHeader.PrevBlock


very clean impl, me likes

cfromknecht · 2018-07-06T19:32:00Z

utxonursery.go

+	if err != nil {
+		return err
+	}
+	newBlockChan, err := u.cfg.Notifier.RegisterBlockEpochNtfn(


is this equivalent to just passing in nil? down the road we'll want to pull this value from the db and pass it in, though that is better left for a later PR as might require migration

wpaulino

This is getting pretty close! The most important thing is that we ensure we don't notify all active clients if only one needs to be caught up. Other than that, most of my comments are pretty minor.

One thing I noticed while running the tests locally, is that it's possible to run into a race condition within testCatchUpOnMissedBlocksWithReorg. This can be solved by guarding the bestBlock with a mutex.

wpaulino · 2018-07-24T22:51:10Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -263,62 +270,58 @@ out:
 			case *blockEpochRegistration:
 				chainntnfs.Log.Infof("New block epoch subscription")
 				b.blockEpochClients[msg.epochID] = msg
+				if err := b.catchUpClientOnBlocks(msg.bestBlock); err != nil {
+					msg.errorChan <- err
+					chainntnfs.Log.Error(err)


No need to log this error as it'll be returned to the caller.

wpaulino · 2018-07-24T23:09:17Z

chainntnfs/bitcoindnotify/bitcoind.go

+			return fmt.Errorf("unable to find blockhash for height=%d: %v",
+				height, err)
+		}
+		b.notifyBlockEpochs(height, hash)


This will dispatch notifications to all active clients. Instead, we want to make sure we send the missed block notifications only to the client that requested them, which we track internally through epochID. Ideally this lives under its own method.

wpaulino · 2018-07-24T23:11:16Z

chainntnfs/bitcoindnotify/bitcoind.go

+// It returns the height of the nearest common ancestor between the two hashes,
+// or an error
+func (b *BitcoindNotifier) getCommonBlockAncestorHeight(
+	reorgHash, chainHash chainhash.Hash) (int32, error) {


Should add a newline after.

wpaulino · 2018-07-24T23:14:38Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	if err != nil {
+		t.Fatalf("unable to generate blocks: %v", err)
+	}
+	time.Sleep(5 * time.Second)


What's the rationale behind this?

wpaulino · 2018-07-24T23:37:59Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	// We set the notifier's best block to be the last block mined on the
+	// shorter chain, to test that the notifier correctly rewinds to
+	// the common ancestor between the two chains.
+	time.Sleep(time.Second * 10)


Same comment w.r.t. the sleep.

wpaulino · 2018-07-24T23:54:32Z

chainntnfs/interface.go

@@ -55,7 +55,12 @@ type ChainNotifier interface {
 	// new block connected to the tip of the main chain. The returned
 	// BlockEpochEvent struct contains a channel which will be sent upon
 	// for each new block discovered.
-	RegisterBlockEpochNtfn() (*BlockEpochEvent, error)
+	//
+	// Clients have the option of passing in their best known block.


wpaulino · 2018-07-24T23:55:16Z

chainntnfs/interface_test.go

+	// So we'll use a WaitGroup to synchronize the test.
+	for i := 0; i < numClients; i++ {
+		epochClient, err := notifier.RegisterBlockEpochNtfn(
+			&chainntnfs.BlockEpoch{


Could pull this out into its own var outdatedBlock.

wpaulino · 2018-07-24T23:57:41Z

chainntnfs/neutrinonotify/neutrino.go

-		n.heightMtx.RLock()
-		currentHeight := n.bestHeight
-		n.heightMtx.RUnlock()
+		n.bestBlockMtx.RLock()


I know this was the previous behavior, but this should ideally be changed so that the lock is outside of the loop.

Why move it outside? Then we'd be sleeping with the lock held

I think we'd also enter an infinite loop, since the notifier couldn't acquire the write lock to change it and break out

wpaulino · 2018-07-25T00:00:22Z

chainntnfs/neutrinonotify/neutrino.go

 // transactions included this block will processed to either send notifications
 // now or after numConfirmations confs.
 func (n *NeutrinoNotifier) handleBlockConnected(newBlock *filteredBlock) error {
-	// First we'll notify any subscribed clients of the block.
+


Extra newline.

wpaulino · 2018-07-25T00:09:37Z

chainntnfs/neutrinonotify/neutrino.go

-	heightMtx  sync.RWMutex
-	bestHeight uint32
+	bestBlockMtx sync.RWMutex
+	bestBlock    chainntnfs.BlockEpoch


It doesn't seem like we gain anything from also keeping track of our best hash as we can't rewind until the common ancestor, so this probably isn't needed.

cfromknecht

Looking great on this PR, very happy with latest changes 👌 looking forward to these changes landing in master! Great work on the fixes for external consistency, reorg handling, missed blocks, etc ✨

My lingering comments are mainly related to code duplication in tests, can these be consolidated into the interface_test?

cfromknecht · 2018-07-25T01:43:05Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	}
+}
+
+func testCatchUpOnMissedBlocksWithReorg(miner *rpctest.Harness,


Could these tests be moved to chainntnfs/interface_test.go? Would be great if all impls share a single test suite as much as possible. Am I missing something that requires them being distinct?

My thought for how to test a notifier's ability to handle missed blocks was:

Generate some blocks

Assign the notifier's bestBlock to be a past block

Generate a new block, forcing the catchup logic to be triggered

Reassigning a notifier's bestBlock requires belonging to its package, hence why the current implementation wouldn't work in interface_test.
But I'm not sure that's the best way, so open to hearing other ideas!

cfromknecht · 2018-07-25T01:43:44Z

chainntnfs/bitcoindnotify/bitcoind.go

+					chainntnfs.Log.Error(err)
+					continue
+				}
+				msg.errorChan <- nil


cfromknecht · 2018-07-25T01:45:42Z

chainntnfs/bitcoindnotify/bitcoind.go

+		return fmt.Errorf("unable to find common ancestor: %v", err)
+	}
+
+	numMissedBlocks := b.bestBlock.Height - commonAncestorHeight


looks like this value is unused? similar comment for other numMissedBlocks

cfromknecht · 2018-07-25T01:50:53Z

chainntnfs/neutrinonotify/neutrino.go

-		n.heightMtx.RLock()
-		currentHeight := n.bestHeight
-		n.heightMtx.RUnlock()
+		n.bestBlockMtx.RLock()


Why move it outside? Then we'd be sleeping with the lock held

I think we'd also enter an infinite loop, since the notifier couldn't acquire the write lock to change it and break out

cfromknecht · 2018-07-25T01:56:21Z

chainntnfs/bitcoindnotify/bitcoind.go

+		}
+
+		chainntnfs.Log.Infof("Block disconnected from main chain: "+
+			"height=%v, sha=%v", height, hash)


Think this should print bestBlock.Hash, as hash is the previous block :)

cfromknecht · 2018-07-25T02:11:18Z

chainntnfs/bitcoindnotify/bitcoind.go

+// (2) the hash of the block of the same height from the main chain
+// It returns the height of the nearest common ancestor between the two hashes,
+// or an error
+func (b *BitcoindNotifier) getCommonBlockAncestorHeight(


Since this function doesn't modify any internal state, this could be moved into the chainntnfs package and exposed as a helper method. I think btcd and bitcoind can share this method since their chainConn's both implement chain.Interface as:

height, err := chainntnfs.GetCommonBlockAncestorHeight( b.chainConn, reorgHash, chainHash, )

Lol, love dis idea. I think btcd's chainConn might not fit, though. Looks like rpcclient.Client is missing a few of the chain.Interface's methods?

If you just need GetBlockHeader and GetBlockHeaderVerbose, you can create a new interface only with these two, and pass in :)

halseth

This looks great! My main concern is that the code duplication is getting out of hand in this package (I'm guilty 🙊🙈). If we could try to extract as much common logic as possible into reusable methods, that would help a lot.

Also, commits should be squashed to make it easier to review :)

halseth · 2018-07-25T12:25:38Z

chainntnfs/bitcoindnotify/bitcoind.go

+// is behind our current best block.
+// It dispatches block notifications for all the blocks missed by the client
+// or does nothing if the client doesn't send us its best block.
+func (b *BitcoindNotifier) catchUpClientOnBlocks(bestBlock *chainntnfs.BlockEpoch) error {


looks like also can be extracted into a common method that takes the chainConn?

halseth · 2018-07-25T12:26:16Z

chainntnfs/bitcoindnotify/bitcoind.go

+// handleMissedBlocks is called when the chain backend misses a series of
+// blocks. It dispatches all relevant notifications for the missed blocks,
+// handling a reorg if necessary.
+func (b *BitcoindNotifier) handleMissedBlocks(newHeight int32) error {


looks like also can be extracted into a common method that takes the chainConn?

halseth · 2018-07-25T12:26:26Z

chainntnfs/bitcoindnotify/bitcoind.go

+// rewindChain handles internal state updates associated with disconnected blocks.
+// It has no effect if given a height greater than or equal to our current best
+// known height.
+func (b *BitcoindNotifier) rewindChain(newBestHeight int32) error {


looks like also can be extracted into a common method that takes the chainConn?

halseth · 2018-07-27T17:21:57Z

.githooks/pre-commit

@@ -0,0 +1,28 @@
+#!/bin/sh


When I said "squash the commits" I really meant squash commits that are altering the same files/methods together, sorry! 😅

No biggie though, but changes like this file would be nice to have in its own commit with a commit message that explains what it is.

halseth · 2018-07-27T17:23:32Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -1,3 +1,5 @@
+// +build !debug


Looks like this is in a WIP state? Let me know when it is ready for another round of review 😄

cfromknecht

Pumped about this refactoring, really like the neutrino conn using the chainntnfs helpers! In addition to Johan’s comments on commits, I’ve got a few more comments on tests/docs. The notifiers themselves look correct to me, just want verify that with a few more assertions :)

Two things that come to mind are checking the heights clients see, and also that we don’t send any extra epochs

cfromknecht · 2018-07-28T03:57:26Z

chainntnfs/interface.go

+	if numMissedBlocks < 0 {
+		numMissedBlocks = 0
+	}
+	missedBlocks := make([]BlockEpoch, numMissedBlocks)


This will allocate numMissedBlocks blank BlockEpochs, when we append later these will still be there as the prefix.

To allocate with 0 length, but capacity ofnumMissedBlocks use

missedBlocks := make([]BlockEpoch, 0, numMissedBlocks)

ah that's a great catch haha, done :)

cfromknecht · 2018-07-28T04:36:57Z

chainntnfs/interface_test.go

+		wg.Add(numBlocks)
+		go func() {
+			for i := 0; i < numBlocks; i++ {
+				<-epochClient.Epochs


We may want to check that the heights are emitted correctly, and also that the client doesn’t emit too many blocks

cfromknecht · 2018-07-28T04:49:21Z

chainntnfs/interface_test.go

@@ -1159,7 +1218,185 @@ func testCancelEpochNtfn(node *rpctest.Harness, notifier chainntnfs.ChainNotifie
 	}
 }

-func testReorgConf(miner *rpctest.Harness, notifier chainntnfs.ChainNotifier,
+func testCatchUpOnMissedBlocks(miner1 *rpctest.Harness,


add godoc comment with high level explanation of tests (and others)

cfromknecht · 2018-07-28T04:50:58Z

chainntnfs/bitcoindnotify/bitcoind_debug.go

+// UnsafeStart starts the notifier with a specified best block.
+// Both its bestBlock and its txConfNotifier are initialized with bestBlock.
+// Used for testing.
+func (b *BitcoindNotifier) UnsafeStart(bestBlock chainntnfs.BlockEpoch) error {


cfromknecht · 2018-07-28T04:51:40Z

chainntnfs/interface.go

+// ChainConn enables notifiers to pass in their chain backend
+// to interface functions that require it.
+type ChainConn interface {
+	GetBlockHeader(blockHash *chainhash.Hash) (*wire.BlockHeader, error)


add godoc comments

cfromknecht · 2018-07-28T04:54:12Z

chainntnfs/interface_debug.go

+type TestChainNotifier interface {
+	ChainNotifier
+
+	UnsafeStart(BlockEpoch) error


Add godoc here, also make that comments start with iface/function name

cfromknecht · 2018-07-28T04:55:52Z

chainntnfs/neutrinonotify/neutrino.go

+// NeutrinoChainConn implements chainntfs.ChainConn,
+// allowing it to be passed into chainntnfs functions that require
+// a ChainConn
+type NeutrinoChainConn struct {


Nice addition 😎

Roasbeef · 2018-08-10T03:56:52Z

Yay sigs! Needs rebase!

Roasbeef

Excellent work! The extra effort to introduce the additional set of abstractions and launch modes really paid off as we're able to test this set of changes so extensively. After this PR lands, lnd will be much more robust in the face of partial failures between itself and the backend source. The code duplication that has existed within this package has gotten a bit worse as a result of this PR (to no fault of your own). Before we do additional work on this (well after the persistent height hint PR), we'll want to aggressively refactor this code in order to eliminate the duplication across each of the backends.

Once this has been rebased: LGTM ⚡️

Roasbeef · 2018-08-10T04:34:12Z

chainntnfs/btcdnotify/btcd.go

+	// First process the block for our internal state. A new block has
+	// been connected to the main chain. Send out any N confirmation
+	// notifications which may have been triggered by this new block.
+	rawBlock, err := b.chainConn.GetBlock(epoch.Hash)


Why are we fetching the block again? We already fetch the raw block here: https://github.com/lightningnetwork/lnd/blob/6d62f12eb19808aac6f312240ea4c9995f65919d/chainntnfs/btcdnotify/btcd.go#L385

I believe it saves a bit of code duplication since otherwise we'd have to do the same filtered block construction here: https://github.com/lightningnetwork/lnd/pull/1439/files#diff-0bfcf75d8a29f4e9d616031ff3e5a805R400 [because HandleMissedBlocks just returns a list of BlockEpochs]

Clients can optionally pass their best block known into RegisterBlockEpochNtfn. This enables the notifiers to catch up clients on blocks they may have missed.

This allows notifiers to pass their chain backend into interface functions to retrieve information from the chain.

If a client passes in their best known block when registering for block notifications, check to see if it's behind our best block. If so, dispatch the missed block notifications to the client. This is necessary because clients that persist their best known block can miss new blocks while registering for notifications.

If the chain backend misses telling the notifier about a series of disconnected blocks, the notifier is now able to disconnect the tip to its new best block.

This prevents the situation where we notify clients about a newly connected block, and then the block connection itself fails. We also want to set our best block in between connecting the block and notifying clients, in case a client makes queries about the new block they have received.

This resolves the situation where a notifier's chain backend skips a series of blocks, causing the notifier to need to dispatch historical block notifications to clients. Additionally, if the current notifier's best block has been reorged out, this logic enables the notifier to rewind to the common ancestor between the current chain and the outdated best block and dispatches notifications from the ancestor.

TestChainNotifier wraps the ChainNotifier interface to allow adding additional testing methods with access to private fields in the notifiers. These testing methods are only compiled when the build tag "debug" is set. UnsafeStart allows starting a notifier with a specified best block. UnsafeStart is useful for the purpose of testing cases where a notifier's best block is out of date when it receives a new block.

Switches all ChainNotifier parameters to be TestChainNotifiers. This allows access to the extra testing methods provided by the TestChainNotifier interface.

This tests the case where a client registers for block notifications with an outdated best block, to ensure that the client is properly caught up on the blocks that it has missed.

Tests for the case where a chain backend skips a series of blocks, such that the notifier's best block is out of date. Also tests the case where a notifier's best block has been reorged out of the chain.

cfromknecht

I like the refactoring of UnsafeStart to generate the blocks inside each notifier, definitely looks like it would be more reliable than our prior design. LGTM! 🌱

Roasbeef

LGTM 🏄🏾

bvolpato · 2019-01-10T19:51:02Z

Sounds like I got a scenario recently, where one block was missed and the chain wasn't able to continue after that without a restart.

I added more details to a previous card: #1597 (comment)
Please take a look to check if it's related to this improvement.

cc @Roasbeef @valentinewallace

wpaulino suggested changes Jun 27, 2018

View reviewed changes

valentinewallace force-pushed the historical-block-epoch-ntfns branch from 884c51d to 7229b68 Compare July 4, 2018 00:52

cfromknecht suggested changes Jul 6, 2018

View reviewed changes

Roasbeef added notifications P2 should be fixed if one has time needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet labels Jul 11, 2018

cfromknecht added the first pass review done PR has had first pass of review, needs more tho label Jul 11, 2018

valentinewallace force-pushed the historical-block-epoch-ntfns branch 5 times, most recently from 955dcaf to 90497d7 Compare July 19, 2018 20:01

Roasbeef mentioned this pull request Jul 21, 2018

lnd stopped receiving updates from bitcoind zmq #1597

Closed

valentinewallace force-pushed the historical-block-epoch-ntfns branch from 90497d7 to b450f91 Compare July 24, 2018 11:23

wpaulino suggested changes Jul 25, 2018

View reviewed changes

cfromknecht reviewed Jul 25, 2018

View reviewed changes

halseth reviewed Jul 25, 2018

View reviewed changes

valentinewallace force-pushed the historical-block-epoch-ntfns branch 7 times, most recently from 6891917 to ddeb70f Compare July 27, 2018 11:57

halseth reviewed Jul 27, 2018

View reviewed changes

valentinewallace force-pushed the historical-block-epoch-ntfns branch from ddeb70f to ead04da Compare July 27, 2018 23:05

cfromknecht reviewed Jul 28, 2018

View reviewed changes

valentinewallace force-pushed the historical-block-epoch-ntfns branch 2 times, most recently from 606cb43 to acfd061 Compare July 29, 2018 11:42

valentinewallace force-pushed the historical-block-epoch-ntfns branch 5 times, most recently from fbc7cdf to 695a91e Compare August 9, 2018 07:06

Roasbeef previously approved these changes Aug 10, 2018

View reviewed changes

valentinewallace dismissed Roasbeef’s stale review via 9a41475 August 10, 2018 06:09

valentinewallace force-pushed the historical-block-epoch-ntfns branch 2 times, most recently from 9a41475 to c524364 Compare August 10, 2018 07:40

valentinewallace added 9 commits August 10, 2018 01:08

chainntnfs: allow clients to pass in best block

71a81f5

Clients can optionally pass their best block known into RegisterBlockEpochNtfn. This enables the notifiers to catch up clients on blocks they may have missed.

multi: update to latest RegisterBlockEpochNtfn interface

1ffc3bb

chainntnfs: track best block in btcd and bitcoind

d4cf271

chainntnfs/interface: add ChainConn interface

f400517

This allows notifiers to pass their chain backend into interface functions to retrieve information from the chain.

chainntnfs/neutrino: add neutrino ChainConn implementation

02ee565

chainntnfs: rewind chain on missed disconnected blocks

cbf1799

If the chain backend misses telling the notifier about a series of disconnected blocks, the notifier is now able to disconnect the tip to its new best block.

valentinewallace force-pushed the historical-block-epoch-ntfns branch 2 times, most recently from bd58116 to d98c0b7 Compare August 11, 2018 01:23

valentinewallace added 4 commits August 11, 2018 22:45

chainntnfs/interface_test: utilize TestChainNotifier interface in tests

f6ba640

Switches all ChainNotifier parameters to be TestChainNotifiers. This allows access to the extra testing methods provided by the TestChainNotifier interface.

chainntnfs/interface_test: add test for client catchup on block ntfns

dea22eb

This tests the case where a client registers for block notifications with an outdated best block, to ensure that the client is properly caught up on the blocks that it has missed.

chaintnfs/interface_test: test notifiers handling missed blocks

fb7deac

Tests for the case where a chain backend skips a series of blocks, such that the notifier's best block is out of date. Also tests the case where a notifier's best block has been reorged out of the chain.

valentinewallace force-pushed the historical-block-epoch-ntfns branch from d98c0b7 to fb7deac Compare August 12, 2018 05:51

cfromknecht approved these changes Aug 13, 2018

View reviewed changes

Roasbeef approved these changes Aug 14, 2018

View reviewed changes

Roasbeef merged commit 5efc122 into lightningnetwork:master Aug 14, 2018

		@@ -71,6 +71,9 @@ type BitcoindNotifier struct {

		blockEpochClients map[uint64]*blockEpochRegistration

		bestBlockMtx sync.RWMutex

Historical block epoch ntfns #1439

Historical block epoch ntfns #1439

Conversation

valentinewallace commented Jun 24, 2018 • edited by Roasbeef Loading

wpaulino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Aug 10, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

valentinewallace commented Jun 24, 2018 •

edited by Roasbeef

Loading

valentinewallace Jun 29, 2018 •

edited

Loading

valentinewallace Jul 25, 2018 •

edited

Loading

valentinewallace Aug 10, 2018 •

edited

Loading