-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor get block generator #16335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor get block generator #16335
Changes from all commits
eeb8cbd
72ded77
a0425ff
9791d33
1544069
163564e
489bb26
97c70ad
4b438a4
e911183
b5e9ebf
777ccce
f5d57d5
c5682d5
73ab429
fffc28f
a1c62a7
07cbdfe
c99c512
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,11 +252,10 @@ async def add_block( | |
self, | ||
self.block_store, | ||
self.coin_store, | ||
self.get_peak(), | ||
block, | ||
block.height, | ||
npc_result, | ||
fork_point_with_peak, | ||
-1 if fork_point_with_peak is None else fork_point_with_peak, | ||
self.get_block_generator, | ||
# If we did not already validate the signature, validate it now | ||
validate_signature=not pre_validation_result.validated_signature, | ||
|
@@ -404,10 +403,12 @@ async def _reconsider_peak( | |
# We need to recompute the additions and removals, since they are not stored on DB (only generator is). | ||
if fetched_block_record.header_hash == block_record.header_hash: | ||
tx_removals, tx_additions, npc_res = await self.get_tx_removals_and_additions( | ||
fetched_full_block, npc_result | ||
fetched_full_block, fork_height, npc_result | ||
) | ||
else: | ||
tx_removals, tx_additions, npc_res = await self.get_tx_removals_and_additions(fetched_full_block, None) | ||
tx_removals, tx_additions, npc_res = await self.get_tx_removals_and_additions( | ||
fetched_full_block, fork_height | ||
) | ||
|
||
# Collect the NPC results for later post-processing | ||
if npc_res is not None: | ||
|
@@ -438,7 +439,7 @@ async def _reconsider_peak( | |
) | ||
|
||
async def get_tx_removals_and_additions( | ||
self, block: FullBlock, npc_result: Optional[NPCResult] = None | ||
self, block: FullBlock, fork_height: int, npc_result: Optional[NPCResult] = None | ||
) -> Tuple[List[bytes32], List[Coin], Optional[NPCResult]]: | ||
if not block.is_transaction_block(): | ||
return [], [], None | ||
|
@@ -447,7 +448,7 @@ async def get_tx_removals_and_additions( | |
return [], [], None | ||
|
||
if npc_result is None: | ||
block_generator: Optional[BlockGenerator] = await self.get_block_generator(block) | ||
block_generator: Optional[BlockGenerator] = await self.get_block_generator(block, fork_height) | ||
assert block_generator is not None | ||
npc_result = get_name_puzzle_conditions( | ||
block_generator, | ||
|
@@ -625,11 +626,10 @@ async def validate_unfinished_block( | |
self, | ||
self.block_store, | ||
self.coin_store, | ||
self.get_peak(), | ||
block, | ||
uint32(prev_height + 1), | ||
npc_result, | ||
None, | ||
prev_height, | ||
self.get_block_generator, | ||
validate_signature=False, # Signature was already validated before calling this method, no need to validate | ||
) | ||
|
@@ -647,6 +647,7 @@ async def pre_validate_blocks_multiprocessing( | |
wp_summaries: Optional[List[SubEpochSummary]] = None, | ||
*, | ||
validate_signatures: bool, | ||
fork_height: Optional[uint32] = None, | ||
) -> List[PreValidationResult]: | ||
return await pre_validate_blocks_multiprocessing( | ||
self.constants, | ||
|
@@ -657,6 +658,7 @@ async def pre_validate_blocks_multiprocessing( | |
npc_results, | ||
self.get_block_generator, | ||
batch_size, | ||
fork_height, | ||
wp_summaries, | ||
validate_signatures=validate_signatures, | ||
) | ||
|
@@ -882,7 +884,10 @@ def seen_compact_proofs(self, vdf_info: VDFInfo, height: uint32) -> bool: | |
return False | ||
|
||
async def get_block_generator( | ||
self, block: BlockInfo, additional_blocks: Optional[Dict[bytes32, FullBlock]] = None | ||
self, | ||
block: BlockInfo, | ||
fork: Optional[int] = None, | ||
additional_blocks: Optional[Dict[uint32, FullBlock]] = None, | ||
) -> Optional[BlockGenerator]: | ||
if additional_blocks is None: | ||
additional_blocks = {} | ||
|
@@ -905,56 +910,32 @@ async def get_block_generator( | |
# (as long as we're in the main chain) | ||
result = await self.block_store.get_generators_at(block.transactions_generator_ref_list) | ||
else: | ||
# First tries to find the blocks in additional_blocks | ||
reorg_chain: Dict[uint32, FullBlock] = {} | ||
curr = block | ||
additional_height_dict = {} | ||
while curr.prev_header_hash in additional_blocks: | ||
prev: FullBlock = additional_blocks[curr.prev_header_hash] | ||
additional_height_dict[prev.height] = prev | ||
if isinstance(curr, FullBlock): | ||
assert curr.height == prev.height + 1 | ||
reorg_chain[prev.height] = prev | ||
curr = prev | ||
|
||
peak: Optional[BlockRecord] = self.get_peak() | ||
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 | ||
Comment on lines
-921
to
-935
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still need, at least, the height-to-hash mapping that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think what confused me is the if statement, we assume curr would be in the current chain ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by I think the intention of that if-statement, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ? |
||
|
||
assert fork is not None | ||
almogdepaz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
prev_full_block = await self.block_store.get_full_block(block.prev_header_hash) | ||
assert prev_full_block is not None | ||
reorg_chain_height_to_hash = {} | ||
block_recs = await self.block_store.get_block_records_in_range(fork, prev_full_block.height) | ||
curr = block_recs[prev_full_block.header_hash] | ||
while curr.height > fork and curr.height > 0: | ||
reorg_chain_height_to_hash[curr.height] = curr.header_hash | ||
|
||
# todo aggregate heights and hashes to one query | ||
for ref_height in block.transactions_generator_ref_list: | ||
if ref_height in reorg_chain: | ||
ref_block = reorg_chain[ref_height] | ||
if ref_height in additional_blocks: | ||
ref_block = additional_blocks[ref_height] | ||
assert ref_block is not None | ||
if ref_block.transactions_generator is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
result.append(ref_block.transactions_generator) | ||
if ref_height > fork: | ||
gen = await self.block_store.get_generator(reorg_chain_height_to_hash[ref_height]) | ||
if gen is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
result.append(gen) | ||
else: | ||
if ref_height in additional_height_dict: | ||
ref_block = additional_height_dict[ref_height] | ||
assert ref_block is not None | ||
if ref_block.transactions_generator is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
result.append(ref_block.transactions_generator) | ||
else: | ||
header_hash = self.height_to_hash(ref_height) | ||
if header_hash is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
gen = await self.block_store.get_generator(header_hash) | ||
if gen is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
result.append(gen) | ||
[gen] = await self.block_store.get_generators_at([ref_height]) | ||
if gen is None: | ||
raise ValueError(Err.GENERATOR_REF_HAS_NO_GENERATOR) | ||
result.append(gen) | ||
assert len(result) == len(ref_list) | ||
return BlockGenerator(block.transactions_generator, result, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 spaceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by "FullNode" objects?