Merge pull request #7597 from guggero/persistent-fee-params

Update `channelLink` policy correctly when opening channel with custom fee parameters

docs/release-notes/release-notes-0.16.1.md

@@ -69,6 +69,7 @@ https://github.com/lightningnetwork/lnd/pull/7359)
   anything for the users since the message type(36) stays unchanged, except in
   the logging all the appearance of `funding_locked` replated experssion is
   replaced with `channel_ready`.
+
 ## Bug Fixes
 
 * [Fix a bug where lnd crashes when psbt data is not fully 
@@ -80,6 +81,10 @@ available](https://github.com/lightningnetwork/lnd/pull/7529).
 * [Fix log output](https://github.com/lightningnetwork/lnd/pull/7604).
 
 
+* [Channels opened with custom fee policies are now able to forward payments
+  correctly without needing to restart
+  first](https://github.com/lightningnetwork/lnd/pull/7597).
+
 # Contributors (Alphabetical Order)
 
 * ardevd

funding/manager.go

@@ -488,6 +488,11 @@ type Config struct {
 	// transition from pending open to open.
 	NotifyOpenChannelEvent func(wire.OutPoint)
 
+	// UpdateForwardingPolicies is used by the manager to update active
+	// links with a new policy.
+	UpdateForwardingPolicies func(
+		chanPolicies map[wire.OutPoint]htlcswitch.ForwardingPolicy)
+
 	// OpenChannelPredicate is a predicate on the lnwire.OpenChannel message
 	// and on the requesting node's public key that returns a bool which
 	// tells the funding manager whether or not to accept the channel.
@@ -3159,6 +3164,28 @@ func (f *Manager) addToRouterGraph(completeChan *channeldb.OpenChannel,
 		return ErrFundingManagerShuttingDown
 	}
 
+	// The user can define non-default channel policies when opening a
+	// channel. They are stored in the database to be persisted from the
+	// moment of funding the channel to it being confirmed. We just
+	// announced those policies to the network, but we also need to update
+	// our local policy in the switch to make sure we can forward payments
+	// with the correct fees. We can't do this when creating the link
+	// initially as that only takes the static channel parameters.
+	updatedPolicy := map[wire.OutPoint]htlcswitch.ForwardingPolicy{
+		completeChan.FundingOutpoint: {
+			MinHTLCOut: ann.chanUpdateAnn.HtlcMinimumMsat,
+			MaxHTLC:    ann.chanUpdateAnn.HtlcMaximumMsat,
+			BaseFee: lnwire.MilliSatoshi(
+				ann.chanUpdateAnn.BaseFee,
+			),
+			FeeRate: lnwire.MilliSatoshi(
+				ann.chanUpdateAnn.FeeRate,
+			),
+			TimeLockDelta: uint32(ann.chanUpdateAnn.TimeLockDelta),
+		},
+	}
+	f.cfg.UpdateForwardingPolicies(updatedPolicy)
+
 	return nil
 }
 
@@ -3729,7 +3756,7 @@ func (f *Manager) newChanAnnouncement(localPubKey,
 		chanUpdateAnn.FeeRate = uint32(storedFwdingPolicy.FeeRate)
 
 	default:
-		log.Infof("No channel forwaring policy specified for channel "+
+		log.Infof("No channel forwarding policy specified for channel "+
 			"announcement of ChannelID(%v). "+
 			"Assuming default fee parameters.", chanID)
 		chanUpdateAnn.BaseFee = uint32(

funding/manager_test.go

@@ -250,6 +250,7 @@ type testNode struct {
 	testDir         string
 	shutdownChannel chan struct{}
 	reportScidChan  chan struct{}
+	updatedPolicies chan map[wire.OutPoint]htlcswitch.ForwardingPolicy
 	localFeatures   []lnwire.FeatureBit
 	remoteFeatures  []lnwire.FeatureBit
 
@@ -368,6 +369,9 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 	publTxChan := make(chan *wire.MsgTx, 1)
 	shutdownChan := make(chan struct{})
 	reportScidChan := make(chan struct{})
+	updatedPolicies := make(
+		chan map[wire.OutPoint]htlcswitch.ForwardingPolicy, 1,
+	)
 
 	wc := &mock.WalletController{
 		RootKey: alicePrivKey,
@@ -524,6 +528,11 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 			return nil, nil
 		},
 		AliasManager: aliasMgr,
+		UpdateForwardingPolicies: func(
+			p map[wire.OutPoint]htlcswitch.ForwardingPolicy) {
+
+			updatedPolicies <- p
+		},
 	}
 
 	for _, op := range options {
@@ -548,6 +557,7 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 		testDir:         tempTestDir,
 		shutdownChannel: shutdownChan,
 		reportScidChan:  reportScidChan,
+		updatedPolicies: updatedPolicies,
 		addr:            addr,
 	}
 
@@ -627,11 +637,12 @@ func recreateAliceFundingManager(t *testing.T, alice *testNode) {
 		UpdateLabel: func(chainhash.Hash, string) error {
 			return nil
 		},
-		ZombieSweeperInterval: oldCfg.ZombieSweeperInterval,
+		ZombieSweeperInterval:    oldCfg.ZombieSweeperInterval,
-		ReservationTimeout:    oldCfg.ReservationTimeout,
+		ReservationTimeout:       oldCfg.ReservationTimeout,
-		OpenChannelPredicate:  chainedAcceptor,
+		OpenChannelPredicate:     chainedAcceptor,
-		DeleteAliasEdge:       oldCfg.DeleteAliasEdge,
+		DeleteAliasEdge:          oldCfg.DeleteAliasEdge,
-		AliasManager:          oldCfg.AliasManager,
+		AliasManager:             oldCfg.AliasManager,
+		UpdateForwardingPolicies: oldCfg.UpdateForwardingPolicies,
 	})
 	require.NoError(t, err, "failed recreating aliceFundingManager")
 
@@ -1085,6 +1096,22 @@ func assertChannelAnnouncements(t *testing.T, alice, bob *testNode,
 
 	t.Helper()
 
+	// The following checks are to make sure the parameters are used
+	// correctly, as we currently only support 2 values, one for each node.
+	aliceCfg := alice.fundingMgr.cfg
+	if len(customMinHtlc) > 0 {
+		require.Len(t, customMinHtlc, 2, "incorrect usage")
+	}
+	if len(customMaxHtlc) > 0 {
+		require.Len(t, customMaxHtlc, 2, "incorrect usage")
+	}
+	if len(baseFees) > 0 {
+		require.Len(t, baseFees, 2, "incorrect usage")
+	}
+	if len(feeRates) > 0 {
+		require.Len(t, feeRates, 2, "incorrect usage")
+	}
+
 	// After the ChannelReady message is sent, Alice and Bob will each send
 	// the following messages to their gossiper:
 	//	1) ChannelAnnouncement
@@ -1103,6 +1130,21 @@ func assertChannelAnnouncements(t *testing.T, alice, bob *testNode,
 			}
 		}
 
+		// At this point we should also have gotten a policy update that
+		// was sent to the switch subsystem. Make sure it contains the
+		// same values.
+		var policyUpdate htlcswitch.ForwardingPolicy
+		select {
+		case policyUpdateMap := <-node.updatedPolicies:
+			require.Len(t, policyUpdateMap, 1)
+			for _, policy := range policyUpdateMap {
+				policyUpdate = policy
+			}
+
+		case <-time.After(time.Second * 5):
+			t.Fatalf("node didn't send policy update")
+		}
+
 		gotChannelAnnouncement := false
 		gotChannelUpdate := false
 		for _, msg := range announcements {
@@ -1115,102 +1157,60 @@ func assertChannelAnnouncements(t *testing.T, alice, bob *testNode,
 				// advertise the MinHTLC value required by the
 				// _other_ node.
 				other := (j + 1) % 2
-				minHtlc := nodes[other].fundingMgr.cfg.
+				otherCfg := nodes[other].fundingMgr.cfg
-					DefaultMinHtlcIn
+
+				minHtlc := otherCfg.DefaultMinHtlcIn
+				maxHtlc := aliceCfg.RequiredRemoteMaxValue(
+					capacity,
+				)
+				baseFee := aliceCfg.DefaultRoutingPolicy.BaseFee
+				feeRate := aliceCfg.DefaultRoutingPolicy.FeeRate
+
+				require.EqualValues(t, 1, m.MessageFlags)
 
 				// We might expect a custom MinHTLC value.
 				if len(customMinHtlc) > 0 {
-					if len(customMinHtlc) != 2 {
-						t.Fatalf("only 0 or 2 custom " +
-							"min htlc values " +
-							"currently supported")
-					}
-
 					minHtlc = customMinHtlc[j]
 				}
-
+				require.Equal(t, minHtlc, m.HtlcMinimumMsat)
-				if m.HtlcMinimumMsat != minHtlc {
+				require.Equal(
-					t.Fatalf("expected ChannelUpdate to "+
+					t, minHtlc, policyUpdate.MinHTLCOut,
-						"advertise min HTLC %v, had %v",
-						minHtlc, m.HtlcMinimumMsat)
-				}
-
-				maxHtlc := alice.fundingMgr.cfg.RequiredRemoteMaxValue(
-					capacity,
 				)
+
 				// We might expect a custom MaxHltc value.
 				if len(customMaxHtlc) > 0 {
-					if len(customMaxHtlc) != 2 {
-						t.Fatalf("only 0 or 2 custom " +
-							"min htlc values " +
-							"currently supported")
-					}
-
 					maxHtlc = customMaxHtlc[j]
 				}
-				if m.MessageFlags != 1 {
+				require.Equal(t, maxHtlc, m.HtlcMaximumMsat)
-					t.Fatalf("expected message flags to "+
+				require.Equal(t, maxHtlc, policyUpdate.MaxHTLC)
-						"be 1, was %v", m.MessageFlags)
-				}
-
-				if maxHtlc != m.HtlcMaximumMsat {
-					t.Fatalf("expected ChannelUpdate to "+
-						"advertise max HTLC %v, had %v",
-						maxHtlc,
-						m.HtlcMaximumMsat)
-				}
-
-				baseFee := alice.fundingMgr.cfg.DefaultRoutingPolicy.BaseFee
 
 				// We might expect a custom baseFee value.
 				if len(baseFees) > 0 {
-					if len(baseFees) != 2 {
-						t.Fatalf("only 0 or 2 custom " +
-							"base fee values " +
-							"currently supported")
-					}
-
 					baseFee = baseFees[j]
 				}
-
+				require.EqualValues(t, baseFee, m.BaseFee)
-				if uint32(baseFee) != m.BaseFee {
+				require.EqualValues(
-					t.Fatalf("expected ChannelUpdate to "+
+					t, baseFee, policyUpdate.BaseFee,
-						"advertise base fee %v, had %v",
+				)
-						baseFee,
-						m.BaseFee)
-				}
-
-				feeRate := alice.fundingMgr.cfg.DefaultRoutingPolicy.FeeRate
 
 				// We might expect a custom feeRate value.
 				if len(feeRates) > 0 {
-					if len(feeRates) != 2 {
-						t.Fatalf("only 0 or 2 custom " +
-							"fee rate values " +
-							"currently supported")
-					}
-
 					feeRate = feeRates[j]
 				}
-
+				require.EqualValues(t, feeRate, m.FeeRate)
-				if uint32(feeRate) != m.FeeRate {
+				require.EqualValues(
-					t.Fatalf("expected ChannelUpdate to "+
+					t, feeRate, policyUpdate.FeeRate,
-						"advertise base fee %v, had %v",
+				)
-						feeRate,
-						m.FeeRate)
-				}
 
 				gotChannelUpdate = true
 			}
 		}
 
-		if !gotChannelAnnouncement {
+		require.Truef(
-			t.Fatalf("did not get ChannelAnnouncement from node %d",
+			t, gotChannelAnnouncement,
-				j)
+			"ChannelAnnouncement from %d", j,
-		}
+		)
-		if !gotChannelUpdate {
+		require.Truef(t, gotChannelUpdate, "ChannelUpdate from %d", j)
-			t.Fatalf("did not get ChannelUpdate from node %d", j)
-		}
 
 		// Make sure no other message is sent.
 		select {
@@ -1335,14 +1335,12 @@ func assertInitialFwdingPolicyNotFound(t *testing.T, node *testNode,
 			time.Sleep(testPollSleepMs * time.Millisecond)
 		}
 		fwdingPolicy, err = node.fundingMgr.getInitialFwdingPolicy(
-			*chanID)
+			*chanID,
-		if err == channeldb.ErrChannelNotFound {
+		)
-			// Got expected result, return with success.
+		require.ErrorIs(t, err, channeldb.ErrChannelNotFound)
-			return
+
-		} else if err != nil {
+		// Got expected result, return with success.
-			t.Fatalf("unable to get forwarding policy from db: %v",
+		return
-				err)
-		}
 	}
 
 	// 10 tries without success.

itest/lnd_open_channel_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/btcsuite/btcd/btcjson"
+	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/integration/rpctest"
 	"github.com/lightningnetwork/lnd/chainreg"
@@ -159,19 +160,22 @@ func testOpenChannelAfterReorg(ht *lntest.HarnessTest) {
 	ht.CloseChannel(alice, chanPoint)
 }
 
-// testOpenChannelFeePolicy checks if different channel fee scenarios
+// testOpenChannelFeePolicy checks if different channel fee scenarios are
-// are correctly handled when the optional channel fee parameters
+// correctly handled when the optional channel fee parameters baseFee and
-// baseFee and feeRate are provided. If the OpenChannelRequest is not
+// feeRate are provided. If the OpenChannelRequest is not provided with a value
-// provided with a value for baseFee/feeRate the expectation is that the
+// for baseFee/feeRate the expectation is that the default baseFee/feeRate is
-// default baseFee/feeRate is applied.
+// applied.
-// 1.) no params provided to OpenChannelRequest
+//
-// ChannelUpdate --> defaultBaseFee, defaultFeeRate
+//  1. No params provided to OpenChannelRequest:
-// 2.) only baseFee provided to OpenChannelRequest
+//     ChannelUpdate --> defaultBaseFee, defaultFeeRate
-// ChannelUpdate --> provided baseFee, defaultFeeRate
+//  2. Only baseFee provided to OpenChannelRequest:
-// 3.) only feeRate provided to OpenChannelRequest
+//     ChannelUpdate --> provided baseFee, defaultFeeRate
-// ChannelUpdate --> defaultBaseFee, provided FeeRate
+//  3. Only feeRate provided to OpenChannelRequest:
-// 4.) baseFee and feeRate provided to OpenChannelRequest
+//     ChannelUpdate --> defaultBaseFee, provided FeeRate
-// ChannelUpdate --> provided baseFee, provided feeRate.
+//  4. baseFee and feeRate provided to OpenChannelRequest:
+//     ChannelUpdate --> provided baseFee, provided feeRate
+//  5. Both baseFee and feeRate are set to a value lower than the default:
+//     ChannelUpdate --> provided baseFee, provided feeRate
 func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 	const (
 		defaultBaseFee       = 1000
@@ -180,6 +184,8 @@ func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 		defaultMinHtlc       = 1000
 		optionalBaseFee      = 1337
 		optionalFeeRate      = 1337
+		lowBaseFee           = 0
+		lowFeeRate           = 900
 	)
 
 	defaultMaxHtlc := lntest.CalculateMaxHtlc(funding.MaxBtcFundingAmount)
@@ -216,6 +222,14 @@ func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 			UseBaseFee: true,
 			UseFeeRate: true,
 		},
+		{
+			Amt:        chanAmt,
+			PushAmt:    pushAmt,
+			BaseFee:    lowBaseFee,
+			FeeRate:    lowFeeRate,
+			UseBaseFee: true,
+			UseFeeRate: true,
+		},
 	}
 
 	expectedPolicies := []lnrpc.RoutingPolicy{
@@ -247,6 +261,13 @@ func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 			MinHtlc:          defaultMinHtlc,
 			MaxHtlcMsat:      defaultMaxHtlc,
 		},
+		{
+			FeeBaseMsat:      lowBaseFee,
+			FeeRateMilliMsat: lowFeeRate,
+			TimeLockDelta:    defaultTimeLockDelta,
+			MinHtlc:          defaultMinHtlc,
+			MaxHtlcMsat:      defaultMaxHtlc,
+		},
 	}
 
 	bobExpectedPolicy := lnrpc.RoutingPolicy{
@@ -257,19 +278,29 @@ func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 		MaxHtlcMsat:      defaultMaxHtlc,
 	}
 
+	// In this basic test, we'll need a third node, Carol, so we can forward
+	// a payment through the channel we'll open with the different fee
+	// policies.
+	carol := ht.NewNode("Carol", nil)
+
 	alice, bob := ht.Alice, ht.Bob
+	nodes := []*node.HarnessNode{alice, bob, carol}
 
 	runTestCase := func(ht *lntest.HarnessTest,
-		fs lntest.OpenChannelParams,
+		chanParams lntest.OpenChannelParams,
 		alicePolicy, bobPolicy *lnrpc.RoutingPolicy) {
 
 		// Create a channel Alice->Bob.
-		chanPoint := ht.OpenChannel(alice, bob, fs)
+		chanPoint := ht.OpenChannel(alice, bob, chanParams)
 		defer ht.CloseChannel(alice, chanPoint)
 
-		// We add all the nodes' update channels to a slice, such that
+		// Create a channel Carol->Alice.
-		// we can make sure they all receive the expected updates.
+		chanPoint2 := ht.OpenChannel(
-		nodes := []*node.HarnessNode{alice, bob}
+			carol, alice, lntest.OpenChannelParams{
+				Amt: 500000,
+			},
+		)
+		defer ht.CloseChannel(carol, chanPoint2)
 
 		// Alice and Bob should see each other's ChannelUpdates,
 		// advertising the preferred routing policies.
@@ -279,20 +310,37 @@ func testOpenChannelUpdateFeePolicy(ht *lntest.HarnessTest) {
 		assertNodesPolicyUpdate(ht, nodes, bob, bobPolicy, chanPoint)
 
 		// They should now know about the default policies.
-		for _, node := range nodes {
+		for _, n := range nodes {
 			ht.AssertChannelPolicy(
-				node, alice.PubKeyStr, alicePolicy, chanPoint,
+				n, alice.PubKeyStr, alicePolicy, chanPoint,
 			)
 			ht.AssertChannelPolicy(
-				node, bob.PubKeyStr, bobPolicy, chanPoint,
+				n, bob.PubKeyStr, bobPolicy, chanPoint,
 			)
 		}
+
+		// We should be able to forward a payment from Carol to Bob
+		// through the new channel we opened.
+		payReqs, _, _ := ht.CreatePayReqs(bob, paymentAmt, 1)
+		ht.CompletePaymentRequests(carol, payReqs)
 	}
 
 	for i, feeScenario := range feeScenarios {
 		ht.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 			st := ht.Subtest(t)
-			ht.EnsureConnected(alice, bob)
+			st.EnsureConnected(alice, bob)
+
+			st.RestartNode(carol)
+
+			// Because we're using ht.Subtest(), we need to restart
+			// any node we have to refresh its runtime context.
+			// Otherwise, we'll get a "context canceled" error on
+			// RPC calls.
+			st.EnsureConnected(alice, carol)
+
+			// Send Carol enough coins to be able to open a channel
+			// to Alice.
+			ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
 
 			runTestCase(
 				st, feeScenario,

peer/brontide.go

@@ -2437,6 +2437,11 @@ out:
 			// values, as they currently are always set to the default values
 			// at initial channel creation. Note that the maximum HTLC value
 			// defaults to the cap on the total value of outstanding HTLCs.
+			//
+			// TODO(guggero): We should instead pass in the current
+			// forwarding policy from the funding manager to avoid
+			// needing us to update the link once the channel is
+			// announced to the network with custom user values.
 			fwdMinHtlc := lnChan.FwdMinHtlc()
 			defaultPolicy := p.cfg.RoutingPolicy
 			forwardingPolicy := &htlcswitch.ForwardingPolicy{

server.go

@@ -1439,8 +1439,9 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		RegisteredChains:              cfg.registeredChains,
 		MaxAnchorsCommitFeeRate: chainfee.SatPerKVByte(
 			s.cfg.MaxCommitFeeRateAnchors * 1000).FeePerKWeight(),
-		DeleteAliasEdge: deleteAliasEdge,
+		DeleteAliasEdge:          deleteAliasEdge,
-		AliasManager:    s.aliasMgr,
+		AliasManager:             s.aliasMgr,
+		UpdateForwardingPolicies: s.htlcSwitch.UpdateForwardingPolicies,
 	})
 	if err != nil {
 		return nil, err