Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing+htlcswitch: fix stuck inflight payments #9150

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
7 changes: 6 additions & 1 deletion channeldb/payment_control.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ var (
// amount exceed the total amount.
ErrSentExceedsTotal = errors.New("total sent exceeds total amount")

// ErrRegisterAttempt is returned when a new htlc attempt cannot be
// registered.
ErrRegisterAttempt = errors.New("cannot register htlc attempt")

// errNoAttemptInfo is returned when no attempt info is stored yet.
errNoAttemptInfo = errors.New("unable to find attempt info for " +
"inflight payment")
Expand Down Expand Up @@ -342,7 +346,8 @@ func (p *PaymentControl) RegisterAttempt(paymentHash lntypes.Hash,

// Check if registering a new attempt is allowed.
if err := payment.Registrable(); err != nil {
return err
return fmt.Errorf("%w: %v", ErrRegisterAttempt,
err.Error())
}

// If the final hop has encrypted data, then we know this is a
Expand Down
15 changes: 4 additions & 11 deletions channeldb/payment_control_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -841,9 +841,9 @@ func TestPaymentControlMultiShard(t *testing.T) {
b.AttemptID = 3
_, err = pControl.RegisterAttempt(info.PaymentIdentifier, &b)
if test.settleFirst {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have the if else here if both do the same thing ?

require.ErrorIs(t, err, ErrPaymentPendingSettled)
require.ErrorIs(t, err, ErrRegisterAttempt)
} else {
require.ErrorIs(t, err, ErrPaymentPendingFailed)
require.ErrorIs(t, err, ErrRegisterAttempt)
}

assertPaymentStatus(t, pControl, info.PaymentIdentifier, StatusInFlight)
Expand Down Expand Up @@ -897,32 +897,25 @@ func TestPaymentControlMultiShard(t *testing.T) {
require.NoError(t, err, "unable to fail")
}

var (
finalStatus PaymentStatus
registerErr error
)
var finalStatus PaymentStatus

switch {
// If one of the attempts settled but the other failed with
// terminal error, we would still consider the payment is
// settled.
case test.settleFirst && !test.settleLast:
finalStatus = StatusSucceeded
registerErr = ErrPaymentAlreadySucceeded

case !test.settleFirst && test.settleLast:
finalStatus = StatusSucceeded
registerErr = ErrPaymentAlreadySucceeded

// If both failed, we end up in a failed status.
case !test.settleFirst && !test.settleLast:
finalStatus = StatusFailed
registerErr = ErrPaymentAlreadyFailed

// Otherwise, the payment has a succeed status.
case test.settleFirst && test.settleLast:
finalStatus = StatusSucceeded
registerErr = ErrPaymentAlreadySucceeded
}

assertPaymentStatus(
Expand All @@ -931,7 +924,7 @@ func TestPaymentControlMultiShard(t *testing.T) {

// Finally assert we cannot register more attempts.
_, err = pControl.RegisterAttempt(info.PaymentIdentifier, &b)
require.Equal(t, registerErr, err)
require.ErrorIs(t, err, ErrRegisterAttempt)
}

for _, test := range tests {
Expand Down
10 changes: 10 additions & 0 deletions routing/payment_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,16 @@ lifecycle:
// We found a route to try, create a new HTLC attempt to try.
attempt, err := p.registerAttempt(rt, ps.RemainingAmt)
if err != nil {
// If the error is due to we cannot register another
Copy link
Collaborator

@ziggie1984 ziggie1984 Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should in neither case when receiving an error also in requestRoute just exist the payment without marking it as failed not collecting all results. So what we should do is, every error that occurs should fail the payment in the paymentDB and wait for all collectors to resolve.

// HTLC, we will skip this iteration and continue to
// the next one in case there are inflight HTLCs.
//
// TODO(yy): remove this check once we have a finer
// control over errors returned from the switch.
if errors.Is(err, channeldb.ErrRegisterAttempt) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this case, because when should this ever happen we now always have one single send and process result thread, so we would already know probably in the decideNextStep func ?

continue lifecycle
}

return exitWithErr(err)
}

Expand Down