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-1943 Order header blocks by height in get_header_blocks_in_range #18967

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

Conversation

AmineKhaldi
Copy link
Contributor

Purpose:

This allows us to provide a start and (inclusive) end height and get header blocks in that order.

Current Behavior:

The order depends on whether we'll find blocks in the cache or fetch them from the DB.

New Behavior:

We'll maintain the order of the input heights despite fetching blocks from both cache and DB.

@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 3, 2024 16:05
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner December 3, 2024 16:05
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

why is this an improvement?
If there is a good reason to do this, we would need a test as well

@AmineKhaldi AmineKhaldi force-pushed the order_by_height_get_header_blocks_in_range branch from b1dd979 to 1ca3fe1 Compare December 11, 2024 14:27
@AmineKhaldi
Copy link
Contributor Author

why is this an improvement? If there is a good reason to do this, we would need a test as well

My understanding is that FullNodeAPI's request_block_headers returns block headers in the order of the input range, so this is done to maintain that behavior so I can leverage get_header_blocks_in_range in my plan to simplify that endpoint.

I've added a test that fails in main and passes with the PR.

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