Forkchoice external locks v2 #12036
Forkchoice external locks v2 #12036
Conversation
| s.cfg.ForkChoiceStore.Lock() | ||
| defer s.cfg.ForkChoiceStore.Unlock() |
There was a problem hiding this comment.
s.cfg.ForkChoiceStore.IsOptimistic is also called within onBlock
There was a problem hiding this comment.
Same with finalized and justified epochs
There was a problem hiding this comment.
this is not a problem, the problem is calls to chaininfo.IsOptimistic any access to forkchoice directly is not a problem
There was a problem hiding this comment.
Some missing locks from my quick audit:
Blockchain pkg:
func (s *Service) saveGenesisData(ctx context.Context, genesisState state.BeaconState) error
func (s *Service) StartFromSavedState(saved state.BeaconState)
func (s *Service) ReceiveAttesterSlashing(ctx context.Context, slashing *ethpb.AttesterSlashing) // Can be triggered via p2p
Debug rpc pkg
(ds *Server) GetForkChoice(ctx context.Context, _ *emptypb.Empty) (*ethpbv1.ForkChoiceDump, error
Proposer RPC pkg
(vs *Server) circuitBreakBuilder(s primitives.Slot) (bool, error)
Stategen pkg
New(beaconDB db.NoHeadAccessDatabase, fc forkchoice.ForkChoicer, opts ...StateGenOption) *State
| @@ -122,7 +122,7 @@ func (s *Service) saveHead(ctx context.Context, newHeadRoot [32]byte, headBlock | |||
| reorgDistance.Observe(float64(dis)) | |||
| reorgDepth.Observe(float64(dep)) | |||
|
|
|||
| isOptimistic, err := s.IsOptimistic(ctx) | |||
| isOptimistic, err := s.ForkChoicer().IsOptimistic(newHeadRoot) | |||
There was a problem hiding this comment.
This function saveHead calls later the function notifyNewHeadEvent which acquires a lock for IsOptimistic. It would be nice to check the safety of this
potuz
added
Blocked
|
Blocking it for some more eyes cc. @nisdas |
| @@ -277,6 +285,8 @@ func (s *Service) CurrentFork() *ethpb.Fork { | |||
|
|
|||
| // IsCanonical returns true if the input block root is part of the canonical chain. | |||
| func (s *Service) IsCanonical(ctx context.Context, blockRoot [32]byte) (bool, error) { | |||
| s.ForkChoicer().RLock() | |||
| defer s.ForkChoicer().RUnlock() | |||
| // If the block has not been finalized, check fork choice store to see if the block is canonical | |||
| if s.cfg.ForkChoiceStore.HasNode(blockRoot) { | |||
There was a problem hiding this comment.
Not really related to this PR, but can we stick to either just using Forkchoicer or ForkChoiceStore ? Using both gives the impression they are different things.
There was a problem hiding this comment.
In part I am guilty of this since I never liked the overhead of an extra call to ForkChoicer(), it would be nice to clean this up and actually not export the field and only use the getter.
beacon-chain/blockchain/head.go
Outdated
| @@ -52,6 +52,7 @@ type head struct { | |||
|
|
|||
| // This saves head info to the local service cache, it also saves the | |||
| // new head root to the DB. | |||
| // Requires a lock on forkchoice. | |||
There was a problem hiding this comment.
| // Requires a lock on forkchoice. | |
| // Caller of the method MUST acquire a lock on forkchoice. |
| reportSlotMetrics(blockCopy.Block().Slot(), s.HeadSlot(), s.CurrentSlot(), finalized) | ||
| } | ||
|
|
||
| if err := s.cfg.BeaconDB.SaveBlocks(ctx, s.getInitSyncBlocks()); err != nil { | ||
| return err | ||
| } | ||
| finalized := s.FinalizedCheckpt() | ||
| finalized := s.ForkChoicer().FinalizedCheckpoint() |
There was a problem hiding this comment.
why not do the copy here ?
finalized := ðpb.Checkpoint{Epoch: cp.Epoch, Root: bytesutil.SafeCopyBytes(cp.Root[:])}
| func (s *Service) checkSaveHotStateDB(ctx context.Context) error { | ||
| currentEpoch := slots.ToEpoch(s.CurrentSlot()) | ||
| // Prevent `sinceFinality` going underflow. | ||
| var sinceFinality primitives.Epoch | ||
| finalized := s.FinalizedCheckpt() | ||
| finalized := s.ForkChoicer().FinalizedCheckpoint() |
There was a problem hiding this comment.
same thing here:
finalized := ðpb.Checkpoint{Epoch: cp.Epoch, Root: bytesutil.SafeCopyBytes(cp.Root[:])}
A different take on #12014
We remove all locks from forkchoice and lock in
chain_info.goso that RPC calls do not need to lock. Writers of blockchain package core functions need to access directly forkchoice instead of chain_info functions.blockchain.headLock