Merge pull request #3181 from joostjager/check-htlc-fix

invoices: check htlc amount also for accepted and settled invoices

channeldb/invoice_test.go

@@ -662,5 +662,9 @@ func TestQueryInvoices(t *testing.T) {
 }
 
 func checkHtlcParameters(invoice *Invoice) error {
+	if invoice.Terms.State == ContractSettled {
+		return ErrInvoiceAlreadySettled
+	}
+
 	return nil
 }

channeldb/invoices.go

@@ -1003,17 +1003,6 @@ func acceptOrSettleInvoice(invoices, settleIndex *bbolt.Bucket,
 		return nil, err
 	}
 
-	state := invoice.Terms.State
-
-	switch {
-	case state == ContractAccepted:
-		return &invoice, ErrInvoiceAlreadyAccepted
-	case state == ContractSettled:
-		return &invoice, ErrInvoiceAlreadySettled
-	case state == ContractCanceled:
-		return &invoice, ErrInvoiceAlreadyCanceled
-	}
-
 	// If the invoice is still open, check the htlc parameters.
 	if err := checkHtlcParameters(&invoice); err != nil {
 		return &invoice, err

invoices/invoiceregistry.go

@@ -526,6 +526,30 @@ func (i *InvoiceRegistry) LookupInvoice(rHash lntypes.Hash) (channeldb.Invoice,
 func (i *InvoiceRegistry) checkHtlcParameters(invoice *channeldb.Invoice,
 	amtPaid lnwire.MilliSatoshi, htlcExpiry uint32, currentHeight int32) error {
 
+	// If the invoice is already canceled, there is no further checking to
+	// do.
+	if invoice.Terms.State == channeldb.ContractCanceled {
+		return channeldb.ErrInvoiceAlreadyCanceled
+	}
+
+	// If a payment has already been made, we only accept more payments if
+	// the amount is the exact same. This prevents probing with small
+	// amounts on settled invoices to find out the receiver node.
+	if invoice.AmtPaid != 0 && amtPaid != invoice.AmtPaid {
+		return ErrInvoiceAmountTooLow
+	}
+
+	// Return early in case the invoice was already accepted or settled. We
+	// don't want to check the expiry again, because it may be that we are
+	// just restarting.
+	switch invoice.Terms.State {
+	case channeldb.ContractAccepted:
+		return channeldb.ErrInvoiceAlreadyAccepted
+	case channeldb.ContractSettled:
+		return channeldb.ErrInvoiceAlreadySettled
+	}
+
+	// The invoice is still open. Check the expiry.
 	expiry, err := i.decodeFinalCltvExpiry(string(invoice.PaymentRequest))
 	if err != nil {
 		return err
@@ -539,10 +563,7 @@ func (i *InvoiceRegistry) checkHtlcParameters(invoice *channeldb.Invoice,
 		return ErrInvoiceExpiryTooSoon
 	}
 
-	// If an invoice amount is specified, check that enough is paid. This
+	// If an invoice amount is specified, check that enough is paid.
-	// check is only performed for open invoices. Once a sufficiently large
-	// payment has been made and the invoice is in the accepted or settled
-	// state, any amount will be accepted on top of that.
 	if invoice.Terms.Value > 0 && amtPaid < invoice.Terms.Value {
 		return ErrInvoiceAmountTooLow
 	}

invoices/invoiceregistry_test.go

@@ -148,21 +148,43 @@ func TestSettleInvoice(t *testing.T) {
 		t.Fatal("no update received")
 	}
 
-	// Try to settle again.
+	// Try to settle again. We need this idempotent behaviour after a
-	_, err = registry.NotifyExitHopHtlc(
+	// restart.
+	event, err := registry.NotifyExitHopHtlc(
 		hash, amtPaid, testInvoiceExpiry, testCurrentHeight, hodlChan,
 	)
 	if err != nil {
-		t.Fatal("expected duplicate settle to succeed")
+		t.Fatalf("unexpected NotifyExitHopHtlc error: %v", err)
+	}
+	if event.Preimage == nil {
+		t.Fatal("expected settle event")
 	}
 
-	// Try to settle again with a different amount.
+	// Try to settle again with a higher amount. This should result in a
-	_, err = registry.NotifyExitHopHtlc(
+	// cancel event because after a restart the amount should still be the
+	// same. New HTLCs with a different amount should be rejected.
+	event, err = registry.NotifyExitHopHtlc(
 		hash, amtPaid+600, testInvoiceExpiry, testCurrentHeight,
 		hodlChan,
 	)
 	if err != nil {
-		t.Fatal("expected duplicate settle to succeed")
+		t.Fatalf("unexpected NotifyExitHopHtlc error: %v", err)
+	}
+	if event.Preimage != nil {
+		t.Fatal("expected cancel event")
+	}
+
+	// Try to settle again with a lower amount. This should show the same
+	// behaviour as settling with a higher amount.
+	event, err = registry.NotifyExitHopHtlc(
+		hash, amtPaid-600, testInvoiceExpiry, testCurrentHeight,
+		hodlChan,
+	)
+	if err != nil {
+		t.Fatalf("unexpected NotifyExitHopHtlc error: %v", err)
+	}
+	if event.Preimage != nil {
+		t.Fatal("expected cancel event")
 	}
 
 	// Check that settled amount remains unchanged.