use headblock for prunePostBlockOperationPools, remove duplicate markInclusionBLStoExecutionChange calls #12085
Conversation
james-prysm
changed the title
james-prysm
changed the title
| if err := s.forkchoiceUpdateWithExecution(ctx, headRoot, s.CurrentSlot()+1); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if isNewHead { | ||
| // uses the newly saved block from forkchoiceUpdateWithExecution | ||
| headBlock, err := s.headBlock() |
There was a problem hiding this comment.
will use HeadBlock(ctx) instead
| return err | ||
| } | ||
| // Handle post block operations such as attestations and exits. | ||
| if err := s.handlePostBlockOperations(headBlock.Block()); err != nil { |
There was a problem hiding this comment.
Currently the function handlePostBlockOperations only marks as included operations in our pools right? we may as well rename this function to pruneOperationPools or similar.
| if err := s.forkchoiceUpdateWithExecution(ctx, headRoot, s.CurrentSlot()+1); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if isNewHead { |
There was a problem hiding this comment.
Perhaps we can consider moving this block to ReceiveBlock to alleviate the cognitive value of onBlock
There was a problem hiding this comment.
may need bigger refactors to do this, blockRoot passed in is checked against headRoot first as a comparison that's why this check if afterwards. would need to move more things around to enable this. maybe we can keep it here until I move bigger pieces.
There was a problem hiding this comment.
moved it based on slack discussion.
james-prysm
changed the title
| @@ -48,6 +49,9 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig | |||
|
|
|||
| s.cfg.ForkChoiceStore.Lock() | |||
| defer s.cfg.ForkChoiceStore.Unlock() | |||
| // Checks if the current blockRoot ( which is also the head root) is new. | |||
| // This check must come before forkchoiceUpdateWithExecution because it saves the new head. | |||
| isNewHead := s.isNewHead(blockRoot) | |||
There was a problem hiding this comment.
Why do this here versus next to the usage?
There was a problem hiding this comment.
I need to get is New Head before forkchoiceUpdateWithExecution saves the head which happens inside the onblock. otherwise it's not new anymore.
There was a problem hiding this comment.
This doesn't work either: an incoming block will always have isNewHead==true here. You need to save the headroot here and then after the call to onBlock check the the new head.
| func (s *Service) handlePostBlockOperations(b interfaces.ReadOnlyBeaconBlock) error { | ||
| // prunePostBlockOperationPools only runs on new head otherwise should return a nil. | ||
| func (s *Service) prunePostBlockOperationPools(ctx context.Context, root [32]byte) error { | ||
| headRoot, err := s.HeadRoot(ctx) |
There was a problem hiding this comment.
I think there will be a dead lock. HeadRoot requires a read lock
There was a problem hiding this comment.
I thought what potuz mentioned here #12085 (comment) was to use this lock instead.
There was a problem hiding this comment.
HeadRoot does not require a read lock, the function takes one.
There was a problem hiding this comment.
Ah ok. HeadRoot uses headLock from Blockchain pkg not ForkchoiceLock. Confusing how there are two different locks
| @@ -48,6 +49,9 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig | |||
|
|
|||
| s.cfg.ForkChoiceStore.Lock() | |||
| defer s.cfg.ForkChoiceStore.Unlock() | |||
| // Checks if the current blockRoot ( which is also the head root) is new. | |||
| // This check must come before forkchoiceUpdateWithExecution because it saves the new head. | |||
| isNewHead := s.isNewHead(blockRoot) | |||
There was a problem hiding this comment.
This doesn't work either: an incoming block will always have isNewHead==true here. You need to save the headroot here and then after the call to onBlock check the the new head.
| func (s *Service) handlePostBlockOperations(b interfaces.ReadOnlyBeaconBlock) error { | ||
| // prunePostBlockOperationPools only runs on new head otherwise should return a nil. | ||
| func (s *Service) prunePostBlockOperationPools(ctx context.Context, root [32]byte) error { | ||
| headRoot, err := s.HeadRoot(ctx) |
There was a problem hiding this comment.
HeadRoot does not require a read lock, the function takes one.
| // uses the newly saved block from forkchoiceUpdateWithExecution | ||
| headBlock, err := s.HeadBlock(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Why not just pass the block as part of the argument? We can save a block copy
| @@ -57,7 +57,8 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig | |||
| } | |||
|
|
|||
| // Handle post block operations such as pruning exits and bls messages. | |||
| if err := s.prunePostBlockOperationPools(ctx, blockRoot); err != nil { | |||
| // Prune service config vlaues based on the block copy if it's the head. | |||
There was a problem hiding this comment.
What does this mean? Maybe Prune block operations if it's the head
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
We are accidentally looping twice on marking bls changes for inclusion, and also using the current processed block instead of the head block. I migrated this function along with others in handlePostBlockOperations to onBlock to correctly deal with only canonical blocks.