From 05fb272deb3bddffab38708a792d5a18171f6289 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:41 +0100 Subject: [PATCH 1/9] lnwallet/channel: make validateCommitmentSanity take our/their predict add To ba able to validate the commitment sanity both for remote and local commitments, and at the same time predict both our and their add, we let validateCommitmentSanity take an extra payment descriptor to make this possible. --- lnwallet/channel.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index fb774c1a5..9519e7fe1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3103,12 +3103,13 @@ func (lc *LightningChannel) getUnsignedAckedUpdates() []channeldb.LogUpdate { // validateCommitmentSanity is used to validate the current state of the // commitment transaction in terms of the ChannelConstraints that we and our -// remote peer agreed upon during the funding workflow. The predictAdded -// parameter should be set to a valid PaymentDescriptor if we are validating -// in the state when adding a new HTLC, or nil otherwise. +// remote peer agreed upon during the funding workflow. The +// predict[Our|Their]Add should parameters should be set to a valid +// PaymentDescriptor if we are validating in the state when adding a new HTLC, +// or nil otherwise. func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, ourLogCounter uint64, remoteChain bool, - predictAdded *PaymentDescriptor) error { + predictOurAdd, predictTheirAdd *PaymentDescriptor) error { // Fetch all updates not committed. view := lc.fetchHTLCView(theirLogCounter, ourLogCounter) @@ -3116,14 +3117,11 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, // If we are checking if we can add a new HTLC, we add this to the // appropriate update log, in order to validate the sanity of the // commitment resulting from _actually adding_ this HTLC to the state. - if predictAdded != nil { - // If the remoteChain bool is true, add to ourUpdates. - if remoteChain { - view.ourUpdates = append(view.ourUpdates, predictAdded) - } else { - // Else add to theirUpdates. - view.theirUpdates = append(view.theirUpdates, predictAdded) - } + if predictOurAdd != nil { + view.ourUpdates = append(view.ourUpdates, predictOurAdd) + } + if predictTheirAdd != nil { + view.theirUpdates = append(view.theirUpdates, predictTheirAdd) } commitChain := lc.localCommitChain @@ -3296,7 +3294,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, []ch // party set up when we initially set up the channel. If we are, then // we'll abort this state transition. err := lc.validateCommitmentSanity( - remoteACKedIndex, lc.localUpdateLog.logIndex, true, nil, + remoteACKedIndex, lc.localUpdateLog.logIndex, true, nil, nil, ) if err != nil { return sig, htlcSigs, nil, err @@ -4050,7 +4048,7 @@ func (lc *LightningChannel) ReceiveNewCommitment(commitSig lnwire.Sig, // the constraints we specified during initial channel setup. If not, // then we'll abort the channel as they've violated our constraints. err := lc.validateCommitmentSanity( - lc.remoteUpdateLog.logIndex, localACKedIndex, false, nil, + lc.remoteUpdateLog.logIndex, localACKedIndex, false, nil, nil, ) if err != nil { return err @@ -4647,7 +4645,7 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC, // must keep on our commitment transaction. remoteACKedIndex := lc.localCommitChain.tail().theirMessageIndex err := lc.validateCommitmentSanity( - remoteACKedIndex, lc.localUpdateLog.logIndex, true, pd, + remoteACKedIndex, lc.localUpdateLog.logIndex, true, pd, nil, ) if err != nil { return 0, err @@ -4685,7 +4683,7 @@ func (lc *LightningChannel) ReceiveHTLC(htlc *lnwire.UpdateAddHTLC) (uint64, err // Clamp down on the number of HTLC's we can receive by checking the // commitment sanity. err := lc.validateCommitmentSanity( - lc.remoteUpdateLog.logIndex, localACKedIndex, false, pd, + lc.remoteUpdateLog.logIndex, localACKedIndex, false, nil, pd, ) if err != nil { return 0, err From 4ea822efeb18fda284304bf20ceb015296c719c8 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:41 +0100 Subject: [PATCH 2/9] lnwallet tests: add test for dipping remote below chan reserve This commit adds a test that was previously not performed, namely that adding a HTLC would dip the remote initiator below its channel reserve. --- lnwallet/channel_test.go | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 7cd5aa9f1..3f97f9f9d 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -6007,6 +6007,60 @@ func TestChanReserve(t *testing.T) { ) } +// TestChanReserveRemoteInitiator tests that the channel reserve of the +// initiator is accounted for when adding HTLCs, whether the initiator is the +// local or remote node. +func TestChanReserveRemoteInitiator(t *testing.T) { + t.Parallel() + + // We start out with a channel where both parties have 5 BTC. + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + true, + ) + if err != nil { + t.Fatal(err) + } + defer cleanUp() + + // Set Alice's channel reserve to be 5 BTC-commitfee. This means she + // has just enough balance to cover the comitment fee, but not enough + // to add any more HTLCs to the commitment. Although a reserve this + // high is unrealistic, a channel can easiliy get into a situation + // where the initiator cannot pay for the fee of any more HTLCs. + commitFee := aliceChannel.channelState.LocalCommitment.CommitFee + aliceMinReserve := 5*btcutil.SatoshiPerBitcoin - commitFee + + aliceChannel.channelState.LocalChanCfg.ChanReserve = aliceMinReserve + bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve + + // Now let Bob attempt to add an HTLC of 0.1 BTC. He has plenty of + // money available to spend, but Alice, which is the initiator, cannot + // afford any more HTLCs on the commitment transaction because that + // would take here below her channel reserve.. + htlcAmt := lnwire.NewMSatFromSatoshis(0.1 * btcutil.SatoshiPerBitcoin) + htlc, _ := createHTLC(0, htlcAmt) + + // Bob should refuse to add this HTLC, since he realizes it will create + // an invalid commitment. + _, err = bobChannel.AddHTLC(htlc, nil) + if err != ErrBelowChanReserve { + t.Fatalf("expected ErrBelowChanReserve, instead received: %v", + err) + } + + // Of course Alice will also not have enough balance to add it herself. + _, err = aliceChannel.AddHTLC(htlc, nil) + if err != ErrBelowChanReserve { + t.Fatalf("expected ErrBelowChanReserve, instead received: %v", + err) + } + + // Same for Alice, she should refuse to accept this second HTLC. + if _, err := aliceChannel.ReceiveHTLC(htlc); err != ErrBelowChanReserve { + t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err) + } +} + // TestMinHTLC tests that the ErrBelowMinHTLC error is thrown if an HTLC is added // that is below the minimm allowed value for HTLCs. func TestMinHTLC(t *testing.T) { From 0d9a1b865621c918820a885007b21da8e4ddcd1b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:41 +0100 Subject: [PATCH 3/9] lnwallet: check local commitment sanity when adding HTLC This commit adds an extra validation step when adding HTLCs. Previously we would only validate the remote commitment resulting from adding an HTLC, which in most cases is enough. However, there are situations where the dustlimits are different, which could lead to the resulting remote commitment from adding the HTLC being valid, but not the local commitment. Now we also validate the local commitment. A test to trigger the case is added. --- lnwallet/channel.go | 18 +++++++++++++++- lnwallet/channel_test.go | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 9519e7fe1..8b2593656 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4642,8 +4642,11 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC, } // Make sure adding this HTLC won't violate any of the constraints we - // must keep on our commitment transaction. + // must keep on the commitment transactions. remoteACKedIndex := lc.localCommitChain.tail().theirMessageIndex + + // First we'll check whether this HTLC can be added to the remote + // commitment transaction without violation any of the constraints. err := lc.validateCommitmentSanity( remoteACKedIndex, lc.localUpdateLog.logIndex, true, pd, nil, ) @@ -4651,6 +4654,19 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC, return 0, err } + // We must also check whether it can be added to our own commitment + // transaction, or the remote node will refuse to sign. This is not + // totally bullet proof, as the remote might be adding updates + // concurrently, but if we fail this check there is for sure not + // possible for us to add the HTLC. + err = lc.validateCommitmentSanity( + lc.remoteUpdateLog.logIndex, lc.localUpdateLog.logIndex, + false, pd, nil, + ) + if err != nil { + return 0, err + } + lc.localUpdateLog.appendHtlc(pd) return pd.HtlcIndex, nil diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 3f97f9f9d..e9455b754 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -6061,6 +6061,51 @@ func TestChanReserveRemoteInitiator(t *testing.T) { } } +// TestChanReserveLocalInitiatorDustHtlc tests that fee the initiator must pay +// when adding HTLCs is accounted for, even though the HTLC is considered dust +// by the remote bode. +func TestChanReserveLocalInitiatorDustHtlc(t *testing.T) { + t.Parallel() + + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + true, + ) + if err != nil { + t.Fatal(err) + } + defer cleanUp() + + // The amount of the HTLC should not be considered dust according to + // Alice's dust limit (200 sat), but be dust according to Bob's dust + // limit (1300 sat). It is considered dust if the amount remaining + // after paying the HTLC fee is below the dustlimit, so we choose a + // size of 500+htlcFee. + htlcSat := btcutil.Amount(500) + htlcTimeoutFee( + chainfee.SatPerKWeight( + aliceChannel.channelState.LocalCommitment.FeePerKw, + ), + ) + + // Set Alice's channel reserve to be low enough to carry the value of + // the HTLC, but not low enough to allow the extra fee from adding the + // HTLC to the commitment. + commitFee := aliceChannel.channelState.LocalCommitment.CommitFee + aliceMinReserve := 5*btcutil.SatoshiPerBitcoin - commitFee - htlcSat + + aliceChannel.channelState.LocalChanCfg.ChanReserve = aliceMinReserve + bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve + + htlcDustAmt := lnwire.NewMSatFromSatoshis(htlcSat) + htlc, _ := createHTLC(0, htlcDustAmt) + + // Alice should realize that the fee she must pay to add this HTLC to + // the local commitment would take her below the channel reserve. + _, err = aliceChannel.AddHTLC(htlc, nil) + if err != ErrBelowChanReserve { + t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err) + } +} + // TestMinHTLC tests that the ErrBelowMinHTLC error is thrown if an HTLC is added // that is below the minimm allowed value for HTLCs. func TestMinHTLC(t *testing.T) { From 58dec106800a90def81f9dbd52c08808a58c83f9 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:41 +0100 Subject: [PATCH 4/9] lnwallet/channel: break up availableBalance --- lnwallet/channel.go | 58 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 8b2593656..074c8bf2c 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5997,11 +5997,14 @@ func (lc *LightningChannel) CompleteCooperativeClose(localSig, remoteSig []byte, return closeTx, ourBalance, nil } -// AvailableBalance returns the current available balance within the channel. -// By available balance, we mean that if at this very instance s new commitment -// were to be created which evals all the log entries, what would our available -// balance me. This method is useful when deciding if a given channel can -// accept an HTLC in the multi-hop forwarding scenario. +// AvailableBalance returns the current balance available for sending within +// the channel. By available balance, we mean that if at this very instance a +// new commitment were to be created which evals all the log entries, what +// would our available balance for adding an additional HTLC be. It takes into +// account the fee that must be paid for adding this HTLC (if we're the +// initiator), and that we cannot spend from the channel reserve. This method +// is useful when deciding if a given channel can accept an HTLC in the +// multi-hop forwarding scenario. func (lc *LightningChannel) AvailableBalance() lnwire.MilliSatoshi { lc.RLock() defer lc.RUnlock() @@ -6021,19 +6024,50 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) { htlcView := lc.fetchHTLCView(remoteACKedIndex, lc.localUpdateLog.logIndex) - // Then compute our current balance for that view. - ourBalance, _, commitWeight, filteredView, err := - lc.computeView(htlcView, false, false) + // Calculate our available balance from our local commitment. + // + // NOTE: This is not always accurate, since the remote node can always + // add updates concurrently, causing our balance to go down if we're + // the initiator, but this is a problem on the protocol level. + ourLocalCommitBalance, commitWeight := lc.availableCommitmentBalance( + htlcView, + ) + + return ourLocalCommitBalance, commitWeight +} + +// availableCommitmentBalance attempts to calculate the balance we have +// available for HTLCs on the local/remote commitment given the htlcView. To +// account for sending HTLCs of different sizes, it will report the balance +// available for sending non-dust HTLCs, which will be manifested on the +// commitment, increasing the commitment fee we must pay as an initiator, +// eating into our balance. It will make sure we won't violate the channel +// reserve constraints for this amount. +func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( + lnwire.MilliSatoshi, int64) { + + // Compute the current balances for this commitment. This will take + // into account HTLCs to determine the commit weight, which the + // initiator must pay the fee for. + ourBalance, _, commitWeight, filteredView, err := lc.computeView( + view, false, false, + ) if err != nil { lc.log.Errorf("Unable to fetch available balance: %v", err) return 0, 0 } - // If we are the channel initiator, we must remember to subtract the - // commitment fee from our available balance. - commitFee := filteredView.feePerKw.FeeForWeight(commitWeight) + // Given the commitment weight, find the commitment fee in case of no + // added HTLC output. + feePerKw := filteredView.feePerKw + baseCommitFee := lnwire.NewMSatFromSatoshis( + feePerKw.FeeForWeight(commitWeight), + ) + + // If we are the channel initiator, we must to subtract the commitment + // fee from our available balance. if lc.channelState.IsInitiator { - ourBalance -= lnwire.NewMSatFromSatoshis(commitFee) + ourBalance -= baseCommitFee } return ourBalance, commitWeight From 5e89d5b6c2f402664e36e03dabe3822f56785bf7 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:42 +0100 Subject: [PATCH 5/9] link+lnwallet: move bandwidth channel reserve validation into channel Since we want to handle the edge case where paying the HTLC fee would take the initiator below the reserve, we move the subtraction of the reserve into availableBalance where this calculation will be performed. --- htlcswitch/link.go | 17 ++++++----------- lnwallet/channel.go | 19 +++++++++++++++++-- lnwallet/channel_test.go | 13 ++++++++++--- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 80b90f483..e127cfa13 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2127,26 +2127,21 @@ func (l *channelLink) ChanID() lnwire.ChannelID { // // NOTE: Part of the ChannelLink interface. func (l *channelLink) Bandwidth() lnwire.MilliSatoshi { + // Get the balance available on the channel for new HTLCs. This takes + // the channel reserve into account so HTLCs up to this value won't + // violate it. channelBandwidth := l.channel.AvailableBalance() - overflowBandwidth := l.overflowQueue.TotalHtlcAmount() // To compute the total bandwidth, we'll take the current available // bandwidth, then subtract the overflow bandwidth as we'll eventually // also need to evaluate those HTLC's once space on the commitment // transaction is free. - linkBandwidth := channelBandwidth - overflowBandwidth - - // If the channel reserve is greater than the total available balance - // of the link, just return 0. - reserve := lnwire.NewMSatFromSatoshis(l.channel.LocalChanReserve()) - if linkBandwidth < reserve { + overflowBandwidth := l.overflowQueue.TotalHtlcAmount() + if channelBandwidth < overflowBandwidth { return 0 } - // Else the amount that is available to flow through the link at this - // point is the available balance minus the reserve amount we are - // required to keep as collateral. - return linkBandwidth - reserve + return channelBandwidth - overflowBandwidth } // AttachMailBox updates the current mailbox used by this link, and hooks up diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 074c8bf2c..f8512d72a 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6057,6 +6057,17 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( return 0, 0 } + // We can never spend from the channel reserve, so we'll subtract it + // from our available balance. + ourReserve := lnwire.NewMSatFromSatoshis( + lc.channelState.LocalChanCfg.ChanReserve, + ) + if ourReserve <= ourBalance { + ourBalance -= ourReserve + } else { + ourBalance = 0 + } + // Given the commitment weight, find the commitment fee in case of no // added HTLC output. feePerKw := filteredView.feePerKw @@ -6067,6 +6078,9 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( // If we are the channel initiator, we must to subtract the commitment // fee from our available balance. if lc.channelState.IsInitiator { + if ourBalance < baseCommitFee { + return 0, commitWeight + } ourBalance -= baseCommitFee } @@ -6278,13 +6292,14 @@ func (lc *LightningChannel) MaxFeeRate(maxAllocation float64) chainfee.SatPerKWe // The maximum fee depends of the available balance that can be // committed towards fees. - balance, weight := lc.availableBalance() + commit := lc.channelState.LocalCommitment feeBalance := float64( - balance.ToSatoshis() + lc.channelState.LocalCommitment.CommitFee, + commit.LocalBalance.ToSatoshis() + commit.CommitFee, ) maxFee := feeBalance * maxAllocation // Ensure the fee rate doesn't dip below the fee floor. + _, weight := lc.availableBalance() maxFeeRate := maxFee / (float64(weight) / 1000) return chainfee.SatPerKWeight( math.Max(maxFeeRate, float64(chainfee.FeePerKwFloor)), diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index e9455b754..77af8d3fa 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -4599,6 +4599,10 @@ func TestChanAvailableBandwidth(t *testing.T) { } defer cleanUp() + aliceReserve := lnwire.NewMSatFromSatoshis( + aliceChannel.channelState.LocalChanCfg.ChanReserve, + ) + assertBandwidthEstimateCorrect := func(aliceInitiate bool) { // With the HTLC's added, we'll now query the AvailableBalance // method for the current available channel bandwidth from @@ -4625,11 +4629,14 @@ func TestChanAvailableBandwidth(t *testing.T) { // Now, we'll obtain the current available bandwidth in Alice's // latest commitment and compare that to the prior estimate. aliceBalance := aliceChannel.channelState.LocalCommitment.LocalBalance - if aliceBalance != aliceAvailableBalance { + + // The balance we have available for new HTLCs should be the + // current local commitment balance, minus the channel reserve. + expBalance := aliceBalance - aliceReserve + if expBalance != aliceAvailableBalance { _, _, line, _ := runtime.Caller(1) t.Fatalf("line: %v, incorrect balance: expected %v, "+ - "got %v", line, aliceBalance, - aliceAvailableBalance) + "got %v", line, expBalance, aliceAvailableBalance) } } From 9ff79ae59517c34444d66c4a13e802b0cdac52d5 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:42 +0100 Subject: [PATCH 6/9] lnwallet/channel: account for HTLC fee when reporting available balance --- htlcswitch/link_test.go | 23 ++++--- lnwallet/channel.go | 26 +++++--- lnwallet/channel_test.go | 135 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 166 insertions(+), 18 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 427322ee9..ca31d68d7 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -1982,8 +1982,11 @@ func TestChannelLinkBandwidthConsistency(t *testing.T) { ) // The starting bandwidth of the channel should be exactly the amount - // that we created the channel between her and Bob. - expectedBandwidth := lnwire.NewMSatFromSatoshis(chanAmt - defaultCommitFee) + // that we created the channel between her and Bob, minus the + // commitment fee and fee for adding an additional HTLC. + expectedBandwidth := lnwire.NewMSatFromSatoshis( + chanAmt-defaultCommitFee, + ) - htlcFee assertLinkBandwidth(t, aliceLink, expectedBandwidth) // Next, we'll create an HTLC worth 1 BTC, and send it into the link as @@ -2656,8 +2659,10 @@ func TestChannelLinkTrimCircuitsPending(t *testing.T) { // The starting bandwidth of the channel should be exactly the amount // that we created the channel between her and Bob, minus the commitment - // fee. - expectedBandwidth := lnwire.NewMSatFromSatoshis(chanAmt - defaultCommitFee) + // fee and fee of adding an HTLC. + expectedBandwidth := lnwire.NewMSatFromSatoshis( + chanAmt-defaultCommitFee, + ) - htlcFee assertLinkBandwidth(t, alice.link, expectedBandwidth) // Capture Alice's starting bandwidth to perform later, relative @@ -2935,8 +2940,10 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { // The starting bandwidth of the channel should be exactly the amount // that we created the channel between her and Bob, minus the commitment - // fee. - expectedBandwidth := lnwire.NewMSatFromSatoshis(chanAmt - defaultCommitFee) + // fee and fee for adding an additional HTLC. + expectedBandwidth := lnwire.NewMSatFromSatoshis( + chanAmt-defaultCommitFee, + ) - htlcFee assertLinkBandwidth(t, alice.link, expectedBandwidth) // Capture Alice's starting bandwidth to perform later, relative @@ -3191,9 +3198,9 @@ func TestChannelLinkBandwidthChanReserve(t *testing.T) { // The starting bandwidth of the channel should be exactly the amount // that we created the channel between her and Bob, minus the channel - // reserve. + // reserve, commitment fee and fee for adding an additional HTLC. expectedBandwidth := lnwire.NewMSatFromSatoshis( - chanAmt - defaultCommitFee - chanReserve) + chanAmt-defaultCommitFee-chanReserve) - htlcFee assertLinkBandwidth(t, aliceLink, expectedBandwidth) // Next, we'll create an HTLC worth 3 BTC, and send it into the link as diff --git a/lnwallet/channel.go b/lnwallet/channel.go index f8512d72a..f3368218e 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6068,20 +6068,30 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( ourBalance = 0 } - // Given the commitment weight, find the commitment fee in case of no - // added HTLC output. + // Calculate the commitment fee in the case where we would add another + // HTLC to the commitment, as only the balance remaining after this fee + // has been paid is actually available for sending. feePerKw := filteredView.feePerKw - baseCommitFee := lnwire.NewMSatFromSatoshis( - feePerKw.FeeForWeight(commitWeight), + htlcCommitFee := lnwire.NewMSatFromSatoshis( + feePerKw.FeeForWeight(commitWeight + input.HTLCWeight), ) - // If we are the channel initiator, we must to subtract the commitment - // fee from our available balance. + // If we are the channel initiator, we must to subtract this commitment + // fee from our available balance in order to ensure we can afford both + // the value of the HTLC and the additional commitment fee from adding + // the HTLC. if lc.channelState.IsInitiator { - if ourBalance < baseCommitFee { + // There is an edge case where our non-zero balance is lower + // than the htlcCommitFee, where we could still be sending dust + // HTLCs, but we return 0 in this case. This is to avoid + // lowering our balance even further, as this takes us into a + // bad state wehere neither we nor our channel counterparty can + // add HTLCs. + if ourBalance < htlcCommitFee { return 0, commitWeight } - ourBalance -= baseCommitFee + + ourBalance -= htlcCommitFee } return ourBalance, commitWeight diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 77af8d3fa..096976295 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -4602,6 +4602,12 @@ func TestChanAvailableBandwidth(t *testing.T) { aliceReserve := lnwire.NewMSatFromSatoshis( aliceChannel.channelState.LocalChanCfg.ChanReserve, ) + feeRate := chainfee.SatPerKWeight( + aliceChannel.channelState.LocalCommitment.FeePerKw, + ) + htlcFee := lnwire.NewMSatFromSatoshis( + feeRate.FeeForWeight(input.HTLCWeight), + ) assertBandwidthEstimateCorrect := func(aliceInitiate bool) { // With the HTLC's added, we'll now query the AvailableBalance @@ -4631,8 +4637,9 @@ func TestChanAvailableBandwidth(t *testing.T) { aliceBalance := aliceChannel.channelState.LocalCommitment.LocalBalance // The balance we have available for new HTLCs should be the - // current local commitment balance, minus the channel reserve. - expBalance := aliceBalance - aliceReserve + // current local commitment balance, minus the channel reserve + // and the fee for adding an HTLC. + expBalance := aliceBalance - aliceReserve - htlcFee if expBalance != aliceAvailableBalance { _, _, line, _ := runtime.Caller(1) t.Fatalf("line: %v, incorrect balance: expected %v, "+ @@ -4714,6 +4721,130 @@ func TestChanAvailableBandwidth(t *testing.T) { // TODO(roasbeef): additional tests from diff starting conditions } +// TestChanAvailableBalanceNearHtlcFee checks that we get the expected reported +// balance when it is close to the htlc fee. +func TestChanAvailableBalanceNearHtlcFee(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(true) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // Alice starts with half the channel capacity. + aliceBalance := lnwire.NewMSatFromSatoshis(5 * btcutil.SatoshiPerBitcoin) + + aliceReserve := lnwire.NewMSatFromSatoshis( + aliceChannel.channelState.LocalChanCfg.ChanReserve, + ) + + aliceDustlimit := lnwire.NewMSatFromSatoshis( + aliceChannel.channelState.LocalChanCfg.DustLimit, + ) + feeRate := chainfee.SatPerKWeight( + aliceChannel.channelState.LocalCommitment.FeePerKw, + ) + htlcFee := lnwire.NewMSatFromSatoshis( + feeRate.FeeForWeight(input.HTLCWeight), + ) + commitFee := lnwire.NewMSatFromSatoshis( + aliceChannel.channelState.LocalCommitment.CommitFee, + ) + htlcTimeoutFee := lnwire.NewMSatFromSatoshis( + htlcTimeoutFee(feeRate), + ) + + // Helper method to check the current reported balance. + checkBalance := func(t *testing.T, expBalanceAlice lnwire.MilliSatoshi) { + t.Helper() + balance := aliceChannel.AvailableBalance() + if balance != expBalanceAlice { + t.Fatalf("Expected balance %v, got %v", expBalanceAlice, + balance) + } + } + + // Helper method to send an HTLC from Alice to Bob, decreasing Alice's + // balance. + htlcIndex := uint64(0) + sendHtlc := func(htlcAmt lnwire.MilliSatoshi) { + 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) + } + + 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++ + aliceBalance -= htlcAmt + } + + // Balance should start out equal to half the channel capacity minus + // the commitment fee Alice must pay and the channel reserve. In + // addition the HTLC fee will be subtracted fromt the balance to + // reflect that this value must be reserved for any payment above the + // dust limit. + expAliceBalance := aliceBalance - commitFee - aliceReserve - htlcFee + checkBalance(t, expAliceBalance) + + // Find the minumim size of a non-dust HTLC. + aliceNonDustHtlc := aliceDustlimit + htlcTimeoutFee + + // Send a HTLC leaving the remaining balance just enough to have + // nonDustHtlc left after paying the commit fee and htlc fee. + htlcAmt := aliceBalance - (commitFee + aliceReserve + htlcFee + aliceNonDustHtlc) + sendHtlc(htlcAmt) + + // Now the real balance left will be + // nonDustHtlc+commitfee+aliceReserve+htlcfee. The available balance + // reported will just be nonDustHtlc, since the rest of the balance is + // reserved. + expAliceBalance = aliceNonDustHtlc + checkBalance(t, expAliceBalance) + + // Send an HTLC using all but one msat of the reported balance. + htlcAmt = aliceNonDustHtlc - 1 + sendHtlc(htlcAmt) + + // 1 msat should be left. + expAliceBalance = 1 + checkBalance(t, expAliceBalance) + + // Sendng the last msat. + htlcAmt = 1 + sendHtlc(htlcAmt) + + // No balance left. + expAliceBalance = 0 + checkBalance(t, expAliceBalance) +} + // 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 f94464d987468757763a90f66a7a3f23ecfc6a2b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:42 +0100 Subject: [PATCH 7/9] lnwallet: take remote initiator's balance into account When we send non-dust HTLCs as the non-initiator, the remote party will have to pay the extra commitment fee. To account for this we figure out if they can afford paying this fee, if not we report that we only have balance available for dust HTLCs, since these HTLCs won't increase the commitment fee. --- lnwallet/channel.go | 39 ++++++++++++++++++++-- lnwallet/channel_test.go | 72 ++++++++++++++++++++++++++++++++++------ 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index f3368218e..0cc512d5e 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6049,7 +6049,7 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( // Compute the current balances for this commitment. This will take // into account HTLCs to determine the commit weight, which the // initiator must pay the fee for. - ourBalance, _, commitWeight, filteredView, err := lc.computeView( + ourBalance, theirBalance, commitWeight, filteredView, err := lc.computeView( view, false, false, ) if err != nil { @@ -6091,7 +6091,42 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( return 0, commitWeight } - ourBalance -= htlcCommitFee + return ourBalance - htlcCommitFee, commitWeight + } + + // If we're not the initiator, we must check whether the remote has + // enough balance to pay for the fee of our HTLC. We'll start by also + // subtracting our counterparty's reserve from their balance. + theirReserve := lnwire.NewMSatFromSatoshis( + lc.channelState.RemoteChanCfg.ChanReserve, + ) + if theirReserve <= theirBalance { + theirBalance -= theirReserve + } else { + theirBalance = 0 + } + + // We'll use the dustlimit and htlcFee to find the largest HTLC value + // that will be considered dust on the commitment. + dustlimit := lnwire.NewMSatFromSatoshis( + lc.channelState.LocalChanCfg.DustLimit, + ) + + // For an extra HTLC fee to be paid on our commitment, the HTLC must be + // large enough to make a non-dust HTLC timeout transaction. + htlcFee := lnwire.NewMSatFromSatoshis( + htlcTimeoutFee(feePerKw), + ) + + // The HTLC output will be manifested on the commitment if it + // is non-dust after paying the HTLC fee. + nonDustHtlcAmt := dustlimit + htlcFee + + // If they cannot pay the fee if we add another non-dust HTLC, we'll + // report our available balance just below the non-dust amount, to + // avoid attempting HTLCs larger than this size. + if theirBalance < htlcCommitFee && ourBalance >= nonDustHtlcAmt { + ourBalance = nonDustHtlcAmt - 1 } return ourBalance, commitWeight diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 096976295..be7395201 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -4735,16 +4735,23 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { } defer cleanUp() - // Alice starts with half the channel capacity. + // Alice and Bob start with half the channel capacity. aliceBalance := lnwire.NewMSatFromSatoshis(5 * btcutil.SatoshiPerBitcoin) + bobBalance := lnwire.NewMSatFromSatoshis(5 * btcutil.SatoshiPerBitcoin) aliceReserve := lnwire.NewMSatFromSatoshis( aliceChannel.channelState.LocalChanCfg.ChanReserve, ) + bobReserve := lnwire.NewMSatFromSatoshis( + bobChannel.channelState.LocalChanCfg.ChanReserve, + ) aliceDustlimit := lnwire.NewMSatFromSatoshis( aliceChannel.channelState.LocalChanCfg.DustLimit, ) + bobDustlimit := lnwire.NewMSatFromSatoshis( + bobChannel.channelState.LocalChanCfg.DustLimit, + ) feeRate := chainfee.SatPerKWeight( aliceChannel.channelState.LocalCommitment.FeePerKw, ) @@ -4759,12 +4766,20 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { ) // Helper method to check the current reported balance. - checkBalance := func(t *testing.T, expBalanceAlice lnwire.MilliSatoshi) { + checkBalance := func(t *testing.T, expBalanceAlice, + expBalanceBob lnwire.MilliSatoshi) { + t.Helper() - balance := aliceChannel.AvailableBalance() - if balance != expBalanceAlice { - t.Fatalf("Expected balance %v, got %v", expBalanceAlice, - balance) + aliceBalance := aliceChannel.AvailableBalance() + if aliceBalance != expBalanceAlice { + t.Fatalf("Expected alice balance %v, got %v", + expBalanceAlice, aliceBalance) + } + + bobBalance := bobChannel.AvailableBalance() + if bobBalance != expBalanceBob { + t.Fatalf("Expected bob balance %v, got %v", + expBalanceBob, bobBalance) } } @@ -4803,6 +4818,7 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { htlcIndex++ aliceBalance -= htlcAmt + bobBalance += htlcAmt } // Balance should start out equal to half the channel capacity minus @@ -4811,12 +4827,17 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { // reflect that this value must be reserved for any payment above the // dust limit. expAliceBalance := aliceBalance - commitFee - aliceReserve - htlcFee - checkBalance(t, expAliceBalance) + + // Bob is not the initiator, so he will have all his balance available, + // since Alice pays for fees. Bob only need to keep his balance above + // the reserve. + expBobBalance := bobBalance - bobReserve + checkBalance(t, expAliceBalance, expBobBalance) // Find the minumim size of a non-dust HTLC. aliceNonDustHtlc := aliceDustlimit + htlcTimeoutFee - // Send a HTLC leaving the remaining balance just enough to have + // Send a HTLC leaving Alice's remaining balance just enough to have // nonDustHtlc left after paying the commit fee and htlc fee. htlcAmt := aliceBalance - (commitFee + aliceReserve + htlcFee + aliceNonDustHtlc) sendHtlc(htlcAmt) @@ -4826,7 +4847,8 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { // reported will just be nonDustHtlc, since the rest of the balance is // reserved. expAliceBalance = aliceNonDustHtlc - checkBalance(t, expAliceBalance) + expBobBalance = bobBalance - bobReserve + checkBalance(t, expAliceBalance, expBobBalance) // Send an HTLC using all but one msat of the reported balance. htlcAmt = aliceNonDustHtlc - 1 @@ -4834,7 +4856,12 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { // 1 msat should be left. expAliceBalance = 1 - checkBalance(t, expAliceBalance) + + // Bob should still have all his balance available, since even though + // Alice cannot afford to add a non-dust HTLC, she can afford to add a + // non-dust HTLC from Bob. + expBobBalance = bobBalance - bobReserve + checkBalance(t, expAliceBalance, expBobBalance) // Sendng the last msat. htlcAmt = 1 @@ -4842,7 +4869,30 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { // No balance left. expAliceBalance = 0 - checkBalance(t, expAliceBalance) + + // We try to always reserve enough for the non-iniitator to be able to + // add an HTLC, hence Bob should still have all his non-reserved + // balance available. + expBobBalance = bobBalance - bobReserve + checkBalance(t, expAliceBalance, expBobBalance) + + // Even though Alice has a reported balance of 0, this is because we + // try to avoid getting into the position where she cannot pay the fee + // for Bob adding another HTLC. This means she actually _has_ some + // balance left, and we now force the channel into this situation by + // sending yet another HTLC. In practice this can also happen if a fee + // update eats into Alice's balance. + htlcAmt = 1 + sendHtlc(htlcAmt) + + // Now Alice balance is so low that she cannot even afford to add a new + // HTLC from Bob to the commitment transaction. Bob's balance should + // reflect this, by only reporting dust amount being available. Alice + // should still report a zero balance. + bobNonDustHtlc := bobDustlimit + htlcTimeoutFee + expBobBalance = bobNonDustHtlc - 1 + expAliceBalance = 0 + checkBalance(t, expAliceBalance, expBobBalance) } // TestSignCommitmentFailNotLockedIn tests that a channel will not attempt to From e398544b8bc385f16d5688021d7ca5101ec4f7a1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:42 +0100 Subject: [PATCH 8/9] lnwallet/channel: take remote commitment view into availableBalance calculation Since our HTLC must also be added to the remote commitment, we do the balance caluclation also from the remote chain perspective and report our minimum balance from the two commit views as our available balance. --- lnwallet/channel.go | 29 +++++++++++++++++++++++++---- lnwallet/channel_test.go | 13 +++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 0cc512d5e..01be61176 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6030,9 +6030,19 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) { // add updates concurrently, causing our balance to go down if we're // the initiator, but this is a problem on the protocol level. ourLocalCommitBalance, commitWeight := lc.availableCommitmentBalance( - htlcView, + htlcView, false, ) + // Do the same calculation from the remote commitment point of view. + ourRemoteCommitBalance, _ := lc.availableCommitmentBalance( + htlcView, true, + ) + + // Return which ever balance is lowest. + if ourRemoteCommitBalance < ourLocalCommitBalance { + return ourRemoteCommitBalance, commitWeight + } + return ourLocalCommitBalance, commitWeight } @@ -6043,14 +6053,14 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) { // commitment, increasing the commitment fee we must pay as an initiator, // eating into our balance. It will make sure we won't violate the channel // reserve constraints for this amount. -func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( - lnwire.MilliSatoshi, int64) { +func (lc *LightningChannel) availableCommitmentBalance(view *htlcView, + remoteChain bool) (lnwire.MilliSatoshi, int64) { // Compute the current balances for this commitment. This will take // into account HTLCs to determine the commit weight, which the // initiator must pay the fee for. ourBalance, theirBalance, commitWeight, filteredView, err := lc.computeView( - view, false, false, + view, remoteChain, false, ) if err != nil { lc.log.Errorf("Unable to fetch available balance: %v", err) @@ -6118,6 +6128,17 @@ func (lc *LightningChannel) availableCommitmentBalance(view *htlcView) ( htlcTimeoutFee(feePerKw), ) + // If we are looking at the remote commitment, we must use the remote + // dust limit and the fee for adding an HTLC success transaction. + if remoteChain { + dustlimit = lnwire.NewMSatFromSatoshis( + lc.channelState.RemoteChanCfg.DustLimit, + ) + htlcFee = lnwire.NewMSatFromSatoshis( + htlcSuccessFee(feePerKw), + ) + } + // The HTLC output will be manifested on the commitment if it // is non-dust after paying the HTLC fee. nonDustHtlcAmt := dustlimit + htlcFee diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index be7395201..c4c886709 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -4749,9 +4749,6 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { aliceDustlimit := lnwire.NewMSatFromSatoshis( aliceChannel.channelState.LocalChanCfg.DustLimit, ) - bobDustlimit := lnwire.NewMSatFromSatoshis( - bobChannel.channelState.LocalChanCfg.DustLimit, - ) feeRate := chainfee.SatPerKWeight( aliceChannel.channelState.LocalCommitment.FeePerKw, ) @@ -4764,6 +4761,9 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { htlcTimeoutFee := lnwire.NewMSatFromSatoshis( htlcTimeoutFee(feeRate), ) + htlcSuccessFee := lnwire.NewMSatFromSatoshis( + htlcSuccessFee(feeRate), + ) // Helper method to check the current reported balance. checkBalance := func(t *testing.T, expBalanceAlice, @@ -4889,7 +4889,12 @@ func TestChanAvailableBalanceNearHtlcFee(t *testing.T) { // HTLC from Bob to the commitment transaction. Bob's balance should // reflect this, by only reporting dust amount being available. Alice // should still report a zero balance. - bobNonDustHtlc := bobDustlimit + htlcTimeoutFee + + // Since the dustlimit is different for the two commitments, the + // largest HTLC Bob can send that Alice can afford on both commitments + // (remember she cannot afford to pay the HTLC fee) is the largest dust + // HTLC on Alice's commitemnt, since her dust limit is lower. + bobNonDustHtlc := aliceDustlimit + htlcSuccessFee expBobBalance = bobNonDustHtlc - 1 expAliceBalance = 0 checkBalance(t, expAliceBalance, expBobBalance) From 3e5f2e437da7e7b37d4bdc98e543900b78ecc581 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 19 Feb 2020 12:27:42 +0100 Subject: [PATCH 9/9] lnwallet/channel: add TODO --- lnwallet/channel.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 01be61176..08ec5ebca 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4623,6 +4623,12 @@ func (lc *LightningChannel) InitNextRevocation(revKey *btcec.PublicKey) error { // The additional openKey argument corresponds to the incoming CircuitKey of the // committed circuit for this HTLC. This value should never be nil. // +// Note that AddHTLC doesn't reserve the HTLC fee for future payment (like +// AvailableBalance does), so one could get into the "stuck channel" state by +// sending dust HTLCs. +// TODO(halseth): fix this either by using additional reserve, or better commit +// format. See https://github.com/lightningnetwork/lightning-rfc/issues/728 +// // NOTE: It is okay for sourceRef to be nil when unit testing the wallet. func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC, openKey *channeldb.CircuitKey) (uint64, error) { @@ -6025,6 +6031,8 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) { lc.localUpdateLog.logIndex) // Calculate our available balance from our local commitment. + // TODO(halseth): could reuse parts validateCommitmentSanity to do this + // balance calculation, as most of the logic is the same. // // NOTE: This is not always accurate, since the remote node can always // add updates concurrently, causing our balance to go down if we're