From 51c066e7edcd36eb7e306e5de7a8a679e54382b3 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 10 Oct 2019 09:32:38 +0200 Subject: [PATCH 1/2] chainntnfs: don't register notifications twice if details exist --- chainntnfs/txnotifier.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index becdb9b1..8c16966f 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -103,6 +103,9 @@ type confNtfnSet struct { // details serves as a cache of the confirmation details of a // transaction that we'll use to determine if a transaction/output // script has already confirmed at the time of registration. + // details is also used to make sure that in case of an address reuse + // (funds sent to a previously confirmed script) no additional + // notification is registered which would lead to an inconsistent state. details *TxConfirmation } @@ -1507,6 +1510,15 @@ func (n *TxNotifier) handleConfDetailsAtTip(confRequest ConfRequest, // TODO(wilmer): cancel pending historical rescans if any? confSet := n.confNotifications[confRequest] + + // If we already have details for this request, we don't want to add it + // again since we have already dispatched notifications for it. + if confSet.details != nil { + Log.Warnf("Ignoring address reuse for %s at height %d.", + confRequest, details.BlockHeight) + return + } + confSet.rescanStatus = rescanComplete confSet.details = details From fa948c23feb546013f400f1fce7e1caceccb373b Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 2 Oct 2019 13:56:59 +0200 Subject: [PATCH 2/2] chainntnfs: add test that validates fixed nil pointer dereference --- chainntnfs/txnotifier_test.go | 160 ++++++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/chainntnfs/txnotifier_test.go b/chainntnfs/txnotifier_test.go index e7f9664d..1e5c267d 100644 --- a/chainntnfs/txnotifier_test.go +++ b/chainntnfs/txnotifier_test.go @@ -604,6 +604,166 @@ func TestTxNotifierFutureSpendDispatch(t *testing.T) { } } +// TestTxNotifierFutureConfDispatchReuseSafe tests that the notifier does not +// misbehave even if two confirmation requests for the same script are issued +// at different block heights (which means funds are being sent to the same +// script multiple times). +func TestTxNotifierFutureConfDispatchReuseSafe(t *testing.T) { + t.Parallel() + + currentBlock := uint32(10) + hintCache := newMockHintCache() + n := chainntnfs.NewTxNotifier( + currentBlock, 2, hintCache, hintCache, + ) + + // We'll register a TX that sends to our test script and put it into a + // block. Additionally we register a notification request for just the + // script which should also be confirmed with that block. + tx1 := wire.MsgTx{Version: 1} + tx1.AddTxOut(&wire.TxOut{PkScript: testRawScript}) + tx1Hash := tx1.TxHash() + ntfn1, err := n.RegisterConf(&tx1Hash, testRawScript, 1, 1) + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + scriptNtfn1, err := n.RegisterConf(nil, testRawScript, 1, 1) + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + block := btcutil.NewBlock(&wire.MsgBlock{ + Transactions: []*wire.MsgTx{&tx1}, + }) + currentBlock++ + err = n.ConnectTip(block.Hash(), currentBlock, block.Transactions()) + if err != nil { + t.Fatalf("unable to connect block: %v", err) + } + if err := n.NotifyHeight(currentBlock); err != nil { + t.Fatalf("unable to dispatch notifications: %v", err) + } + + // Expect an update and confirmation of TX 1 at this point. We save the + // confirmation details because we expect to receive the same details + // for all further registrations. + var confDetails *chainntnfs.TxConfirmation + select { + case <-ntfn1.Event.Updates: + default: + t.Fatal("expected update of TX 1") + } + select { + case confDetails = <-ntfn1.Event.Confirmed: + if confDetails.BlockHeight != currentBlock { + t.Fatalf("expected TX to be confirmed in latest block") + } + default: + t.Fatal("expected confirmation of TX 1") + } + + // The notification for the script should also have received a + // confirmation. + select { + case <-scriptNtfn1.Event.Updates: + default: + t.Fatal("expected update of script ntfn") + } + select { + case details := <-scriptNtfn1.Event.Confirmed: + assertConfDetails(t, details, confDetails) + default: + t.Fatal("expected update of script ntfn") + } + + // Now register a second TX that spends to two outputs with the same + // script so we have a different TXID. And again register a confirmation + // for just the script. + tx2 := wire.MsgTx{Version: 1} + tx2.AddTxOut(&wire.TxOut{PkScript: testRawScript}) + tx2.AddTxOut(&wire.TxOut{PkScript: testRawScript}) + tx2Hash := tx2.TxHash() + ntfn2, err := n.RegisterConf(&tx2Hash, testRawScript, 1, 1) + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + scriptNtfn2, err := n.RegisterConf(nil, testRawScript, 1, 1) + if err != nil { + t.Fatalf("unable to register ntfn: %v", err) + } + block2 := btcutil.NewBlock(&wire.MsgBlock{ + Transactions: []*wire.MsgTx{&tx2}, + }) + currentBlock++ + err = n.ConnectTip(block2.Hash(), currentBlock, block2.Transactions()) + if err != nil { + t.Fatalf("unable to connect block: %v", err) + } + if err := n.NotifyHeight(currentBlock); err != nil { + t.Fatalf("unable to dispatch notifications: %v", err) + } + + // Transaction 2 should get a confirmation here too. Since it was + // a different TXID we wouldn't get the cached details here but the TX + // should be confirmed right away still. + select { + case <-ntfn2.Event.Updates: + default: + t.Fatal("expected update of TX 2") + } + select { + case details := <-ntfn2.Event.Confirmed: + if details.BlockHeight != currentBlock { + t.Fatalf("expected TX to be confirmed in latest block") + } + default: + t.Fatal("expected update of TX 2") + } + + // The second notification for the script should also have received a + // confirmation. Since it's the same script, we expect to get the cached + // details from the first TX back immediately. Nothing should be + // registered at the notifier for the current block height for that + // script any more. + select { + case <-scriptNtfn2.Event.Updates: + default: + t.Fatal("expected update of script ntfn") + } + select { + case details := <-scriptNtfn2.Event.Confirmed: + assertConfDetails(t, details, confDetails) + default: + t.Fatal("expected update of script ntfn") + } + + // Finally, mine a few empty blocks and expect both TXs to be confirmed. + for currentBlock < 15 { + block := btcutil.NewBlock(&wire.MsgBlock{}) + currentBlock++ + err = n.ConnectTip( + block.Hash(), currentBlock, block.Transactions(), + ) + if err != nil { + t.Fatalf("unable to connect block: %v", err) + } + if err := n.NotifyHeight(currentBlock); err != nil { + t.Fatalf("unable to dispatch notifications: %v", err) + } + } + + // Events for both confirmation requests should have been dispatched. + select { + case <-ntfn1.Event.Done: + default: + t.Fatal("expected notifications for TX 1 to be done") + } + select { + case <-ntfn2.Event.Done: + default: + t.Fatal("expected notifications for TX 2 to be done") + } +} + // TestTxNotifierHistoricalSpendDispatch tests that the TxNotifier dispatches // registered notifications when an outpoint is spent before registration. func TestTxNotifierHistoricalSpendDispatch(t *testing.T) {