From 3a12b5867f1079c2125b8b6d2ff87c94d076235f Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 Sep 2020 10:42:09 +0200 Subject: [PATCH 1/3] txnotifier test: use Cancel() directly --- chainntnfs/txnotifier_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/chainntnfs/txnotifier_test.go b/chainntnfs/txnotifier_test.go index b2262d4a..b474a3f9 100644 --- a/chainntnfs/txnotifier_test.go +++ b/chainntnfs/txnotifier_test.go @@ -1159,7 +1159,6 @@ func TestTxNotifierCancelConf(t *testing.T) { if err != nil { t.Fatalf("unable to register spend ntfn: %v", err) } - cancelConfRequest := ntfn2.HistoricalDispatch.ConfRequest // Extend the chain with a block that will confirm both transactions. // This will queue confirmation notifications to dispatch once their @@ -1175,7 +1174,7 @@ func TestTxNotifierCancelConf(t *testing.T) { } // Cancel the second notification before connecting the block. - n.CancelConf(cancelConfRequest, 2) + ntfn2.Event.Cancel() err = n.ConnectTip(block.Hash(), startingHeight+1, block.Transactions()) if err != nil { @@ -1184,7 +1183,7 @@ func TestTxNotifierCancelConf(t *testing.T) { // Cancel the third notification before notifying to ensure its queued // confirmation notification gets removed as well. - n.CancelConf(cancelConfRequest, 3) + ntfn3.Event.Cancel() if err := n.NotifyHeight(startingHeight + 1); err != nil { t.Fatalf("unable to dispatch notifications: %v", err) From 2665836fa321977130173534faebdf42ecb731e0 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 Sep 2020 10:46:12 +0200 Subject: [PATCH 2/3] txnotifier: delete ntfn by confirm height always When we cancel a confirmation request, we should remove the request from the height map regardless of the current height. Otherwise we end up in the situation when the height is reached, the notification is attempted sent which results in a crash. --- chainntnfs/txnotifier.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index 0a8c745d..2e527cfa 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -763,9 +763,7 @@ func (n *TxNotifier) CancelConf(confRequest ConfRequest, confID uint64) { if confSet.details != nil { confHeight := confSet.details.BlockHeight + ntfn.NumConfirmations - 1 - if confHeight <= n.currentHeight { - delete(n.ntfnsByConfirmHeight[confHeight], ntfn) - } + delete(n.ntfnsByConfirmHeight[confHeight], ntfn) } } From df6606d37f51a955905572d099efc714aa04053e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 Sep 2020 10:51:50 +0200 Subject: [PATCH 3/3] txnotifier test: add test for confirmation after cancellation This test addition would cause the txnotifier to crash prior to the previous commit. --- chainntnfs/txnotifier_test.go | 57 ++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/chainntnfs/txnotifier_test.go b/chainntnfs/txnotifier_test.go index b474a3f9..40fc052d 100644 --- a/chainntnfs/txnotifier_test.go +++ b/chainntnfs/txnotifier_test.go @@ -4,6 +4,7 @@ import ( "bytes" "sync" "testing" + "time" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" @@ -1138,7 +1139,7 @@ func TestTxNotifierCancelConf(t *testing.T) { hintCache := newMockHintCache() n := chainntnfs.NewTxNotifier(startingHeight, 100, hintCache, hintCache) - // We'll register three notification requests. The last two will be + // We'll register four notification requests. The last three will be // canceled. tx1 := wire.NewMsgTx(1) tx1.AddTxOut(&wire.TxOut{PkScript: testRawScript}) @@ -1160,6 +1161,12 @@ func TestTxNotifierCancelConf(t *testing.T) { t.Fatalf("unable to register spend ntfn: %v", err) } + // This request will have a three block num confs. + ntfn4, err := n.RegisterConf(&tx2Hash, testRawScript, 3, 1) + if err != nil { + t.Fatalf("unable to register spend ntfn: %v", err) + } + // Extend the chain with a block that will confirm both transactions. // This will queue confirmation notifications to dispatch once their // respective heights have been met. @@ -1217,6 +1224,54 @@ func TestTxNotifierCancelConf(t *testing.T) { default: t.Fatal("expected Confirmed channel to be closed") } + + // Connect yet another block. + block1 := btcutil.NewBlock(&wire.MsgBlock{ + Transactions: []*wire.MsgTx{}, + }) + + err = n.ConnectTip(block1.Hash(), startingHeight+2, block1.Transactions()) + if err != nil { + t.Fatalf("unable to connect block: %v", err) + } + + if err := n.NotifyHeight(startingHeight + 2); err != nil { + t.Fatalf("unable to dispatch notifications: %v", err) + } + + // Since neither it reached the set confirmation height or was + // canceled, nothing should happen to ntfn4 in this block. + select { + case <-ntfn4.Event.Confirmed: + t.Fatal("expected nothing to happen") + case <-time.After(10 * time.Millisecond): + } + + // Now cancel the notification. + ntfn4.Event.Cancel() + select { + case _, ok := <-ntfn4.Event.Confirmed: + if ok { + t.Fatal("expected Confirmed channel to be closed") + } + default: + t.Fatal("expected Confirmed channel to be closed") + } + + // Finally, confirm a block that would trigger ntfn4 confirmation + // hadn't it already been canceled. + block2 := btcutil.NewBlock(&wire.MsgBlock{ + Transactions: []*wire.MsgTx{}, + }) + + err = n.ConnectTip(block2.Hash(), startingHeight+3, block2.Transactions()) + if err != nil { + t.Fatalf("unable to connect block: %v", err) + } + + if err := n.NotifyHeight(startingHeight + 3); err != nil { + t.Fatalf("unable to dispatch notifications: %v", err) + } } // TestTxNotifierCancelSpend ensures that a spend notification after a client