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.

Sign up for GitHub

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

[MPP] "slow settle" for TrackPayment #4142

Closed
wants to merge 2 commits into from
Closed

[MPP] "slow settle" for TrackPayment #4142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00e9db5
channeldb: return updated payment from FailAttempt
halseth Apr 2, 2020
9a69f1f
routing/control_tower: "slow settle" payment tracking
halseth Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions channeldb/payment_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,16 +213,15 @@ func (p *PaymentControl) SettleAttempt(hash lntypes.Hash,

// FailAttempt marks the given payment attempt failed.
func (p *PaymentControl) FailAttempt(hash lntypes.Hash,
attemptID uint64, failInfo *HTLCFailInfo) error {
attemptID uint64, failInfo *HTLCFailInfo) (*MPPayment, error) {

var b bytes.Buffer
if err := serializeHTLCFailInfo(&b, failInfo); err != nil {
return err
return nil, err
}
failBytes := b.Bytes()

_, err := p.updateHtlcKey(hash, attemptID, htlcFailInfoKey, failBytes)
return err
return p.updateHtlcKey(hash, attemptID, htlcFailInfoKey, failBytes)
}

// updateHtlcKey updates a database key for the specified htlc.
Expand Down
4 changes: 2 additions & 2 deletions channeldb/payment_control_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func TestPaymentControlSwitchFail(t *testing.T) {
t.Fatalf("unable to register attempt: %v", err)
}

err = pControl.FailAttempt(
_, err = pControl.FailAttempt(
info.PaymentHash, 2, &HTLCFailInfo{
Reason: HTLCFailUnreadable,
},
Expand Down Expand Up @@ -362,7 +362,7 @@ func TestPaymentControlDeleteNonInFligt(t *testing.T) {

if p.failed {
// Fail the payment attempt.
err := pControl.FailAttempt(
_, err := pControl.FailAttempt(
info.PaymentHash, attempt.AttemptID,
&HTLCFailInfo{
Reason: HTLCFailUnreadable,
Expand Down
58 changes: 42 additions & 16 deletions routing/control_tower.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,27 @@ func (p *controlTower) SettleAttempt(paymentHash lntypes.Hash,
return err
}

// Notify subscribers of success event.
p.notifyFinalEvent(
paymentHash, createSuccessResult(payment.HTLCs),
)

// If settling this attempt took the payment to a final state, we
// should notify all subscribers of success event.
p.notifyFinalEvent(payment)
return nil
}

// FailAttempt marks the given payment attempt failed.
func (p *controlTower) FailAttempt(paymentHash lntypes.Hash,
attemptID uint64, failInfo *channeldb.HTLCFailInfo) error {

return p.db.FailAttempt(paymentHash, attemptID, failInfo)
payment, err := p.db.FailAttempt(paymentHash, attemptID, failInfo)
if err != nil {
return err
}

// If failing this attempt took the payment to a final state, we
// should notify all subscribers of the final payment status. This can
// happen if the payment already had a settled attempt or terminal
// failure recorded, and this attempt was the last one in-flight.
p.notifyFinalEvent(payment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this causing a duplicate event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idea is that only the first event that take the payment to Settle/failed state will produce the notification. The second one will find the payment also in this state, but by that time the subscribers list will be empty (and cannot be filled again since we only add to it for in-flight payments)

Copy link
Contributor

Choose a reason for hiding this comment

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

And there is no race condition possible there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the first one that gets the lock on the subscribers will empty the list, and at that point it is impossible to add more to the list, so I think it is okay.

If we merge #3970 first I can add some more testcases here for MPP scenarios.

Copy link
Contributor

@joostjager joostjager Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was actually thinking about a race between subscribe and notify, because the payment update in FailAttempt isn't inside the subscriber lock.

I think the bottom line of this discussion is that the concurrent behavior isn't very clear. There is potentially a duplicate notification, but it is never broadcast because the subscriber lock makes sure the list is already empty.

Also control tower notification needs to be updated anyway to sends updates for non-final state transitions.

Isn't a single PR on top of #3970 that fixes both things a better idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

in order to be totally consistent i think, yes, there would need to be a lock around the db operation as well. this is preexisting tho, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the direction that I am thinking of atm: master...joostjager:notify-htlc-arrival-2

return nil
}

// createSuccessResult creates a success result to send to subscribers.
Expand Down Expand Up @@ -173,13 +181,12 @@ func (p *controlTower) Fail(paymentHash lntypes.Hash,
return err
}

// Notify subscribers of fail event.
p.notifyFinalEvent(
paymentHash, createFailedResult(
payment.HTLCs, reason,
),
)

// If recording the failure reason took the payment to a final state,
// we should notify all subscribers of the final payment status. This
// can happen if the payment had no more attempts in flight at this
// point, and recording this payment failure took the payment to a
// final failure state.
p.notifyFinalEvent(payment)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't notify here, because there are still htlcs in-flight, is this Fail method guarantueed to be called again? (does every failed htlc eventually trigger a call to this method?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. That's why we also call it in FailAttemp above

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, clear.

return nil
}

Expand Down Expand Up @@ -249,9 +256,28 @@ func (p *controlTower) SubscribePayment(paymentHash lntypes.Hash) (
}

// notifyFinalEvent sends a final payment event to all subscribers of this
// payment. The channel will be closed after this.
func (p *controlTower) notifyFinalEvent(paymentHash lntypes.Hash,
event *PaymentResult) {
// payment if the payment is found in a state. The channel will be closed after
// this.
func (p *controlTower) notifyFinalEvent(payment *channeldb.MPPayment) {
var event *PaymentResult
switch payment.Status {

// Notify subscribers of success event.
case channeldb.StatusSucceeded:
event = createSuccessResult(payment.HTLCs)

// Notify subscribers of fail event.
case channeldb.StatusFailed:
event = createFailedResult(
payment.HTLCs, *payment.FailureReason,
)

// Non-final status.
default:
return
}

paymentHash := payment.Info.PaymentHash

// Get all subscribers for this hash. As there is only a single outcome,
// the subscriber list can be cleared.
Expand Down