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

multiprocess_validation refactor #18541

Merged
merged 3 commits into from
Sep 23, 2024
Merged

multiprocess_validation refactor #18541

merged 3 commits into from
Sep 23, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Aug 30, 2024

Purpose:

This patch removes the thin wrapper pre_validate_blocks_multiprocess() method from the Blockchain class.
Specifically, this part of the patch: https://github.com/Chia-Network/chia-blockchain/pull/18541/files#diff-40409ea13958d9e8cec2be388c12e68da7c3179de55f01879713c5abdb1fed38L799-L825

The purpose of this wrapper is to pass the constants, self (as the BlocksProtocol) and ProcessPoolExecutor (also a member of Blockchain) into the underlying call to the free functionpre_validate_blocks_multiprocessing().

The reason to remove it is to prepare call sites for passing in other classes implementing the BlocksProtocol, such as AugmentedBlockchain, to support pipelining block validation during long sync.

The underlying free function takes more parameters, specifically, ConsensusConstants, BlocksProtocol and ProcessPoolExecutor. To reduce the impact of these additional parameters, two other parameters are removed; check_filter and batch_size. In all calls these are always set to True and 4, respectively. In fact, the batch_size can probably be removed (along with the batching itself) once validation is run in threads.

Current Behavior:

Many calls to pre_validate_blocks_multiprocessing() are made via the wrapper in Blockchain, passing in the blockchain object itself.

pre_validate_blocks_multiprocessing() takes parameters check_filter and batch_size.

New Behavior:

All calls to pre_validate_blocks_multiprocessing() explicitly pass in the object implementing the BlocksProtocol (e.g. Blockchain).

pre_validate_blocks_multiprocessing() no longer takes parameters check_filter and batch_size, but hard code those to True and 4.

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Aug 30, 2024
@arvidn arvidn force-pushed the multiprocess-validation branch 2 times, most recently from 9bae17c to 4a7b264 Compare August 30, 2024 14:17

This comment was marked as outdated.

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

This comment was marked as outdated.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 4, 2024

This comment was marked as outdated.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 6, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 7, 2024

This comment was marked as outdated.

Copy link

Pull Request Test Coverage Report for Build 10748783848

Details

  • 231 of 250 (92.4%) changed or added relevant lines in 12 files are covered.
  • 25 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.01%) to 90.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/consensus/multiprocess_validation.py 26 28 92.86%
chia/_tests/blockchain/test_augmented_chain.py 77 82 93.9%
chia/util/augmented_chain.py 76 88 86.36%
Files with Coverage Reduction New Missed Lines %
chia/timelord/timelord.py 1 81.78%
chia/rpc/rpc_server.py 1 88.82%
chia/consensus/get_block_generator.py 1 95.0%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/full_node/full_node.py 1 86.86%
chia/server/ws_connection.py 3 87.96%
chia/wallet/wallet_node.py 4 88.37%
chia/_tests/blockchain/test_get_block_generator.py 4 92.0%
chia/full_node/weight_proof.py 4 90.51%
chia/_tests/core/util/test_lockfile.py 5 89.47%
Totals Coverage Status
Change from base Build 10745304670: 0.01%
Covered Lines: 101681
Relevant Lines: 111810

💛 - Coveralls

This comment was marked as outdated.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 11, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 12, 2024

This comment was marked as outdated.

@arvidn arvidn changed the title slight multiprocess_validation refactor multiprocess_validation refactor Sep 12, 2024
@arvidn arvidn force-pushed the multiprocess-validation branch 2 times, most recently from 2285575 to b20def2 Compare September 12, 2024 06:48
…size is always 4, check_filter is always True. Remove the member function wrapper around it in Blockchain, to prepare for being able to pass wrapped blockchains into it
@arvidn arvidn marked this pull request as ready for review September 12, 2024 08:47
@arvidn arvidn requested a review from a team as a code owner September 12, 2024 08:47
Copy link
Contributor

@AmineKhaldi AmineKhaldi 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. Added some suggestions to keep the calls consistent and Blockchain oriented.

chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/full_node/full_node.py Outdated Show resolved Hide resolved
chia/full_node/full_node.py Outdated Show resolved Hide resolved
chia/simulator/full_node_simulator.py Outdated Show resolved Hide resolved
chia/simulator/full_node_simulator.py Outdated Show resolved Hide resolved
@arvidn arvidn requested a review from emlowe September 18, 2024 12:13
@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Sep 23, 2024
@pmaslana pmaslana merged commit 1e8c999 into main Sep 23, 2024
375 checks passed
@pmaslana pmaslana deleted the multiprocess-validation branch September 23, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants