-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Assert height to hash in contains block #19136
base: main
Are you sure you want to change the base?
Changes from all commits
e46b2bc
e06555f
0fcbac1
74b27f4
b229e7e
6feaf13
9c1eab1
14c3339
e0cd0cd
fed3e6e
31bc5b0
1437e4e
4f144a6
5a918d7
5cf019f
4215992
0ccadb5
a48ce05
bf03d6d
1ab95bc
8c3271d
c85adc4
4533711
ec8ef12
30d0323
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 |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not obvious why you removed this line. Don't we need to clear 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. 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 |
||
self._extra_blocks.pop(hh)[1] | ||
|
||
# BlocksProtocol | ||
async def lookup_block_generators(self, header_hash: bytes32, generator_refs: set[uint32]) -> dict[uint32, bytes]: | ||
|
@@ -82,12 +81,11 @@ async def get_block_record_from_db(self, header_hash: bytes32) -> Optional[Block | |
|
||
def add_block_record(self, block_record: BlockRecord) -> None: | ||
self._underlying.add_block_record(block_record) | ||
|
||
self._height_to_hash[block_record.height] = block_record.header_hash | ||
# now that we're adding the block to the underlying blockchain, we don't | ||
# need to keep the extra block around anymore | ||
hh = block_record.header_hash | ||
if hh in self._extra_blocks: | ||
del self._height_to_hash[block_record.height] | ||
del self._extra_blocks[hh] | ||
|
||
# BlockRecordsProtocol | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose you can do this now 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. 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 |
||
return self._underlying.height_to_block_record(height) | ||
|
||
def height_to_hash(self, height: uint32) -> Optional[bytes32]: | ||
|
@@ -117,8 +116,11 @@ 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: uint32) -> bool: | ||
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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,7 +192,11 @@ 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: | ||
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. | ||
""" | ||
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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return header_hash in self._block_records | ||
|
||
def contains_height(self, height: uint32) -> bool: | ||
|
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.
on line 640, we create another
blockchain = AugmentedBlockchain()
. It looks like a mistakeThere 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.
right this is from the merge with my other pr that went into main, thanks