multi: add max local csv config option

To allow nodes more control over the amount of time that their funds will be locked up, we add a MaxLocalCSVDelay option which sets the maximum csv delay we will accept for all channels. We default to the existing constant of 10000, and set a sane minimum on this value so that clients cannot set unreasonably low maximum csv delays which will result in their node rejecting all channels.
ProofOfKeags · Nov 4, 2020 · f4136de · f4136de
1 parent fbb430a
commit f4136de
Show file tree

Hide file tree

Showing 8 changed files with 235 additions and 19 deletions.
diff --git a/config.go b/config.go
@@ -112,6 +112,11 @@ const (
 	// concurrent HTLCs the remote party may add to commitment transactions.
 	// This value can be overridden with --default-remote-max-htlcs.
 	defaultRemoteMaxHtlcs = 483
+
+	// defaultMaxLocalCSVDelay is the maximum delay we accept on our
+	// commitment output.
+	// TODO(halseth): find a more scientific choice of value.
+	defaultMaxLocalCSVDelay = 10000
 )
 
 var (
@@ -344,6 +349,7 @@ func DefaultConfig() Config {
 			BaseFee:       chainreg.DefaultBitcoinBaseFeeMSat,
 			FeeRate:       chainreg.DefaultBitcoinFeeRate,
 			TimeLockDelta: chainreg.DefaultBitcoinTimeLockDelta,
+			MaxLocalDelay: defaultMaxLocalCSVDelay,
 			Node:          "btcd",
 		},
 		BtcdMode: &lncfg.Btcd{
@@ -362,6 +368,7 @@ func DefaultConfig() Config {
 			BaseFee:       chainreg.DefaultLitecoinBaseFeeMSat,
 			FeeRate:       chainreg.DefaultLitecoinFeeRate,
 			TimeLockDelta: chainreg.DefaultLitecoinTimeLockDelta,
+			MaxLocalDelay: defaultMaxLocalCSVDelay,
 			Node:          "ltcd",
 		},
 		LtcdMode: &lncfg.Btcd{
@@ -822,10 +829,11 @@ func ValidateConfig(cfg Config, usageMessage string) (*Config, error) {
 			"litecoin.active must be set to 1 (true)", funcName)
 
 	case cfg.Litecoin.Active:
-		if cfg.Litecoin.TimeLockDelta < minTimeLockDelta {
-			return nil, fmt.Errorf("timelockdelta must be at least %v",
-				minTimeLockDelta)
+		err := cfg.Litecoin.Validate(minTimeLockDelta, minLtcRemoteDelay)
+		if err != nil {
+			return nil, err
 		}
+
 		// Multiple networks can't be selected simultaneously.  Count
 		// number of network flags passed; assign active network params
 		// while we're at it.
@@ -946,9 +954,9 @@ func ValidateConfig(cfg Config, usageMessage string) (*Config, error) {
 			return nil, err
 		}
 
-		if cfg.Bitcoin.TimeLockDelta < minTimeLockDelta {
-			return nil, fmt.Errorf("timelockdelta must be at least %v",
-				minTimeLockDelta)
+		err := cfg.Bitcoin.Validate(minTimeLockDelta, minBtcRemoteDelay)
+		if err != nil {
+			return nil, err
 		}
 
 		switch cfg.Bitcoin.Node {

diff --git a/fundingmanager.go b/fundingmanager.go
@@ -339,6 +339,10 @@ type fundingConfig struct {
 	// incoming channels having a non-zero push amount.
 	RejectPush bool
 
+	// MaxLocalCSVDelay is the maximum csv delay we will allow for our
+	// commit output. Channels that exceed this value will be failed.
+	MaxLocalCSVDelay uint16
+
 	// NotifyOpenChannelEvent informs the ChannelNotifier when channels
 	// transition from pending open to open.
 	NotifyOpenChannelEvent func(wire.OutPoint)
@@ -1342,7 +1346,9 @@ func (f *fundingManager) handleFundingOpen(peer lnpeer.Peer,
 		MaxAcceptedHtlcs: msg.MaxAcceptedHTLCs,
 		CsvDelay:         msg.CsvDelay,
 	}
-	err = reservation.CommitConstraints(channelConstraints)
+	err = reservation.CommitConstraints(
+		channelConstraints, f.cfg.MaxLocalCSVDelay,
+	)
 	if err != nil {
 		fndgLog.Errorf("Unacceptable channel constraints: %v", err)
 		f.failFundingFlow(peer, msg.PendingChannelID, err)
@@ -1525,7 +1531,9 @@ func (f *fundingManager) handleFundingAccept(peer lnpeer.Peer,
 		MaxAcceptedHtlcs: msg.MaxAcceptedHTLCs,
 		CsvDelay:         msg.CsvDelay,
 	}
-	err = resCtx.reservation.CommitConstraints(channelConstraints)
+	err = resCtx.reservation.CommitConstraints(
+		channelConstraints, f.cfg.MaxLocalCSVDelay,
+	)
 	if err != nil {
 		fndgLog.Warnf("Unacceptable channel constraints: %v", err)
 		f.failFundingFlow(peer, msg.PendingChannelID, err)

diff --git a/fundingmanager_test.go b/fundingmanager_test.go
@@ -37,6 +37,7 @@ import (
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -433,6 +434,7 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 		ZombieSweeperInterval:         1 * time.Hour,
 		ReservationTimeout:            1 * time.Nanosecond,
 		MaxChanSize:                   MaxFundingAmount,
+		MaxLocalCSVDelay:              defaultMaxLocalCSVDelay,
 		MaxPendingChannels:            lncfg.DefaultMaxPendingChannels,
 		NotifyOpenChannelEvent:        evt.NotifyOpenChannelEvent,
 		OpenChannelPredicate:          chainedAcceptor,
@@ -1273,6 +1275,159 @@ func TestFundingManagerNormalWorkflow(t *testing.T) {
 	assertNoChannelState(t, alice, bob, fundingOutPoint)
 }
 
+// TestFundingManagerRejectCSV tests checking of local CSV values against our
+// local CSV limit for incoming and outgoing channels.
+func TestFundingManagerRejectCSV(t *testing.T) {
+	t.Run("csv too high", func(t *testing.T) {
+		testLocalCSVLimit(t, 400, 500)
+	})
+	t.Run("csv within limit", func(t *testing.T) {
+		testLocalCSVLimit(t, 600, 500)
+	})
+}
+
+// testLocalCSVLimit creates two funding managers, alice and bob, where alice
+// has a limit on her maximum local CSV and bob sets his required CSV for alice.
+// We test an incoming and outgoing channel, ensuring that alice accepts csvs
+// below her maximum, and rejects those above it.
+func testLocalCSVLimit(t *testing.T, aliceMaxCSV, bobRequiredCSV uint16) {
+	t.Parallel()
+
+	alice, bob := setupFundingManagers(t)
+	defer tearDownFundingManagers(t, alice, bob)
+
+	// Set a maximum local delay in alice's config to aliceMaxCSV and overwrite
+	// bob's required remote delay function to return bobRequiredCSV.
+	alice.fundingMgr.cfg.MaxLocalCSVDelay = aliceMaxCSV
+	bob.fundingMgr.cfg.RequiredRemoteDelay = func(_ btcutil.Amount) uint16 {
+		return bobRequiredCSV
+	}
+
+	// For convenience, we bump our max pending channels to 2 so that we
+	// can test incoming and outgoing channels without needing to step
+	// through the full funding process.
+	alice.fundingMgr.cfg.MaxPendingChannels = 2
+	bob.fundingMgr.cfg.MaxPendingChannels = 2
+
+	// If our maximum is less than the value bob sets, we expect this test
+	// to fail.
+	expectFail := aliceMaxCSV < bobRequiredCSV
+
+	// First, we will initiate an outgoing channel from Alice -> Bob.
+	errChan := make(chan error, 1)
+	updateChan := make(chan *lnrpc.OpenStatusUpdate)
+	initReq := &openChanReq{
+		targetPubkey:    bob.privKey.PubKey(),
+		chainHash:       *fundingNetParams.GenesisHash,
+		localFundingAmt: 200000,
+		fundingFeePerKw: 1000,
+		updates:         updateChan,
+		err:             errChan,
+	}
+
+	// Alice should have sent the OpenChannel message to Bob.
+	alice.fundingMgr.initFundingWorkflow(bob, initReq)
+	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)
+	require.True(t, ok)
+
+	// Let Bob handle the init message.
+	bob.fundingMgr.ProcessFundingMsg(openChannelReq, alice)
+
+	// Bob should answer with an AcceptChannel message.
+	acceptChannelResponse := assertFundingMsgSent(
+		t, bob.msgChan, "AcceptChannel",
+	).(*lnwire.AcceptChannel)
+
+	// They now should both have pending reservations for this channel
+	// active.
+	assertNumPendingReservations(t, alice, bobPubKey, 1)
+	assertNumPendingReservations(t, bob, alicePubKey, 1)
+
+	// Forward the response to Alice.
+	alice.fundingMgr.ProcessFundingMsg(acceptChannelResponse, bob)
+
+	// At this point, Alice has received an AcceptChannel message from
+	// bob with the CSV value that he has set for her, and has to evaluate
+	// whether she wants to accept this channel. If we get an error, we
+	// assert that we expected the channel to fail, otherwise we assert that
+	// she proceeded with the channel open as usual.
+	select {
+	case err := <-errChan:
+		require.Error(t, err)
+		require.True(t, expectFail)
+
+	case msg := <-alice.msgChan:
+		_, ok := msg.(*lnwire.FundingCreated)
+		require.True(t, ok)
+		require.False(t, expectFail)
+
+	case <-time.After(time.Second):
+		t.Fatal("funding flow was not failed")
+	}
+
+	// We do not need to complete the rest of the funding flow (it is
+	// covered in other tests). So now we test that Alice will appropriately
+	// handle incoming channels, opening a channel from Bob->Alice.
+	errChan = make(chan error, 1)
+	updateChan = make(chan *lnrpc.OpenStatusUpdate)
+	initReq = &openChanReq{
+		targetPubkey:    alice.privKey.PubKey(),
+		chainHash:       *fundingNetParams.GenesisHash,
+		localFundingAmt: 200000,
+		fundingFeePerKw: 1000,
+		updates:         updateChan,
+		err:             errChan,
+	}
+
+	bob.fundingMgr.initFundingWorkflow(alice, initReq)
+
+	// Bob should have sent the OpenChannel message to Alice.
+	var bobMsg lnwire.Message
+	select {
+	case bobMsg = <-bob.msgChan:
+
+	case err := <-initReq.err:
+		t.Fatalf("bob OpenChannel message failed: %v", err)
+
+	case <-time.After(time.Second * 5):
+		t.Fatalf("bob did not send OpenChannel message")
+	}
+
+	openChannelReq, ok = bobMsg.(*lnwire.OpenChannel)
+	require.True(t, ok)
+
+	// Let Alice handle the init message.
+	alice.fundingMgr.ProcessFundingMsg(openChannelReq, bob)
+
+	// We expect a error message from Alice if we're expecting the channel
+	// to fail, otherwise we expect her to proceed with the channel as
+	// usual.
+	select {
+	case msg := <-alice.msgChan:
+		var ok bool
+		if expectFail {
+			_, ok = msg.(*lnwire.Error)
+		} else {
+			_, ok = msg.(*lnwire.AcceptChannel)
+		}
+		require.True(t, ok)
+
+	case <-time.After(time.Second * 5):
+		t.Fatal("funding flow was not failed")
+	}
+}
+
 func TestFundingManagerRestartBehavior(t *testing.T) {
 	t.Parallel()
 

diff --git a/lncfg/chain.go b/lncfg/chain.go
@@ -1,6 +1,10 @@
 package lncfg
 
-import "github.com/lightningnetwork/lnd/lnwire"
+import (
+	"fmt"
+
+	"github.com/lightningnetwork/lnd/lnwire"
+)
 
 // Chain holds the configuration options for the daemon's chain settings.
 type Chain struct {
@@ -16,9 +20,28 @@ type Chain struct {
 
 	DefaultNumChanConfs int                 `long:"defaultchanconfs" description:"The default number of confirmations a channel must have before it's considered open. If this is not set, we will scale the value according to the channel size."`
 	DefaultRemoteDelay  int                 `long:"defaultremotedelay" description:"The default number of blocks we will require our channel counterparty to wait before accessing its funds in case of unilateral close. If this is not set, we will scale the value according to the channel size."`
+	MaxLocalDelay       uint16              `long:"maxlocaldelay" description:"The maximum blocks we will allow our funds to be timelocked before accessing its funds in case of unilateral close. If a peer proposes a value greater than this, we will reject the channel."`
 	MinHTLCIn           lnwire.MilliSatoshi `long:"minhtlc" description:"The smallest HTLC we are willing to accept on our channels, in millisatoshi"`
 	MinHTLCOut          lnwire.MilliSatoshi `long:"minhtlcout" description:"The smallest HTLC we are willing to send out on our channels, in millisatoshi"`
 	BaseFee             lnwire.MilliSatoshi `long:"basefee" description:"The base fee in millisatoshi we will charge for forwarding payments on our channels"`
 	FeeRate             lnwire.MilliSatoshi `long:"feerate" description:"The fee rate used when forwarding payments on our channels. The total fee charged is basefee + (amount * feerate / 1000000), where amount is the forwarded amount."`
 	TimeLockDelta       uint32              `long:"timelockdelta" description:"The CLTV delta we will subtract from a forwarded HTLC's timelock value"`
 }
+
+// Validate performs validation on our chain config.
+func (c *Chain) Validate(minTimeLockDelta uint32, minDelay uint16) error {
+	if c.TimeLockDelta < minTimeLockDelta {
+		return fmt.Errorf("timelockdelta must be at least %v",
+			minTimeLockDelta)
+	}
+
+	// Check that our max local delay isn't set below some reasonable
+	// minimum value. We do this to prevent setting an unreasonably low
+	// delay, which would mean that the node would accept no channels.
+	if c.MaxLocalDelay < minDelay {
+		return fmt.Errorf("MaxLocalDelay must be at least: %v",
+			minDelay)
+	}
+
+	return nil
+}
diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go
@@ -91,6 +91,8 @@ var (
 
 	bobAddr, _   = net.ResolveTCPAddr("tcp", "10.0.0.2:9000")
 	aliceAddr, _ = net.ResolveTCPAddr("tcp", "10.0.0.3:9000")
+
+	defaultMaxLocalCsvDelay uint16 = 10000
 )
 
 // assertProperBalance asserts than the total value of the unspent outputs
@@ -447,7 +449,9 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness,
 		MaxAcceptedHtlcs: input.MaxHTLCNumber / 2,
 		CsvDelay:         csvDelay,
 	}
-	err = aliceChanReservation.CommitConstraints(channelConstraints)
+	err = aliceChanReservation.CommitConstraints(
+		channelConstraints, defaultMaxLocalCsvDelay,
+	)
 	if err != nil {
 		t.Fatalf("unable to verify constraints: %v", err)
 	}
@@ -481,7 +485,9 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness,
 	if err != nil {
 		t.Fatalf("bob unable to init channel reservation: %v", err)
 	}
-	err = bobChanReservation.CommitConstraints(channelConstraints)
+	err = bobChanReservation.CommitConstraints(
+		channelConstraints, defaultMaxLocalCsvDelay,
+	)
 	if err != nil {
 		t.Fatalf("unable to verify constraints: %v", err)
 	}
@@ -893,7 +899,9 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness,
 		MaxAcceptedHtlcs: input.MaxHTLCNumber / 2,
 		CsvDelay:         csvDelay,
 	}
-	err = aliceChanReservation.CommitConstraints(channelConstraints)
+	err = aliceChanReservation.CommitConstraints(
+		channelConstraints, defaultMaxLocalCsvDelay,
+	)
 	if err != nil {
 		t.Fatalf("unable to verify constraints: %v", err)
 	}
@@ -934,7 +942,9 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness,
 	if err != nil {
 		t.Fatalf("unable to create bob reservation: %v", err)
 	}
-	err = bobChanReservation.CommitConstraints(channelConstraints)
+	err = bobChanReservation.CommitConstraints(
+		channelConstraints, defaultMaxLocalCsvDelay,
+	)
 	if err != nil {
 		t.Fatalf("unable to verify constraints: %v", err)
 	}

diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go
@@ -383,15 +383,14 @@ func (r *ChannelReservation) SetNumConfsRequired(numConfs uint16) {
 // of satoshis that can be transferred in a single commitment. This function
 // will also attempt to verify the constraints for sanity, returning an error
 // if the parameters are seemed unsound.
-func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints) error {
+func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints,
+	maxLocalCSVDelay uint16) error {
 	r.Lock()
 	defer r.Unlock()
 
-	// Fail if we consider csvDelay excessively large.
-	// TODO(halseth): find a more scientific choice of value.
-	const maxDelay = 10000
-	if c.CsvDelay > maxDelay {
-		return ErrCsvDelayTooLarge(c.CsvDelay, maxDelay)
+	// Fail if the csv delay for our funds exceeds our maximum.
+	if c.CsvDelay > maxLocalCSVDelay {
+		return ErrCsvDelayTooLarge(c.CsvDelay, maxLocalCSVDelay)
 	}
 
 	// The channel reserve should always be greater or equal to the dust