From 68881f5c202d2d14c65155799cd92f156c398c8a Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 14 Apr 2023 14:55:39 +0200 Subject: [PATCH 1/6] funding: refactor manager test --- funding/manager_test.go | 120 ++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 79 deletions(-) diff --git a/funding/manager_test.go b/funding/manager_test.go index 43798b230..d7205afd6 100644 --- a/funding/manager_test.go +++ b/funding/manager_test.go @@ -1085,6 +1085,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 @@ -1115,102 +1131,50 @@ 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] } + require.Equal(t, minHtlc, m.HtlcMinimumMsat) - if m.HtlcMinimumMsat != minHtlc { - t.Fatalf("expected ChannelUpdate to "+ - "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 { - 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) // 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) // 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) 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 +1299,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. From af70af27104d6e1fe3e61a09a713835398cd59b5 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 14 Apr 2023 14:55:40 +0200 Subject: [PATCH 2/6] funding+server: update switch after sending channel update With this commit we give the funding manager the ability to inform the switch about custom channel policies, right after we've announced the channel to the network. This change is necessary because before #6753 a channel could only be opened with the default forwarding policies, so the switch automatically had the "correct" default values. Since #6753 added the ability to specify forwarding policies at channel open time, we announced those policies to the network but never updated the switch to inform it about the changed policies (previously changing the policies was only possible through the UpdateChannelPolicy RPC which did call the switch). --- funding/manager.go | 29 +++++++++++++++++++++++++- funding/manager_test.go | 46 ++++++++++++++++++++++++++++++++++++----- server.go | 5 +++-- 3 files changed, 72 insertions(+), 8 deletions(-) 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 d7205afd6..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") @@ -1119,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 { @@ -1147,24 +1173,34 @@ func assertChannelAnnouncements(t *testing.T, alice, bob *testNode, minHtlc = customMinHtlc[j] } require.Equal(t, minHtlc, m.HtlcMinimumMsat) + require.Equal( + t, minHtlc, policyUpdate.MinHTLCOut, + ) // We might expect a custom MaxHltc value. if len(customMaxHtlc) > 0 { maxHtlc = customMaxHtlc[j] } require.Equal(t, maxHtlc, m.HtlcMaximumMsat) + require.Equal(t, maxHtlc, policyUpdate.MaxHTLC) // We might expect a custom baseFee value. if len(baseFees) > 0 { baseFee = baseFees[j] } require.EqualValues(t, baseFee, m.BaseFee) + require.EqualValues( + t, baseFee, policyUpdate.BaseFee, + ) // We might expect a custom feeRate value. if len(feeRates) > 0 { feeRate = feeRates[j] } require.EqualValues(t, feeRate, m.FeeRate) + require.EqualValues( + t, feeRate, policyUpdate.FeeRate, + ) gotChannelUpdate = true } 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 From ebf11bc2f18cafb0fc9e9fea85c76e65c3f2cc39 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 14 Apr 2023 14:55:41 +0200 Subject: [PATCH 3/6] itest: move lnd_custom_{message => message_test}.go This recently added file didn't follow the golang naming convention for tests. --- itest/{lnd_custom_message.go => lnd_custom_message_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename itest/{lnd_custom_message.go => lnd_custom_message_test.go} (100%) 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 From 1eebbe3f42b9e58747fb2a0be80d4a83669174b7 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 14 Apr 2023 14:55:42 +0200 Subject: [PATCH 4/6] itest: update channel open test to send payment This commit enhances the custom fee policy channel open test by adding a second channel and testing forwarding payments through the channel with the custom forwarding policies. This was able to reproduce the bug reported in #5796 which was fixed in a previous commit of this PR. --- itest/lnd_open_channel_test.go | 92 ++++++++++++++++++++++++++-------- 1 file changed, 70 insertions(+), 22 deletions(-) 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, From 5fbaed6b06322107afd706381a71adb97ab01a15 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 18 Apr 2023 11:46:48 +0200 Subject: [PATCH 5/6] peer: add TODO about initial forwarding policy The right way to solve the problem of the link not being up to date with custom user set forwarding policies once the channel is announced would be to pass in those custom values when the link is created initially. This requires a bit more of a refactor and is not addressed in this bug fix. --- peer/brontide.go | 5 +++++ 1 file changed, 5 insertions(+) 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{ From d54bc8974a2956cdd42de9fc7d46dc07142c03e6 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 14 Apr 2023 15:00:08 +0200 Subject: [PATCH 6/6] docs: add release notes --- docs/release-notes/release-notes-0.16.1.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.16.1.md b/docs/release-notes/release-notes-0.16.1.md index 07a0be8e4..6e8fdf0bb 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 @@ -77,6 +78,10 @@ available](https://github.com/lightningnetwork/lnd/pull/7529). * [Put back P2TR as default change type in batch_open_channel](https://github.com/lightningnetwork/lnd/pull/7603). +* [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