pre-flight fcu for speed bump#4194
Conversation
|
|
||
| # Enqueue the actual forkchoice apply. For ack-only fCUs we don't await | ||
| # the result. This avoids the response getting blocked behind the shared queue worker | ||
| let fcuFut = chain.queueForkChoice(headHash, finalizedBlockHash, safeBlockHash) |
There was a problem hiding this comment.
Is it possible for the queueForkChoice background job to fail in the background? Or after running preflightForkChoice and returning a success to the caller , should the job always succeed?
There was a problem hiding this comment.
Theoretically it is not possible to fail.
The only possibility is if the db is corrupt, or somehow the db connection is broken ( or other db level failures )
There was a problem hiding this comment.
Actually it might be possible for it to fail when processing multiple heads. The removeBlockFromCache function can remove blocks from the hashToBlock table which can cause forkChoice to fail. removeBlockFromCache gets called from two places updateBase and updateFinalized. If those functions get called asynchronously before the forkChoice task runs then you get a silent failure.
There was a problem hiding this comment.
Well the queueForkChoice just adds the fCU call to the asyncQueue. So nothing else calls it async. And the queue is processed one by one
There was a problem hiding this comment.
removeBlockFromCachegets called from two placesupdateBaseandupdateFinalized. If those functions get called asynchronously before the forkChoice task runs then you get a silent failure.
I think not possible in the current design with asyncQueue implementation
There was a problem hiding this comment.
Hmm, I'm not sure. Maybe its ok because the single queue enforces the ordering as long as there are no await calls which yield to other tasks before the forkChoice task is added to the queue.
There was a problem hiding this comment.
Hmm, I'm not sure. Maybe its ok because the single queue enforces the ordering as long as there are no await calls which yield to other tasks before the forkChoice task is added to the queue.
Since the queueForkChoice is a template and contains an await call (which is good for rate limiting purposes), then the await will yield the event loop meaning something else can run concurrently perhaps before the forkChoice task is added to the queue. Maybe this is fine as long as no other code calls updateBase or updateFinalized outside of the task queue flow.
There was a problem hiding this comment.
Yes that is true.
For now by design updateBase & updateFinalized is not called outside the queue
Will add some comments on this
|
LGTM |
| return err("Cannot find head block: " & headHash.short) | ||
|
|
||
| if safeHash != zeroHash32 and c.hashToBlock.getOrDefault(safeHash).isNil: | ||
| return err("Cannot find safe block: " & safeHash.short) |
There was a problem hiding this comment.
This check is more strict now than the implementation in forkChoice which would silently ignore if the safeHash doesn't exist in the hashToBlock map. Not sure if this change is desired or not.
|
|
||
| # Enqueue the actual forkchoice apply. For ack-only fCUs we don't await | ||
| # the result. This avoids the response getting blocked behind the shared queue worker | ||
| let fcuFut = chain.queueForkChoice(headHash, finalizedBlockHash, safeBlockHash) |
There was a problem hiding this comment.
Looks like there is nothing logging the result of the forkChoice call. Even if we don't expect any failures we should probably log something. Perhaps the queueForkChoice asyncHandler could log the result of forkChoice.
There was a problem hiding this comment.
Surely yeah, will add some logs. Will help identify issues
| # Payload generation reads `chain.latestHeader`, so we must wait for the | ||
| # apply to complete before assembling the block. | ||
| (await fcuFut).isOkOr: | ||
| return invalidFCU(error, chain, header) |
There was a problem hiding this comment.
Since we end up awaiting here for the block building it might be better to not use the async queue flow at all if attrsOpt.isSome. Block building would be faster in that case.
There was a problem hiding this comment.
Yes that PR is also ready on top of this. Just testing and ironing out few issues on that
( little difficult to test as I don't have much validators, so very less proposals to test )
3 participants
makes fcu call super-fast with minimal downside
Upon testing shows significant performance improvement, and improvement in attestations from CL side ( fewer attestation misses )