lnwallet+htlcswitch: make Switch dust-aware #5770

Crypt-iQ · 2021-09-20T21:58:21Z

Commit overview:

lnwallet: introduce GetDustSum method to calculate worst-case dust sum adds a method on LightningChannel to calculate worst-case dust on a commitment. It uses an over-estimation currently, but can be changed if somebody can figure out an exact method to count dust w/o under-counting.
htlcswitch: extend Mailbox iface with dust, fee methods allows the mailbox to be dust-aware. Dust is tracked from the time a packet is added (AddPacket) to the time it's removed (AckPacket).
htlcswitch: extend ChannelLink iface with dustHandler iface allows the link to call Mailbox.SetDustLimits and bubble up results from LightningChannel.GetDustSum.
htlcswitch: call evaluateDustThreshold in SendHTLC, handlePacketForward modifies the switch to be dust-aware so that SendHTLC or handlePacketForward may fail if dust sum exceeds the new default dust threshold of 500000 satoshis.
multi: introduce config-level DustThreshold for defining threshold allows lnd to be started with a command-line option --dust-threshold= that lets a user define their acceptable dust threshold.

Questions:

Should the dust threshold be configurable per-channel? A default was easier to implement, but user-configurable may be better. It would, however, make this diff bigger.
The htlcswitch/switch_test.go test in this diff is quite slow due to a looping call to SendHTLC which itself is mostly synchronous. Any way to speed it up? Is this test complete?
There is over-counting in the GetDustSum method because some HTLC's in the update logs may be removed after a call to compactLogs. Is there a way to fix this? There is a double-counting of mailbox dust since packets that are in the mailbox and not yet signed for will also be in the channel's update logs. This decreases the dust through-put of channels.
Pathfinding is not aware of dust errors from SendHTLC, should that be modified as well?

Related:

orijbot · 2021-09-20T21:58:24Z

Visit https://dashboard.github.orijtech.com?back=0&pr=5770&remote=true&repo=Crypt-iQ%2Flnd to see benchmark details.

Roasbeef

Happy the drafted changes worked out so well! Great commits generally, completed an intial pass, nothing major blocking so far. Will continue to test this out and also do another pass afterwards

lnwallet/channel.go

htlcswitch/mailbox.go

htlcswitch/mailbox_test.go

htlcswitch/switch.go

Roasbeef · 2021-09-22T22:31:34Z

htlcswitch/switch.go

+	// Attempt to fetch the target link before creating a circuit so that
+	// we don't leave dangling circuits. The getLocalLink method does not
+	// require the circuit variable to be set on the *htlcPacket.
+	link, linkErr := s.getLocalLink(packet, htlc)


Seems this actually also fixes a bug (that would occur rarely since the bandwidth hints would report 0 if the link wasn't online?), where we would possibly never clean up a circuit?

I didn't look at the routing-level call-graph -- my reasoning was that SendHTLC would be called and the link has died before/during with RemoveLink being called and so the circuit would still exist. On start-up, we'd only clean non-hop.Source circuits. I didn't want to make this problem worse by creating a circuit, and then potentially failing if the outbound link was dust-exposed.

Roasbeef

LGTM 💎

Same comment from the other one re the new release notes file

htlcswitch/link.go

carlaKC

first pass, just a few questions from me

htlcswitch/link.go

carlaKC · 2021-09-29T08:23:46Z

htlcswitch/link.go

@@ -2527,6 +2568,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)


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

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.

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.

comment added

htlcswitch/switch.go

config.go

carlaKC · 2021-09-30T09:58:24Z

lnwallet/channel_test.go

+	// Settling the HTLC back from Alice to Bob should not change the dust
+	// sum.


nit: add "because alice has not yet received the revocation commitment" to comment? had to think about it for a second.

"because the HTLC is counted until it's removed from the update logs via compactLogs."?

carlaKC

great work on the tests for this, very thorough!

I think release notes for this need to be 0.14 (could be wrong), then we're gg.

lnwallet/channel_test.go

htlcswitch/link.go

carlaKC · 2021-09-30T10:58:00Z

htlcswitch/link.go

@@ -2527,6 +2568,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)


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.

htlcswitch/switch.go

htlcswitch/switch_test.go

carlaKC · 2021-09-30T11:25:36Z

lntest/itest/lnd_test.go

@@ -203,6 +203,7 @@ func TestLightningNetworkDaemon(t *testing.T) {
 	// TODO(roasbeef): create master balanced channel with all the monies?
 	aliceBobArgs := []string{
 		"--default-remote-max-htlcs=483",
+		"--dust-threshold=5000000",


Reminder to drop before merge

dropped commit but kept the change since o/w a flake occurs in async_bidi_payments

docs/release-notes/release-notes-0.13.3.md

It over-estimates the local or remote commitment's dust sum by counting all updates in both updateLogs that are dust using the trimmed-to-dust mechanism if applicable. The over-estimation is done because ensuring an accurate counting is a trade-off between code simplicity and accuracy.

This commit extends the Mailbox interface with the SetDustClosure, SetFeeRate, and DustPackets methods. This enables the mailbox to report the dust exposure to the Switch when the Switch decides whether to forward a dust packet. The dust is counted from the time an Add is introduced via AddPacket until it is removed via AckPacket. This can lead to some packets being counted twice before they are signed for, but this is a trade-off between accuracy and simplicity.

This allows the Switch to determine the dust exposure of a certain channel and allows the link to set the feerate of the mailbox given a fee update.

This commit makes SendHTLC (we are the source) evaluate the dust threshold of the outgoing channel against the default threshold of 500K satoshis. If the threshold is exceeded by adding this HTLC, we fail backwards. It also makes handlePacketForward (we are forwarding) evaluate the dust threshold of the incoming channel and the outgoing channel and fails backwards if either channel's dust sum exceeds the default threshold.

Roasbeef · 2021-10-01T03:19:57Z

I think release notes for this need to be 0.14 (could be wrong), then we're gg.

This is gonna be packaged up in a 0.13.3 release, which will drop before 0.14

Crypt-iQ added safety General label for issues/PRs related to the safety of using the software htlcswitch spec labels Sep 20, 2021

Crypt-iQ added this to the v0.14.0 milestone Sep 20, 2021

Crypt-iQ requested a review from Roasbeef September 20, 2021 21:58

Crypt-iQ force-pushed the dust_threshold_0619 branch from 0aaeb59 to a4eb9d7 Compare September 21, 2021 14:45

Roasbeef requested changes Sep 22, 2021

View reviewed changes

Crypt-iQ force-pushed the dust_threshold_0619 branch 2 times, most recently from 2ee4dc6 to 05a14a2 Compare September 28, 2021 15:51

Roasbeef requested a review from carlaKC September 28, 2021 22:15

Roasbeef approved these changes Sep 28, 2021

View reviewed changes

htlcswitch/link.go Show resolved Hide resolved

carlaKC reviewed Sep 29, 2021

View reviewed changes

carlaKC self-requested a review September 29, 2021 08:57

Crypt-iQ force-pushed the dust_threshold_0619 branch from 2d551c4 to e32ece6 Compare September 29, 2021 17:50

Crypt-iQ requested a review from Roasbeef September 29, 2021 18:55

carlaKC reviewed Sep 30, 2021

View reviewed changes

Crypt-iQ added 6 commits September 30, 2021 13:44

htlcswitch: extend ChannelLink iface with dustHandler iface

0ce6194

This allows the Switch to determine the dust exposure of a certain channel and allows the link to set the feerate of the mailbox given a fee update.

multi: introduce config-level DustThreshold for defining threshold

702b3a3

docs: update release notes for 0.13.3

25a0fe2

Crypt-iQ force-pushed the dust_threshold_0619 branch from e32ece6 to 25a0fe2 Compare September 30, 2021 17:44

Roasbeef merged commit 32fa48d into lightningnetwork:master Oct 1, 2021

Crypt-iQ deleted the dust_threshold_0619 branch October 4, 2021 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnwallet+htlcswitch: make Switch dust-aware #5770

lnwallet+htlcswitch: make Switch dust-aware #5770

Crypt-iQ commented Sep 20, 2021 •

edited

Loading

orijbot commented Sep 20, 2021

Roasbeef left a comment

Roasbeef Sep 22, 2021

Crypt-iQ Sep 23, 2021

Roasbeef left a comment

carlaKC left a comment

carlaKC Sep 29, 2021

Crypt-iQ Sep 29, 2021

carlaKC Sep 30, 2021

Crypt-iQ Sep 30, 2021

carlaKC Sep 30, 2021

Crypt-iQ Sep 30, 2021

carlaKC left a comment

carlaKC Sep 30, 2021

carlaKC Sep 30, 2021

Crypt-iQ Sep 30, 2021

Roasbeef commented Oct 1, 2021

		// Settling the HTLC back from Alice to Bob should not change the dust
		// sum.

lnwallet+htlcswitch: make Switch dust-aware #5770

lnwallet+htlcswitch: make Switch dust-aware #5770

Conversation

Crypt-iQ commented Sep 20, 2021 • edited Loading

orijbot commented Sep 20, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Oct 1, 2021

Crypt-iQ commented Sep 20, 2021 •

edited

Loading