diff --git a/channeldb/mp_payment.go b/channeldb/mp_payment.go index df10385a9..55112f3c0 100644 --- a/channeldb/mp_payment.go +++ b/channeldb/mp_payment.go @@ -163,11 +163,6 @@ type MPPaymentState struct { // calculate remaining fee budget. FeesPaid lnwire.MilliSatoshi - // Terminate indicates the payment is in its final stage and no more - // shards should be launched. This value is true if we have an HTLC - // settled or the payment has an error. - Terminate bool - // HasSettledHTLC is true if at least one of the payment's HTLCs is // settled. HasSettledHTLC bool @@ -327,11 +322,6 @@ func (m *MPPayment) setState() error { // Get any terminal info for this payment. settle, failure := m.TerminalInfo() - // If either an HTLC settled, or the payment has a payment level - // failure recorded, it means we should terminate the moment all shards - // have returned with a result. - terminate := settle != nil || failure != nil - // Now determine the payment's status. status, err := decidePaymentStatus(m.HTLCs, m.FailureReason) if err != nil { @@ -343,7 +333,6 @@ func (m *MPPayment) setState() error { NumAttemptsInFlight: len(m.InFlightHTLCs()), RemainingAmt: totalAmt - sentAmt, FeesPaid: fees, - Terminate: terminate, HasSettledHTLC: settle != nil, PaymentFailed: failure != nil, } @@ -361,6 +350,102 @@ func (m *MPPayment) SetState() error { return m.setState() } +// NeedWaitAttempts decides whether we need to hold creating more HTLC attempts +// and wait for the results of the payment's inflight HTLCs. Return an error if +// the payment is in an unexpected state. +func (m *MPPayment) NeedWaitAttempts() (bool, error) { + // Check when the remainingAmt is not zero, which means we have more + // money to be sent. + if m.State.RemainingAmt != 0 { + switch m.Status { + // If the payment is newly created, no need to wait for HTLC + // results. + case StatusInitiated: + return false, nil + + // If we have inflight HTLCs, we'll check if we have terminal + // states to decide if we need to wait. + case StatusInFlight: + // We still have money to send, and one of the HTLCs is + // settled. We'd stop sending money and wait for all + // inflight HTLC attempts to finish. + if m.State.HasSettledHTLC { + log.Warnf("payment=%v has remaining amount "+ + "%v, yet at least one of its HTLCs is "+ + "settled", m.Info.PaymentIdentifier, + m.State.RemainingAmt) + + return true, nil + } + + // The payment has a failure reason though we still + // have money to send, we'd stop sending money and wait + // for all inflight HTLC attempts to finish. + if m.State.PaymentFailed { + return true, nil + } + + // Otherwise we don't need to wait for inflight HTLCs + // since we still have money to be sent. + return false, nil + + // We need to send more money, yet the payment is already + // succeeded. Return an error in this case as the receiver is + // violating the protocol. + case StatusSucceeded: + return false, fmt.Errorf("%w: parts of the payment "+ + "already succeeded but still have remaining "+ + "amount %v", ErrPaymentInternal, + m.State.RemainingAmt) + + // The payment is failed and we have no inflight HTLCs, no need + // to wait. + case StatusFailed: + return false, nil + + // Unknown payment status. + default: + return false, fmt.Errorf("%w: %s", + ErrUnknownPaymentStatus, m.Status) + } + } + + // Now we determine whether we need to wait when the remainingAmt is + // already zero. + switch m.Status { + // When the payment is newly created, yet the payment has no remaining + // amount, return an error. + case StatusInitiated: + return false, fmt.Errorf("%w: %v", ErrPaymentInternal, m.Status) + + // If the payment is inflight, we must wait. + // + // NOTE: an edge case is when all HTLCs are failed while the payment is + // not failed we'd still be in this inflight state. However, since the + // remainingAmt is zero here, it means we cannot be in that state as + // otherwise the remainingAmt would not be zero. + case StatusInFlight: + return true, nil + + // If the payment is already succeeded, no need to wait. + case StatusSucceeded: + return false, nil + + // If the payment is already failed, yet the remaining amount is zero, + // return an error as this indicates an error state. We will only each + // this status when there are no inflight HTLCs and the payment is + // marked as failed with a reason, which means the remainingAmt must + // not be zero because our sentAmt is zero. + case StatusFailed: + return false, fmt.Errorf("%w: %v", ErrPaymentInternal, m.Status) + + // Unknown payment status. + default: + return false, fmt.Errorf("%w: %s", ErrUnknownPaymentStatus, + m.Status) + } +} + // serializeHTLCSettleInfo serializes the details of a settled htlc. func serializeHTLCSettleInfo(w io.Writer, s *HTLCSettleInfo) error { if _, err := w.Write(s.Preimage[:]); err != nil { diff --git a/channeldb/mp_payment_test.go b/channeldb/mp_payment_test.go index e18cf5989..ef00d90cc 100644 --- a/channeldb/mp_payment_test.go +++ b/channeldb/mp_payment_test.go @@ -229,6 +229,145 @@ func TestPaymentSetState(t *testing.T) { } } +// TestNeedWaitAttempts checks whether we need to wait for the results of the +// HTLC attempts against ALL possible payment statuses. +func TestNeedWaitAttempts(t *testing.T) { + t.Parallel() + + testCases := []struct { + status PaymentStatus + remainingAmt lnwire.MilliSatoshi + hasSettledHTLC bool + hasFailureReason bool + needWait bool + expectedErr error + }{ + { + // For a newly created payment we don't need to wait + // for results. + status: StatusInitiated, + remainingAmt: 1000, + needWait: false, + expectedErr: nil, + }, + { + // With HTLCs inflight we don't need to wait when the + // remainingAmt is not zero and we have no settled + // HTLCs. + status: StatusInFlight, + remainingAmt: 1000, + needWait: false, + expectedErr: nil, + }, + { + // With HTLCs inflight we need to wait when the + // remainingAmt is not zero but we have settled HTLCs. + status: StatusInFlight, + remainingAmt: 1000, + hasSettledHTLC: true, + needWait: true, + expectedErr: nil, + }, + { + // With HTLCs inflight we need to wait when the + // remainingAmt is not zero and the payment is failed. + status: StatusInFlight, + remainingAmt: 1000, + needWait: true, + hasFailureReason: true, + expectedErr: nil, + }, + + { + // With the payment settled, but the remainingAmt is + // not zero, we have an error state. + status: StatusSucceeded, + remainingAmt: 1000, + needWait: false, + expectedErr: ErrPaymentInternal, + }, + { + // Payment is in terminal state, no need to wait. + status: StatusFailed, + remainingAmt: 1000, + needWait: false, + expectedErr: nil, + }, + { + // A newly created payment with zero remainingAmt + // indicates an error. + status: StatusInitiated, + remainingAmt: 0, + needWait: false, + expectedErr: ErrPaymentInternal, + }, + { + // With zero remainingAmt we must wait for the results. + status: StatusInFlight, + remainingAmt: 0, + needWait: true, + expectedErr: nil, + }, + { + // Payment is terminated, no need to wait for results. + status: StatusSucceeded, + remainingAmt: 0, + needWait: false, + expectedErr: nil, + }, + { + // Payment is terminated, no need to wait for results. + status: StatusFailed, + remainingAmt: 0, + needWait: false, + expectedErr: ErrPaymentInternal, + }, + { + // Payment is in an unknown status, return an error. + status: 0, + remainingAmt: 0, + needWait: false, + expectedErr: ErrUnknownPaymentStatus, + }, + { + // Payment is in an unknown status, return an error. + status: 0, + remainingAmt: 1000, + needWait: false, + expectedErr: ErrUnknownPaymentStatus, + }, + } + + for _, tc := range testCases { + tc := tc + + p := &MPPayment{ + Info: &PaymentCreationInfo{ + PaymentIdentifier: [32]byte{1, 2, 3}, + }, + Status: tc.status, + State: &MPPaymentState{ + RemainingAmt: tc.remainingAmt, + HasSettledHTLC: tc.hasSettledHTLC, + PaymentFailed: tc.hasFailureReason, + }, + } + + name := fmt.Sprintf("status=%s|remainingAmt=%v|"+ + "settledHTLC=%v|failureReason=%v", tc.status, + tc.remainingAmt, tc.hasSettledHTLC, tc.hasFailureReason) + + t.Run(name, func(t *testing.T) { + t.Parallel() + + result, err := p.NeedWaitAttempts() + require.ErrorIs(t, err, tc.expectedErr) + require.Equalf(t, tc.needWait, result, "status=%v, "+ + "remainingAmt=%v", tc.status, tc.remainingAmt) + }) + } +} + func makeActiveAttempt(total, fee int) HTLCAttempt { return HTLCAttempt{ HTLCAttemptInfo: makeAttemptInfo(total, total-fee), diff --git a/channeldb/payment_control.go b/channeldb/payment_control.go index c726a6469..4ad29167e 100644 --- a/channeldb/payment_control.go +++ b/channeldb/payment_control.go @@ -30,6 +30,13 @@ var ( // existing payment that is not failed. ErrPaymentExists = errors.New("payment already exists") + // ErrPaymentInternal is returned when performing the payment has a + // conflicting state, such as, + // - payment has StatusSucceeded but remaining amount is not zero. + // - payment has StatusInitiated but remaining amount is zero. + // - payment has StatusFailed but remaining amount is zero. + ErrPaymentInternal = errors.New("internal error") + // ErrPaymentNotInitiated is returned if the payment wasn't initiated. ErrPaymentNotInitiated = errors.New("payment isn't initiated") diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index ea0f49247..09020667e 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -1,12 +1,12 @@ package routing import ( - "fmt" "sync" "time" "github.com/btcsuite/btcd/btcec/v2" "github.com/davecgh/go-spew/spew" + "github.com/go-errors/errors" sphinx "github.com/lightningnetwork/lightning-onion" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/htlcswitch" @@ -17,7 +17,7 @@ import ( ) // errShardHandlerExiting is returned from the shardHandler when it exits. -var errShardHandlerExiting = fmt.Errorf("shard handler exiting") +var errShardHandlerExiting = errors.New("shard handler exiting") // paymentLifecycle holds all information about the current state of a payment // needed to resume if from any point. @@ -31,27 +31,6 @@ type paymentLifecycle struct { currentHeight int32 } -// terminated returns a bool to indicate there are no further actions needed -// and we should return what we have, either the payment preimage or the -// payment error. -func terminated(ps *channeldb.MPPaymentState) bool { - // If the payment is in final stage and we have no in flight shards to - // wait result for, we consider the whole action terminated. - return ps.Terminate && ps.NumAttemptsInFlight == 0 -} - -// needWaitForShards returns a bool to specify whether we need to wait for the -// outcome of the shardHandler. -func needWaitForShards(ps *channeldb.MPPaymentState) bool { - // If we have in flight shards and the payment is in final stage, we - // need to wait for the outcomes from the shards. Or if we have no more - // money to be sent, we need to wait for the already launched shards. - if ps.NumAttemptsInFlight == 0 { - return false - } - return ps.Terminate || ps.RemainingAmt == 0 -} - // calcFeeBudget returns the available fee to be used for sending HTLC // attempts. func (p *paymentLifecycle) calcFeeBudget( @@ -132,15 +111,14 @@ lifecycle: log.Debugf("Payment %v in state terminate=%v, "+ "active_shards=%v, rem_value=%v, fee_limit=%v", - p.identifier, ps.Terminate, ps.NumAttemptsInFlight, - ps.RemainingAmt, remainingFees) + p.identifier, payment.Terminated(), + ps.NumAttemptsInFlight, ps.RemainingAmt, remainingFees) // TODO(yy): sanity check all the states to make sure // everything is expected. - switch { // We have a terminal condition and no active shards, we are // ready to exit. - case payment.Terminated(): + if payment.Terminated() { // Find the first successful shard and return // the preimage and route. for _, a := range payment.HTLCs { @@ -163,11 +141,17 @@ lifecycle: // Payment failed. return [32]byte{}, nil, *payment.FailureReason + } // If we either reached a terminal error condition (but had // active shards still) or there is no remaining value to send, // we'll wait for a shard outcome. - case needWaitForShards(ps): + wait, err := payment.NeedWaitAttempts() + if err != nil { + return [32]byte{}, nil, err + } + + if wait { // We still have outstanding shards, so wait for a new // outcome to be available before re-evaluating our // state.