From f83d56c91ff7bb5555e25b6b76d9a74584c9545b Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:08:43 -0800 Subject: [PATCH 01/10] lnwire: modify lnwire.MilliSatoshi to be an unsigned integer In this commit, we modify lnwire.MilliSatoshi to be an unsigned integer. We do this as all values within the specification are meant to be unsigned unless otherwise specified. Our usage of signed integers to this date has caused some compatibility issues with the other implementations, so this is the first step to reconciling these compatibility issues. --- lnwire/msat.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lnwire/msat.go b/lnwire/msat.go index 1a1e21e0..9ab0e31d 100644 --- a/lnwire/msat.go +++ b/lnwire/msat.go @@ -8,7 +8,7 @@ import ( // mSatScale is a value that's used to scale satoshis to milli-satoshis, and // the other way around. -const mSatScale int64 = 1000 +const mSatScale uint64 = 1000 // MilliSatoshi are the native unit of the Lightning Network. A milli-satoshi // is simply 1/1000th of a satoshi. There are 1000 milli-satoshis in a single @@ -16,12 +16,12 @@ const mSatScale int64 = 1000 // milli-satoshis. As milli-satoshis aren't deliverable on the native // blockchain, before settling to broadcasting, the values are rounded down to // the nearest satoshi. -type MilliSatoshi int64 +type MilliSatoshi uint64 // NewMSatFromSatoshis creates a new MilliSatoshi instance from a target amount // of satoshis. func NewMSatFromSatoshis(sat btcutil.Amount) MilliSatoshi { - return MilliSatoshi(int64(sat) * mSatScale) + return MilliSatoshi(uint64(sat) * mSatScale) } // ToBTC converts the target MilliSatoshi amount to its corresponding value @@ -34,10 +34,12 @@ func (m MilliSatoshi) ToBTC() float64 { // ToSatoshis converts the target MilliSatoshi amount to satoshis. Simply, this // sheds a factor of 1000 from the mSAT amount in order to convert it to SAT. func (m MilliSatoshi) ToSatoshis() btcutil.Amount { - return btcutil.Amount(int64(m) / mSatScale) + return btcutil.Amount(uint64(m) / mSatScale) } // String returns the string representation of the mSAT amount. func (m MilliSatoshi) String() string { - return fmt.Sprintf("%v mSAT", int64(m)) + return fmt.Sprintf("%v mSAT", uint64(m)) } + +// TODO(roasbeef): extend with arithmetic operations? From 5ecef17e0fa2439e809e65a9f2bec90b2b929baa Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:09:49 -0800 Subject: [PATCH 02/10] lnwallet: modify logging to display mSAT amount if funding constrains invalid --- lnwallet/reservation.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index ad0b6456..6f4de486 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -288,40 +288,36 @@ func (r *ChannelReservation) CommitConstraints(csvDelay, maxHtlcs uint16, return ErrCsvDelayTooLarge(csvDelay, maxDelay) } - // Fail if we consider the channel reserve to be too large. - // We currently fail if it is greater than 20% of the - // channel capacity. + // Fail if we consider the channel reserve to be too large. We + // currently fail if it is greater than 20% of the channel capacity. maxChanReserve := r.partialState.Capacity / 5 if chanReserve > maxChanReserve { return ErrChanReserveTooLarge(chanReserve, maxChanReserve) } - // Fail if the minimum HTLC value is too large. If this is - // too large, the channel won't be useful for sending small - // payments. This limit is currently set to maxValueInFlight, - // effictively letting the remote setting this as large as - // it wants. - // TODO(halseth): set a reasonable/dynamic value. + // Fail if the minimum HTLC value is too large. If this is too large, + // the channel won't be useful for sending small payments. This limit + // is currently set to maxValueInFlight, effectively letting the remote + // setting this as large as it wants. if minHtlc > maxValueInFlight { return ErrMinHtlcTooLarge(minHtlc, maxValueInFlight) } - // Fail if maxHtlcs is above the maximum allowed number of 483. - // This number is specified in BOLT-02. + // Fail if maxHtlcs is above the maximum allowed number of 483. This + // number is specified in BOLT-02. if maxHtlcs > uint16(MaxHTLCNumber/2) { return ErrMaxHtlcNumTooLarge(maxHtlcs, uint16(MaxHTLCNumber/2)) } - // Fail if we consider maxHtlcs too small. If this is too small - // we cannot offer many HTLCs to the remote. + // Fail if we consider maxHtlcs too small. If this is too small we + // cannot offer many HTLCs to the remote. const minNumHtlc = 5 if maxHtlcs < minNumHtlc { return ErrMaxHtlcNumTooSmall(maxHtlcs, minNumHtlc) } - // Fail if we consider maxValueInFlight too small. We currently - // require the remote to at least allow minNumHtlc * minHtlc - // in flight. + // Fail if we consider maxValueInFlight too small. We currently require + // the remote to at least allow minNumHtlc * minHtlc in flight. if maxValueInFlight < minNumHtlc*minHtlc { return ErrMaxValueInFlightTooSmall(maxValueInFlight, minNumHtlc*minHtlc) From 8425a35684b74dd63fa8e40341d0ea66f5fe41c8 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:11:14 -0800 Subject: [PATCH 03/10] lnwallet: in NewChannelReservation ensure commit fees are payable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we fix a bug introduced by the recent change of lnwire.MilliSatoshi to be an unsigned integer. After this change an integer underflow was left undetected, as a result we’ll now momentarily cast to a signed integer in order to ensure that both sides can pay the proper fee. --- lnwallet/reservation.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index 6f4de486..21a365bd 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -157,6 +157,15 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, ourBalance = pushMSat theirBalance = capacityMSat - feeMSat - pushMSat initiator = false + + // If the responder doesn't have enough funds to actually pay + // the fees, then we'll bail our early. + if int64(theirBalance) < 0 { + return nil, ErrFunderBalanceDust( + int64(commitFee), int64(theirBalance.ToSatoshis()), + int64(2*DefaultDustLimit()), + ) + } } else { // TODO(roasbeef): need to rework fee structure in general and // also when we "unlock" dual funder within the daemon @@ -177,6 +186,15 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, } initiator = true + + // If we, the initiator don't have enough funds to actually pay + // the fees, then we'll exit with an error. + if int64(ourBalance) < 0 { + return nil, ErrFunderBalanceDust( + int64(commitFee), int64(ourBalance), + int64(2*DefaultDustLimit()), + ) + } } // If we're the initiator and our starting balance within the channel From 19bc477b9ab14ca349e60faf18865f8197a42cc3 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:12:16 -0800 Subject: [PATCH 04/10] lnwallet: update interface tests to account for recent lnwire.MilliSatoshi change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lnwire.MilliSatoshi is now a signed integer, as a result, we’ll return a different error if our balances go to negative due to the inability to pay a the set fee. --- lnwallet/transactions_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lnwallet/transactions_test.go b/lnwallet/transactions_test.go index 47ee6d81..3be7a404 100644 --- a/lnwallet/transactions_test.go +++ b/lnwallet/transactions_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/keychain" "github.com/lightningnetwork/lnd/lnwire" @@ -806,8 +807,9 @@ func TestCommitmentAndHTLCTransactions(t *testing.T) { dustLimit: tc.dustLimit, isOurs: true, } - err = channel.createCommitmentTx(commitmentView, theHTLCView, - keys) + err = channel.createCommitmentTx( + commitmentView, theHTLCView, keys, + ) if err != nil { t.Errorf("Case %d: Failed to create new commitment tx: %v", i, err) continue @@ -836,8 +838,8 @@ func TestCommitmentAndHTLCTransactions(t *testing.T) { // Check that commitment transaction was created correctly. if commitTx.WitnessHash() != *expectedCommitmentTx.WitnessHash() { t.Errorf("Case %d: Generated unexpected commitment tx: "+ - "expected %s, got %s", i, expectedCommitmentTx.WitnessHash(), - commitTx.WitnessHash()) + "expected %s, got %s", i, spew.Sdump(expectedCommitmentTx), + spew.Sdump(commitTx)) continue } From 5f5e4554cbf8d037dd4db6402b1a0fed6f171ee4 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:15:25 -0800 Subject: [PATCH 05/10] lnwallet: if the initiator is unable to pay fees, then consume their entire output In this commit, we add logic to account for an edge case in the protocol. If they initiator if unable to pay the fees for a commitment, then their *entire* output is meant to go to fees. The recent change to properly interpret balances as unsigned integers (within the protocol) let to the discovery of this missed edge case. --- lnwallet/channel.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 66036dfa..0a0b1e29 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2142,10 +2142,19 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment, // Currently, within the protocol, the initiator always pays the fees. // So we'll subtract the fee amount from the balance of the current - // initiator. - if lc.channelState.IsInitiator { + // initiator. If the initiator is unable to pay the fee fully, then + // their entire output is consumed. + switch { + case lc.channelState.IsInitiator && commitFee > ourBalance.ToSatoshis(): + ourBalance = 0 + + case lc.channelState.IsInitiator: ourBalance -= lnwire.NewMSatFromSatoshis(commitFee) - } else if !lc.channelState.IsInitiator { + + case !lc.channelState.IsInitiator && commitFee > theirBalance.ToSatoshis(): + theirBalance = 0 + + case !lc.channelState.IsInitiator: theirBalance -= lnwire.NewMSatFromSatoshis(commitFee) } From 217166fb10301ef45dc2a00b295d8fe5014d2401 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:16:49 -0800 Subject: [PATCH 06/10] 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() From ac90a8288e20fa62ffb64621b2eaa9f63ddb3680 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:19:24 -0800 Subject: [PATCH 07/10] lnwallet: precalculate fees in mSAT to avoid multiple conversions --- lnwallet/channel.go | 62 ++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 842d57a1..7fdcecb9 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2139,6 +2139,7 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment, // With the weight known, we can now calculate the commitment fee, // ensuring that we account for any dust outputs trimmed above. commitFee := c.feePerKw.FeeForWeight(totalCommitWeight) + commitFeeMSat := lnwire.NewMSatFromSatoshis(commitFee) // Currently, within the protocol, the initiator always pays the fees. // So we'll subtract the fee amount from the balance of the current @@ -2149,13 +2150,13 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment, ourBalance = 0 case lc.channelState.IsInitiator: - ourBalance -= lnwire.NewMSatFromSatoshis(commitFee) + ourBalance -= commitFeeMSat case !lc.channelState.IsInitiator && commitFee > theirBalance.ToSatoshis(): theirBalance = 0 case !lc.channelState.IsInitiator: - theirBalance -= lnwire.NewMSatFromSatoshis(commitFee) + theirBalance -= commitFeeMSat } var ( @@ -3227,57 +3228,58 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, validateUpdates := func(updates []*PaymentDescriptor, constraints *channeldb.ChannelConfig) error { - // We keep track of the number of HTLCs in flight for - // the commitment, and the amount in flight. + // We keep track of the number of HTLCs in flight for the + // commitment, and the amount in flight. var numInFlight uint16 var amtInFlight lnwire.MilliSatoshi - // Go through all updates, checking that they don't - // violate the channel constraints. + // Go through all updates, checking that they don't violate the + // channel constraints. for _, entry := range updates { if entry.EntryType == Add { - // An HTLC is being added, this will - // add to the number and amount in - // flight. + // An HTLC is being added, this will add to the + // number and amount in flight. amtInFlight += entry.Amount numInFlight++ - // Check that the value of the HTLC they - // added is above our minimum. + // Check that the value of the HTLC they added + // is above our minimum. if entry.Amount < constraints.MinHTLC { return ErrBelowMinHTLC } } } - // Now that we know the total value of added HTLCs, - // we check that this satisfy the MaxPendingAmont - // contraint. + // Now that we know the total value of added HTLCs, we check + // that this satisfy the MaxPendingAmont contraint. if amtInFlight > constraints.MaxPendingAmount { return ErrMaxPendingAmount } - // In this step, we verify that the total number of - // active HTLCs does not exceed the constraint of the - // maximum number of HTLCs in flight. + // In this step, we verify that the total number of active + // HTLCs does not exceed the constraint of the maximum number + // of HTLCs in flight. if numInFlight > constraints.MaxAcceptedHtlcs { return ErrMaxHTLCNumber } + return nil } - // First check that the remote updates won't violate it's - // channel constraints. - err := validateUpdates(filteredView.theirUpdates, - lc.remoteChanCfg) + // First check that the remote updates won't violate it's channel + // constraints. + err := validateUpdates( + filteredView.theirUpdates, lc.remoteChanCfg, + ) if err != nil { return err } - // Secondly check that our updates won't violate our - // channel constraints. - err = validateUpdates(filteredView.ourUpdates, - lc.localChanCfg) + // Secondly check that our updates won't violate our channel + // constraints. + err = validateUpdates( + filteredView.ourUpdates, lc.localChanCfg, + ) if err != nil { return err } @@ -3836,11 +3838,13 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC) (uint64, error) OnionBlob: htlc.OnionBlob[:], } - // Make sure adding this HTLC won't violate any of the constrainst - // we must keep on our commitment transaction. + // Make sure adding this HTLC won't violate any of the constraints we + // must keep on our commitment transaction. remoteACKedIndex := lc.localCommitChain.tail().theirMessageIndex - if err := lc.validateCommitmentSanity(remoteACKedIndex, - lc.localUpdateLog.logIndex, true, pd); err != nil { + err := lc.validateCommitmentSanity( + remoteACKedIndex, lc.localUpdateLog.logIndex, true, pd, + ) + if err != nil { return 0, err } From b8d0df998ae2c596ae2f8d1613b6d007f39aa84c Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:19:46 -0800 Subject: [PATCH 08/10] lnwallet: when validating fee updates, ensure newFee < balance --- lnwallet/channel.go | 12 ++++++++++-- lnwallet/channel_test.go | 4 ++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 7fdcecb9..3cb97f81 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3054,7 +3054,7 @@ func (lc *LightningChannel) ChanSyncMsg() (*lnwire.ChannelReestablish, error) { // 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 +// applying the HTLCs to the latest commitment. The returned balances are the // balances *before* subtracting the commitment fee from the initiator's // balance. // @@ -5137,10 +5137,18 @@ func (lc *LightningChannel) validateFeeRate(feePerKw SatPerKWeight) error { newFee := lnwire.NewMSatFromSatoshis( feePerKw.FeeForWeight(txWeight), ) - balanceAfterFee := availableBalance - newFee + + // If the total fee exceeds our available balance, then we'll reject + // this update as it would mean we need to trim our entire output. + if newFee > availableBalance { + return fmt.Errorf("cannot apply fee_update=%v sat/kw, new fee "+ + "of %v is greater than balance of %v", int64(feePerKw), + newFee, availableBalance) + } // If this new balance is below our reserve, then we can't accommodate // the fee change, so we'll reject it. + balanceAfterFee := availableBalance - newFee if balanceAfterFee.ToSatoshis() < lc.channelState.LocalChanCfg.ChanReserve { return fmt.Errorf("cannot apply fee_update=%v sat/kw, "+ "insufficient balance: start=%v, end=%v", diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 66834a19..29895f6f 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -2477,7 +2477,7 @@ 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 + // 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) @@ -4378,7 +4378,7 @@ func TestDesyncHTLCs(t *testing.T) { // 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 + // HTLC and signs the next state, Bob cannot assume she received the // FailHTLC, and must assume she doesn't have the necessary balance // available. // From 7031b5d2171f41bcccb8f348191e83e4ecf6140b Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 19:21:42 -0800 Subject: [PATCH 09/10] htlcswitch: modify forwarding fee assertion to compare emperical fees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this commit, we fix a bug that was uncovered by the recent change to lnwire.MilliSatoshi. Rather than manually compute the diff in fees, we’ll directly compare the fee that is given against the fee that we expect. --- htlcswitch/link.go | 15 +++++++++------ htlcswitch/link_test.go | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index ae923277..09257ff4 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1729,15 +1729,18 @@ func (l *channelLink) processLockedInHtlcs( fwdInfo.AmountToForward, ) - // If the amount of the incoming HTLC, minus - // our expected fee isn't equal to the - // forwarding instructions, then either the - // values have been tampered with, or the send - // used incorrect/dated information to + // If the actual fee is less than our expected + // fee, then we'll reject this HTLC as it + // didn't provide a sufficient amount of fees, + // or the values have been tampered with, or + // the send used incorrect/dated information to // construct the forwarding information for // this hop. In any case, we'll cancel this // HTLC. - if pd.Amount-expectedFee < fwdInfo.AmountToForward { + actualFee := pd.Amount - fwdInfo.AmountToForward + if pd.Amount < fwdInfo.AmountToForward || + actualFee < expectedFee { + log.Errorf("Incoming htlc(%x) has "+ "insufficient fee: expected "+ "%v, got %v", pd.RHash[:], diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 8add4e84..6602dd5a 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -777,7 +777,7 @@ func TestUpdateForwardingPolicy(t *testing.T) { testStartingHeight, n.firstBobChannelLink, n.carolChannelLink) - // First, send this 1 BTC payment over the three hops, the payment + // First, send this 10 mSAT payment over the three hops, the payment // should succeed, and all balances should be updated accordingly. payResp, err := n.makePayment(n.aliceServer, n.carolServer, n.bobServer.PubKey(), hops, amountNoFee, htlcAmt, @@ -827,7 +827,7 @@ func TestUpdateForwardingPolicy(t *testing.T) { n.firstBobChannelLink.UpdateForwardingPolicy(newPolicy) // Next, we'll send the payment again, using the exact same per-hop - // payload for each node. This payment should fail as it wont' factor + // payload for each node. This payment should fail as it won't factor // in Bob's new fee policy. _, err = n.makePayment(n.aliceServer, n.carolServer, n.bobServer.PubKey(), hops, amountNoFee, htlcAmt, From 8d35ea381e437e3f91f998a8cc39b1609b256a34 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Sat, 24 Feb 2018 20:11:18 -0800 Subject: [PATCH 10/10] zpay32: remove test case with negative amt --- zpay32/invoice_internal_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/zpay32/invoice_internal_test.go b/zpay32/invoice_internal_test.go index 99fa8375..50596baf 100644 --- a/zpay32/invoice_internal_test.go +++ b/zpay32/invoice_internal_test.go @@ -149,10 +149,6 @@ func TestEncodeAmount(t *testing.T) { valid bool result string }{ - { - msat: -10, // mSat - valid: false, // negative amount - }, { msat: 1, // mSat valid: true,