diff --git a/node/pkg/governor/governor.go b/node/pkg/governor/governor.go index eff53f295d..81e90acd69 100644 --- a/node/pkg/governor/governor.go +++ b/node/pkg/governor/governor.go @@ -28,6 +28,7 @@ package governor import ( "context" "encoding/hex" + "errors" "fmt" "math" "math/big" @@ -389,9 +390,12 @@ 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)) + governedChainIds := make([]vaa.ChainID, len(gov.chains), 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 diff --git a/node/pkg/governor/governor_test.go b/node/pkg/governor/governor_test.go index 1b5eec25eb..f1d6748dc8 100644 --- a/node/pkg/governor/governor_test.go +++ b/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) {