From 2221d0ccb154503f3c5331cb9c0e157bb70bd306 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 8 Dec 2022 11:48:51 +0100 Subject: [PATCH 1/5] 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 | 45 +++++++++ lntemp/rpc/signer.go | 17 ++++ lntemp/rpc/wallet_kit.go | 14 +++ lntest/itest/list_on_test.go | 4 + lntest/itest/lnd_recovery_test.go | 163 ++++++++++++++++++++++++++++++ 5 files changed, 243 insertions(+) diff --git a/lntemp/harness_assertion.go b/lntemp/harness_assertion.go index d48c5ada2..cd4864384 100644 --- a/lntemp/harness_assertion.go +++ b/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 +} diff --git a/lntemp/rpc/signer.go b/lntemp/rpc/signer.go index 4a7bea403..c7f54e12c 100644 --- a/lntemp/rpc/signer.go +++ b/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 +} diff --git a/lntemp/rpc/wallet_kit.go b/lntemp/rpc/wallet_kit.go index 986e5b388..fe1ccdec1 100644 --- a/lntemp/rpc/wallet_kit.go +++ b/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 +} diff --git a/lntest/itest/list_on_test.go b/lntest/itest/list_on_test.go index d08aab674..bb94c7394 100644 --- a/lntest/itest/list_on_test.go +++ b/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, diff --git a/lntest/itest/lnd_recovery_test.go b/lntest/itest/lnd_recovery_test.go index 8d0d417b4..fa53f7c4b 100644 --- a/lntest/itest/lnd_recovery_test.go +++ b/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) +} From 5e851912a7b16af4577cd650900afc97aa41d4d8 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 8 Dec 2022 11:48:53 +0100 Subject: [PATCH 2/5] mod: bump btcwallet to latest version --- go.mod | 2 +- go.sum | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 048f0de24..0762c4643 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 1c4856482..72276f75c 100644 --- a/go.sum +++ b/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.20221203002441-6c7480c8a46b/go.mod h1:mM19pFB3lGVxOL+kvHhHZAhdSWXKsZGiHvpJVvxL0D8= +github.com/btcsuite/btcwallet v0.16.6-0.20221208210930-6f3f55efb230 h1:tWxnQZ35WZVMMjfHwVhqWJ1ARUnIKBtNu8kz/+y4szg= +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= From f26a9a84d71fb48ddd77abed4ce81258b41387d2 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 8 Dec 2022 11:48:54 +0100 Subject: [PATCH 3/5] lntest: fix rescan test to show correct behavior Now that we have patched the btcwallet version to the one that includes the fix, we can adjust the rescan test to show the correct and expected behavior. --- lntest/itest/lnd_recovery_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lntest/itest/lnd_recovery_test.go b/lntest/itest/lnd_recovery_test.go index fa53f7c4b..4ba521912 100644 --- a/lntest/itest/lnd_recovery_test.go +++ b/lntest/itest/lnd_recovery_test.go @@ -355,18 +355,15 @@ func testRescanAddressDetection(ht *lntemp.HarnessTest) { // 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) + 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].Address == ghostUtxoAddr.String() { - ghostUtxoIndex = unspent.Utxos[0].Outpoint.OutputIndex - } else { - ghostUtxoIndex = unspent.Utxos[1].Outpoint.OutputIndex + if unspent.Utxos[0].Outpoint.OutputIndex == 0 { + ghostUtxoIndex = 1 } ghostUtxoHash, err := chainhash.NewHash( @@ -442,6 +439,5 @@ func testRescanAddressDetection(ht *lntemp.HarnessTest) { // 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) + ht.AssertNumUTXOsConfirmed(carol, 1) } From d04182f4828ef201959fa1619b7c75322601e792 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 8 Dec 2022 11:48:55 +0100 Subject: [PATCH 4/5] docs: add release notes --- docs/release-notes/release-notes-0.16.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index b01399301..c58f4f801 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/docs/release-notes/release-notes-0.16.0.md @@ -93,6 +93,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 From ad0dbb1c15d7a744c7eeaa4448bd0f5364dd41ee Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 9 Dec 2022 11:09:05 +0100 Subject: [PATCH 5/5] walletunlocker: re-create loader after unloading wallet Because the wallet loader sets its internal database reference to nil when unloading a wallet, it cannot be loaded again in case of a remote DB such as etcd or postgres. To avoid running into a nil pointer panic we just re-create the loader as well before opening the wallet. --- walletunlocker/service.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/walletunlocker/service.go b/walletunlocker/service.go index 6d58b0478..d896c2d88 100644 --- a/walletunlocker/service.go +++ b/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