Assert height to hash in contains block #19136
Conversation
almogdepaz
added
full_node
sync
Fixed
Pull Request Test Coverage Report for Build 13111506377Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
| """ | ||
| True if we have already added this block to the chain. This may return false for orphan blocks | ||
| that we have added but no longer keep in memory. | ||
| """ |
There was a problem hiding this comment.
I think it's important to document this on the full node side as well. Perhaps this wallet counterpart will be removed or disconnected from the full node eventually
There was a problem hiding this comment.
but this is no longer true for the full node, after this pr contains block will return true/false according to the height to hash and not the cache
chia/consensus/blockchain.py
Outdated
| 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.
this seems a bit risky, don't you think?
The caller think they're asking if the block is part of the main chain, but gets false if it's not in the cache, even if it is part of the main chain.
This should be clearly documented.
| @@ -619,6 +619,7 @@ async def short_sync_batch(self, peer: WSChiaConnection, start_height: uint32, t | |||
| fork_hash = self.constants.GENESIS_CHALLENGE | |||
| assert fork_hash | |||
| fork_info = ForkInfo(start_height - 1, start_height - 1, fork_hash) | |||
| blockchain = AugmentedBlockchain(self.blockchain) | |||
There was a problem hiding this comment.
on line 640, we create another blockchain = AugmentedBlockchain(). It looks like a mistake
There was a problem hiding this comment.
right this is from the merge with my other pr that went into main, thanks
| @@ -47,8 +47,7 @@ def add_extra_block(self, block: FullBlock, block_record: BlockRecord) -> None: | |||
|
|
|||
| def remove_extra_block(self, hh: bytes32) -> None: | |||
| if hh in self._extra_blocks: | |||
| block_record = self._extra_blocks.pop(hh)[1] | |||
| del self._height_to_hash[block_record.height] | |||
There was a problem hiding this comment.
it's not obvious why you removed this line. Don't we need to clear the _height_to_hash entry?
There was a problem hiding this comment.
no, the point of this pr is for the augmented chain to contain the alternate height_to_hash, remove extra block is called once we validated the block but not necessarily when we added it to the height to height to hash, so if i remove from the height to hash there will be blocks that are not in the augmented chain height to hash and not in the underlying chain height to hash
| @@ -109,6 +107,7 @@ def height_to_block_record(self, height: uint32) -> BlockRecord: | |||
| ret = self._get_block_record(header_hash) | |||
| if ret is not None: | |||
| return ret | |||
| return self._underlying.block_record(header_hash) | |||
There was a problem hiding this comment.
I suppose you can do this now that _height_to_hash grows indefinitely. But what's the upside of this?
There was a problem hiding this comment.
i have to do this, since we remove from extra_blocks once we validate and move to the underlying cache there might be a block in the augmented height_to_hash that was moved
Purpose:
use the height to hash when checking for "contains_block" block that way we always get the relevant block for the chain being validated
and create better separation between Augmentedblockchain and the underlining blockchain.
Current Behavior:
we call contains block that checks the block_record cache, this will return true if the block is in the cache regardless if is relevant to the chain
New Behavior:
contains_block will check the height_to_hash, the AugmentedBlockchain will return true for blocks that are in the fork and the underlining will return true only for blocks that are in the current chain
Testing Notes: