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

lnwallet+htlcswitch: make Switch dust-aware #5770

Merged
merged 6 commits into from
Oct 1, 2021
Merged
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 config.go
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,8 @@ type Config struct {

GcCanceledInvoicesOnTheFly bool `long:"gc-canceled-invoices-on-the-fly" description:"If true, we'll delete newly canceled invoices on the fly."`

DustThreshold uint64 `long:"dust-threshold" description:"Sets the dust sum threshold in satoshis for a channel after which dust HTLC's will be failed."`
carlaKC marked this conversation as resolved.
Show resolved Hide resolved

Invoices *lncfg.Invoices `group:"invoices" namespace:"invoices"`

Routing *lncfg.Routing `group:"routing" namespace:"routing"`
Expand Down Expand Up @@ -550,6 +552,7 @@ func DefaultConfig() Config {
MaxOutgoingCltvExpiry: htlcswitch.DefaultMaxOutgoingCltvExpiry,
MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation,
MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte,
DustThreshold: uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()),
LogWriter: build.NewRotatingLogWriter(),
DB: lncfg.DefaultDB(),
Cluster: lncfg.DefaultCluster(),
Expand Down
4 changes: 4 additions & 0 deletions docs/release-notes/release-notes-0.13.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

* [The `DefaultDustLimit` method has been removed in favor of `DustLimitForSize` which calculates the proper network dust limit for a given output size. This also fixes certain APIs like SendCoins to be able to send 294 sats to a P2WPKH script.](https://github.com/lightningnetwork/lnd/pull/5781)

## Safety

* [The `htlcswitch.Switch` has been modified to take into account the total dust sum on the incoming and outgoing channels before forwarding. After the defined threshold is reached, dust HTLC's will start to be failed. The default threshold is 500K satoshis and can be modified by setting `--dust-threshold=` when running `lnd`.](https://github.com/lightningnetwork/lnd/pull/5770)

# Contributors (Alphabetical Order)

* Eugene Siegel
19 changes: 19 additions & 0 deletions htlcswitch/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/lightningnetwork/lnd/lnpeer"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/lightningnetwork/lnd/lnwire"
"github.com/lightningnetwork/lnd/record"
)
Expand Down Expand Up @@ -57,6 +58,21 @@ type packetHandler interface {
handleLocalAddPacket(*htlcPacket) error
}

// dustHandler is an interface used exclusively by the Switch to evaluate
// whether a link has too much dust exposure.
type dustHandler interface {
// getDustSum returns the dust sum on either the local or remote
// commitment.
getDustSum(remote bool) lnwire.MilliSatoshi

// getFeeRate returns the current channel feerate.
getFeeRate() chainfee.SatPerKWeight

// getDustClosure returns a closure that can evaluate whether a passed
// HTLC is dust.
getDustClosure() dustClosure
}

// ChannelUpdateHandler is an interface that provides methods that allow
// sending lnwire.Message to the underlying link as well as querying state.
type ChannelUpdateHandler interface {
Expand Down Expand Up @@ -122,6 +138,9 @@ type ChannelLink interface {
// Embed the ChannelUpdateHandler interface.
ChannelUpdateHandler

// Embed the dustHandler interface.
dustHandler

// ChannelPoint returns the channel outpoint for the channel link.
ChannelPoint() *wire.OutPoint

Expand Down
75 changes: 75 additions & 0 deletions htlcswitch/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/btcsuite/btcd/wire"
"github.com/btcsuite/btclog"
"github.com/btcsuite/btcutil"
"github.com/davecgh/go-spew/spew"
"github.com/go-errors/errors"
"github.com/lightningnetwork/lnd/build"
Expand Down Expand Up @@ -1929,6 +1930,10 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) {
"error receiving fee update: %v", err)
return
}

// Update the mailbox's feerate as well.
l.mailBox.SetFeeRate(fee)
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved

case *lnwire.Error:
// Error received from remote, MUST fail channel, but should
// only print the contents of the error message if all
Expand Down Expand Up @@ -2192,6 +2197,64 @@ func (l *channelLink) MayAddOutgoingHtlc() error {
return l.channel.MayAddOutgoingHtlc()
}

// getDustSum is a wrapper method that calls the underlying channel's dust sum
// method.
//
// NOTE: Part of the dustHandler interface.
func (l *channelLink) getDustSum(remote bool) lnwire.MilliSatoshi {
return l.channel.GetDustSum(remote)
}

// getFeeRate is a wrapper method that retrieves the underlying channel's
// feerate.
//
// NOTE: Part of the dustHandler interface.
func (l *channelLink) getFeeRate() chainfee.SatPerKWeight {
return l.channel.CommitFeeRate()
}

// getDustClosure returns a closure that can be used by the switch or mailbox
// to evaluate whether a given HTLC is dust.
//
// NOTE: Part of the dustHandler interface.
func (l *channelLink) getDustClosure() dustClosure {
localDustLimit := l.channel.State().LocalChanCfg.DustLimit
remoteDustLimit := l.channel.State().RemoteChanCfg.DustLimit
chanType := l.channel.State().ChanType

return dustHelper(chanType, localDustLimit, remoteDustLimit)
}

// dustClosure is a function that evaluates whether an HTLC is dust. It returns
// true if the HTLC is dust. It takes in a feerate, a boolean denoting whether
// the HTLC is incoming (i.e. one that the remote sent), a boolean denoting
// whether to evaluate on the local or remote commit, and finally an HTLC
// amount to test.
type dustClosure func(chainfee.SatPerKWeight, bool, bool, btcutil.Amount) bool
carlaKC marked this conversation as resolved.
Show resolved Hide resolved

// dustHelper is used to construct the dustClosure.
func dustHelper(chantype channeldb.ChannelType, localDustLimit,
remoteDustLimit btcutil.Amount) dustClosure {

isDust := func(feerate chainfee.SatPerKWeight, incoming,
localCommit bool, amt btcutil.Amount) bool {

if localCommit {
return lnwallet.HtlcIsDust(
chantype, incoming, true, feerate, amt,
localDustLimit,
)
}

return lnwallet.HtlcIsDust(
chantype, incoming, false, feerate, amt,
remoteDustLimit,
)
}

return isDust
}

// AttachMailBox updates the current mailbox used by this link, and hooks up
// the mailbox's message and packet outboxes to the link's upstream and
// downstream chans, respectively.
Expand All @@ -2201,6 +2264,14 @@ func (l *channelLink) AttachMailBox(mailbox MailBox) {
l.upstream = mailbox.MessageOutBox()
l.downstream = mailbox.PacketOutBox()
l.Unlock()

// Set the mailbox's fee rate. This may be refreshing a feerate that was
// never committed.
l.mailBox.SetFeeRate(l.getFeeRate())

// Also set the mailbox's dust closure so that it can query whether HTLC's
// are dust given the current feerate.
l.mailBox.SetDustClosure(l.getDustClosure())
}

// UpdateForwardingPolicy updates the forwarding policy for the target
Expand Down Expand Up @@ -2501,6 +2572,10 @@ func (l *channelLink) updateChannelFee(feePerKw chainfee.SatPerKWeight) error {
return err
}

// The fee passed the channel's validation checks, so we update the
// mailbox feerate.
l.mailBox.SetFeeRate(feePerKw)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it a problem if we set fee rate here but then fail to send the new fee rate through to our peer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The assumption I made here is that SendMessage either sends the message or doesn't send the message. If it doesn't send the message, the connection died and the link is shutdown. This assumes that brontide.Noise doesn't drop messages or get into some weird state. On restart the mailbox will be called with SetFeeRate(local commitment fee rate).

Typing this out, I realize that the mailbox could have an outdated feerate:

  • ---update_fee--->
  • ---commit_sig--->
  • LHS dies
  • * AttachMailBox with LocalCommit feerate, which won't get immediately refreshed

Could add another method to LightningChannel to retrieve the remote commitment tip feerate if we're initiator (which will always be the latest), but honestly don't think it's necessary as it should eventually get to the feerate after a link restart or another fee update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, agree it's not worth the extra code, maybe just drop a comment on the mailbox feeRate field that this can be briefly out of sync with the channel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment added


// We'll then attempt to send a new UpdateFee message, and also lock it
// in immediately by triggering a commitment update.
msg := lnwire.NewUpdateFee(l.ChanID(), uint32(feePerKw))
Expand Down
78 changes: 78 additions & 0 deletions htlcswitch/mailbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/lightningnetwork/lnd/clock"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/lightningnetwork/lnd/lnwire"
)

Expand Down Expand Up @@ -66,6 +67,17 @@ type MailBox interface {
// Reset the packet head to point at the first element in the list.
ResetPackets() error

// SetDustClosure takes in a closure that is used to evaluate whether
// mailbox HTLC's are dust.
SetDustClosure(isDust dustClosure)

// SetFeeRate sets the feerate to be used when evaluating dust.
SetFeeRate(feerate chainfee.SatPerKWeight)

// DustPackets returns the dust sum for Adds in the mailbox for the
// local and remote commitments.
DustPackets() (lnwire.MilliSatoshi, lnwire.MilliSatoshi)
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved

// Start starts the mailbox and any goroutines it needs to operate
// properly.
Start()
Expand Down Expand Up @@ -131,6 +143,17 @@ type memoryMailBox struct {
wireShutdown chan struct{}
pktShutdown chan struct{}
quit chan struct{}

// feeRate is set when the link receives or sends out fee updates. It
// is refreshed when AttachMailBox is called in case a fee update did
// not get committed. In some cases it may be out of sync with the
// channel's feerate, but it should eventually get back in sync.
feeRate chainfee.SatPerKWeight

// isDust is set when AttachMailBox is called and serves to evaluate
// the outstanding dust in the memoryMailBox given the current set
// feeRate.
isDust dustClosure
}

// newMemoryMailBox creates a new instance of the memoryMailBox.
Expand Down Expand Up @@ -610,6 +633,61 @@ func (m *memoryMailBox) AddPacket(pkt *htlcPacket) error {
return nil
}

// SetFeeRate sets the memoryMailBox's feerate for use in DustPackets.
func (m *memoryMailBox) SetFeeRate(feeRate chainfee.SatPerKWeight) {
m.pktCond.L.Lock()
defer m.pktCond.L.Unlock()

m.feeRate = feeRate
}

// SetDustClosure sets the memoryMailBox's dustClosure for use in DustPackets.
func (m *memoryMailBox) SetDustClosure(isDust dustClosure) {
m.pktCond.L.Lock()
defer m.pktCond.L.Unlock()

m.isDust = isDust
}

// DustPackets returns the dust sum for add packets in the mailbox. The first
// return value is the local dust sum and the second is the remote dust sum.
// This will keep track of a given dust HTLC from the time it is added via
// AddPacket until it is removed via AckPacket.
func (m *memoryMailBox) DustPackets() (lnwire.MilliSatoshi,
lnwire.MilliSatoshi) {

m.pktCond.L.Lock()
defer m.pktCond.L.Unlock()

var (
localDustSum lnwire.MilliSatoshi
remoteDustSum lnwire.MilliSatoshi
)

// Run through the map of HTLC's and determine the dust sum with calls
// to the memoryMailBox's isDust closure. Note that all mailbox packets
// are outgoing so the second argument to isDust will be false.
for _, e := range m.addIndex {
addPkt := e.Value.(*pktWithExpiry).pkt

// Evaluate whether this HTLC is dust on the local commitment.
if m.isDust(
m.feeRate, false, true, addPkt.amount.ToSatoshis(),
) {
localDustSum += addPkt.amount
}

// Evaluate whether this HTLC is dust on the remote commitment.
if m.isDust(
m.feeRate, false, false, addPkt.amount.ToSatoshis(),
) {
remoteDustSum += addPkt.amount
}
}

return localDustSum, remoteDustSum
}

// FailAdd fails an UpdateAddHTLC that exists within the mailbox, removing it
// from the in-memory replay buffer. This will prevent the packet from being
// delivered after the link restarts if the switch has remained online. The
Expand Down
Loading