-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use next slot cache for payload attribute #12286
Conversation
@@ -265,7 +265,8 @@ func (s *Service) getPayloadAttribute(ctx context.Context, st state.BeaconState, | |||
|
|||
// Get previous randao. | |||
st = st.Copy() | |||
st, err := transition.ProcessSlotsIfPossible(ctx, st, slot) | |||
headRoot := s.headRoot() | |||
st, err := transition.ProcessSlotsUsingNextSlotCache(ctx, st, headRoot[:], slot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a functional difference between ProcessSlotsIfPossible
and using NSC: the former does not error out if the requested slot is higher than the current slot. I've seen this fail on tests and I wonder if this won't fail on runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The easiest way to solve this is to add process slots only if it's higher within ProcessSlotsUsingNextSlotCache
. It seems no harm to do that but curious what you think
@@ -14,7 +14,7 @@ import ( | |||
) | |||
|
|||
type nextSlotCache struct { | |||
sync.Mutex | |||
sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change here? Is there a possibility of a deadlock? we should very rarely call getPayloadAttributes
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dead lock but curious why we don't use RWMutex
to begin with? It seems inevitable we move to that direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with a comment, should we error out if slot < st.Slot()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code seems good. I didn't run it locally
This PR changes
getPayloadAttribute
to avoid duplicatedProcessSlotsIfPossible
if necessary. This becomes relevant for epoch boundary blocks