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-1945 Avoid computing a transactions filter with just reward coins when the filter is not requested from get_block_header #18969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmineKhaldi
Copy link
Contributor

@AmineKhaldi AmineKhaldi commented Dec 3, 2024

Purpose:

We tend to use get_block_header in paths where we don't care about the transactions filter, by passing empty additions and removals, yet when a transaction block contains reward coins, we end up computing a non-empty one. Now make it explicit that we care about the transactions filter by sending a tuple of additions and removals.

Current Behavior:

We generate a non-empty transactions filter when we get called with a transaction block that has reward coins, even if we pass empty additions and removals, which we usually do to signal not caring about the filter.

New Behavior:

We generate an empty transactions filter unless we're passed a tuple of additions and removals, in which case we account for them, as well as reward coins, in the computation of the transactions filter.

@AmineKhaldi AmineKhaldi added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Dec 3, 2024
@AmineKhaldi AmineKhaldi self-assigned this Dec 3, 2024
@AmineKhaldi AmineKhaldi marked this pull request as ready for review December 4, 2024 11:09
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner December 4, 2024 11:09
@arvidn
Copy link
Contributor

arvidn commented Dec 6, 2024

what do you think of passing Optional[tuple[Collection[Coin], Collection[Coin]]] instead? That way, you explicitly pass None as additions and removals to indicate that you're not interested in the transaction filter.

One issue with adding a separate tx_filter: bool parameter is that you can pass in empty lists (because you don't care) but forget to pass in tx_filter=False.

… filter is not requested from get_block_header.
@AmineKhaldi AmineKhaldi force-pushed the get_block_header_tx_filter branch from b7cbfd1 to 67dcaf9 Compare December 11, 2024 14:42
@AmineKhaldi AmineKhaldi changed the title CHIA-1945 Accept a tx_filter param in get_block_header and exclude reward coins when the filter is not requested CHIA-1945 Avoid computing a transactions filter with just reward coins when the filter is not requested from get_block_header Dec 11, 2024
@AmineKhaldi
Copy link
Contributor Author

what do you think of passing Optional[tuple[Collection[Coin], Collection[Coin]]] instead? That way, you explicitly pass None as additions and removals to indicate that you're not interested in the transaction filter.

One issue with adding a separate tx_filter: bool parameter is that you can pass in empty lists (because you don't care) but forget to pass in tx_filter=False.

That's a good point. Done.

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 Cleanup Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants