openchannel: optimize open channel request creation and err handling

staticaddr/openchannel/manager.go

@@ -23,6 +23,7 @@ import (
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwallet/chanfunding"
+	"google.golang.org/protobuf/proto"
 )
 
 const (
@@ -366,64 +367,55 @@ func (m *Manager) OpenChannel(ctx context.Context,
 		return nil, err
 	}
 
-	openChanRequest := &lnrpc.OpenChannelRequest{
-		NodePubkey:                 req.NodePubkey,
-		LocalFundingAmount:         int64(chanFundingAmt),
-		PushSat:                    req.PushSat,
-		Private:                    req.Private,
-		MinHtlcMsat:                req.MinHtlcMsat,
-		RemoteCsvDelay:             req.RemoteCsvDelay,
-		MinConfs:                   defaultUtxoMinConf,
-		SpendUnconfirmed:           false,
-		CloseAddress:               req.CloseAddress,
-		RemoteMaxValueInFlightMsat: req.RemoteMaxValueInFlightMsat,
-		RemoteMaxHtlcs:             req.RemoteMaxHtlcs,
-		MaxLocalCsv:                req.MaxLocalCsv,
-		CommitmentType:             chanCommitmentType,
-		ZeroConf:                   req.ZeroConf,
-		ScidAlias:                  req.ScidAlias,
-		BaseFee:                    req.BaseFee,
-		FeeRate:                    req.FeeRate,
-		UseBaseFee:                 req.UseBaseFee,
-		UseFeeRate:                 req.UseFeeRate,
-		RemoteChanReserveSat:       req.RemoteChanReserveSat,
-		Memo:                       req.Memo,
+	// Clone the request message and set mandatory parameters.
+	reqClone := proto.Clone(req).(*lnrpc.OpenChannelRequest)
+
+	// Override fields consumed locally or incompatible with PSBT funding.
+	reqClone.LocalFundingAmount = int64(chanFundingAmt)
+
+	// TODO: Once lnd's PSBT channel open flow supports fundmax natively,
+	// we can pass FundMax through to lnd and remove Loop's local coin
+	// selection for the fundmax case.
+	reqClone.FundMax = false
+	reqClone.MinConfs = defaultUtxoMinConf
+	reqClone.SpendUnconfirmed = false
+	// In the lnd PSBT flow, fee estimation params on the request are
+	// explicitly disallowed.
+	reqClone.SatPerVbyte = 0
+	reqClone.CommitmentType = chanCommitmentType
+
+	chanOutpoint, err := m.openChannelPsbt(ctx, reqClone, deposits, feeRate)
+	if err == nil {
+		return chanOutpoint, nil
 	}
 
-	chanOutpoint, err := m.openChannelPsbt(
-		ctx, openChanRequest, deposits, feeRate,
-	)
-	if err != nil {
-		log.Infof("error opening channel: %v", err)
+	log.Infof("error opening channel: %v", err)
 
-		// If the PSBT was already finalized and sent to lnd, the
-		// funding transaction may have been broadcast. In that case
-		// we must not blindly roll back. Instead, try to recover
-		// the deposits now so they don't remain stuck in
-		// OpeningChannel until the next restart.
-		if errors.Is(err, errPsbtFinalized) {
-			recoverErr := m.recoverOpeningChannelDeposits(ctx)
-			if recoverErr != nil {
-				log.Errorf("failed recovering deposits "+
-					"after PSBT finalize: %v",
-					recoverErr)
-			}
-		} else {
-			err2 := m.cfg.DepositManager.TransitionDeposits(
-				ctx, deposits, fsm.OnError,
-				deposit.Deposited,
-			)
-			if err2 != nil {
-				log.Errorf("failed transitioning deposits "+
-					"after failed channel open: %v",
-					err2)
-			}
+	// If the PSBT was already finalized and sent to lnd, the
+	// funding transaction may have been broadcast. In that case
+	// we must not blindly roll back. Instead, try to recover
+	// the deposits now so they don't remain stuck in
+	// OpeningChannel until the next restart.
+	if errors.Is(err, errPsbtFinalized) {
+		recoverErr := m.recoverOpeningChannelDeposits(ctx)
+		if recoverErr != nil {
+			log.Errorf("failed recovering deposits "+
+				"after PSBT finalize: %v",
+				recoverErr)
+		}
+	} else {
+		err2 := m.cfg.DepositManager.TransitionDeposits(
+			ctx, deposits, fsm.OnError,
+			deposit.Deposited,
+		)
+		if err2 != nil {
+			log.Errorf("failed transitioning deposits "+
+				"after failed channel open: %v",
+				err2)
 		}
-
-		return nil, err
 	}
 
-	return chanOutpoint, nil
+	return nil, err
 }
 
 // openChannelPsbt starts an interactive channel open protocol that uses a
@@ -455,6 +447,7 @@ func (m *Manager) openChannelPsbt(ctx context.Context,
 		psbtFinalized bool
 		basePsbtBytes []byte
 		quit          = make(chan struct{})
+		quitCause     error
 		closeQuitOnce sync.Once
 		srvMsg        = make(chan *lnrpc.OpenStatusUpdate, 1)
 		srvErr        = make(chan error, 1)
@@ -594,6 +587,8 @@ func (m *Manager) openChannelPsbt(ctx context.Context,
 				shimPending = false
 				shimMu.Unlock()
 			}
+
+			quitCause = err
 			closeQuit()
 
 		case <-quit:
@@ -606,6 +601,9 @@ func (m *Manager) openChannelPsbt(ctx context.Context,
 		case srvResponse = <-srvMsg:
 		case <-quit:
 			cancelErr := fmt.Errorf("open channel flow canceled")
+			if quitCause != nil {
+				cancelErr = quitCause
+			}
 			if psbtFinalized {
 				return nil, fmt.Errorf("%w: %v",
 					errPsbtFinalized, cancelErr)
@@ -757,6 +755,26 @@ func validateInitialPsbtFlags(req *lnrpc.OpenChannelRequest) error {
 			"for PSBT funding")
 	}
 
+	if req.TargetConf != 0 {
+		return fmt.Errorf("TargetConf is not supported for PSBT " +
+			"funding, use SatPerVbyte to specify fee rate")
+	}
+
+	if req.SatPerByte != 0 { //nolint:staticcheck
+		return fmt.Errorf("SatPerByte is deprecated and not " +
+			"supported for PSBT funding, use SatPerVbyte")
+	}
+
+	if req.NodePubkeyString != "" { //nolint:staticcheck
+		return fmt.Errorf("NodePubkeyString is not supported, " +
+			"use NodePubkey instead")
+	}
+
+	if req.FundingShim != nil {
+		return fmt.Errorf("FundingShim is not supported, it is " +
+			"managed internally for PSBT funding")
+	}
+
 	return nil
 }
 

staticaddr/openchannel/manager_test.go

@@ -471,42 +471,67 @@ func TestValidateInitialPsbtFlags(t *testing.T) {
 
 	tests := []struct {
 		name              string
-		minConfs          int32
-		spendUnconfirmed  bool
+		req               *lnrpc.OpenChannelRequest
 		expectedErrSubstr string
 	}{
 		{
-			name:             "default min confs accepted",
-			minConfs:         0,
-			spendUnconfirmed: false,
+			name: "default min confs accepted",
+			req:  &lnrpc.OpenChannelRequest{},
 		},
 		{
-			name:             "explicit default min confs accepted",
-			minConfs:         defaultUtxoMinConf,
-			spendUnconfirmed: false,
+			name: "explicit default min confs accepted",
+			req: &lnrpc.OpenChannelRequest{
+				MinConfs: defaultUtxoMinConf,
+			},
 		},
 		{
-			name:              "custom min confs rejected",
-			minConfs:          defaultUtxoMinConf + 1,
-			spendUnconfirmed:  false,
+			name: "custom min confs rejected",
+			req: &lnrpc.OpenChannelRequest{
+				MinConfs: defaultUtxoMinConf + 1,
+			},
 			expectedErrSubstr: "custom MinConfs not supported",
 		},
 		{
-			name:              "spend unconfirmed rejected",
-			minConfs:          defaultUtxoMinConf,
-			spendUnconfirmed:  true,
+			name: "spend unconfirmed rejected",
+			req: &lnrpc.OpenChannelRequest{
+				MinConfs:         defaultUtxoMinConf,
+				SpendUnconfirmed: true,
+			},
 			expectedErrSubstr: "SpendUnconfirmed is not supported",
 		},
+		{
+			name: "target conf rejected",
+			req: &lnrpc.OpenChannelRequest{
+				TargetConf: 6,
+			},
+			expectedErrSubstr: "TargetConf is not supported",
+		},
+		{
+			name: "sat per byte rejected",
+			req: &lnrpc.OpenChannelRequest{
+				SatPerByte: 10, //nolint:staticcheck
+			},
+			expectedErrSubstr: "SatPerByte is deprecated",
+		},
+		{
+			name: "node pubkey string rejected",
+			req: &lnrpc.OpenChannelRequest{
+				NodePubkeyString: "abc", //nolint:staticcheck
+			},
+			expectedErrSubstr: "NodePubkeyString is not supported",
+		},
+		{
+			name: "funding shim rejected",
+			req: &lnrpc.OpenChannelRequest{
+				FundingShim: &lnrpc.FundingShim{},
+			},
+			expectedErrSubstr: "FundingShim is not supported",
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			req := &lnrpc.OpenChannelRequest{
-				MinConfs:         tc.minConfs,
-				SpendUnconfirmed: tc.spendUnconfirmed,
-			}
-
-			err := validateInitialPsbtFlags(req)
+			err := validateInitialPsbtFlags(tc.req)
 			if tc.expectedErrSubstr == "" {
 				require.NoError(t, err)
 				return