Allow reorg during sync #9268
Allow reorg during sync #9268
Conversation
tbenr
force-pushed
the
allow-reorg-while-syncing
branch
from
c64aad0 to
3a0c94a
Compare
| long subscribeToBlocksImportedEvent(BlocksImportedSubscriber subscriber); | ||
|
|
||
| interface BlocksImportedSubscriber { | ||
| void onBlocksImported(SignedBeaconBlock lastImportedBlock); |
There was a problem hiding this comment.
well cose I don't want this to look like you can subscribe to it and pretend to get all blocks.
Maybe I should rename everything to be onBatchImported(SignedBeaconBlock lastImportedBlock)
There was a problem hiding this comment.
oh, problem is that I put that on Sync interface, and not BatchSync interface. So the concept of "batch" is not part of that...
Suggestions? :)
| public void onBlocksImported(final SignedBeaconBlock lastImportedBlock) { | ||
| eventThread.execute( | ||
| () -> { | ||
| if (isSyncSpeculative()) { |
There was a problem hiding this comment.
why do we skip this case?
(sorry, I just don't understand what is speculative sync)
There was a problem hiding this comment.
speculative syncing is when we are on sync but we "hear" about a chain that we speculatively sync. So we don't want the sync reorg logic to be applied in that case (we are actually in sync in that scenario)
| .get() | ||
| .getSlot() | ||
| .plus(REORG_SLOT_THRESHOLD) | ||
| .isGreaterThan(lastImportedBlock.getSlot())) { |
There was a problem hiding this comment.
Not sure about this rule, we determinate best chain by votes, two chains could have similar length having a big difference in votes. I guess we don't want ot switch back and forth and it could help. Is this a purpose?
There was a problem hiding this comment.
as discussed, the sync reorg logic only applies during initial syncing when all protoarray votes are 0
ProtoArray::reorgWhileSyncing
// check we are in syncing mode where all nodes have a max weight of 1
if (commonAncestorNode.getWeight().isGreaterThan(1)) {
return;
}
and it is actually ok to switch back and forth, as long as we keep as canonical the longest chain we are syncing to
| * <p>The function set all weights to 0 to the reorged chain and set all weights to 1 to the new | ||
| * chain. | ||
| */ | ||
| public void reorgWhileSyncing( |
There was a problem hiding this comment.
so we just removed weight from old chain and put it to the new chain undoubtely? We need some reasons for this. Could we get real weights? There is no clue new chain is better than old
There was a problem hiding this comment.
the idea is to make it the chain we want to reorg to as canonical only by 1 vote. So once we start counting actual votes that +1 becomes irrelevant and goes away as we follow the chain (latest imported block won't have that +1 anymore)
tbenr
force-pushed
the
allow-reorg-while-syncing
branch
from
ffb369a to
6a90496
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| break; | ||
| } | ||
| node = getNodeByIndex(node.getParentIndex().get()); | ||
| } |
There was a problem hiding this comment.
Bug: Reorg Method Misaligns Weighting Chain
The reorgWhileSyncing method starts adding weight from newHeadNode.getBestDescendantIndex() instead of newHeadNode itself. This can lead to weighting an unintended descendant chain, as getBestDescendantIndex() relies on existing weights that may not align with the reorg's target path.
There was a problem hiding this comment.
this is actually intended. The idea is to select the tip of that chain where new head is (if in the meantime we added a new block on top of it) and set all chain's nodes to weight 1
2 participants
SyncReorgManagerwhich is triggered at each successful batch import.SyncReorgManagerwill call a protoarray'sreorgWhileSyncing(via ForkChoiceTrigger->ForkChoice) which will "switch" votes from the old chain to the new chain. This logic only applies to protoarray in "syncing state" (no votes applied)It is designed to reorg only if we import a block at a more recent slot than our current head (+ 10 slot).
There could be use-cases in which we want to reorg sooner: if our DB contains a long non-canonical chain we will keep the tip of that as our canonical head until the contending chain we are syncing overcome that tip.
But for now this implementation solves the problem in which our chain tip is just on a reorged block and we just keep syncing from its parent.
fixes #9195
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog