block on forkchoiceUpdated EL calls due to doing fewer of them #4609
Conversation
| @@ -543,7 +549,7 @@ proc storeBlock*( | |||
| # - "Beacon chain gapped" from DAG head to optimistic head, | |||
| # - followed by "Beacon chain reorged" from optimistic head back to DAG. | |||
| self.consensusManager[].updateHead(newHead.get.blck) | |||
| asyncSpawn eth1Monitor.runForkchoiceUpdatedDiscardResult( | |||
| await eth1Monitor.runForkchoiceUpdatedDiscardResult( | |||
There was a problem hiding this comment.
refactor to put the discard here?
There was a problem hiding this comment.
runForkchoiceUpdatedDiscardResult is one such example of a now-too-fine delineation, yes which was there only to support some cases being asyncSpawn, indeed, for example.
| @@ -572,10 +578,10 @@ proc storeBlock*( | |||
| # Some attached validator is next proposer, so prepare payload. As | |||
| # updateHead() updated the DAG head, runProposalForkchoiceUpdated, | |||
| # which needs the state corresponding to that head block, can run. | |||
| asyncSpawn self.consensusManager.runProposalForkchoiceUpdated( | |||
| await self.consensusManager.runProposalForkchoiceUpdated( | |||
There was a problem hiding this comment.
it's kind of weird that we sometimes run fcu through consensusmanager and sometimes through eth1monitor - can't we collect all FCU calls in consensus manager? also, can't we move the proposer-or-not logic there?
There was a problem hiding this comment.
#4465 already does this in elManager.forkchoiceUpdated
There was a problem hiding this comment.
soo... this PR looks like it might solve the immediate problem of concurrent / stale head updates, but it also feels like the code could be simplified further with a round of refactoring (which we can leave for another PR) - ie it's starting to get quite hairy, to understand what sets which head where and why
|
merging for now since sync is completely broken by the concurrent head updates when spawned tasks get reordered |
There is potentially other cleanup from cases to consolidate -- previously, one distinction was exactly whether to |
In a steady-state synced situation, the CL needs to wait anyway for the EL to finish, so it was not truly asynchronous except during syncing.