Merge pull request #4130 from cfromknecht/open-channel-refactor

rpcserver: align OpenChannel + OpenChannelSync request parsing

lntest/itest/lnd_test.go

@@ -214,9 +214,15 @@ func openChannelAndAssert(ctx context.Context, t *harnessTest,
 	net *lntest.NetworkHarness, alice, bob *lntest.HarnessNode,
 	p lntest.OpenChannelParams) *lnrpc.ChannelPoint {
 
-	chanOpenUpdate, err := net.OpenChannel(
+	// Wait until we are able to fund a channel successfully. This wait
-		ctx, alice, bob, p,
+	// prevents us from erroring out when trying to create a channel while
-	)
+	// the node is starting up.
+	var chanOpenUpdate lnrpc.Lightning_OpenChannelClient
+	err := wait.NoError(func() error {
+		var err error
+		chanOpenUpdate, err = net.OpenChannel(ctx, alice, bob, p)
+		return err
+	}, defaultTimeout)
 	if err != nil {
 		t.Fatalf("unable to open channel: %v", err)
 	}

rpcserver.go

@@ -1603,19 +1603,38 @@ func newPsbtAssembler(req *lnrpc.OpenChannelRequest, normalizedMinConfs int32,
 	), nil
 }
 
-// OpenChannel attempts to open a singly funded channel specified in the
+// canOpenChannel returns an error if the necessary subsystems for channel
-// request to a remote peer.
+// funding are not ready.
-func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
+func (r *rpcServer) canOpenChannel() error {
-	updateStream lnrpc.Lightning_OpenChannelServer) error {
+	// We can't open a channel until the main server has started.
-
-	rpcsLog.Tracef("[openchannel] request to NodeKey(%v) "+
-		"allocation(us=%v, them=%v)", in.NodePubkeyString,
-		in.LocalFundingAmount, in.PushSat)
-
 	if !r.server.Started() {
 		return ErrServerNotActive
 	}
 
+	// Creation of channels before the wallet syncs up is currently
+	// disallowed.
+	isSynced, _, err := r.server.cc.wallet.IsSynced()
+	if err != nil {
+		return err
+	}
+	if !isSynced {
+		return errors.New("channels cannot be created before the " +
+			"wallet is fully synced")
+	}
+
+	return nil
+}
+
+// praseOpenChannelReq parses an OpenChannelRequest message into the server's
+// native openChanReq struct. The logic is abstracted so that it can be shared
+// between OpenChannel and OpenChannelSync.
+func (r *rpcServer) parseOpenChannelReq(in *lnrpc.OpenChannelRequest,
+	isSync bool) (*openChanReq, error) {
+
+	rpcsLog.Debugf("[openchannel] request to NodeKey(%x) "+
+		"allocation(us=%v, them=%v)", in.NodePubkey,
+		in.LocalFundingAmount, in.PushSat)
+
 	localFundingAmt := btcutil.Amount(in.LocalFundingAmount)
 	remoteInitialBalance := btcutil.Amount(in.PushSat)
 	minHtlcIn := lnwire.MilliSatoshi(in.MinHtlcMsat)
@@ -1627,15 +1646,15 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 	//
 	// TODO(roasbeef): incorporate base fee?
 	if remoteInitialBalance >= localFundingAmt {
-		return fmt.Errorf("amount pushed to remote peer for initial " +
+		return nil, fmt.Errorf("amount pushed to remote peer for " +
-			"state must be below the local funding amount")
+			"initial state must be below the local funding amount")
 	}
 
 	// Ensure that the user doesn't exceed the current soft-limit for
 	// channel size. If the funding amount is above the soft-limit, then
 	// we'll reject the request.
 	if localFundingAmt > MaxFundingAmount {
-		return fmt.Errorf("funding amount is too large, the max "+
+		return nil, fmt.Errorf("funding amount is too large, the max "+
 			"channel size is: %v", MaxFundingAmount)
 	}
 
@@ -1643,8 +1662,8 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 	// level, we'll ensure that the output we create after accounting for
 	// fees that a dust output isn't created.
 	if localFundingAmt < minChanFundingSize {
-		return fmt.Errorf("channel is too small, the minimum channel "+
+		return nil, fmt.Errorf("channel is too small, the minimum "+
-			"size is: %v SAT", int64(minChanFundingSize))
+			"channel size is: %v SAT", int64(minChanFundingSize))
 	}
 
 	// Then, we'll extract the minimum number of confirmations that each
@@ -1652,36 +1671,50 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 	// satisfy.
 	minConfs, err := extractOpenChannelMinConfs(in)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	var (
-		nodePubKey      *btcec.PublicKey
-		nodePubKeyBytes []byte
-	)
-
 	// TODO(roasbeef): also return channel ID?
 
-	// Ensure that the NodePubKey is set before attempting to use it
+	var nodePubKey *btcec.PublicKey
-	if len(in.NodePubkey) == 0 {
-		return fmt.Errorf("NodePubKey is not set")
-	}
 
-	// Parse the raw bytes of the node key into a pubkey object so we
+	// Parse the remote pubkey the NodePubkey field of the request. If it's
-	// can easily manipulate it.
+	// not present, we'll fallback to the deprecated version that parses the
-	nodePubKey, err = btcec.ParsePubKey(in.NodePubkey, btcec.S256())
+	// key from a hex string if this is for REST for backwards compatibility.
-	if err != nil {
+	switch {
-		return err
+
+	// Parse the raw bytes of the node key into a pubkey object so we can
+	// easily manipulate it.
+	case len(in.NodePubkey) > 0:
+		nodePubKey, err = btcec.ParsePubKey(in.NodePubkey, btcec.S256())
+		if err != nil {
+			return nil, err
+		}
+
+	// Decode the provided target node's public key, parsing it into a pub
+	// key object. For all sync call, byte slices are expected to be encoded
+	// as hex strings.
+	case isSync:
+		keyBytes, err := hex.DecodeString(in.NodePubkeyString)
+		if err != nil {
+			return nil, err
+		}
+
+		nodePubKey, err = btcec.ParsePubKey(keyBytes, btcec.S256())
+		if err != nil {
+			return nil, err
+		}
+
+	default:
+		return nil, fmt.Errorf("NodePubkey is not set")
 	}
 
 	// Making a channel to ourselves wouldn't be of any use, so we
 	// explicitly disallow them.
 	if nodePubKey.IsEqual(r.server.identityPriv.PubKey()) {
-		return fmt.Errorf("cannot open channel to self")
+		return nil, fmt.Errorf("cannot open channel to self")
 	}
 
-	nodePubKeyBytes = nodePubKey.SerializeCompressed()
-
 	// Based on the passed fee related parameters, we'll determine an
 	// appropriate fee rate for the funding transaction.
 	satPerKw := chainfee.SatPerKVByte(in.SatPerByte * 1000).FeePerKWeight()
@@ -1692,7 +1725,7 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 		},
 	)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	rpcsLog.Debugf("[openchannel]: using fee of %v sat/kw for funding tx",
@@ -1700,13 +1733,14 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 
 	script, err := parseUpfrontShutdownAddress(in.CloseAddress)
 	if err != nil {
-		return fmt.Errorf("error parsing upfront shutdown: %v", err)
+		return nil, fmt.Errorf("error parsing upfront shutdown: %v",
+			err)
 	}
 
 	// Instruct the server to trigger the necessary events to attempt to
 	// open a new channel. A stream is returned in place, this stream will
 	// be used to consume updates of the state of the pending channel.
-	req := &openChanReq{
+	return &openChanReq{
 		targetPubkey:    nodePubKey,
 		chainHash:       *activeNetParams.GenesisHash,
 		localFundingAmt: localFundingAmt,
@@ -1717,6 +1751,21 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 		remoteCsvDelay:  remoteCsvDelay,
 		minConfs:        minConfs,
 		shutdownScript:  script,
+	}, nil
+}
+
+// OpenChannel attempts to open a singly funded channel specified in the
+// request to a remote peer.
+func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
+	updateStream lnrpc.Lightning_OpenChannelServer) error {
+
+	if err := r.canOpenChannel(); err != nil {
+		return err
+	}
+
+	req, err := r.parseOpenChannelReq(in, false)
+	if err != nil {
+		return err
 	}
 
 	// If the user has provided a shim, then we'll now augment the based
@@ -1752,7 +1801,7 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest,
 			// transaction.
 			copy(req.pendingChanID[:], psbtShim.PendingChanId)
 			req.chanFunder, err = newPsbtAssembler(
-				in, minConfs, psbtShim,
+				in, req.minConfs, psbtShim,
 				&r.server.cc.wallet.Cfg.NetParams,
 			)
 			if err != nil {
@@ -1769,7 +1818,7 @@ out:
 		select {
 		case err := <-errChan:
 			rpcsLog.Errorf("unable to open channel to NodeKey(%x): %v",
-				nodePubKeyBytes, err)
+				req.targetPubkey.SerializeCompressed(), err)
 			return err
 		case fundingUpdate := <-updateChan:
 			rpcsLog.Tracef("[openchannel] sending update: %v",
@@ -1801,7 +1850,7 @@ out:
 	}
 
 	rpcsLog.Tracef("[openchannel] success NodeKey(%x), ChannelPoint(%v)",
-		nodePubKeyBytes, outpoint)
+		req.targetPubkey.SerializeCompressed(), outpoint)
 	return nil
 }
 
@@ -1812,109 +1861,21 @@ out:
 func (r *rpcServer) OpenChannelSync(ctx context.Context,
 	in *lnrpc.OpenChannelRequest) (*lnrpc.ChannelPoint, error) {
 
-	rpcsLog.Tracef("[openchannel] request to NodeKey(%v) "+
+	if err := r.canOpenChannel(); err != nil {
-		"allocation(us=%v, them=%v)", in.NodePubkeyString,
-		in.LocalFundingAmount, in.PushSat)
-
-	// We don't allow new channels to be open while the server is still
-	// syncing, as otherwise we may not be able to obtain the relevant
-	// notifications.
-	if !r.server.Started() {
-		return nil, ErrServerNotActive
-	}
-
-	// Creation of channels before the wallet syncs up is currently
-	// disallowed.
-	isSynced, _, err := r.server.cc.wallet.IsSynced()
-	if err != nil {
-		return nil, err
-	}
-	if !isSynced {
-		return nil, errors.New("channels cannot be created before the " +
-			"wallet is fully synced")
-	}
-
-	// Decode the provided target node's public key, parsing it into a pub
-	// key object. For all sync call, byte slices are expected to be
-	// encoded as hex strings.
-	keyBytes, err := hex.DecodeString(in.NodePubkeyString)
-	if err != nil {
-		return nil, err
-	}
-	nodepubKey, err := btcec.ParsePubKey(keyBytes, btcec.S256())
-	if err != nil {
 		return nil, err
 	}
 
-	localFundingAmt := btcutil.Amount(in.LocalFundingAmount)
+	req, err := r.parseOpenChannelReq(in, true)
-	remoteInitialBalance := btcutil.Amount(in.PushSat)
-	minHtlcIn := lnwire.MilliSatoshi(in.MinHtlcMsat)
-	remoteCsvDelay := uint16(in.RemoteCsvDelay)
-
-	// Ensure that the initial balance of the remote party (if pushing
-	// satoshis) does not exceed the amount the local party has requested
-	// for funding.
-	if remoteInitialBalance >= localFundingAmt {
-		return nil, fmt.Errorf("amount pushed to remote peer for " +
-			"initial state must be below the local funding amount")
-	}
-
-	// Restrict the size of the channel we'll actually open. At a later
-	// level, we'll ensure that the output we create after accounting for
-	// fees that a dust output isn't created.
-	if localFundingAmt < minChanFundingSize {
-		return nil, fmt.Errorf("channel is too small, the minimum channel "+
-			"size is: %v SAT", int64(minChanFundingSize))
-	}
-
-	// Then, we'll extract the minimum number of confirmations that each
-	// output we use to fund the channel's funding transaction should
-	// satisfy.
-	minConfs, err := extractOpenChannelMinConfs(in)
 	if err != nil {
 		return nil, err
 	}
 
-	// Based on the passed fee related parameters, we'll determine an
-	// appropriate fee rate for the funding transaction.
-	satPerKw := chainfee.SatPerKVByte(in.SatPerByte * 1000).FeePerKWeight()
-	feeRate, err := sweep.DetermineFeePerKw(
-		r.server.cc.feeEstimator, sweep.FeePreference{
-			ConfTarget: uint32(in.TargetConf),
-			FeeRate:    satPerKw,
-		},
-	)
-	if err != nil {
-		return nil, err
-	}
-
-	rpcsLog.Tracef("[openchannel] target sat/kw for funding tx: %v",
-		int64(feeRate))
-
-	script, err := parseUpfrontShutdownAddress(in.CloseAddress)
-	if err != nil {
-		return nil, fmt.Errorf("error parsing upfront shutdown: %v", err)
-	}
-
-	req := &openChanReq{
-		targetPubkey:    nodepubKey,
-		chainHash:       *activeNetParams.GenesisHash,
-		localFundingAmt: localFundingAmt,
-		pushAmt:         lnwire.NewMSatFromSatoshis(remoteInitialBalance),
-		minHtlcIn:       minHtlcIn,
-		fundingFeePerKw: feeRate,
-		private:         in.Private,
-		remoteCsvDelay:  remoteCsvDelay,
-		minConfs:        minConfs,
-		shutdownScript:  script,
-	}
-
 	updateChan, errChan := r.server.OpenChannel(req)
 	select {
 	// If an error occurs them immediately return the error to the client.
 	case err := <-errChan:
 		rpcsLog.Errorf("unable to open channel to NodeKey(%x): %v",
-			nodepubKey, err)
+			req.targetPubkey.SerializeCompressed(), err)
 		return nil, err
 
 	// Otherwise, wait for the first channel update. The first update sent