From 990dda4b18a4d10e26d891cd530be48afe3ea161 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 19 Oct 2021 09:37:47 +0200 Subject: [PATCH] multi: thread bandwidth check amount down to MayAddOutgoingHtlc Pass htlc amount down to the channel so that we don't need to rely on minHtlc (and pad it when the channel sets a 0 min htlc). Update test to just check some sane values since we're no longer relying on minHtlc amount at all. --- htlcswitch/interfaces.go | 5 ++-- htlcswitch/link.go | 10 ++++--- htlcswitch/mock.go | 2 +- lnwallet/channel.go | 30 +++++++++++++------- lnwallet/channel_test.go | 33 +++++++++++++++++----- lnwallet/test_utils.go | 4 ++- peer/test_utils.go | 2 +- routing/bandwidth.go | 20 ++++++++----- routing/bandwidth_test.go | 2 +- routing/integrated_routing_context_test.go | 4 +-- routing/mock_test.go | 2 +- routing/pathfind.go | 4 ++- routing/unified_policies.go | 2 +- 13 files changed, 81 insertions(+), 39 deletions(-) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index e12825c53..dc3a91ef9 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -102,8 +102,9 @@ type ChannelUpdateHandler interface { EligibleToForward() bool // MayAddOutgoingHtlc returns an error if we may not add an outgoing - // htlc to the channel. - MayAddOutgoingHtlc() error + // htlc to the channel, taking the amount of the htlc to add as a + // parameter. + MayAddOutgoingHtlc(lnwire.MilliSatoshi) error // ShutdownIfChannelClean shuts the link down if the channel state is // clean. This can be used with dynamic commitment negotiation or coop diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 3d258befe..f347a7f26 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2203,10 +2203,12 @@ func (l *channelLink) Bandwidth() lnwire.MilliSatoshi { return l.channel.AvailableBalance() } -// MayAddOutgoingHtlc indicates whether we may add any more outgoing htlcs to -// this channel. -func (l *channelLink) MayAddOutgoingHtlc() error { - return l.channel.MayAddOutgoingHtlc() +// MayAddOutgoingHtlc indicates whether we can add an outgoing htlc with the +// amount provided to the link. This check does not reserve a space, since +// forwards or other payments may use the available slot, so it should be +// considered best-effort. +func (l *channelLink) MayAddOutgoingHtlc(amt lnwire.MilliSatoshi) error { + return l.channel.MayAddOutgoingHtlc(amt) } // getDustSum is a wrapper method that calls the underlying channel's dust sum diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index b7e073de6..d9ca346bd 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -784,7 +784,7 @@ func (f *mockChannelLink) Peer() lnpeer.Peer { return func (f *mockChannelLink) ChannelPoint() *wire.OutPoint { return &wire.OutPoint{} } func (f *mockChannelLink) Stop() {} func (f *mockChannelLink) EligibleToForward() bool { return f.eligible } -func (f *mockChannelLink) MayAddOutgoingHtlc() error { return nil } +func (f *mockChannelLink) MayAddOutgoingHtlc(lnwire.MilliSatoshi) error { return nil } func (f *mockChannelLink) ShutdownIfChannelClean() error { return nil } func (f *mockChannelLink) setLiveShortChanID(sid lnwire.ShortChannelID) { f.shortChanID = sid } func (f *mockChannelLink) UpdateShortChanID() (lnwire.ShortChannelID, error) { diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 55db9ed51..96f29f884 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5038,19 +5038,29 @@ func (lc *LightningChannel) GetDustSum(remote bool) lnwire.MilliSatoshi { } // MayAddOutgoingHtlc validates whether we can add an outgoing htlc to this -// channel. We don't have a value or circuit for this htlc, because we just -// want to test that we have slots for a potential htlc so we use a "mock" -// htlc to validate a potential commitment state with one more outgoing htlc. -func (lc *LightningChannel) MayAddOutgoingHtlc() error { +// channel. We don't have a circuit for this htlc, because we just want to test +// that we have slots for a potential htlc so we use a "mock" htlc to validate +// a potential commitment state with one more outgoing htlc. If a zero htlc +// amount is provided, we'll attempt to add the smallest possible htlc to the +// channel (either the minimum htlc, or 1 sat). +func (lc *LightningChannel) MayAddOutgoingHtlc(amt lnwire.MilliSatoshi) error { lc.Lock() defer lc.Unlock() - // As this is a mock HTLC, we'll attempt to add the smallest possible - // HTLC permitted in the channel. However certain implementations may - // set this value to zero, so we'll catch that and increment things so - // we always use a non-zero value. - mockHtlcAmt := lc.channelState.LocalChanCfg.MinHTLC - if mockHtlcAmt == 0 { + var mockHtlcAmt lnwire.MilliSatoshi + switch { + // If the caller specifically set an amount, we use it. + case amt != 0: + mockHtlcAmt = amt + + // In absence of a specific amount, we want to use minimum htlc value + // for the channel. However certain implementations may set this value + // to zero, so we only use this value if it is non-zero. + case lc.channelState.LocalChanCfg.MinHTLC != 0: + mockHtlcAmt = lc.channelState.LocalChanCfg.MinHTLC + + // As a last resort, we just add a non-zero amount. + default: mockHtlcAmt++ } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index b22156567..f3b06b655 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9841,10 +9841,9 @@ func TestChannelSignedAckRegression(t *testing.T) { require.NoError(t, err) } -// TestMayAddOutgoingHtlcZeroValue tests that if the minHTLC value of the -// channel is zero, then the MayAddOutgoingHtlc doesn't exit early due to -// running into a zero valued HTLC. -func TestMayAddOutgoingHtlcZeroValue(t *testing.T) { +// TestMayAddOutgoingHtlc tests MayAddOutgoingHtlc against zero and non-zero +// htlc amounts. +func TestMayAddOutgoingHtlc(t *testing.T) { t.Parallel() // The default channel created as a part of the test fixture already @@ -9857,9 +9856,29 @@ func TestMayAddOutgoingHtlcZeroValue(t *testing.T) { defer cleanUp() // The channels start out with a 50/50 balance, so both sides should be - // able to add an outgoing HTLC. - require.NoError(t, aliceChannel.MayAddOutgoingHtlc()) - require.NoError(t, bobChannel.MayAddOutgoingHtlc()) + // able to add an outgoing HTLC with no specific amount added. + require.NoError(t, aliceChannel.MayAddOutgoingHtlc(0)) + require.NoError(t, bobChannel.MayAddOutgoingHtlc(0)) + + chanBal, err := btcutil.NewAmount(testChannelCapacity) + require.NoError(t, err) + + // Each side should be able to add 1/4 of total channel balance since + // we're 50/50 split. + mayAdd := lnwire.MilliSatoshi(chanBal / 4 * 1000) + require.NoError(t, aliceChannel.MayAddOutgoingHtlc(mayAdd)) + require.NoError(t, bobChannel.MayAddOutgoingHtlc(mayAdd)) + + // Both channels should fail if we try to add an amount more than + // their current balance. + mayNotAdd := lnwire.MilliSatoshi(chanBal * 1000) + require.Error(t, aliceChannel.MayAddOutgoingHtlc(mayNotAdd)) + require.Error(t, bobChannel.MayAddOutgoingHtlc(mayNotAdd)) + + // Hard set alice's min htlc to zero and test the case where we just + // fall back to a non-zero value. + aliceChannel.channelState.LocalChanCfg.MinHTLC = 0 + require.NoError(t, aliceChannel.MayAddOutgoingHtlc(0)) } // TestIsChannelClean tests that IsChannelClean returns the expected values diff --git a/lnwallet/test_utils.go b/lnwallet/test_utils.go index 52bf53cfd..ab6e5e1a4 100644 --- a/lnwallet/test_utils.go +++ b/lnwallet/test_utils.go @@ -96,6 +96,8 @@ var ( aliceDustLimit = btcutil.Amount(200) bobDustLimit = btcutil.Amount(1300) + + testChannelCapacity float64 = 10 ) // CreateTestChannels creates to fully populated channels to be used within @@ -109,7 +111,7 @@ var ( func CreateTestChannels(chanType channeldb.ChannelType) ( *LightningChannel, *LightningChannel, func(), error) { - channelCapacity, err := btcutil.NewAmount(10) + channelCapacity, err := btcutil.NewAmount(testChannelCapacity) if err != nil { return nil, nil, nil, err } diff --git a/peer/test_utils.go b/peer/test_utils.go index 8ca0a18f3..f69e42e57 100644 --- a/peer/test_utils.go +++ b/peer/test_utils.go @@ -445,7 +445,7 @@ func (m *mockUpdateHandler) Bandwidth() lnwire.MilliSatoshi { return 0 } func (m *mockUpdateHandler) EligibleToForward() bool { return false } // MayAddOutgoingHtlc currently returns nil. -func (m *mockUpdateHandler) MayAddOutgoingHtlc() error { return nil } +func (m *mockUpdateHandler) MayAddOutgoingHtlc(lnwire.MilliSatoshi) error { return nil } // ShutdownIfChannelClean currently returns nil. func (m *mockUpdateHandler) ShutdownIfChannelClean() error { return nil } diff --git a/routing/bandwidth.go b/routing/bandwidth.go index 4aad0f080..e6084649b 100644 --- a/routing/bandwidth.go +++ b/routing/bandwidth.go @@ -12,8 +12,13 @@ import ( type bandwidthHints interface { // availableChanBandwidth returns the total available bandwidth for a // channel and a bool indicating whether the channel hint was found. - // If the channel is unavailable, a zero amount is returned. - availableChanBandwidth(channelID uint64) (lnwire.MilliSatoshi, bool) + // The amount parameter is used to validate the outgoing htlc amount + // that we wish to add to the channel against its flow restrictions. If + // a zero amount is provided, the minimum htlc value for the channel + // will be used. If the channel is unavailable, a zero amount is + // returned. + availableChanBandwidth(channelID uint64, + amount lnwire.MilliSatoshi) (lnwire.MilliSatoshi, bool) } // getLinkQuery is the function signature used to lookup a link. @@ -65,7 +70,8 @@ func newBandwidthManager(graph routingGraph, sourceNode route.Vertex, // available bandwidth. Note that this function assumes that the channel being // queried is one of our local channels, so any failure to retrieve the link // is interpreted as the link being offline. -func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID) lnwire.MilliSatoshi { +func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID, + amount lnwire.MilliSatoshi) lnwire.MilliSatoshi { link, err := b.getLink(cid) if err != nil { // If the link isn't online, then we'll report that it has @@ -82,7 +88,7 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID) lnwire.MilliS // If our link isn't currently in a state where it can add another // outgoing htlc, treat the link as unusable. - if err := link.MayAddOutgoingHtlc(); err != nil { + if err := link.MayAddOutgoingHtlc(amount); err != nil { return 0 } @@ -94,8 +100,8 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID) lnwire.MilliS // availableChanBandwidth returns the total available bandwidth for a channel // and a bool indicating whether the channel hint was found. If the channel is // unavailable, a zero amount is returned. -func (b *bandwidthManager) availableChanBandwidth(channelID uint64) ( - lnwire.MilliSatoshi, bool) { +func (b *bandwidthManager) availableChanBandwidth(channelID uint64, + amount lnwire.MilliSatoshi) (lnwire.MilliSatoshi, bool) { shortID := lnwire.NewShortChanIDFromInt(channelID) _, ok := b.localChans[shortID] @@ -103,5 +109,5 @@ func (b *bandwidthManager) availableChanBandwidth(channelID uint64) ( return 0, false } - return b.getBandwidth(shortID), true + return b.getBandwidth(shortID, amount), true } diff --git a/routing/bandwidth_test.go b/routing/bandwidth_test.go index b362f2985..9d044b418 100644 --- a/routing/bandwidth_test.go +++ b/routing/bandwidth_test.go @@ -119,7 +119,7 @@ func TestBandwidthManager(t *testing.T) { require.NoError(t, err) bandwidth, found := m.availableChanBandwidth( - testCase.channelID, + testCase.channelID, 10, ) require.Equal(t, testCase.expectedBandwidth, bandwidth) require.Equal(t, testCase.expectFound, found) diff --git a/routing/integrated_routing_context_test.go b/routing/integrated_routing_context_test.go index 008470b7b..79fa37080 100644 --- a/routing/integrated_routing_context_test.go +++ b/routing/integrated_routing_context_test.go @@ -22,8 +22,8 @@ type mockBandwidthHints struct { hints map[uint64]lnwire.MilliSatoshi } -func (m *mockBandwidthHints) availableChanBandwidth(channelID uint64) ( - lnwire.MilliSatoshi, bool) { +func (m *mockBandwidthHints) availableChanBandwidth(channelID uint64, + _ lnwire.MilliSatoshi) (lnwire.MilliSatoshi, bool) { if m.hints == nil { return 0, false diff --git a/routing/mock_test.go b/routing/mock_test.go index d5928f19f..e32e002d5 100644 --- a/routing/mock_test.go +++ b/routing/mock_test.go @@ -760,6 +760,6 @@ func (m *mockLink) EligibleToForward() bool { } // MayAddOutgoingHtlc returns the error configured in our mock. -func (m *mockLink) MayAddOutgoingHtlc() error { +func (m *mockLink) MayAddOutgoingHtlc(_ lnwire.MilliSatoshi) error { return m.mayAddOutgoingErr } diff --git a/routing/pathfind.go b/routing/pathfind.go index 01aea32b6..c07271550 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -373,7 +373,9 @@ func getOutgoingBalance(node route.Vertex, outgoingChans map[uint64]struct{}, } } - bandwidth, ok := bandwidthHints.availableChanBandwidth(chanID) + bandwidth, ok := bandwidthHints.availableChanBandwidth( + chanID, 0, + ) // If the bandwidth is not available, use the channel capacity. // This can happen when a channel is added to the graph after diff --git a/routing/unified_policies.go b/routing/unified_policies.go index 1df166b9b..b54689e13 100644 --- a/routing/unified_policies.go +++ b/routing/unified_policies.go @@ -170,7 +170,7 @@ func (u *unifiedPolicy) getPolicyLocal(amt lnwire.MilliSatoshi, // channel. The bandwidth hint is expected to be // available. bandwidth, ok := bandwidthHints.availableChanBandwidth( - edge.policy.ChannelID, + edge.policy.ChannelID, amt, ) if !ok { bandwidth = lnwire.MaxMilliSatoshi