From e0b133297d83a3ab0e710a1488897c791d60caa2 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 13 Nov 2020 11:46:12 +0100 Subject: [PATCH 1/2] lnwallet/channel: correct dust calculation on outgoing HTLC In this commit we fix a bug resulting in the wrong commit weight being calculated when an HTLC just below the remote's duslimit was added. This was a result of using the timeoutFee instead of the successFee when checking whether it was dust, making us consider it non-dust when it should have been. --- lnwallet/channel.go | 2 +- lnwallet/channel_test.go | 107 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index fdfda736..7e767d68 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4021,7 +4021,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, var totalHtlcWeight int64 for _, htlc := range filteredHTLCView.ourUpdates { if htlcIsDust( - lc.channelState.ChanType, remoteChain, !remoteChain, + lc.channelState.ChanType, false, !remoteChain, feePerKw, htlc.Amount.ToSatoshis(), dustLimit, ) { continue diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 190e31c1..18d1c924 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5331,6 +5331,113 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { checkBalance(t, expAliceBalance, expBobBalance) } +// TestChanCommitWeightDustHtlcs checks that we correctly calculate the +// commitment weight when some HTLCs are dust. +func TestChanCommitWeightDustHtlcs(t *testing.T) { + t.Parallel() + + // Create a test channel which will be used for the duration of this + // unittest. The channel will be funded evenly with Alice having 5 BTC, + // and Bob having 5 BTC. + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit, + ) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + bobDustlimit := lnwire.NewMSatFromSatoshis( + bobChannel.channelState.LocalChanCfg.DustLimit, + ) + + feeRate := chainfee.SatPerKWeight( + aliceChannel.channelState.LocalCommitment.FeePerKw, + ) + htlcSuccessFee := lnwire.NewMSatFromSatoshis( + HtlcSuccessFee(aliceChannel.channelState.ChanType, feeRate), + ) + + // Helper method to add an HTLC from Alice to Bob. + htlcIndex := uint64(0) + addHtlc := func(htlcAmt lnwire.MilliSatoshi) lntypes.Preimage { + t.Helper() + + htlc, preImage := createHTLC(int(htlcIndex), htlcAmt) + if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil { + t.Fatalf("unable to add htlc: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlc); err != nil { + t.Fatalf("unable to recv htlc: %v", err) + } + + if err := ForceStateTransition(aliceChannel, bobChannel); err != nil { + t.Fatalf("unable to complete alice's state "+ + "transition: %v", err) + } + + return preImage + } + + settleHtlc := func(preImage lntypes.Preimage) { + t.Helper() + + err = bobChannel.SettleHTLC(preImage, htlcIndex, nil, nil, nil) + if err != nil { + t.Fatalf("unable to settle htlc: %v", err) + } + err = aliceChannel.ReceiveHTLCSettle(preImage, htlcIndex) + if err != nil { + t.Fatalf("unable to settle htlc: %v", err) + } + + if err := ForceStateTransition(aliceChannel, bobChannel); err != nil { + t.Fatalf("unable to complete alice's state "+ + "transition: %v", err) + } + htlcIndex++ + } + + // Helper method that fetches the current remote commitment weight + // fromt the given channel's POV. + remoteCommitWeight := func(lc *LightningChannel) int64 { + remoteACKedIndex := lc.localCommitChain.tip().theirMessageIndex + htlcView := lc.fetchHTLCView(remoteACKedIndex, + lc.localUpdateLog.logIndex) + + _, w := lc.availableCommitmentBalance( + htlcView, true, + ) + + return w + } + + // Start by getting the initial remote commitment wight seen from + // Alice's perspective. At this point there are no HTLCs on the + // commitment. + weight1 := remoteCommitWeight(aliceChannel) + + // Now add an HTLC that will be just below Bob's dustlimit. + // Since this is an HTLC added from Alice on Bob's commitment, we will + // use the HTLC success fee. + bobDustHtlc := bobDustlimit + htlcSuccessFee - 1 + preimg := addHtlc(bobDustHtlc) + + // Now get the current wight of the remote commitment. We expect it to + // not have changed, since the HTLC we added is considered dust. + weight2 := remoteCommitWeight(aliceChannel) + require.Equal(t, weight1, weight2) + + // In addition, we expect this weight to result in the fee we currently + // see being paid on the remote commitent. + calcFee := feeRate.FeeForWeight(weight2) + remoteCommitFee := aliceChannel.channelState.RemoteCommitment.CommitFee + require.Equal(t, calcFee, remoteCommitFee) + + // Settle the HTLC, bringing commitment weight back to base. + settleHtlc(preimg) +} + // TestSignCommitmentFailNotLockedIn tests that a channel will not attempt to // create a new state if it doesn't yet know of the next revocation point for // the remote party. From 953443e10c7032ae8da930700923433a38b9af62 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 13 Nov 2020 11:48:45 +0100 Subject: [PATCH 2/2] lnwallet/channel: correct dust calculation on incoming HTLC Similar to the previous commit, we fix a bug resulting in the wrong commit weight being calculated when an HTLC just above the remote's duslimit was added from the remote. This was a result of using the successFee instead of the timeoutFee when checking whether it was dust, making us consider it dust when it shouldn't have been. --- lnwallet/channel.go | 2 +- lnwallet/channel_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 7e767d68..090f8f00 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4031,7 +4031,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, } for _, htlc := range filteredHTLCView.theirUpdates { if htlcIsDust( - lc.channelState.ChanType, !remoteChain, !remoteChain, + lc.channelState.ChanType, true, !remoteChain, feePerKw, htlc.Amount.ToSatoshis(), dustLimit, ) { continue diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 18d1c924..b69b2faf 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5347,6 +5347,9 @@ func TestChanCommitWeightDustHtlcs(t *testing.T) { } defer cleanUp() + aliceDustlimit := lnwire.NewMSatFromSatoshis( + aliceChannel.channelState.LocalChanCfg.DustLimit, + ) bobDustlimit := lnwire.NewMSatFromSatoshis( bobChannel.channelState.LocalChanCfg.DustLimit, ) @@ -5354,6 +5357,9 @@ func TestChanCommitWeightDustHtlcs(t *testing.T) { feeRate := chainfee.SatPerKWeight( aliceChannel.channelState.LocalCommitment.FeePerKw, ) + htlcTimeoutFee := lnwire.NewMSatFromSatoshis( + HtlcTimeoutFee(aliceChannel.channelState.ChanType, feeRate), + ) htlcSuccessFee := lnwire.NewMSatFromSatoshis( HtlcSuccessFee(aliceChannel.channelState.ChanType, feeRate), ) @@ -5436,6 +5442,27 @@ func TestChanCommitWeightDustHtlcs(t *testing.T) { // Settle the HTLC, bringing commitment weight back to base. settleHtlc(preimg) + + // Now we do a similar check from Bob's POV. Start with getting his + // current view of Alice's commitment weight. + weight1 = remoteCommitWeight(bobChannel) + + // We'll add an HTLC from Alice to Bob, that is just above dust on + // Alice's commitment. Now we'll use the timeout fee. + aliceDustHtlc := aliceDustlimit + htlcTimeoutFee + preimg = addHtlc(aliceDustHtlc) + + // Get the current remote commitment weight from Bob's POV, and ensure + // it is now heavier, since Alice added a non-dust HTLC. + weight2 = remoteCommitWeight(bobChannel) + require.Greater(t, weight2, weight1) + + // Ensure the current remote commit has the expected commitfee. + calcFee = feeRate.FeeForWeight(weight2) + remoteCommitFee = bobChannel.channelState.RemoteCommitment.CommitFee + require.Equal(t, calcFee, remoteCommitFee) + + settleHtlc(preimg) } // TestSignCommitmentFailNotLockedIn tests that a channel will not attempt to