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

Fix VM.GetBlockIDAtHeight #595

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Fix VM.GetBlockIDAtHeight #595

merged 3 commits into from
Jul 8, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Jul 7, 2024

Why this should be merged

The specification for GetBlockIDAtHeight is:

	// GetBlockIDAtHeight returns:
	// - The ID of the block that was accepted with [height].
	// - database.ErrNotFound if the [height] index is unknown.
	//
	// Note: A returned value of [database.ErrNotFound] typically means that the
	//       underlying VM was state synced and does not have access to the
	//       blockID at [height].
	GetBlockIDAtHeight(ctx context.Context, height uint64) (ids.ID, error)

It should return database.ErrNotFound if the height index is unknown. Currently this function returns potentially arbitrary blkIDs when queried for heights greater than the last accepted height.

How this works

  1. Immediately returns database.ErrNotFound for heights larger than the last accepted height.
  2. Removes the reading of the block from disk, as it is not needed to serve this request.

How this was tested

  • Added additional testing to the canonical height index.

@StephenButtolph StephenButtolph self-assigned this Jul 7, 2024
@StephenButtolph StephenButtolph changed the title Fix get block id at height Fix VM.GetBlockIDAtHeight Jul 7, 2024
Copy link
Collaborator

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

lgtm.
e2e test should pass after this one gets merged #596

@darioush darioush enabled auto-merge (squash) July 8, 2024 16:05
@darioush darioush merged commit 7684836 into master Jul 8, 2024
8 checks passed
@darioush darioush deleted the fix-get-block-id-at-height branch July 8, 2024 16:39
oxbee pushed a commit to taurusgroup/coreth that referenced this pull request Nov 6, 2024
* Fix and optimize GetBlockIDAtHeight

* Add test

---------

Co-authored-by: Darioush Jalali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants