From 5d18fccd423cd42f11797e8701f21da6fa04a4cd Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:08 +0100 Subject: [PATCH 1/7] itest: reproduce remote signing issue This commit attempts to reproduce the issue as described in #7276. --- lntest/itest/lnd_funding_test.go | 49 ++++++++++++++++++-------- lntest/itest/lnd_remote_signer_test.go | 6 ++++ 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/lntest/itest/lnd_funding_test.go b/lntest/itest/lnd_funding_test.go index bd57997ee..d9e66d792 100644 --- a/lntest/itest/lnd_funding_test.go +++ b/lntest/itest/lnd_funding_test.go @@ -330,23 +330,43 @@ func testUnconfirmedChannelFunding(ht *lntemp.HarnessTest) { // testChannelFundingInputTypes tests that any type of supported input type can // be used to fund channels. func testChannelFundingInputTypes(ht *lntemp.HarnessTest) { - const ( - chanAmt = funding.MaxBtcFundingAmount - burnAddr = "bcrt1qxsnqpdc842lu8c0xlllgvejt6rhy49u6fmpgyz" - ) - - fundWithTypes := []func(amt btcutil.Amount, target *node.HarnessNode){ - ht.FundCoins, ht.FundCoinsNP2WKH, ht.FundCoinsP2TR, - } - - alice := ht.Alice - // We'll start off by creating a node for Carol. carol := ht.NewNode("Carol", nil) // Now, we'll connect her to Alice so that they can open a // channel together. - ht.ConnectNodes(carol, alice) + ht.ConnectNodes(carol, ht.Alice) + + runChannelFundingInputTypes(ht, ht.Alice, carol) +} + +// runChannelFundingInputTypes tests that any type of supported input type can +// be used to fund channels. +func runChannelFundingInputTypes(ht *lntemp.HarnessTest, alice, + carol *node.HarnessNode) { + + const ( + chanAmt = funding.MaxBtcFundingAmount + burnAddr = "bcrt1qxsnqpdc842lu8c0xlllgvejt6rhy49u6fmpgyz" + ) + + fundMixed := func(amt btcutil.Amount, target *node.HarnessNode) { + ht.FundCoins(amt/5, target) + ht.FundCoins(amt/5, target) + ht.FundCoinsP2TR(amt/5, target) + ht.FundCoinsP2TR(amt/5, target) + ht.FundCoinsP2TR(amt/5, target) + } + fundMultipleP2TR := func(amt btcutil.Amount, target *node.HarnessNode) { + ht.FundCoinsP2TR(amt/4, target) + ht.FundCoinsP2TR(amt/4, target) + ht.FundCoinsP2TR(amt/4, target) + ht.FundCoinsP2TR(amt/4, target) + } + fundWithTypes := []func(amt btcutil.Amount, target *node.HarnessNode){ + ht.FundCoins, ht.FundCoinsNP2WKH, ht.FundCoinsP2TR, fundMixed, + fundMultipleP2TR, + } // Creates a helper closure to be used below which asserts the // proper response to a channel balance RPC. @@ -386,8 +406,9 @@ func testChannelFundingInputTypes(ht *lntemp.HarnessTest) { } for _, funder := range fundWithTypes { - // We'll send her some confirmed funds. - funder(chanAmt*2, carol) + // We'll send her some confirmed funds. We send 10% more than + // we need to account for fees. + funder((chanAmt*11)/10, carol) chanOpenUpdate := ht.OpenChannelAssertStream( carol, alice, lntemp.OpenChannelParams{ diff --git a/lntest/itest/lnd_remote_signer_test.go b/lntest/itest/lnd_remote_signer_test.go index eccf396b9..0afdf9bc4 100644 --- a/lntest/itest/lnd_remote_signer_test.go +++ b/lntest/itest/lnd_remote_signer_test.go @@ -84,6 +84,12 @@ func testRemoteSigner(ht *lntemp.HarnessTest) { fn: func(tt *lntemp.HarnessTest, wo, carol *node.HarnessNode) { runBasicChannelCreationAndUpdates(tt, wo, carol) }, + }, { + name: "channel funding input types", + sendCoins: false, + fn: func(tt *lntemp.HarnessTest, wo, carol *node.HarnessNode) { + runChannelFundingInputTypes(tt, carol, wo) + }, }, { name: "async payments", sendCoins: true, From e4f3a35c0c45076d6dcb10f0fcd7a706d7abadfe Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:09 +0100 Subject: [PATCH 2/7] funding: fix peer key serialization in log message --- funding/manager.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/funding/manager.go b/funding/manager.go index 809a56a07..80be2a67d 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1766,11 +1766,15 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, pendingChanID := msg.PendingChannelID peerKey := peer.IdentityKey() + var peerKeyBytes []byte + if peerKey != nil { + peerKeyBytes = peerKey.SerializeCompressed() + } resCtx, err := f.getReservationCtx(peerKey, pendingChanID) if err != nil { - log.Warnf("Can't find reservation (peerKey:%v, chan_id:%v)", - peerKey, pendingChanID) + log.Warnf("Can't find reservation (peerKey:%x, chan_id:%v)", + peerKeyBytes, pendingChanID) return } @@ -1950,7 +1954,8 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, addr, amt, packet, err := psbtErr.Intent.FundingParams() if err != nil { log.Errorf("Unable to process PSBT funding params "+ - "for contribution from %v: %v", peerKey, err) + "for contribution from %x: %v", peerKeyBytes, + err) f.failFundingFlow(peer, msg.PendingChannelID, err) return } @@ -1958,7 +1963,7 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, err = packet.Serialize(&buf) if err != nil { log.Errorf("Unable to serialize PSBT for "+ - "contribution from %v: %v", peerKey, err) + "contribution from %x: %v", peerKeyBytes, err) f.failFundingFlow(peer, msg.PendingChannelID, err) return } @@ -1974,8 +1979,8 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, } psbtIntent = psbtErr.Intent } else if err != nil { - log.Errorf("Unable to process contribution from %v: %v", - peerKey, err) + log.Errorf("Unable to process contribution from %x: %v", + peerKeyBytes, err) f.failFundingFlow(peer, msg.PendingChannelID, err) return } From a2a96c77120bd4ce9a09c09f28631061ffb6f348 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:10 +0100 Subject: [PATCH 3/7] lnwallet: don't re-use sign descriptor Fixes an issue where re-using a sign descriptor in a loop carried over signing information from one call to the next, which caused the remote signing issue. --- lnwallet/chanfunding/wallet_assembler.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lnwallet/chanfunding/wallet_assembler.go b/lnwallet/chanfunding/wallet_assembler.go index 6d9c1974e..c81c1cb53 100644 --- a/lnwallet/chanfunding/wallet_assembler.go +++ b/lnwallet/chanfunding/wallet_assembler.go @@ -110,13 +110,12 @@ func (f *FullIntent) CompileFundingTx(extraInputs []*wire.TxIn, prevOutFetcher := NewSegWitV0DualFundingPrevOutputFetcher( f.coinSource, extraInputs, ) - signDesc := input.SignDescriptor{ - SigHashes: txscript.NewTxSigHashes( - fundingTx, prevOutFetcher, - ), - PrevOutputFetcher: prevOutFetcher, - } + sigHashes := txscript.NewTxSigHashes(fundingTx, prevOutFetcher) for i, txIn := range fundingTx.TxIn { + signDesc := input.SignDescriptor{ + SigHashes: sigHashes, + PrevOutputFetcher: prevOutFetcher, + } // We can only sign this input if it's ours, so we'll ask the // coin source if it can map this outpoint into a coin we own. // If not, then we'll continue as it isn't our input. From c6c52b8eb3cc89fad0a84fadab95987210522434 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:11 +0100 Subject: [PATCH 4/7] itest: add coverage for np2wkh in remote signing --- lntest/itest/lnd_remote_signer_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lntest/itest/lnd_remote_signer_test.go b/lntest/itest/lnd_remote_signer_test.go index 0afdf9bc4..8b7a2b6a6 100644 --- a/lntest/itest/lnd_remote_signer_test.go +++ b/lntest/itest/lnd_remote_signer_test.go @@ -116,6 +116,11 @@ func testRemoteSigner(ht *lntemp.HarnessTest) { runSignPsbtSegWitV1KeySpendBip86(tt, wo) runSignPsbtSegWitV1KeySpendRootHash(tt, wo) runSignPsbtSegWitV1ScriptSpend(tt, wo) + + // The above tests all make sure we can sign for keys + // that aren't in the wallet. But we also want to make + // sure we can fund and then sign PSBTs from our wallet. + runFundAndSignPsbt(ht, wo) }, }, { name: "sign output raw", From 8bc16b4fb27f9f85732187144cc0ece1cc5f81a1 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:12 +0100 Subject: [PATCH 5/7] rpcwallet: fix np2wkh inputs in remote signing There is a one byte difference between the sigScript and the witnessScript in case of a np2wkh input. The remote signer actually needs the witness script and not the sig script to produce a valid signature. --- lnwallet/rpcwallet/rpcwallet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lnwallet/rpcwallet/rpcwallet.go b/lnwallet/rpcwallet/rpcwallet.go index 0d5b2ead9..d5ab4e254 100644 --- a/lnwallet/rpcwallet/rpcwallet.go +++ b/lnwallet/rpcwallet/rpcwallet.go @@ -617,7 +617,7 @@ func (r *RPCKeyRing) ComputeInputScript(tx *wire.MsgTx, // Let's give the TX to the remote instance now, so it can sign the // input. - sig, err := r.remoteSign(tx, signDesc, sigScript) + sig, err := r.remoteSign(tx, signDesc, witnessProgram) if err != nil { return nil, fmt.Errorf("error signing with remote instance: %v", err) From df1ea46d9e4cf11426f61d2e99c9f6bd856bfc28 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:13 +0100 Subject: [PATCH 6/7] rpcwallet: fix p2tr inputs in remote signing The SendOutputs method isn't used very often in our code so the missing Taproot sighash type wasn't detected before. Also, a P2TR input will never have a sigScript, so we can explicitly set that parameter to nil instead of relying on it being nil anyway. --- lnwallet/rpcwallet/rpcwallet.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lnwallet/rpcwallet/rpcwallet.go b/lnwallet/rpcwallet/rpcwallet.go index d5ab4e254..38f345c14 100644 --- a/lnwallet/rpcwallet/rpcwallet.go +++ b/lnwallet/rpcwallet/rpcwallet.go @@ -159,6 +159,10 @@ func (r *RPCKeyRing) SendOutputs(outputs []*wire.TxOut, return nil, fmt.Errorf("error looking up utxo: %v", err) } + if txscript.IsPayToTaproot(info.PkScript) { + signDesc.HashType = txscript.SigHashDefault + } + // Now that we know the input is ours, we'll populate the // signDesc with the per input unique information. signDesc.Output = &wire.TxOut{ @@ -593,7 +597,7 @@ func (r *RPCKeyRing) ComputeInputScript(tx *wire.MsgTx, signDesc.SignMethod = input.TaprootKeySpendBIP0086SignMethod signDesc.WitnessScript = nil - sig, err := r.remoteSign(tx, signDesc, sigScript) + sig, err := r.remoteSign(tx, signDesc, nil) if err != nil { return nil, fmt.Errorf("error signing with remote"+ "instance: %v", err) From e7fb48ff732b80ffd520dd8c29402b3090fcded4 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 7 Feb 2023 22:15:14 +0100 Subject: [PATCH 7/7] docs: add release notes --- docs/release-notes/release-notes-0.16.0.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index bee7566a9..0a18ad342 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/docs/release-notes/release-notes-0.16.0.md @@ -139,6 +139,10 @@ current gossip sync query status. * [Add log message for edge case](https://github.com/lightningnetwork/lnd/pull/7115). +* [A remote signer issue was fixed that caused opening channels to fail when + multiple mixed-type inputs were used for the funding + transaction](https://github.com/lightningnetwork/lnd/pull/7386). + ## Build * [The project has updated to Go