diff --git a/docs/release-notes/release-notes-0.16.1.md b/docs/release-notes/release-notes-0.16.1.md index 1ce156bb3..771997056 100644 --- a/docs/release-notes/release-notes-0.16.1.md +++ b/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 diff --git a/funding/manager.go b/funding/manager.go index c55cbba32..ad66ba1c1 100644 --- a/funding/manager.go +++ b/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( diff --git a/funding/manager_test.go b/funding/manager_test.go index 43798b230..ec9a05c0f 100644 --- a/funding/manager_test.go +++ b/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, - ReservationTimeout: oldCfg.ReservationTimeout, - OpenChannelPredicate: chainedAcceptor, - DeleteAliasEdge: oldCfg.DeleteAliasEdge, - AliasManager: oldCfg.AliasManager, + ZombieSweeperInterval: oldCfg.ZombieSweeperInterval, + ReservationTimeout: oldCfg.ReservationTimeout, + OpenChannelPredicate: chainedAcceptor, + DeleteAliasEdge: oldCfg.DeleteAliasEdge, + 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. - DefaultMinHtlcIn + otherCfg := nodes[other].fundingMgr.cfg + + 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] } - - if m.HtlcMinimumMsat != minHtlc { - t.Fatalf("expected ChannelUpdate to "+ - "advertise min HTLC %v, had %v", - minHtlc, m.HtlcMinimumMsat) - } - - maxHtlc := alice.fundingMgr.cfg.RequiredRemoteMaxValue( - capacity, + require.Equal(t, minHtlc, m.HtlcMinimumMsat) + require.Equal( + t, minHtlc, policyUpdate.MinHTLCOut, ) + // 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 { - t.Fatalf("expected message flags to "+ - "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 + require.Equal(t, maxHtlc, m.HtlcMaximumMsat) + require.Equal(t, maxHtlc, policyUpdate.MaxHTLC) // 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] } - - if uint32(baseFee) != m.BaseFee { - t.Fatalf("expected ChannelUpdate to "+ - "advertise base fee %v, had %v", - baseFee, - m.BaseFee) - } - - feeRate := alice.fundingMgr.cfg.DefaultRoutingPolicy.FeeRate + require.EqualValues(t, baseFee, m.BaseFee) + require.EqualValues( + t, baseFee, policyUpdate.BaseFee, + ) // 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] } - - if uint32(feeRate) != m.FeeRate { - t.Fatalf("expected ChannelUpdate to "+ - "advertise base fee %v, had %v", - feeRate, - m.FeeRate) - } + require.EqualValues(t, feeRate, m.FeeRate) + require.EqualValues( + t, feeRate, policyUpdate.FeeRate, + ) gotChannelUpdate = true } } - if !gotChannelAnnouncement { - t.Fatalf("did not get ChannelAnnouncement from node %d", - j) - } - if !gotChannelUpdate { - t.Fatalf("did not get ChannelUpdate from node %d", j) - } + require.Truef( + t, gotChannelAnnouncement, + "ChannelAnnouncement from %d", j, + ) + require.Truef(t, gotChannelUpdate, "ChannelUpdate from %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) - if err == channeldb.ErrChannelNotFound { - // Got expected result, return with success. - return - } else if err != nil { - t.Fatalf("unable to get forwarding policy from db: %v", - err) - } + *chanID, + ) + require.ErrorIs(t, err, channeldb.ErrChannelNotFound) + + // Got expected result, return with success. + return } // 10 tries without success. diff --git a/itest/lnd_custom_message.go b/itest/lnd_custom_message_test.go similarity index 100% rename from itest/lnd_custom_message.go rename to itest/lnd_custom_message_test.go diff --git a/itest/lnd_open_channel_test.go b/itest/lnd_open_channel_test.go index bfe80cbd1..744d211f1 100644 --- a/itest/lnd_open_channel_test.go +++ b/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 -// are correctly handled when the optional channel fee parameters -// baseFee and feeRate are provided. If the OpenChannelRequest is not -// provided with a value for baseFee/feeRate the expectation is that the -// default baseFee/feeRate is applied. -// 1.) no params provided to OpenChannelRequest -// ChannelUpdate --> defaultBaseFee, defaultFeeRate -// 2.) only baseFee provided to OpenChannelRequest -// ChannelUpdate --> provided baseFee, defaultFeeRate -// 3.) only feeRate provided to OpenChannelRequest -// ChannelUpdate --> defaultBaseFee, provided FeeRate -// 4.) baseFee and feeRate provided to OpenChannelRequest -// ChannelUpdate --> provided baseFee, provided feeRate. +// testOpenChannelFeePolicy checks if different channel fee scenarios are +// correctly handled when the optional channel fee parameters baseFee and +// feeRate are provided. If the OpenChannelRequest is not provided with a value +// for baseFee/feeRate the expectation is that the default baseFee/feeRate is +// applied. +// +// 1. No params provided to OpenChannelRequest: +// ChannelUpdate --> defaultBaseFee, defaultFeeRate +// 2. Only baseFee provided to OpenChannelRequest: +// ChannelUpdate --> provided baseFee, defaultFeeRate +// 3. Only feeRate provided to OpenChannelRequest: +// ChannelUpdate --> defaultBaseFee, 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 - // we can make sure they all receive the expected updates. - nodes := []*node.HarnessNode{alice, bob} + // Create a channel Carol->Alice. + chanPoint2 := ht.OpenChannel( + 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, diff --git a/peer/brontide.go b/peer/brontide.go index edc6c6b51..82b11e310 100644 --- a/peer/brontide.go +++ b/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{ diff --git a/server.go b/server.go index 2224976e5..7d76353de 100644 --- a/server.go +++ b/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, - AliasManager: s.aliasMgr, + DeleteAliasEdge: deleteAliasEdge, + AliasManager: s.aliasMgr, + UpdateForwardingPolicies: s.htlcSwitch.UpdateForwardingPolicies, }) if err != nil { return nil, err