Skip to content
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.

Sign up for GitHub

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

add assertions to contains block #19001

Closed
wants to merge 8 commits into from
Closed

add assertions to contains block #19001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e46b2bc
add assertions
almogdepaz Dec 9, 2024
e06555f
use_height_if_available
almogdepaz Dec 9, 2024
0fcbac1
use try_block_record
almogdepaz Dec 9, 2024
74b27f4
fix recursion
almogdepaz Dec 9, 2024
b229e7e
handle none
almogdepaz Dec 9, 2024
6feaf13
remove assert
almogdepaz Dec 9, 2024
9c1eab1
remove assert
almogdepaz Dec 9, 2024
14c3339
use try in wallet blockchain
almogdepaz Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chia/_tests/blockchain/test_augmented_chain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def height_to_block_record(self, height: uint32) -> BlockRecord:
def height_to_hash(self, height: uint32) -> Optional[bytes32]:
return self.heights.get(height)

def contains_block(self, header_hash: bytes32) -> bool:
def contains_block(self, header_hash: bytes32, height: Optional[uint32] = None) -> bool:
return False # pragma: no cover

def contains_height(self, height: uint32) -> bool:
Expand Down
16 changes: 14 additions & 2 deletions chia/_tests/util/blockchain_mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,20 @@ def height_to_hash(self, height: uint32) -> Optional[bytes32]:
assert height in self._height_to_hash
return self._height_to_hash[height]

def contains_block(self, header_hash: bytes32) -> bool:
return header_hash in self._block_records
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
that we have added but no longer keep in memory.
"""
if height is None:
block_rec = self.try_block_record(header_hash)
if block_rec is None:
return False
height = block_rec.height
block_hash_from_hh = self.height_to_hash(height)
if block_hash_from_hh is None or block_hash_from_hh != header_hash:
return False
return True

async def contains_block_from_db(self, header_hash: bytes32) -> bool:
return header_hash in self._block_records
Expand Down
14 changes: 11 additions & 3 deletions chia/consensus/blockchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

that we have added but no longer keep in memory.
"""
return header_hash in self.__block_records
if height is None:
block_rec = self.try_block_record(header_hash)
if block_rec is None:
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also return false for blocks that are part of the main chain, but no longer in the cache.

height = block_rec.height
block_hash_from_hh = self.height_to_hash(height)
if block_hash_from_hh is None or block_hash_from_hh != header_hash:
return False
Comment on lines +783 to +784
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really make sense to me in the case of the short_sync methods or even for add block

return True

def block_record(self, header_hash: bytes32) -> BlockRecord:
return self.__block_records[header_hash]
Expand Down Expand Up @@ -932,7 +940,7 @@ async def get_block_records_at(self, heights: list[uint32], batch_size: int = 90
return records

def try_block_record(self, header_hash: bytes32) -> Optional[BlockRecord]:
if self.contains_block(header_hash):
if header_hash in self.__block_records:
return self.block_record(header_hash)
Comment on lines +943 to 944
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return None

Expand Down
2 changes: 1 addition & 1 deletion chia/consensus/blockchain_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class BlockRecordsProtocol(Protocol):
def try_block_record(self, header_hash: bytes32) -> Optional[BlockRecord]: ...
def block_record(self, header_hash: bytes32) -> BlockRecord: ...
def contains_height(self, height: uint32) -> bool: ...
def contains_block(self, header_hash: bytes32) -> bool: ...
def contains_block(self, header_hash: bytes32, height: Optional[uint32] = None) -> bool: ...
def height_to_hash(self, height: uint32) -> Optional[bytes32]: ...
def height_to_block_record(self, height: uint32) -> BlockRecord: ...

Expand Down
13 changes: 8 additions & 5 deletions chia/full_node/full_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I guess we expect both in this case.

self.log.info("Batch syncing stopped, this is a deep chain")
self.sync_store.batch_syncing.remove(peer.peer_node_id)
# First sb not connected to our blockchain, do a long sync instead
Expand Down Expand Up @@ -699,7 +699,10 @@ async def short_sync_backtrack(
f"Failed to fetch block {curr_height} from {peer.get_peer_logging()}, wrong type {type(curr)}"
)
blocks.append(curr.block)
if self.blockchain.contains_block(curr.block.prev_header_hash) or curr_height == 0:
if (
self.blockchain.contains_block(curr.block.prev_header_hash, curr.block.height - 1)
or curr_height == 0
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this call site is not concerned with whether the block is in the cache or not

found_fork_point = True
break
curr_height -= 1
Expand Down Expand Up @@ -748,7 +751,7 @@ async def new_peak(self, request: full_node_protocol.NewPeak, peer: WSChiaConnec
# Store this peak/peer combination in case we want to sync to it, and to keep track of peers
self.sync_store.peer_has_block(request.header_hash, peer.peer_node_id, request.weight, request.height, True)

if self.blockchain.contains_block(request.header_hash):
if self.blockchain.contains_block(request.header_hash, request.height):
return None

# Not interested in less heavy peaks
Expand Down Expand Up @@ -2006,7 +2009,7 @@ async def add_block(

# Adds the block to seen, and check if it's seen before (which means header is in memory)
header_hash = block.header_hash
if self.blockchain.contains_block(header_hash):
if self.blockchain.contains_block(header_hash, block.height):
return None

pre_validation_result: Optional[PreValidationResult] = None
Expand Down Expand Up @@ -2078,7 +2081,7 @@ async def add_block(
enable_profiler(self.profile_block_validation) as pr,
):
# After acquiring the lock, check again, because another asyncio thread might have added it
if self.blockchain.contains_block(header_hash):
if self.blockchain.contains_block(header_hash, block.height):
return None
validation_start = time.monotonic()
# Tries to add the block to the blockchain, if we already validated transactions, don't do it again
Expand Down
14 changes: 12 additions & 2 deletions chia/util/augmented_chain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,18 @@ def height_to_hash(self, height: uint32) -> Optional[bytes32]:
return ret
return self._underlying.height_to_hash(height)

def contains_block(self, header_hash: bytes32) -> bool:
return (header_hash in self._extra_blocks) or self._underlying.contains_block(header_hash)
def contains_block(self, header_hash: bytes32, height: Optional[uint32] = None) -> bool:
if height is None:
block_rec = self.try_block_record(header_hash)
if block_rec is None:
return False
height = block_rec.height
if not self.contains_height(height):
return False
block_hash_from_hh = self.height_to_hash(height)
if block_hash_from_hh is None or block_hash_from_hh != header_hash:
return False
return True

def contains_height(self, height: uint32) -> bool:
return (height in self._height_to_hash) or self._underlying.contains_height(height)
Expand Down
16 changes: 14 additions & 2 deletions chia/util/block_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,20 @@ def height_to_hash(self, height: uint32) -> Optional[bytes32]:
return None
return self._height_to_hash[height]

def contains_block(self, header_hash: bytes32) -> bool:
return header_hash in self._block_records
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
that we have added but no longer keep in memory.
"""
if height is None:
block_rec = self.try_block_record(header_hash)
if block_rec is None:
return False
height = block_rec.height
block_hash_from_hh = self.height_to_hash(height)
if block_hash_from_hh is None or block_hash_from_hh != header_hash:
return False
return True

def contains_height(self, height: uint32) -> bool:
return height in self._height_to_hash
Expand Down
16 changes: 14 additions & 2 deletions chia/wallet/wallet_blockchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,20 @@ async def get_finished_sync_up_to(self) -> uint32:
def get_latest_timestamp(self) -> uint64:
return self._latest_timestamp

def contains_block(self, header_hash: bytes32) -> bool:
return header_hash in self._block_records
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
that we have added but no longer keep in memory.
"""
if height is None:
block_rec = self.try_block_record(header_hash)
if block_rec is None:
return False
height = block_rec.height
block_hash_from_hh = self.height_to_hash(height)
if block_hash_from_hh is None or block_hash_from_hh != header_hash:
return False
return True

def contains_height(self, height: uint32) -> bool:
return height in self._height_to_hash
Expand Down
Loading