From 217166fb10301ef45dc2a00b295d8fe5014d2401 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:16:49 -0800 Subject: [PATCH] lnwallet: within validateCommitmentSanity check for balance underflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we add an additional check within validateCommitmentSanity due to the recent change to unsigned integers for peer balances in the channel state machine. If after evaluation (just applying HTLC updates), the balances are negative, then we’ll return ErrBelowChanReserve. --- lnwallet/channel.go | 72 +++++++++++++++++++++++----------------- lnwallet/channel_test.go | 37 +++++++++++---------- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 0a0b1e29..842d57a1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3051,14 +3051,14 @@ func (lc *LightningChannel) ChanSyncMsg() (*lnwire.ChannelReestablish, error) { }, nil } -// computeView takes the given htlcView, and calculates the balances, -// filtered view (settling unsettled HTLCs), commitment weight and -// feePerKw, after applying the HTLCs to the latest commitment. The -// returned balanced are the balances *before* subtracting the -// commitment fee from the initiator's balance. +// computeView takes the given htlcView, and calculates the balances, filtered +// view (settling unsettled HTLCs), commitment weight and feePerKw, after +// applying the HTLCs to the latest commitment. The returned balanced are the +// balances *before* subtracting the commitment fee from the initiator's +// balance. // -// If the updateState boolean is set true, the add and remove heights -// of the HTLCs will be set to the next commitment height. +// If the updateState boolean is set true, the add and remove heights of the +// HTLCs will be set to the next commitment height. func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, updateState bool) (lnwire.MilliSatoshi, lnwire.MilliSatoshi, int64, *htlcView, SatPerKWeight) { @@ -3070,16 +3070,16 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, dustLimit = lc.remoteChanCfg.DustLimit } - // Since the fetched htlc view will include all updates added - // after the last committed state, we start with the balances - // reflecting that state. + // Since the fetched htlc view will include all updates added after the + // last committed state, we start with the balances reflecting that + // state. ourBalance := commitChain.tip().ourBalance theirBalance := commitChain.tip().theirBalance // Add the fee from the previous commitment state back to the // initiator's balance, so that the fee can be recalculated and - // re-applied in case fee estimation parameters have changed or - // the number of outstanding HTLCs has changed. + // re-applied in case fee estimation parameters have changed or the + // number of outstanding HTLCs has changed. if lc.channelState.IsInitiator { ourBalance += lnwire.NewMSatFromSatoshis( commitChain.tip().fee) @@ -3089,11 +3089,10 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, } nextHeight := commitChain.tip().height + 1 - // We evaluate the view at this stage, meaning settled and - // failed HTLCs will remove their corresponding added HTLCs. - // The resulting filtered view will only have Add entries left, - // making it easy to compare the channel constraints to the - // final commitment state. + // We evaluate the view at this stage, meaning settled and failed HTLCs + // will remove their corresponding added HTLCs. The resulting filtered + // view will only have Add entries left, making it easy to compare the + // channel constraints to the final commitment state. filteredHTLCView := lc.evaluateHTLCView(view, &ourBalance, &theirBalance, nextHeight, remoteChain, updateState) @@ -3164,6 +3163,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, ourLogCounter uint64, remoteChain bool, predictAdded *PaymentDescriptor) error { + // Fetch all updates not committed. view := lc.fetchHTLCView(theirLogCounter, ourLogCounter) @@ -3171,8 +3171,8 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, // update log, in order to validate the sanity of the commitment // resulting from _actually adding_ this HTLC to the state. if predictAdded != nil { - // If we are adding an HTLC, this will be an Add to the - // local update log. + // If we are adding an HTLC, this will be an Add to the local + // update log. view.ourUpdates = append(view.ourUpdates, predictAdded) } @@ -3183,21 +3183,33 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, ourInitialBalance := commitChain.tip().ourBalance theirInitialBalance := commitChain.tip().theirBalance - ourBalance, theirBalance, commitWeight, filteredView, feePerKw := - lc.computeView(view, remoteChain, false) + ourBalance, theirBalance, commitWeight, filteredView, feePerKw := lc.computeView( + view, remoteChain, false, + ) - // Calculate the commitment fee, and subtract it from the - // initiator's balance. + // Calculate the commitment fee, and subtract it from the initiator's + // balance. commitFee := feePerKw.FeeForWeight(commitWeight) + commitFeeMsat := lnwire.NewMSatFromSatoshis(commitFee) if lc.channelState.IsInitiator { - ourBalance -= lnwire.NewMSatFromSatoshis(commitFee) + ourBalance -= commitFeeMsat } else { - theirBalance -= lnwire.NewMSatFromSatoshis(commitFee) + theirBalance -= commitFeeMsat } - // If the added HTLCs will decrease the balance, make sure - // they won't dip the local and remote balances below the - // channel reserves. + // As a quick sanity check, we'll ensure that if we interpret the + // balances as signed integers, they haven't dipped down below zero. If + // they have, then this indicates that a party doesn't have sufficient + // balance to satisfy the final evaluated HTLC's. + switch { + case int64(ourBalance) < 0: + return ErrBelowChanReserve + case int64(theirBalance) < 0: + return ErrBelowChanReserve + } + + // If the added HTLCs will decrease the balance, make sure they won't + // dip the local and remote balances below the channel reserves. if ourBalance < ourInitialBalance && ourBalance < lnwire.NewMSatFromSatoshis( lc.localChanCfg.ChanReserve) { @@ -3210,8 +3222,8 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, return ErrBelowChanReserve } - // validateUpdates take a set of updates, and validates them - // against the passed channel constraints. + // validateUpdates take a set of updates, and validates them against + // the passed channel constraints. validateUpdates := func(updates []*PaymentDescriptor, constraints *channeldb.ChannelConfig) error { diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 3ea70f15..66834a19 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -2462,8 +2462,8 @@ func TestAddHTLCNegativeBalance(t *testing.T) { } defer cleanUp() - // We set the channel reserve to 0, such that we can add HTLCs - // all the way to a negative balance. + // We set the channel reserve to 0, such that we can add HTLCs all the + // way to a negative balance. aliceChannel.localChanCfg.ChanReserve = 0 // First, we'll add 3 HTLCs of 1 BTC each to Alice's commitment. @@ -2476,14 +2476,15 @@ func TestAddHTLCNegativeBalance(t *testing.T) { } } - // Alice now has an available balance of 2 BTC. We'll add a new HTLC - // of value 2 BTC, which should make Alice's balance negative (since - // (she has to pay a commitment fee). + // Alice now has an available balance of 2 BTC. We'll add a new HTLC of + // value 2 BTC, which should make Alice's balance negative (since (she + // has to pay a commitment fee). htlcAmt = lnwire.NewMSatFromSatoshis(2 * btcutil.SatoshiPerBitcoin) htlc, _ := createHTLC(numHTLCs+1, htlcAmt) _, err = aliceChannel.AddHTLC(htlc) if err != ErrBelowChanReserve { - t.Fatalf("expected balance below channel reserve, instead got: %v", err) + t.Fatalf("expected balance below channel reserve, instead "+ + "got: %v", err) } } @@ -4375,23 +4376,22 @@ func TestDesyncHTLCs(t *testing.T) { t.Fatalf("unable to recv htlc cancel: %v", err) } - // Alice now has gotten all here original balance (5 BTC) back, - // however, adding a new HTLC at this point SHOULD fail, since - // if she add the HTLC and sign the next state, Bob cannot assume - // she received the FailHTLC, and must assume she doesn't have - // the necessary balance available. + // Alice now has gotten all her original balance (5 BTC) back, however, + // adding a new HTLC at this point SHOULD fail, since if she adds the + // HTLC and sign the next state, Bob cannot assume she received the + // FailHTLC, and must assume she doesn't have the necessary balance + // available. // - // We try adding an HTLC of value 1 BTC, which should fail - // because the balance is unavailable. + // We try adding an HTLC of value 1 BTC, which should fail because the + // balance is unavailable. htlcAmt = lnwire.NewMSatFromSatoshis(1 * btcutil.SatoshiPerBitcoin) htlc, _ = createHTLC(1, htlcAmt) if _, err = aliceChannel.AddHTLC(htlc); err != ErrBelowChanReserve { - t.Fatalf("expected ErrInsufficientBalance, instead received: %v", - err) + t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err) } - // Now do a state transition, which will ACK the FailHTLC, making - // Alice able to add the new HTLC. + // Now do a state transition, which will ACK the FailHTLC, making Alice + // able to add the new HTLC. if err := forceStateTransition(aliceChannel, bobChannel); err != nil { t.Fatalf("unable to complete state update: %v", err) } @@ -4403,7 +4403,8 @@ func TestDesyncHTLCs(t *testing.T) { // TODO(roasbeef): testing.Quick test case for retrans!!! // TestMaxAcceptedHTLCs tests that the correct error message (ErrMaxHTLCNumber) -// is thrown when a node tries to accept more than MaxAcceptedHTLCs in a channel. +// is thrown when a node tries to accept more than MaxAcceptedHTLCs in a +// channel. func TestMaxAcceptedHTLCs(t *testing.T) { t.Parallel()