Merge pull request #3401 from wpaulino/channel-initiator-max-fee

htlcswitch: avoid proposing fee updates exceeding max fee allowed

config.go

@@ -317,6 +317,8 @@ type config struct {
 
 	MaxOutgoingCltvExpiry uint32 `long:"max-cltv-expiry" description:"The maximum number of blocks funds could be locked up for when forwarding payments."`
 
+	MaxChannelFeeAllocation float64 `long:"max-channel-fee-allocation" description:"The maximum percentage of total funds that can be allocated to a channel's commitment fee. This only applies for the initiator of the channel. Valid values are within [0.1, 1]."`
+
 	net tor.Net
 
 	Routing *routing.Conf `group:"routing" namespace:"routing"`
@@ -433,6 +435,7 @@ func loadConfig() (*config, error) {
 			TowerDir: defaultTowerDir,
 		},
 		MaxOutgoingCltvExpiry:   htlcswitch.DefaultMaxOutgoingCltvExpiry,
+		MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation,
 	}
 
 	// Pre-parse the command line options to pick up an alternative config
@@ -591,6 +594,13 @@ func loadConfig() (*config, error) {
 		return nil, err
 	}
 
+	// Ensure a valid max channel fee allocation was set.
+	if cfg.MaxChannelFeeAllocation <= 0 || cfg.MaxChannelFeeAllocation > 1 {
+		return nil, fmt.Errorf("invalid max channel fee allocation: "+
+			"%v, must be within (0, 1]",
+			cfg.MaxChannelFeeAllocation)
+	}
+
 	// Validate the Tor config parameters.
 	socks, err := lncfg.ParseAddressString(
 		cfg.Tor.SOCKS, strconv.Itoa(defaultTorSOCKSPort),

htlcswitch/link.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"fmt"
+	"math"
 	prand "math/rand"
 	"sync"
 	"sync/atomic"
@@ -50,6 +51,11 @@ const (
 	// DefaultMaxLinkFeeUpdateTimeout represents the maximum interval in
 	// which a link should propose to update its commitment fee rate.
 	DefaultMaxLinkFeeUpdateTimeout = 60 * time.Minute
+
+	// DefaultMaxLinkFeeAllocation is the highest allocation we'll allow
+	// a channel's commitment fee to be of its balance. This only applies to
+	// the initiator of the channel.
+	DefaultMaxLinkFeeAllocation float64 = 0.5
 )
 
 // ForwardingPolicy describes the set of constraints that a given ChannelLink
@@ -250,6 +256,11 @@ type ChannelLinkConfig struct {
 	// accept for a forwarded HTLC. The value is relative to the current
 	// block height.
 	MaxOutgoingCltvExpiry uint32
+
+	// MaxFeeAllocation is the highest allocation we'll allow a channel's
+	// commitment fee to be of its balance. This only applies to the
+	// initiator of the channel.
+	MaxFeeAllocation float64
 }
 
 // channelLink is the service which drives a channel's commitment update
@@ -995,22 +1006,27 @@ out:
 			// If we are the initiator, then we'll sample the
 			// current fee rate to get into the chain within 3
 			// blocks.
-			feePerKw, err := l.sampleNetworkFee()
+			netFee, err := l.sampleNetworkFee()
 			if err != nil {
 				log.Errorf("unable to sample network fee: %v", err)
 				continue
 			}
 
 			// We'll check to see if we should update the fee rate
-			// based on our current set fee rate.
+			// based on our current set fee rate. We'll cap the new
+			// fee rate to our max fee allocation.
 			commitFee := l.channel.CommitFeeRate()
-			if !shouldAdjustCommitFee(feePerKw, commitFee) {
+			maxFee := l.channel.MaxFeeRate(l.cfg.MaxFeeAllocation)
+			newCommitFee := lnwallet.SatPerKWeight(
+				math.Min(float64(netFee), float64(maxFee)),
+			)
+			if !shouldAdjustCommitFee(newCommitFee, commitFee) {
 				continue
 			}
 
 			// If we do, then we'll send a new UpdateFee message to
 			// the remote party, to be locked in with a new update.
-			if err := l.updateChannelFee(feePerKw); err != nil {
+			if err := l.updateChannelFee(newCommitFee); err != nil {
 				log.Errorf("unable to update fee rate: %v", err)
 				continue
 			}

htlcswitch/link_test.go

@@ -1686,6 +1686,7 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) (
 		MinFeeUpdateTimeout:   30 * time.Minute,
 		MaxFeeUpdateTimeout:   40 * time.Minute,
 		MaxOutgoingCltvExpiry: DefaultMaxOutgoingCltvExpiry,
+		MaxFeeAllocation:      DefaultMaxLinkFeeAllocation,
 	}
 
 	const startingHeight = 100
@@ -3736,9 +3737,10 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) {
 	// First, we'll create our traditional three hop network. We'll only be
 	// interacting with and asserting the state of two of the end points
 	// for this test.
+	const aliceInitialBalance = btcutil.SatoshiPerBitcoin * 3
 	channels, cleanUp, _, err := createClusterChannels(
-		btcutil.SatoshiPerBitcoin*3,
+		aliceInitialBalance, btcutil.SatoshiPerBitcoin*5,
-		btcutil.SatoshiPerBitcoin*5)
+	)
 	if err != nil {
 		t.Fatalf("unable to create channel: %v", err)
 	}
@@ -3757,8 +3759,15 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) {
 		{"alice", "bob", &lnwire.FundingLocked{}, false},
 		{"bob", "alice", &lnwire.FundingLocked{}, false},
 
+		// First fee update.
 		{"alice", "bob", &lnwire.UpdateFee{}, false},
+		{"alice", "bob", &lnwire.CommitSig{}, false},
+		{"bob", "alice", &lnwire.RevokeAndAck{}, false},
+		{"bob", "alice", &lnwire.CommitSig{}, false},
+		{"alice", "bob", &lnwire.RevokeAndAck{}, false},
 
+		// Second fee update.
+		{"alice", "bob", &lnwire.UpdateFee{}, false},
 		{"alice", "bob", &lnwire.CommitSig{}, false},
 		{"bob", "alice", &lnwire.RevokeAndAck{}, false},
 		{"bob", "alice", &lnwire.CommitSig{}, false},
@@ -3775,62 +3784,73 @@ func TestChannelLinkUpdateCommitFee(t *testing.T) {
 	defer n.stop()
 	defer n.feeEstimator.Stop()
 
-	// For the sake of this test, we'll reset the timer to fire in a second
-	// so that Alice's link queries for a new network fee.
-	n.aliceChannelLink.updateFeeTimer.Reset(time.Millisecond)
-
 	startingFeeRate := channels.aliceToBob.CommitFeeRate()
 
+	// triggerFeeUpdate is a helper closure to determine whether a fee
+	// update was triggered and completed properly.
+	triggerFeeUpdate := func(feeEstimate, newFeeRate lnwallet.SatPerKWeight,
+		shouldUpdate bool) {
+
+		t.Helper()
+
+		// Record the fee rates before the links process the fee update
+		// to test the case where a fee update isn't triggered.
+		aliceBefore := channels.aliceToBob.CommitFeeRate()
+		bobBefore := channels.bobToAlice.CommitFeeRate()
+
+		// For the sake of this test, we'll reset the timer so that
+		// Alice's link queries for a new network fee.
+		n.aliceChannelLink.updateFeeTimer.Reset(time.Millisecond)
+
 		// Next, we'll send the first fee rate response to Alice.
 		select {
-	case n.feeEstimator.byteFeeIn <- startingFeeRate:
+		case n.feeEstimator.byteFeeIn <- feeEstimate:
 		case <-time.After(time.Second * 5):
 			t.Fatalf("alice didn't query for the new network fee")
 		}
 
+		// Give the links some time to process the fee update.
 		time.Sleep(time.Second)
 
-	// The fee rate on the alice <-> bob channel should still be the same
+		// Record the fee rates after the links have processed the fee
-	// on both sides.
+		// update and ensure they are correct based on whether a fee
-	aliceFeeRate := channels.aliceToBob.CommitFeeRate()
+		// update should have been triggered.
-	bobFeeRate := channels.bobToAlice.CommitFeeRate()
+		aliceAfter := channels.aliceToBob.CommitFeeRate()
-	if aliceFeeRate != startingFeeRate {
+		bobAfter := channels.bobToAlice.CommitFeeRate()
+
+		switch {
+		case shouldUpdate && aliceAfter != newFeeRate:
+			t.Fatalf("alice's fee rate didn't change: expected %v, "+
+				"got %v", newFeeRate, aliceAfter)
+
+		case shouldUpdate && bobAfter != newFeeRate:
+			t.Fatalf("bob's fee rate didn't change: expected %v, "+
+				"got %v", newFeeRate, bobAfter)
+
+		case !shouldUpdate && aliceAfter != aliceBefore:
 			t.Fatalf("alice's fee rate shouldn't have changed: "+
-			"expected %v, got %v", aliceFeeRate, startingFeeRate)
+				"expected %v, got %v", aliceAfter, aliceAfter)
-	}
+
-	if bobFeeRate != startingFeeRate {
+		case !shouldUpdate && bobAfter != bobBefore:
 			t.Fatalf("bob's fee rate shouldn't have changed: "+
-			"expected %v, got %v", bobFeeRate, startingFeeRate)
+				"expected %v, got %v", bobBefore, bobAfter)
+		}
 	}
 
-	// We'll reset the timer once again to ensure Alice's link queries for a
+	// Triggering the link to update the fee of the channel with the same
-	// new network fee.
+	// fee rate should not send a fee update.
-	n.aliceChannelLink.updateFeeTimer.Reset(time.Millisecond)
+	triggerFeeUpdate(startingFeeRate, startingFeeRate, false)
 
-	// Next, we'll set up a deliver a fee rate that's triple the current
+	// Triggering the link to update the fee of the channel with a much
-	// fee rate. This should cause the Alice (the initiator) to trigger a
+	// larger fee rate _should_ send a fee update.
-	// fee update.
 	newFeeRate := startingFeeRate * 3
-	select {
+	triggerFeeUpdate(newFeeRate, newFeeRate, true)
-	case n.feeEstimator.byteFeeIn <- newFeeRate:
-	case <-time.After(time.Second * 5):
-		t.Fatalf("alice didn't query for the new network fee")
-	}
 
-	time.Sleep(time.Second)
+	// Triggering the link to update the fee of the channel with a fee rate
-
+	// that exceeds its maximum fee allocation should result in a fee rate
-	// At this point, Alice should've triggered a new fee update that
+	// corresponding to the maximum fee allocation.
-	// increased the fee rate to match the new rate.
+	const maxFeeRate lnwallet.SatPerKWeight = 207182320
-	aliceFeeRate = channels.aliceToBob.CommitFeeRate()
+	triggerFeeUpdate(maxFeeRate+1, maxFeeRate, true)
-	bobFeeRate = channels.bobToAlice.CommitFeeRate()
-	if aliceFeeRate != newFeeRate {
-		t.Fatalf("alice's fee rate didn't change: expected %v, got %v",
-			newFeeRate, aliceFeeRate)
-	}
-	if bobFeeRate != newFeeRate {
-		t.Fatalf("bob's fee rate didn't change: expected %v, got %v",
-			newFeeRate, aliceFeeRate)
-	}
 }
 
 // TestChannelLinkAcceptDuplicatePayment tests that if a link receives an
@@ -4232,6 +4252,7 @@ func (h *persistentLinkHarness) restartLink(
 		// Set any hodl flags requested for the new link.
 		HodlMask:              hodl.MaskFromFlags(hodlFlags...),
 		MaxOutgoingCltvExpiry: DefaultMaxOutgoingCltvExpiry,
+		MaxFeeAllocation:      DefaultMaxLinkFeeAllocation,
 	}
 
 	const startingHeight = 100

htlcswitch/test_utils.go

@@ -1117,6 +1117,7 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer,
 			OnChannelFailure:        func(lnwire.ChannelID, lnwire.ShortChannelID, LinkFailureError) {},
 			OutgoingCltvRejectDelta: 3,
 			MaxOutgoingCltvExpiry:   DefaultMaxOutgoingCltvExpiry,
+			MaxFeeAllocation:        DefaultMaxLinkFeeAllocation,
 		},
 		channel,
 	)

lnwallet/channel.go

@@ -6280,6 +6280,26 @@ func (lc *LightningChannel) CalcFee(feeRate SatPerKWeight) btcutil.Amount {
 	return feeRate.FeeForWeight(input.CommitWeight)
 }
 
+// MaxFeeRate returns the maximum fee rate given an allocation of the channel
+// initiator's spendable balance. This can be useful to determine when we should
+// stop proposing fee updates that exceed our maximum allocation.
+//
+// NOTE: This should only be used for channels in which the local commitment is
+// the initiator.
+func (lc *LightningChannel) MaxFeeRate(maxAllocation float64) SatPerKWeight {
+	lc.RLock()
+	defer lc.RUnlock()
+
+	// The maximum fee depends of the available balance that can be
+	// committed towards fees.
+	balance, weight := lc.availableBalance()
+	feeBalance := float64(
+		balance.ToSatoshis() + lc.channelState.LocalCommitment.CommitFee,
+	)
+	maxFee := feeBalance * maxAllocation
+	return SatPerKWeight(maxFee / (float64(weight) / 1000))
+}
+
 // RemoteNextRevocation returns the channelState's RemoteNextRevocation.
 func (lc *LightningChannel) RemoteNextRevocation() *btcec.PublicKey {
 	lc.RLock()

peer.go

@@ -585,6 +585,7 @@ func (p *peer) addLink(chanPoint *wire.OutPoint,
 		OutgoingCltvRejectDelta: p.outgoingCltvRejectDelta,
 		TowerClient:             p.server.towerClient,
 		MaxOutgoingCltvExpiry:   cfg.MaxOutgoingCltvExpiry,
+		MaxFeeAllocation:        cfg.MaxChannelFeeAllocation,
 	}
 
 	link := htlcswitch.NewChannelLink(linkCfg, lnChan)