lntest+lntemp: add itest case for wallet rescan problem

In this commit we add a test that demonstrates the problem of ghost
UTXOs lingering in the wallet after a re-scan, even if they were spent.

NOTE: This test shows the incorrect behavior and will be adjusted in a
follow-up commit to show the correct behavior after patching the
btcwallet version.

lntemp/harness_assertion.go

@@ -10,6 +10,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/btcsuite/btcd/btcec/v2"
+	"github.com/btcsuite/btcd/btcec/v2/schnorr"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
@@ -1648,3 +1650,46 @@ func (h *HarnessTest) AssertNumChannelUpdates(hn *node.HarnessNode,
 	err := hn.Watcher.WaitForNumChannelUpdates(op, num)
 	require.NoError(h, err, "failed to assert num of channel updates")
 }
+
+// CreateBurnAddr creates a random burn address of the given type.
+func (h *HarnessTest) CreateBurnAddr(addrType lnrpc.AddressType) ([]byte,
+	btcutil.Address) {
+
+	randomPrivKey, err := btcec.NewPrivateKey()
+	require.NoError(h, err)
+
+	randomKeyBytes := randomPrivKey.PubKey().SerializeCompressed()
+
+	var addr btcutil.Address
+	switch addrType {
+	case lnrpc.AddressType_WITNESS_PUBKEY_HASH:
+		addr, err = btcutil.NewAddressWitnessPubKeyHash(
+			btcutil.Hash160(randomKeyBytes), harnessNetParams,
+		)
+
+	case lnrpc.AddressType_TAPROOT_PUBKEY:
+		taprootKey := txscript.ComputeTaprootKeyNoScript(
+			randomPrivKey.PubKey(),
+		)
+		addr, err = btcutil.NewAddressPubKey(
+			schnorr.SerializePubKey(taprootKey), harnessNetParams,
+		)
+
+	case lnrpc.AddressType_NESTED_PUBKEY_HASH:
+		var witnessAddr btcutil.Address
+		witnessAddr, err = btcutil.NewAddressWitnessPubKeyHash(
+			btcutil.Hash160(randomKeyBytes), harnessNetParams,
+		)
+		require.NoError(h, err)
+
+		addr, err = btcutil.NewAddressScriptHash(
+			h.PayToAddrScript(witnessAddr), harnessNetParams,
+		)
+
+	default:
+		h.Fatalf("Unsupported burn address type: %v", addrType)
+	}
+	require.NoError(h, err)
+
+	return h.PayToAddrScript(addr), addr
+}

lntemp/rpc/signer.go

@@ -1,5 +1,22 @@
 package rpc
 
+import (
+	"context"
+
+	"github.com/lightningnetwork/lnd/lnrpc/signrpc"
+)
+
 // =====================
 // Signer related RPCs.
 // =====================
+
+// SignOutputRaw makes a RPC call to node's SignOutputRaw and asserts.
+func (h *HarnessRPC) SignOutputRaw(req *signrpc.SignReq) *signrpc.SignResp {
+	ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout)
+	defer cancel()
+
+	resp, err := h.Signer.SignOutputRaw(ctxt, req)
+	h.NoError(err, "SignOutputRaw")
+
+	return resp
+}

lntemp/rpc/wallet_kit.go

@@ -149,3 +149,17 @@ func (h *HarnessRPC) PendingSweeps() *walletrpc.PendingSweepsResponse {
 
 	return resp
 }
+
+// PublishTransaction makes an RPC call to the node's WalletKitClient and
+// asserts.
+func (h *HarnessRPC) PublishTransaction(
+	req *walletrpc.Transaction) *walletrpc.PublishResponse {
+
+	ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout)
+	defer cancel()
+
+	resp, err := h.WalletKit.PublishTransaction(ctxt, req)
+	h.NoError(err, "PublishTransaction")
+
+	return resp
+}

lntest/itest/list_on_test.go

@@ -119,6 +119,10 @@ var allTestCasesTemp = []*lntemp.TestCase{
 		Name:     "onchain fund recovery",
 		TestFunc: testOnchainFundRecovery,
 	},
+	{
+		Name:     "wallet rescan address detection",
+		TestFunc: testRescanAddressDetection,
+	},
 	{
 		Name:     "basic funding flow with all input types",
 		TestFunc: testChannelFundingInputTypes,

lntest/itest/lnd_recovery_test.go

@@ -1,17 +1,24 @@
 package itest
 
 import (
+	"bytes"
 	"fmt"
 	"math"
 
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/btcutil/hdkeychain"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/txscript"
+	"github.com/btcsuite/btcd/wire"
 	"github.com/lightningnetwork/lnd/aezeed"
+	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnrpc"
+	"github.com/lightningnetwork/lnd/lnrpc/signrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/walletrpc"
 	"github.com/lightningnetwork/lnd/lntemp"
 	"github.com/lightningnetwork/lnd/lntemp/node"
 	"github.com/lightningnetwork/lnd/lntest/wait"
+	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/stretchr/testify/require"
 )
 
@@ -282,3 +289,159 @@ func testOnchainFundRecovery(ht *lntemp.HarnessTest) {
 
 	restoreCheckBalance(finalBalance-minerAmt-fee, 1, 21, nil)
 }
+
+// testRescanAddressDetection makes sure that addresses created from internal
+// (m/1017' scope) keys aren't detected as UTXOs when re-scanning the wallet
+// with --reset-wallet-transactions to avoid showing them as un-spent ghost
+// UTXOs even if they are being spent. This is to test a fix in the wallet that
+// addresses the following scenario:
+//  1. A key is derived from the internal 1017' scope with a custom key family
+//     and a p2wkh address is derived from that key.
+//  2. Funds are sent to the address created above in a way that also creates a
+//     change output. The change output is recognized as belonging to the
+//     wallet, which is correct.
+//  3. The funds on the address created in step 1 are fully spent (without
+//     creating a change output) into an output that doesn't belong to the
+//     wallet (e.g. a channel funding output).
+//  4. At some point the user re-scans their wallet by using the
+//     --reset-wallet-transactions flag.
+//  5. The wallet re-scan detects the change output created in step 2 and flags
+//     the transaction as relevant.
+//  6. While adding the relevant TX to the wallet DB, the wallet also detects
+//     the address from step 1 as belonging to the wallet (because the internal
+//     key scope is defined as having the address type p2wkh) and adds that
+//     output as an UTXO as well (<- this is the bug). The wallet now has two
+//     UTXOs in its database.
+//  7. The transaction that spends the UTXO of the address from step 1 is not
+//     detected by the wallet as belonging to it (because the output is a
+//     channel output and the input (correctly) isn't recognized as belonging to
+//     the wallet in that part of the code, it is never marked as spent and
+//     stays in the wallet as a ghost UTXO forever.
+//
+// The fix in the wallet is simple: In step 6, don't detect addresses from
+// internal scopes while re-scanning to be in line with the logic in other areas
+// of the wallet code.
+func testRescanAddressDetection(ht *lntemp.HarnessTest) {
+	// We start off by creating a new node with the wallet re-scan flag
+	// enabled. This won't have any effect on the first startup but will
+	// come into effect after we re-start the node.
+	walletPassword := []byte("some-password")
+	carol, _, _ := ht.NewNodeWithSeed(
+		"carol", []string{"--reset-wallet-transactions"},
+		walletPassword, false,
+	)
+	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
+
+	// Create an address generated from internal keys.
+	keyDesc := carol.RPC.DeriveNextKey(&walletrpc.KeyReq{KeyFamily: 123})
+	pubKeyHash := btcutil.Hash160(keyDesc.RawKeyBytes)
+	ghostUtxoAddr, err := btcutil.NewAddressWitnessPubKeyHash(
+		pubKeyHash, harnessNetParams,
+	)
+	require.NoError(ht, err)
+
+	// Send funds to the (p2wkh!) address generated from the internal
+	// (m/1017') key scope. Because the internal key scope is defined as
+	// p2wkh address type, this might be incorrectly detected by the wallet
+	// in some situations (which this test makes sure is fixed).
+	const ghostUtxoAmount = 456_000
+	carol.RPC.SendCoins(&lnrpc.SendCoinsRequest{
+		Addr:        ghostUtxoAddr.String(),
+		Amount:      ghostUtxoAmount,
+		SatPerVbyte: 1,
+	})
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Make sure we see the change output in our list of unspent outputs.
+	// We _don't_ expect to see the ghost UTXO here as in this step it's
+	// ignored as an internal address correctly.
+	// TODO(guggero): This is incorrect, should only be 1 UTXO.
+	ht.AssertNumUTXOsConfirmed(carol, 2)
+	unspent := carol.RPC.ListUnspent(&walletrpc.ListUnspentRequest{
+		MinConfs: 1,
+	})
+
+	// Which one was the change output and which one the ghost UTXO output?
+	var ghostUtxoIndex uint32
+	if unspent.Utxos[0].Address == ghostUtxoAddr.String() {
+		ghostUtxoIndex = unspent.Utxos[0].Outpoint.OutputIndex
+	} else {
+		ghostUtxoIndex = unspent.Utxos[1].Outpoint.OutputIndex
+	}
+
+	ghostUtxoHash, err := chainhash.NewHash(
+		unspent.Utxos[0].Outpoint.TxidBytes,
+	)
+	require.NoError(ht, err)
+
+	burnScript, _ := ht.CreateBurnAddr(AddrTypeWitnessPubkeyHash)
+
+	// Create fee estimation for a p2wkh input and p2wkh output.
+	feeRate := chainfee.SatPerKWeight(12500)
+	estimator := input.TxWeightEstimator{}
+	estimator.AddP2WKHInput()
+	estimator.AddP2WKHOutput()
+	estimatedWeight := int64(estimator.Weight())
+	requiredFee := feeRate.FeeForWeight(estimatedWeight)
+
+	tx := wire.NewMsgTx(2)
+	tx.TxIn = []*wire.TxIn{{
+		PreviousOutPoint: wire.OutPoint{
+			Hash:  *ghostUtxoHash,
+			Index: ghostUtxoIndex,
+		},
+	}}
+	value := int64(ghostUtxoAmount - requiredFee)
+	tx.TxOut = []*wire.TxOut{{
+		PkScript: burnScript,
+		Value:    value,
+	}}
+
+	var buf bytes.Buffer
+	require.NoError(ht, tx.Serialize(&buf))
+
+	ghostUtxoScript := ht.PayToAddrScript(ghostUtxoAddr)
+	utxoInfo := []*signrpc.TxOut{{
+		PkScript: ghostUtxoScript,
+		Value:    ghostUtxoAmount,
+	}}
+
+	// Let's sign the input now.
+	signResp := carol.RPC.SignOutputRaw(&signrpc.SignReq{
+		RawTxBytes: buf.Bytes(),
+		SignDescs: []*signrpc.SignDescriptor{{
+			Output:        utxoInfo[0],
+			InputIndex:    0,
+			KeyDesc:       keyDesc,
+			Sighash:       uint32(txscript.SigHashAll),
+			WitnessScript: utxoInfo[0].PkScript,
+		}},
+	})
+
+	// Add the witness to the input and publish the tx.
+	tx.TxIn[0].Witness = wire.TxWitness{
+		append(signResp.RawSigs[0], byte(txscript.SigHashAll)),
+		keyDesc.RawKeyBytes,
+	}
+	buf.Reset()
+	require.NoError(ht, tx.Serialize(&buf))
+	carol.RPC.PublishTransaction(&walletrpc.Transaction{
+		TxHex: buf.Bytes(),
+	})
+
+	// Wait until the spending tx is found and mine a block to confirm it.
+	ht.Miner.AssertNumTxsInMempool(1)
+	ht.MineBlocks(1)
+
+	// The wallet should still just see a single UTXO of the change output
+	// created earlier.
+	ht.AssertNumUTXOsConfirmed(carol, 1)
+
+	// Let's now re-start the node, causing it to do the wallet re-scan.
+	ht.RestartNode(carol)
+
+	// There should now still only be a single UTXO from the change output
+	// instead of two (the ghost UTXO should be missing if the fix works).
+	// TODO(guggero): This is incorrect, should only be 1 UTXO.
+	ht.AssertNumUTXOsConfirmed(carol, 2)
+}