staticaddr/deposit: canonicalize multi-deposit locks

staticaddr/deposit/manager.go

@@ -61,7 +61,8 @@ type ManagerConfig struct {
 //
 // Lock order: if both Manager.mu and a Deposit lock are needed, acquire
 // Manager.mu before Deposit.Lock. Never acquire Manager.mu while holding a
-// Deposit lock.
+// Deposit lock. Multiple deposits must be locked with lockDeposits, which
+// canonicalizes lock order by outpoint.
 type Manager struct {
 	cfg *ManagerConfig
 
@@ -433,8 +434,8 @@ func (m *Manager) GetActiveDepositsInState(stateFilter fsm.StateType) (
 		deposits = append(deposits, fsm.deposit)
 	}
 
-	lockDeposits(deposits)
-	defer unlockDeposits(deposits)
+	lockedDeposits := lockDeposits(deposits)
+	defer unlockDeposits(lockedDeposits)
 
 	filteredDeposits := make([]*Deposit, 0, len(deposits))
 	for _, d := range deposits {
@@ -478,8 +479,8 @@ func (m *Manager) AllOutpointsActiveDeposits(outpoints []wire.OutPoint,
 		return deposits, true
 	}
 
-	lockDeposits(deposits)
-	defer unlockDeposits(deposits)
+	lockedDeposits := lockDeposits(deposits)
+	defer unlockDeposits(lockedDeposits)
 	for _, d := range deposits {
 		if !d.isInStateNoLock(targetState) {
 			return nil, false
@@ -538,8 +539,8 @@ func (m *Manager) TransitionDeposits(ctx context.Context, deposits []*Deposit,
 		return fmt.Errorf("deposits not found in active deposits")
 	}
 
-	lockDeposits(deposits)
-	defer unlockDeposits(deposits)
+	lockedDeposits := lockDeposits(deposits)
+	defer unlockDeposits(lockedDeposits)
 	for _, deposit := range deposits {
 		if deposit.isInFinalStateNoLock() {
 			return fmt.Errorf("deposit %v is no longer active in "+
@@ -565,14 +566,26 @@ func (m *Manager) TransitionDeposits(ctx context.Context, deposits []*Deposit,
 	return nil
 }
 
-func lockDeposits(deposits []*Deposit) {
-	for _, d := range deposits {
+// lockDeposits locks deposits in canonical outpoint order and returns the
+// ordered slice that must be passed to unlockDeposits.
+func lockDeposits(deposits []*Deposit) []*Deposit {
+	lockedDeposits := append([]*Deposit(nil), deposits...)
+	sort.Slice(lockedDeposits, func(i, j int) bool {
+		return lockedDeposits[i].OutPoint.String() <
+			lockedDeposits[j].OutPoint.String()
+	})
+
+	for _, d := range lockedDeposits {
 		d.Lock()
 	}
+
+	return lockedDeposits
 }
 
+// unlockDeposits unlocks deposits in reverse lock order.
 func unlockDeposits(deposits []*Deposit) {
-	for _, d := range deposits {
+	for i := len(deposits) - 1; i >= 0; i-- {
+		d := deposits[i]
 		d.Unlock()
 	}
 }

staticaddr/deposit/manager_reconcile_test.go

@@ -2,6 +2,7 @@ package deposit
 
 import (
 	"testing"
+	"time"
 
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/wire"
@@ -56,3 +57,72 @@ func TestTransitionDepositsRejectsDuplicateOutpoints(t *testing.T) {
 	require.ErrorContains(t, err, "duplicate deposit outpoint")
 	require.Equal(t, Deposited, deposit.GetState())
 }
+
+// TestLockDepositsCanonicalizesOutpoints verifies that lockDeposits takes a
+// canonical copy of the caller's slice so overlapping multi-deposit operations
+// cannot lock deposits in conflicting request orders.
+func TestLockDepositsCanonicalizesOutpoints(t *testing.T) {
+	depositA := &Deposit{
+		OutPoint: wire.OutPoint{
+			Hash:  chainhash.Hash{1},
+			Index: 0,
+		},
+	}
+	depositB := &Deposit{
+		OutPoint: wire.OutPoint{
+			Hash:  chainhash.Hash{2},
+			Index: 0,
+		},
+	}
+
+	deposits := []*Deposit{depositB, depositA}
+	lockedDeposits := lockDeposits(deposits)
+	defer unlockDeposits(lockedDeposits)
+
+	require.Equal(t, []*Deposit{depositA, depositB}, lockedDeposits)
+	require.Equal(t, []*Deposit{depositB, depositA}, deposits)
+}
+
+// TestLockDepositsAllowsReversedConcurrentRequests exercises the reviewer
+// case where overlapping callers request the same deposits in opposite orders.
+func TestLockDepositsAllowsReversedConcurrentRequests(t *testing.T) {
+	depositA := &Deposit{
+		OutPoint: wire.OutPoint{
+			Hash:  chainhash.Hash{3},
+			Index: 0,
+		},
+	}
+	depositB := &Deposit{
+		OutPoint: wire.OutPoint{
+			Hash:  chainhash.Hash{4},
+			Index: 0,
+		},
+	}
+
+	start := make(chan struct{})
+	done := make(chan struct{}, 2)
+	lockAndUnlock := func(deposits []*Deposit) {
+		<-start
+
+		for range 100 {
+			lockedDeposits := lockDeposits(deposits)
+			unlockDeposits(lockedDeposits)
+		}
+
+		done <- struct{}{}
+	}
+
+	go lockAndUnlock([]*Deposit{depositA, depositB})
+	go lockAndUnlock([]*Deposit{depositB, depositA})
+
+	close(start)
+
+	for range 2 {
+		select {
+		case <-done:
+
+		case <-time.After(time.Second):
+			t.Fatal("reversed deposit lock requests deadlocked")
+		}
+	}
+}