storage/store/events.go (1)

212-218: ServiceEvents.ByBlockID has the same mutation vulnerability that was just fixed for Events.

val is returned directly, so any caller that sorts or overwrites Payload bytes on the returned slice will corrupt the cached entry for every concurrent/future reader — identical to the Events bug described in the PR.

♻️ Suggested fix (mirrors the `Events.ByBlockID` change)

 func (e *ServiceEvents) ByBlockID(blockID flow.Identifier) ([]flow.Event, error) {
 	val, err := e.cache.Get(e.db.Reader(), blockID)
 	if err != nil {
 		return nil, err
 	}
-	return val, nil
+	result := make([]flow.Event, len(val))
+	copy(result, val)
+	for i, event := range result {
+		payload := make([]byte, len(event.Payload))
+		copy(payload, event.Payload)
+		result[i].Payload = payload
+	}
+	return result, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events.go` around lines 212 - 218, ServiceEvents.ByBlockID
currently returns the cached slice `val` directly from `e.cache.Get`, which lets
callers mutate event entries (e.g., sort or modify Event.Payload) and corrupt
the cache; update `ServiceEvents.ByBlockID` to create and return a deep copy of
the events: allocate a new slice for the events and for each `flow.Event` copy
the struct and clone the `Payload` byte slice before returning, mirroring the
same defensive copy logic used in `Events.ByBlockID` so cached data from
`e.cache.Get` remains immutable for other callers.

storage/store/events_test.go (1)

259-262: Silent error drop in goroutine could hide test-setup failures.

if err != nil { return } silently swallows any ByBlockID error, so a broken store setup would make the race test pass vacuously. A common pattern that is safe from goroutines is to send errors to a buffered channel and drain it after wg.Wait().

♻️ Suggested improvement

+		errs := make(chan error, 10)
 		for range 10 {
 			wg.Add(1)
 			go func() {
 				defer wg.Done()
 				got, err := eventsStore.ByBlockID(blockID)
 				if err != nil {
-					return
+					errs <- err
+					return
 				}
 				sort.Slice(got, func(i, j int) bool {
 					if got[i].TransactionIndex == got[j].TransactionIndex {
 						return got[i].EventIndex < got[j].EventIndex
 					}
 					return got[i].TransactionIndex < got[j].TransactionIndex
 				})
 			}()
 		}
 		wg.Wait()
+		close(errs)
+		for err := range errs {
+			require.NoError(t, err)
+		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events_test.go` around lines 259 - 262, The goroutine currently
swallows ByBlockID errors with "if err != nil { return }", which can hide setup
failures; change the goroutine to send any error from eventsStore.ByBlockID (and
any other internal failures) into a buffered channel (e.g., errCh := make(chan
error, N)) instead of returning, and after wg.Wait() drain the channel and fail
the test on the first non-nil error (t.Fatalf or require.NoError) so test
failures in ByBlockID(blockID) are surfaced; reference the eventsStore.ByBlockID
call, the blockID variable, the goroutine that invokes it, and vg/wg.Wait() when
making the change.

storage/store/events.go (1)

63-81: BatchStore caches events without sorting, diverging from the retrieve path

retrieve sorts loaded events by (TransactionIndex, EventIndex) before they enter the cache, but BatchStore inserts combinedEvents directly without sorting. If a caller ever passes blockEvents in non-execution order, the cache hit path returns a different ordering than a subsequent DB-reload (cache miss) path. The invariant is implicit and undocumented.

Applying the same sort before cache.Insert makes both paths consistent regardless of input order.

♻️ Proposed defensive fix

+	sort.Slice(combinedEvents, func(i, j int) bool {
+		if combinedEvents[i].TransactionIndex == combinedEvents[j].TransactionIndex {
+			return combinedEvents[i].EventIndex < combinedEvents[j].EventIndex
+		}
+		return combinedEvents[i].TransactionIndex < combinedEvents[j].TransactionIndex
+	})
+
	storage.OnCommitSucceed(batch, func() {
		e.cache.Insert(blockID, combinedEvents)
	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events.go` around lines 63 - 81, The BatchStore path builds
combinedEvents from blockEvents and inserts them into the cache without sorting,
causing a cache/DB ordering mismatch with the retrieve path; before calling
e.cache.Insert(blockID, combinedEvents) sort combinedEvents the same way
retrieve does (by Event.TransactionIndex then Event.EventIndex) — e.g., after
building combinedEvents and before storage.OnCommitSucceed's callback, perform a
stable sort on combinedEvents comparing TransactionIndex then EventIndex so
cache.Insert and retrieve produce the same ordering regardless of blockEvents
input order.

engine/access/state_stream/backend/backend_events_test.go (1)

105-118: Ordering assumption between setupLocalStorage and subscribe's expectedEvents is implicitly coupled

The sort comparator change is correct — (TransactionIndex, EventIndex) now mirrors the storage layer's documented execution order.

One fragility worth noting: subscribe (line 378) builds expectedEvents by iterating s.blockEvents[b.ID()] in its original (unsorted) order, and requireEventsResponse compares via assert.Equal (order-sensitive). When setupLocalStorage() is in effect, the mock returns execution-sorted events. The test only passes if test fixtures happen to already produce events in (TransactionIndex, EventIndex) order — a silent, implicit assumption.

Consider making this explicit in the setupLocalStorage path:

♻️ Suggested fix to make ordering explicit in the expected events construction

-			var expectedEvents flow.EventsList
-			for _, event := range s.blockEvents[b.ID()] {
-				if test.filter.Match(event) {
-					expectedEvents = append(expectedEvents, event)
-				}
-			}
+			// When local storage is used, events are returned in execution order
+			// (TransactionIndex, then EventIndex). Build expectedEvents in the same order.
+			srcEvents := s.blockEvents[b.ID()]
+			sorted := make([]flow.Event, len(srcEvents))
+			copy(sorted, srcEvents)
+			sort.Slice(sorted, func(i, j int) bool {
+				if sorted[i].TransactionIndex == sorted[j].TransactionIndex {
+					return sorted[i].EventIndex < sorted[j].EventIndex
+				}
+				return sorted[i].TransactionIndex < sorted[j].TransactionIndex
+			})
+			var expectedEvents flow.EventsList
+			for _, event := range sorted {
+				if test.filter.Match(event) {
+					expectedEvents = append(expectedEvents, event)
+				}
+			}

This would need to be conditional on whether local storage is active, or you could factor it into a per-test ordering helper. Alternatively, ensure BackendExecutionDataSuite.SetupTest always generates events in execution order and add an assertion on the fixture data.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@engine/access/state_stream/backend/backend_events_test.go` around lines 105 -
118, The test assumes event order implicitly; make expectedEvents match
storage's execution order when setupLocalStorage() is active by sorting
s.blockEvents before building expectedEvents (or by using the same comparator
used earlier: TransactionIndex then EventIndex); update the subscribe test path
that builds expectedEvents (referencing subscribe, expectedEvents,
requireEventsResponse) to either conditionally sort events when
setupLocalStorage() is enabled or call a shared helper that returns
execution-sorted events, and/or add an assertion in
BackendExecutionDataSuite.SetupTest that fixture s.blockEvents are already
execution-ordered to avoid the fragile implicit coupling.

storage/store/events_test.go (1)

262-264: Same redundant txIDLow[0] = 0x00 no-op as in TestByBlockID_SortsEventsByExecutionOrder.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events_test.go` around lines 262 - 264, The assignment
txIDLow[0] = 0x00 is redundant because a zeroed flow.Identifier is already
initialized to 0; remove that no-op and keep only the meaningful initialization
for txIDHigh (txIDHigh[0] = 0xFF) or explicitly set both if you prefer
clarity—update the test variables txIDLow and txIDHigh in events_test.go (the
declarations of txIDLow and txIDHigh) to remove the redundant txIDLow[0] = 0x00
assignment.

storage/store/events_test.go (2)

143-201: Consider extracting the near-identical sort-order test body into a shared table-driven helper.

TestByBlockID_SortsEventsByExecutionOrder and TestServiceEventByBlockID_SortsEventsByExecutionOrder share identical fixture construction, freshStore bypass logic, and assertions — only the store constructor (NewEvents vs. NewServiceEvents) and BatchStore argument type differ. A small local helper would eliminate the duplication and make future changes (e.g., adding a third event) single-site.

Also applies to: 247-304

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events_test.go` around lines 143 - 201, Extract the duplicated
test logic from TestByBlockID_SortsEventsByExecutionOrder and
TestServiceEventByBlockID_SortsEventsByExecutionOrder into a small table-driven
helper that builds the same fixtures and assertions once and is parameterized by
the store constructor and the BatchStore call signature; e.g., create a local
helper testSortEventsByExecOrder(t, createStore func(m metrics.Collector, db
storage.DB) interface{}, batchStoreFunc func(store interface{}, lctx
lockctx.Context, blockID flow.Identifier, lists []interface{}, rw
storage.ReaderBatchWriter) error) that constructs txIDLow/txIDHigh,
evtTx0/evtTx1A/evtTx1B, invokes BatchStore through the provided batchStoreFunc,
creates a fresh store via createStore to force a DB load, calls ByBlockID (via
type assertion inside the helper) and asserts the three events are in execution
order; then replace both tests to call this helper with NewEvents/appropriate
[]flow.EventsList and NewServiceEvents/appropriate []flow.EventsList types.

---

159-161: txIDLow[0] = 0x00 is a no-op — consider removing it or using a non-zero value.

var txIDLow flow.Identifier zero-initializes the [32]byte array, so assigning 0x00 to index 0 leaves the value unchanged. The comment "first in DB key order" is still accurate (the zero ID will sort before txIDHigh), but the explicit assignment is misleading — it implies intentional mutation while doing nothing.

Either drop the line entirely, or to make the test's intent maximally clear, assign a distinct non-zero value:

♻️ Proposed fix

 var txIDLow, txIDHigh flow.Identifier
-txIDLow[0] = 0x00  // first in DB key order
+txIDLow[0] = 0x01  // first in DB key order
 txIDHigh[0] = 0xFF // second in DB key order

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@storage/store/events_test.go` around lines 159 - 161, The statement
txIDLow[0] = 0x00 is a no-op because var txIDLow flow.Identifier
zero-initializes the [32]byte; either remove that line to avoid misleading
intent, or set a distinct non-zero value (e.g., txIDLow[0] = 0x01) so the test
clearly uses two different IDs; update the code around txIDLow and txIDHigh to
reflect the chosen approach (txIDHigh[0] = 0xFF can remain unchanged).

utils/slices/slices.go (1)

55-73: Consider removing the mutation side-effect instead of just documenting it.

The CAUTION comment is a good stopgap, but given the PR's explicit goal of eliminating in-place-mutation bugs, it's worth also fixing AreStringSlicesEqual to not mutate its inputs. The "slices" stdlib package (already imported) provides everything needed — this would also let you drop the "sort" import and the CAUTION warning entirely.

The project targets Go 1.25.1, so slices.Clone, slices.Sort, and slices.Equal (all available since Go 1.21.0) are fully supported.

♻️ Proposed non-mutating refactor

-// AreStringSlicesEqual returns true if the two string slices are equal.
-//
-// CAUTION: this function performs inplace sorts of the input slices.
+// AreStringSlicesEqual returns true if the two string slices contain
+// the same elements (order-independent). The input slices are not modified.
 func AreStringSlicesEqual(a, b []string) bool {
 	if len(a) != len(b) {
 		return false
 	}
-
-	sort.Strings(a)
-	sort.Strings(b)
-
-	for i := range a {
-		if a[i] != b[i] {
-			return false
-		}
-	}
-
-	return true
+	aCopy := slices.Clone(a)
+	bCopy := slices.Clone(b)
+	slices.Sort(aCopy)
+	slices.Sort(bCopy)
+	return slices.Equal(aCopy, bCopy)
 }

And remove the now-unused "sort" import:

 import (
 	"slices"
-	"sort"
 
 	"golang.org/x/exp/constraints"
 )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@utils/slices/slices.go` around lines 55 - 73, AreStringSlicesEqual mutates
its inputs by calling sort.Strings; change it to clone both inputs using
slices.Clone, sort the clones with slices.Sort, then compare with slices.Equal
so the original slices aren't modified; update the function body accordingly and
remove the "sort" import and the CAUTION comment since mutation no longer
occurs.

engine/access/rest/http/routes/transactions_test.go (1)

1808-1828: Consider setting a non-zero ComputationUsed in transactionResultFixture.

transactionResultFixture remains at the Go zero value for ComputationUsed, so every test that uses it (e.g. "Get by ID with results" at line 170, "get by block ID with expanded results" at line 676) still asserts "computation_used": "0". This only validates the zero path for the expanded-results endpoints at the REST layer — the same behavior as before this PR.

Setting a non-zero value here would trivially cover those code paths without duplicating any logic.

♻️ Suggested update

 func transactionResultFixture(tx flow.TransactionBody) *accessmodel.TransactionResult {
 	txID := tx.ID()
 	cid := unittest.IdentifierFixture()
 	return &accessmodel.TransactionResult{
 		Status:     flow.TransactionStatusSealed,
 		StatusCode: 1,
 		Events: []flow.Event{
 			unittest.EventFixture(
 				unittest.Event.WithEventType(flow.EventAccountCreated),
 				unittest.Event.WithTransactionIndex(0),
 				unittest.Event.WithEventIndex(0),
 				unittest.Event.WithTransactionID(txID),
 				unittest.Event.WithPayload([]byte{}),
 			),
 		},
-		ErrorMessage:  "",
-		BlockID:       tx.ReferenceBlockID,
-		CollectionID:  cid,
-		TransactionID: txID,
+		ErrorMessage:    "",
+		BlockID:         tx.ReferenceBlockID,
+		CollectionID:    cid,
+		TransactionID:   txID,
+		ComputationUsed: 1234,
 	}
 }

The corresponding expected JSON strings in tests using this fixture would then need "computation_used": "1234".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@engine/access/rest/http/routes/transactions_test.go` around lines 1808 -
1828, transactionResultFixture currently leaves
accessmodel.TransactionResult.ComputationUsed at the Go zero value, causing
tests to only validate the zero path; update transactionResultFixture (the
function named transactionResultFixture) to set ComputationUsed to a non-zero
value (e.g., 1234) in the returned accessmodel.TransactionResult, and then
update the expected JSON in tests that use this fixture (e.g., "Get by ID with
results" and "get by block ID with expanded results") to assert
"computation_used": "1234".

Configuration used: defaults

Review profile: CHILL

Plan: Pro