fix getUniqueClientId to actually return unique ID (#1127)

fix getUniqueClientId to actually return unique ID

node/pkg/reporter/attestation_events.go

@@ -11,6 +11,8 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 )
 
+const maxClientId = 1e6
+
 type (
 	// MessagePublication is a VAA along with a transaction identifer from the EmiterChain
 	MessagePublication struct {
@@ -47,10 +49,10 @@ func EventListener(logger *zap.Logger) *AttestationEventReporter {
 
 // getUniqueClientId loops to generate & test integers for existence as key of map. returns an int that is not a key in map.
 func (re *AttestationEventReporter) getUniqueClientId() int {
-	clientId := rand.Intn(1e6)
-	found := false
+	clientId := 0
+	found := true
 	for found {
-		clientId = rand.Intn(1e6)
+		clientId = rand.Intn(maxClientId) //#nosec G404 The clientIds don't need to be unpredictable. They just need to be unique.
 		_, found = re.subs[clientId]
 	}
 	return clientId

node/pkg/reporter/attestation_events_test.go

@@ -0,0 +1,36 @@
+package reporter
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetUniqueClientId(t *testing.T) {
+	/*
+		Rationale:
+		Pro: This test does not have false positives. It is guaranteed to fail if the magic value for the maximum client ID changes.
+		Con: It takes ca. 0.463s to run this test
+	*/
+
+	var almostFullMap = make(map[int]*lifecycleEventChannels, maxClientId)
+
+	firstExpectedValue := 0
+	secondExpectedValue := 1
+
+	// build a full map
+	for i := 0; i < maxClientId; i++ {
+		almostFullMap[i] = nil
+	}
+
+	// Test that we can find the empty slot in the map
+	delete(almostFullMap, firstExpectedValue)
+	re := AttestationEventReporter{sync.RWMutex{}, nil, almostFullMap}
+	assert.Equal(t, re.getUniqueClientId(), firstExpectedValue)
+
+	// Test that we can find a different empty slot in the map
+	almostFullMap[firstExpectedValue] = nil
+	delete(almostFullMap, secondExpectedValue)
+	assert.Equal(t, re.getUniqueClientId(), secondExpectedValue)
+}