From 2783bd59cae757d4a8f6e0a4109eb99753fe2836 Mon Sep 17 00:00:00 2001 From: Sam Lewis Date: Thu, 9 Nov 2017 18:10:15 +1000 Subject: [PATCH 1/3] chainntnfs: Add chainntfs lazy consumer test This test adds a test for a consumer that registers for a transaction confirmation but takes some time to check if that confirmation has occured. The test reveals a race condition that can cause btcdnotify to add a confirmation entry to its internal heap twice. If the notification consumer is not prompt in reading from the confirmation channel, this can cause the notifier to block indefinitely. --- chainntnfs/interface_test.go | 84 ++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/chainntnfs/interface_test.go b/chainntnfs/interface_test.go index c55f3bb4918..31846bc0845 100644 --- a/chainntnfs/interface_test.go +++ b/chainntnfs/interface_test.go @@ -604,6 +604,86 @@ func testTxConfirmedBeforeNtfnRegistration(miner *rpctest.Harness, } } +// Test the case of a notification consumer having forget or being delayed in +// checking for a confirmation. This should not cause the notifier to stop +// working +func testLazyNtfnConsumer(miner *rpctest.Harness, + notifier chainntnfs.ChainNotifier, t *testing.T) { + + // Create a transaction to be notified about. We'll register for + // notifications on this transaction but won't be prompt in checking them + txid, err := getTestTxId(miner) + if err != nil { + t.Fatalf("unable to create test tx: %v", err) + } + + _, currentHeight, err := miner.Node.GetBestBlock() + if err != nil { + t.Fatalf("unable to get current height: %v", err) + } + + numConfs := uint32(3) + + // Add a block right before registering, this makes race conditions + // between the historical dispatcher and the normal dispatcher more obvious + if _, err := miner.Node.Generate(1); err != nil { + t.Fatalf("unable to generate blocks: %v", err) + } + + firstConfIntent, err := notifier.RegisterConfirmationsNtfn(txid, numConfs, + uint32(currentHeight)) + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + + // Generate another 2 blocks, this should dispatch the confirm notification + if _, err := miner.Node.Generate(2); err != nil { + t.Fatalf("unable to generate blocks: %v", err) + } + + // Now make another transaction, just because we haven't checked to see + // if the first transaction has confirmed doesn't mean that we shouldn't + // be able to see if this transaction confirms first + txid, err = getTestTxId(miner) + if err != nil { + t.Fatalf("unable to create test tx: %v", err) + } + + _, currentHeight, err = miner.Node.GetBestBlock() + if err != nil { + t.Fatalf("unable to get current height: %v", err) + } + + numConfs = 1 + + secondConfIntent, err := notifier.RegisterConfirmationsNtfn(txid, numConfs, + uint32(currentHeight)) + + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + + if _, err := miner.Node.Generate(1); err != nil { + t.Fatalf("unable to generate blocks: %v", err) + } + + select { + case <-secondConfIntent.Confirmed: + // Successfully receive the second notification + break + case <-time.After(30 * time.Second): + t.Fatalf("Second confirmation notification never received") + } + + // Make sure the first tx confirmed successfully + select { + case <-firstConfIntent.Confirmed: + break + case <-time.After(30 * time.Second): + t.Fatalf("First confirmation notification never received") + } +} + // Tests the case in which a spend notification is requested for a spend that // has already been included in a block. In this case, the spend notification // should be dispatched immediately. @@ -900,6 +980,10 @@ var ntfnTests = []testCase{ name: "cancel epoch ntfn", test: testCancelEpochNtfn, }, + { + name: "lazy ntfn consumer", + test: testLazyNtfnConsumer, + }, } // TestInterfaces tests all registered interfaces with a unified set of tests From addee0e29b728e9375745e34744e0b94112eac41 Mon Sep 17 00:00:00 2001 From: Sam Lewis Date: Thu, 9 Nov 2017 18:18:00 +1000 Subject: [PATCH 2/3] chainntnfs: Fix btcdnotify dispatch race condition This race condition can occur if a transaction is included in a block right when a notification is being added to the notifier for it AND when the confirmation requires > 1 confirmations. In this case, the confirmation gets added to the confirmation heap twice. --- chainntnfs/btcdnotify/btcd.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index a6b305afe44..7001c59dff0 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -418,6 +418,8 @@ out: // attemptHistoricalDispatch tries to use historical information to decide if a // notification ca be dispatched immediately, or is partially confirmed so it // can skip straight to the confirmations heap. +// +// Returns true if the transaction was either partially or completely confirmed func (b *BtcdNotifier) attemptHistoricalDispatch(msg *confirmationsNotification, currentHeight int32) bool { @@ -489,7 +491,7 @@ func (b *BtcdNotifier) attemptHistoricalDispatch(msg *confirmationsNotification, } heap.Push(b.confHeap, heapEntry) - return false + return true } // notifyBlockEpochs notifies all registered block epoch clients of the newly From 232eeafb645d36d3acc137447683d82966b1ea23 Mon Sep 17 00:00:00 2001 From: Sam Lewis Date: Thu, 9 Nov 2017 21:18:13 +1000 Subject: [PATCH 3/3] chainntfns: Fix off by 1 block height error In the historical dispatch of btcdnotify, the dispatcher checks if a transaction has been included in a block. If this check happens before the notifier has processed the update, it's possible that the currentHeight of the notifier and the currentHeight of the chain might be out of sync which causes an off by one error when calculating a target height for the transaction confirmation. This change uses the height of the block the transaction was found in, rather than the currentHeight that's known by the notifier to eliminate this. --- chainntnfs/btcdnotify/btcd.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index 7001c59dff0..0ef874aab4c 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -295,7 +295,7 @@ out: // If the notification can be partially or // fully dispatched, then we can skip the first // phase for ntfns. - if b.attemptHistoricalDispatch(msg, currentHeight) { + if b.attemptHistoricalDispatch(msg) { continue } @@ -420,8 +420,8 @@ out: // can skip straight to the confirmations heap. // // Returns true if the transaction was either partially or completely confirmed -func (b *BtcdNotifier) attemptHistoricalDispatch(msg *confirmationsNotification, - currentHeight int32) bool { +func (b *BtcdNotifier) attemptHistoricalDispatch( + msg *confirmationsNotification) bool { chainntnfs.Log.Infof("Attempting to trigger dispatch for %v from "+ "historical chain", msg.txid) @@ -482,8 +482,8 @@ func (b *BtcdNotifier) attemptHistoricalDispatch(msg *confirmationsNotification, // Otherwise, the transaction has only been *partially* confirmed, so // we need to insert it into the confirmation heap. - confsLeft := msg.numConfirmations - uint32(tx.Confirmations) - confHeight := uint32(currentHeight) + confsLeft + // Find the block height at which this transaction will be confirmed + confHeight := uint32(block.Height) + msg.numConfirmations - 1 heapEntry := &confEntry{ msg, confDetails,