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, diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 66036dfa..3cb97f81 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2139,14 +2139,24 @@ 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 - // initiator. - if lc.channelState.IsInitiator { - ourBalance -= lnwire.NewMSatFromSatoshis(commitFee) - } else if !lc.channelState.IsInitiator { - theirBalance -= lnwire.NewMSatFromSatoshis(commitFee) + // 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 -= commitFeeMSat + + case !lc.channelState.IsInitiator && commitFee > theirBalance.ToSatoshis(): + theirBalance = 0 + + case !lc.channelState.IsInitiator: + theirBalance -= commitFeeMSat } var ( @@ -3042,14 +3052,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 balances 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) { @@ -3061,16 +3071,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) @@ -3080,11 +3090,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) @@ -3155,6 +3164,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) @@ -3162,8 +3172,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) } @@ -3174,21 +3184,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) { @@ -3201,62 +3223,63 @@ 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 { - // 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 } @@ -3815,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 } @@ -5112,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 3ea70f15..29895f6f 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 signs 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() diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index ad0b6456..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 @@ -288,40 +306,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) 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 } 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? 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,