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

[CHIA-786] remove the original block compression #18209

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jun 20, 2024

Purpose:

Now that the hard fork has activated and we serialize CLVM in a more efficient way, we can remove the old code to simplify our code.

The original compression deduplicates vanilla transactions, only in blocks made up of only such transactions.
To properly take advantage of the block references, the farmer would need to maintain an index of blocks containing popular puzzle reveals. I don't believe any of the old infrastructure is suitable to be re-worked into such feature, and it's also not on our roadmap to implement any time soon.

Current Behavior:

When farming blocks prior to the hard fork, we may replace a block of only vanilla puzzle reveals with a block reference.

New Behavior:

When farming blocks prior to the hard fork, we always produce a plain block, with all the puzzle reveals.

@arvidn arvidn changed the title remove the original form block compression remove the original block compression Jun 20, 2024
@arvidn arvidn changed the title remove the original block compression [CHIA-786] remove the original block compression Jun 20, 2024
@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 20, 2024
@arvidn arvidn requested review from richardkiss and aqk June 20, 2024 09:12
…s activated and we serialize CLVM in a more efficient way
Copy link

coveralls-official bot commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9596253668

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 8 files are covered.
  • 37 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.04%) to 90.84%

Files with Coverage Reduction New Missed Lines %
chia/_tests/generator/test_compression.py 1 93.94%
chia/rpc/rpc_server.py 1 88.7%
chia/wallet/wallet_node.py 1 88.94%
chia/plotters/madmax.py 1 50.6%
chia/full_node/full_node.py 4 85.46%
chia/daemon/server.py 7 83.31%
chia/_tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 9571014256: -0.04%
Covered Lines: 99956
Relevant Lines: 109974

💛 - Coveralls

@arvidn arvidn closed this Jun 20, 2024
@arvidn arvidn reopened this Jun 20, 2024
@arvidn arvidn marked this pull request as ready for review June 20, 2024 15:20
@arvidn arvidn requested a review from a team as a code owner June 20, 2024 15:20
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 really understand this code well. But what I'd expect is for stuff to be deleted only, and that's pretty much what I see. And it doesn't seem to affect consensus. So from that perspective, it seems good.

@arvidn arvidn requested a review from wjblanke July 8, 2024 16:33
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

@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Jul 9, 2024
@arvidn arvidn closed this Jul 9, 2024
@arvidn arvidn reopened this Jul 9, 2024
Copy link

Pull Request Test Coverage Report for Build 9858557039

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

  • 21 of 21 (100.0%) changed or added relevant lines in 8 files are covered.
  • 9 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.02%) to 90.954%

Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.5%
chia/_tests/generator/test_compression.py 1 93.94%
chia/daemon/keychain_proxy.py 1 72.57%
chia/rpc/rpc_server.py 1 87.71%
chia/farmer/farmer.py 1 72.23%
chia/full_node/full_node.py 1 86.08%
chia/daemon/client.py 1 73.33%
chia/wallet/wallet_node.py 2 89.01%
Totals Coverage Status
Change from base Build 9850517077: 0.02%
Covered Lines: 101390
Relevant Lines: 111426

💛 - Coveralls

@Starttoaster Starttoaster merged commit b4290a2 into main Jul 9, 2024
1071 of 1075 checks passed
@Starttoaster Starttoaster deleted the old-compression branch July 9, 2024 19:16
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