Sync: Gracefully handle blocks from an unknown fork#11085
Conversation
|
/cmd prdoc --audience node_dev --bump patch |
…e_dev --bump patch'
| if !continues_known_fork { | ||
| let current = number.min(best_queued_number); | ||
| peer.common_number = peer.common_number.min(self.client.info().finalized_number); | ||
| peer.state = PeerSyncState::AncestorSearch { |
There was a problem hiding this comment.
Could we get this peer stuck in an AncestorSearch? What would be the worst case if peer B is maliciously advertising a block 21? Could it force us to go back to genesis (e.g., if the block is 1M and on a malicious fork)?
There was a problem hiding this comment.
The node can not go back to block 21, especially if this block is below the last finalized block. Ancestry search is always the state with one peer and not with all peers together. So, if we are doing ancestry search with B, we can still import blocks from other peers.
| peer.update_common_number(number.saturating_sub(One::one())); | ||
| } | ||
|
|
||
| // If this announced block isn't following any known fork, we have do start an ancestor |
There was a problem hiding this comment.
nit: / we have do start / we have to start
| // The node is continuing a known fork if either the block itself is known, the parent is | ||
| // known or the block references the previously announced `best_hash`. | ||
| let continues_known_fork = | ||
| known || known_parent || announce.header.parent_hash() == &peer.best_hash; | ||
|
|
||
| let best_queued_number = self.best_queued_number; |
There was a problem hiding this comment.
nit: these can be moved inside the if is_best branch
| @@ -521,11 +534,29 @@ where | |||
| // is either one further ahead or it's the one they just announced, if we know about it. | |||
| if is_best { | |||
| if known && self.best_queued_number >= number { | |||
There was a problem hiding this comment.
nit: here and below could use best_queued_number or remove the let best_queued_number?
| let mut branch1 = None; | ||
| for i in 0..2 { | ||
| let at = if i == 0 { BlockId::Number(10) } else { BlockId::Hash(branch1.unwrap()) }; | ||
| branch1 = net.peer(0).push_blocks_at(at, 1, i == 0).pop(); |
There was a problem hiding this comment.
dq: why not true instead of i == 0 (ie just the first block has tx)?
There was a problem hiding this comment.
The test block builder failed. I did not wanted to dig deeper xD
| ) -> Result<ImportResult, Self::Error> { | ||
| self.inner.check_block(block).await | ||
| let result = self.inner.check_block(block).await; | ||
| if !matches!(result, Ok(ImportResult::Imported(_) | ImportResult::AlreadyInChain)) { |
There was a problem hiding this comment.
Nit: Should KnownBad also be tolerated here?
| // The node is continuing a known fork if either the block itself is known, the parent is | ||
| // known or the block references the previously announced `best_hash`. | ||
| let continues_known_fork = | ||
| known || known_parent || announce.header.parent_hash() == &peer.best_hash; |
There was a problem hiding this comment.
Can we run into a race condition here? Something like a previous block triggered ancestor search and peer.best_hash is already set. But then next block gets announced and this condition would be true.
There was a problem hiding this comment.
When the peer is in ancestry search mode, this method aborts early (check above).
|
|
||
| // The node is continuing a known fork if either the block itself is known, the | ||
| // parent is known or the block references the previously announced `best_hash`. | ||
| let continues_known_fork = |
There was a problem hiding this comment.
How about moving this inside
if is_best {
let continues_known_fork =
known || known_parent || announce.header.parent_hash() == &peer.best_hash;
(...)
}where it is only used?
There was a problem hiding this comment.
This doesn't work, because then peer.best_hash maybe is already updated.
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
5 participants
There is the possibility that node A connects to node B. Both are at the same best block (20). Shortly after this, node B announces a block 21 that is from a completely different fork (started at e.g. block 15). Right now this leads to node A downloading this block 21 and then failing to import it because it doesn't have the parent block.
This pull request solves this situation by putting the peer into ancestry search when it detects a fork that is "unknown".