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

run_block_generator2() #16241

Merged
merged 4 commits into from
Sep 12, 2023
Merged

run_block_generator2() #16241

merged 4 commits into from
Sep 12, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Sep 5, 2023

This PR is best reviewed one commit at a time.

This updates the hard-fork to use run_block_generator2() as intended. this alters the cost block generators as the cost of the ROM itself is not charged (once the hard fork activates)

The new run_block_generator2() can be reviewed here: https://github.com/Chia-Network/chia_rs/blob/main/src/gen/run_block_generator.rs#L111-L191

Since testnet10 has already activated the hard-fork, this fix needs a separate activation height there. In mainnet, the activation stays the same as the original hard fork.

There are new pre-generated test blockchains in the test-cache repository, with the _harfork suffix.

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

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 5, 2023
@github-actions

This comment was marked as outdated.

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Sep 5, 2023
@github-actions

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, 2023
@github-actions

This comment was marked as outdated.

1 similar comment
@github-actions

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 6, 2023
@github-actions

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 8, 2023
@github-actions

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 8, 2023
@github-actions

This comment was marked as outdated.

@arvidn arvidn marked this pull request as ready for review September 8, 2023 06:36
@arvidn arvidn requested a review from a team as a code owner September 8, 2023 06:36
@coveralls-official
Copy link

Pull Request Test Coverage Report for Build 6122871321

  • 66 of 67 (98.51%) changed or added relevant lines in 9 files are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.01%) to 89.139%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/core/mempool/test_mempool.py 9 10 90.0%
Files with Coverage Reduction New Missed Lines %
chia/timelord/timelord.py 1 80.39%
chia/wallet/wallet_node.py 1 87.09%
chia/full_node/full_node.py 6 85.96%
Totals Coverage Status
Change from base Build 6118026788: 0.01%
Covered Lines: 86477
Relevant Lines: 96958

💛 - Coveralls

@arvidn
Copy link
Contributor Author

arvidn commented Sep 8, 2023

I'm removing coverage-diff label. the one missing line points into a benchmark, which we don't instrument for coverage

richardkiss
richardkiss previously approved these changes Sep 8, 2023
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

I don't quite understand the intricacies of all the new test branches, although I do have the idea that they have to do with different costs for the new run_block_generator2. I assume the new costs are always lower than the old costs, which will give us a slightly simpler hard fork discontinuity (non-smoothness) of expecting the committed costs to be below the actual block_generator2 costs (maybe even by a constant?).

tests/conftest.py Outdated Show resolved Hide resolved
@arvidn
Copy link
Contributor Author

arvidn commented Sep 9, 2023

I assume the new costs are always lower than the old costs, which will give us a slightly simpler hard fork discontinuity (non-smoothness) of expecting the committed costs to be below the actual block_generator2 costs

correct.

(maybe even by a constant?)

it won't be constant. the most expensive thing the generator ROM does is to compute the puzzle hashes of puzzles. This will be free after the hard-fork, so the "savings" depend on how expensive the tree-hashes are, essentially.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

File Coverage Missing Lines
tests/core/mempool/test_mempool.py 90.0% lines 2493
Total Missing Coverage
73 lines Unknown 98%

@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready and removed coverage-diff labels Sep 11, 2023
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok thanks guys!

@cmmarslender cmmarslender merged commit fd51693 into main Sep 12, 2023
207 of 208 checks passed
@cmmarslender cmmarslender deleted the block-generator2 branch September 12, 2023 16:27
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.

None yet

4 participants