node: Fix bug in chainIds allocation

- This resolves a mistake with allocating the chainIds in the governor
initialization that causes nil entries in the slice.
- Add unit tests to ensure that the chainIds slice matches the chains
  map
- Add unit test to ensure that TrimAndSumValueForChain checks for a nil
  pointer to avoid panics

node/pkg/governor/governor.go

@@ -28,6 +28,7 @@ package governor
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"math"
 	"math/big"
@@ -390,8 +391,11 @@ func (gov *ChainGovernor) initConfig() error {
 	// Populate a sorted list of chain IDs so that we can iterate over maps in a determinstic way.
 	// https://go.dev/blog/maps, "Iteration order" section
 	governedChainIds := make([]vaa.ChainID, len(gov.chains))
+	i := 0
 	for id := range gov.chains {
-		governedChainIds = append(governedChainIds, id)
+		// updating the slice in place here to satisfy prealloc lint. In theory this should be more performant
+		governedChainIds[i] = id
+		i++
 	}
 	// Custom sorting for the vaa.ChainID type
 	sort.Slice(governedChainIds, func(i, j int) bool {
@@ -709,7 +713,11 @@ func (gov *ChainGovernor) CheckPendingForTime(now time.Time) ([]*common.MessageP
 
 	// Iterate deterministically by accessing keys from this slice instead of the chainEntry map directly
 	for _, chainId := range gov.chainIds {
-		ce := gov.chains[chainId]
+		ce, ok := gov.chains[chainId]
+		if !ok {
+			gov.logger.Error("chainId not found in gov.chains", zap.Stringer("chainId", chainId))
+
+		}
 		// Keep going as long as we find something that will fit.
 		for {
 			foundOne := false
@@ -919,6 +927,10 @@ func computeValue(amount *big.Int, token *tokenEntry) (uint64, error) {
 // queued until space opens up.
 // SECURITY Invariant: The `sum` return value should never be less than 0
 func (gov *ChainGovernor) TrimAndSumValueForChain(chainEntry *chainEntry, startTime time.Time) (sum uint64, err error) {
+	if chainEntry == nil {
+		// We don't expect this to happen but this prevents a nil pointer deference
+		return 0, errors.New("TrimAndSumValeForChain parameter chainEntry must not be nil")
+	}
 	// Sum the value of all transfers for this chain. This sum can be negative if flow-cancelling is enabled
 	// and the incoming value of flow-cancelling assets exceeds the summed value of all outgoing assets.
 	var sumValue int64

node/pkg/governor/governor_test.go

@@ -185,6 +185,21 @@ func TestTrimEmptyTransfers(t *testing.T) {
 	assert.Equal(t, 0, len(updatedTransfers))
 }
 
+// Make sure that the code doesn't panic if called with a nil chainEntry
+func TestTrimAndSumValueForChainReturnsErrorForNilChainEntry(t *testing.T) {
+	ctx := context.Background()
+	gov, err := newChainGovernorForTest(ctx)
+	require.NoError(t, err)
+	assert.NotNil(t, gov)
+
+	now, err := time.Parse("Jan 2, 2006 at 3:04pm (MST)", "Jun 1, 2022 at 12:00pm (CST)")
+	require.NoError(t, err)
+
+	sum, err := gov.TrimAndSumValueForChain(nil, now)
+	require.Error(t, err)
+	assert.Equal(t, uint64(0), sum)
+}
+
 func TestSumAllFromToday(t *testing.T) {
 	ctx := context.Background()
 	gov, err := newChainGovernorForTest(ctx)
@@ -1432,7 +1447,6 @@ func TestTransfersUpToAndOverTheLimit(t *testing.T) {
 func TestPendingTransferBeingReleased(t *testing.T) {
 	ctx := context.Background()
 	gov, err := newChainGovernorForTest(ctx)
-
 	require.NoError(t, err)
 	assert.NotNil(t, gov)
 
@@ -1577,7 +1591,9 @@ func TestPendingTransferBeingReleased(t *testing.T) {
 	assert.Equal(t, 4, len(gov.msgsSeen))
 
 	// If we check pending before noon, nothing should happen.
-	now, _ = time.Parse("Jan 2, 2006 at 3:04pm (MST)", "Jun 2, 2022 at 9:00am (CST)")
+	now, err = time.Parse("Jan 2, 2006 at 3:04pm (MST)", "Jun 2, 2022 at 9:00am (CST)")
+	require.NoError(t, err)
+	assert.NotNil(t, now)
 	toBePublished, err := gov.CheckPendingForTime(now)
 	require.NoError(t, err)
 	assert.Equal(t, 0, len(toBePublished))
@@ -1605,6 +1621,29 @@ func TestPendingTransferBeingReleased(t *testing.T) {
 	assert.Equal(t, 3, len(gov.msgsSeen))
 }
 
+func TestPopulateChainIds(t *testing.T) {
+	ctx := context.Background()
+	gov, err := newChainGovernorForTest(ctx)
+	require.NoError(t, err)
+	assert.NotNil(t, gov)
+	// Sanity check
+	assert.NotZero(t, len(gov.chainIds))
+
+	// Ensure that the chainIds slice match the entries in the chains map
+	assert.Equal(t, len(gov.chains), len(gov.chainIds))
+	lowest := 0
+	for _, chainId := range gov.chainIds {
+		chainEntry, ok := gov.chains[chainId]
+		assert.NotNil(t, chainEntry)
+		assert.True(t, ok)
+		assert.Equal(t, chainEntry.emitterChainId, chainId)
+		// Check that the chainIds are in ascending order. The point of this slice is that it provides
+		// deterministic ordering over chainIds.
+		assert.Greater(t, int(chainId), lowest)
+		lowest = int(chainId)
+	}
+}
+
 // Test that, when a small transfer (under the 'big tx limit') of a flow-cancelling asset is queued and
 // later released, it causes a reduction in the Governor usage for the destination chain.
 func TestPendingTransferFlowCancelsWhenReleased(t *testing.T) {