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.

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

Merged
merged 32 commits into from
Feb 11, 2025

Conversation

almogdepaz
Copy link
Contributor

@almogdepaz almogdepaz commented Jan 14, 2025

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, as we switch to the fork blocks in the underlying blockchain once the new blocks pass the height of the old fork peak we clean the height_to_hash blocks from the augmented (since now they are present in the underlying height_to_hash)

Testing Notes:

@almogdepaz almogdepaz added full_node sync Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels Jan 14, 2025
@almogdepaz almogdepaz marked this pull request as ready for review January 27, 2025 17:27
@almogdepaz almogdepaz requested a review from a team as a code owner January 27, 2025 17:27
Copy link

coveralls-official bot commented Jan 27, 2025

Pull Request Test Coverage Report for Build 13111506377

Warning: 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

  • 43 of 56 (76.79%) changed or added relevant lines in 10 files are covered.
  • 25 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.009%) to 91.517%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/util/augmented_chain.py 13 15 86.67%
chia/util/block_cache.py 8 10 80.0%
chia/_tests/util/blockchain_mock.py 1 10 10.0%
Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.45%
chia/full_node/full_node_api.py 1 84.38%
chia/rpc/rpc_server.py 1 89.16%
chia/full_node/full_node.py 1 86.09%
chia/consensus/difficulty_adjustment.py 3 97.69%
chia/daemon/server.py 7 83.54%
chia/timelord/timelord.py 11 78.52%
Totals Coverage Status
Change from base Build 13019247618: 0.009%
Covered Lines: 105295
Relevant Lines: 114876

💛 - Coveralls

@almogdepaz almogdepaz requested a review from arvidn February 3, 2025 14:00
chia/wallet/wallet_blockchain.py Show resolved Hide resolved
chia/consensus/blockchain.py Outdated Show resolved Hide resolved
chia/full_node/full_node.py Show resolved Hide resolved
chia/util/augmented_chain.py Show resolved Hide resolved
chia/util/augmented_chain.py Show resolved Hide resolved
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

given that contains_block() is non-trivial now, it would probably be good to have a unit test

Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good, just a few comments/questions

chia/util/block_cache.py Outdated Show resolved Hide resolved
Copy link
Contributor

File Coverage Missing Lines
chia/_tests/util/blockchain_mock.py 20.0% lines 70-73
chia/util/augmented_chain.py 87.5% lines 50, 130
chia/util/block_cache.py 80.0% lines 49
Total Missing Coverage
88 lines 7 lines 92%

@pmaslana pmaslana merged commit 59583e7 into main Feb 11, 2025
359 of 360 checks passed
@pmaslana pmaslana deleted the assert-height-to-hash-in-contains-block branch February 11, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage-diff Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog full_node sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants