Merge pull request #1034 from halseth/funding-custom-csv-delay-bugfix

[bugfix] Custom remote_csv_delay and min_htlc_msat

fundingmanager.go

@@ -81,6 +81,10 @@ type reservationWithCtx struct {
 
 	chanAmt btcutil.Amount
 
+	// Constraints we require for the remote.
+	remoteCsvDelay uint16
+	remoteMinHtlc  lnwire.MilliSatoshi
+
 	updateMtx   sync.RWMutex
 	lastUpdated time.Time
 
@@ -995,8 +999,8 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) {
 	// We'll also validate and apply all the constraints the initiating
 	// party is attempting to dictate for our commitment transaction.
 	err = reservation.CommitConstraints(
-		uint16(msg.CsvDelay), msg.MaxAcceptedHTLCs,
+		msg.CsvDelay, msg.MaxAcceptedHTLCs, msg.MaxValueInFlight,
-		msg.MaxValueInFlight, msg.HtlcMinimum, msg.ChannelReserve,
+		msg.HtlcMinimum, msg.ChannelReserve,
 	)
 	if err != nil {
 		fndgLog.Errorf("Unaccaptable channel constraints: %v", err)
@@ -1005,31 +1009,11 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) {
 		)
 		return
 	}
-	reservation.RegisterMinHTLC(f.cfg.DefaultRoutingPolicy.MinHTLC)
 
 	fndgLog.Infof("Requiring %v confirmations for pendingChan(%x): "+
 		"amt=%v, push_amt=%v", numConfsReq, fmsg.msg.PendingChannelID,
 		amt, msg.PushAmount)
 
-	// Once the reservation has been created successfully, we add it to
-	// this peer's map of pending reservations to track this particular
-	// reservation until either abort or completion.
-	f.resMtx.Lock()
-	if _, ok := f.activeReservations[peerIDKey]; !ok {
-		f.activeReservations[peerIDKey] = make(pendingChannels)
-	}
-	resCtx := &reservationWithCtx{
-		reservation: reservation,
-		chanAmt:     amt,
-		err:         make(chan error, 1),
-		peerAddress: fmsg.peerAddress,
-	}
-	f.activeReservations[peerIDKey][msg.PendingChannelID] = resCtx
-	f.resMtx.Unlock()
-
-	// Update the timestamp once the fundingOpenMsg has been handled.
-	defer resCtx.updateTimestamp()
-
 	// Using the RequiredRemoteDelay closure, we'll compute the remote CSV
 	// delay we require given the total amount of funds within the channel.
 	remoteCsvDelay := f.cfg.RequiredRemoteDelay(amt)
@@ -1038,6 +1022,28 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) {
 	chanReserve := f.cfg.RequiredRemoteChanReserve(amt)
 	maxValue := f.cfg.RequiredRemoteMaxValue(amt)
 	maxHtlcs := f.cfg.RequiredRemoteMaxHTLCs(amt)
+	minHtlc := f.cfg.DefaultRoutingPolicy.MinHTLC
+
+	// Once the reservation has been created successfully, we add it to
+	// this peer's map of pending reservations to track this particular
+	// reservation until either abort or completion.
+	f.resMtx.Lock()
+	if _, ok := f.activeReservations[peerIDKey]; !ok {
+		f.activeReservations[peerIDKey] = make(pendingChannels)
+	}
+	resCtx := &reservationWithCtx{
+		reservation:    reservation,
+		chanAmt:        amt,
+		remoteCsvDelay: remoteCsvDelay,
+		remoteMinHtlc:  minHtlc,
+		err:            make(chan error, 1),
+		peerAddress:    fmsg.peerAddress,
+	}
+	f.activeReservations[peerIDKey][msg.PendingChannelID] = resCtx
+	f.resMtx.Unlock()
+
+	// Update the timestamp once the fundingOpenMsg has been handled.
+	defer resCtx.updateTimestamp()
 
 	// With our parameters set, we'll now process their contribution so we
 	// can move the funding workflow ahead.
@@ -1049,7 +1055,7 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) {
 				DustLimit:        msg.DustLimit,
 				MaxPendingAmount: maxValue,
 				ChanReserve:      chanReserve,
-				MinHTLC:          msg.HtlcMinimum,
+				MinHTLC:          minHtlc,
 				MaxAcceptedHtlcs: maxHtlcs,
 			},
 			CsvDelay: remoteCsvDelay,
@@ -1092,7 +1098,7 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) {
 		MaxValueInFlight:     maxValue,
 		ChannelReserve:       chanReserve,
 		MinAcceptDepth:       uint32(numConfsReq),
-		HtlcMinimum:          ourContribution.MinHTLC,
+		HtlcMinimum:          minHtlc,
 		CsvDelay:             remoteCsvDelay,
 		MaxAcceptedHTLCs:     maxHtlcs,
 		FundingKey:           ourContribution.MultiSigKey.PubKey,
@@ -1148,8 +1154,8 @@ func (f *fundingManager) handleFundingAccept(fmsg *fundingAcceptMsg) {
 	// they've specified for commitment states we can create.
 	resCtx.reservation.SetNumConfsRequired(uint16(msg.MinAcceptDepth))
 	err = resCtx.reservation.CommitConstraints(
-		uint16(msg.CsvDelay), msg.MaxAcceptedHTLCs,
+		msg.CsvDelay, msg.MaxAcceptedHTLCs, msg.MaxValueInFlight,
-		msg.MaxValueInFlight, msg.HtlcMinimum, msg.ChannelReserve,
+		msg.HtlcMinimum, msg.ChannelReserve,
 	)
 	if err != nil {
 		fndgLog.Warnf("Unacceptable channel constraints: %v", err)
@@ -1176,9 +1182,10 @@ func (f *fundingManager) handleFundingAccept(fmsg *fundingAcceptMsg) {
 				DustLimit:        msg.DustLimit,
 				MaxPendingAmount: maxValue,
 				ChanReserve:      chanReserve,
-				MinHTLC:          msg.HtlcMinimum,
+				MinHTLC:          resCtx.remoteMinHtlc,
 				MaxAcceptedHtlcs: maxHtlcs,
 			},
+			CsvDelay: resCtx.remoteCsvDelay,
 			MultiSigKey: keychain.KeyDescriptor{
 				PubKey: copyPubKey(msg.FundingKey),
 			},
@@ -1196,7 +1203,6 @@ func (f *fundingManager) handleFundingAccept(fmsg *fundingAcceptMsg) {
 			},
 		},
 	}
-	remoteContribution.CsvDelay = f.cfg.RequiredRemoteDelay(resCtx.chanAmt)
 	err = resCtx.reservation.ProcessContribution(remoteContribution)
 	if err != nil {
 		fndgLog.Errorf("Unable to process contribution from %v: %v",
@@ -2538,28 +2544,6 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) {
 	fndgLog.Infof("Target commit tx sat/kw for pendingID(%x): %v", chanID,
 		int64(commitFeePerKw))
 
-	// If a pending channel map for this peer isn't already created, then
-	// we create one, ultimately allowing us to track this pending
-	// reservation within the target peer.
-	peerIDKey := newSerializedKey(peerKey)
-	f.resMtx.Lock()
-	if _, ok := f.activeReservations[peerIDKey]; !ok {
-		f.activeReservations[peerIDKey] = make(pendingChannels)
-	}
-
-	resCtx := &reservationWithCtx{
-		chanAmt:     capacity,
-		reservation: reservation,
-		peerAddress: msg.peerAddress,
-		updates:     msg.updates,
-		err:         msg.err,
-	}
-	f.activeReservations[peerIDKey][chanID] = resCtx
-	f.resMtx.Unlock()
-
-	// Update the timestamp once the initFundingMsg has been handled.
-	defer resCtx.updateTimestamp()
-
 	// If the remote CSV delay was not set in the open channel request,
 	// we'll use the RequiredRemoteDelay closure to compute the delay we
 	// require given the total amount of funds within the channel.
@@ -2572,9 +2556,32 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) {
 		minHtlc = f.cfg.DefaultRoutingPolicy.MinHTLC
 	}
 
+	// If a pending channel map for this peer isn't already created, then
+	// we create one, ultimately allowing us to track this pending
+	// reservation within the target peer.
+	peerIDKey := newSerializedKey(peerKey)
+	f.resMtx.Lock()
+	if _, ok := f.activeReservations[peerIDKey]; !ok {
+		f.activeReservations[peerIDKey] = make(pendingChannels)
+	}
+
+	resCtx := &reservationWithCtx{
+		chanAmt:        capacity,
+		remoteCsvDelay: remoteCsvDelay,
+		remoteMinHtlc:  minHtlc,
+		reservation:    reservation,
+		peerAddress:    msg.peerAddress,
+		updates:        msg.updates,
+		err:            msg.err,
+	}
+	f.activeReservations[peerIDKey][chanID] = resCtx
+	f.resMtx.Unlock()
+
+	// Update the timestamp once the initFundingMsg has been handled.
+	defer resCtx.updateTimestamp()
+
 	// Once the reservation has been created, and indexed, queue a funding
 	// request to the remote peer, kicking off the funding workflow.
-	reservation.RegisterMinHTLC(minHtlc)
 	ourContribution := reservation.OurContribution()
 
 	// Finally, we'll use the current value of the channels and our default
@@ -2595,7 +2602,7 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) {
 		DustLimit:            ourContribution.DustLimit,
 		MaxValueInFlight:     maxValue,
 		ChannelReserve:       chanReserve,
-		HtlcMinimum:          ourContribution.MinHTLC,
+		HtlcMinimum:          minHtlc,
 		FeePerKiloWeight:     uint32(commitFeePerKw),
 		CsvDelay:             remoteCsvDelay,
 		MaxAcceptedHTLCs:     maxHtlcs,

fundingmanager_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/contractcourt"
+	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/keychain"
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnwallet"
@@ -282,6 +283,12 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 
 			return nil, fmt.Errorf("unable to find channel")
 		},
+		DefaultRoutingPolicy: htlcswitch.ForwardingPolicy{
+			MinHTLC:       5,
+			BaseFee:       100,
+			FeeRate:       1000,
+			TimeLockDelta: 10,
+		},
 		NumRequiredConfs: func(chanAmt btcutil.Amount,
 			pushAmt lnwire.MilliSatoshi) uint16 {
 			return 3
@@ -1934,3 +1941,202 @@ func TestFundingManagerPrivateRestart(t *testing.T) {
 	// from the database, as the channel is announced.
 	assertNoChannelState(t, alice, bob, fundingOutPoint)
 }
+
+// TestFundingManagerCustomChannelParameters checks that custom requirements we
+// specify during the channel funding flow is preserved correcly on both sides.
+func TestFundingManagerCustomChannelParameters(t *testing.T) {
+	alice, bob := setupFundingManagers(t)
+	defer tearDownFundingManagers(t, alice, bob)
+
+	// This is the custom parameters we'll use.
+	const csvDelay = 67
+	const minHtlc = 1234
+
+	// We will consume the channel updates as we go, so no buffering is
+	// needed.
+	updateChan := make(chan *lnrpc.OpenStatusUpdate)
+
+	// Create a funding request with the custom parameters and start the
+	// workflow.
+	errChan := make(chan error, 1)
+	initReq := &openChanReq{
+		targetPubkey:    bob.privKey.PubKey(),
+		chainHash:       *activeNetParams.GenesisHash,
+		localFundingAmt: 5000000,
+		pushAmt:         lnwire.NewMSatFromSatoshis(0),
+		private:         false,
+		minHtlc:         minHtlc,
+		remoteCsvDelay:  csvDelay,
+		updates:         updateChan,
+		err:             errChan,
+	}
+
+	alice.fundingMgr.initFundingWorkflow(bobAddr, initReq)
+
+	// Alice should have sent the OpenChannel message to Bob.
+	var aliceMsg lnwire.Message
+	select {
+	case aliceMsg = <-alice.msgChan:
+	case err := <-initReq.err:
+		t.Fatalf("error init funding workflow: %v", err)
+	case <-time.After(time.Second * 5):
+		t.Fatalf("alice did not send OpenChannel message")
+	}
+
+	openChannelReq, ok := aliceMsg.(*lnwire.OpenChannel)
+	if !ok {
+		errorMsg, gotError := aliceMsg.(*lnwire.Error)
+		if gotError {
+			t.Fatalf("expected OpenChannel to be sent "+
+				"from bob, instead got error: %v",
+				lnwire.ErrorCode(errorMsg.Data[0]))
+		}
+		t.Fatalf("expected OpenChannel to be sent from "+
+			"alice, instead got %T", aliceMsg)
+	}
+
+	// Check that the custom CSV delay is sent as part of OpenChannel.
+	if openChannelReq.CsvDelay != csvDelay {
+		t.Fatalf("expected OpenChannel to have CSV delay %v, got %v",
+			csvDelay, openChannelReq.CsvDelay)
+	}
+
+	// Check that the custom minHTLC value is sent.
+	if openChannelReq.HtlcMinimum != minHtlc {
+		t.Fatalf("expected OpenChannel to have minHtlc %v, got %v",
+			minHtlc, openChannelReq.HtlcMinimum)
+	}
+
+	chanID := openChannelReq.PendingChannelID
+
+	// Let Bob handle the init message.
+	bob.fundingMgr.processFundingOpen(openChannelReq, aliceAddr)
+
+	// Bob should answer with an AcceptChannel message.
+	acceptChannelResponse := assertFundingMsgSent(
+		t, bob.msgChan, "AcceptChannel",
+	).(*lnwire.AcceptChannel)
+
+	// Bob should require the default delay of 4.
+	if acceptChannelResponse.CsvDelay != 4 {
+		t.Fatalf("expected AcceptChannel to have CSV delay %v, got %v",
+			4, acceptChannelResponse.CsvDelay)
+	}
+
+	// And the default MinHTLC value of 5.
+	if acceptChannelResponse.HtlcMinimum != 5 {
+		t.Fatalf("expected AcceptChannel to have minHtlc %v, got %v",
+			5, acceptChannelResponse.HtlcMinimum)
+	}
+
+	// Forward the response to Alice.
+	alice.fundingMgr.processFundingAccept(acceptChannelResponse, bobAddr)
+
+	// Alice responds with a FundingCreated message.
+	fundingCreated := assertFundingMsgSent(
+		t, alice.msgChan, "FundingCreated",
+	).(*lnwire.FundingCreated)
+
+	// Give the message to Bob.
+	bob.fundingMgr.processFundingCreated(fundingCreated, aliceAddr)
+
+	// Finally, Bob should send the FundingSigned message.
+	fundingSigned := assertFundingMsgSent(
+		t, bob.msgChan, "FundingSigned",
+	).(*lnwire.FundingSigned)
+
+	// Forward the signature to Alice.
+	alice.fundingMgr.processFundingSigned(fundingSigned, bobAddr)
+
+	// After Alice processes the singleFundingSignComplete message, she will
+	// broadcast the funding transaction to the network. We expect to get a
+	// channel update saying the channel is pending.
+	var pendingUpdate *lnrpc.OpenStatusUpdate
+	select {
+	case pendingUpdate = <-updateChan:
+	case <-time.After(time.Second * 5):
+		t.Fatalf("alice did not send OpenStatusUpdate_ChanPending")
+	}
+
+	_, ok = pendingUpdate.Update.(*lnrpc.OpenStatusUpdate_ChanPending)
+	if !ok {
+		t.Fatal("OpenStatusUpdate was not OpenStatusUpdate_ChanPending")
+	}
+
+	// Wait for Alice to published the funding tx to the network.
+	select {
+	case <-alice.publTxChan:
+	case <-time.After(time.Second * 5):
+		t.Fatalf("alice did not publish funding tx")
+	}
+
+	// Helper method for checking the CSV delay stored for a reservation.
+	assertDelay := func(resCtx *reservationWithCtx,
+		ourDelay, theirDelay uint16) error {
+
+		ourCsvDelay := resCtx.reservation.OurContribution().CsvDelay
+		if ourCsvDelay != ourDelay {
+			return fmt.Errorf("expected our CSV delay to be %v, "+
+				"was %v", ourDelay, ourCsvDelay)
+		}
+
+		theirCsvDelay := resCtx.reservation.TheirContribution().CsvDelay
+		if theirCsvDelay != theirDelay {
+			return fmt.Errorf("expected their CSV delay to be %v, "+
+				"was %v", theirDelay, theirCsvDelay)
+		}
+		return nil
+	}
+
+	// Helper method for checking the MinHtlc value stored for a
+	// reservation.
+	assertMinHtlc := func(resCtx *reservationWithCtx,
+		expOurMinHtlc, expTheirMinHtlc lnwire.MilliSatoshi) error {
+
+		ourMinHtlc := resCtx.reservation.OurContribution().MinHTLC
+		if ourMinHtlc != expOurMinHtlc {
+			return fmt.Errorf("expected our minHtlc to be %v, "+
+				"was %v", expOurMinHtlc, ourMinHtlc)
+		}
+
+		theirMinHtlc := resCtx.reservation.TheirContribution().MinHTLC
+		if theirMinHtlc != expTheirMinHtlc {
+			return fmt.Errorf("expected their minHtlc to be %v, "+
+				"was %v", expTheirMinHtlc, theirMinHtlc)
+		}
+		return nil
+	}
+
+	// Check that the custom channel parameters were properly set in the
+	// channel reservation.
+	resCtx, err := alice.fundingMgr.getReservationCtx(bobPubKey, chanID)
+	if err != nil {
+		t.Fatalf("unable to find ctx: %v", err)
+	}
+
+	// Alice's CSV delay should be 4 since Bob sent the fedault value, and
+	// Bob's should be 67 since Alice sent the custom value.
+	if err := assertDelay(resCtx, 4, csvDelay); err != nil {
+		t.Fatal(err)
+	}
+
+	// The minimum HTLC value Alice can offer should be 5, and the minimum
+	// Bob can offer should be 1234.
+	if err := assertMinHtlc(resCtx, 5, minHtlc); err != nil {
+		t.Fatal(err)
+	}
+
+	// Also make sure the parameters are properly set on Bob's end.
+	resCtx, err = bob.fundingMgr.getReservationCtx(alicePubKey, chanID)
+	if err != nil {
+		t.Fatalf("unable to find ctx: %v", err)
+	}
+
+	if err := assertDelay(resCtx, csvDelay, 4); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := assertMinHtlc(resCtx, minHtlc, 5); err != nil {
+		t.Fatal(err)
+	}
+}

lnwallet/reservation.go

@@ -276,16 +276,6 @@ func (r *ChannelReservation) SetNumConfsRequired(numConfs uint16) {
 	r.partialState.NumConfsRequired = numConfs
 }
 
-// RegisterMinHTLC registers our desired amount for the smallest acceptable
-// HTLC we'll accept within this channel. Any HTLC's that are extended which
-// are below this value will SHOULD be rejected.
-func (r *ChannelReservation) RegisterMinHTLC(minHTLC lnwire.MilliSatoshi) {
-	r.Lock()
-	defer r.Unlock()
-
-	r.ourContribution.MinHTLC = minHTLC
-}
-
 // CommitConstraints takes the constraints that the remote party specifies for
 // the type of commitments that we can generate for them. These constraints
 // include several parameters that serve as flow control restricting the amount