Merge pull request #7243 from guggero/ghost-utxo-fix

wallet: fix ghost UTXOs not being detected

docs/release-notes/release-notes-0.16.0.md

@@ -96,6 +96,9 @@
   funding a PSBT through
   `FundPsbt`](https://github.com/lightningnetwork/lnd/pull/7209).
 
+* [Fix the issue of ghost UTXOs not being detected as spent if they were created
+  with an external tool](https://github.com/lightningnetwork/lnd/pull/7243).
+
 ## Build
 
 [The project has updated to Go

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/btcsuite/btcd/btcutil/psbt v1.1.5
 	github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2
 	github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
-	github.com/btcsuite/btcwallet v0.16.6-0.20221203002441-6c7480c8a46b
+	github.com/btcsuite/btcwallet v0.16.6-0.20221208210930-6f3f55efb230
 	github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2
 	github.com/btcsuite/btcwallet/wallet/txrules v1.2.0
 	github.com/btcsuite/btcwallet/walletdb v1.4.0

go.sum

@@ -94,15 +94,14 @@ github.com/btcsuite/btcd/btcutil/psbt v1.1.4/go.mod h1:9AyU6EQVJ9Iw9zPyNT1lcdHd6
 github.com/btcsuite/btcd/btcutil/psbt v1.1.5 h1:x0ZRrYY8j75ThV6xBz86CkYAG82F5bzay4H5D1c8b/U=
 github.com/btcsuite/btcd/btcutil/psbt v1.1.5/go.mod h1:kA6FLH/JfUx++j9pYU0pyu+Z8XGBQuuTmuKYUf6q7/U=
 github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0/go.mod h1:7SFka0XMvUgj3hfZtydOrQY2mwhPclbT2snogU7SQQc=
-github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1 h1:q0rUy8C/TYNBQS1+CGKw68tLOFYSNEs0TFnxxnS9+4U=
 github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1/go.mod h1:7SFka0XMvUgj3hfZtydOrQY2mwhPclbT2snogU7SQQc=
 github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2 h1:KdUfX2zKommPRa+PD0sWZUyXe9w277ABlgELO7H04IM=
 github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2/go.mod h1:7SFka0XMvUgj3hfZtydOrQY2mwhPclbT2snogU7SQQc=
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f h1:bAs4lUbRJpnnkd9VhRV3jjAVU7DJVjMaK+IsvSeZvFo=
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f/go.mod h1:TdznJufoqS23FtqVCzL0ZqgP5MqXbb4fg/WgDys70nA=
 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg=
-github.com/btcsuite/btcwallet v0.16.6-0.20221203002441-6c7480c8a46b h1:aA007quh9iZx9890HkqfpqZjHLmjzq7oHz7XOvV3azU=
+github.com/btcsuite/btcwallet v0.16.6-0.20221208210930-6f3f55efb230 h1:tWxnQZ35WZVMMjfHwVhqWJ1ARUnIKBtNu8kz/+y4szg=
-github.com/btcsuite/btcwallet v0.16.6-0.20221203002441-6c7480c8a46b/go.mod h1:mM19pFB3lGVxOL+kvHhHZAhdSWXKsZGiHvpJVvxL0D8=
+github.com/btcsuite/btcwallet v0.16.6-0.20221208210930-6f3f55efb230/go.mod h1:mM19pFB3lGVxOL+kvHhHZAhdSWXKsZGiHvpJVvxL0D8=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.2.3/go.mod h1:T2xSiKGpUkSLCh68aF+FMXmKK9mFqNdHl9VaqOr+JjU=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2 h1:etuLgGEojecsDOYTII8rYiGHjGyV5xTqsXi+ZQ715UU=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2/go.mod h1:Zpk/LOb2sKqwP2lmHjaZT9AdaKsHPSbNLm2Uql5IQ/0=

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"
@@ -1679,3 +1681,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,155 @@ 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.
+	ht.AssertNumUTXOsConfirmed(carol, 1)
+	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].Outpoint.OutputIndex == 0 {
+		ghostUtxoIndex = 1
+	}
+
+	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).
+	ht.AssertNumUTXOsConfirmed(carol, 1)
+}

walletunlocker/service.go

@@ -677,7 +677,13 @@ func (u *UnlockerService) LoadAndUnlock(password []byte,
 			return nil, nil, dropErr
 		}
 
-		// All looks good, let's now open the wallet again.
+		// All looks good, let's now open the wallet again. The loader
+		// was unloaded and might have removed its remote DB connection,
+		// so let's re-create it as well.
+		loader, err = u.newLoader(recoveryWindow)
+		if err != nil {
+			return nil, nil, err
+		}
 		unlockedWallet, err = loader.OpenExistingWallet(password, false)
 		if err != nil {
 			return nil, nil, err