[FIX] Processing checkpoint blocks by fetcher#866
Conversation
|
|
||
| private def handleNewCheckpointBlockNotOnTop(block: Block, peerId: PeerId, state: BlockFetcherState): Unit = { | ||
| log.debug("Got checkpoint block") | ||
| if (state.waitingHeaders.exists(_.hash == block.hash || state.readyBlocks.exists(_.hash == block.hash))) { |
There was a problem hiding this comment.
I think it would be quite beneficial to:
- extract methods for checking headers and blocks existence by hash (in fetcher state)
- save results to variables here, so computation is not performed with each
if
| log.debug( | ||
| s"Checkpoint block ${ByteStringUtils.hash2string(block.hash)} not fit into queues - clearing the queues" | ||
| ) | ||
| val newState = state |
There was a problem hiding this comment.
shouldn't the block be saved somehow or passed to block importer in this case? This case means more or less "we're on different fork" and we don't do anything in order to switch into checkpointed branch
| .withPeerForBlocks(peerId, Seq(block.number)) | ||
| .withKnownTopAt(block.number) | ||
| fetchBlocks(newState) | ||
| } else handleFutureBlock(block, state) |
There was a problem hiding this comment.
Same here - we just try to update known top block and believe other nodes will do their job so we land on checkpointed branch.
If this code doesn't fork on checkpointed testnet - we can probably leave it as it is now and pass a follow-up task to the team. WDYT?
| fetchBlocks(newState) | ||
| if (state.readyBlocks.exists(_.header.hash == block.header.parentHash)) { | ||
| log.debug(s"Checkpoint block fit into current ready blocks queue") | ||
| val newState = state |
There was a problem hiding this comment.
For inserting checkpoint into right slot - please make and use methods that protect fetcher state invariants (like for headers there's appendHeaders)
It could probably even simplify the code here significantly to something in lines of:
state.tryInsertCheckpointBlock(block, peerId) match {
case Left(err) if block.number <= state.lastBlock =>
log.debug("Couldn't insert checkpoint to queues, passing to importer")
state.importer ! NewCheckpointBlock(block, peerId)
case Left(err) =>
log.debug("Couldn't insert checkpoint to queues")
//whatever
case Right(newState) =>
fetchBlocks(newState)
}
| } | ||
| .getOrElse(this) | ||
|
|
||
| def enqueueHeader(header: BlockHeader): BlockFetcherState = { |
There was a problem hiding this comment.
please remove these methods and use safer counterparts - lack of them caused a bit of headache already
| } | ||
|
|
||
| "should process checkpoint blocks when checkpoint can fit into ready blocks queue" in new TestSetup { | ||
| startFetcher() |
There was a problem hiding this comment.
does it make sense to extract some common parts from these tests?
pslaski
force-pushed
the
fix-fetcher-processing-checkpoints
branch
from
4b84b5a to
af7d8ff
Compare
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
Minor things, besides that looks good to me!
|
|
||
| def tryInsertBlock(block: Block, peerId: PeerId): Either[String, BlockFetcherState] = { | ||
| val blockHash = block.hash | ||
| if (isExistInReadyBlocks(blockHash) || isExistInWaitingHeaders(blockHash)) { |
|
|
||
| // Fetcher should enqueue all the received blocks | ||
| importer.send(blockFetcher, PickBlocks(chain.size)) | ||
| importer.send(blockFetcher, PickBlocks(firstBlocksBatch.size)) |
There was a problem hiding this comment.
isn't this send call duplicate now?
pslaski
force-pushed
the
fix-fetcher-processing-checkpoints
branch
from
af7d8ff to
e1fbab4
Compare
pslaski
force-pushed
the
fix-fetcher-processing-checkpoints
branch
2 times, most recently
from
83dd3b3 to
fbb54f4
Compare
pslaski
force-pushed
the
fix-fetcher-processing-checkpoints
branch
from
fbb54f4 to
4ad48cf
Compare
3 participants
Description
Currently, the fetcher is not processing new checkpoint blocks which are older than the current top
Important Changes Introduced
Testing
Syncing with Mordor testnet: ✔️
Need to be tested with federation nodes.