refactor get block generator #16335
refactor get block generator #16335
Conversation
almogdepaz
added
full_node
tech-debt
Fixed
pass fork_point_with_peak pr fixes
| if self.contains_block(curr.prev_header_hash) and peak is not None: | ||
| # Then we look up blocks up to fork point one at a time, backtracking | ||
| previous_block_hash = curr.prev_header_hash | ||
| prev_block_record = await self.block_store.get_block_record(previous_block_hash) | ||
| prev_block = await self.block_store.get_full_block(previous_block_hash) | ||
| assert prev_block is not None | ||
| assert prev_block_record is not None | ||
| fork = find_fork_point_in_chain(self, peak, prev_block_record) | ||
| curr_2: Optional[FullBlock] = prev_block | ||
| assert curr_2 is not None and isinstance(curr_2, FullBlock) | ||
| reorg_chain[curr_2.height] = curr_2 | ||
| while curr_2.height > fork and curr_2.height > 0: | ||
| curr_2 = await self.block_store.get_full_block(curr_2.prev_header_hash) | ||
| assert curr_2 is not None | ||
| reorg_chain[curr_2.height] = curr_2 |
There was a problem hiding this comment.
we still need, at least, the height-to-hash mapping that reorg_chain provides. I don't think you can remove this whole seciont
There was a problem hiding this comment.
ok think i get what wrong here.
we traverse all the heights in the end anyway and fetch the missing blocks from db but we do that by height so might get the wrong one, so i just need the mapping to get the hashes this code still redundant, we dont need all the full blocks up to the fork we only need the referenced ones
There was a problem hiding this comment.
i think what confused me is the if statement, we assume curr would be in the current chain ?
after that i get that we would get the blocks from the reorg_chain but why would we even get into the if, we dont expect block_records to contain blocks from the reorg
There was a problem hiding this comment.
by block_records, you mean the Blockchain object, right? The self. It has access to all blocks, including orphaned blocks and other forks. If we're in a reorg, those blocks are in the Blockchain object as well.
I think the intention of that if-statement, if self.contains_block(curr.prev_header_hash) was to ask if we know of that block, not if we happen to have it in the cache right now. If we assume the intention was the latter, there's another block of code missing here to do the same thing but pull the block hashes from the database.
the additional_blocks is just the current batch of block's we're validating. It does not contain the whole fork. The fork is still only stored in the database at this point, that's where we need to get the generators from.
There was a problem hiding this comment.
then i dont get the point of this check, im pretty sure the initial intention was to see if its in the chain
otherwise we are just checking the store to contain something that we expect to be there, in that case i would have expected an assert or some exception being thrown, what is the case then when the block not in blockchain.blockrecords ?
github-actions
bot
added
the
merge_conflict
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
# Conflicts: # tests/core/full_node/stores/test_full_node_store.py
fix assertion in block body validation
github-actions
bot
removed
merge_conflict
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
1 similar comment
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
There was a problem hiding this comment.
we use two different representations for fork-height in the code base. one is Optional[uint32], where None either means the chains don't share any blocks, or it means we don't know what the fork height is. The other is int, where -1 means the chains don't share any blocks.
In the case of Optional, I think there's a risk of confusion what None really means. The case of int makes arithmetic simpler, because -1 naturally fits in to comparing heights.
I think it would be good to get some clarity on exactly what semantics the code ascribes to the different values of fork height, and maybe try to unify them.
| @@ -42,7 +42,7 @@ async def validate_block_body( | |||
| height: uint32, | |||
| npc_result: Optional[NPCResult], | |||
| fork_point_with_peak: int, | |||
| get_block_generator: Callable[[BlockInfo, uint32], Awaitable[Optional[BlockGenerator]]], | |||
| get_block_generator: Callable[[BlockInfo, int, Dict[uint32, FullBlock]], Awaitable[Optional[BlockGenerator]]], | |||
There was a problem hiding this comment.
I think we should bundle the fork_height and this height->block map into a class. We'll need to extend this with passing in the unspent coin set of the fork as well.
Also, I would worry about storing all the FullNode objects in memory. We're not likely to actually need all of that, and it can take up a lot of space
There was a problem hiding this comment.
what do you mean by "FullNode" objects?
github-actions
bot
added
the
merge_conflict
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties. |
github-actions
bot
added
the
stale-pr
Purpose:
Current Behavior:
New Behavior:
Testing Notes: