add assertions to contains block #19001
Conversation
almogdepaz
added
full_node
sync
Fixed
| if height is None: | ||
| block_rec = self.try_block_record(header_hash) | ||
| if block_rec is None: | ||
| return False |
There was a problem hiding this comment.
we also return false for blocks that are part of the main chain, but no longer in the cache.
| @@ -769,12 +769,20 @@ async def validate_unfinished_block( | |||
|
|
|||
| return PreValidationResult(None, required_iters, conds, uint32(0)) | |||
|
|
|||
| def contains_block(self, header_hash: bytes32) -> bool: | |||
| def contains_block(self, header_hash: bytes32, height: Optional[uint32] = None) -> bool: | |||
| """ | |||
| True if we have already added this block to the chain. This may return false for orphan blocks | |||
There was a problem hiding this comment.
this comment should be updated to say that it will return false for orphan blocks. I take the current "may" to mean that orphan blocks are less likely to be in the cache.
This function also always returned false for blocks that aren't in the cache, regardless of whether they're in the main chain or not. The comment should say this too.
The new height parameter isn't really explained either. I assume it adds an extra check; not only that header_hash is in the main chain and in the cache, but also that it has the specified height.
There was a problem hiding this comment.
the hight is an optimization for most cases where we now the hight of the block (the only case we dont is unfinished blocks)
i agree with all your points, im still trying to figure out exactly what the correct Behavior is supposed to be but im pretty sure the current state is incorect for both short_sync_backtrack and short_sync_batch
| if header_hash in self.__block_records: | ||
| return self.block_record(header_hash) |
There was a problem hiding this comment.
it seems a bit backwards to make try_block_record() be implemented in terms of block_record(). It (probably) would be simpler to do it the other way around.
There was a problem hiding this comment.
i dont have a strong opinion on that, if we do it the other way around we need to throw an explicit exception for when we dont have the block
| if block_hash_from_hh is None or block_hash_from_hh != header_hash: | ||
| return False |
There was a problem hiding this comment.
I'm not convinced this was the intention of this function, or rather, that it is the expectation of callers. I think the block being in the cache is one of the expectations.
There was a problem hiding this comment.
this doesn't really make sense to me in the case of the short_sync methods or even for add block
| @@ -595,7 +595,7 @@ async def short_sync_batch(self, peer: WSChiaConnection, start_height: uint32, t | |||
| self.sync_store.batch_syncing.remove(peer.peer_node_id) | |||
| self.log.error(f"Error short batch syncing, could not fetch block at height {start_height}") | |||
| return False | |||
| if not self.blockchain.contains_block(first.block.prev_header_hash): | |||
| if not self.blockchain.contains_block(first.block.prev_header_hash, first.block.height - 1): | |||
There was a problem hiding this comment.
here, we're not really asking whether the blocks i part of the main chain, we're asking if its in the block cache. if it isn't, we declare this a deep reorg or catch-up (and presumably transition to long-sync).
There was a problem hiding this comment.
i disagree, for short_sync_batch we assume we are syncing from start_height where start_height - 1 is in our chain so checking if first -1 is in the main chain is correct
this is only done for the first_block.prev_header_hash which is expected to be the fork point i.e in out chain
There was a problem hiding this comment.
right, I guess we expect both in this case.
| if ( | ||
| self.blockchain.contains_block(curr.block.prev_header_hash, curr.block.height - 1) | ||
| or curr_height == 0 | ||
| ): |
There was a problem hiding this comment.
I agree that this call site is not concerned with whether the block is in the cache or not
Purpose:
make sure height_to_hash is used when checking contains_block
Current Behavior:
New Behavior:
in some places we use the block_records cache for this and in some we use the height_to_hash
Testing Notes: