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

create_block_generator refactor #19207

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

create_block_generator refactor #19207

wants to merge 4 commits into from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jan 29, 2025

This PR is best reviewed one commit at a time.

Purpose:

Change the mempool interface to create a block generator, rather than a large spend bundle (to then be turned into a block generator)

The main reason for this is to prepare for changing the implementation of the block generator creation to use an incrementally compressed block creator, to fill block more effectively.

It has some other benefits I think:

  1. The spend bundle is an unnecessary step, skipping it should save some time and space
  2. It makes it easier to unit test the block generator creation (which currently requires interacting with full_node_api, and probably requires a full simulation environment.

The first commit is only tangentially related, and simplifies the function interface related to mempool item inclusion.

Current Behavior:

The mempool has a method: create_bundle_from_mempool_items()

New Behavior:

The mempool has a method: create_block_generator

benchmarks

This is not expected to affect performance, but the new function combines picking transactions and serializing the block, so it will take longer than just picking the transactions. The benchmarks are now:

Serializing the block with compression is especially expensive:

before:

Profiling create_bundle_from_mempool()
  output written to: mempool-create-mt.png
  time: 4.6944s
  per call: 9.39ms

after:

Profiling create_block_generator()
  output written to: mempool-create-mt.png
  time: 43.8237s
  per call: 876.47ms

@arvidn arvidn force-pushed the create_block_generator branch from f4c2342 to 6909222 Compare February 5, 2025 10:19
@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Feb 5, 2025
@arvidn arvidn changed the title Create block generator create_block_generator refactor Feb 5, 2025
@arvidn arvidn requested a review from AmineKhaldi February 5, 2025 10:20
@arvidn arvidn marked this pull request as ready for review February 5, 2025 10:24
@arvidn arvidn requested a review from a team as a code owner February 5, 2025 10:24
@arvidn arvidn force-pushed the create_block_generator branch from 6909222 to 3d9f473 Compare February 5, 2025 10:44
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, left some suggestions.

chia/_tests/core/mempool/test_mempool.py Outdated Show resolved Hide resolved
chia/full_node/mempool.py Show resolved Hide resolved
@arvidn arvidn force-pushed the create_block_generator branch from 3d9f473 to f225f82 Compare February 5, 2025 14:03
@arvidn arvidn closed this Feb 7, 2025
@arvidn arvidn reopened this Feb 7, 2025
@arvidn arvidn closed this Feb 10, 2025
@arvidn arvidn reopened this Feb 10, 2025
@arvidn arvidn force-pushed the create_block_generator branch from f225f82 to b3541df Compare February 10, 2025 15:08
@arvidn arvidn requested a review from wjblanke February 10, 2025 18:33
@arvidn arvidn closed this Feb 10, 2025
@arvidn arvidn reopened this Feb 10, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants