multi: change RegisterAttempt error checking order

Move our more generic terminal check forward so that we only
need to handle a single class of expected errors. This change
is mirrored in our mock, and our reproducing tests are updated
to assert that this move catches both classes of errors we get.
This commit is contained in:
carla 2021-04-23 08:39:45 +02:00
parent 12136a97a9
commit 58d95be4dd
No known key found for this signature in database
GPG Key ID: 4CA7FE54A6213C91
4 changed files with 14 additions and 17 deletions

@ -290,18 +290,19 @@ func (p *PaymentControl) RegisterAttempt(paymentHash lntypes.Hash,
return err return err
} }
// Ensure the payment is in-flight.
if err := ensureInFlight(p); err != nil {
return err
}
// We cannot register a new attempt if the payment already has // We cannot register a new attempt if the payment already has
// reached a terminal condition: // reached a terminal condition. We check this before
// ensureInFlight because it is a more general check.
settle, fail := p.TerminalInfo() settle, fail := p.TerminalInfo()
if settle != nil || fail != nil { if settle != nil || fail != nil {
return ErrPaymentTerminal return ErrPaymentTerminal
} }
// Ensure the payment is in-flight.
if err := ensureInFlight(p); err != nil {
return err
}
// Make sure any existing shards match the new one with regards // Make sure any existing shards match the new one with regards
// to MPP options. // to MPP options.
mpp := attempt.Route.FinalHop().MPP mpp := attempt.Route.FinalHop().MPP

@ -1013,19 +1013,15 @@ func TestPaymentControlMultiShard(t *testing.T) {
// up in the Succeeded state. If both failed the payment should // up in the Succeeded state. If both failed the payment should
// also be Failed at this poinnt. // also be Failed at this poinnt.
finalStatus := StatusFailed finalStatus := StatusFailed
expRegErr := ErrPaymentAlreadyFailed
if test.settleFirst || test.settleLast { if test.settleFirst || test.settleLast {
finalStatus = StatusSucceeded finalStatus = StatusSucceeded
expRegErr = ErrPaymentAlreadySucceeded
} }
assertPaymentStatus(t, pControl, info.PaymentHash, finalStatus) assertPaymentStatus(t, pControl, info.PaymentHash, finalStatus)
// Finally assert we cannot register more attempts. // Finally assert we cannot register more attempts.
_, err = pControl.RegisterAttempt(info.PaymentHash, &b) _, err = pControl.RegisterAttempt(info.PaymentHash, &b)
if err != expRegErr { require.Equal(t, ErrPaymentTerminal, err)
t.Fatalf("expected error %v, got: %v", expRegErr, err)
}
} }
for _, test := range tests { for _, test := range tests {

@ -325,6 +325,10 @@ func (m *mockControlTower) RegisterAttempt(phash lntypes.Hash,
_, settled := m.successful[phash] _, settled := m.successful[phash]
_, failed := m.failed[phash] _, failed := m.failed[phash]
if settled || failed {
return channeldb.ErrPaymentTerminal
}
if settled && !inFlight { if settled && !inFlight {
return channeldb.ErrPaymentAlreadySucceeded return channeldb.ErrPaymentAlreadySucceeded
} }
@ -333,10 +337,6 @@ func (m *mockControlTower) RegisterAttempt(phash lntypes.Hash,
return channeldb.ErrPaymentAlreadyFailed return channeldb.ErrPaymentAlreadyFailed
} }
if settled || failed {
return channeldb.ErrPaymentTerminal
}
// Add attempt to payment. // Add attempt to payment.
p.attempts = append(p.attempts, channeldb.HTLCAttempt{ p.attempts = append(p.attempts, channeldb.HTLCAttempt{
HTLCAttemptInfo: *a, HTLCAttemptInfo: *a,

@ -619,7 +619,7 @@ func TestRouterPaymentStateMachine(t *testing.T) {
// A MP payment scenario when our path finding returns // A MP payment scenario when our path finding returns
// after we've just received a terminal failure, and // after we've just received a terminal failure, and
// demonstrates a bug where the payment will return with // demonstrates a bug where the payment will return with
// and "unexpected" ErrPaymentAlreadyFailed rather than // and "unexpected" ErrPaymentTerminal rather than
// failing with a permanent error. This results in the // failing with a permanent error. This results in the
// payment getting stuck. // payment getting stuck.
name: "MP path found after failure", name: "MP path found after failure",
@ -648,7 +648,7 @@ func TestRouterPaymentStateMachine(t *testing.T) {
routes: []*route.Route{ routes: []*route.Route{
shard, shard, shard, shard,
}, },
paymentErr: channeldb.ErrPaymentAlreadyFailed, paymentErr: channeldb.ErrPaymentTerminal,
}, },
{ {
// A MP payment scenario when our path finding returns // A MP payment scenario when our path finding returns