From e8c0226e1cb2437befb551e1b80d271f8760486f Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 8 Mar 2023 00:48:22 +0800 Subject: [PATCH] routing: add `AllowMoreAttempts` to decide whether more attempts are allowed --- channeldb/mp_payment.go | 45 +++++++++ channeldb/mp_payment_test.go | 177 +++++++++++++++++++++++++++++++++++ routing/control_tower.go | 5 + routing/mock_test.go | 5 + 4 files changed, 232 insertions(+) diff --git a/channeldb/mp_payment.go b/channeldb/mp_payment.go index 30c595049..a4b323349 100644 --- a/channeldb/mp_payment.go +++ b/channeldb/mp_payment.go @@ -469,6 +469,51 @@ func (m *MPPayment) GetFailureReason() *FailureReason { return m.FailureReason } +// AllowMoreAttempts is used to decide whether we can safely attempt more HTLCs +// for a given payment state. Return an error if the payment is in an +// unexpected state. +func (m *MPPayment) AllowMoreAttempts() (bool, error) { + // Now check whether the remainingAmt is zero or not. If we don't have + // any remainingAmt, no more HTLCs should be made. + if m.State.RemainingAmt == 0 { + // If the payment is newly created, yet we don't have any + // remainingAmt, return an error. + if m.Status == StatusInitiated { + return false, fmt.Errorf("%w: initiated payment has "+ + "zero remainingAmt", ErrPaymentInternal) + } + + // Otherwise, exit early since all other statuses with zero + // remainingAmt indicate no more HTLCs can be made. + return false, nil + } + + // Otherwise, the remaining amount is not zero, we now decide whether + // to make more attempts based on the payment's current status. + // + // If at least one of the payment's attempts is settled, yet we haven't + // sent all the amount, it indicates something is wrong with the peer + // as the preimage is received. In this case, return an error state. + if m.Status == StatusSucceeded { + return false, fmt.Errorf("%w: payment already succeeded but "+ + "still have remaining amount %v", ErrPaymentInternal, + m.State.RemainingAmt) + } + + // Now check if we can register a new HTLC. + err := m.Registrable() + if err != nil { + log.Warnf("Payment(%v): cannot register HTLC attempt: %v, "+ + "current status: %s", m.Info.PaymentIdentifier, + err, m.Status) + + return false, nil + } + + // Now we know we can register new HTLCs. + return true, nil +} + // 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 ef00d90cc..51eda72bb 100644 --- a/channeldb/mp_payment_test.go +++ b/channeldb/mp_payment_test.go @@ -368,6 +368,183 @@ func TestNeedWaitAttempts(t *testing.T) { } } +// TestAllowMoreAttempts checks whether more attempts can be created against +// ALL possible payment statuses. +func TestAllowMoreAttempts(t *testing.T) { + t.Parallel() + + testCases := []struct { + status PaymentStatus + remainingAmt lnwire.MilliSatoshi + hasSettledHTLC bool + paymentFailed bool + allowMore bool + expectedErr error + }{ + { + // A newly created payment with zero remainingAmt + // indicates an error. + status: StatusInitiated, + remainingAmt: 0, + allowMore: false, + expectedErr: ErrPaymentInternal, + }, + { + // With zero remainingAmt we don't allow more HTLC + // attempts. + status: StatusInFlight, + remainingAmt: 0, + allowMore: false, + expectedErr: nil, + }, + { + // With zero remainingAmt we don't allow more HTLC + // attempts. + status: StatusSucceeded, + remainingAmt: 0, + allowMore: false, + expectedErr: nil, + }, + { + // With zero remainingAmt we don't allow more HTLC + // attempts. + status: StatusFailed, + remainingAmt: 0, + allowMore: false, + expectedErr: nil, + }, + { + // With zero remainingAmt and settled HTLCs we don't + // allow more HTLC attempts. + status: StatusInFlight, + remainingAmt: 0, + hasSettledHTLC: true, + allowMore: false, + expectedErr: nil, + }, + { + // With zero remainingAmt and failed payment we don't + // allow more HTLC attempts. + status: StatusInFlight, + remainingAmt: 0, + paymentFailed: true, + allowMore: false, + expectedErr: nil, + }, + { + // With zero remainingAmt and both settled HTLCs and + // failed payment, we don't allow more HTLC attempts. + status: StatusInFlight, + remainingAmt: 0, + hasSettledHTLC: true, + paymentFailed: true, + allowMore: false, + expectedErr: nil, + }, + { + // A newly created payment can have more attempts. + status: StatusInitiated, + remainingAmt: 1000, + allowMore: true, + expectedErr: nil, + }, + { + // With HTLCs inflight we can have more attempts when + // the remainingAmt is not zero and we have neither + // failed payment or settled HTLCs. + status: StatusInFlight, + remainingAmt: 1000, + allowMore: true, + expectedErr: nil, + }, + { + // With HTLCs inflight we cannot have more attempts + // though the remainingAmt is not zero but we have + // settled HTLCs. + status: StatusInFlight, + remainingAmt: 1000, + hasSettledHTLC: true, + allowMore: false, + expectedErr: nil, + }, + { + // With HTLCs inflight we cannot have more attempts + // though the remainingAmt is not zero but we have + // failed payment. + status: StatusInFlight, + remainingAmt: 1000, + paymentFailed: true, + allowMore: false, + expectedErr: nil, + }, + { + // With HTLCs inflight we cannot have more attempts + // though the remainingAmt is not zero but we have + // settled HTLCs and failed payment. + status: StatusInFlight, + remainingAmt: 1000, + hasSettledHTLC: true, + paymentFailed: true, + allowMore: false, + expectedErr: nil, + }, + { + // With the payment settled, but the remainingAmt is + // not zero, we have an error state. + status: StatusSucceeded, + remainingAmt: 1000, + hasSettledHTLC: true, + allowMore: false, + expectedErr: ErrPaymentInternal, + }, + { + // With the payment failed with no inflight HTLCs, we + // don't allow more attempts to be made. + status: StatusFailed, + remainingAmt: 1000, + paymentFailed: true, + allowMore: false, + expectedErr: nil, + }, + { + // With the payment in an unknown state, we don't allow + // more attempts to be made. + status: 0, + remainingAmt: 1000, + allowMore: false, + expectedErr: nil, + }, + } + + for i, 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.paymentFailed, + }, + } + + name := fmt.Sprintf("test_%d|status=%s|remainingAmt=%v", i, + tc.status, tc.remainingAmt) + + t.Run(name, func(t *testing.T) { + t.Parallel() + + result, err := p.AllowMoreAttempts() + require.ErrorIs(t, err, tc.expectedErr) + require.Equalf(t, tc.allowMore, 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/routing/control_tower.go b/routing/control_tower.go index b23b7df5c..80ffbe5d9 100644 --- a/routing/control_tower.go +++ b/routing/control_tower.go @@ -33,6 +33,11 @@ type dbMPPayment interface { // GetFailureReason returns the reason the payment failed. GetFailureReason() *channeldb.FailureReason + + // AllowMoreAttempts is used to decide whether we can safely attempt + // more HTLCs for a given payment state. Return an error if the payment + // is in an unexpected state. + AllowMoreAttempts() (bool, error) } // ControlTower tracks all outgoing payments made, whose primary purpose is to diff --git a/routing/mock_test.go b/routing/mock_test.go index 6db22797e..5a2ee4206 100644 --- a/routing/mock_test.go +++ b/routing/mock_test.go @@ -839,6 +839,11 @@ func (m *mockMPPayment) GetFailureReason() *channeldb.FailureReason { return reason.(*channeldb.FailureReason) } +func (m *mockMPPayment) AllowMoreAttempts() (bool, error) { + args := m.Called() + return args.Bool(0), args.Error(1) +} + type mockLink struct { htlcswitch.ChannelLink bandwidth lnwire.MilliSatoshi