From c19b1011ee77548a26d984b826d4b79b00f488d7 Mon Sep 17 00:00:00 2001
From: Sarah Jamie Lewis <sarah@openprivacy.ca>
Date: Wed, 8 Sep 2021 11:53:39 -0700
Subject: [PATCH] Actively Deduplicate Connections on WaitForCapabilityOrClose

A very rare bug happens when 2 contacts peer with each other at the same
time. This results in duplicate higher level constructs like PeerApp
which can make tracking state-related bugs difficult, especially
in integration tests.

This commit fixes an existing bug in WaitForCapabilityOrClose
which hid the existence of a duplicate connections from clients
(and replaces it with active deduping)
---
 networks/tor/BaseOnionService.go | 33 +++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go
index f15b3e2..9361cb8 100644
--- a/networks/tor/BaseOnionService.go
+++ b/networks/tor/BaseOnionService.go
@@ -10,7 +10,6 @@ import (
 	"git.openprivacy.ca/openprivacy/log"
 	"golang.org/x/crypto/ed25519"
 	"sync"
-	"time"
 )
 
 // BaseOnionService is a concrete implementation of the service interface over Tor onion services.
@@ -66,16 +65,36 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab
 	conn, err := s.GetConnection(cid)
 	// If there is at least one active connection, then check the one that is returned and use it
 	for conn != nil {
-		if conn.HasCapability(name) {
+
+		// if we have only one and it has the right capability then return
+		if conn.HasCapability(name) && err == nil {
 			return conn, nil
 		}
-		time.Sleep(time.Millisecond * 200)
-		// If we received both a Connection and an Error from GetConnection that means we have multiple connections
-		// If that was the case we refresh from the connection store as the likely case is one of them has been
-		// closed and the other one has the capabilities we need.
+
+		// If We have 2 connections for the same hostname...
 		if err != nil {
-			conn, err = s.GetConnection(cid)
+			log.Debugf("found duplicate connections for %v <-> %v", s.id.Hostname(), cid)
+			// By convention the lowest lexicographical hostname purges all their outbounds to the higher
+			// hostname
+			// Which should only leave a single connection remaining (as we dedupe on connect too)
+			// This does allow people to attempt to guarantee being the outbound connection but due to the bidirectional
+			// authentication this should never result in an advantage in the protocol.
+			if s.id.Hostname() < cid {
+				// Close all output connections to connection
+				s.connections.Range(func(key, value interface{}) bool {
+					connection := value.(tapir.Connection)
+					if connection.Hostname() == cid {
+						if !connection.IsClosed() && connection.IsOutbound() {
+							connection.Close()
+						}
+					}
+					return true
+				})
+			}
 		}
+
+		// We had a connection but it didn't have a cpability...try again
+		conn, err = s.GetConnection(cid)
 	}
 	// There are no active connections with that hostname
 	return nil, err