Skip to content

Fix shared slice mutation in fork schedule builder - #85

Open
damilolaedwards wants to merge 1 commit into
ethpandaops:masterfrom
damilolaedwards:fix/fork-schedule-shared-mutation
Open

Fix shared slice mutation in fork schedule builder#85
damilolaedwards wants to merge 1 commit into
ethpandaops:masterfrom
damilolaedwards:fix/fork-schedule-shared-mutation

Conversation

@damilolaedwards

Copy link
Copy Markdown

Summary

ForkScheduleFromForkEpochs sorted its argument in place with sort.Slice. Since a slice argument shares its backing array with the caller, calling this read-shaped, innocuously named method reorders the caller's Spec.ForkEpochs out from under it, and races against anything else reading that slice concurrently, such as the fork metrics job that runs by default.

Fix sorts a copy instead, the same way BlobSchedule.GetMaxBlobsPerBlock already does.

Test plan

  • Added tests in pkg/beacon/state/scheduled_fork_test.go: one confirming the caller's slice is no longer reordered, one running the builder concurrently against a reader using the same access pattern as ForkMetrics.calculateCurrent
  • Confirmed both fail against the old code (mutation test fails outright, race test fails under -race) and pass against the fix
  • go build ./..., go vet ./..., go test -race ./... all green

ForkScheduleFromForkEpochs sorted its argument in place, which shares
a backing array with whatever Spec the caller holds. Calling this
read-shaped method reordered Spec.ForkEpochs out from under the
caller, and raced against anything else reading that slice
concurrently, such as the fork metrics job. Now it sorts a copy,
matching how BlobSchedule already handles this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant