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

refactor pre_validate_blocks_multiprocessing #18469

Merged
merged 22 commits into from
Sep 6, 2024

Conversation

almogdepaz
Copy link
Contributor

@almogdepaz almogdepaz commented Aug 14, 2024

Purpose:

this pr simplifies pre_validate_blocks_multiprocessing by passing in the prev sub epoch block the sub slot iters and the difficulty, moving the calls to get_next_sub_slot_iters_and_difficulty outside

this means that the sub slot iters and difficulty are not validates inside pre_validate_blocks_multiprocessing so we still need to call get_next_sub_slot_iters_and_difficulty and compare once every epoch since that is the place in the code where we validate the sub slot iters and the difficulty using the timestamps from the chain

New Behavior:

no change to behavior

Testing Notes:

@almogdepaz almogdepaz added full_node Cleanup Code cleanup Blockchain team Issues tagged for Blockchain team to work on Performance labels Aug 14, 2024
@almogdepaz almogdepaz added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Aug 14, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 19, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 19, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 21, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 27, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

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.

without my proposed changes, the failure results in throwing an IndexError, rather than the expected INVALID_PREV_BLOCK_HASH error return value

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 3, 2024
Copy link
Contributor

github-actions bot commented Sep 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Sep 4, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 4, 2024
@almogdepaz almogdepaz marked this pull request as ready for review September 4, 2024 11:24
@almogdepaz almogdepaz requested a review from a team as a code owner September 4, 2024 11:24
@arvidn
Copy link
Contributor

arvidn commented Sep 4, 2024

you might want to rebase on top of refactor-get-block-generator in advance, to make sure it's a clean merge

@arvidn arvidn closed this Sep 6, 2024
@arvidn arvidn reopened this Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

File Coverage Missing Lines
chia/consensus/blockchain.py 88.2% lines 346, 349
chia/consensus/multiprocess_validation.py 97.4% lines 233
chia/full_node/full_node.py 88.4% lines 1422, 1443-1444, 1448, 1450, 1458-1462
Total Missing Coverage
227 lines 13 lines 94%

Copy link

Pull Request Test Coverage Report for Build 10743333697

Details

  • 213 of 226 (94.25%) changed or added relevant lines in 17 files are covered.
  • 29 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.02%) to 90.908%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/consensus/blockchain.py 14 15 93.33%
chia/consensus/multiprocess_validation.py 37 39 94.87%
chia/full_node/full_node.py 76 86 88.37%
Files with Coverage Reduction New Missed Lines %
chia/data_layer/data_store.py 1 94.58%
chia/consensus/multiprocess_validation.py 1 94.07%
chia/rpc/rpc_server.py 1 89.14%
chia/daemon/client.py 1 73.94%
chia/full_node/weight_proof.py 4 90.51%
chia/server/server.py 4 82.21%
chia/full_node/full_node.py 8 86.16%
chia/server/node_discovery.py 9 79.43%
Totals Coverage Status
Change from base Build 10742893367: -0.02%
Covered Lines: 101582
Relevant Lines: 111707

💛 - Coveralls

@pmaslana
Copy link
Contributor

pmaslana commented Sep 6, 2024

@arvidn @almogdepaz Is it ok to remove the coverage_diff label and merge?

@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready and removed coverage-diff labels Sep 6, 2024
@pmaslana pmaslana merged commit 9559295 into main Sep 6, 2024
737 of 739 checks passed
@pmaslana pmaslana deleted the refactor-multiproc-validation branch September 6, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blockchain team Issues tagged for Blockchain team to work on Cleanup Code cleanup Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog full_node Performance ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants