Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process epoch error to use correct state version #14069

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Jun 2, 2024

h/t Seamonkey for pointing out a regression from #14001.

If the ProcessEpoch function returns an error, the state is nil by default. We can't use that to get the state version, or it will panic as seen below:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x468 pc=0x37a1d60]

goroutine 607921584 [running]:
github.com/prysmaticlabs/prysm/v5/beacon-chain/core/transition.ProcessSlots({0x1b5eb88?, 0xc10910b1f0?}, {0x1b8abe8, 0xc10de18700}, 0x1b16a0)
        beacon-chain/core/transition/transition.go:263 +0xba0
github.com/prysmaticlabs/prysm/v5/beacon-chain/core/transition.UpdateNextSlotCache({0x1b5eb88, 0xc10910b1f0}, {0xc07f7ffea0, 0x20, 0x1ed738942d8c?}, {0x1b8abe8?, 0xc05b936a80?})

This PR fixes the caller return pattern since state is mutated, we don't need to return it

@terencechain terencechain requested a review from a team as a code owner June 2, 2024 21:10
@terencechain terencechain added the Ready For Review A pull request ready for code review label Jun 3, 2024
@terencechain terencechain added the Priority: High High priority item label Jun 3, 2024
@@ -257,16 +257,18 @@ func ProcessSlots(ctx context.Context, state state.BeaconState, slot primitives.
return nil, errors.Wrap(err, "could not process epoch with optimizations")
}
} else if state.Version() <= version.Deneb {
stateVersion := version.String(state.Version())
state, err = altair.ProcessEpoch(ctx, state)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop the returned state? ProcessEpoch mutates the state argument, there is no benefit to conditionally returnign the state here. Refactor the method signature to only return an error.

Suggested change
state, err = altair.ProcessEpoch(ctx, state)
err = altair.ProcessEpoch(ctx, state)

@@ -257,16 +257,18 @@ func ProcessSlots(ctx context.Context, state state.BeaconState, slot primitives.
return nil, errors.Wrap(err, "could not process epoch with optimizations")
}
} else if state.Version() <= version.Deneb {
stateVersion := version.String(state.Version())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should do it outside , but it's also possible to trigger the nil pointer issue in the else statement maybe. It was my idea to add this state version but seems to be causing other issues.

@terencechain terencechain force-pushed the fix-state-version branch 2 times, most recently from e5f6964 to a9e0a05 Compare June 3, 2024 15:19
rkapka
rkapka previously approved these changes Jun 3, 2024
@terencechain terencechain added this pull request to the merge queue Jun 3, 2024
Merged via the queue into develop with commit 5783043 Jun 3, 2024
16 of 17 checks passed
@terencechain terencechain deleted the fix-state-version branch June 3, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants