Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing: don't set custom amount if manager isn't handling #9502

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/release-notes/release-notes-0.19.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@
fail to persist (and hence, propagate) node announcements containing address
types (such as a DNS hostname) unknown to LND.

* [The aux bandwidth calculation was fixed for non-asset
channels](https://github.com/lightningnetwork/lnd/pull/9502).

# New Features

* [Support](https://github.com/lightningnetwork/lnd/pull/8390) for
Expand Down
14 changes: 12 additions & 2 deletions htlcswitch/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,18 @@ const (

// OptionalBandwidth is a type alias for the result of a bandwidth query that
// may return a bandwidth value or fn.None if the bandwidth is not available or
// not applicable.
type OptionalBandwidth = fn.Option[lnwire.MilliSatoshi]
// not applicable. IsHandled is set to false if the external traffic shaper does
// not handle the channel in question.
type OptionalBandwidth struct {
// IsHandled is true if the external traffic shaper handles the channel.
// If this is false, then the bandwidth value is not applicable.
IsHandled bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does this set to true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, good catch. Copy/paste mistake. Fixed!
We'll validate all of this with an integration test in litd. Unfortunately we aren't really set up to test these things properly in unit or integration tests with in lnd (but probably worth the investment very soon).


// Bandwidth is the available bandwidth for the channel, as determined
// by the external traffic shaper. If the external traffic shaper is not
// handling the channel, this value will be fn.None.
Bandwidth fn.Option[lnwire.MilliSatoshi]
}

// ChannelLink is an interface which represents the subsystem for managing the
// incoming htlc requests, applying the changes to the channel, and also
Expand Down
21 changes: 14 additions & 7 deletions htlcswitch/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -3443,9 +3443,13 @@ func (l *channelLink) canSendHtlc(policy models.ForwardingPolicy,
return NewLinkError(&lnwire.FailTemporaryNodeFailure{})
}

auxBandwidth.WhenSome(func(bandwidth lnwire.MilliSatoshi) {
availableBandwidth = bandwidth
})
if auxBandwidth.IsHandled && auxBandwidth.Bandwidth.IsSome() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this second arg (auxBandwidth.Bandwidth.IsSome()), if the some is not set we just pass the check below ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is super-defensive I guess. More for the reader of the code than the compiler. But can remove if you feel it has the opposite effect, reducing clarity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh ok yeah let's keep it then.

auxBandwidth.Bandwidth.WhenSome(
func(bandwidth lnwire.MilliSatoshi) {
availableBandwidth = bandwidth
},
)
}

// Check to see if there is enough balance in this channel.
if amt > availableBandwidth {
Expand All @@ -3471,8 +3475,6 @@ func (l *channelLink) AuxBandwidth(amount lnwire.MilliSatoshi,
cid lnwire.ShortChannelID, htlcBlob fn.Option[tlv.Blob],
ts AuxTrafficShaper) fn.Result[OptionalBandwidth] {

unknownBandwidth := fn.None[lnwire.MilliSatoshi]()

fundingBlob := l.FundingCustomBlob()
shouldHandle, err := ts.ShouldHandleTraffic(cid, fundingBlob)
if err != nil {
Expand All @@ -3486,7 +3488,9 @@ func (l *channelLink) AuxBandwidth(amount lnwire.MilliSatoshi,
// If this channel isn't handled by the aux traffic shaper, we'll return
// early.
if !shouldHandle {
return fn.Ok(unknownBandwidth)
return fn.Ok(OptionalBandwidth{
IsHandled: false,
})
}

// Ask for a specific bandwidth to be used for the channel.
Expand All @@ -3502,7 +3506,10 @@ func (l *channelLink) AuxBandwidth(amount lnwire.MilliSatoshi,
log.Debugf("ShortChannelID=%v: aux traffic shaper reported available "+
"bandwidth: %v", cid, auxBandwidth)

return fn.Ok(fn.Some(auxBandwidth))
return fn.Ok(OptionalBandwidth{
IsHandled: true,
Bandwidth: fn.Some(auxBandwidth),
})
}

// Stats returns the statistics of channel link.
Expand Down
2 changes: 1 addition & 1 deletion htlcswitch/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,7 @@ func (f *mockChannelLink) AuxBandwidth(lnwire.MilliSatoshi,
lnwire.ShortChannelID,
fn.Option[tlv.Blob], AuxTrafficShaper) fn.Result[OptionalBandwidth] {

return fn.Ok(fn.None[lnwire.MilliSatoshi]())
return fn.Ok(OptionalBandwidth{})
}

var _ ChannelLink = (*mockChannelLink)(nil)
Expand Down
9 changes: 8 additions & 1 deletion routing/bandwidth.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID,
"auxiliary bandwidth: %w", err))
}

// If the external traffic shaper is not handling the
// channel, we'll just return the original bandwidth and
// no custom amount.
if !auxBandwidth.IsHandled {
return fn.Ok(bandwidthResult{})
}

// We don't know the actual HTLC amount that will be
// sent using the custom channel. But we'll still want
// to make sure we can add another HTLC, using the
Expand All @@ -152,7 +159,7 @@ func (b *bandwidthManager) getBandwidth(cid lnwire.ShortChannelID,
// the max number of HTLCs on the channel. A proper
// balance check is done elsewhere.
return fn.Ok(bandwidthResult{
bandwidth: auxBandwidth,
bandwidth: auxBandwidth.Bandwidth,
htlcAmount: fn.Some[lnwire.MilliSatoshi](0),
})
},
Expand Down
4 changes: 1 addition & 3 deletions routing/mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -904,9 +904,7 @@ func (m *mockLink) AuxBandwidth(lnwire.MilliSatoshi, lnwire.ShortChannelID,
fn.Option[tlv.Blob],
htlcswitch.AuxTrafficShaper) fn.Result[htlcswitch.OptionalBandwidth] {

return fn.Ok[htlcswitch.OptionalBandwidth](
fn.None[lnwire.MilliSatoshi](),
)
return fn.Ok(htlcswitch.OptionalBandwidth{})
}

// EligibleToForward returns the mock's configured eligibility.
Expand Down
Loading