client: Patch fcu skeleton blockfill process to avoid chain reset #3137
Conversation
414014e to
1ef50dd
Compare
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| // cause chain reset. This can happen if any other async process added a batch of blocks like | ||
| // execution's setHead. If that caused this chain to be not canonical anymore than the next | ||
| // putblocks should fail causing the fill to exit with skeleton stepback | ||
| if (this.chain.blocks.height <= block.header.number) { |
There was a problem hiding this comment.
this is the only real change here, i.e. skip putting if chain has been advanced by some other putBlock process. But in the hive tests i ran locally, there would still be a random race where this check would pass but still when actual writing the block, a concurrent putblock wins the race causing an issue
(so we eventually needs to fix it since the only actual locking happens in the blockchain class)
There was a problem hiding this comment.
Maybe this is a silly question but do we need to check block.header.number and ensure its only one block ahead of height?
There was a problem hiding this comment.
if its not, the putBlock will fail anyway
| this.config.updateSynchronizedState(headBlock.header) | ||
| if (!isPrevSynced && this.chain.config.synchronized) { | ||
| this.service.txPool.checkRunState() | ||
| } | ||
| } else if (!headBlock.isGenesis()) { |
There was a problem hiding this comment.
What does removing this section do?
There was a problem hiding this comment.
it seems to be a redundant txpool start snippet, just below this if/else there is a similar snippet
Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>
to avoid the chain being reset by getting interfered with sksleton's fill Canonicaland fcU's sethead's putblocks of executed blocks
now skeleton attempts blocking fill for the max number of executed blocks which fcu wants to put via execution's setHead
This PR should fix the hive pytests failing because of this issue
ps:
this is a patch but a proper solution needs to happen at blockchain layer where blocks shouldn't reset the chain unless explicitly asked to or via a proper rule that they should extend the max height of the chain.
Also skeleton 's canonical block handling/tracking needs to be up streamed to the chain , but that could be a little more involved work to address separately