Skip to content

Refactor BlockTools #19492

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

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

Refactor BlockTools #19492

wants to merge 9 commits into from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Apr 9, 2025

Purpose:

Simplify BlockTools to make it easier and more robust to additions and changes.

This PR is best reviewed one commit at a time.

One realization (that's mentioned in a new comment in this PR) is that we don't know whether a block will end up a transaction block or not up-front. We always prepare the transactions and pass into get_full_block_and_block_record(), only afterwards do we know whether the transactions were included or not.

The include_transactions feature (which includes some number of transactions in each TX block) maintains a list of available_coins that it spends some of, and add the coins created by those spends. These spends used to rely on another feature transaction_data (which can also be passed in by the caller), in order to "wait" until we get a transaction block to make the next set of transactions.

This patch removes this coupling and instead explicitly updates available_coins every time we get a transaction block. This makes the transactions not be created identically to the current chains and causes validation failures. That's why this needs an updated test-cache version of the blockchains. (available here)

Current Behavior:

  • there is a lot of code that's duplicated in get_consecutive_blocks().
  • there are unintuitive (and difficult to follow) dependencies between loop iterations when generating multiple blocks
    • include_transactions feature generate dummy transactions by setting transaction_data
    • transaction_data_included is set to true when transaction_data is included into a block that turned out to be a transaction block. We then set transaction_data to None to not include it again.
    • the start_height parameter was unused
  • when generating transactions, we track available_coins and we remove the coins immediately when we create the bundle. This means we must keep the bundle around until the generated blocks turns out to be a transaction block (hence the dependency on transaction_data.

New Behavior:

  • there is slightly less code duplicated (some code was factored out into setup_new_gen())
  • there code to generate blocks is more insulated and has fewer unexpected dependencies. For example
    • the transaction_data feature is now independent from include_transactions feature.
    • transaction_data_included was removed. transaction_data is set to None directly
    • the start_height parameter was removed
  • when generating transactions, we track available_coins and we remove coins we spent if the block turned into a transaction block

Testing Notes:

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Apr 9, 2025
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Apr 9, 2025
Copy link
Contributor

github-actions bot commented Apr 9, 2025

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

Copy link
Contributor

github-actions bot commented Apr 9, 2025

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 Apr 9, 2025
@arvidn arvidn marked this pull request as ready for review April 10, 2025 06:51
@arvidn arvidn requested review from a team as code owners April 10, 2025 06:51
@arvidn arvidn closed this Apr 11, 2025
@arvidn arvidn reopened this Apr 11, 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.

3 participants