fix: block on payment hash when marking transaction as settled (#1878)

transactions/payments_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -191,6 +192,49 @@ func TestMarkSettled_Sent(t *testing.T) {
 	assert.Equal(t, &dbTransaction, settledTransaction)
 }
 
+func TestMarkSettled_Twice(t *testing.T) {
+	svc, err := tests.CreateTestService(t)
+	require.NoError(t, err)
+	defer svc.Remove()
+
+	dbTransaction := db.Transaction{
+		State:       constants.TRANSACTION_STATE_PENDING,
+		Type:        constants.TRANSACTION_TYPE_OUTGOING,
+		PaymentHash: tests.MockLNClientTransaction.PaymentHash,
+		AmountMsat:  123000,
+	}
+	svc.DB.Create(&dbTransaction)
+
+	mockEventConsumer := tests.NewMockEventConsumer()
+	svc.EventPublisher.RegisterSubscriber(mockEventConsumer)
+	transactionsService := NewTransactionsService(svc.DB, svc.EventPublisher)
+	var wg sync.WaitGroup
+	n := 10
+	wg.Add(n)
+	for range n {
+		go func() {
+			defer wg.Done()
+			err = svc.DB.Transaction(func(tx *gorm.DB) error {
+				time.Sleep(time.Duration(n) * 10 * time.Millisecond)
+				_, err = transactionsService.markTransactionSettled(tx, &dbTransaction, "test", 0, false)
+				time.Sleep(time.Duration(n) * 10 * time.Millisecond)
+				return err
+			})
+			require.NoError(t, err)
+		}()
+	}
+	wg.Wait()
+
+	// ensure we only mark transaction settled once and only fire
+	// settled notifications once
+	assert.NoError(t, err)
+	assert.Equal(t, constants.TRANSACTION_STATE_SETTLED, dbTransaction.State)
+	assert.Equal(t, 1, len(mockEventConsumer.GetConsumedEvents()))
+	assert.Equal(t, "nwc_payment_sent", mockEventConsumer.GetConsumedEvents()[0].Event)
+	settledTransaction := mockEventConsumer.GetConsumedEvents()[0].Properties.(*db.Transaction)
+	assert.Equal(t, &dbTransaction, settledTransaction)
+}
+
 func TestMarkSettled_Received(t *testing.T) {
 	svc, err := tests.CreateTestService(t)
 	require.NoError(t, err)

transactions/transactions_service.go

@@ -19,6 +19,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"gorm.io/datatypes"
 	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
 
 	"github.com/getAlby/hub/constants"
 	"github.com/getAlby/hub/db"
@@ -310,7 +311,7 @@ func (svc *transactionsService) SendPaymentSync(payReq string, amountMsat *uint6
 			selfPayment = true
 		}
 	}
-	
+
 	var dbTransaction db.Transaction
 
 	paymentAmount := uint64(paymentRequest.MSatoshi)
@@ -1337,7 +1338,19 @@ func (svc *transactionsService) SetTransactionMetadata(ctx context.Context, id u
 }
 
 func (svc *transactionsService) markTransactionSettled(tx *gorm.DB, dbTransaction *db.Transaction, preimage string, fee uint64, selfPayment bool) (*db.Transaction, error) {
-	// TODO: it would be better to have a database constraint so we cannot have two pending payments
+	if preimage == "" {
+		return nil, errors.New("no preimage in payment")
+	}
+
+	if tx.Dialector.Name() == "postgres" {
+		// lock based on payment hash to ensure we only mark one transaction as settled
+		// (in sqlite transactions are serializable by default)
+		transactionsWithPaymentHash := []db.Transaction{}
+		tx.Where(&db.Transaction{
+			PaymentHash: dbTransaction.PaymentHash,
+		}).Clauses(clause.Locking{Strength: "UPDATE"}).Find(&transactionsWithPaymentHash)
+	}
+
 	var existingSettledTransaction db.Transaction
 	if tx.Limit(1).Find(&existingSettledTransaction, &db.Transaction{
 		Type:        dbTransaction.Type,
@@ -1348,10 +1361,6 @@ func (svc *transactionsService) markTransactionSettled(tx *gorm.DB, dbTransactio
 		return &existingSettledTransaction, nil
 	}
 
-	if preimage == "" {
-		return nil, errors.New("no preimage in payment")
-	}
-
 	now := time.Now()
 	err := tx.Model(dbTransaction).Updates(map[string]interface{}{
 		"State":          constants.TRANSACTION_STATE_SETTLED,