chainntnfs: remove block info from conf detail copy

[guggero]

Noticed this while updating to latest master in another project: If there are two notifications for the same output being registered and the first one does NOT request the block to be included, then the second one also will not receive the block, even if it explicitly requests it.
This is caused by the block being removed from the original confirmation set instead of a copy (as it is done in NotifyHeight and UpdateConfDetails).

Fixes #7501

[yyforyongyu]

Thanks for the fix! LGTM⛵️

[Roasbeef]

Fixes #7501?

[guggero]

Yes! Wasn't aware there was already an issue.

[Roasbeef]

So I tried to write a test to confirm the fix, and after messing with a diff few iterations (conf before registration, conf after, switch order of registration, etc, etc), it still seems to pass:

diff --git a/chainntnfs/test/test_interface.go b/chainntnfs/test/test_interface.go
index 9def356c6..774bb1702 100644
--- a/chainntnfs/test/test_interface.go
+++ b/chainntnfs/test/test_interface.go
@@ -25,6 +25,7 @@ import (
 	"github.com/lightningnetwork/lnd/chainntnfs/btcdnotify"
 	"github.com/lightningnetwork/lnd/chainntnfs/neutrinonotify"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/lnutils"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1709,6 +1710,55 @@ func testCatchUpOnMissedBlocksWithReorg(miner1 *rpctest.Harness,
 	}
 }
 
+// testIncludeBlockAsymmetry tests that if the same output is registered for a
+// notification by two callers, one is able to get a notification with the
+// block and the other one without it.
+func testIncludeBlockAsymmetry(miner *rpctest.Harness,
+	notifier chainntnfs.TestChainNotifier, scriptDispatch bool,
+	t *testing.T) {
+
+	// We'll start by creating a new test transaction, waiting long enough
+	// for it to get into the mempool.
+	txid, pkScript, err := chainntnfs.GetTestTxidAndScript(miner)
+	require.NoError(t, err, "unable to create test tx")
+	if err := chainntnfs.WaitForMempoolTx(miner, txid); err != nil {
+		t.Fatalf("tx not relayed to miner: %v", err)
+	}
+
+	_, currentHeight, err := miner.Client.GetBestBlock()
+	require.NoError(t, err, "unable to get current height")
+
+	// Next, we'll generate a single block, which should confirm the
+	// transaction created above.
+	_, err = miner.Client.Generate(1)
+	require.NoError(t, err, "unable to generate single block")
+
+	numConfs := uint32(1)
+
+	confIntentNoBlock, err := notifier.RegisterConfirmationsNtfn(
+		nil, pkScript, numConfs, uint32(currentHeight),
+	)
+	require.NoError(t, err)
+
+	confNtfnNoBlock, err := lnutils.RecvOrTimeout(
+		confIntentNoBlock.Confirmed, time.Second*5,
+	)
+	require.NoError(t, err)
+	require.True(t, (*confNtfnNoBlock).Block == nil, "block was included")
+
+	confIntentBlock, err := notifier.RegisterConfirmationsNtfn(
+		nil, pkScript, numConfs, uint32(currentHeight),
+		chainntnfs.WithIncludeBlock(),
+	)
+	require.NoError(t, err)
+
+	confNtfnBlock, err := lnutils.RecvOrTimeout(
+		confIntentBlock.Confirmed, time.Second*5,
+	)
+	require.NoError(t, err)
+	require.True(t, (*confNtfnBlock).Block != nil, "block not included")
+}
+
 type txNtfnTestCase struct {
 	name string
 	test func(node *rpctest.Harness, notifier chainntnfs.TestChainNotifier,
@@ -1728,7 +1778,7 @@ type blockCatchupTestCase struct {
 }
 
 var txNtfnTests = []txNtfnTestCase{
-	{
+	/*{
 		name: "single conf ntfn",
 		test: testSingleConfirmationNotification,
 	},
@@ -1771,22 +1821,26 @@ var txNtfnTests = []txNtfnTestCase{
 	{
 		name: "cancel spend ntfn",
 		test: testCancelSpendNtfn,
+	},*/
+	{
+		name: "test include block asymmetry",
+		test: testIncludeBlockAsymmetry,
 	},
 }
 
 var blockNtfnTests = []blockNtfnTestCase{
-	{
+	/*{
 		name: "block epoch",
 		test: testBlockEpochNotification,
 	},
 	{
 		name: "cancel epoch ntfn",
 		test: testCancelEpochNtfn,
-	},
+	},*/
 }
 
 var blockCatchupTests = []blockCatchupTestCase{
-	{
+	/*{
 		name: "catch up client on historical block epoch ntfns",
 		test: testCatchUpClientOnMissedBlocks,
 	},
@@ -1797,7 +1851,7 @@ var blockCatchupTests = []blockCatchupTestCase{
 	{
 		name: "test catch up on missed blocks w/ reorged best block",
 		test: testCatchUpOnMissedBlocksWithReorg,
-	},
+	},*/
 }
 
 // TestInterfaces tests all registered interfaces with a unified set of tests

In the wild, I would observe a panic in lndclient as it would try to deref the block as it assumed it was properly included when in fact it wasn't.

[Roasbeef]

Fix looks good, but having trouble writing a unit test to repro and confirm the fix, see comment below.

[guggero]

@Roasbeef thanks a lot for the patch! That gave me the required inspiration to be able to reproduce.
So the reproduction is a tiny bit more involved than what you attempted. We basically need to trigger the rescan result from a previous client. So I just ended up doing the same notification registrations twice and now it correctly triggers the fixed code path.

[Roasbeef]

We basically need to trigger the rescan result from a previous client. So I just ended up doing the same notification registrations twice and now it correctly triggers the fixed code path.

Ahh sure, I knew I was missing something subtle. Will check out the latest diff.

[Roasbeef]

LGTM 💼