htlcswitch/mailbox: fail on duplicate adds

htlcswitch/mailbox.go

@@ -12,9 +12,15 @@ import (
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
-// ErrMailBoxShuttingDown is returned when the mailbox is interrupted by a
-// shutdown request.
-var ErrMailBoxShuttingDown = errors.New("mailbox is shutting down")
+var (
+	// ErrMailBoxShuttingDown is returned when the mailbox is interrupted by
+	// a shutdown request.
+	ErrMailBoxShuttingDown = errors.New("mailbox is shutting down")
+
+	// ErrPacketAlreadyExists signals that an attempt to add a packet failed
+	// because it already exists in the mailbox.
+	ErrPacketAlreadyExists = errors.New("mailbox already has packet")
+)
 
 // MailBox is an interface which represents a concurrent-safe, in-order
 // delivery queue for messages from the network and also from the main switch.
@@ -564,7 +570,7 @@ func (m *memoryMailBox) AddPacket(pkt *htlcPacket) error {
 	case *lnwire.UpdateFulfillHTLC, *lnwire.UpdateFailHTLC:
 		if _, ok := m.pktIndex[pkt.inKey()]; ok {
 			m.pktCond.L.Unlock()
-			return nil
+			return ErrPacketAlreadyExists
 		}
 
 		entry := m.htlcPkts.PushBack(pkt)
@@ -577,7 +583,7 @@ func (m *memoryMailBox) AddPacket(pkt *htlcPacket) error {
 	case *lnwire.UpdateAddHTLC:
 		if _, ok := m.addIndex[pkt.inKey()]; ok {
 			m.pktCond.L.Unlock()
-			return nil
+			return ErrPacketAlreadyExists
 		}
 
 		entry := m.addPkts.PushBack(&pktWithExpiry{

htlcswitch/mailbox_test.go

@@ -44,7 +44,10 @@ func TestMailBoxCouriers(t *testing.T) {
 		}
 		sentPackets[i] = pkt
 
-		mailBox.AddPacket(pkt)
+		err := mailBox.AddPacket(pkt)
+		if err != nil {
+			t.Fatalf("unable to add packet: %v", err)
+		}
 	}
 
 	// Next, we'll do the same, but this time adding wire messages.
@@ -495,6 +498,47 @@ func TestMailBoxAddExpiry(t *testing.T) {
 	ctx.checkFails(secondBatch)
 }
 
+// TestMailBoxDuplicateAddPacket asserts that the mailbox returns an
+// ErrPacketAlreadyExists failure when two htlcPackets are added with identical
+// incoming circuit keys.
+func TestMailBoxDuplicateAddPacket(t *testing.T) {
+	t.Parallel()
+
+	mailBox := newMemoryMailBox(&mailBoxConfig{
+		clock: clock.NewDefaultClock(),
+	})
+	mailBox.Start()
+	defer mailBox.Stop()
+
+	addTwice := func(t *testing.T, pkt *htlcPacket) {
+		// The first add should succeed.
+		err := mailBox.AddPacket(pkt)
+		if err != nil {
+			t.Fatalf("unable to add packet: %v", err)
+		}
+
+		// Adding again with the same incoming circuit key should fail.
+		err = mailBox.AddPacket(pkt)
+		if err != ErrPacketAlreadyExists {
+			t.Fatalf("expected ErrPacketAlreadyExists, got: %v", err)
+		}
+	}
+
+	// Assert duplicate AddPacket calls fail for all types of HTLCs.
+	addTwice(t, &htlcPacket{
+		incomingHTLCID: 0,
+		htlc:           &lnwire.UpdateAddHTLC{},
+	})
+	addTwice(t, &htlcPacket{
+		incomingHTLCID: 1,
+		htlc:           &lnwire.UpdateFulfillHTLC{},
+	})
+	addTwice(t, &htlcPacket{
+		incomingHTLCID: 2,
+		htlc:           &lnwire.UpdateFailHTLC{},
+	})
+}
+
 // TestMailOrchestrator asserts that the orchestrator properly buffers packets
 // for channels that haven't been made live, such that they are delivered
 // immediately after BindLiveShortChanID. It also tests that packets are delivered